[pipeline-dcdad92d][slice-6/12] Roster framework: always-on... by jwbron · Pull Request #201 · Khan/actions

jwbron · 2026-07-03T18:58:48Z

Define the always-on reviewers (holistic, completeness, test-adequacy, first-principles, conventions) with their trust/advisory constraints, apply the model launch defaults + effort table, and wire the kept gates to the new roster.

Base PR: #196

What's in this PR

Commits (2):

Persist BRC history for slice-6 (#2548)
implement(documenter): slice-6 roster framework — always-on reviewers, model+effort defaults, kept gates

.egg-state/brc-history/pipeline-dcdad92d-implement-slice-6.json | 620 ++++++++++++++++++++++++++++
 .egg-state/brc-history/pipeline-dcdad92d-implement-slice-6.md   | 584 ++++++++++++++++++++++++++
 workflows/review/review.md                                      | 532 ++++++++++++++++++++++--
 3 files changed, 1695 insertions(+), 41 deletions(-)

This slice

Roster framework: always-on reviewers, model defaults, kept gates

Files affected:

workflows/review/review.md

Tasks (4) + acceptance criteria

task-6-1: Always-on reviewers: holistic; completeness (Jira/Confluence read-only inside the non-posting sub-agent; fetched text is data under review); test-adequacy; first-principles (advisory-only, never blocks, Fable 5 day one); conventions (advisory; router-gated by greppable trigger signatures).
- Acceptance criteria: Five always-on reviewers defined with their trust/advisory constraints.
task-6-2: Prompt edit 14: named mandates for holistic / completeness / first-principles.
- Acceptance criteria: Mandates present.
task-6-3: R12 model launch defaults + effort table: Opus 4.8 workhorse; medium = triage/reconciliation; high = lenses/whole-change; xhigh = security lens + claim-validator/refuters; first-principles = Fable 5. Do NOT re-pin review: skip redundant approvals, drop skipped-dimensions note, persist sub-agent outputs #194's existing model pins -- extend the effort assignments.
- Acceptance criteria: Every role carries its launch-default model + effort matching the R12 table.
task-6-4: Kept gates wired to the new roster: pattern-triage (exclusions in guidance comment), claim-validator + refuter panel (batched/parallel), deterministic dedup + verdict bookends (verdict from slice 2), thread-reconciler.
- Acceptance criteria: Gates preserved; no §3 regression.

Stack

Position: slice 6 of 12 in pipeline pipeline-dcdad92d
Base PR: review: roster restructure, determinism boundary, eval suite #196
Stacked on top of egg/pipeline-dcdad92d/slice-5

changeset-bot · 2026-07-03T18:58:52Z

🦋 Changeset detected

Latest commit: 8f8cb7c

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

This PR includes changesets to release 1 package

Name	Type
review	Minor

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

…ing (slice-1) R1 (task-1-2): standardize Step 6 review submission on one robust submit-pull-request-review call with a real, non-empty body; remove the empty-body branch and any fallback/retry path. E2 (task-1-3): stage /tmp/gh-aw/review/pr-context.json in Step 1 and wire all six sub-agent read blocks to consume it. Extends #194 diff staging; foundation for slice-7 lenses. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

task-1-1: workflows/review/lib/finding-schema.ts - FINDING_SCHEMA_VERSION exported constant; each finding carries schema_version and the validator rejects an unrecognized version (breaking-shape guard). - Finding shape (snake_case wire keys, matching #194 per-run JSON artifacts): id, lens, anchor, severity, confidence, evidence_trace, optional suggested_patch, optional pre_merge_obligation, producing_hunt, model_authored_prose. - Anchor is a discriminated union with the required PR-level type (pr | file | line, line supports side + multi-line start_line..line range). - KNOWN_LENSES (11 specialist + always-on/triage) and SEVERITIES exported for reuse by the slice-3 router and slice-2 verdict; lens/severity validated against them. - validateFinding collects ALL violations (diagnosable per-lens drop rate), plus isValidFinding narrowing wrapper and throwing assertFinding. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…k-1-4) Covers the exported FINDING_SCHEMA_VERSION constant, well-formed findings across every anchor type (line/range/side, file, PR-level) plus optional suggested_patch/pre_merge_obligation, and malformed findings for every required field. Asserts validateFinding collects ALL violations (per-lens drop-rate diagnosability), and exercises isValidFinding narrowing and assertFinding throwing. 37 tests green under vitest. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…nts) - Restore the empty-review-body convention: with inline comments present the verdict is submitted with an empty body; a non-empty body exists only for comment-less reviews and skipped-dimension notes. Keeps the single-call submission rule. - Deliver workflows/review/lib/ to consumer repos: gh-aw's source: import only copies the .md, so the job checks out Khan/actions at the pinned review-v* release (single version surface for prompt + code) and installs its runtime deps. - Drop planning identifiers (E2, R8, slice numbers, operator-directive note) from shipped file comments.

jeresig

In general this is looking much better to me!

Pushing back for a few reasons:

Now that we have a bunch of agents we keep having to extend the explanations/text inside the reviewer itself which is getting quite lengthy. Let's find ways to make everything generic so that we don't have to repeat ourselves.
Let's make all the sub-reviewers (including the skill one) produce the same output so that we can keep the instructions very simple and clear.
In too many places the main coordinator is being given knowledge of what the sub-agents do or how they're configured. This feels very unnecessary - having a cleaner separation would allow the main reviewer to delegate more and also have less specification (e.g. it shouldn't care what models a sub-agent uses or what files it reads, it should only care that it's producing output in the standard format that we expect).

jeresig · 2026-07-07T14:44:39Z

 GitHub call and every safe-output write. Run them in three phases (the third runs
 only when there are candidate comments to validate).

+### Model launch defaults and effort


I think we should get rid of this section. I don't think this is providing anything to the reviewer since all of these are already hard-coded into the sub-agent definitions (and we already have explanations for the agents what they do elsewhere).

jeresig · 2026-07-07T14:45:27Z

 review config), parsed deterministically, one rule per line:
 ```
 # <pattern> [lens=<lens>,…] [tier=trivial|low|medium|high] [direction-dependent]
+# enable <reviewer>[,<reviewer>…]


As mentioned in the other PR I think we don't have to define this here at all as the reviewer isn't going to care about this.

jeresig · 2026-07-07T14:47:13Z

+(one turn), and wait for all. The opt-in whole-change reviewers (`holistic`,
+`completeness`, `test-adequacy`, `first-principles`, `conventions`) are not path-gated
+the way lenses are: a repo turns each on with an `enable <reviewer>` line in its
+`ROUTING` file, and **none run by default** — the default roster is exactly the
+reviewers above, and a reviewer earns its `enable` line through the eval suite, not by
+shipping. Each returns `findings[]` in the same shape as the `correctness-reviewer` (a
+`label` per finding), so they feed the verdict, scope filter, validation, and inline
+comments through the exact same path — no new gate.


I think this is probably too much detail for the reviewer to care about - we can probably just save that they should always be run and return findings like correctness-reviewer and leave it at that.

jeresig · 2026-07-07T14:47:56Z

+The first three carry an
+explicit, named mandate; state each mandate to the sub-agent so it reviews *that*
+dimension and nothing else:


Do we have to do that? Can't we just encode that in the sub-agent explicitly?

jeresig · 2026-07-07T14:50:58Z

+Use a blocking label only for a whole-change defect that genuinely must be fixed before
+approval. If the change hangs together, return {"findings": []}.
+
+## agent: `completeness`


I'd be very surprised if this one produces substantially different results from the holistic one.

It should, I've been something similar for a couple of weeks now. They're aimed at different questions. Holistic reviews the change (does the diff hang together, is the approach internally consistent, does it solve what it set out to solve), while first-principles reviews the justification for the change (is the premise right, should this exist as written, is there a materially simpler approach). Its primary input is the stated rationale rather than the line-level diff, it runs on a different, more capable model (fable) specifically to get a non-opus perspective, and it's advisory-only so it can never block. The intent is questioning the core reasoning of a change.

The prompt didn't make that distinction sharp enough, so I'll tighten the mandate text. Since it's opt-in with zero default cost, "it won't differ from holistic" is a testable claim the eval suite can answer directly. If the two produce substantially overlapping findings there, merging them is the easy follow-up.

jeresig · 2026-07-07T14:54:55Z

+<details>
+<summary><strong>Excluded from review</strong> (3 files)</summary>
+
+Not individually reviewed by <code>pattern-triage</code> — generated, formatting-only, or


Suggested change

Not individually reviewed by <code>pattern-triage</code> — generated, formatting-only, or

Not individually reviewed — generated, formatting-only, or

jeresig · 2026-07-07T14:56:57Z

 `out/` directory if needed), naming the file after the sub-agent — `pattern-triage.json`,
-`correctness-reviewer.json`, `skill-auditor.json`, `thread-reconciler.json`, and
-(Phase 3) `claim-validator.json`. These files are uploaded
+`correctness-reviewer.json`, `skill-auditor.json`, each enabled whole-change
+reviewer's `<reviewer>.json` (`holistic.json`, `completeness.json`,
+`test-adequacy.json`, `first-principles.json`, `conventions.json`), each
+dispatched lens's `<lens>.json`, `thread-reconciler.json`, and (Phase 3)
+`claim-validator.json`. These files are uploaded


We should probably not include the list of all the files otherwise this will just keep increasing, it should be fine to have it follow the naming proposal at the top (especially since we aren't actually reading these files directly).

jeresig · 2026-07-07T14:57:47Z

+finding-producing reviewer's output — the `correctness-reviewer`'s `findings[]`, the
+`skill-auditor`'s `violations[]`, any enabled whole-change reviewers' `findings[]` (`holistic`,
+`completeness`, `test-adequacy`, `first-principles`, `conventions`),
+and any dispatched lens's `findings[]` — against the new-code scope from Step 1
+(`/tmp/gh-aw/review/new-scope.json`). This is what stops the reviewer from re-commenting


Could we make it so that everything returns a findings and then we no longer encode a list of all agents here, just tell it to look at all findings culmulatively?

jeresig · 2026-07-07T14:59:46Z

+- **`first-principles`** — **mandate: a diverse-perspective sanity check, advisory
+  only.** Ask whether the change *should exist as written* — is there a materially
+  simpler approach, a wrong assumption baked in, a problem better not solved here at all?
+  It runs on **Fable 5** (`claude-fable-5`) for a genuinely different perspective. It is


I don't think the reviewer cares about this.

… rendering (slice-2) task-2-1/2-2 verdict.ts: pure computeVerdict(labels + dimension availability + policy conflicts) -> APPROVE | REQUEST_CHANGES | HOLD_FOR_HUMAN. Consumes #194's mechanical label rule (isBlockingLabel); does not re-implement it. R2 gate: a missing core dimension (correctness/skill-severity) or a policy-named conflict holds for a human; a lost pattern-triage is note-and-continue. Documented default blocking threshold = 1 (tunable, not a HITL gate). task-2-3 render-comment.ts: code-owned Conventional-Comment label taxonomy + deterministic label-wrapping/templating from the finding schema; models own all prose. Review-body rendering mirrors review.md Step 6 + skipped-dimension notes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…pshot tests (task-2-4) Truth-table tests for computeVerdict (verdict.test.ts): APPROVE / REQUEST_CHANGES / HOLD_FOR_HUMAN precedence, the R2 core-dimension gate (correctness + skill-severity absence holds; hold dominates a blocking label with all reasons retained), pattern-triage note-and-continue, policy-named conflicts (verbatim detail passthrough), blocking-threshold clamping (>=1) + never-RC-on-zero, and purity (deterministic, non-mutating). Rendering snapshot tests (render-comment.test.ts): isBlockingLabel over the full label taxonomy + unknown-label safe default; labelForFinding severity x lens matrix (conventions => best-practice, specialists => plain); renderComment inline snapshots (label wrap + verbatim prose + optional suggestion block) with verbatim prose/patch passthrough asserted; renderReviewBody inline snapshots for every verdict x inline branch + skipped-dimension notes + non-empty safe-output guard. Fixtures validated through assertFinding so tests can't pass on schema-invalid findings. 50 tests green; full pnpm test suite 253/253; tsc --noEmit clean (node24). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

- Remove the baked-in DEFAULT_LENS_RULES/DEFAULT_RISK_RULES: a repo's routing map now lives in its own .github/aw/review/ROUTING file (REVIEWERS-style line grammar, parsed deterministically in the new routing-config module), complementing the model-facing risk-classification.md. Missing config degrades safely: no specialist lenses, always-on reviewers still run, budget floored, and routing.json carries a loud routingConfig warning the orchestrator surfaces on the PR. - runCli reads repo files under REVIEW_REPO_ROOT so the router can run from the shared-lib checkout against the reviewed repo. - review.md: apply reviewer-suggested edits (drop stale reviewer-mapper references, fix the router-reads-the-diff claim), document the ROUTING format, the checkout-based invocation, and the back-to-back two-pass flow for direction-dependent tiers with a concrete example. - Drop planning identifiers from shipped comments.

…EADME

…lice-4) E1: correctness-reviewer names the high-risk trigger + one-line judgment in riskReason. E3: untrusted-input rule broadened to all PR content; injection attempts to steer the reviewer are themselves findings (issue (blocking)). E5: deletions are findings -- flag removals that drop a guard/check/test. R3b: pre-existing bugs on touched lines are fair to flag, scoped to touched lines, building on #194 severity (does not reopen it). E6: stage the full reply chain per bot thread; reconciler weighs author reasoning and never re-raises a conceded point. E7: stage open human-thread lines; reconciler emits skipLines and the orchestrator defers there (verdict + Step 5 kept consistent). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

- Rework the injection rule: an author's request in the PR title/description is legitimate context to weigh and respond to (humans may steer the reviewer); steering text inside code/comments/fixtures is still never followed, but is surfaced as a non-blocking note instead of an auto-blocking issue. - Propagate the touched-lines scoping to the skill auditor: violations anchor on lines the PR adds or modifies, never surrounding context or pre-existing style, keeping the mechanical Step 3 scope filter a backstop. - Strip the remaining plan identifiers (E1/E3/E5/E6/E7/R3b) from the shipped prompt text.

…ewers (task-5-1) Add the R9 bounded-investigation protocol to review.md: finding-producing sub-agents (correctness-reviewer, skill-auditor, the slice-7 lenses) and the claim-validator may grep callers/definitions, trace a call chain a step or two, and run one targeted cheap read-only check per finding before committing to it. Stays read-only and on the checkout (no GitHub, no network, no writes), keeps all read content as untrusted input, and cites/drops per investigation result. Because gh-aw sub-agents are prompt-isolated (each sees only its own body), the protocol is embedded verbatim in each finding-producing agent rather than cross-referenced; slice-7 lenses embed the same block. An orchestrator-level note documents the per-finding tool-call cap enforced in code (workflows/review/lib/investigation-cap.ts, task-5-2), sized inside the router runBudget so a high-risk PR gets more room and a misrouted one keeps a floor. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

task-5-2: enforce a cap on tool calls per finding in code, drawing the numbers from the slice-3 run budget (maxToolCallsPerFinding + maxTotalToolCalls) rather than configuring them separately. Pure decideToolCall() + stateful InvestigationCap guard; over-cap calls are refused deterministically with a fixed reason code (no prose about the code under review, per the R8 boundary). Default caps documented (mirror the router 'low' tier / misrouted floor); not a HITL surface. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Cover the pure decideToolCall verdict (per-finding-before-run-total precedence, headroom clamping, malformed-cap normalisation) and the stateful InvestigationCap guard (per-finding independence, the shared run-total pool, no state mutation on a refusal, deterministic replay, snapshot copy-safety) plus the RunBudget wiring (capsFromRunBudget / fromRunBudget). AC: cap enforced; over-cap calls refused deterministically. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…total) — task-5-3 check() is the non-mutating preview half of the InvestigationCap guard; the existing tests only exercised its allowed path. Add two cases asserting check() reports the same refusal request() would (per-finding-cap-exceeded and run-total-cap-exceeded) with correct remaining* headroom and zero mutation (snapshot unchanged). No production code change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…allers) - The cap was a library nothing invoked. Add a CLI entrypoint (investigation-cap.ts request <finding-id>) over an append-only journal shared by the run's sub-agents: caps come from routing.json's runBudget, spent calls are recounted from the journal, allowed requests append, and a refusal exits non-zero so shell callers can gate on it. - Wire the invocation into the three finding-producing prompts (correctness reviewer, skill auditor, claim validator) so the cap is requested before every investigation tool call, per the reviewer suggestion; lenses inherit the same block if a repo enables them. - Drop the R*/slice/task planning identifiers from shipped files.

…, model+effort defaults, kept gates - task-6-1: define 5 always-on reviewers (holistic, completeness, test-adequacy, first-principles, conventions) as sub-agents with trust/advisory constraints. completeness reads Jira/Confluence read-only inside the non-posting sub-agent, fetched text = untrusted data (interface §4.4). first-principles advisory-only, never blocks, Fable 5 day one. conventions advisory, router-gated. - task-6-2: prompt edit 14 named mandates for holistic/completeness/first-principles. - task-6-3: R12 model launch defaults + effort table (Opus 4.8 workhorse; medium triage/reconciliation; high lenses/whole-change; xhigh claim-validator/refuters; first-principles=Fable 5). Effort recorded per role (annotation + authoritative table); gh-aw has no per-agent effort field. Does not re-pin #194's model pins. - task-6-4: always-on reviewers feed verdict/scope/claim-validator via the same findings path (no new gate); pattern-triage exclusions surfaced in guidance comment folded into risksPatternsKey; refuter panel noted as wave-2; dedup + verdict bookends + thread-reconciler preserved; advisory reviewers can never block. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Per reviewer feedback on the roster strategy: the default roster stays exactly the #194 set (correctness, skill-auditor, pattern-triage, thread-reconciler, claim-validator) — zero new per-PR cost. The whole-change reviewers (holistic, completeness, test-adequacy, first-principles, conventions) land as capability only: - New 'enable <reviewer>' directive in the consumer ROUTING file; the router validates names against ENABLEABLE_REVIEWERS and surfaces the set as routing.json's enabledReviewers (empty by default). - review.md dispatches an opt-in reviewer only when enabledReviewers names it, and states the policy: a reviewer earns its enable line through the eval suite, never by shipping. - Model/effort table marks the opt-in rows and carries the cost note. - Align the completeness reviewer's embedded-instruction handling with the slice-4 steering-text rule (non-blocking note, not auto-blocking). - Strip remaining plan identifiers (R12, E3, R9, prompt-edit/wave numbers).

…tail moved to sub-agents/README)

jeresig

Thanks for re-working this!

…/slice-6 # Conflicts: # workflows/review/README.md # workflows/review/lib/finding-schema.ts # workflows/review/lib/router.test.ts # workflows/review/lib/router.ts # workflows/review/lib/routing-config.ts # workflows/review/review.md

jwbron added egg agent:orchestrator labels Jul 3, 2026

jwbron mentioned this pull request Jul 3, 2026

review: roster restructure, determinism boundary, eval suite #196

Closed

jwbron force-pushed the egg/pipeline-dcdad92d/slice-5 branch from 8e51405 to 6ea5d72 Compare July 3, 2026 21:34

jwbron force-pushed the egg/pipeline-dcdad92d/slice-6 branch from d67e240 to 47fc0fc Compare July 3, 2026 21:34

jwbron force-pushed the egg/pipeline-dcdad92d/slice-5 branch from 6ea5d72 to ba44e5a Compare July 3, 2026 21:42

jwbron force-pushed the egg/pipeline-dcdad92d/slice-6 branch from 47fc0fc to 341bde8 Compare July 3, 2026 21:42

egg and others added 7 commits July 3, 2026 14:43

Persist BRC history for slice-1 (#2548)

8826894

slice-1: prettier fixes + changeset

c02da14

slice-1: drop BRC history artifacts from slice PR

b119a01

jwbron force-pushed the egg/pipeline-dcdad92d/slice-5 branch from ba44e5a to 39ffacc Compare July 3, 2026 21:43

jwbron force-pushed the egg/pipeline-dcdad92d/slice-6 branch from 341bde8 to 22a9f40 Compare July 3, 2026 21:43

jwbron marked this pull request as ready for review July 3, 2026 21:44

jwbron requested a review from jeresig July 3, 2026 21:44

khan-actions-bot requested review from a team and jaredly and removed request for a team July 3, 2026 21:44

jwbron force-pushed the egg/pipeline-dcdad92d/slice-5 branch from 39ffacc to bc564fc Compare July 6, 2026 18:19

jwbron force-pushed the egg/pipeline-dcdad92d/slice-6 branch from 22a9f40 to 9d3e344 Compare July 6, 2026 18:19

khan-actions-bot requested a review from a team July 6, 2026 18:20

jeresig requested changes Jul 7, 2026

View reviewed changes

jwbron and others added 3 commits July 7, 2026 11:58

review: drop lib install step; scripts run via npx tsx

123a10f

jwbron and others added 16 commits July 7, 2026 11:59

slice-3: replace satisfies with prettier-2-compatible check, changeset

cb8d714

slice-3: drop BRC history artifacts from slice PR

29f2b6f

review: last-match-wins tier precedence; move ROUTING format doc to R…

06cbb2e

…EADME

Persist BRC history for slice-4 (#2548)

5bb941d

slice-4: changeset

a21b4ce

slice-4: drop BRC history artifacts from slice PR

fdf05d2

slice-5: prettier fix + changeset

1340a42

review: investigation-cap call sites run via npx tsx

3f01827

jwbron force-pushed the egg/pipeline-dcdad92d/slice-5 branch from bc564fc to 3f01827 Compare July 7, 2026 19:04

egg and others added 6 commits July 7, 2026 12:04

Persist BRC history for slice-6 (#2548)

8d21077

slice-6: changeset

481198d

slice-6: drop BRC history artifacts from slice PR

33de326

review: genericize orchestrator (uniform findings contract, roster de…

8f8cb7c

…tail moved to sub-agents/README)

jwbron force-pushed the egg/pipeline-dcdad92d/slice-6 branch from 9d3e344 to 8f8cb7c Compare July 7, 2026 19:13

jwbron mentioned this pull request Jul 7, 2026

[pipeline-dcdad92d][slice-1/12] Foundations: finding schema... #197

Merged

jeresig approved these changes Jul 8, 2026

View reviewed changes

Base automatically changed from egg/pipeline-dcdad92d/slice-5 to main July 8, 2026 16:47

jwbron merged commit 16f1469 into main Jul 8, 2026
3 checks passed

jwbron deleted the egg/pipeline-dcdad92d/slice-6 branch July 8, 2026 16:48

	Not individually reviewed by <code>pattern-triage</code> — generated, formatting-only, or
	Not individually reviewed — generated, formatting-only, or

Uh oh!

Conversation

jwbron commented Jul 3, 2026

What's in this PR

This slice

Stack

Uh oh!

changeset-bot Bot commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

jeresig left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeresig left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

changeset-bot Bot commented Jul 3, 2026 •

edited

Loading