Activity

1. polewskm

   added

   follow-upWork a plan found and did not doWork a plan found and did not do

   on Oct 31, 2026
2. polewskm commented on Oct 31, 2026

   

   polewskm

   on Oct 31, 2026

   ContributorAuthor

   More actions

   Plan snapshot. The working copy is PLAN.md in the plan's checkout; this comment is not updated as steps run.

   Plan: Close the test gaps declined in review rounds (6 steps)

   Plan: Close the test gaps declined in review rounds

   Issue: #96

   Goal

   Add the tests the review bot asked for on #64, #68, #69 and #71 that were declined as past the round limit: review() building each part's prompt (diff, rules, charter) and marking the other part's files on a two-part diff, a project reviewer run through review() on its configured tier, and the Bedrock rerun of the selection and context calls (plus a direct test of withoutTools). Only test files and the changelog change. The two gaps that need a non-test change first (#73's judge spend and #75's answer()) become follow-ups.

   Context for the executor

   • Build: npm run typecheck
   • Test: npm test (one file: node --test test/<name>.test.ts)
   • Conventions: Node 24 runs TypeScript directly; tsconfig.json is strict (noUncheckedIndexedAccess, exactOptionalPropertyTypes, noUnusedLocals). Tests use node:test and node:assert/strict, one behavior per test, hermetic. Test names are full sentences in the present tense. UTF-8 without BOM, LF line endings.
   • Gotchas:
     • review(run, gathered, charters, log, agent) in src/review.ts takes the agent as its last parameter: pass a stub so no model is called. Selection and context requests have a string prompt; a reviewer request's prompt is a two-element array: [sharedBlock, rulesAndCharter]. prompt[0] holds the conventions, the file list and <diff>\n<part's diff>\n</diff>; on a diff in parts it says this is part <n>. (1-based). prompt[1] is the rules block (empty when the call has no rules) then \n\nYour charter:\n<charter text>, so with no rules it is exactly \n\nYour charter:\n<charter text>.
     • A guidance RuleFile named errors-and-validation.md gives rules with ids G-errors-and-validation-1, -2, … (one per line starting - **) that go to the correctness reviewer; a bullet ending *Applies to:* `src/A.cs` reaches only parts of the diff that contain src/A.cs. Picked rules also run a rule-check reviewer call per part.
     • A repository RuleFile named review.instructions.md with text - Keep greetings lowercase.\n gives one rule, R-review-1, for the conventions reviewer; when the context call picks it, conventions joins the floor and a rule-check call runs.
     • Environment variables change review(): clear CODE_REVIEW_TIERS, CODE_REVIEW_RULE_CHECKS, CODE_REVIEW_SHADOW, CODE_REVIEW_SHADOW_RATE, CODE_REVIEW_VERIFICATION and CODE_REVIEW_MAX_RUN_USD in a beforeEach, as test/shadow.test.ts does.
     • node --test exits 0 when a name pattern matches nothing, so each Done-when checks that the new test's name is in the output.

   Decisions

   • Kind of plan: tests. The issue's only label is follow-up; its title and body list missing tests.
   • The judge-spend split (Evals: measure precision and findings per pull request, and compare two runs #73) lives only inside main() of evals/run.ts, and answer() (/review explain <finding>: answer whether a finding still holds #75) is a private function of src/main.ts, which runs main() when imported. Testing either needs a non-test refactor first; other plans are changing src/ in parallel and this plan stays in test/, so both are follow-ups. Made on the user's behalf (autopilot): the run asked for a test-only plan.
   • The two-part changed-file marking (Never post a finding that says a changed file isn't changed #71) is tested through review() on a two-part diff, in the same file as the part-prompt tests, rather than by exporting the private changeContext: it checks the wiring the bot named and changes no src/ file.
   • The review() prompt and project-reviewer tests go in a new file, test/review-prompts.test.ts, with its own stub agent copied from stubAgent() in test/shadow.test.ts: no existing file is about how review() builds prompts, and a stub agent keeps them independent of runAgent.
   • The Bedrock tests stay in test/fallback.test.ts and run through its fake Agent SDK (fakeQuery), so they exercise the real runAgent and withoutTools. reviewWithTestsRefused() becomes a wrapper over a general reviewOnFakeSdk(input, apiRefuses, bedrockFails) helper so the new tests reuse it; the existing tests keep their behavior.
   • The fake refuses the selection call by its prompt starting Roster: and the context call by its prompt containing <catalog>: each phrase appears only in that call's prompt.
   • A synthesis-call refusal isn't tested separately: the issue names selection and context, and the direct withoutTools test covers the path all three share.
   • No Release: line: AGENTS.md's Landing section has release: none. The changelog gets one ### Changed entry under ## [Unreleased], as every plan's last step does.

   Out of scope

   • Any change outside test/review-prompts.test.ts, test/fallback.test.ts and CHANGELOG.md. In particular, no change to src/, evals/ or training/.
   • Changing an existing test's assertions in test/fallback.test.ts (only its helper is split, step 4).
   • Tests for answer() in src/main.ts or for the eval review cost in evals/run.ts (follow-ups).

   Steps

   [ ] 1. Test that each part's reviewers get that part's diff and the other part's file marked

   • Files: test/review-prompts.test.ts (new)
   • Change: Create the file with:
     • Imports: assert from node:assert/strict; beforeEach, test from node:test; type AgentRequest, AgentResult, AgentRun, CallRecord, runAgent from ../src/agent.ts; type Gathered from ../src/gather.ts; review, type Charters from ../src/review.ts.
     • stubAgent(answer: (request: AgentRequest) => unknown): copy stubAgent() from test/shadow.test.ts, but answer returns the output directly (no error case) and the record's provider is 'bedrock', model is request.tier.
     • diffA / diffB: the five-line diffs of test/shadow.test.ts (const diff = ...) for src/A.cs and src/B.cs.
     • twoParts(fields: Partial<Gathered> = {}): Gathered: pull request 7 in o/r, files src/A.cs and src/B.cs (each additions: 1, deletions: 0, generated: false, shown: true), diff: ${diffA}${diffB}``, chunks: [diffA, diffB], `guidance: [{ name: 'errors-and-validation.md', globs: [], text: ['- Rule for A why. Check: c Applies to: `src/A.cs`', '- Rule for B why. Check: c Applies to: `src/B.cs`'].join('\n') }]`, every other field as in `gathered()` of `test/shadow.test.ts`, then `...fields`.
     • charters: Charters: prompts 'SPECIALIST PROMPT', 'SELECTION PROMPT', 'SYNTHESIS PROMPT', ruleCheckPrompt: 'RULE-CHECK CHARTER', 'VERIFICATION PROMPT', 'RE-REVIEW PROMPT'; reviewers correctness, hygiene, security, tests, each { text: '<NAME IN CAPITALS> CHARTER', shadow: false }; shadowPrompts: {}.
     • answers(request): context → { rules: ['G-errors-and-validation-1', 'G-errors-and-validation-2'], decisions: [] }; selection → { add: [] }; anything else → { findings: [] }.
     • run: AgentRun = { provider: 'bedrock', proxyUrl: 'http://127.0.0.1:1', checkout: '/nowhere' }; a beforeEach that deletes the six variables in Gotchas; reviewerCalls(requests, part): the requests whose role starts reviewer: and whose String(prompt[0]) includes this is part ${part}..
     • Test review() gives each part's reviewers that part's diff, and marks the other part's file as in another part: run review(run, twoParts(), charters, () => {}, agent). For part 1 (own diffA, other diffB) and part 2 (own diffB, other diffA): the sorted roles of reviewerCalls are ['reviewer:correctness', 'reviewer:hygiene', 'reviewer:rule-check', 'reviewer:tests']; each call's prompt[0] includes <diff>\n${own}\n</diff> and not the other diff's first line. Part 1's first call's prompt[0] matches /\nsrc\/A\.cs \(\+1 -0\)\nsrc\/B\.cs \(\+1 -0\) \[diff in another part\]\n/; part 2's matches /\nsrc\/A\.cs \(\+1 -0\) \[diff in another part\]\nsrc\/B\.cs \(\+1 -0\)\n/.
   • Pattern: test/shadow.test.ts (stubAgent, gathered, beforeEach, and the last test's two-part Gathered).
   • Done when: $out = node --test test/review-prompts.test.ts 2>&1 | Out-String; if ($LASTEXITCODE -ne 0 -or $out -notmatch 'marks the other part') { $out; exit 1 }; npm run typecheck

   [ ] 2. Test that each reviewer call gets its part's rules and its own charter

   • Files: test/review-prompts.test.ts (edit)
   • Change: Add test review() gives each reviewer call its part's rules and its own charter, and the rule check the rule-check prompt: same run as step 1. For part 1 (own 'Rule for A', other 'Rule for B') and part 2 (the reverse), with charter(role) = String(prompt[1]) of that part's call with that role: for reviewer:correctness and reviewer:rule-check, it includes the own rule and not the other; charter('reviewer:correctness') matches /Your charter:\nCORRECTNESS CHARTER$/; charter('reviewer:rule-check') matches /Your charter:\nRULE-CHECK CHARTER$/; charter('reviewer:hygiene') matches /^\n\nYour charter:\nHYGIENE CHARTER$/; charter('reviewer:tests') matches /^\n\nYour charter:\nTESTS CHARTER$/; every call of the part has systemPrompt === 'SPECIALIST PROMPT'.
   • Pattern: the step 1 test in the same file.
   • Done when: $out = node --test test/review-prompts.test.ts 2>&1 | Out-String; if ($LASTEXITCODE -ne 0 -or $out -notmatch 'the rule check the rule-check prompt') { $out; exit 1 }; npm run typecheck

   [ ] 3. Test project reviewers run through review()

   • Files: test/review-prompts.test.ts (edit)
   • Change: Add projectReviewers: { name: 'oauth', text: '# oauth\n\nDoes the change follow RFC 6749?\n', paths: ['src/**/OAuth/**'], shadow: false } and { name: 'billing', text: '# billing\n\nAre amounts in cents?\n', paths: ['src/Billing/**'], shadow: false }. Add oauthChange(): Gathered: diff = diffA.replaceAll('src/A.cs', 'src/Server/OAuth/Token.cs'), then twoParts({ files: [that one file, additions 1], diff, chunks: [diff], guidance: [], projectReviewers, tiers: { 'project:oauth': 'opus' } }). Add two tests:
     • review() runs a project reviewer whose paths match a changed file, on the tier the repository's config gives it: with stubAgent(answers), the selection request's prompt matches /^Roster: correctness, hygiene, security, tests, project:oauth, project:billing\nThis repository's own reviewers:\n- project:oauth: Does the change follow RFC 6749\?\n- project:billing: Are amounts in cents\?\nAlready chosen: [^\n]*\bproject:oauth\b/ and doesn't match /Already chosen: [^\n]*project:billing/; the reviewer:project:oauth request has tier 'opus' and its prompt[1] matches /Your charter:\n# oauth\n\nDoes the change follow RFC 6749\?\n$/; no request has role reviewer:project:billing; outcome.reviewers has project:oauth with status 'ran'; outcome.skipped includes project:billing.
     • selection can add a project reviewer whose paths match no changed file, and it runs on Haiku by default: the stub answers selection with { add: [{ reviewer: 'project:billing', reason: 'it charges a fee' }] } and otherwise answers(request); the reviewer:project:billing request's tier is 'haiku'; outcome.reviewers has project:billing with status 'ran'; outcome.skipped doesn't include it.
   • Pattern: the step 1 test in the same file; test/settings.test.ts for the project reviewer shapes.
   • Done when: $out = node --test test/review-prompts.test.ts 2>&1 | Out-String; if ($LASTEXITCODE -ne 0 -or $out -notmatch 'on the tier the repository' -or $out -notmatch 'runs on Haiku by default') { $out; exit 1 }; npm run typecheck

   [ ] 4. Test the Bedrock rerun of the selection and context calls

   • Files: test/fallback.test.ts (edit)
   • Change:
     • In outputFor(), after the 'add' line, add if ('rules' in properties) return { rules: ['R-review-1'], decisions: [] };.
     • Split reviewWithTestsRefused(): a new reviewOnFakeSdk(input: Gathered, apiRefuses: (prompt: string) => boolean, bedrockFails: (prompt: string) => boolean = () => false) holds its body (env save/restore, seen, fallback, run with query: fakeQuery(seen, apiRefuses, bedrockFails), review(run, input, loadCharters(root), () => {}), and the same { outcome, fallback, seen, byRole } return). reviewWithTestsRefused(bedrockFails = () => false) computes testsCharter as now and returns reviewOnFakeSdk(gathered(), (prompt) => prompt.includes(testsCharter), (prompt) => prompt.includes(testsCharter) && bedrockFails(prompt)). Keep its comment, minus the run description moved to the new helper.
     • After the test a reviewer whose Bedrock rerun fails too …, add:
       • a credit error on the selection call reruns it on Bedrock, and the review adds the reviewer the rerun picked: reviewOnFakeSdk(gathered(), (prompt) => prompt.startsWith('Roster: ')). fallback deep-equals { switched: true, reason: creditsRanOutReason }; byRole('selection') is [['anthropic', modelIds.anthropic.sonnet, creditsRanOutReason], ['bedrock', modelIds.bedrock.sonnet, 'success']]; byRole('reviewer:tests') is [['bedrock', modelIds.bedrock.haiku, 'success']]; byRole('synthesis') is [['bedrock', modelIds.bedrock.opus, 'success']]; reviewers [['hygiene', 'ran'], ['tests', 'ran']].
       • a credit error on the context call reruns it on Bedrock, and the reviewers get the rule the rerun picked: input { ...gathered(), reviewRules: [{ name: 'review.instructions.md', globs: [], text: '- Keep greetings lowercase.\n' }] }, refusing (prompt) => prompt.includes('<catalog>'). Same fallback check; byRole('context') is [['anthropic', modelIds.anthropic.sonnet, creditsRanOutReason], ['bedrock', modelIds.bedrock.sonnet, 'success']]; byRole('selection') is [['bedrock', modelIds.bedrock.sonnet, 'success']]; reviewers [['hygiene', 'ran'], ['conventions', 'ran'], ['tests', 'ran'], ['rule-check', 'ran']], with a comment that the picked rule puts conventions on the floor and runs a rule check.
   • Pattern: the test a credit error reruns that call on Bedrock and sends every later call there in the same file.
   • Done when: $out = node --test test/fallback.test.ts 2>&1 | Out-String; if ($LASTEXITCODE -ne 0 -or $out -notmatch 'credit error on the selection call' -or $out -notmatch 'credit error on the context call' -or $out -notmatch 'sends every later call there') { $out; exit 1 }; npm run typecheck

   [ ] 5. Test withoutTools directly

   • Files: test/fallback.test.ts (edit)
   • Change: Add withoutTools to the import from ../src/agent.ts (alphabetical, after totalCost). After the step 4 tests, add withoutTools keeps the run's provider, proxy, fallback and query, and drops its checkout and working folder: fallback = { switched: false }, query = fakeQuery([], () => false), run = { provider: 'anthropic', proxyUrl: 'http://127.0.0.1:1', checkout: '/checkout', workdir: '/work', fallback, query }, bare = withoutTools(run). Object.keys(bare).sort() deep-equals ['fallback', 'provider', 'proxyUrl', 'query']; bare.provider is 'anthropic'; bare.proxyUrl is 'http://127.0.0.1:1'; bare.fallback and bare.query are the same objects (assert.equal).
   • Pattern: the test only a run that starts on the Claude API with a Bedrock key gets a fallback in the same file.
   • Done when: $out = node --test test/fallback.test.ts 2>&1 | Out-String; if ($LASTEXITCODE -ne 0 -or $out -notmatch 'withoutTools keeps') { $out; exit 1 }; npm run typecheck

   [ ] 6. Record the tests in the changelog

   • Files: CHANGELOG.md (edit)
   • Change: Under ## [Unreleased], append to the end of the existing ### Changed list (after the entry that starts - The Release workflow fails before it tags), this entry on one line:
     - The tests cover how a review builds each reviewer's prompt on a diff in parts (its own part's diff, rules and charter, with the other parts' files marked as in another part), run the repository's own reviewers on the tier its config gives them, and rerun the selection and context calls on Bedrock when the API credits run out ([#96](https://github.com/NCodeGroup/code-review/issues/96)).
   • Pattern: the other entries under ### Changed.
   • Done when: if (-not (Select-String -Path CHANGELOG.md -SimpleMatch -Quiet 'rerun the selection and context calls on Bedrock when the API credits run out ([#96]')) { exit 1 }; npm run typecheck; if ($LASTEXITCODE) { exit 1 }; npm test

   Review checklist

   • Each new test fails when the wiring it covers breaks: swapping sharedBlocks[job.part] for another part, giving the rule check a reviewer's charter, dropping projectRoster(project) or projectFloor(...) from review(), ignoring gathered.tiers, or withoutTools dropping fallback.
   • The existing tests in test/fallback.test.ts still assert what they did; only reviewWithTestsRefused()'s body moved.
   • No file outside test/review-prompts.test.ts, test/fallback.test.ts and CHANGELOG.md changed.
   • The new tests are hermetic: no model call, no network, and every environment variable review() reads is cleared first.

   Follow-ups

   • Test that judge spend stays out of the eval review cost: move the per-case accumulation in main() of evals/run.ts (the reviewCost sum over outcome.calls, the caseCost over calls with the judge calls, and the pushes into precisions, findingsPerCase and reviewCosts) into an exported pure function, and test that the review cost leaves out the concern-judge and precision-judge calls while the case cost includes them, so eval:compare can't silently report the Opus judge spend as review cost (asked on Evals: measure precision and findings per pull request, and compare two runs #73, listed in Test gaps declined during review rounds #96).
   • Test /review explain's answer() directly: move the file-writing and answered flag decision out of answer() in src/main.ts (which runs main() on import, so no test can import it) into an exported function that takes the resolution, the outcome and an output folder, and test that an unknown ref writes only answer.json without a model call, a call that ran also writes its trace, the answered flag is set, and no case writes results.json (asked on /review explain <finding>: answer whether a finding still holds #75, listed in Test gaps declined during review rounds #96).
3. polewskm

   closed this as completedin #99on Oct 31, 2026
4. polewskm commented on Oct 31, 2026

   

   polewskm

   on Oct 31, 2026

   ContributorAuthor

   More actions

   Landed in #99. Follow-ups filed: #107, #108. The changelog entry, which merged into the released 0.4.0 section, moves back under Unreleased in #106.
5. polewskm

   added a commit that references this issue on Oct 31, 2026

   Merge pull request #106 from NCodeGroup/changelog-96

   ...

   a89693c