Activity

1. polewskm

   added

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

   on Nov 1, 2026
2. polewskm commented on Nov 1, 2026

   

   polewskm

   on Nov 1, 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: Stop caching the one-request calls (2 steps)

   Plan: Stop caching the one-request calls

   Issue: #133

   Goal

   #128 turned prompt caching on for the context, selection and synthesis calls on the assumption that their structured output takes a second request that reads the first's cache. Traces and eval outputs since v1.0.0 show it doesn't: those calls make one request, write their whole prompt to the cache at 1.25× the input price, and read back under 1% of it. Reviewer followers do read their leader's cache, as #128 claimed. This plan turns caching off for the three tool-less one-shot calls and records the measurements (#133).

   Context for the executor

   • Build: npm run typecheck
   • Test: npm test (one file: node --test test/<name>.test.ts)
   • Node is not on PATH on this machine: start every command that runs node or npm with $env:PATH = "C:\Users\Marcin\AppData\Local\nvs\node\24.22.2\x64;$env:PATH";.
   • Conventions: Node 24 runs TypeScript directly; strict TypeScript with exactOptionalPropertyTypes. Tests use node:test, one behavior per test. LF line endings, UTF-8 without BOM. Refer to issues in code comments as issue #N.
   • Gotchas: AgentRequest.cache (src/agent.ts) is already documented as "False only for a call that makes one model request and whose prompt nothing else reuses"; agentEnvironment sets DISABLE_PROMPT_CACHING=1 when it is false. test/depth.test.ts's run harness records each request in calls.

   Decisions

   • cache: false on the requests with role context (src/context.ts), selection and shadow-prompt:selection, and synthesis and shadow-prompt:synthesis (src/review.ts). Each is tool-less (withoutTools), makes one model request, and shares no prefix with another call: a shadow variant has its own system prompt, which comes first.
   • Calls with tools (reviewers, re-review reviewers, explain, verification) keep caching: their turns are real requests, and reviewer followers and verification batches read a shared prefix.
   • Evidence (24 reviews in the 2026-11-01 eval outputs plus the v1.0.0 review of Run a plain /review only on an open pull request #144, run 41286048709): context wrote 2,387,348 tokens and read 15,479; selection 464,201 and 14,743; synthesis 736,932 and 75,132; reviewers that followed a leader read 30,005,330 against 3,535,280 written, in 395 of 396 calls. In Run a plain /review only on an open pull request #144's trace context, selection and synthesis each took 2 SDK turns but one request (their duration equals their first response's time). The reviewer phase on the smoke cases took 45–62 s (the large ones) and 18–29 s (the small one); production's week of traces stays to be read.

   Out of scope

   • Any call with tools; the cache breakpoints in promptInput; the reviewer scheduling.

   Steps

   [ ] 1. Turn caching off for the context, selection and synthesis calls

   • Issue: Confirm the scheduling and caching gains in production traces #133
   • Files: src/context.ts (edit), src/review.ts (edit), test/depth.test.ts (edit)
   • Change: Add cache: false, to the context request in src/context.ts (the object with role: 'context'), and to the objects selectionRequest and synthesisRequest return in src/review.ts, each with the comment // One request, whose prompt nothing else reuses: caching would pay to write it and never read it (issue #133). (once per request). In test/depth.test.ts, add cache: boolean | undefined to the Call interface and cache: request.cache to what the harness records, and add the test "the context, selection and synthesis calls don't cache their prompts, and the reviewers do": run a standard review on src/Payments/Ledger.cs with one should-fix from reviewer:correctness (as the file's other tests do; the harness's context call only runs when there are rules or decisions, so if no context call is recorded, assert only on the ones recorded) and assert every recorded call with role selection or synthesis (and context, if any) has cache === false, and every reviewer:* call has cache !== false.
   • Done when: $env:PATH = "C:\Users\Marcin\AppData\Local\nvs\node\24.22.2\x64;$env:PATH"; node --test test/depth.test.ts; if ($LASTEXITCODE) { exit 1 }; npm run typecheck; if ($LASTEXITCODE) { exit 1 }; if ((Select-String -Path src/context.ts, src/review.ts -Pattern 'cache: false,').Count -lt 3) { exit 1 }

   [ ] 2. Record the trace measurements and the change

   • Issue: Confirm the scheduling and caching gains in production traces #133
   • Files: README.md (edit), CHANGELOG.md (edit)
   • Change: In README's ## Timings, after the paragraph that starts "Each model call's record", add one paragraph (one long line) with the Decisions' evidence: what v1.0.0's traces and the 2026-11-01 eval outputs showed about the cache (followers read their leader's cache: 30.0M tokens read against 3.5M written over 395 reviewer calls; context, selection and synthesis make one request each, so their 3.6M tokens written were read back under 3%), that these three calls therefore don't cache, and that the reviewer phase took 45–62 s on the large smoke cases; production's reviewer phase is to be read from a week of v1 traces with node tools/timings.ts. Link run 41286048709 (https://github.com/NCodeGroup/code-review/actions/runs/41286048709). In CHANGELOG.md, under ## [Unreleased], add a ### Fixed group after ### Changed (create it if missing) with: - The context, selection and synthesis calls no longer cache their prompts: each makes one request, so caching paid 25% more to write about 150,000 tokens a review that nothing read. Reviewer followers keep reading their leader's cache, as the traces confirmed ([#133](https://github.com/NCodeGroup/code-review/issues/133)).
   • Done when: $env:PATH = "C:\Users\Marcin\AppData\Local\nvs\node\24.22.2\x64;$env:PATH"; if (-not (Select-String -Path README.md -Pattern 'actions/runs/41286048709' -Quiet)) { exit 1 }; if (-not (Select-String -Path CHANGELOG.md -Pattern 'issues/133' -Quiet)) { exit 1 }; npm test

   Review checklist

   • Only tool-less one-request calls stop caching; every call with tools still caches.
   • The README's numbers match the Decisions.
3. polewskm commented on Nov 1, 2026

   

   polewskm

   on Nov 1, 2026

   ContributorAuthor

   More actions

   Update from #145. Both cache claims are now checked, against v1.0.0's trace of #144 and 24 reviews from the 2026-11-01 evals:

   • Reviewer followers read their leader's cache: confirmed. 30.0M tokens read against 3.5M written, in 395 of 396 reviewer calls.
   • Context, selection and synthesis read their first turn's cache on the second turn: refuted. Each makes one request. They wrote 3.6M tokens to the cache and read back under 3%, so Stop caching the one-request calls #145 turned caching off for them, which saves about .08 a review.

   Still open: the production reviewer phase. On the smoke cases it took 45–62 s on the large diffs and 18–29 s on the small one; #144's small review took 13.7 s. Production needs a week of v1 traces, around 2026-11-08, read with node tools/timings.ts as the README's Timings section says. This issue stays open for that one check.
4. polewskm commented on Nov 1, 2026

   

   polewskm

   on Nov 1, 2026

   ContributorAuthor

   More actions

   Closing. The actionable part is done (#145). The traces confirmed reviewer followers read their leader's cache. They refuted the second-turn cache reads for context, selection and synthesis, and caching is off for those three now. The one check left was reading a week of production traces for the reviewer phase. That is a measurement, not a change: the README's Timings section says how to run it (node tools/timings.ts over downloaded code-review-trace artifacts), and the monthly report tracks cost. If a later reading shows a regression, it gets an issue of its own with the numbers.
5. polewskm

   closed this as completedon Nov 1, 2026