[pipeline-dcdad92d][slice-5/12] Investigation tooling for reviewers by jwbron · Pull Request #208 · Khan/actions

jwbron · 2026-07-03T21:38:18Z

Give reviewer sub-agents a bounded investigation protocol (grep callers, trace call chains, one targeted cheap check per finding) and enforce a deterministic per-finding tool-call cap that lives inside the slice-3 run budget.

Base PR: #196

What's in this PR

Commits (5):

slice-5: prettier fix + changeset
implement(tester): cover check() refusal previews (per-finding + run-total) — task-5-3
implement(tester): R9 per-finding tool-call cap tests (task-5-3)
implement(coder): R9 per-finding investigation tool-call cap (slice-5)
implement(documenter): R9 bounded-investigation instructions for reviewers (task-5-1)

 .changeset/review-investigation-cap.md         |   5 +
 workflows/review/lib/investigation-cap.test.ts | 363 +++++++++++++++++++++++++
 workflows/review/lib/investigation-cap.ts      | 244 +++++++++++++++++
 workflows/review/review.md                     |  66 +++++
 4 files changed, 678 insertions(+)

This slice

Investigation tooling for reviewers

Files affected:

workflows/review/review.md
workflows/review/lib/investigation-cap.ts
workflows/review/lib/investigation-cap.test.ts

Tasks (3) + acceptance criteria

task-5-1: R9 sub-agent instructions for bounded investigation: grep callers, trace call chains, run one targeted cheap check per finding.
- Acceptance criteria: Instructions present; consumed by slice-7 lenses.
task-5-2: R9 enforce a cap on tool calls per finding in code, living inside the run budget from slice 3. Document the assumed default cap (not a HITL surface).
- Acceptance criteria: Cap enforced; over-cap calls refused deterministically.
task-5-3: Test the per-finding tool-call cap.
- Acceptance criteria: Cap test green.

Stack

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

Note: this PR was opened by the operator after the fact; the pipeline's stacked-PR reconciler did not open one for slice-5 (its consensus round required operator-assisted recovery).

changeset-bot · 2026-07-03T21:38:24Z

🦋 Changeset detected

Latest commit: bb93b77

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>

jeresig

This feels kind of half-baked - this tool isn't used anywhere (nor is it called).

(Also, in these PRs there keep being references to R* and "slices" but those are just artifacts of the PRs, not what we care about in the code.)

jeresig · 2026-07-06T15:38:40Z

+**Bounded investigation (R9).** The finding-producing sub-agents — the
+`correctness-reviewer`, the `skill-auditor`, and the specialist lenses added in a later
+slice — and the `claim-validator` when it re-checks a claim may **investigate** on the
+checkout before committing to a finding, rather than guessing from the diff alone: grep
+for callers and definitions, trace a call chain a step or two, and run **one targeted
+cheap read-only check per finding**. Each sub-agent carries this protocol in its own
+prompt (they run isolated and never see this orchestrator prompt), so the rule is
+repeated verbatim in each finding-producing agent below and every slice-7 lens embeds the
+same block. Investigation never leaves the checkout — no GitHub, no network, no writes.
+A **per-finding tool-call cap is enforced in code** (`workflows/review/lib/investigation-cap.ts`),
+sized inside the router's `runBudget` (Step 3) so a high-risk PR gets more investigation
+room and a misrouted one keeps a floor; over-cap calls are refused deterministically, so
+the investigation stays shallow no matter what a sub-agent attempts.
+


Suggested change

**Bounded investigation (R9).** The finding-producing sub-agents — the

`correctness-reviewer`, the `skill-auditor`, and the specialist lenses added in a later

slice — and the `claim-validator` when it re-checks a claim may **investigate** on the

checkout before committing to a finding, rather than guessing from the diff alone: grep

for callers and definitions, trace a call chain a step or two, and run **one targeted

cheap read-only check per finding**. Each sub-agent carries this protocol in its own

prompt (they run isolated and never see this orchestrator prompt), so the rule is

repeated verbatim in each finding-producing agent below and every slice-7 lens embeds the

same block. Investigation never leaves the checkout — no GitHub, no network, no writes.

A **per-finding tool-call cap is enforced in code** (`workflows/review/lib/investigation-cap.ts`),

sized inside the router's `runBudget` (Step 3) so a high-risk PR gets more investigation

room and a misrouted one keeps a floor; over-cap calls are refused deterministically, so

the investigation stays shallow no matter what a sub-agent attempts.

**Bounded investigation (R9).** The finding-producing sub-agents — the

`correctness-reviewer`, the `skill-auditor`, and the specialist lenses added in a later

slice — and the `claim-validator` when it re-checks a claim may **investigate** on the

checkout before committing to a finding, rather than guessing from the diff alone: grep

for callers and definitions, trace a call chain a step or two, and run **one targeted

cheap read-only check per finding**. Each sub-agent carries this protocol in its own

prompt (they run isolated and never see this orchestrator prompt), so the rule is

repeated verbatim in each finding-producing agent below and every lens embeds the

same block. Investigation never leaves the checkout — no GitHub, no network, no writes.

A **per-finding tool-call cap is enforced in code**,

sized inside the router's `runBudget` (Step 3) so a high-risk PR gets more investigation

room and a misrouted one keeps a floor; over-cap calls are refused deterministically, so

the investigation stays shallow no matter what a sub-agent attempts.

…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

Great - looks like this is all integrated correctly now!

… 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>

Formatting-only follow-up to b2c4d90: applies @khanacademy/eslint-config prettier rules to verdict.test.ts + render-comment.test.ts so the lint check passes on the changed files. No test logic or inline-snapshot content changed — 50/50 still green under node24; tsc --noEmit clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…ema (tester NACK v1) Formatting-only; no logic change. Applies the repo lint gate's prettier 2.6.2 formatting (eslint-plugin-prettier@4.0.0 binds prettier 2.6.2) which had never been run against these files: - render-comment.ts / finding-schema.ts: string-arg + ternary line wrapping. - typeof-indexed-access type aliases dropped the paren form ((typeof X)[number] -> typeof X[number]) — prettier 2.6.2 emits the unparenthesized form; the parens were added by prettier 2.7.0. Semantics unchanged (verified typecheck-clean under the repo tsconfig). Verified: strict typecheck clean; verdict truth-table + render checks still pass; prettier diff against these files is now empty except the exact 2.6.2<->2.8.8 typeof-parens delta, which the pinned CI formatter (2.6.2) resolves in favour of the committed form. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…e-3) task-3-1: file classification from .gitattributes (linguist-generated), path->lens mapping (11 specialist lenses), team mapping from REVIEWERS (most-specific-pattern-wins + fallback ranking, subsuming reviewer-mapper), and per-file risk tier. The one diff-direction-dependent judgement is externalised as pendingRiskQuestions for the orchestrator's small-model call (plan §8.6); the core stays pure/deterministic. task-3-2: ONE budget rule scaling by the highest touched tier + ONE floor for misrouted PRs. Documented default caps (DEFAULT_TIER_BUDGETS, 20min/$10 ceiling per operator); maxToolCallsPerFinding lives here for slice-5 (R9). No human-read prose authored in code (R8 tripwire, §8.7). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…, budget, floor (task-3-4) Covers the five task-3-4 AC axes over fixtures plus the reviewer-mapper subsumption (R12) and the glob/config parsers routing is built on: - classification: linguist-generated vs source (parse + isGenerated + route). - path->lens: per-file union, deduped lensesToSpawn in canonical order, always-on lenses never spawned as specialists. - tier: default, highest-of-matching, diff-direction-dependent deferral (conservative tier + pendingRiskQuestions), resolvedTiers second pass. - budget scaling: monotonic across tiers; run budget = highest touched tier. - misrouted floor: docs-only PR floored to 'low'; already-higher tier not lowered. - teams: most-specific REVIEWERS rule wins, sorted union + file-count fallback rank. Pure/deterministic core (plan §8.6) — no I/O, no model call. Offline: could not run vitest (node_modules absent, network unreachable); hand-verified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…(slice-3 v2) Addresses reviewer_code_holistic v1 NACK (BLOCKING-1 + BLOCKING-2): BLOCKING-1: router.ts now has a CLI entrypoint. review.md Step 3 invokes `node -r @swc-node/register workflows/review/lib/router.ts` directly, so the file doubles as the executable: a require.main===module guard runs runCli(), which reads the Step-1-staged files.json + .gitattributes/.github/REVIEWERS, calls the (unchanged) pure route(), and writes /tmp/gh-aw/review/routing.json. node:fs is loaded lazily inside the guard so the module stays import-clean for the unit tests; runCli takes an injected fs so it is testable without real I/O. BLOCKING-2: new toRoutingJson() adapter emits the exact shape review.md consumes — teams:{owners:{path:[team]}, fallback:[{team,files}]} (owners over source files only, mirroring the old owned-files scope) and perFileTier in the documented capitalized casing (High|Medium|Low|Trivial), bridged from the core's lowercase tiers at the I/O boundary. pendingRiskQuestions is carried in routing.json so the two-invocation resolvedTiers protocol stays intact; the optional second-pass answer is read from /tmp/gh-aw/review/resolved-tiers.json. Pure route()/parsers/budget core is byte-for-byte unchanged (reviewers had no correctness objection); this is purely the I/O+serialization shell. No human-read prose authored in code (R8 tripwire §8.7). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

…/tierFromDisplay/runCli) Extends task-3-4 tests to the coder's v2 additions (34d88d5) — the routing.json I/O boundary review.md Step 3 depends on: - tierFromDisplay: display+lowercase casings, unknown -> low. - toRoutingJson: display-cased perFileTier, owners scoped to source files (generated excluded), fallback + pendingRiskQuestions passthrough. - runCli (fs injected, no real I/O): bare-array & {files} wrapper parsing, misrouted floor, .gitattributes/REVIEWERS parse, resolved-tiers second pass, mkdir + routing.json round-trips to the returned value. Offline (node_modules absent, network unreachable); hand-verified. 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.

…/slice-5 # Conflicts: # workflows/review/review.md

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

jwbron force-pushed the egg/pipeline-dcdad92d/slice-4 branch from 6f17afc to 7310e0f Compare July 3, 2026 21:42

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

khan-actions-bot requested a review from a team 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-4 branch from 7310e0f to 44cb874 Compare July 3, 2026 21:43

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

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

jeresig requested changes Jul 6, 2026

View reviewed changes

jwbron force-pushed the egg/pipeline-dcdad92d/slice-4 branch from 44cb874 to bf188a9 Compare July 6, 2026 18:19

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

jwbron mentioned this pull request Jul 6, 2026

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

Merged

jeresig approved these changes Jul 7, 2026

View reviewed changes

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

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

123a10f

Persist BRC history for slice-2 (#2548)

f618d2a

egg and others added 21 commits July 7, 2026 11:59

Persist BRC history for slice-3 (#2548)

a6984f1

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

jwbron force-pushed the egg/pipeline-dcdad92d/slice-4 branch from bf188a9 to 798016b Compare July 7, 2026 19:04

jwbron mentioned this pull request Jul 7, 2026

review: never post to a merged or closed PR #213

Closed

Base automatically changed from egg/pipeline-dcdad92d/slice-4 to egg/pipeline-dcdad92d/slice-3 July 8, 2026 16:37

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

jwbron added 2 commits July 8, 2026 09:42

Merge main into egg/pipeline-dcdad92d/slice-5

cfffed2

Merge remote-tracking branch 'origin/main' into egg/pipeline-dcdad92d…

bb93b77

…/slice-5 # Conflicts: # workflows/review/review.md

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

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

Navigation Menu

Uh oh!

[pipeline-dcdad92d][slice-5/12] Investigation tooling for reviewers - #208

[pipeline-dcdad92d][slice-5/12] Investigation tooling for reviewers#208
jwbron merged 42 commits into
mainfrom
egg/pipeline-dcdad92d/slice-5

jwbron commented Jul 3, 2026

Uh oh!

changeset-bot Bot commented Jul 3, 2026 •

edited

Loading

Uh oh!

jeresig left a comment

Uh oh!

jeresig Jul 6, 2026

Uh oh!

jeresig left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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!

jeresig Jul 6, 2026

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