feat(ai-review): model cascade + delta/OSH/Test Plan CI

[Hussam Aldarwish (eduly-haldarwish)]

Summary

Combines closed #52 and #53 into one PR against main.

• Model cascade (feat(ai-review): lock Claude→Cursor→free model cascade #52 / Lock Claude→Cursor→free model cascade in ai-review and ai-qa #51): lock Claude primaries in ai-review and ai-qa with Cursor → pinned OpenCode free → auto/best-free fallbacks; unescape literal \n in publish bodies.
• Delta reviews: after last published <!-- ai-review-meta -->, re-runs review prior_head…HEAD (full on missing/inconclusive meta, rebase, base change, or force-full-review).
• Test Plan ↔ CI: inventory CI; uncovered Test Plan items → findings; checklist tick / PR-body write-back retired.
• OSH fan-out: route by roster K — K≤1 Sonnet collapse; K>1 Opus parent + Sonnet Task workers (osh-* agents; no Haiku Task children). Size-based sonnet-*-threshold routing deprecated.

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 (324+ pass)
• Selftest on combined tip: review PASS / APPROVED (gated-demo green)
• Fresh PR: full review + meta marker on published body
• Push follow-up: delta mode + prior-review carry-forward
• force-full-review: true forces full range with valid prior meta
• Test Plan items with weak/no CI → findings; PR body checkboxes unchanged
• Small diff (K=1): Sonnet collapse; large multi-file (K>1): Opus + Task

Made with Cursor

✅ ai-qa: PASS

Deploy health: healthy · findings P0 0 · P1 0 · P2 0 · P3 0

Last updated by ai-qa · QA run

Superseded by a new ai-review run.

[Hussam Aldarwish (eduly-haldarwish)]

Added delta coverage for the incremental-review ask:

• Unit: write-delta.test.js now uses a real git history — delta only includes the follow-up commit’s files; rewritten history → full; CLI main() emits prep KEY=value lines.
• Selftest: after a non-empty verdict, asserts the published review has parseable <!-- ai-review-meta head_sha=… base_sha=… mode=full|delta|inconclusive --> (and tip match for full/delta).

A live second-push delta run still isn’t in CI (would re-enter the workflow / cancel-in-progress); the unit tests cover that range math.

Superseded by a new ai-review run.

Superseded by a new ai-review run.

[mtm-bot]

✅ PASS

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

Confidence: 65 · Merge risk: medium
P0: 0 · P1: 0 · P2: 7 · P3: 11

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

This delta makes context.md deterministic — prep always writes it — and auto-skips the Haiku enrichment stage on delta + roster K≤1. Reviewed as a delta against 73aba88 (7 files, 340 lines churn) with an OSH fan-out: two coverage reviewers, a cross-file tracer, a history pass, and an independent scorer.

Intent (Angle H, framed before the diff): linked-issues.json is empty, so the contract is the PR body. This delta's performance work is not one of the four stated workstreams there — but it is a coherent, well-scoped extension of the delta-review workstream rather than a deviation, and the README and input documentation were updated in the same commit. Judged aligned.

P0 — Blockers

None.

P1 — Should Fix

None.

P2 — Nice-to-Have

• ai-review/action.yml:509 — the new auto-skip branch has no test coverage of any kind. grep -rn "run-context-stage|RUN_CONTEXT_STAGE|ENABLE_CONTEXT_STAGE" ai-review/lib/ .github/workflows/ returns nothing. This is the central behavioral change of the delta, and it lives entirely in shell inside a composite action — the one layer with no harness. selftest.yml deliberately passes no enable-context-stage (correct, so the default path stays exercised), but nothing asserts which branch was taken. A future edit that inverts the condition would skip Haiku on every review, or run it on every review, with a fully green suite. The logic itself is sound: REVIEW_MODE is defaulted at :462 before the :511 comparison, K is guarded as ${K:-0}, and the run-context-stage key is spelled consistently at :558, :608, and :700.
• ai-review/action.yml:1062 — RETRY_BUDGET_MS still justifies itself with a 25-minute timeout that no longer exists. (Carried forward, re-verified.) The budget is 15 minutes while README.md:327 recommends timeout-minutes: 55. A first attempt running 16 minutes skips the retry with ~39 minutes of headroom left.
• ai-review/README.md:130 — per-role findings/<role>.json artifacts the roster path does not produce. (Carried forward, narrowed on fresh evidence.) The scorer partially refuted the original framing: action.yml:1564-1566 does write .ai-review/findings/review-serial.json in shadow-aggregation mode, so the directory is not wholly fictional. The multi-role framing at :130 remains aspirational.
• ai-review/lib/testplan-ci.js:140 — same-rank duplicate check rows resolve to last-in-array. (Carried forward.) Two completed rows for one check name tie on rank, and the Checks API does not guarantee order, so a stale failure can overwrite a newer success. Bounded: this feeds the advisory inventory, not the deterministic CI_SIGNAL.
• ai-review/action.yml:1871 — the happy-path createReview is still unwrapped while addLabels/removeLabel directly below it are hardened. Fail-safe in direction, but it fails by job death rather than by the deterministic non-pass verdict the design intends. (Carried forward.)
• ai-review/action.yml:710 — SIGNAL reads the raw check-runs array while ci-checks.json is deduped. (Carried forward.) The two views can disagree on precisely the duplicate case checkRowRank exists to handle. Direction is fail-safe.
• ai-review/lib/action-refs.js:13 — the reference linter validates step IDs but not output keys. STEP_REF_RE = /steps\.([\w-]+)\./g captures the step id only. A typo in run-context-stage on either the producer or a consumer side would evaluate to the empty string, making == 'true' false and disabling the Haiku stage permanently and silently. A pre-existing limitation, but this delta's new wiring is what falls into it.

P3 — Nits

• ai-review/action.yml:10 — the action's own top-level description: still says the Haiku stage always summarizes the diff first. The README was updated correctly; this in-file description was not, so the marketplace listing now contradicts the behavior.
• ai-review/lib/write-context.test.js:55 — the full-mode else if (m.base_sha) branch is never asserted. Test 1 has no base_sha, test 2 is delta mode, test 3 omits it — so the "active diff base (merge-base)" line is dead code in the suite. One line added to test 3 closes it.
• ADR 0006 and the workstream design spec don't mention the auto-skip. grep -rn "context" docs/adr/0006-…md returns nothing; the spec references the context stage only as a model-ID table row. The decision trail no longer explains why Haiku became conditional. (ADR 0003's "best-effort, read if present" claim still holds — this change strengthens it.)
• ai-review/lib/write-context.js — no byte or count budget on the changed-file and symbol lists. Marginal: roster.js already bounds per-reviewer file and byte budgets upstream.
• ai-review/rubric.md:412, README.md:219, action.yml:92 — all three still claim P3 findings lower the reported confidence, but recompute.js:90 has no P3 term. (Carried forward, re-verified in all three locations.)
• ai-review/lib/write-delta.test.js:109,150,190,228 — bare fs.rmSync while :285,328,390 use try/finally. (Carried forward.)
• ai-review/lib/publish.js:82 — modelFooter(_modelUsed) still discards its parameter while resolveModelUsed parses three logs on every publish. (Carried forward.)
• ai-review/README.md:243 — no blank line before the ## Test Plan ↔ CI heading. The file was edited again in this delta without fixing it. (Carried forward.)
• .github/workflows/selftest.yml:19 — the K=1 Sonnet collapse path still has no CI coverage; this run is K=2. Newly relevant: the auto-skip fires only on delta + K≤1, so the untested collapse lane is now the only lane where the new skip triggers end-to-end.
• .github/workflows/selftest.yml:57 — force-full-review still has no CI lane that sets it. (Carried forward, narrowed — the prior delta added genuine unit coverage; only the wiring is unasserted.)
• .github/workflows/selftest.yml:72 — Test Plan item 4 has no end-to-end assertion. The comment's reason for declining a second push (racing cancel-in-progress) is sound; recorded as a residual gap rather than counted as coverage.

Prior review — carry-forward status

The prior review at 73aba88 was PASS with 0 P0 / 0 P1 / 8 P2 / 9 P3. This delta touches none of the files those findings live in except README.md and action.yml, and none of the open findings were closed by it. All are restated above, each re-verified this session with a fresh command rather than copied forward. Two were revised on new evidence: the findings/<role>.json claim is narrower than stated (shadow mode does write one file), and force-full-review drops to P3 now that real unit coverage exists.

Also carried at prior severity but outside the active range, listed so they are not dropped silently: testplan-ci.test.js:149's ordering-pinned dedupe test, delta.js:17's permissive \S+ SHA pattern, publish.js:38's literal < real newline heuristic, testplan-ci.js:139's callerless findObviousUncoveredItems, osh-agents.json:41's scorer tool grant, and rubric.md:178's commitlint P0/P2 split.

Test Plan ↔ CI mapping

CI inventory: Lint workflow and action files ✅, Static analysis of GitHub Actions workflows ✅, duplicated-snippets ✅, node-test ✅, review (this run, in progress).

• Covered: items 1–2 — node --test maps to node-test, whose glob picks up the new write-context.test.js; the selftest verdict lane is green. Item 3 (meta marker) is asserted by the step added in a prior commit. Item 6 (Test Plan gaps → findings, checkboxes untouched) is exercised by this review itself.
• Partially covered: item 4 — unit-tested against a real git history and demonstrated live by this run, but not asserted end-to-end.
• Uncovered: item 5 (force-full-review, wiring only) and item 7 (K=1 collapse). Both filed as findings above rather than assumed covered. Item 7 is the notable one for this delta — the new auto-skip fires only on the K≤1 path that no CI lane exercises.

PR-body checkboxes were not modified; CI, not ticks, is the coverage source here.

Strengths

• The skip removes exposure to a known failure mode, not just wall-clock. action.yml:825-827 records that three of four historical gateway stalls hit the Context stage, and that removing the stage is "the only measure so far shown to avoid the failure, as opposed to bounding it." Skipping it on small deltas applies that evidence where the enrichment is worth least. That reasoning is written down at the site rather than left in a commit message.
• The deterministic write lands before the optional stage, so Review never loses handoff. The ordering is the whole point of the design and it's implemented correctly: writeRoster writes context.md at prep time, and the Haiku stage only ever enriches. A stalled or skipped Haiku now degrades to a factual context file instead of no context at all — which is what makes skipping it safe in the first place.
• The failure path is genuinely fail-open. The new block in write-manifest.js wraps the context write in try/catch and downgrades any error to a ::warning::, so a broken context write cannot sink a review. The Verify context.md handoff step branches on RAN_HAIKU_CONTEXT to give an accurate diagnostic either way, and a missing output degrades to the generic warning rather than erroring.
• Telemetry was updated to match, so the skip cannot masquerade as a stall. metrics.js has NOT_RUN.ran = false, isStalled requires ran === true, collectMetrics maps a null log to NOT_RUN, and renderSummary prints _skipped_. metrics.test.js:219 ("a skipped stage is not a stall") pins it. Extending an observability contract alongside the behavior change is the part most easily forgotten.
• The test-only no-op branch does not weaken the production path. writeRoster's io seam skips the real write unless a test supplies writeContext, avoiding cwd pollution — and main() calls writeRoster(manifest, sizes) with no io at :151, so production always resolves the real writer. write-manifest.test.js:427-428 backs this with a real-disk main() assertion under a proper chdir/try/finally, so the injection seam cannot silently become the shipped behavior.
• The self-test comment argues for what it deliberately does not configure. selftest.yml:61-65 explains that omitting enable-context-stage is intentional, so the self-test exercises the same default consumers get, and warns against forcing false because that would leave the default path untested. That is a real trade-off recorded at the decision point.
• Documentation was updated at every layer a consumer actually reads. README item 9, the enable-context-stage input row at :222, and the input description at action.yml:113-120 all describe the deterministic write and the delta + K≤1 auto-skip consistently with the implementation. The gap is confined to the ADR trail.

Verification notes

Per instruction, the project's tests were not run and no toolchain was probed — test_execution: "skipped". Every claim above rests on a command run this session; the commands and their output are recorded in verification_evidence. The cross-file tracer found no coherence breakage in the new write-manifest.js → write-context.js wiring. Confidence and severity were assigned by an independent scorer, not by the reviewers who raised the findings — the scorer returned several values off the 0/25/50/75/100 scale (90, 85, 60, 40), which were snapped to the nearest allowed step rather than re-derived.

One gap worth stating plainly: OSH_MODE and PR_NUMBER could not be read from the environment (the shell allowlist rejected every attempt). Fan-out mode was inferred from .ai-review/osh-policy.md being the fan-out policy document, corroborated by assignments.json's k: 2; the PR number came from the history worker's gh pr view. The inference is well-supported but it is an inference.

context.md is untracked and not gitignored, so it shows in git status after a local prep run. No workflow gates on a clean tree (grep for porcelain and git diff --exit-code across .github/workflows/ and action.yml returns nothing), so this is cosmetic — but adding it to .gitignore would keep local git status clean.

[mtm-bot]

✅ ai-qa: PASS

Merge commit a65ff192c75a

Signal	Result
Deploy health (https://raw.githubusercontent.com/EdulyCom/github-actions/main/README.md)	✅ healthy
QA review	⚠️ did not run — reporting deploy-health signal only

Posted by mtm-bot[bot] · workflow run