Repository navigation
ruff_charon: fix prek and the four Codex findings from #129 and #130 - #131
Merged
Merged
Conversation
- typos: the base64 test's `b"fo"` reads as a misspelling; use a/ab/abc. - markdownlint MD040: the test-data README's command block is now a fenced `sh` block. - dangling(): a call resolved through a known trait impl names `<trait> via TraitImpl`, never a function, so a consistent store reported it as dangling. Such rows are no longer checked; function targets still are. - Store::replace: every write goes through its own temporary file (create_new, pid + counter), so concurrent imports of one crate cannot publish each other's half-written bytes or rename each other's file away. Tests: trait_impl_calls_are_not_dangling (a TraitImpl row is not dangling, a call to an undefined function still is); concurrent_imports_of_one_crate_do_not_collide (10 rounds of 8 threads, rows equal a clean import, no temp files left). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6y3drwKSE2zSgoexheLFX
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42bd8e32a8
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…129 review) Two Codex findings on #129: - A call through `TraitRefKind::TraitImpl` was a CALLS_RESOLVED row naming only the trait, so two impls of one trait gave one target, the row matched no DEFINES, and the Kythe ref/call edge pointed at the trait. It now names the function it resolves to: the impl's own method from `TraitImpl.methods`, else the trait's default method. An impl or method missing from the LLBC is a TRAIT_IMPL_METHOD_NOT_FOUND residual, never a row pointing at the trait. - A local function without a structured body was harvested once but represented by its DEFINES row and also counted as a residual, so the receipt could not conserve and `rows` exited 3. The body is now harvested as its own item. Tests: a_call_through_a_known_impl_names_the_resolved_function (real LLBC of a one-file crate, calls rewritten into TraitImpl calls: impl method, trait default, and a missing impl); an_opaque_body_keeps_the_receipt_conserved. Charon emits no TraitImpl calls for any source tried here (fixture, mask-risc, method/qualified/default-method calls), hence the rewrite. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6y3drwKSE2zSgoexheLFX
…moved The fixture's impl lists the inherited default method itself, so the fallback was untested (a disable run stayed green). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6y3drwKSE2zSgoexheLFX
AdaWorldAPI
changed the title
A restarted process can get the same id with its counter back at zero, so an exclusive create hit a leftover and aborted the import. Try the next counter on AlreadyExists; creation stays exclusive. A failed write removes its own temporary file. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X6y3drwKSE2zSgoexheLFX
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This follows #129 and #130. Both merged with a red
prekjob, and each had two Codex findings. This PR fixes all of them.prek (
maincurrently fails it on two lines this crate introduced):b"fo", which typos reads as a misspelling of "of"/"for". The vectors are nowa/ab/abc.shblock, and the README passes mdformat (with the hook's plugins) and markdownlint.#129, Codex 1: an opaque body is counted twice in the receipt. A local function without a structured body was harvested once but counted as both a
DEFINESrow and a residual. The receipt could not conserve, sorowsexited 3. The body is now harvested as its own item.#129, Codex 2: a call through a known impl named only the trait. Two impls of one trait gave the same target, the row matched no
DEFINES, and the Kythe call edge pointed at the trait. The row now names the function the call resolves to:TraitImpl.methods;An impl or method missing from the LLBC becomes a
TRAIT_IMPL_METHOD_NOT_FOUNDresidual.#130, Codex 1:
dangling()reported trait-impl rows. After the fix above, noCALLS_RESOLVEDrow names a trait. The check also skips<trait> via …objects outright, so calls to functions are still checked.#130, Codex 2: concurrent imports shared one temporary file. Every artifact write now goes through its own
create_newtemporary file (process id + counter) before the rename.Test Plan
a_call_through_a_known_impl_names_the_resolved_function. It loads real LLBC of a one-file crate (tests/data/trprobe.rs, one trait with a default method, one impl) and rewrites three calls intoTraitImplcalls. It checks three outcomes:It then removes the default from the impl's table, to reach the trait-default fallback. Charon itself emits no
TraitImplcalls for any source tried: the fixture,mask-risc, and method, qualified and default-method calls. That is why the calls are rewritten.an_opaque_body_keeps_the_receipt_conserved: the receipt conserves with one opaque body, which is harvested as a separate item.trait_impl_calls_are_not_dangling: a trait row is not dangling, while a call to an undefined function in the same store still is.concurrent_imports_of_one_crate_do_not_collide: 10 rounds of 8 threads importing one crate. Every import succeeds, the rows equal a clean import, and no temporary files are left.Disable runs, each failing its own test:
danglingagain;Run 3 first stayed green: Charon lists inherited defaults in the impl's own table. The test now removes that entry to reach the fallback.
The crate passes (3 unit + 8 intake + 8 store); clippy
-D warningsand fmt are clean. typos, markdownlint and mdformat on the changed files are clean. Fullprekdoes not run in this sandbox.🤖 Generated with Claude Code
https://claude.ai/code/session_01X6y3drwKSE2zSgoexheLFX