From 0afc12e026469727049152140ebf171eebb1eb7f Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Fri, 10 Jul 2026 13:45:36 -0700 Subject: [PATCH 1/3] [jwies/review-shed-tuning] review: budget and shed tuning from the 07-10 production sheds The 07-10 reviews on #232/#238 shed claim validation and three whole-change dimensions on substantial PRs. Recalibrate the tier budgets (overhead-aware wall clocks; low/medium invocation caps that fit the seven-reviewer roster), exempt pipeline steps from the invocation cap, gate shedding the claim-validator on hard-ceiling pressure only, and split the skipped-dimension note wording so a planned budget shed no longer reads as an infrastructure failure. This repo's ROUTING re-raises .claude/skills/** and eval plan docs from the broad trivial-docs rule. --- .changeset/review-shed-tuning.md | 5 +++++ .github/aw/review/ROUTING | 10 ++++++++- workflows/review/lib/router.ts | 35 ++++++++++++++++++++++++------- workflows/review/review.md | 36 +++++++++++++++++++++++++------- 4 files changed, 70 insertions(+), 16 deletions(-) create mode 100644 .changeset/review-shed-tuning.md diff --git a/.changeset/review-shed-tuning.md b/.changeset/review-shed-tuning.md new file mode 100644 index 00000000..31c6c0b9 --- /dev/null +++ b/.changeset/review-shed-tuning.md @@ -0,0 +1,5 @@ +--- +"review": patch +--- + +Budget and shed tuning from the 2026-07-10 production sheds (the reviews on Khan/actions #232 and #238, which shed claim validation and three whole-change dimensions on substantial PRs that had routed to the bottom tiers). Three changes. The tier budget table is recalibrated against measured runs: wall clocks now account for the ~3 minutes of fixed staging/router/triage overhead a run spends before the first reviewer returns (trivial 3->6, low 6->10, medium 12->15), and the low/medium invocation caps now fit the standard seven-reviewer whole-change roster (low 4->8, medium 8->10), which the old low cap of 4 deterministically shed on every run. The invocation cap is now explicitly finding-producers only: pattern-triage, thread-reconciler, and the claim-validator are pipeline steps and never consume a slot (the 07-10 runs counted them inconsistently). And the claim-validator moves to the back of the shed order with a hard-ceiling gate: it may be shed only when the job timeout or credits cap is genuinely close, never at a mere soft-target breach, since its cost tracks the visible candidate count rather than the diff. Skipped-dimension notes now distinguish the two causes: a planned budget shed reads `shed under the -tier run budget` and the `output unavailable` wording is reserved for a sub-agent that was dispatched but returned nothing parseable, so an operator can tell budget arithmetic from an infrastructure failure. (This repo's ROUTING also re-raises `.claude/skills/**` and `workflows/review/eval/**/*.md` from the broad trivial-docs rule; that file is consumer config, not part of the package.) diff --git a/.github/aw/review/ROUTING b/.github/aw/review/ROUTING index fef3c570..a2a61a47 100644 --- a/.github/aw/review/ROUTING +++ b/.github/aw/review/ROUTING @@ -29,7 +29,7 @@ workflows/review/eval/** tier=low # are re-raised after the broad docs rule (last match wins). Any shared workflow's # prompt (and prompt imports beside it) is high; the docs living in the same # directory drop back to trivial. Single-star segments do not cross into -# subdirectories, so nothing under eval/ gets re-raised. +# subdirectories. **/*.md tier=trivial .changeset/*.md tier=trivial workflows/*/*.md tier=high @@ -37,6 +37,14 @@ workflows/*/README.md tier=trivial workflows/*/CHANGELOG.md tier=trivial .github/workflows/*.md tier=high +# More executable-not-docs prose the broad docs rule would drop to trivial: +# skills steer the Claude agents that operate on this repo, and eval plan docs +# drive the eval suite's implementation, so they keep the eval dir's dev-only +# tier. (The 2026-07-10 reviews on #232/#238 routed both as trivial docs and +# shed review dimensions for it.) +.claude/skills/** tier=medium +workflows/review/eval/**/*.md tier=low + # The reviewer's own consumer config: config.md carries the add-reviewer team # allowlist and bot token, ROUTING sets these tiers, and the prose imports steer # every sub-agent. A PR tampering with any of them must not run at the trivial diff --git a/workflows/review/lib/router.ts b/workflows/review/lib/router.ts index 28996187..26f46181 100644 --- a/workflows/review/lib/router.ts +++ b/workflows/review/lib/router.ts @@ -157,13 +157,23 @@ export type RunBudget = { tier: RiskTier; /** True when the misrouted floor lifted the budget above the touched tier. */ floored: boolean; - /** Upper bound on specialist+whole-change reviewer invocations for the run. */ + /** + * Upper bound on specialist+whole-change reviewer invocations for the + * run. Only finding-producing dispatches count; pipeline steps + * (`pattern-triage`, `thread-reconciler`, `claim-validator`) never + * consume a slot. + */ maxReviewerInvocations: number; /** Upper bound on investigation tool calls a single finding may spend. */ maxToolCallsPerFinding: number; /** Upper bound on investigation tool calls across the whole run. */ maxTotalToolCalls: number; - /** Soft wall-clock ceiling (minutes) the orchestrator should target. */ + /** + * Soft wall-clock ceiling (minutes) the orchestrator should target, + * measured from run start; it therefore includes the run's fixed + * staging/router/triage overhead (~3 minutes measured), not just + * reviewer time. + */ maxWallClockMinutes: number; /** Soft spend ceiling (USD) the orchestrator should target. */ maxUsd: number; @@ -254,6 +264,17 @@ export type RouteInput = { * per-run ceiling. Every field scales monotonically with the tier, and the * table is exported so the eval suite and consumers can override it without a * code change. + * + * Calibration (re-measured 2026-07-10 against production runs on + * Khan/actions #232/#238): a run spends ~3 minutes of fixed overhead + * (staging, router, provenance, pattern-triage) before the first reviewer + * returns, so wall clocks below ~6 minutes put a run past its shed threshold + * before any review work lands; and the standard enabled roster is seven + * whole-change reviewers (two defaults plus the five eval-justified opt-ins + * both current consumers enable), so an invocation cap of 4 deterministically + * shed dimensions on every low-tier run. Low and medium now fit the roster; + * trivial stays deliberately small (a trivial-only PR gets the two default + * reviewers, with the rest declared as budget sheds). */ export const DEFAULT_TIER_BUDGETS: Record = { trivial: { @@ -262,25 +283,25 @@ export const DEFAULT_TIER_BUDGETS: Record = { maxReviewerInvocations: 2, maxToolCallsPerFinding: 2, maxTotalToolCalls: 10, - maxWallClockMinutes: 3, + maxWallClockMinutes: 6, maxUsd: 0.5, }, low: { tier: "low", floored: false, - maxReviewerInvocations: 4, + maxReviewerInvocations: 8, maxToolCallsPerFinding: 3, maxTotalToolCalls: 20, - maxWallClockMinutes: 6, + maxWallClockMinutes: 10, maxUsd: 1.5, }, medium: { tier: "medium", floored: false, - maxReviewerInvocations: 8, + maxReviewerInvocations: 10, maxToolCallsPerFinding: 5, maxTotalToolCalls: 60, - maxWallClockMinutes: 12, + maxWallClockMinutes: 15, maxUsd: 4, }, high: { diff --git a/workflows/review/review.md b/workflows/review/review.md index 4d4c88ba..23cc9067 100644 --- a/workflows/review/review.md +++ b/workflows/review/review.md @@ -727,8 +727,10 @@ estimate dollars; watch the signals you can observe, as spend proxies: against the run start you recorded in Step 1 at each later checkpoint. This is the sharpest proxy, and the job-timeout ceiling it guards is just as fatal as the credits cap. -- **Dispatch count** vs `runBudget.maxReviewerInvocations`: reviewers and lenses - already dispatched plus still pending. +- **Dispatch count** vs `runBudget.maxReviewerInvocations`: finding-producing + reviewers and lenses already dispatched plus still pending. Only those count. + `pattern-triage`, `thread-reconciler`, and the `claim-validator` are pipeline + steps, not reviewers; they never consume a slot of this cap. - **Run-wide investigation usage** vs `runBudget.maxTotalToolCalls`: one line per authorised call in `/tmp/gh-aw/review/investigation-journal.log` (`wc -l`). - **Trajectory**: an unusually large diff, many sub-agents still pending, many @@ -742,8 +744,16 @@ order: a skipped dimension (Step 6 note). 2. Skip the risks/patterns comment and reviewer requests (Steps 7-8) if they have not happened yet. -3. If the `claim-validator` has not run, post the unvalidated candidates under the - existing missing-validator rule (Phase 3) with its skipped-dimension note. +3. Last, and never at the soft targets alone: the `claim-validator`. It is the + false-positive gate, and its cost scales with the candidate count (which you + can already see when deciding), not with the diff, so validating a small + candidate set costs less than one reviewer dispatch. Shed it only when a + hard ceiling is genuinely close (elapsed wall clock past three-quarters of + the job's `timeout-minutes`, or an equally direct signal that the credits + cap is near); at a mere soft-target breach, dispatch it anyway and shed + elsewhere. When it is shed, post the unvalidated candidates under the + missing-validator rule (Phase 3), using the planned-shed wording of the + skipped-dimension note (Step 6). Then go straight to Steps 4-6: compute the verdict from the findings already validated, post the surviving comments, and submit the review with one @@ -1017,10 +1027,20 @@ keep the body to a single line: Changes requested — see inline comments. ``` -**Skipped dimensions (either verdict).** If a sub-agent's output was unavailable this -run so a dimension could not be assessed (Step 3), append to the review body — after -any verdict-specific text above — one line per skipped dimension, exactly: -`Note: not assessed this run ( output unavailable).` If the +**Skipped dimensions (either verdict).** If a dimension could not be assessed this +run (Step 3), append to the review body — after +any verdict-specific text above — one line per skipped dimension, choosing the +wording by cause: + +- Planned shed (the budget rule stopped the sub-agent from being dispatched): + `Note: not assessed this run (shed under the -tier run budget).` +- The sub-agent was dispatched but its output was missing or unparseable: + `Note: not assessed this run ( output unavailable).` + +The two read very differently to an operator (a shed is budget arithmetic and +expected on small-tier runs; an unavailable output is a failure worth +investigating), so never use the `unavailable` wording for work you chose not +to start. If the change-provenance gate was skipped because `provenance.json` was missing or carried warnings (Step 3), also append exactly: `Note: change-provenance gate skipped this run (diff staging unparseable).` From 651912b1798f237f6c0895dc4a907a47121b8e15 Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Mon, 13 Jul 2026 10:50:46 -0700 Subject: [PATCH 2/3] [jwies/review-shed-tuning] review: lock the invocation caps to the roster size and document the calibration choices --- workflows/review/lib/router.test.ts | 16 ++++++++++++++++ workflows/review/lib/router.ts | 6 +++++- 2 files changed, 21 insertions(+), 1 deletion(-) diff --git a/workflows/review/lib/router.test.ts b/workflows/review/lib/router.test.ts index cb316637..fa1fbb52 100644 --- a/workflows/review/lib/router.test.ts +++ b/workflows/review/lib/router.test.ts @@ -5,6 +5,7 @@ import { computeRunBudget, DEFAULT_MISROUTED_FLOOR_TIER, DEFAULT_TIER_BUDGETS, + ENABLEABLE_REVIEWERS, isGenerated, matchesGlob, parseGitattributesGenerated, @@ -384,6 +385,21 @@ describe("computeRunBudget: scaling", () => { expect(result.runBudget.tier).toBe("high"); expect(result.runBudget.floored).toBe(false); }); + + it("fits the whole-change roster within the low and medium caps", () => { + // Two always-on defaults (correctness-reviewer, skill-auditor; the + // other pipeline steps never consume a slot) plus every opt-in + // reviewer. Pinned to the roster size, not a literal: if a reviewer + // joins ENABLEABLE_REVIEWERS without a cap bump, low/medium runs + // would silently shed dimensions again while the suite stayed green. + const rosterSize = 2 + ENABLEABLE_REVIEWERS.length; + expect( + DEFAULT_TIER_BUDGETS.low.maxReviewerInvocations, + ).toBeGreaterThanOrEqual(rosterSize); + expect( + DEFAULT_TIER_BUDGETS.medium.maxReviewerInvocations, + ).toBeGreaterThanOrEqual(rosterSize); + }); }); /* -------------------------------------------------------------------------- */ diff --git a/workflows/review/lib/router.ts b/workflows/review/lib/router.ts index 26f46181..feaa6cfc 100644 --- a/workflows/review/lib/router.ts +++ b/workflows/review/lib/router.ts @@ -274,7 +274,11 @@ export type RouteInput = { * both current consumers enable), so an invocation cap of 4 deterministically * shed dimensions on every low-tier run. Low and medium now fit the roster; * trivial stays deliberately small (a trivial-only PR gets the two default - * reviewers, with the rest declared as budget sheds). + * reviewers, with the rest declared as budget sheds). Raising the low cap to + * 8 is deliberate: a low-tier run can dispatch the full roster, so only + * trivial sheds by design. `maxUsd` is deliberately left uncalibrated + * (per-run cost measurement is deferred to #249), so the dollar column is + * still the original estimate, not a re-measured figure. */ export const DEFAULT_TIER_BUDGETS: Record = { trivial: { From 61ff935b32fe543e53e8ebd69ef71c78a923937b Mon Sep 17 00:00:00 2001 From: James Wiesebron Date: Mon, 13 Jul 2026 11:28:10 -0700 Subject: [PATCH 3/3] [jwies/review-shed-tuning] review: the low cap fits the lens-free roster; lens-heavy low PRs still shed (review feedback) --- workflows/review/lib/router.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/workflows/review/lib/router.ts b/workflows/review/lib/router.ts index feaa6cfc..7d97134e 100644 --- a/workflows/review/lib/router.ts +++ b/workflows/review/lib/router.ts @@ -275,8 +275,11 @@ export type RouteInput = { * shed dimensions on every low-tier run. Low and medium now fit the roster; * trivial stays deliberately small (a trivial-only PR gets the two default * reviewers, with the rest declared as budget sheds). Raising the low cap to - * 8 is deliberate: a low-tier run can dispatch the full roster, so only - * trivial sheds by design. `maxUsd` is deliberately left uncalibrated + * 8 is deliberate: a lens-free low-tier run can dispatch the full roster. + * Path-matched specialist lenses also consume slots, so a low PR matching + * two or more lenses still sheds from the bottom of the value ranking; that + * residual is sized when the modes are priced, not here. `maxUsd` is + * deliberately left uncalibrated * (per-run cost measurement is deferred to #249), so the dollar column is * still the original estimate, not a re-measured figure. */