review: budget and shed tuning from the 07-10 production sheds #258
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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>-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.) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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. 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, | ||
| * 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<RiskTier, RunBudget> = { | ||
| trivial: { | ||
|
|
@@ -262,25 +283,25 @@ export const DEFAULT_TIER_BUDGETS: Record<RiskTier, RunBudget> = { | |
| maxReviewerInvocations: 2, | ||
| maxToolCallsPerFinding: 2, | ||
| maxTotalToolCalls: 10, | ||
| maxWallClockMinutes: 6, | ||
| maxUsd: 0.5, | ||
| }, | ||
| low: { | ||
| tier: "low", | ||
| floored: false, | ||
| maxReviewerInvocations: 8, | ||
|
Contributor
There was a problem hiding this comment. suggestion (non-blocking): The roster-fitting caps are the point of this change, yet the budget tests ( const rosterSize = 2 + ENABLEABLE_REVIEWERS.length;
expect(DEFAULT_TIER_BUDGETS.low.maxReviewerInvocations).toBeGreaterThanOrEqual(rosterSize);
expect(DEFAULT_TIER_BUDGETS.medium.maxReviewerInvocations).toBeGreaterThanOrEqual(rosterSize);
Contributor
There was a problem hiding this comment. thought (non-blocking): The 07-10 root cause was misrouting, which this PR's ROUTING re-raises already address. Raising the low cap 4 → 8 additionally decides that a correctly-routed low-risk PR should run the full seven-reviewer roster (low 8 vs medium 10 nearly collapses the two tiers on this field). That may well be intended, but it is a broader call than the two incidents evidence — worth a line in the calibration comment if "only trivial sheds by design" is the goal. |
||
| maxToolCallsPerFinding: 3, | ||
| maxTotalToolCalls: 20, | ||
| maxWallClockMinutes: 10, | ||
|
Contributor
There was a problem hiding this comment. note (non-blocking): The PR description explains |
||
| maxUsd: 1.5, | ||
| }, | ||
| medium: { | ||
| tier: "medium", | ||
| floored: false, | ||
| maxReviewerInvocations: 10, | ||
| maxToolCallsPerFinding: 5, | ||
| maxTotalToolCalls: 60, | ||
| maxWallClockMinutes: 15, | ||
| maxUsd: 4, | ||
| }, | ||
| high: { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick (non-blocking): This pattern is wider than the
tier=alignment column the rest of the file keeps, sotier=lowlands one space off the grid. Accept it as the intentional outlier, or widen the column across all rules.Low-confidence (1)
workflows/review/lib/router.ts:286— Baking the measured ~3 min overhead into every tier's wall clock is fragile; measuring elapsed time from triage-end (a second timestamp) instead would keep the table overhead-invariant.