Check the Solidity toolchain rule against the run's own trace - #205
Conversation
`sol-toolchain` (#195) gave the producer a way to ask which toolchain a checkout's own CI judges it with. Nothing read the answer back, so the rule about it could not report its own violation — which is how #116 was found, by a parked PR whose one back-off attempt had gone on a diff that could never pass. `sol-toolchain-audit <trace>` reads the run's own record: the `sol-toolchain` call and its output sit in the trace next to the `nix develop` lines that came after, so the comparison needs no network read and cannot stop a run. Closes #203
…path arm Splitting on `&&`/`|` already reaches inside the quotes of a CHAINED wrapped command, so a test using one passes with the unwrapping deleted. A wrapped command with no shell operator is what the unwrapping is for. The canonical-path equivalence arm had no test at all: the existing repo-flake case matches on the first arm, because the answer and the question spelled the checkout the same way.
|
Warning Review limit reached
Next review available in: 53 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Reviewed e5bc8f4: ready Verified before merge: 20 SUCCESS / 1 skipped / 0 failures, MERGEABLE/CLEAN, 0 unresolved threads. 999 tests from a 981 baseline; 17 mutations. CodeRabbit reviewed and passed. The measurement is what chose the option. 197 Solidity checks across 23 work clones in 5 runs — tens per run, not per-file — and of the 189 naming a resolvable checkout, 170 ran in a shell that checkout's own CI does not run. Live, not notional: rainix HEAD is 37 commits and 20 days ahead of the RAINIX_SHA every rainix-pin repo's CI actually uses. The caveat it raised is the part I would keep. #195 merged at 08:03Z today, so every retained trace predates it and all 189 report Detection over a blocking hook, decided on shape rather than size: a hook would pay 73 toolchain resolutions on the busiest run, each two GitHub reads out of a reusable at a floating ref, on the critical path of every Bash call — to compute something ALREADY RECORDED IN THE TRACE. The I checked the two things that would have made this wrong. The exit code cannot contaminate a run: it is Two of its 17 mutations survived first pass and both were REAL gaps rather than equivalents: a Worth knowing on the first runs after this: exit 3 also fires on |
sol-toolchain(#195) gave the producer a way to ASK which toolchain acheckout's own CI judges it with. Nothing read the answer back. A rule with no
check cannot report its own violation, so the next wrong rule would be found the
way this one was — by a parked PR whose one permitted back-off attempt had gone
on a diff that could never pass.
Worth being precise, because the framing is easy to get wrong: the prompt was
not violated in #116. It said
github:rainlanguage/rainix#sol-shelland theproducer ran exactly that. The rule was wrong, not the compliance. So this is not
"prose does not bind"; it is that nothing could tell us the rule was wrong until
a PR parked.
The measurement, which is what chose the option
The issue asks for the exposure before the choice. Over the 21 retained
traces (4,574 Bash calls, 508
nix develop/run/shellinvocations):is_sol_check, so noforge soldeer install)forge --versionhealth checkReproduce the first three rows with the subcommand this PR adds:
Every retained trace predates #195 (it merged today), so none of them ever
called
sol-toolchainand all 189 resolvable checks reportunasked— thehonest verdict for a run that had no way to ask. The 170 figure is the same
corpus re-audited with each clone's live
sol-toolchainanswer injected ahead ofit, which is what turns
unaskedinto a comparison.The skew is not notional. Every rainix-pin repo's CI runs
RAINIX_SHA53e96a7d(2026-07-10); thegithub:rainlanguage/rainix#sol-shellthose runsused floats on a rainix HEAD 37 commits and 20 days ahead of it. 9 of the
mismatches are the mirrored mistake — the checkout's OWN flake used against a
repo whose CI reads the reusable's pin and never its flake.lock. Broken down by
the audit itself: 159 unpinned-rainix-HEAD against a reusable pin, 9 mirrored,
2 other.
So "nothing, with a number" is out: 170 checks is not a small exposure. But
the shape — tens per run, concentrated in a handful of clones — is what argues
against the blocking hook.
Why the audit and not the hook
the busiest run, each a
RAINIX_SHAread out of the reusable at a floatingref (two GitHub reads apiece), on the critical path of every
nix develop.The audit resolves nothing: the answer and the
nix developthat followed itare both already recorded in the trace.
the run — so the guard is absent exactly when the network is flaky. An audit
computed from a file after the fact cannot be lost that way.
same problem and reports it as a blind spot instead of guessing: 8 of 197
named no checkout, and every one of those was a
forge --versionhealth checkrather than a check on a diff.
distill-trace,run-timings,run-metricsare all post-run trace readers, and CLAUDE.md's north star putsparsing in the binary.
What it gives up is real and is stated in the code: it does not prevent the
wasted attempt inside the run it audits. It moves discovery of the next wrong
rule from a parked PR weeks later to the end of the run that obeyed it.
foreignis deliberately outside the judgment. A foreign checkout's toolchain isone no
nix developenters, so there was never a shell to match; it reports asunmatchableand changes no exit code, which is what #195's own text alreadytells the producer to expect.
What landed
pr-review-report sol-toolchain-audit <trace>— per Solidity check:matched/skew/unasked/unmatchable, plus the audit's own two blindspots (names no checkout; enters the working directory's flake without naming
that directory). Exit 3 on skew or unasked only — failing a run on what the
checker cannot see is the fail-closed direction, and a checker that stops a run
on its own ignorance is worse than the skew it hunts.
campaign-run.shruns it afterrun-metricsand appends the report to the runlog. Best-effort, like every line around it: a skew is something to READ at the
end of a run, not a reason to fail a run whose PRs are already open.
row.
The classification reuses
is_sol_check— #195's own definition of a check whoseverdict depends on the shell — so
forge soldeer installis excluded in bothplaces by the same predicate. What is new is finding the commands to ask it
about in a line a RUN writes rather than a line a workflow writes:
&&chains,pipelines,
bash -c '…',env VAR=V …, and severalnix develops per line. 9of the 241 Solidity invocations in the corpus are
bash -cwrapped and 12 carrymore than one shell.
QA
a_check_run_in_the_named_shell_matches,a_check_run_in_a_different_shell_is_skew,an_answer_does_not_reach_backwards_to_a_check_that_preceded_it,a_check_in_a_checkout_never_asked_about_is_unasked,a_foreign_checkout_is_recorded_not_judged,a_dependency_fetch_is_not_a_check,a_check_wrapped_in_bash_c_is_still_a_check,each_invocation_on_one_line_is_classified_separately,root_names_the_checkout_over_the_working_directory,the_repo_own_flake_entered_by_cd_matches_its_repo_flake_answer,the_audits_blind_spots_are_named_and_do_not_fail_the_run,skew_and_unasked_both_fail_the_audit,the_audits_own_invocation_is_not_a_toolchain_question,an_answer_with_no_verify_line_carries_its_mode,the_outer_command_flag_wins_over_a_wrapped_one,only_an_absolute_working_directory_names_a_checkout,a_repo_flake_answer_matches_the_checkout_however_the_question_spelled_it,a_trace_yields_its_bash_calls_in_issue_order_with_their_output— each failson base (every one names a function this PR adds, so on
9c07414the moduledoes not compile; each was then verified against a real mutation of the line it
is about, below)
src/main.rs, one per behaviour a new test claims; the two that survived thefirst pass are recorded as survivors and the tests they should have killed were
strengthened/added before re-running — both then killed)
classify_sol_verifymatched arm*named == used→!=→a_check_run_in_the_named_shell_matchesclassify_sol_verifyskew arm → returnsMatched→a_check_run_in_a_different_shell_is_skewsol_auditanswer collection → hoisted to a pre-pass, so answers reach backwards →an_answer_does_not_reach_backwards_to_a_check_that_preceded_itclassify_sol_verifyNone => Unasked→None => Matched→a_check_in_a_checkout_never_asked_about_is_unaskedclassify_sol_verifySome(Err(mode)) => Unmatchable→Skew→a_foreign_checkout_is_recorded_not_judgedis_sol_checkforge soldeerearly-return deleted →a_dependency_fetch_is_not_a_checkruns_sol_checkbash -c/sh -cunwrap deleted → SURVIVED the first pass (the test's only case was a chained command, and&&/|splitting already reaches inside the quotes); test strengthened in place with an unchainedbash -c 'forge fmt --check'→ nowa_check_wrapped_in_bash_c_is_still_a_checksol_auditinvocation loop →.take(1), only the first shell on a line →each_invocation_on_one_line_is_classified_separatelysol_verify_target→shell_cwdchecked before--root→root_names_the_checkout_over_the_working_directoryclassify_sol_verifycanonical-path equivalence arm deleted → SURVIVED the first pass (the existing repo-flake test matches on the first arm, because answer and question spelled the checkout identically); new test added → nowa_repo_flake_answer_matches_the_checkout_however_the_question_spelled_itsol_audit_linesexit code → countsUntargetedtoo →the_audits_blind_spots_are_named_and_do_not_fail_the_runsol_audit_linesexit code →skew > 0only, droppingunasked→skew_and_unasked_both_fail_the_auditsol_toolchain_question→starts_with("sol-toolchain")instead of==→the_audits_own_invocation_is_not_a_toolchain_questionsol_toolchain_answer→Err(String::new()), mode dropped →an_answer_with_no_verify_line_carries_its_modenix_develop_inner→.next()instead of.min_by_key(offset), so an inner-cbeats an outer--command→the_outer_command_flag_wins_over_a_wrapped_oneshell_cwd→d.starts_with('/')guard deleted →only_an_absolute_working_directory_names_a_checkoutsol_audit_calls→ recorded output not paired onto its call →a_trace_yields_its_bash_calls_in_issue_order_with_their_outputruns/*.jsonlproducer traces, read as data. Expectedtoolchains come from each repo's OWN
.github/workflowsassol-toolchainreads them, and the pin from
RAINIX_SHAin the rainix reusable — never fromthis code. The command shapes in the tests (
--root,cd … &&,env -C,bash -c '…', two shells on one line) are transcribed from commands runsactually issued, not invented.
verification commands per run, across how many clones, and how many used a
toolchain
sol-toolchainwould not have named; (b) choose among blocking hook/ after-the-fact detection / removing the second place to get it wrong /
nothing-with-a-number, on that evidence; (c) if a guard is built, say what it
does when the answer is unresolvable. Covered a, b, c.
Closes #203