Repository navigation
Re-review synthesis of overlapping findings #116
Description
reReview in src/rereview.ts now posts correctness and security findings side by side with no synthesis, so both can post a thread on the same problem; measure how often it happens in the evals and merge duplicates (same file, nearby line, same problem) if it does.
Activity
polewskm commentedon Oct 31, 2026
Plan snapshot. The working copy is
PLAN.mdin the plan's checkout; this comment is not updated as steps run.Plan: Re-review refutation wording, duplicate merge, eval cases (9 steps)
Plan: Re-review refutation wording, duplicate merge, eval cases
Issue: #114 #116 #117
Goal
A summary no longer says verification "couldn't settle" a finding it refuted but kept as a question (#114). A re-review posts a problem
correctnessandsecurityboth report once, at the higher severity, naming both reviewers, and logs how often the two overlap so the evals can measure it (#116). The evals gain one re-review case whose changes touch a file the selection floor gives tosecurityand one whose changes touch asensitive:path, which a case can now set, plus are-reviewsubset to run the four re-review cases and read their cost per case (#117).Context for the executor
npm run typechecknpm test(one file:node --test test/<name>.test.ts; one test: add--test-name-pattern "<regex>")node file.ts), stricttsconfig.json. Tests usenode:testandnode:assert/strict, one behavior per test, hermetic.plural(n, word)insrc/rereview.tsfor counts.evals/cases.jsonuses 2-space JSON indentation.applyVerificationsinsrc/verify.tslowers a finding's severity beforeverifyPostedinsrc/rereview.tssees it, so a re-review tells a blocker it made a question (now a should-fix, posted) from a should-fix it made a question (now a nit, not posted) only by counts.test/rereview.test.ts'spullRequestRepo()makesa.tswith lines 20 and 51 changed by the pull request; a head that appends'more'adds line 62, so a refutation patha.ts:62cites only added lines anda.ts:1cites unchanged code.evals/run.ts,evals/cases.ts(a field afterauthorAssociation),test/smoke-eval.test.ts,test/engine.test.ts,README.mdandCHANGELOG.md: touch only the lines each step names.Decisions
demotedinsrc/verify.tscounts every finding a call refuted with a reason that verification kept as a question: a blocker or security finding (never dropped), and one whose refutation cites no line of the checkout outside the lines the pull request adds. Before, the second kind counted as "couldn't settle". A newdemotedBlockerscounts the blockers among them, which a re-review needs to tell posted questions from dropped ones.sameProblemholds: same file, lines withinrepeatDistance(3) of each other, and the same title words asfingerprintcompares them, severity aside. The more severe is kept (the more confident on a tie, then the first), with both reviewers named, the kept one's first. Same-reviewer findings never merge. This is the publish step's existing notion of a repeat (repeatsinsrc/threads.ts), minus its same-severity-nearby rule, which could fold two different problems into one thread.mergedandnear(a finding kept apart though within 3 lines of another reviewer's), so an eval run shows how often they overlap; the evals' output already records each posted finding's reviewers. Merging near-but-differently-titled findings waits for that evidence (follow-up).sensitive:, soEvalCasegains an optionalsensitivelist that the eval runner puts on the gathered pull request in place of the base branch's. The new field goes afterearlier, away from the field another open pull request adds.re-review-security-flooris Never post a finding that says a changed file isn't changed #71 round 3 (a7a885e since 0c9103c;src/claims.tschanged, which the floor gives to security; correctness and security on Sonnet), with a must-not concern the author declined.re-review-sensitive-pathis Fetch the pull request's head in the engine, so a calling workflow needs no checkout of untrusted code #19 round 3 (1516681 since 0848176;sensitive: ["src/fetch-head.ts"]; correctness and security on Opus, deep), expecting the fetch-timeout should-fix Follow up the engine fetch's review: a fetch time limit, a testable skip decision, sturdier tests #22 later fixed. Checked withsinceLastReviewandreReviewRosteron this repository. Each carries only its last round's findings (unresolved, as a re-review checks them) to keep the cases small; earlier rounds were all resolved.costUsdandreviewCostUsd; running the paid evals and recording the numbers stays manual (follow-up). Are-reviewsubset inevals/cases.tsruns the four re-review cases.Out of scope
evals/baseline-smoke.jsonor thesmokesubset.evals/run.tsbeyond the one line step 5 adds, and tosrc/publish.ts,src/log.ts,src/writers.ts,src/gather.ts.CHANGELOG.md.Steps
[ ] 1. Count refuted-but-kept findings apart in verification's note
src/verify.ts(edit),test/verify.test.ts(edit),README.md(edit)applyVerifications, in thev.verdict === 'refuted' && v.reason.trim()branch, afterquestioned++;replaceif (citesOutside) demoted++;withdemoted++;andif (f.severity === 'blocker') demotedBlockers++;(declarelet demotedBlockers = 0;besidedemoted). AdddemotedBlockers: numberto its return type and returned object, toVerificationOutcome(comment: "Of those, the blockers, which a re-review still posts as should-fix questions."), anddemotedBlockers: 0toverifyFindings' early return. RewordVerificationOutcome.demoted's comment to: "Of those, the ones a call refuted with a reason: a blocker or security finding, which a refutation never drops, or one whose refutation cites no line of the checkout outside the lines the pull request adds." InverifyReview's return type add'demotedBlockers'to theOmit<VerificationOutcome, ...>list. InverificationNote, make thedemotedsentence'kept one refuted finding as a question one severity lower'for 1 and`kept ${demoted} refuted findings as questions one severity lower`otherwise, and its doc comment end "demoted, the refuted ones among them."test/verify.test.ts): thedeepEqualat the "with verification off" test addsdemotedBlockers: 0; "a refuted blocker is kept…" also assertsout.demotedBlockersis 1; "a refuted security finding…" assertsout.demotedBlockersis 0; "a refutation citing only the lines the pull request adds…" and "a refutation citing a file the checkout doesn't have…" assertout.demotedis 1 (was 0). In "verificationNote names the refuted findings…",verificationNote([], 2, 1)ends"…, and kept one refuted finding as a question one severity lower."andverificationNote([], 2, 2)is' After this summary was written, verification kept 2 refuted findings as questions one severity lower.'. Add a test "verifyReview's summary says a finding refuted only on the change's own lines was refuted and kept, not unsettled": follow the pattern of the existingverifyReviewtests (reviewInput), refute one should-fix with path'src/a.ts:2'(an added line there), and assert the summary matches/kept one refuted finding as a question one severity lower\.$/and does not match/couldn't settle/.verifyReviewtests nearreviewInput()intest/verify.test.ts.npm run typecheck && node --test test/verify.test.ts[ ] 2. Say a re-review kept a refuted finding instead of couldn't settle it
src/rereview.ts(edit),test/rereview.test.ts(edit)ReReviewVerification, keepquestionedPosted/questionedDroppedfor findings verification couldn't settle and addrefutedKeptPosted: numberandrefutedKeptDropped: number(comment: "Blockers it refuted but kept, posted as should-fix questions, and should-fixes it refuted but kept, now nits and not posted."). InverifyPosted, withdropped = outcome.findings.length - posted.length, setrefutedKeptPosted = outcome.demotedBlockers,refutedKeptDropped = outcome.demoted - outcome.demotedBlockers,questionedPosted = outcome.questioned - dropped - refutedKeptPosted,questionedDropped = dropped - refutedKeptDropped. Add the two names to thePickinreReviewSummary'sverificationparameter. InreReviewSummary, between the refuted sentence and thequestionedPostedone, add forv?.refutedKeptPosted(n):`Verification refuted ${plural(n, 'blocker')} but kept ${n === 1 ? 'it as a should-fix question' : 'them as should-fix questions'}.`; between thequestionedPostedandquestionedDroppedsentences add forv?.refutedKeptDropped(n):`Verification refuted ${n === 1 ? "1 should-fix but kept it as a nit, which isn't" : `${n} should-fixes but kept them as nits, which aren't`} posted on a re-review.`.verifiedAwayalso countsv.refutedKeptDropped./Verification refuted 1 blocker but kept it as a should-fix question\. 1 new finding at should-fix or above\.$/. Add "reReview with verification on says a should-fix refuted only on the change's own lines was kept as a nit, not posted, and never approves": as that test, withfinding('should-fix', 'a.ts', 62, 'Shaky')and the verifier answeringShaky: { verdict: 'refuted', path: 'a.ts:62', reason: 'Only the new line guards it.', question: '' }; assertoutcome.findingsis[],outcome.refutedis[], the verdict is'comment', the summary matches/Verification refuted 1 should-fix but kept it as a nit, which isn't posted on a re-review\./and does not match/couldn't settle|No further findings/.verifier()-based tests near the end oftest/rereview.test.ts.npm run typecheck && node --test test/rereview.test.ts[ ] 3. Add the duplicate rule and the merge of overlapping findings
src/threads.ts(edit),src/rereview.ts(edit),test/rereview.test.ts(edit)src/threads.ts, belowrepeats, exportnearby(a, b)(bothPick<ReviewedFinding, 'file' | 'line'>): samefileandMath.abs(a.line - b.line) <= repeatDistance; andsameProblem(a, b)(bothPick<ReviewedFinding, 'file' | 'line' | 'title'>):nearby(a, b) && fingerprint({ ...a, severity: 'nit' }) === fingerprint({ ...b, severity: 'nit' }), with a comment: two findings of one review that repeat each other, in the same file within a few lines and with the same title words, whatever their severity. Insrc/rereview.ts, import both, and export afterpostable:severityRank[severity]thenconfidencedescending (stable). For each finding,others= the kept findings, with their index, that share no reviewer with it and arenearby. If one ofothersissameProblemwith it, replace that kept entry with{ ...kept, reviewers: [...kept.reviewers, ...finding.reviewers] }and countmerged; otherwise countnearwhenothersisn't empty, and keep the finding. Comment: correctness and security can report one problem twice; of two findings by different reviewers thatsameProblempairs, only the more severe is posted, the more confident on a tie, naming both reviewers, so verification never drops it when security is one;nearcounts findings kept apart though close to another reviewer's, for the evals to measure.mergeOverlappingfrom../src/rereview.tsandsameProblemfrom../src/threads.ts;const by = (reviewer: string, f: Finding) => ({ ...f, reviewers: [reviewer] })): (a) correctness should-fixa.ts:10 'Token leaks into the log'and security blockera.ts:12 'Token leaks into the log!'→ one kept,[severity, line, reviewers]=['blocker', 12, ['security', 'correctness']],merged1,near0; (b) two correctness findings with the same title at lines 10 and 11 → both kept,merged0; (c) correctness'Off by one'at 10 and security'Token leaks'at 13 → both kept,near1; (d) same title at 10 and 14, or ina.tsandb.ts→ both kept,near0; (e) both should-fix, correctness confidence 0.6, security 0.9, same title, same line → one kept with confidence 0.9 and reviewers['security', 'correctness']. One test per case; name each "mergeOverlapping …". Add "sameProblem ignores severity and punctuation but not the file or distance".npm run typecheck && node --test --test-name-pattern "mergeOverlapping|sameProblem" test/rereview.test.ts[ ] 4. Merge overlapping findings in the re-review
src/rereview.ts(edit),test/rereview.test.ts(edit),README.md(edit)reReview, afterif (dropped.length) log(...), addconst { kept: distinct, merged, near } = mergeOverlapping(kept);and passdistinctinstead ofkepttobyOwner. In the log line that follows, insert${merged} merged with another reviewer's, ${near} near another reviewer's,after${placed.upstream.length} upstream,. The reviewers' own outcomes (ran) keep every finding.securityChange()androleAgentas "reReview adds security on Sonnet…" does, with correctnessfinding('should-fix', 'token.ts', 1, 'Token is exported')and securityfinding('blocker', 'token.ts', 1, 'Token is exported.'), collecting log lines; assertoutcome.findings.map((f) => [f.id, f.title, f.severity, f.reviewers])is[['F1', 'Token is exported.', 'blocker', ['security', 'correctness']]],outcome.reviewers.map((r) => r.findings.length)is[1, 1], and a log line matches/1 merged with another reviewer's, 0 near another reviewer's/.correctnessandsecurityreport the same problem, in the same file within three lines and with the same title words, it posts it once, at the higher severity, with both reviewers; the log counts these and the findings of the two that sit that close without matching;".npm run typecheck && node --test test/rereview.test.ts; if ($LASTEXITCODE -or -not (Select-String -Path README.md -SimpleMatch 'at the higher severity, with both reviewers' -Quiet)) { exit 1 }[ ] 5. Let an eval case set sensitive paths
evals/cases.ts(edit),evals/run.ts(edit)EvalCase, right afterearlier?: EarlierFinding[];, addevals/run.tsmain(), right afterlet offChanged: string[] = [];, add the two lines:evals/run.ts.npm run typecheck; if ($LASTEXITCODE -or -not (Select-String -Path evals/run.ts -SimpleMatch 'if (c.sensitive) gathered.sensitive = c.sensitive;' -Quiet) -or -not (Select-String -Path evals/cases.ts -SimpleMatch 'sensitive?: string[];' -Quiet)) { exit 1 }[ ] 6. Add the security-floor re-review case
evals/cases.json(edit)idis"re-review-merge-of-main"(add a comma after that object's closing}), exactly as written:{ "id": "re-review-security-floor", "repository": "NCodeGroup/code-review", "pullRequest": 71, "title": "Never post a finding that says a changed file isn't changed", "body": "Root cause: it wasn't chunking or reviewer file selection. At ab5859f all 6 files (18 KB, far under `maxChunkChars`) went into one part, and every reviewer saw `src/command.ts` and `test/engine.test.ts`. The trace shows no reviewer raised F4: it has no sources (`reviewers: []`), so synthesis (Opus) wrote it itself. Synthesis got only the diffs of the files the reviewers' findings named (`synthesisDiff`), here `test/fetch-head.test.ts`, `src/main.ts` and `src/fetch-head.ts`, and no list of changed files. From that partial diff it concluded that `command.ts` had no changes.\r\n\r\nFix: synthesis now gets the complete changed-file list, with each file whose diff isn't in its excerpt marked. Its excerpt starts with the files the findings name, then adds every other file's diff that fits the 120 KB budget. The reviewer and synthesis prompts say the list is complete; in a multi-part review, the reviewers' list marks files whose diff is in another part. A deterministic guard (`src/claims.ts`) drops a finding that says a changed file isn't changed or isn't in the diff (\"no changes to X\", \"no X hunk\", \"doesn't change X\", \"X is unchanged / not in the diff\", …). Dropped findings are logged and listed in the check run and the trace. Run on the 179 findings from the last 15 review traces, the guard drops only the #22 finding.\r\n\r\nDecisions:\r\n- The guard drops findings rather than rewriting them, so a false claim is never posted. A dropped finding frees its slot under the limit and stays visible under \"Dropped before posting\" in the check run.\r\n- To keep false drops rare, the guard matches only when the file is the subject or object of the claim. \"The caller in X isn't in the diff\" and \"the fix needs no changes to X\" are kept.\r\n- Synthesis can still add findings of its own with no sources. I left that alone; it's a separate question for the maintainer.\r\n\r\nCloses #32\r\n", "base": "99395ab829f82f825b3137b115676d04f84f7983", "head": "a7a885ecc314cb84a16fe47b1eb1b4035c225b83", "reviewed": "0c9103c3dddcf03061c20180bb8f30b92e968655", "earlier": [ { "id": "F1", "file": "src/claims.ts", "line": 35, "severity": "should-fix", "title": "PR author can choose file names that make the drop guard suppress real findings", "text": "`mention()` matches a changed file's bare name in finding prose with no extension required. The trailing lookahead `(?![\\w/-]|\\.\\w|'s\\b)` lets a following space through, so an extensionless changed file named `auth`, `validation` or `token` matches ordinary prose like \"no changes to auth\" or \"the diff does not include validation of the token\".\n\n**Consequence:** An author can add a decoy file named after a common word and get reviewer findings about missing validation or missing changes in that area silently dropped, blockers included. The posted review shows only a count of left-out findings, and `settleVerdict` can then downgrade `request-changes`.\n\n**Fix:** Match a bare name only when it has an extension, or only when it is backticked or quoted. Don't let a dropped blocker downgrade the verdict, or keep dropped blockers visible in the posted review.", "resolved": false, "url": "https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430649624", "replies": [ { "author": "polewskm", "body": "Autopilot: fixed in a7a885e: a changed file whose name has no dot or slash (a root-level `auth`, or the name of `lib/token`) now counts as named only in backticks or quotes, so 'no changes to auth' or 'the diff does not include validation of the token' keeps its finding; a test pins both sides. The verdict half stays as it is: round 1 asked that a dropped blocker stop holding up request-changes (https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430608637), and the summary comment already says how many findings were left out and how severe, blockers included." } ] }, { "id": "F2", "file": "src/review.ts", "line": 501, "severity": "should-fix", "title": "No test covers the dropped-finding wiring in review() or changeContext's multi-part marking", "text": "The new tests call `settleFindings`, `synthesisDiff` and `fileList` directly, but nothing runs `review()` itself or `changeContext` with `parts > 1`.\n\n**Consequence:** Each of these can break while the suite stays green: passing the wrong changed-file list to `settleFindings`, building the synthesis prompt's file list from the wrong set, or marking the wrong files `[diff in another part]`. If they do, synthesis again tells the model that changed files are absent (the #32 regression), or dropped findings never reach `Results`.\n\n**Fix:** Add a test that runs `review()` with a stubbed `runAgent` whose synthesis output includes a contradicted finding. Assert that `dropped` and `findings` are set and that the synthesis prompt lists every changed file. Add a `changeContext` test with `parts > 1` asserting that a file outside the part carries `[diff in another part]`.", "resolved": false, "url": "https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430649625", "replies": [ { "author": "polewskm", "body": "Autopilot: declined: review() and changeContext are thin wrappers over pieces the tests drive directly: settleFindings (the drop, the limit, the numbering, both synthesis paths), synthesisDiff, and fileList with a set of files and the 'diff in another part' label. review() calls runAgent directly, so an end-to-end test needs a stubbing seam this PR doesn't add; see the round 1 answer at https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430608798. It could be a follow-up." } ] }, { "id": "F3", "file": "src/review.ts", "line": 275, "severity": "nit", "title": "Synthesis summary isn't reconciled with dropped findings", "text": "`settleFindings` recomputes the verdict when findings are dropped, but it posts synthesis's summary unchanged, and synthesis wrote that summary with the dropped findings in view.\n\n**Consequence:** The summary comment and the check run can repeat the contradicted claim, and the summary may no longer agree with the adjusted verdict.\n\n**Fix:** When anything was dropped, append a short note to the summary, or document the gap.", "resolved": false, "url": "https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430649628", "replies": [ { "author": "polewskm", "body": "Autopilot: declined: a repeat of round 1's finding, answered at https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430608637: the summary comment and the check run say findings were left out, and rewriting synthesis's own summary would take another model call." } ] }, { "id": "F4", "file": "src/review.ts", "line": 310, "severity": "nit", "title": "Is settleVerdict's special-casing worth keeping over re-deriving the verdict from kept findings?", "text": "`settleVerdict` keeps synthesis's `request-changes` only if at least one finding was kept and no dropped finding was a blocker. Those conditions patch the effect of the regex drop.\n\n**Consequence:** Verdict outcomes depend on ad hoc conditions that are hard to reason about and can surprise maintainers.\n\n**Fix:** Consider deriving the verdict from the kept findings alone whenever anything was dropped, or asking synthesis again.", "resolved": false, "url": "https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430649630", "replies": [ { "author": "polewskm", "body": "Autopilot: declined: answered in round 1 at https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430608637. Deriving the verdict from the kept findings alone would throw away synthesis's request-changes on kept should-fixes whenever an unrelated nit was dropped." } ] }, { "id": "F5", "file": "src/claims.ts", "line": 55, "severity": "nit", "title": "Regex filter on model prose is a stopgap behind the prompt fix", "text": "`dropContradicted` matches English phrasings of \"X isn't changed\". It will need new patterns for each new phrasing, and because it can drop real findings, `settleVerdict` has to clean up after it.\n\n**Consequence:** Phrasings the patterns miss still get posted, and over-matches silently remove valid findings, so patterns and verdict special cases keep piling up.\n\n**Fix:** Keep the guard for now, but prefer a structured signal, such as a schema field naming the files each finding depends on, checked against the changed-file list.", "resolved": false, "url": "https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430649633", "replies": [ { "author": "polewskm", "body": "Autopilot: declined: a repeat of round 1's design finding, answered at https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430608862. A schema field naming the files a finding depends on could be a follow-up." } ] }, { "id": "F6", "file": "src/review.ts", "line": 274, "severity": "nit", "title": "mergeSynthesis's limit and numbering are now redundant in production", "text": "`settleFindings` calls `mergeSynthesis` without a limit, then slices and renumbers after `dropContradicted`. So `mergeSynthesis`'s `slice(0, limit)` and its F-id assignment only matter in tests.\n\n**Consequence:** Readers may assume `mergeSynthesis` enforces the limit, and later edits could apply the limit twice or in the wrong place.\n\n**Fix:** Remove the `limit` parameter and the id assignment from `mergeSynthesis`, and have the tests slice the result themselves.", "resolved": false, "url": "https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430649634", "replies": [ { "author": "polewskm", "body": "Autopilot: declined: round 1 asked for this shape, an optional limit with the guard, the limit and the numbering together in settleFindings (https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430609027); removing the parameter now would churn mergeSynthesis's tests with no change in behavior." } ] }, { "id": "F8", "file": "src/results.ts", "line": 162, "severity": "nit", "title": "Severity order and the dropped-claim wording are duplicated", "text": "`droppedNote` hardcodes `['blocker','should-fix','nit']` even though `severityRank` already defines that order. The phrase \"said a file this pull request changes isn't changed\" is also written out separately in `summaryComment`, `checkSummary` and `checkText`.\n\n**Consequence:** If a severity is added or the wording changes, the copies can drift and the summary comment and check run disagree.\n\n**Fix:** Derive the order from one exported severity list, and define the claim phrase once as a constant.", "resolved": false, "url": "https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430649635", "replies": [ { "author": "polewskm", "body": "Autopilot: declined: nit; not worth another round. The three sentences are worded for where they appear (summary comment, check summary, dropped list), not one string copied, and sharing the severity order with review.ts would widen the PR." } ] }, { "id": "F9", "file": "src/review.ts", "line": 288, "severity": "nit", "title": "settleFindings takes four data parameters, and its call site nests a call", "text": "`settleFindings(synthesis, raw, changedFiles, limit)` takes four data parameters, and its call site passes `gathered.files.map((f) => f.path)` inline as an argument.\n\n**Consequence:** This only affects readability. Also, the cited rules come from csharp-production.instructions.md and may not apply to TypeScript.\n\n**Fix:** Assign `const changedFiles = gathered.files.map((f) => f.path)` before the call, and optionally bundle the inputs into one object.", "resolved": false, "url": "https://github.com/NCodeGroup/code-review/pull/71#discussion_r4430649637", "replies": [ { "author": "polewskm", "body": "Autopilot: fixed in a7a885e: review() assigns `const changedFiles` before calling settleFindings. Bundling the four inputs into one object is declined: the cited rule comes from the C# instructions, and the four inputs are distinct and named." } ] } ], "expect": [], "mustNotFlag": [ { "concern": "A finding the drop guard leaves out, a blocker included, should not let the verdict ease from request-changes, or should stay visible in the posted review. The author declined this half of round 2's security finding in its thread: round 1 asked that a dropped blocker stop holding up request-changes, and the summary comment already says how many findings were left out and how severe." } ], "source": "Re-review of #71 (issue #32), round 3, at a7a885e since the review at 0c9103c: what changed is src/claims.ts, which the selection floor gives to security (its path has claim in it), and one line of src/review.ts, so security joins correctness, both on Sonnet; no sensitive: applies. The earlier findings are round 2's as posted, with the replies before round 3, left unresolved as a re-review checks them; round 1's, all resolved, are left out. The must-not concern is the half of round 2's F1 the author declined." }node -e "const c=require('./evals/cases.json'); const i=c.findIndex(x=>x.id==='re-review-security-floor'); const x=c[i]; process.exit(x && c[i-1].id==='re-review-merge-of-main' && x.reviewed==='0c9103c3dddcf03061c20180bb8f30b92e968655' && x.earlier.length===8 && x.mustNotFlag.length===1 ? 0 : 1)" && node --test --test-name-pattern "eval concern" test/engine.test.ts[ ] 7. Add the sensitive-path re-review case
evals/cases.json(edit)idis"re-review-security-floor"(add a comma after that object's closing}), exactly as written:{ "id": "re-review-sensitive-path", "repository": "NCodeGroup/code-review", "pullRequest": 19, "title": "Fetch the pull request's head in the engine, so a calling workflow needs no checkout of untrusted code", "body": "Every repository that adopts the bot gets the same two CodeQL alerts on `code-review.yml`: *checkout of untrusted code in a privileged context* and its TOCTOU variant. They are accepted risks (the checkout is only read), but they repeat for each repository, mark the adoption PR's `CodeQL` check red, and need a dismissal each time. This removes the cause.\n\n## What changes\n\n- **The workflow no longer checks the head out.** The engine fetches the commit itself (`src/fetch-head.ts`) after it has decided to review: a `/review` comment from a writer, a pull request that isn't from a fork, a commit not already reviewed. A caller then needs only the checkout of its default branch.\n- **The exact commit.** It checks out the SHA from the event, or from the API for a comment, instead of `refs/pull/N/head`. A push between a `/review` comment and the checkout can no longer change what is reviewed, which the TOCTOU alert was about.\n- **git runs bare.** No system or user configuration (an empty folder stands in for the home directory), no hooks, no fsmonitor, no credential helper, no prompts. The token goes through `GIT_CONFIG_*` environment variables scoped to the remote's origin, never a command line or the remote's URL, and the test checks it isn't left in `.git/config`. A SHA that isn't 40 hex characters, a bad number and a non-empty folder are refused.\n- **Backward compatible.** A folder that is already a repository is left alone, so v0.2 workflows that still check the head out keep working.\n\n## What does not change\n\nThe risk is the same as before and is mitigated the same way: the head is data on disk, read by an agent with no settings, hooks, MCP servers or `CLAUDE.md` from it, read-only tools and a path guard. This moves the fetch into the audited engine, where those mitigations live, and out of every caller's workflow file. It does not make reading an untrusted checkout safer by itself, and I am not claiming it does.\n\n## Rollout\n\n1. Merge, then release v0.3.0.\n2. Update the template's `code-review.yml` and the repositories' workflows to v0.3.0 and delete their second checkout.\n3. The alerts already open on `main` are dismissed once; no repository gets new ones.\n\n`npm test` (33 tests, 4 new, against a local repository standing in for the remote) and `npm run typecheck` pass. The fetch against github.com itself is untested until the first run of v0.3.0, which I'll watch.\n", "base": "28d50ad3ab13849fa54bf0801334071ccd38011c", "head": "1516681d4b09ff78c6fedd50f2652a5390e12422", "reviewed": "08481768d836784ef32ebfa9a39d5beb6031c847", "sensitive": [ "src/fetch-head.ts" ], "earlier": [ { "id": "F2", "file": "src/fetch-head.ts", "line": 72, "severity": "nit", "title": "`has()` treats every git failure as 'commit missing'", "text": "`has` catches any exception from `git cat-file -e`, including a broken repository or a failed spawn, and returns false.\n\n**Consequence:** The real cause stays hidden until the second fetch fails with a generic message. The run still fails, but the failure is harder to diagnose.\n\n**Fix:** Only treat the not-found case as false (check the exit status or stderr), or add a comment saying that any failure is meant to mean 'fetch it'.", "resolved": false, "url": "https://github.com/NCodeGroup/code-review/pull/19#discussion_r4429497458", "replies": [ { "author": "polewskm", "body": "Deliberate, now said in a comment above `has` (1516681): any failure counts as 'not there', and the fetch by name that follows reports a broken repository with git's own message, so the cause is not hidden for long. I did not narrow it to one exit code, because `cat-file -e` on a missing object and on a broken repository both exit 128 and can't be told apart without parsing stderr." } ] }, { "id": "F3", "file": "src/fetch-head.ts", "line": 91, "severity": "nit", "title": "`rev-parse` failure drops git's error detail", "text": "The catch around `git rev-parse HEAD` throws a fixed message and throws away the original error.\n\n**Consequence:** A corrupt or unreadable `.git` gets reported as 'without a commit checked out', which can send the user looking in the wrong place.\n\n**Fix:** Append the original error message to the thrown text, the way the fetch failure below appends `(${detail})`.", "resolved": false, "url": "https://github.com/NCodeGroup/code-review/pull/19#discussion_r4429497462", "replies": [ { "author": "polewskm", "body": "Fixed in 1516681. The error now ends with the original message: \"<path> is a repository without a commit checked out. (<git's message>)\". New test: a repository made with `git init` and no commit." } ] }, { "id": "F5", "file": "README.md", "line": 56, "severity": "nit", "title": "Example pins `v0.3.0` before that release exists", "text": "The README points consumers at `NCodeGroup/code-review@v0.3.0`, but this PR creates no tag, and no CHANGELOG or release note records the behavior change.\n\n**Consequence:** Anyone who copies the example before v0.3.0 is published gets a failing workflow, and nothing in the repository says that the checkout step was removed.\n\n**Fix:** Bump the pin in the release PR, or confirm v0.3.0 ships this change. Record the change in the release notes: the engine now fetches the head, and the workflow's checkout step was removed.", "resolved": false, "url": "https://github.com/NCodeGroup/code-review/pull/19#discussion_r4429497468", "replies": [ { "author": "polewskm", "body": "Intended. The example names the release this change ships in, and the release follows the merge (I cut v0.3.0 from main after it lands). The repository has no CHANGELOG; its release notes are generated from the merged pull request titles, and this one's title says the engine now fetches the head. I did not want to leave the README naming v0.2.1 with an example that no longer matches the engine." } ] }, { "id": "F7", "file": "src/fetch-head.ts", "line": 54, "severity": "nit", "title": "`git` helper takes four data parameters", "text": "`git(cwd, empty, args, extra)` goes past the 2–3 parameter limit, and `cwd` and `empty` are passed as a pair at every call site.\n\n**Consequence:** Every call repeats the same two arguments, which makes the calls noisy and easy to get in the wrong order.\n\n**Fix:** Bind `cwd` and `empty` in a small context object or closure, so each call passes only `args` and `extra`.", "resolved": false, "url": "https://github.com/NCodeGroup/code-review/pull/19#discussion_r4429497469", "replies": [ { "author": "polewskm", "body": "Fixed in 1516681. `git` now takes `(where, args, extra)`, with `where = { cwd, empty }` built once per call to `fetchPullRequestHead`, so no call site repeats the pair." } ] } ], "expect": [ { "file": "src/fetch-head.ts", "concern": "The git fetches of the pull request's head run with no time limit, so a remote that accepts the connection and never answers holds the review until the job's own timeout." } ], "mustNotFlag": [], "source": "Re-review of #19, round 3, at 1516681 since the review at 0848176: what changed is src/fetch-head.ts, which fetches the pull request's head with the app's token, and src/command.ts and src/main.ts. No path the selection floor gives to security changed, and the repository has no sensitive: setting, so the case's sensitive names src/fetch-head.ts as a repository guarding that fetch would: security joins correctness and the re-review goes deep, both on Opus. The expected concern is round 3's F1 (correctness, should-fix), which #22 fixed with a fetch time limit. The earlier findings are round 2's as posted, with the replies before round 3, left unresolved as a re-review checks them; round 1's, all resolved, are left out." }node -e "const c=require('./evals/cases.json'); const i=c.findIndex(x=>x.id==='re-review-sensitive-path'); const x=c[i]; process.exit(x && c[i-1].id==='re-review-security-floor' && x.sensitive[0]==='src/fetch-head.ts' && x.earlier.length===4 && x.expect.length===1 ? 0 : 1)" && node --test --test-name-pattern "eval concern" test/engine.test.ts[ ] 8. Add the re-review subset, its test and the README
evals/cases.ts(edit),test/eval-cases.test.ts(new),README.md(edit)evals/cases.ts, aboveexport const subsets, extend the comment with a line: "re-review: every case with a reviewed head, to compare re-reviews and read each one's cost (costUsd, reviewCostUsd in the output)." and add the entry're-review': ['re-review-declined-trigger', 're-review-merge-of-main', 're-review-security-floor', 're-review-sensitive-path'],aftersmoke. Newtest/eval-cases.test.ts(imports:node:assert/strict,node:fsreadFileSync,node:path,node:test,selectCasesfrom../evals/run.ts, typeEvalCasefrom../evals/cases.ts; readevals/cases.jsonastest/smoke-eval.test.tsdoes) with two tests: "the re-review subset is every eval case with a reviewed head" (selectCases(all, undefined, 're-review')ids deep-equalall.filter((c) => c.reviewed).map((c) => c.id)), and "a re-review eval case changes a sensitive: path, and its globs are plain paths" (some case hasreviewedand a non-emptysensitive; everysensitiveentry of every case is a non-empty string).re-review-declined-trigger(Make the type checker fail on what a linter would warn about, and check commit attribution in CI #29) andre-review-merge-of-main(Post a new summary comment for each review #30, with main merged in) are the two;--only re-review-declined-trigger,re-review-merge-of-mainruns just them." with: "re-review-declined-trigger(Make the type checker fail on what a linter would warn about, and check commit attribution in CI #29),re-review-merge-of-main(Post a new summary comment for each review #30, with main merged in),re-review-security-floor(Never post a finding that says a changed file isn't changed #71, a change tosrc/claims.ts, which the selection floor gives tosecurity) andre-review-sensitive-path(Fetch the pull request's head in the engine, so a calling workflow needs no checkout of untrusted code #19, a change tosrc/fetch-head.ts, which the case'ssensitivemarks, so the re-review goes deep on Opus) are the four. A case'ssensitivestands in forconfig.yml's, for a repository that sets none.--subset re-reviewruns just them, and the output has each one's cost (costUsd,reviewCostUsd)."node --test test/eval-cases.test.ts test/smoke-eval.test.ts; if ($LASTEXITCODE -or -not (Select-String -Path README.md -SimpleMatch '--subset re-review' -Quiet)) { exit 1 }[ ] 9. Write the changelog entries
CHANGELOG.md(edit)## [Unreleased], add to the existing### Changedgroup (at its end):- A re-review posts a problem \correctness` and `security` both report, in the same file within three lines and with the same title words, once, at the higher severity and naming both reviewers, instead of two threads; its log counts these merges and the findings of the two that sit that close without matching, for the evals to measure (#116).`- The evals have a re-review case whose changes touch a file the selection floor gives to \security` and one whose changes touch a `sensitive:` path, which a case can now set for a repository that has none; `--subset re-review` runs the four re-review cases, whose output has each one's cost (#117).and to the existing### Fixed` group (at its end):- A summary no longer says verification couldn't settle a finding it refuted but kept as a question, a refuted blocker on a re-review or a refutation that cites only the lines the pull request adds: it says the finding was refuted and kept ([#114](https://github.com/NCodeGroup/code-review/issues/114)).(The backslashes above only escape the backticks for this plan; write plain backticks.) Change no other section.
$u = (Get-Content CHANGELOG.md -Raw) -split '\n## \[' | Select-Object -Index 1; if ($u -notmatch '^Unreleased\]' -or $u -notmatch 'issues/114\)' -or $u -notmatch 'issues/116\)' -or $u -notmatch 'issues/117\)') { exit 1 }Review checklist
verificationNoteandreReviewSummarynever say "couldn't settle" for a finding a call refuted with a reason; a refutation without a reason and an uncertain verdict still do.questionedPosted + refutedKeptPostedblockers posted,questionedDropped + refutedKeptDroppedshould-fixes made nits; no count can go negative.mergeOverlappingmerges only across reviewers and onlysameProblempairs; the merged finding keepssecurityinreviewerswhen either had it, and the higher severity.dropContradictedand beforebyOwner, so upstream findings and verification see the merged list;outcome.reviewersstill holds each reviewer's raw findings for the evals.evals/run.tschanged by exactly the two added lines;evals/cases.ts's new field is afterearlier.re-review-merge-of-main, and carry full 40-character SHAs;npm testpasses.Follow-ups
node evals/run.ts --subset re-reviewwith verification off and on, record each case'sreviewCostUsdin theEvalssection ofREADME.md, and count from the log how oftencorrectnessandsecurityfindings sit within three lines without matching (near), to decide whethermergeOverlappinginsrc/rereview.tsshould also merge them; the new cases measure the deep re-review on Opus, which has never been run.