feat(review): ownership retires, three lenses move to an optional pass - #70
Merged
Merged
Conversation
…an opt-in pass
The lens roster was paying for lenses the measurements do not support. Across
three independent runs of the budget-deterministic engine (large Rust diff, 14
lenses x 3 rounds, 215 findings) nine lenses produced High findings and five
produced none; the whole run returned 19 High against 118 Low/Info.
`ownership` is retired outright: no High finding in any of the three runs,
against hundreds of suspicions verification would not confirm. It leaves
`profile.lenses` and `lensBrief`, and the scout rules stop naming it — a roster
entry left behind turns into a silent nothing. The `OWN-*` rules stay in the
rust-review rubric: a standalone whole-diff reviewer still applies them.
`performance`, `api-idioms` and `api-boundary` become an OPTIONAL PASS. They
earn nothing by severity but produce hundreds of confirmed Medium-and-below
findings, so deleting them would make the review worse while paying for them
every run is what the numbers refuse.
How the optional pass works:
- OFF by default. One gate, `admitted()`, sits under every path into the plan —
the scout's own picks and `blanket()`, and therefore the security-sensitive
floor and the dead-scout fallback that both call it.
- Requested with `optional=true` / `--optional` / `optional=all`, or by name
(`optional=performance,api-boundary`). The request ADDS the lenses rather than
merely permitting them; an unrecognised name is refused out loud, since one
that quietly buys nothing is the silence this change exists to remove.
- Its absence is VISIBLE. Every report the engine can return carries a
"Not looked at" section naming the skipped lenses, appended mechanically by
`out()` — never asked of the synthesis model, which can die. The run record
carries `optionalPass: {requested, ran, skipped}`, so a cheap run cannot read
later as a full one. The section renders only when something was skipped: a
marker present on every run is one people stop reading.
- Strict mode and the security floor move it in NEITHER direction, and the code
says why: `strict` is the maintainability bar and must not come to mean two
things; a floor is a statement of ignorance about the diff, which is no reason
to buy the three lenses that measured worst. Neither may disable an explicit
request either. The request is the only switch.
Executed, not asserted: lib/review-optional-lenses.test.mjs drives the whole
engine through `runEngine` and pins every path — ownership unreachable via the
scout, the floor, the fallback and strict; the three optional lenses absent by
default and present on request; the skipped set reaching both the report and the
filed record.
…nd the tally is derived from dispatch The critic composed its follow-ups from `profile.lenses.filter(l => !plan.lenses.includes(l))` — by construction the entire optional set — and filtered only by that candidate list, never by `admitted()`. An ordinary large diff therefore bought two of the three lenses the optional pass exists to stop paying for, through a fourth road into the plan that the gate did not sit under. The boundary this project drew is "the scout classifies, the code budgets": the critic is the same model, and a side door for it is the same spend back in model hands. It is gated now. The signal is not discarded — an optional lens the critic names as an uncovered surface reaches the report and the run record, with the explicit `optional=` request that buys it. `optionalRan`/`optionalSkipped` were snapshotted off the plan the moment it was built, which is before the critic composes anything: the report printed "these lenses were NOT dispatched" over lenses whose findings were in the same report, and the record carried the same untruth into analyze-runs. The tally is now derived — the universe from the planner, the dispatches from `runLens`, the single point every lens on every path goes through — so report, record and what actually ran cannot disagree, whatever road is added next. The test preamble claimed the optional set was unreachable on every path while listing four and driving none of them to the critic. It names the fifth now, and the suite reaches it.
A cold review caught the consumer-facing text claiming "measured over three independent runs" for all three optional lenses. Only `ownership` — retired, not part of this trio — has three independent counts. The two earlier store-wide counts named api-idioms and performance among the bottom four; `api-boundary` was not in them at all. For the composition of THIS trio the basis is ONE run: one diff, one repository, one domain. The realm node that carries the measurement requires that limit to be stated rather than dropped, and the report reaches a consumer who would otherwise read a stronger basis than exists — and therefore see no reason to buy the pass.
# Conflicts: # workflows/review.js
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.
The first measurement of the new deterministic-budget engine, on a large Rust diff: 14 lenses × 3 rounds, 215 findings. Nine lenses produced the high tier — tests 3, reconciler 3, compat 3, intent 2, safety 2, invariants 2, negative-space 2, errors 1, concurrency 1, nineteen in all. Five produced none: maintainability (22 findings, 14 Low/Info), ownership (19, of which 18 Low/Info and 1 Medium), performance (15, 12 Low/Info), api-idioms (14, 10), api-boundary (11, 7). The output as a whole: 19 high-tier against 118 Low/Info.
The decision is different for each, which is the substance of it.
ownershipretires. The only one that can be called not worth its place without a caveat: no High finding in any of three independent counts, against hundreds of suspicions verification does not confirm. Its rubric rules stay in the reference — the single-pass agent and neighbouring lenses apply them, and the lens registry lives only in this engine, so the two deliveries do not diverge.performance,api-idioms,api-boundarymove to an optional pass. They do not earn on severity, but they produce hundreds of confirmed Medium and Low findings: deleting them is wrong, and so is paying for them on every run.maintainabilitystays in the main fan-out. The most precise lens of the profile by refutation rate; removing it to improve a metric would worsen the review. It is the lens strict mode exists for.The device
One gate under every road into the plan, not three separate checks — the security floor, the empty fallback and a dead scout's conservative plan all travel through one top-up. The request adds the lenses rather than merely permitting them: on a small diff with no floor, "permit" would have meant "nothing happened". An unrecognised name refuses loudly and buys nothing.
The absence is visible, which is the requirement the device exists for: the section is appended inside
out(), so it reaches every report — the synthesised one, the mechanical fallback and the early exits — and does not depend on a synthesis model that can die. It renders only when something was skipped, because a marker that fires on every run stops being read. Plus a field in the run record.The critic may not buy the optional pass. The first cold review found it doing exactly that — taking candidates from "lenses not in the plan", which by construction is the optional set, bypassing the gate. So on large or security-floored diffs the saving never happened, and the report printed "not dispatched" over lenses that were. The same boundary applies here as everywhere: the scout classifies, the code budgets; the critic is the same model, and its opinion on one run does not overturn a decision taken on measurements. The signal is not lost — what the critic names reaches the report as uncovered, with how to buy it.
And the tally is no longer a snapshot of the plan: what was skipped is derived by subtracting what was actually dispatched, marked at the single point every road passes through. A snapshot would have lied at the next door too, not only at this one.
Deliberately not done
Skipping the optional pass does not make the verdict INCOMPLETE. That marker means "this run went badly, re-run it"; firing it on every ordinary run is precisely the mechanism by which "not covered" comes to read as "clean". The distinction stays mechanical: the section plus the record field, against the lens being present in the plan.
Verification
CI gates prove form, not behaviour. Each change was closed by execution — reverted, the test watched to fail, restored. 27 tests over the engine driven end to end, asserting on actual dispatch labels and the filed record, including a premise test that the critic really was dispatched (the hole the previous review round stood on).
Two rounds of cold review; the second returned merge, no blockers, having walked all nine writes into the plan and all three lens dispatch sites to show there is no fifth road.
A finding from that round is fixed in the last commit: the consumer-facing text claimed "three independent runs" for all three lenses. Only the retired
ownershiphas three; for this trio the basis is one run — one diff, one repository, one domain. The text now says that, so nobody reads a stronger basis than exists.Local gate: 628 tests in scope,
check-workflows,check-skills,check-delivery-parity,check-evals, eslint — all zero.Knowingly open
optional=name refuses only into the run log; the typo does not reach the report or the record, so such a run later reads as an ordinary default run.