Skip to content

review: budget and shed tuning from the 07-10 production sheds#258

Open
jwbron wants to merge 1 commit into
mainfrom
jwies/review-shed-tuning
Open

review: budget and shed tuning from the 07-10 production sheds#258
jwbron wants to merge 1 commit into
mainfrom
jwies/review-shed-tuning

Conversation

@jwbron

@jwbron jwbron commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary:

The 2026-07-10 reviewer runs on #238 (review) and #232 (review) shed claim validation and three whole-change dimensions on substantial PRs, and reported the sheds with wording that reads as infrastructure failure. Root causes, from the run logs: both PRs misrouted to the bottom tiers via the broad **/*.md tier=trivial rule; the tier wall clocks ignore the ~3 minutes of fixed staging/router/triage overhead, so a low-tier run was at 80% of its soft budget the moment its reviewers returned; the low-tier invocation cap of 4 cannot fit the seven-reviewer whole-change roster this repo (and webapp) enables, so every low-tier run shed dimensions by arithmetic; and the two runs counted the cap inconsistently (#238 charged pattern-triage against it, #232 did not).

Four changes:

  • ROUTING (repo-local config, not the package): re-raise .claude/skills/** (medium) and workflows/review/eval/**/*.md (low) from the trivial-docs rule; both are executable-not-docs prose.
  • Budget table (router.ts): overhead-aware wall clocks (trivial 3->6, low 6->10, medium 12->15) and roster-fitting invocation caps (low 4->8, medium 8->10). Trivial stays deliberately small. maxUsd is untouched: those numbers are also unrealistic (measured runs cost $3.2-3.6 against a $0.5-1.5 target), but spend is unobservable mid-run today and review: clamp the run budget to the effective credit cap; land before the cap #249 is building the credits-cap machinery that makes it observable, so recalibrating dollars belongs there.
  • Cap accounting (review.md): the invocation cap counts finding-producers only; pattern-triage, thread-reconciler, and claim-validator are pipeline steps and never consume a slot.
  • Shed order and note wording (review.md): the claim-validator may be shed only under genuine hard-ceiling pressure (three-quarters of timeout-minutes, or a direct credits-cap signal), never at a mere soft-target breach; its cost tracks the visible candidate count, not the diff. Skipped-dimension notes now distinguish shed under the <tier>-tier run budget from <sub-agent> output unavailable, so budget arithmetic no longer masquerades as failure.

Interaction with #249: complementary. #249 clamps the selected tier budget to the effective credit cap; this PR resizes the un-clamped table and the shed discipline. Expect a textual merge conflict in the review.md graceful-landing section, whichever lands second.

Test plan:

  • pnpm test (656 tests, all passing; the budget tests assert monotonicity and tier identity, both preserved).
  • pnpm exec prettier --check on the touched TS.
  • Glob semantics of the new ROUTING rules verified against globToRegExp (**/ collapses to zero-or-more segments, so workflows/review/eval/**/*.md matches live-ab-plan.md at the eval root).
  • The next reviewer runs on doc-heavy PRs should show: skills/eval-doc PRs routing medium/low instead of trivial, full roster dispatch at low tier, and shed notes (if any) carrying the planned-shed wording.

…-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.
@jwbron jwbron self-assigned this Jul 10, 2026
@changeset-bot

changeset-bot Bot commented Jul 10, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 0afc12e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
review Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@khan-actions-bot khan-actions-bot requested review from a team, jeresig and kevinb-khan and removed request for a team July 10, 2026 20:46
@github-actions

Copy link
Copy Markdown
Contributor

Review Guidance

github-actions (3 files)
File Reason
ROUTING Consumer config that sets the per-file risk tiers gating every review's budget and shed behavior; a mistake here misroutes all future reviews.
router.ts The shared reviewer's deterministic budget table; these numbers bound the work every consuming repo's review is allowed to do.
review.md The shared orchestrator prompt; its shed-order and cap-accounting prose is the reviewer's behavior in every consuming repo.

Common patterns

2 files: The pipeline-steps-don't-consume-a-cap-slot carve-out (pattern-triage, thread-reconciler, claim-validator) is stated once in the router.ts JSDoc and once in the review.md prose.

Excluded from review (1 file)

Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:

  • .changeset/review-shed-tuning.md — pattern-only (changeset/release-notes entry)

tier: "low",
floored: false,
maxReviewerInvocations: 4,
maxReviewerInvocations: 8,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion (non-blocking): The roster-fitting caps are the point of this change, yet the budget tests (router.test.ts) only assert monotonicity and tier identity — nothing ties maxReviewerInvocations to the roster it must accommodate. If a sixth entry is later added to ENABLEABLE_REVIEWERS (roster → 8) without bumping this cap, low-tier runs silently shed a dimension again — the exact regression this PR fixes — and the suite stays green. Consider locking the relationship down:

const rosterSize = 2 + ENABLEABLE_REVIEWERS.length;
expect(DEFAULT_TIER_BUDGETS.low.maxReviewerInvocations).toBeGreaterThanOrEqual(rosterSize);
expect(DEFAULT_TIER_BUDGETS.medium.maxReviewerInvocations).toBeGreaterThanOrEqual(rosterSize);

tier: "low",
floored: false,
maxReviewerInvocations: 4,
maxReviewerInvocations: 8,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: 6,
maxWallClockMinutes: 10,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

note (non-blocking): The PR description explains maxUsd is deliberately left uncalibrated (deferred to #249). Since this calibration comment is where the next tuner will look, consider mirroring that one line here too — otherwise the table reads as fully "recalibrated against measured runs" with one field silently untouched.

Comment thread .github/aw/review/ROUTING
# 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

Copy link
Copy Markdown
Contributor

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, so tier=low lands 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant