feat: a review arm is scored against dispositions this repository already wrote (#108, #109) - #120
Merged
Merged
Conversation
…eady wrote (#108, #109) Issue #109 asks which of two review arms finds more real defects per thousand output tokens, and what the compressed one drops. Neither half has an answer without a record of which findings were real, and a model's own `confirmed` label is not one: grading an arm on its own claim measures its confidence. This repository already holds the answer. AGENTS.md disposes of every review finding with a fenced `review-verdict` block, written by a person who read the code. Issue #108 mines those blocks and collects nothing new. A record retains the thread and states no disposition. `bench/verdicts.mjs` derives one, and `check:verdicts` prints what it derived and joins the gate as a ninth check. Two readings come off one thread and each withholds on its own cause, so a thread nobody answered and one anchored on the left of the diff say different things rather than both reading as broken. A scenario is one review ROUND, and the corpus pins the commit that reviewer read. The merged diff was the obvious choice and it is wrong twice over: every accepted defect is fixed in it, and the anchors point at lines that moved. Pinning the reviewed commit removes all version drift from the matching rule. `score.mjs --review` adds five columns, and only under that flag. Both counts are BOUNDS: a match is a path and a line within ten lines, so two findings close together in one file are not separable. `confirmed` is a ceiling and `missed` is a floor. A promoted study retains the corpus it scored against, because the re-run refuses a path outside the study. Nothing here spends a model call. `bench/review-arms.mjs` rebuilds each diff, prints what it would cost, and prints the two invocations rather than running them. A run names its pull requests with `--pr`, because eligibility says what the corpus may hold and only the operator decides what to buy. Three of five mined rounds refuse promotion's content scan, because the diffs under study are this repository's own and carry that scan's own test fixtures. The scan is right and the corpus is smaller. ADR-0032 records the corpus rule, the matching rule and its failure mode, the scorer cells, where the token count comes from and that it is unverified, and two deviations from the shapes issue #109 sketched.
github-actions
Bot
added
documentation
The Windows jobs went red on the review-study test, and the cause reaches further than the test. `bench/retain.mjs` built all three path arguments of the retained scorer command with `path.relative`, which spells them `bench\\samples\\...` on win32. Two of the three predate this branch, and the new test is simply the first thing that ever asserted the spelling. That spelling travels. Measured on darwin against a Windows-spelled command, `commandProblems` answers `names s\\verdicts, which is not inside this study.` — so a study promoted on Windows cannot be re-run anywhere else, a refused study is never re-run at all, and the message names containment rather than the separator. `commandPath` in `bench/study.mjs` now states every command path with `/`, and `commandProblems` refuses the other spelling by name, before containment, so the check gives one verdict on every platform. The writer and the reader of the rule sit in one file. `bench/review-arms.mjs` prints its plan through the same helper, because those lines are pasted into zsh. Nothing else a record retains carried the wrong spelling: arm manifest keys come from `walk`, which already joins with `/`, the study manifest's own path fields are literal templates, and a probe or verdict record retains no filesystem path. The new assertion is platform-independent — no argument of a retained command may carry a backslash — so it is red on Windows before this fix rather than only after CI says so.
![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: 752461418c
ℹ️ 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".
Seven review findings, each reproduced before it was adopted.
**A thread names its round's reviewed commit.** A round IS a reviewed commit, and
`threadProblems` never compared the two — so a hand-edited record could anchor a
disposition in a tree no arm reads, and both derived counts would describe the
wrong file. `null` is refused there for the same reason.
**One pull request labels a scenario once.** Two copies of a valid record both
passed, and `loadCorpus` appended both. Measured: `matchDispositions` deduplicates
by thread identifier while `missed` counted array entries, so a duplicated corpus
read `{confirmed:1, missed:1}` where the truth is `{confirmed:1, missed:0}` —
inflating the counterweight, the direction that makes the compressed arm look
worse. Both halves ship: `corpusProblems` refuses the duplicate and binds the
filename to `identity.pr`, and `reviewMetrics` counts distinct dispositions so
the invariant holds whatever it is handed.
**A fence is indented at most three spaces.** `micromark` settles it: a
four-space `review-verdict` example renders as `<pre><code>` with its backticks
showing, while this reader read it as a block — and the last-block rule then made
the EXAMPLE the current verdict over a real `ACCEPTED` above it. That is
`row-indented` in a third place, at the parser's own bound rather than a house
guess, pinned in `test/gfm-render.test.js` by ADR-0028's rule.
**Chronology comes from forge identifiers.** `verdictOf` read array order, so a
reordered record made an older disposition current. It sorts by reply id now, and
an out-of-order record is refused as well, because the collector always writes
them sorted.
**A corpus entry is a plain file**, asked through `destinationState` rather than a
fourth spelling of the same lstat question. A symbolic link was followed by
`readFile` and passed as an ordinary record, and `review-arms.mjs` would have
selected commits from bytes outside the corpus.
**A token value the collector could not have written is refused.** `garbage`,
`-1` and `Infinity` all withheld the primary figure exactly as the supported
`absent` does, while the run still read audited. ADR-0024's split: `absent` is a
protocol spelling and decides a reading, and an impossible value refuses. `0`
stays valid and still withholds the rate.
**Every arm and scenario directory carries its selection.** This one corrected a
claim I had written into the ADR. It said `armState` catches a second selection;
traced, it does not — every run wrote into ONE prompt directory, so after
`--pr 112` then `--pr 118` that directory holds both scenarios, `run.sh` derives
its plan from all of them, and the arm covers that larger plan legitimately.
Nothing is ever unexpected, and promotion would have produced a study larger than
the selection. A run now writes `review-prompts/<tag>` and plans
`review-baseline-<tag>`, so the same selection still resumes and a different one
cannot reach it. ADR-0032, bench/README.md and the runner comment all carry the
correction rather than the claim.
Self-review of the batch caught four more: the miner carried a second literal for
a filename the reader now binds, `buildScenarios` skipped the set-level check it
shares with the corpus readers, and two callers reported a symbolic link as
unreadable JSON.
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.
Closes #108. Builds the runner and the scorer support for #109, which stays open
because the arms have not run.
What this is
#109 asks which of two review arms finds more real defects per thousand output
tokens, and what the compressed one drops. Neither half has an answer without a
record of which findings were real, and a model's own
confirmedlabel is notone.
This repository already holds the answer. AGENTS.md disposes of every review
finding with a fenced
review-verdictblock. #108 mines those, and collectsnothing new.
No metered call runs anywhere in this change.
bench/review-arms.mjsrebuilds each diff, prints what an arm would cost, and prints the two
invocations rather than running them. The operator runs the arms after merge.
The design decisions, and the ADR
All three live in
ADR-0032.
The corpus. A pull request is ELIGIBLE when the forge merged it and at least
one thread derives a disposition. Eligibility is not selection: a run names its
pull requests with
--pr, and a number the corpus does not hold is refusedrather than skipped. A SCENARIO is one review round, and the record pins the
commit that reviewer read. The merged diff was the obvious choice and it is
wrong twice over — every accepted defect is fixed in it, so the ground truth is
not there to find, and the anchors point at lines that moved.
The matching rule, and its failure mode. An arm's finding matches when the
file paths are equal and the arm's line falls within ten lines of the mined
anchor. The instrument reads both arms the same way, by collecting every
distinct
path:linethe reply names, because the baseline fixes no shape and aparser for the treatment's shape would measure the two arms with two
instruments.
The window makes the match many to one. Two confirmed findings less than twenty
lines apart in one file are not separable. PR #118 is that case, and
test/review-study.test.jsmeasures it: one stated line reaches three of itsfive confirmed findings. So
confirmedis a ceiling on agreement andmissedis a floor on what was dropped, and no document here states eitheras a count. Two further limits: a finding naming a file and no line states no
anchor, and a path with no extension is outside the form.
The scorer cells, and the token source.
bench/score.mjs --reviewaddsanchors,confirmed,missed,outTokensandperKtok, and only under thatflag.
check:studiesderives one figure per cell of that table, unchanged.The token count comes from
modelUsage[<build>].outputTokensinclaude -p --output-format json, withoutput_tokensas a second spelling.bench/extract.mjshas read that field since it was written, which is how therunner picks the build that answered. It is not verified under a review
invocation, and verifying it costs a metered call, so the protocol carries the
absence:
extract.mjswritesabsentrather than a zero,reviewMetricswithholds
perKtokrather than dividing, and a withheld cell derives no figure.--reviewrequires the sidecar FIELD and admitsabsentas its VALUE, which isADR-0024's split.
The counterweight #109 asks for is the difference between the two arms'
missedrows. It is deliberately not a cell: a cell would have to pick which baseline
sample to subtract, and would hide that choice inside one number.
Two deviations, recorded
The treatment reaches the model as an appended system prompt, not as the user
prompt #109's own invocation shape shows. The scorer requires both arms to share
a prompt digest, so delivering the contract inside the prompt would make the
arms incomparable by this bench's own rule.
bench/README.mdalready stateswhat that channel costs.
The review arms run through
bench/run.sh, with a new--promptsflag, ratherthan a second runner. A second runner would be a second copy of every refusal
that file carries.
What the corpus actually holds
Four eligible pull requests, thirteen threads, thirteen confirmed findings. The
earlier pull requests here were disposed of in bold prose rather than a fenced
block, and the reader reads one form.
Three of the five mined rounds refuse promotion's content scan, and the cause is
worth stating: the diffs under study are this repository's own, and they carry
that scan's own test fixtures —
sk-ant-oat01-LEAKEDCREDENTIAL0123in one test,/Users/someone/in another. The scan is right and the corpus is smaller. Thescan is not narrowed here.
So a clone can build two pull requests today, and the first run is
operator-scoped to three. That shortfall is a corpus fact, not a tool fault.
Two exits stay open: a later pull request disposed of in fenced blocks becomes
eligible by being mined, and redaction — which the measurement design names and
nothing builds — returns the three refused rounds.
What remains
node bench/review-arms.mjs --pr ... --write, then the twobench/run.shlines it prints. A person does this and pays for it.bench/retain.mjs, with--promptsand
--verdicts.bench-study:marker and noeditorial stamp or audit date is written by this change.
Checks
npm run checkpasses, exit 0. It is nine checks now:check:verdictsjoinsthe gate and the CI workflow together. 888 tests pass.
bench/run.shis exercised end to end against a stand-in harness rather thanonly read, because CI never runs it — the
--promptsflag, the extractor's twofields, and the
output_tokenssidecar entry were all watched working.