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: Test eval review cost and explain's answer (4 steps)

   Plan: Test eval review cost and explain's answer

   Issue: #107 #108

   Goal

   Pin two behaviors that only main() functions hold today, so no test reaches them. #107: the evals' review cost must leave out the concern judge's and precision judge's calls while the case cost includes them, or eval:compare reports the Opus judge spend as review cost. #108: a /review explain run must write answer.json (and trace.json only when a call ran), set the answered output, and never write results.json. Each behavior moves, unchanged, into an exported function with tests.

   Context for the executor

   • Build: npm run typecheck
   • Test: npm test
   • Conventions: Node 24 runs TypeScript directly; tsconfig.json is strict with noUnusedLocals, so remove an import a step leaves unused. Tests use node:test and node:assert/strict, one behavior per test, hermetic (scratch folders from mkdtempSync(path.join(tmpdir(), ...))). Comments explain why, in plain sentences, above the function. UTF-8, LF.
   • Gotchas: node/npm may be missing from PATH; in each PowerShell command first run $env:PATH = "C:\Users\Marcin\AppData\Local\nvs\node\26.12.0\x64;$env:PATH". src/main.ts runs main() when imported, so no test may import it; evals/run.ts only runs main() when it is the entry script, so tests import it. Other open pull requests edit src/main.ts line 10 (the ./agent.ts import) and the review part of main(): do not touch those lines.

   Decisions

   • Kind: tests (labels are only follow-up; both titles ask for tests). The extractions are the smallest that let a test reach the behavior; no behavior or log text changes.
   • Test that judge spend stays out of the eval review cost #107: tallyCase mutates the CaseTally it is given (no I/O, deterministic) rather than returning a new one, so main() keeps one accumulator; saveVerdicts stays in main() because it is I/O.
   • Test /review explain's answer() directly #108: writeForPublish moves verbatim from src/main.ts to src/outputs.ts (next to assertNoSecrets, which it uses), because the new function in src/explain.ts must call it and src/main.ts can't be imported. teach() and the review keep calling it, now imported.
   • Test /review explain's answer() directly #108: the new writeAnswer in src/explain.ts takes the ref, the resolution and the ask callback and calls answerFor itself, so a test can show an unknown ref never asks. The two log lines about the call's outcome move into ask in src/main.ts, after the call, so src/main.ts keeps using totalCost and errorText and its ./agent.ts import line stays as it is; the log order is unchanged.
   • The answered flag is tested through the GITHUB_OUTPUT file, set and restored as test/outcomes.test.ts does around line 566.
   • Changelog: two ### Changed entries under ## [Unreleased]; no Release: line (release: none).

   Out of scope

   • Any behavior, log text, output file or field change; renames other than those named; formatting churn.
   • src/main.ts line 10 and the review part of main() (after the teach branch).
   • Tests of writeForPublish's secret scan (already covered through postAnswer and the publish step).

   Steps

   [ ] 1. Extract and test the eval's per-case tally

   • Issue: Test that judge spend stays out of the eval review cost #107
   • Files: evals/run.ts (edit), test/smoke-eval.test.ts (edit)
   • Change: In evals/run.ts, above const raisedLabel, add an exported interface CaseTally { precisions: CasePrecision[]; findingsPerCase: number[]; reviewCosts: number[] } and export function tallyCase(tally: CaseTally, outcome: { findings: readonly unknown[]; calls: readonly CallRecord[] }, calls: CallRecord[], precision: CasePrecision | undefined): { reviewCost: number; caseCost: number }. Its body, in this order: if (precision) tally.precisions.push(precision);, push outcome.findings.length to tally.findingsPerCase, const reviewCost = outcome.calls.reduce((sum, call) => sum + call.costUsd, 0);, push it to tally.reviewCosts, return { reviewCost, caseCost: totalCost(calls) }. Comment it: it adds one case to the run's tallies; the review cost is only the review's own calls (outcome.calls), while the case cost counts every call in calls, the concern judge's and precision judge's included, so eval:compare never reports the judges' Opus spend as review cost.
   • In main(): replace the three const precisions/findingsPerCase/reviewCosts lines with const tally: CaseTally = { precisions: [], findingsPerCase: [], reviewCosts: [] };. Replace the lines from if (precision) { through const caseCost = totalCost(calls); with if (precision) saveVerdicts(verdictFile, verdictCache); then const { reviewCost, caseCost } = tallyCase(tally, outcome, calls, precision);. In the summary use totalPrecision(tally.precisions), spread(tally.findingsPerCase), tally.reviewCosts.reduce((a, b) => a + b, 0) and spread(tally.reviewCosts).
   • In test/smoke-eval.test.ts: add tallyCase, type CaseTally to the ../evals/run.ts import, import type { CallRecord } from '../src/agent.ts'; and type CasePrecision to the ../evals/precision.ts import. Add a helper call(role: string, costUsd: number): CallRecord (provider 'bedrock', model 'm', tokens 0, turns 1, durationMs 1, outcome 'success') and two tests after the casePrecision tests:
     • tallyCase leaves the concern and precision judges out of the review cost and counts them in the case cost: review calls [call('security', 1), call('synthesis', 0.5)], calls = those plus call('judge', 2) and call('precision-judge', 4); expect { reviewCost: 1.5, caseCost: 7.5 } and tally.reviewCosts [1.5].
     • tallyCase adds each case's findings and its precision, when it has one, to the tallies: a precision { posted: 1, matched: 1, mustNot: 0, real: 0, notReal: 0, unsure: 0, precision: 1, judged: [] }; tally a case with one finding and that precision, then one with no findings and undefined; expect tally.precisions [precision] and tally.findingsPerCase [1, 0].
   • Pattern: the casePrecision tests in test/smoke-eval.test.ts.
   • Done when: npm run typecheck; if ($LASTEXITCODE) { exit 1 }; if (-not (Select-String -Path test/smoke-eval.test.ts -Pattern "^test\('tallyCase" -Quiet)) { exit 1 }; node --test test/smoke-eval.test.ts; exit $LASTEXITCODE

   [ ] 2. Move writeForPublish to outputs.ts

   • Issue: Test /review explain's answer() directly #108
   • Files: src/outputs.ts (edit), src/main.ts (edit)
   • Change: Cut writeForPublish and its one-line comment from src/main.ts and paste them, unchanged except for export, at the end of src/outputs.ts. Add import { appendFileSync, mkdirSync, writeFileSync } from 'node:fs'; and import path from 'node:path'; at the top of src/outputs.ts (after its header comment). In src/main.ts, change import { assertNoSecrets } from './outputs.ts'; to import { writeForPublish } from './outputs.ts';. Leave src/main.ts's node:fs import as it is (writeStatus and the pause still use it).
   • Pattern: none.
   • Done when: npm run typecheck; if ($LASTEXITCODE) { exit 1 }; if (-not (Select-String -Path src/outputs.ts -Pattern '^export function writeForPublish' -Quiet)) { exit 1 }; if (Select-String -Path src/main.ts -Pattern '^function writeForPublish' -Quiet) { exit 1 }; node --test test/explain.test.ts test/teach.test.ts test/pause.test.ts; exit $LASTEXITCODE

   [ ] 3. Extract and test writeAnswer

   • Issue: Test /review explain's answer() directly #108
   • Files: src/explain.ts (edit), src/main.ts (edit), test/explain.test.ts (edit)
   • Change: In src/explain.ts, import writeForPublish beside assertNoSecrets from ./outputs.ts, and after answerFor add export async function writeAnswer(outDir: string, where: { repository: string; number: number; question: string; secrets: string[] }, ref: FindingRef, resolution: Resolution, ask: (found: FoundFinding) => Promise<AgentResult<Explanation>>): Promise<void>. Body: the code now at the end of answer() in src/main.ts: const { body, explained } = await answerFor(ref, resolution, ask);, the files record with answerFile, then, when explained, files['trace.json'] = { explain: { id: found.id, comment: found.comment.html_url ?? found.comment.id, question: where.question, output: result.output, calls: result.records } }; (no logging), then writeForPublish(outDir, files, where.secrets, 'answered');. Comment: what a /review explain run leaves for the publish step: answer.json, plus trace.json when the agent was asked; an unknown ref writes only the answer, without asking; never results.json.
   • In src/main.ts answer(): inside ask, right after the Switched to Bedrock partway line, add the ::warning::The explain call failed line (if (result.error) ...), the const outcome = ... line and the console.log(\${outcome}, $${totalCost(...)...}`)line, moved from the end ofanswer()unchanged. Replace everything fromconst { body, explained } = await answerFor(to the end ofanswer()withawait writeAnswer(outDir, { repository, number, question: command.question, secrets }, command.ref, resolution, ask);. In the ./explain.tsimport dropanswerForandtype Answerand addwriteAnswer`.
   • In test/explain.test.ts: import writeAnswer, type Resolution, and readdirSync, readFileSync from node:fs. Add a helper answerRun(resolution: Resolution, result: AgentResult<Explanation>) that makes a scratch dir, sets process.env['GITHUB_OUTPUT'] to a file in it (restoring the old value in finally, as test/outcomes.test.ts does near line 566), calls writeAnswer(path.join(dir, 'out'), { repository: 'NCodeGroup/code-review', number: 7, question: 'why?', secrets: ['s'.repeat(20)] }, { kind: 'id', id: 'F3' }, resolution, async (f) => { asked.push(f); return result; }), and returns { outDir, files: readdirSync(outDir).sort(), outputs: <the GITHUB_OUTPUT file's text>, asked }. Tests:
     • writeAnswer answers an unknown ref with answer.json alone, without asking the agent: { validIds: ['F1'] }; asked [], files ['answer.json'], answer.json parses to { repository: 'NCodeGroup/code-review', pullRequest: 7, body: unknownRefBody({ kind: 'id', id: 'F3' }, ['F1']) }.
     • writeAnswer writes the trace of a call that ran beside the answer: { found } and agentResult({ answer: 'a', holds: true, reason: 'r' }); asked [found], files ['answer.json', 'trace.json'], trace.json parses to { explain: { id: 'F3', comment: found.comment.html_url ?? found.comment.id, question: 'why?', output: result.output, calls: result.records } }.
     • writeAnswer sets the answered output for the publish step: for both resolutions above, outputs is 'answered=true\n'.
     • writeAnswer never writes results.json, so the publish step posts no review: for both resolutions, files doesn't include 'results.json'.
   • Pattern: the answerFor and publishAnswer tests in test/explain.test.ts.
   • Done when: npm run typecheck; if ($LASTEXITCODE) { exit 1 }; if (Select-String -Path src/main.ts -Pattern 'answerFor|trace\.json' -Quiet) { exit 1 }; if (-not (Select-String -Path test/explain.test.ts -Pattern "^test\('writeAnswer" -Quiet)) { exit 1 }; node --test test/explain.test.ts test/pause.test.ts; exit $LASTEXITCODE

   [ ] 4. Record the changes in the changelog

   • Issue: Test that judge spend stays out of the eval review cost #107 Test /review explain's answer() directly #108
   • Files: CHANGELOG.md (edit)
   • Change: Under ## [Unreleased] → ### Changed, after the last entry (the one starting - The `api-compat` floor also reads), add these two lines, each as its own bullet:
     • - The tests check that the evals' review cost leaves out the concern judge's and precision judge's calls, which the case cost counts, so \eval:compare` never reports the judges' spend as review cost (#107).`
     • - The tests check what a \/review explain` run leaves for the publish step: only `answer.json` for an unknown finding, without a model call, `trace.json` too when the call ran, the `answered` output, and never `results.json` (#108).`
       (Write the backslash-escaped backticks as plain backticks.) Change no other section.
   • Pattern: the Test gaps declined during review rounds #96 entry in the same group.
   • Done when: if ((Select-String -Path CHANGELOG.md -Pattern 'issues/107\)\)\.' -Quiet) -and (Select-String -Path CHANGELOG.md -Pattern 'issues/108\)\)\.' -Quiet)) { npm test; exit $LASTEXITCODE } else { exit 1 }

   Review checklist

   • evals/run.ts main() writes the same summary fields (precision, findingsPerCase, reviewCostUsd, reviewCostPerCase, costUsd) and per-case reviewCostUsd/costUsd as before.
   • src/main.ts answer() logs the same lines in the same order, and writes the same answer.json/trace.json with the same answered output; teach() and the review still write through writeForPublish.
   • src/main.ts line 10 (./agent.ts import) is unchanged, so PR Trusted suggestions, safer verification, tracked findings apart #102 merges cleanly.
   • Tests restore GITHUB_OUTPUT and stay hermetic.
3. polewskm

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

   

   polewskm

   on Oct 31, 2026

   ContributorAuthor

   More actions

   Landed in #115. No follow-ups.