feat(ai-review): delta reviews, Test Plan↔CI findings, OSH fan-out

[Hussam Aldarwish (eduly-haldarwish)]

Summary

• Delta reviews: after the last published <!-- ai-review-meta -->, re-runs review only prior_head…HEAD (full on missing/inconclusive meta, rebase, base change, or force-full-review).
• Test Plan ↔ CI: inventory CI on all PR events; uncovered Test Plan items become normal findings; checklist tick / status-block write-back is retired (update-pr-body no-op).
• OSH fan-out: route by roster K — K≤1 single Sonnet collapse, K>1 Opus parent + Sonnet/Haiku Task subagents; size-based sonnet-*-threshold routing deprecated. Publish still gates on parent/collapsed structured output.

Stacked on #52 (model cascade). Spec: docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md. ADR: docs/adr/0006-test-plan-ci-findings-and-osh-routing.md.

Test plan

• node --test ai-review/lib/*.test.js (323 pass locally)
• Fresh PR: full review + meta marker on published body
• Push follow-up commit: delta mode + prior-review.md carry-forward in prompt/artifacts
• force-full-review: true forces full range even with valid prior meta
• Test Plan items with weak/no CI → findings; PR body checkboxes unchanged
• Small diff (K=1): Sonnet collapse, no Task allowlist
• Large multi-file diff (K>1): Opus + Task; SO confidence from Haiku scorer path

Made with Cursor

Superseded by a new ai-review run.

Superseded by a new ai-review run.

Superseded by a new ai-review run.

[mtm-bot]

✅ PASS

12 P2 / 11 P3 finding(s) noted — non-blocking.

Confidence: 40 · Merge risk: medium
P0: 0 · P1: 0 · P2: 12 · P3: 11

Re-run this job if you need another review pass.

AI Review — PR #8: review stage + schema verdict + deterministic publish (Task 2.4)

Review mode: full (base dd8f5ab → head 86fcdfd), 27 files, 3,105 lines of churn.
OSH mode: fan-out (K=4) — four coverage reviewers, a cross-file tracer, a history role, and an independent scorer (finder ≠ scorer). All 27 changed files were read in full by the coverage roster.
Test execution: skipped by design — this stage provisions no toolchain (ADR 0003 §2). The node-test CI lane is the authoritative signal and is green.

No blockers. The advisory pile is large, but it clusters into three themes worth reading together: classification booleans that feed the gate's own math (F8, F9), the collapse branch and delta path shipping unexercised end-to-end (TP2, TP3), and documentation that has drifted behind the code (F4, F6, F7, F20).

---

Intent alignment — ✅ Aligned (with scope creep)

.ai-review/linked-issues.json is [], so intent was framed from the PR body alone, before any diff analysis.

The body's three Summary bullets — real two-stage review, schema-validated structured output, deterministic Publish recomputation — are all implemented and verifiable in the diff. But the diff also delivers three substantial features the body never mentions: delta-mode incremental review (delta.js +267, write-delta.js +185), Test Plan ↔ CI findings (testplan-ci.js +259), and OSH fan-out routing (osh-agents.json, both osh-policy-*.md). That is scope creep, filed as [Intent] P2 below. It does not deviate from the goal — the extra work is coherent and well-documented in ADR 0006 and the 2026-08-17 design spec — so per the rubric's derivation table, P2 scope creep alone still reads as Aligned. The PR body is simply stale relative to what shipped.

Test Plan ↔ CI mapping

Mapping the PR body's three Test Plan items against .ai-review/ci-checks.json (review in-progress; Static analysis, node-test, Lint, duplicated-snippets all green):

Test Plan item	CI coverage	Verdict
selftest.yml's review job completes	review check — this run	Covered (in flight)
Labels / review comment posted correctly	No check asserts posting outcome	Weak — folded into TP1
gated-demo runs iff verdict == pass	gated-demo absent from the check inventory	Uncovered → TP1

One note on the staged artifact itself: .ai-review/test-plan-items.json contains seven items that do not appear in the live PR body (which has exactly three). Applying testplan-ci.js's own extraction logic to the live body should yield those three — ## Test plan matches HEADING_TEST_PLAN_RE case-insensitively, and the section holds three checkboxes. I could not determine the cause without re-running Prep, so TP4 reports the divergence rather than claiming a located defect. It is worth a look before merge, since it sits on this PR's centerpiece feature.

---

P0 — Blockers

None.

P1 — Should Fix

None.

P2 — Nice-to-Have

• [Intent] Scope creep — the PR body describes only Task 2.4, but delta mode, Test Plan ↔ CI, and OSH fan-out all ship here with no rationale in the body. Updating the Summary would save the next reader a trip through the ADRs.
• ai-review/lib/prep.js:63 — TEST_RE misses compound test directories. The alternation (^|/)(tests?|spec|__tests__|__mocks__)/ requires an exact segment, so e2e-tests/checkout_flow.py (preceded by -, not /) matches neither branch. A PR whose only coverage lives there gets no_tests_for_changed_logic: true and eats a spurious −15 confidence penalty. This boolean feeds the gate directly.
• ai-review/lib/prep.js:193 — no_tests_for_changed_logic is computed globally, not scoped to the changed logic. hasLogicChange && !hasTestChange means touching any test file clears the penalty: fix one flaky legacy test, add an entirely untested feature in the same commit, and the flag reads false. The expression measures "a test file was touched," not the invariant it names.
• ai-review/lib/testplan-ci.js:16 — No fenced-code-block awareness in Test Plan extraction. extractCheckboxes walks lines with no ``` state, so example checklist markup inside a fence is harvested as a real requirement, producing uncovered-item findings against requirements nobody wrote.
• .github/workflows/selftest.yml:24 — 55-minute job timeout is below the documented worst case. action.yml:1027 records "review-stage durations up to 79.5 min were measured, and 32 of 682 jobs died on the caller's clock." The retry_budget gate only suppresses a late second attempt; it does not bound the first. This ceiling was already raised once (30m → 55m) for exactly this failure.
• .github/workflows/selftest.yml:74 — gated-demo runs iff verdict == pass has no coverage for the only-if direction. A regression that always emits pass still goes green; the negative case produces a skipped job that asserts nothing, and gated-demo is not in the check inventory at all.
• ai-review/lib/write-delta.js — The delta path has no end-to-end CI coverage. node-test covers the pure resolver well, but selftest.yml runs a single pull_request-triggered job, so no check ever performs the second run that delta mode exists for. The step wiring (reviews.json fetch → prior-review.md → manifest range → Publish meta marker) is verified only at the function level.
• ai-review/action.yml:508 — The K≤1 collapse branch is never exercised in CI. Every selftest run on this repo takes the fan-out branch (this one is K=4), so the Sonnet/collapse half of the new routing — its allowlist string, its osh-policy-collapse.md copy — reaches consumers having never run end-to-end. Consumers with small diffs hit it first.
• ai-review/lib/testplan-ci.js:26 — Staged Test Plan items diverge from the live PR body (7 staged vs 3 in the body). Cause undetermined; see the mapping section above.
• ai-review/action.yml:641 — The collaborator-permission 503 workaround was reverted with no replacement. The revert itself is clean — git show 86fcdfd --stat is 37 deletions and 0 insertions, no orphaned inputs or dangling wiring — so this is an accepted-risk trade, not a broken revert. But the flakiness 1e1be5f diagnosed ("Claude stages abort when GET .../permission returns 503, before any model work") is now unmitigated and undocumented. Worth a comment recording the decision.
• ai-review/action.yml:523 and :826 — Bash(find:*) permits find -exec, and the same cat/head/tail grants can read /proc/self/environ, which carries the plaintext Authorization: Bearer token. Chained via prompt injection from PR content on a runner checked out at the PR head, that is a credential-exfiltration path into a publicly posted comment_markdown. Scored at low confidence (25) deliberately: I verified the allowlist string is byte-identical to the pre-existing one — the diff only relocates it from an inline --allowedTools into REVIEW_TOOLS_COLLAPSE/REVIEW_TOOLS_FANOUT, and only Task is genuinely new. This is a pre-existing condition the refactor made visible, not a regression this PR introduces. Recorded because the env exposure and the tool grant are now legible in one place for the first time, and because Bash(find:*) buys little that Glob does not.

P3 — Nits

• ai-review/lib/publish.js:232 — buildStatusBlock, upsertStatusBlock, and tickVerifiedBoxes (~90 lines) are exported and tested but have zero callers; action.yml requires only stripLeadingBannerArtifacts/buildReviewBody/buildInconclusiveBody. tickVerifiedBoxes carries a comment explaining its retirement — the other two do not, so a future maintainer may rewire them believing they are live.
• ai-review/README.md:266 — The routing table's "no Haiku scorer" phrasing implies fan-out has one. It does not: osh-scorer is Sonnet, and README.md:79-81 explicitly bans Haiku as a Task child. The same stale phrasing appears in the staged item "SO confidence from Haiku scorer path."
• ai-review/rubric.md:372 — The Confidence Rate Calculation section omits recompute.js:73-75's −10 penalty for test_execution: "passed" with empty verification_evidence. A model reading only the rubric will self-report a confidence the gate then contradicts by 10 points.
• ai-review/osh-policy-fanout.md:41 — "or dispatch an Opus-tier Task" has no target: all four registered agents are Sonnet-tier.
• docs/adr/0003-...md:3 — The "partially superseded" header flags only §4 and §5, but §2 ("the allowlist now includes the JS/Python/make test runners") is equally dead — the action now instructs the model not to run tests and marks test-command/test-hint DEPRECATED.
• ai-review/lib/write-delta.js:146 — A corrupted reviews.json is swallowed with no log line, unlike every sibling error path. Fail-open to full mode is the right default; the missing diagnostic is the defect.
• ai-review/action.yml:750 — A testplan-ci.js crash degrades to empty stub artifacts behind a ::warning:: rather than failing the step, so a parsing bug reads as "no Test Plan items found."
• ai-review/lib/write-delta.test.js — main() is never exercised: neither the missing-SHA throw nor the JSON.parse fallback is covered. It is the one part of the module action.yml actually invokes.
• ai-review/lib/testplan-ci.js:104 — 1. [ ] Do X is captured as the literal [ ] Do X; the marker is never stripped.
• ai-review/action.yml:848 — --agents '${{ ... }}' wraps JSON in single quotes. Latent today (no apostrophes in osh-agents.json), but one contraction in an agent description breaks the invocation.
• ai-review/lib/roster.js:797 — uncappedK re-derives computeK's formula instead of computeK exposing its pre-clamp value; a future formula change silently desyncs k_capped telemetry. This file's own comments record having hit that exact divergence twice.

---

Refuted during verification

Two candidates were dropped rather than reported, both refuted from the code:

• "The 2026-08-07 design doc is stale on coverage_below_threshold_on_critical_paths" — the doc's header reads "Status: approved shape, not yet implemented." Its "Dropped in v1" line is a proposal for an unimplemented design, not a claim about current behavior.
• "META_RE's base_sha=(\S+) absorbs a trailing -->" — greedy quantifiers backtrack. For base_sha=bbb--> the engine resolves the capture to bbb so the literal --> still matches. The corruption is not constructible.

Strengths

• No script-injection surface. Not one ${{ github.event.* }} expression is interpolated into a run: body anywhere in action.yml — every reference is bound through env: and read as $VAR, with attacker-influenceable text (PR title/body) routed to files rather than step outputs, and a comment explaining the newline-safety reasoning.
• Publish fails closed. A missing or invalid structured_output after retry yields verdict=fail + REQUEST_CHANGES + the fail label, never a silent pass.
• Consistent least-privilege token separation. Context/Review/repair/retry all run on the unprivileged fallback token; the App-minted write-scoped identity is reserved for Publish and Reset, with the rationale restated at each stage.
• Cross-file contracts are clean. The tracer traced every symbol_manifest entry to its call sites and found no break: every argv/env contract into delta.js, write-manifest.js, and testplan-ci.js matches each script's process.env reads; every file written is read back at the correct path; roster.k = bins.length correctly yields 0 on an empty diff, matching the K -le 1 collapse branch; and the --json-schema blob is byte-identical across all three invocations, so there is no drift between first attempt and retry.
• recompute.js matches rubric.md exactly on the P0×30 / P1×15 / P2×5 weights, the gate formula's dropped P2 term, and the merge-risk bands — the one place where doc/code agreement matters most.
• write-manifest.js is genuinely defensive: atomic temp-file-plus-rename writes with tests proving partial writes and rename failures cannot corrupt the target, a symlink/non-regular-file guard in statSize closing a real hang/OOM path, and fail-closed writeRoster error handling exercised for both build-time and write-time throws.
• roster.js documents its own history. The blendedCost and packClusters comments name the specific silent-divergence bugs the current shape exists to prevent — which is what made the partition-safety property tractable to verify rather than take on faith.

[Hussam Aldarwish (eduly-haldarwish)]

Superseded by #54 — combining with model-cascade (#52) into one PR against main.