feat: build profile review rules - #21
Merged
Merged
Conversation
The code under review is not always the code that ships. Two review rules for the gap: SAF-007 (HIGH) — arithmetic on an untrusted-input path whose outcome differs between the dev/test profile (overflow-checks on) and the shipping release profile (off by default). The profile-gated panic is the floor of the impact, not the ceiling: "doesn't reproduce in release" is not a close — a silent wrap that truncates a length or misresolves an index is worse than the panic because nothing reports it. Report both facets, and read [profile.release] before assuming the default. SAF-008 (CRITICAL) — debug_assert! as the only guard on an unsafe precondition or other load-bearing invariant; it compiles out in release, leaving the shipped binary unguarded. rust-performance already states the authoring rule, this is the review rule that catches its violation. Also wires both into the safety lens brief, the lens table, and a "what proves what" row: a panic is release-unreachable only if the repro was re-run under the shipping profile AND the release behaviour is stated.
Most findings rest on a claim that is not visible at the line they cite — "the dependency rejects this", "reachable from untrusted input", "no caller guards it". That premise is the one a review most reliably invents, and a second, smarter reading does not catch it: it reproduces the same assumption. Only opening the code does. So make it structural rather than exhortative. - FINDING_ITEM gains a required `whereChecked`: the file:line the lens actually opened, dependency sources included. Lens prompts state that an unopened premise is inadmissible — open it or drop the claim. - VERDICT_SCHEMA gains a required `premiseSupported`: a verifier must open the claimed evidence and vote on whether it shows what is claimed. - An unsupported premise demotes Confirmed to Suspected. It is never filed as refuted: unsupported is not disproven, and adversarial-review feeds its refuted list forward as "do not re-report", which would bury a possibly real defect for the rest of the run over a missing citation. - Dedup unions whereChecked across merged members, so evidence pinned by a member that loses the merge does not cost the group its tier. - triage-findings takes the symmetric half: premise discipline binds *reject* as much as accept. "A caller already validates this", waved through without opening the caller, is the same unfounded claim as the finding it dismisses — and it discards a real bug silently. Same treatment in adversarial-review; rubric sections in rust-review and nix-review; both lens agents. Tests pin the schema contract, that verifyPrompt asks for every key the verdict schema requires, and that whereChecked reaches prompts flattened with path identifiers intact.
Three additions to the Rust review rubric.
fp-rules.md — the mirror of rules.md. FP-001..FP-007 are the precedents
under which a finding is dropped; each demands a specific trace, not a
pattern match ("looks guarded" does not fire the invariant-protected rule;
following the invariant to its source and showing it dominates the sink on
every path does). A rejection is a claim and carries the same burden as the
finding it kills, so the verdict cites the ID.
Two of them are downgrades rather than refutations: operator-controlled
input and an operator-only panic surface leave the technical claim intact
and remove only the attacker's access. Killing such a finding outright on
an input that turns out to be network-reachable is the expensive mistake,
and it is invisible once the finding is gone.
KEEP-001..004 are the converse — dismissals that sound decisive and have
repeatedly killed real defects: soundness in a public API no current caller
reaches, a logic bug in safe Rust, a panic unwinding through an unsafe
region, and an unverifiable premise (that is Suspected, not refuted).
INV-005, the mirror walk: on a two-sided contract the finding IS the
asymmetry, no crash required. The error enum indexes the invariants; each
enforcement site is checked against its mirror along four axes
(client/server, send/receive, offered/accepted, one-param/all-params); a
guard present in the last released tag and gone at HEAD is a regression,
which changes severity.
SAF-009 with measured magnitude: "same class as X" is a claim about
mechanism, not severity, and the gap between two exhaustion bugs sharing a
root cause can be orders of magnitude. Rate by attack throughput against a
real-data baseline and attacker-bytes-per-victim-CPU-second, not by the
neighbour's label. Crashes have no throughput curve — rate those by
convention and say it is a judgement call.
Wiring: verifyPrompt now takes the profile so it names the catalog only for
a profile that ships one (nix does not, and a dangling file reference would
send it hunting). Tests cover that gating, that every profile declares
fpRules, and the verifyPrompt extractor is now signature-agnostic.
Closes the last tranche of practices mined from rust-in-peace. addressing-findings — three checks for when a fix is actually done, each there because a fix that passed the obvious check still shipped broken: - Every facet, not the loudest one. A bug with two observable effects (a panic and silent corruption, two profiles, two entry points) is not fixed until the case for each re-runs green. Saturating arithmetic removes the panic while the saturated value still collapses downstream, leaving the misresolution alive. - Your own check, not the fixer's. "Fixed, tests pass" is a claim; the test can construct the broken state differently from the real entry point. Point a scratch crate at the fix branch and drive the public API. - Sibling sweep. A fix can close the reported instance and leave an identical sibling untouched, invisible to the one case tested. A sibling found this way is a NEW finding with its own stable id — folding it into the one being closed reports a defect resolved while it still ships. rust-review — the control/attack differential, the proof form for findings with no crash (a silently dropped value, a message accepted that should be rejected): one variable changes between CONTROL and ATTACK, the oracle is the delta, and asserting both halves makes the reproducer a regression test a one-sided fix cannot pass. Reach the state through the crate's own test helpers; a PoC over a mock measures the mock. State what was demonstrated versus what is inferred. review.js — reachability is about the ROUTE. Reaching the state by constructing the object directly (builder, new, fixture) bypasses exactly the validation the question asks about and proves nothing about untrusted-input reachability. The trap catches careful reviewers, so the verifier now checks it explicitly. docs/LESSONS.md — lessons not derivable from the code, seeded with the two this adoption produced: a borrowed rule that was inert because craft's agents have a different shape, and an imported rule set that had to be re-routed through craft's verdict model before it could be written down.
…silent
Two defects found by running the analyzer over a 60-run store.
The NOISE rank filtered on candidate count alone, never on the refute rate
it claims to rank by. Every lens with >=4 candidates was listed under
"lenses over-refuting" with the advice "tighten this lens's rubric" —
including lenses at refute 0.00. On the store in question 13 of 13 ranked
lenses were listed, the bottom two at 0/9 and 0/5. The advice is backwards
there: tightening a lens whose findings are all confirmed suppresses real
ones. Adds a rate floor, and distinguishes "none are noisy" from "no
telemetry yet" so an empty section is not ambiguous.
loadRecords swallowed unparseable files. The same store holds a 0-byte
record from a write that died mid-flight; the only trace was the run count
not matching the file listing, and the report said nothing. A record that
fails to parse is a run whose telemetry is gone — that is damage, not a
smaller store. It now returns {records, unreadable} and the CLI reports
both the lost records and the pre-telemetry ones it filters out, so the run
total is always explainable.
Neither was pinned by a test: the render test only asserted the NOISE
header exists, and loadRecords had no coverage at all.
Both defects surfaced by reviewing two real run stores (31 and 60 runs). A dimension row is emitted for every PLANNED lens, so a lens that never returned recorded as a 0-finding row — identical to one that ran and found nothing, and its dead runs still counted in the yield denominator. That is the difference between "redundant, drop it" and "broken, fix it", and it is exactly the signal the self-improvement loop reads. Runs now carry ranLenses, dimension rows carry `ran`, and the analyzer excludes dead runs from the denominator and flags them. Records predating the flag count as having run rather than being guessed at. The api-idioms lens brief asks for repeated completeness nits to be rolled into one finding. The store shows it is not obeyed: 126 confirmed findings over 21 runs, 100 of them Low/Info, burying the ~2% of findings that actually drive the verdict. An instruction the model can quietly skip is not a cap, so rollupPool enforces it: past a threshold, occurrences of a listed low-value rule fold into one grouped finding that states the count and names locations. Only completeness nits are listed (API-001/003/004/ 005) — never a rule whose instances carry distinct risk — the worst instances stay individual, nothing is dropped, and the fold is logged. SEV_RANK moves into the declarations prefix so severity-ranking helpers are reachable from the test harness.
… gate Two things the vodopad PR run exposed. **Gate.** A diff review was reproducing CI locally: it checked port 15432, ran docker ps, and sat in a cold workspace build for hours. It did that because CI consumption required the covering check to be marked `required` — and most repos have no branch protection at all, so `isRequired` is null on every check and the shortcut was dead code. Required-ness decides whether RED blocks a merge upstream; it says nothing about whether GREEN is trustworthy. Green is now consumed regardless, with generous name matching (`cargo nextest`, `just clippy`, `ci / test (stable)`). Alongside: the gate never stands up infrastructure — a check needing a database, container or broker is CI's, recorded unknown with a reason. A review that never starts is worth far less than one with an unestablished test signal. Local commands run under `timeout`, and a timeout is an unknown signal, never a retry. Lint semantics (features, -A allows) come from the project's own recipe; scope (-p changed packages) and --message-format=short are ours, since neither changes what a lint says. **Engine identity.** Records carried `schemaVersion` (the record format) and `commit` (the *reviewed* project) but nothing identifying craft itself, so every aggregate silently averaged across rubric versions and no before/after question could be answered. Adds `craftVersion` (const, kept in sync with plugin.json by check-workflows — verified to fail on drift) and `craftCommit` (craft's HEAD, which separates two runs of one release while the rubric is being edited). Both ride in index.jsonl, since that is what a filter scans. analyze-runs grows `--version latest|<v>`, and warns when a store mixes versions instead of quietly averaging them.
…ecords Verification was 67.6% of a measured run (154 agents, 21MB of transcript on 111 findings) and scaled linearly with finding count, uncapped. Each finding now goes to the cheapest treatment that cannot change its outcome: - INDIVIDUAL — Critical/High keep the full adversarial panel, never batched, never skipped. Plus any severity carrying a SAF/ERR/CON rule: a lens UNDER-calling severity is the one real risk of routing, and those are the families where it would hurt. - BATCHED — Medium can only reach Warning, so one agent judges a group of six from one file instead of six agents. A missing index in the batch reply falls back to Suspected: a verifier losing a finding must not read as a refutation. - SKIPPED — Low/Info. No verdict on these can move Approve/Warning/Block, and Suspected is already defined as "borderline or UNVERIFIED; surfaced, never changes the verdict". Spending a skeptic to move a finding from Suspected to Suspected buys nothing. Each says on its face that it was not verified, and the routing is logged — an unverified item that reads like a verified one is the silent cap this codebase refuses elsewhere. On the measured run's mix that is ~53 fewer agents, ~20% of the whole run, with the blocking tier untouched. tierFromVotes is now shared by both paths so the batched one cannot drift from the individual one. Also: the run record was being RETYPED by a haiku agent from a JSON blob in its prompt. On a large record it dropped the big arrays — the completed vodopad review persisted `findings: 111` with `dimensions: []` and no `verification`, destroying exactly the per-lens telemetry the store exists for. It is now copied through a quoted heredoc and merged with jq, the agent verifies the written key set against the input and reports any loss, and the model scales with payload size. reviewRecord also gained the craftVersion stamp the earlier commit missed (it builds its own literal).
…ns yield
Measured on a second vodopad run (137 agents, 8.8M tokens, 316 min).
**The escape hatch ate the saving.** Keying it on the SAF/ERR/CON rule
*families* sent 88 of 137 agents — 49% of transcript volume — back into
individual verification, because those families cover unwrap, dropped
errors and every concurrency rule, i.e. most of a Rust review. The hatch
only ever belonged on the SKIPPED tier: batching is still verification, so
a Medium the lens under-called gets a real adversarial judgement either
way. It now fires only for a Low/Info finding citing a specific
block-on-sight rule — that combination is the under-call, and it is rare.
**CI was found by branch name only.** `gh pr checks` resolves the PR from
the current branch, so a review worktree (`pr-1203-review`), a detached
HEAD or a local rename all read as "no PR" while CI is green — a false
negative that costs the entire shortcut and sends the gate into a local
build. The second run hit exactly this. It now falls back to resolving the
PR by HEAD commit via `repos/{owner}/{repo}/commits/{sha}/pulls`.
**Lens rounds are instrumented, not cut.** Lenses are the other half of the
cost (182 min, 31 agents) and every round re-runs every lens over the whole
diff — but whether round 2 earns that cannot be recovered from a finished
run: the gate's seed findings make the pool non-empty from round 1, so no
transcript can be split by round. Records now carry
`lensRounds[{round, agents, returned, newFindings}]`. A maxRounds cut
should be argued from those numbers, not guessed at.
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
1 participant
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.
No description provided.