review: value-ranked shedding and dispatch; never shed reviewer requests#260
Conversation
…-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.
…h; never shed reviewer requests The shed list's first bucket had no ordering, so a budget-pressed run shed arbitrarily and an under-cap dispatch filled slots by enablement mechanism instead of value. Rank both: shed generic dimensions first (conventions, first-principles, holistic, completeness/test-adequacy) and matched specialist lenses last; dispatch in the reverse order when the invocation cap cannot fit the roster. Reviewer requests (Step 8) leave the shed list entirely: one tool call, and the human fallback matters most when the bot's own coverage is partial.
🦋 Changeset detectedLatest commit: 1e7b4f9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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
requested review from
a team,
jeresig and
kevinb-khan
and removed request for
a team
Review Guidancegithub-actions (1 file)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
…urate cross-reference, provisional-ordering note, scoped step 8 rationale, minor bump)
| (one turn), and wait for all. If `runBudget.maxReviewerInvocations` cannot fit | ||
| that whole set, fill the slots by the dispatch ranking (the budget rule below: | ||
| Step 3, graceful-landing bucket 1): defaults first, then matched lenses, then | ||
| the targeted opt-in dimensions, then the generic ones. Never choose arbitrarily, and record every |
There was a problem hiding this comment.
suggestion (non-blocking): The dispatch order here — "the targeted opt-in dimensions, then the generic ones" — is offered as bucket 1's shed ranking read from the other end, but bucket 1 spells out a five-item order (conventions < first-principles < holistic < completeness/test-adequacy < lenses) while this side collapses the non-lens opt-ins into two tiers that are never bound to named dimensions. On a budget-pressed run that fits only some of {holistic, first-principles, conventions}, the dispatch rule can't say which to keep even though the shed rule can — the same "choose arbitrarily" gap this PR closes on the shed side. Consider stating the dispatch tiers at bucket 1's granularity:
| the targeted opt-in dimensions, then the generic ones. Never choose arbitrarily, and record every | |
| the targeted opt-in dimensions (completeness/test-adequacy), then the generic ones (holistic, then first-principles, then conventions) — the exact reverse of the Step 3 bucket-1 shed order. Never choose arbitrarily, and record every |
| The ranking, from first-shed to last-shed: `conventions`, then | ||
| `first-principles`, then `holistic`, then `completeness` and | ||
| `test-adequacy`, and only then any path-triggered specialist lens from | ||
| `lensesToSpawn`. A matched lens is the most targeted signal in the run (the |
There was a problem hiding this comment.
thought (non-blocking): "A matched lens ... outranks every generic dimension" treats a path-glob match as proof of value, but lensesToSpawn is a boolean glob union (router.ts lensesForFile) with no measure of match breadth — a lens triggered by one incidental file outranks completeness/test-adequacy just the same. That's the same "slots filled by mechanism, not value" failure this change fixes, with the mechanism inverted from enablement to path-match. Might be worth naming match breadth as a known blind spot for the eval-corpus replacement to address.
…' into jwies/review-shed-ranking # Conflicts: # workflows/review/lib/router.ts # workflows/review/review.md
| not happened yet. | ||
| 1. Skip not-yet-dispatched opt-in reviewers and specialist lenses in value | ||
| order, lowest value first; each becomes a skipped dimension (Step 6 note). | ||
| The ranking, from first-shed to last-shed: `conventions`, then |
There was a problem hiding this comment.
note (non-blocking): This ranking hard-codes the five current opt-in reviewers by name with no fallback position for a future one. A newly added opt-in reviewer would be unranked here, quietly reintroducing the arbitrary shed this change removes — until someone remembers to edit this prose. Consider stating where an unranked reviewer sheds (e.g. after conventions, before the lenses) so new reviewers are never unranked.
Review live A/BBaseline:
Regressions (baseline caught, candidate missed)
Adversarial hard gate: FAILED on the candidate arm
Gate flips retried (best of three, flipped cases only)
Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. |
2 participants
Summary:
Stacked on #258 (it rewrites the same graceful-landing text; this PR retargets to main automatically when #258 merges).
Follow-up from the 07-10 shed analysis: the shed list's first bucket ("skip any not-yet-dispatched opt-in reviewers and lenses") had no ordering, and Phase 2 gave no rule for choosing what to dispatch when the invocation cap cannot fit the roster. The 07-10 runs showed both gaps: slots filled by enablement mechanism rather than value, and nothing preventing a matched specialist lens (the most targeted signal in a run) from being shed to afford
conventions.Three changes, all review.md prose:
conventions,first-principles,holistic,completeness/test-adequacy, then path-triggered specialist lenses last.maxReviewerInvocationscannot fit the full roster (defaults, matched lenses, targeted opt-ins, generic opt-ins), with every undispatched reviewer recorded as a planned shed instead of chosen arbitrarily.The ranking is a first-cut editorial ordering. The corpus records which producer catches which must-catch spec, so it should be replaced by measured per-dimension catch contribution once that data is compiled; the changeset says so.
Deliberately not included: any change to the APPROVE event on heavily-shed runs (whether a partial review should submit COMMENT instead of APPROVE past some shed threshold). That changes what the green check means to every consumer, so it is queued as a human decision (round-three Q6) rather than shipped here.
Test plan:
pnpm test(656 passing; prose-only change plus changeset, no code touched).add_reviewercall surviving even on shed runs with risky files.