From 19a603cd9967dc9af9ae46d545e5a8cdc567e9fe Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 13:08:04 +0300 Subject: [PATCH 01/16] docs: add ai-review OSH+delta+testplan design spec --- ...-08-07-ai-review-parallel-review-design.md | 3 + ...-17-ai-review-osh-delta-testplan-design.md | 344 ++++++++++++++++++ 2 files changed, 347 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md diff --git a/docs/superpowers/specs/2026-08-07-ai-review-parallel-review-design.md b/docs/superpowers/specs/2026-08-07-ai-review-parallel-review-design.md index b13f898..50c3246 100644 --- a/docs/superpowers/specs/2026-08-07-ai-review-parallel-review-design.md +++ b/docs/superpowers/specs/2026-08-07-ai-review-parallel-review-design.md @@ -1,6 +1,9 @@ # `ai-review` parallel code-review architecture — design **Status:** approved shape, not yet implemented. Sequencing in §8 governs how it lands. +**Extended by:** [`2026-08-17-ai-review-osh-delta-testplan-design.md`](./2026-08-17-ai-review-osh-delta-testplan-design.md) +(delta baseline, Test Plan ↔ CI findings / no checklist ticks, OSH ship order). That document does +not rewrite this one; §1 binding constraints and the scoring/`recompute.js` contract here still apply. **Supersedes:** the 8-angle/14-section vendored rubric as the *finding* methodology (the rubric's *scoring* vocabulary survives — see §3). **Authors:** Opus (architecture), Fable (coverage/coherence redesign + calibration review), synthesized diff --git a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md new file mode 100644 index 0000000..a383fa2 --- /dev/null +++ b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md @@ -0,0 +1,344 @@ +# `ai-review` OSH + delta + Test-Plan/CI — design + +**Status:** approved shape, not yet implemented. Sequencing in §8 governs how it lands. +**Extends:** [`2026-08-07-ai-review-parallel-review-design.md`](./2026-08-07-ai-review-parallel-review-design.md) +(parallel OSH / coverage–coherence / rubric scoring). This document does **not** supersede that +design’s §1 binding constraints or its scoring/`recompute.js` contract. +**Authors:** synthesized from a live design session with the repo owner (2026-08-17), freezing the +locked product decisions below. + +--- + +## 1. Purpose + +Three changes land as one end-state for `ai-review`: + +1. **Delta reviews** — on re-runs, review only commits after the last published ``, + not the full merge-base…HEAD range every time. +2. **Test Plan ↔ CI** — map PR Test Plan / checklist items to CI coverage; gaps become normal findings + (P0–P3). Stop ticking checklist boxes in the PR body. +3. **OSH fan-out** — Opus parent manages native Sonnet workers and Haiku helpers inside one + `claude-code-action` invocation, collapsing to a single reviewer when the roster says `K=1`. + +Together they cut cost and wall-clock on `synchronize` / re-runs, make Test Plan signal honest (CI +coverage, not “verified” ticks), and deliver the parallel OSH orchestration the 2026-08-07 design +already specified — without replacing how findings are scored or gated. + +--- + +## 2. Relationship to the parallel OSH design (2026-08-07) + +The 2026-08-07 document remains the source of truth for: + +- Binding constraints (§1): **rubric scoring stays**; **must read all** for the active review range. +- Coverage / coherence / intent / history roles, `K = clamp(ceil(bytes / BUDGET), 1, 4)`, cluster + packing, independent Haiku scoring, deterministic aggregation. +- Fail-closed matrix and frozen artifact shapes (`assignments.json`, per-role findings, gate input). +- No Node `Promise.race` orchestrator — concurrency is Claude Code’s subagent scheduler inside one + process. + +This 2026-08-17 document **adds**: + +- A durable **delta baseline** (HTML meta on the published review) and invalidate → full rules. +- **Test Plan vs CI** finding semantics and retirement of checklist tick write-back. +- An explicit **ship order** that lands delta and Test-Plan/CI *before* OSH fan-out, because gateway + stall risk (#43 / ADR 0005) multiplies with concurrent subagent surface. + +Where the two conflict on product intent, treat this document as the later refinement for delta, +Test-Plan/CI, and orchestration *delivery*; treat 2026-08-07 as authoritative for scoring vocabulary, +must-read-all, roster math, and aggregation. + +--- + +## 3. Locked product decisions + +| Decision | Choice | +|---|---| +| Orchestration | Opus parent + native Sonnet/Haiku subagents in one `claude-code-action` invocation | +| Delta baseline | Commits after last published `` with meta `head_sha` | +| Full review when | First run, missing meta, inconclusive prior, force-push/non-ancestor, base change, `force-full-review` | +| Test plan | Map to CI; gaps → findings P0–P3; stop ticking checklist boxes in PR body | +| Ship order | Slice 1 delta → Slice 2 test-plan/CI → Slice 3 OSH fan-out | + +Rejected alternatives (recorded so they are not re-opened casually): + +- Separate composite stages per worker / Opus-only judge stage (orchestration option B). +- Always re-run a full-PR intent+cross-file pass on every delta (delta option B). +- Delta = `github.event.before…after` only (weaker on rebase / manual re-runs). +- Floor uncovered Test Plan items at P1 or auto-classify regression gaps as P0 (severity stays + model-judged via the rubric, then `recompute.js`). + +--- + +## 4. Binding constraints carried forward + +From parallel design §1 — still non-negotiable: + +- **Rubric scoring stays.** P0–P3 severity vocabulary, confidence formula, and `recompute.js` input + contract are unchanged. What changes is *how findings are produced* (OSH roles, Test Plan gaps as + findings, delta range), never how they are scored or gated. P2/P3 never block; gate uses + `gateConfidence` (ADR 0004). +- **Must read all (active range).** Full contents of every file in the **active review range**, + always. No sampling, no truncation, no diff-hunk-only reasoning. In full mode the active range is + every file changed merge-base…HEAD. In delta mode it is every file touched in + `prior_head…HEAD`. Neighbor files outside the delta may be read when prior findings or imports + require it (prompt rule); that is targeted expansion, not a silent downgrade of must-read-all on + the active set. + +Additional constraints that remain binding for this work: + +- No test runners / package managers in the allowlist (ADR 0004 / README). `test_execution` stays + `"skipped"`. +- Fail closed on missing structured output. +- Schema stages: Claude primary → structured-output-capable free fallbacks only (no Cursor on + `--json-schema` stages). Context (non-schema) may still use the Claude → Cursor → free cascade. +- Injection safety: attacker-controlled content via files / `env:`, never `${{ }}` interpolation into + `run:` / `script:` bodies (ADR 0001). +- Never untick human checklist boxes; under this design, **stop ticking** verified boxes entirely. + +--- + +## 5. OSH roles (orchestration) + +```mermaid +flowchart TD + prep[Prep_manifest_delta_CI] + opus[Opus_parent] + workers[Sonnet_Haiku_subagents] + schema[Structured_output] + recompute[recompute_Publish] + + prep --> opus + opus --> workers + workers --> opus + opus --> schema + schema --> recompute +``` + +### 5.1 Role map + +| Tier | Who | Mandate | +|---|---|---| +| **O**pus (parent) | One `claude-code-action` session with `--model` Opus when fan-out is live | Intent isolation ownership (or dispatch of the intent role), prioritization, conflict resolution across worker outputs, final structured judgment (`comment_markdown` + schema fields). Does **not** exhaustively re-read every file on large diffs when workers already covered them. | +| **S**onnet (workers) | Coverage cluster reviewers R1..Rk from `roster.js` / `assignments.json`; tracer / coherence as in 2026-08-07 §4 | Full-file reads of assigned paths; propose findings with severity. | +| **H**aiku (helpers) | History / mechanical gathers + **independent confidence scoring** | Cheap collection and scoring that must not be the same model that found the issue (parallel design §3). | + +Only the **Opus parent** emits `--json-schema` structured output for Publish. Workers return freeform +or JSON files under `.ai-review/`; the parent aggregates into the schema contract. Aggregation +(`lib/aggregate.js` when landed) and `recompute.js` remain deterministic consumers — model-reported +counts are never trusted as gate inputs. + +### 5.2 Collapse when K=1 + +`K` is still the read-budget from parallel design §5: + +``` +K = clamp(ceil(total_fullfile_bytes / BUDGET), 1, 4) +``` + +Cap remains **K≤4**. Fan-out is an option when the work exceeds one reviewer’s comprehension budget, +not a fixed pipeline every PR pays for. + +**Collapse rule (this design):** + +- If the roster / `assignments.json` implies **K=1** (single coverage reviewer holds the whole + active-range byte budget): run a **single Sonnet** review session — no Opus parent fan-out. Same + artifact contracts and fail-closed path; lower cost. +- If **K>1**: Opus parent + native Sonnet/Haiku subagents consuming `assignments.json`. + +Prep always emits the manifest and roster. Topology collapses by roster size, not by a second +code-path architecture. Diff-size Sonnet-vs-Opus routing thresholds (`sonnet-files-threshold` / +`sonnet-churn-threshold`) are deprecated once Slice 3 is live; they may remain accepted for backward +compatibility until removed in a follow-up. + +### 5.3 What this deliberately is not + +There is **no** separate Node orchestrator, no per-worker composite stage matrix, and no +`Promise.race` around SDK subprocesses. Concurrency is Claude Code’s own subagent scheduler inside +one process; wall-clock is bounded by the caller’s job `timeout-minutes`. + +--- + +## 6. Delta baseline and invalidate rules + +### 6.1 Meta marker (exact format) + +Published review bodies carry a machine-readable baseline immediately after ``: + +```html + +``` + +- `head_sha` — the PR HEAD that was reviewed. +- `base_sha` — the merge-base (or documented review base) at publish time. +- `mode` — `full` or `delta` for successful structured reviews. + +For **inconclusive** publishes: omit `mode` or set `mode=inconclusive` so the next run cannot treat +that body as a valid delta baseline (forces full). + +Banner / strip logic that already removes `` for display must continue to strip the +meta line cleanly. + +### 6.2 Resolving the baseline + +On each run, Prep (or a dedicated step after identity): + +1. List PR reviews from the bot. +2. Find the latest body containing `` with parseable meta and a usable `head_sha`. +3. Decide `mode: full | delta` and write `.ai-review/delta.json` (plus optional + `.ai-review/prior-review.md` for carry-forward). + +**Delta mode** when a valid prior exists: review range is `prior_head_sha…current_head_sha` (commits +and files after the last published review). The review prompt must read prior findings and mark +resolved vs still-open. + +**Full mode** when any of the following hold: + +| Trigger | Why | +|---|---| +| First run on the PR (no prior ``) | No baseline | +| Missing or unparseable meta | Cannot trust prior HEAD | +| Prior review inconclusive (`mode` absent / `inconclusive`) | Prior judgment incomplete | +| Force-push / prior `head_sha` not an ancestor of current HEAD | History rewritten | +| Base branch SHA change (merge-base moved materially vs meta `base_sha`) | Diff identity changed | +| Input `force-full-review: true` | Operator override | + +Manifest fields (additive; keep existing `base_sha` / `head_sha` as PR merge-base and HEAD for +telemetry): + +- `review_mode`: `full` \| `delta` +- `delta_base_sha`: nullable (prior head when delta) +- `prior_head_sha`: nullable + +### 6.3 Must-read-all under delta + +“Must read all” applies to **files in the active range**. Cross-file risks that span outside the +delta: the Opus parent (or Sonnet worker under parent direction) must pull neighbor files when prior +findings or imports require it. Default behavior is not a second full-PR exhaustive read. + +--- + +## 7. Test Plan ↔ CI finding rules + +### 7.1 Intent + +The review does **not** execute the Test Plan and does **not** mark checklist items verified in the +PR body. CI is the source of truth for “was this exercised?”; uncovered Test Plan intent becomes a +**finding**, not a tick. + +### 7.2 Prep artifacts + +- Parse PR body Test Plan section and/or checklist items → `.ai-review/test-plan-items.json`. +- Inventory workflow jobs / check runs for the PR (all PR events, not only `workflow_dispatch`) → + `.ai-review/ci-checks.json`. +- Mapping quality may be model-assisted in the review prompt; pure helpers extract and summarize. + +### 7.3 Finding contract + +- Each Test Plan / checklist item that is **not** covered (or only weakly covered) by CI becomes a + normal `findings[]` entry with severity **P0–P3** judged via the existing rubric. +- Gate rules unchanged: P0/P1 can fail; P2/P3 never block; `recompute.js` unchanged. +- Do **not** populate checklist-for-ticking fields for Publish write-back (schema field may remain an + empty array). + +### 7.4 Stop ticking + +- Disable / remove Publish paths that call `tickVerifiedBoxes` or status-block checklist write-back + for verified items (`update-pr-body` checklist behavior off for this path). +- Linked-issue updates that are independent of checklist ticks may remain if unchanged by this work. +- Never untick boxes humans already checked. + +### 7.5 Constants that stay + +- `test_execution: skipped`, no runner allowlist, no package-manager execution from PR-head content. + +--- + +## 8. Ship order + +Implement as three sequential slices. Do not start Slice 3 until Slice 1 and Slice 2 are landed and +stable enough that fan-out is not the first multiplier on an unfinished baseline/Test-Plan path. + +| Slice | Deliverable | Why this order | +|---|---|---| +| **1 — Delta** | Meta marker, baseline resolver, manifest/prompt range wiring, `force-full-review` | Immediate token and wall-clock win on re-runs; works with today’s serial review | +| **2 — Test-Plan/CI** | CI inventory + items artifacts; findings for gaps; stop checklist ticks | Honest Test Plan signal without waiting on OSH | +| **3 — OSH fan-out** | Opus parent + Sonnet/Haiku subagents from roster; K=1 collapse; deprecate size-based model routing | Largest stall-surface change; ship after cheaper slices reduce how often / how large full reviews are | + +Follow-up docs (README / plan / short ADR note that checklist tick write-back is retired) land after +the slices or alongside Slice 2–3 as consumer-facing copy catches up. + +--- + +## 9. Non-goals + +- **No test execution** in the review stage (already removed; this design does not restore it). +- **No separate stage orchestrator** (rejected option B): no matrix of worker jobs and no Node + Promise.race wrapper around the SDK. +- **No fix for gateway stall #43 / ADR 0005** in this workstream — fan-out multiplies surface; keep + `K≤4` and ship delta + Test-Plan/CI first. +- **No ai-qa redesign** unless a follow-up explicitly opens it. +- **No rewriting** of historical dismissed review bodies on GitHub to backfill meta. +- **No replacement** of `recompute.js` or the fail-closed Publish contract. + +--- + +## 10. Frozen interfaces (additive) + +These names are frozen for implementation parcels; details may deepen in code but the contracts +below must not silently change meaning. + +**`.ai-review/delta.json`** (Slice 1) — example shape: + +```json +{ + "schema": 1, + "mode": "delta", + "reason": "prior-meta-ancestor", + "delta_base_sha": "abc…", + "prior_head_sha": "abc…", + "head_sha": "def…", + "merge_base_sha": "…", + "prior_body_path": ".ai-review/prior-review.md" +} +``` + +**Meta helpers** (Slice 1): + +- `parseReviewMeta(body) → { headSha, baseSha, mode } | null` +- `formatReviewMeta({ headSha, baseSha, mode }) → ''` +- `resolveDeltaBaseline({ reviews, headSha, mergeBaseSha, forceFull }) → { mode, deltaBaseSha, priorHeadSha, priorBody, reason }` + +**Test-Plan/CI helpers** (Slice 2): + +- `extractTestPlanItems(prBody) → string[]` +- `summarizeCiChecks(checkRuns) → { name, conclusion, status }[]` + +**Roster / assignments** (Slice 3) — continue to follow 2026-08-07 §6; parent prompt consumes +`assignments.json` and must not invent a second partition scheme. + +--- + +## 11. Risk notes (design-level) + +| Risk | Mitigation | +|---|---| +| Delta under-reads cross-file breakage | Prompt rule: expand to neighbors when prior findings / imports require; invalidate → full on rebase/base change | +| Fan-out × gateway stall (#43) | Ship order Slice 1→2→3; K≤4; single parent SO | +| Resume / missing `session_id` on SO failure | Keep repair recovery + fail-closed + retry budget (already in cascade work) | +| Checklist semantics confusion | Docs: ticks retired; findings carry uncovered Test Plan signal | +| Subagents emit schema | Forbidden — only Opus parent (or collapsed single Sonnet session) owns `--json-schema` | + +--- + +## 12. Self-consistency checklist + +This design is complete when: + +- Locked decisions in §3 match the owner-approved brainstorm (2026-08-17). +- Parallel design §1 constraints (rubric scoring; must-read-all for the active range) are affirmed, + not weakened. +- Meta format is exactly the HTML comment in §6.1. +- Non-goals explicitly exclude test execution, separate stage orchestrators, and fixing #43 here. +- Ship order is Slice 1 delta → Slice 2 test-plan/CI → Slice 3 OSH, with K≤4 collapse at K=1. +- `recompute.js`, fail-closed Publish, and SO-free cascade for schema stages remain in force. From 32612b6775b1c190c39ce3d7454dd26bb8628770 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 13:12:51 +0300 Subject: [PATCH 02/16] docs: clarify K=1 collapse and base-change invalidate --- ...-17-ai-review-osh-delta-testplan-design.md | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md index a383fa2..743a3e1 100644 --- a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md +++ b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md @@ -46,7 +46,9 @@ This 2026-08-17 document **adds**: Where the two conflict on product intent, treat this document as the later refinement for delta, Test-Plan/CI, and orchestration *delivery*; treat 2026-08-07 as authoritative for scoring vocabulary, -must-read-all, roster math, and aggregation. +must-read-all, roster math, and aggregation. **Exception (K=1 collapse):** §5.2 of this document +overrides parallel design §5’s minimum roster `{R1,H}` — the collapse path is a single Sonnet session +with no Opus parent and no independent Haiku scorer (see §5.2). --- @@ -139,17 +141,21 @@ K = clamp(ceil(total_fullfile_bytes / BUDGET), 1, 4) Cap remains **K≤4**. Fan-out is an option when the work exceeds one reviewer’s comprehension budget, not a fixed pipeline every PR pays for. -**Collapse rule (this design):** +**Collapse rule (this design — overrides parallel design §5 minimum `{R1,H}`):** - If the roster / `assignments.json` implies **K=1** (single coverage reviewer holds the whole - active-range byte budget): run a **single Sonnet** review session — no Opus parent fan-out. Same - artifact contracts and fail-closed path; lower cost. -- If **K>1**: Opus parent + native Sonnet/Haiku subagents consuming `assignments.json`. - -Prep always emits the manifest and roster. Topology collapses by roster size, not by a second -code-path architecture. Diff-size Sonnet-vs-Opus routing thresholds (`sonnet-files-threshold` / -`sonnet-churn-threshold`) are deprecated once Slice 3 is live; they may remain accepted for backward -compatibility until removed in a follow-up. + active-range byte budget): run a **single Sonnet** review session that emits `--json-schema` + structured output directly — **no Opus parent**, **no independent Haiku scorer**. Artifact + contracts may match today’s single-session shape (not full findings/scores fan-in). Fail-closed + path unchanged; lower cost. +- If **K>1**: Opus parent + native Sonnet/Haiku subagents consuming `assignments.json`. Independent + Haiku scoring applies only on this path (finder ≠ scorer), per parallel design §3 / §7b. + +Prep always emits the manifest and roster. For **K>1**, topology still collapses by roster size +inside the Opus parent; for **K=1**, the topology is the collapsed single-session path above (not +the parallel design’s `{R1,H}` minimum). Diff-size Sonnet-vs-Opus routing thresholds +(`sonnet-files-threshold` / `sonnet-churn-threshold`) are deprecated once Slice 3 is live; they may +remain accepted for backward compatibility until removed in a follow-up. ### 5.3 What this deliberately is not @@ -200,7 +206,7 @@ resolved vs still-open. | Missing or unparseable meta | Cannot trust prior HEAD | | Prior review inconclusive (`mode` absent / `inconclusive`) | Prior judgment incomplete | | Force-push / prior `head_sha` not an ancestor of current HEAD | History rewritten | -| Base branch SHA change (merge-base moved materially vs meta `base_sha`) | Diff identity changed | +| Base branch SHA change (current merge-base SHA ≠ meta `base_sha`) | Diff identity changed | | Input `force-full-review: true` | Operator override | Manifest fields (additive; keep existing `base_sha` / `head_sha` as PR merge-base and HEAD for From 6a1ac347ac63f3f96c8851390a6feb5412e7cffc Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 13:18:32 +0300 Subject: [PATCH 03/16] feat(ai-review): add delta meta parse/resolve and publish marker Slice 1 helpers for OSH redesign: parse/format ai-review-meta, resolve full vs delta range, and stamp the marker on published review bodies. --- ai-review/lib/delta.js | 217 ++++++++++++++++++++++++++++++++++ ai-review/lib/delta.test.js | 214 +++++++++++++++++++++++++++++++++ ai-review/lib/publish.js | 29 ++++- ai-review/lib/publish.test.js | 33 ++++++ 4 files changed, 491 insertions(+), 2 deletions(-) create mode 100644 ai-review/lib/delta.js create mode 100644 ai-review/lib/delta.test.js diff --git a/ai-review/lib/delta.js b/ai-review/lib/delta.js new file mode 100644 index 0000000..35e64ca --- /dev/null +++ b/ai-review/lib/delta.js @@ -0,0 +1,217 @@ +"use strict"; + +// Delta baseline for re-runs (spec §6 / frozen §10). +// +// Published review bodies carry: +// +// +// +// Prep (Task 3) will walk prior bot reviews, parse that meta, and decide +// full vs delta. This module is pure: no git, no GitHub I/O. The caller +// supplies whether prior head is an ancestor of current HEAD. + +const AI_REVIEW_MARKER = ""; + +// Exact key order matches the frozen format in the design spec. +const META_RE = + //; + +/** + * @param {string|null|undefined} body + * @returns {{ headSha: string, baseSha: string, mode: string|null } | null} + */ +function parseReviewMeta(body) { + if (typeof body !== "string" || !body.includes("ai-review-meta")) return null; + const m = body.match(META_RE); + if (!m) return null; + const headSha = m[1]; + const baseSha = m[2]; + const mode = m[3] || null; + if (!headSha || !baseSha) return null; + return { headSha, baseSha, mode }; +} + +/** + * @param {{ headSha: string, baseSha: string, mode?: string|null }} args + * @returns {string} + */ +function formatReviewMeta({ headSha, baseSha, mode }) { + if (!headSha || !baseSha) { + throw new Error("formatReviewMeta requires headSha and baseSha"); + } + if (mode == null || mode === "") { + return ``; + } + return ``; +} + +/** + * Resolve the git range for this review run from an already-parsed prior meta. + * + * - full → `baseSha` is the PR merge-base; `headSha` is current HEAD + * - delta → `baseSha` is the prior published head; `headSha` is current HEAD + * + * @param {{ + * priorMeta: { headSha: string, baseSha: string, mode: string|null } | null, + * headSha: string, + * mergeBaseSha: string, + * forceFull?: boolean, + * priorHeadIsAncestor?: boolean, + * }} args + * @returns {{ mode: 'full'|'delta', baseSha: string, headSha: string, reason: string }} + */ +function resolveReviewRange({ + priorMeta, + headSha, + mergeBaseSha, + forceFull = false, + priorHeadIsAncestor, +}) { + const full = (reason) => ({ + mode: "full", + baseSha: mergeBaseSha, + headSha, + reason, + }); + + if (forceFull) return full("force-full-review"); + if (!priorMeta || !priorMeta.headSha || !priorMeta.baseSha) { + return full("missing-or-unparseable-meta"); + } + if (!priorMeta.mode || priorMeta.mode === "inconclusive") { + return full("prior-inconclusive"); + } + if (priorMeta.mode !== "full" && priorMeta.mode !== "delta") { + return full("missing-or-unparseable-meta"); + } + // Spec §6.2: strict inequality on merge-base SHA. + if (mergeBaseSha !== priorMeta.baseSha) { + return full("base-sha-changed"); + } + if (priorHeadIsAncestor !== true) { + return full("prior-head-not-ancestor"); + } + return { + mode: "delta", + baseSha: priorMeta.headSha, + headSha, + reason: "prior-meta-ancestor", + }; +} + +/** + * Pick the chronologically latest PR review body that carries ``. + * @param {Array<{ body?: string, submitted_at?: string, submittedAt?: string, id?: number }>|null|undefined} reviews + * @returns {{ body: string, submitted_at?: string, id?: number } | null} + */ +function findLatestAiReview(reviews) { + if (!Array.isArray(reviews) || reviews.length === 0) return null; + const candidates = reviews.filter( + (r) => r && typeof r.body === "string" && r.body.includes(AI_REVIEW_MARKER) + ); + if (!candidates.length) return null; + + candidates.sort((a, b) => { + const ta = Date.parse(a.submitted_at || a.submittedAt || "") || 0; + const tb = Date.parse(b.submitted_at || b.submittedAt || "") || 0; + if (ta !== tb) return tb - ta; + return (Number(b.id) || 0) - (Number(a.id) || 0); + }); + return candidates[0]; +} + +/** + * Walk prior reviews and decide full vs delta (frozen §10). + * + * `priorHeadIsAncestor` must be supplied by the caller (git merge-base --is-ancestor); + * this module stays I/O-free. + * + * @param {{ + * reviews: Array<{ body?: string, submitted_at?: string, id?: number }>, + * headSha: string, + * mergeBaseSha: string, + * forceFull?: boolean, + * priorHeadIsAncestor?: boolean, + * }} args + * @returns {{ + * mode: 'full'|'delta', + * deltaBaseSha: string|null, + * priorHeadSha: string|null, + * priorBody: string|null, + * reason: string, + * }} + */ +function resolveDeltaBaseline({ + reviews, + headSha, + mergeBaseSha, + forceFull = false, + priorHeadIsAncestor, +}) { + if (forceFull) { + return { + mode: "full", + deltaBaseSha: null, + priorHeadSha: null, + priorBody: null, + reason: "force-full-review", + }; + } + + const prior = findLatestAiReview(reviews); + if (!prior) { + return { + mode: "full", + deltaBaseSha: null, + priorHeadSha: null, + priorBody: null, + reason: "no-prior-review", + }; + } + + const meta = parseReviewMeta(prior.body); + if (!meta || !meta.headSha) { + return { + mode: "full", + deltaBaseSha: null, + priorHeadSha: null, + priorBody: prior.body, + reason: "missing-or-unparseable-meta", + }; + } + + const range = resolveReviewRange({ + priorMeta: meta, + headSha, + mergeBaseSha, + forceFull: false, + priorHeadIsAncestor, + }); + + if (range.mode === "delta") { + return { + mode: "delta", + deltaBaseSha: meta.headSha, + priorHeadSha: meta.headSha, + priorBody: prior.body, + reason: range.reason, + }; + } + + return { + mode: "full", + deltaBaseSha: null, + priorHeadSha: meta.headSha, + priorBody: prior.body, + reason: range.reason, + }; +} + +module.exports = { + AI_REVIEW_MARKER, + parseReviewMeta, + formatReviewMeta, + resolveReviewRange, + findLatestAiReview, + resolveDeltaBaseline, +}; diff --git a/ai-review/lib/delta.test.js b/ai-review/lib/delta.test.js new file mode 100644 index 0000000..ce7f72c --- /dev/null +++ b/ai-review/lib/delta.test.js @@ -0,0 +1,214 @@ +"use strict"; + +const test = require("node:test"); +const assert = require("node:assert/strict"); + +const { + parseReviewMeta, + formatReviewMeta, + resolveReviewRange, + findLatestAiReview, + resolveDeltaBaseline, +} = require("./delta.js"); + +const HEAD = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; +const PRIOR = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; +const BASE = "cccccccccccccccccccccccccccccccccccccccc"; +const BASE2 = "dddddddddddddddddddddddddddddddddddddddd"; + +// --- formatReviewMeta / parseReviewMeta ------------------------------------ + +test("formatReviewMeta emits the frozen HTML comment shape", () => { + assert.equal( + formatReviewMeta({ headSha: HEAD, baseSha: BASE, mode: "full" }), + `` + ); + assert.equal( + formatReviewMeta({ headSha: HEAD, baseSha: BASE, mode: "delta" }), + `` + ); +}); + +test("formatReviewMeta omits mode when absent (inconclusive-capable)", () => { + assert.equal( + formatReviewMeta({ headSha: HEAD, baseSha: BASE }), + `` + ); +}); + +test("parseReviewMeta round-trips a formatted marker", () => { + const line = formatReviewMeta({ headSha: HEAD, baseSha: BASE, mode: "delta" }); + const body = `\n${line}\n**✅ PASS**\n`; + assert.deepEqual(parseReviewMeta(body), { + headSha: HEAD, + baseSha: BASE, + mode: "delta", + }); +}); + +test("parseReviewMeta returns null for missing or garbage meta", () => { + assert.equal(parseReviewMeta(null), null); + assert.equal(parseReviewMeta(""), null); + assert.equal(parseReviewMeta("\n**✅ PASS**"), null); + assert.equal(parseReviewMeta(""), null); +}); + +test("parseReviewMeta accepts mode-less meta (mode null)", () => { + const body = ``; + assert.deepEqual(parseReviewMeta(body), { + headSha: HEAD, + baseSha: BASE, + mode: null, + }); +}); + +// --- resolveReviewRange ---------------------------------------------------- + +const rangeArgs = (over = {}) => ({ + priorMeta: { headSha: PRIOR, baseSha: BASE, mode: "full" }, + headSha: HEAD, + mergeBaseSha: BASE, + priorHeadIsAncestor: true, + ...over, +}); + +test("resolveReviewRange: valid prior → delta from prior head to current HEAD", () => { + assert.deepEqual(resolveReviewRange(rangeArgs()), { + mode: "delta", + baseSha: PRIOR, + headSha: HEAD, + reason: "prior-meta-ancestor", + }); +}); + +test("resolveReviewRange: missing meta → full from merge-base", () => { + assert.deepEqual(resolveReviewRange(rangeArgs({ priorMeta: null })), { + mode: "full", + baseSha: BASE, + headSha: HEAD, + reason: "missing-or-unparseable-meta", + }); +}); + +test("resolveReviewRange: inconclusive prior → full", () => { + assert.equal( + resolveReviewRange( + rangeArgs({ priorMeta: { headSha: PRIOR, baseSha: BASE, mode: "inconclusive" } }) + ).reason, + "prior-inconclusive" + ); + assert.equal( + resolveReviewRange( + rangeArgs({ priorMeta: { headSha: PRIOR, baseSha: BASE, mode: null } }) + ).reason, + "prior-inconclusive" + ); +}); + +test("resolveReviewRange: base SHA mismatch (strict) → full", () => { + const out = resolveReviewRange(rangeArgs({ mergeBaseSha: BASE2 })); + assert.equal(out.mode, "full"); + assert.equal(out.baseSha, BASE2); + assert.equal(out.reason, "base-sha-changed"); +}); + +test("resolveReviewRange: prior head not ancestor → full", () => { + assert.equal( + resolveReviewRange(rangeArgs({ priorHeadIsAncestor: false })).reason, + "prior-head-not-ancestor" + ); + assert.equal( + resolveReviewRange(rangeArgs({ priorHeadIsAncestor: undefined })).reason, + "prior-head-not-ancestor" + ); +}); + +test("resolveReviewRange: forceFull → full", () => { + assert.equal(resolveReviewRange(rangeArgs({ forceFull: true })).reason, "force-full-review"); +}); + +// --- findLatestAiReview / resolveDeltaBaseline ----------------------------- + +test("findLatestAiReview picks the newest body with ", () => { + const latest = findLatestAiReview([ + { + id: 1, + submitted_at: "2026-01-01T00:00:00Z", + body: "\nold", + }, + { + id: 2, + submitted_at: "2026-06-01T00:00:00Z", + body: `\n${formatReviewMeta({ headSha: PRIOR, baseSha: BASE, mode: "full" })}\n`, + }, + { id: 3, submitted_at: "2026-07-01T00:00:00Z", body: "human comment" }, + ]); + assert.equal(latest.id, 2); +}); + +test("resolveDeltaBaseline: no prior → full / no-prior-review", () => { + assert.deepEqual( + resolveDeltaBaseline({ + reviews: [], + headSha: HEAD, + mergeBaseSha: BASE, + }), + { + mode: "full", + deltaBaseSha: null, + priorHeadSha: null, + priorBody: null, + reason: "no-prior-review", + } + ); +}); + +test("resolveDeltaBaseline: valid meta + ancestor → delta", () => { + const body = [ + "", + formatReviewMeta({ headSha: PRIOR, baseSha: BASE, mode: "full" }), + "**✅ PASS**", + ].join("\n"); + const out = resolveDeltaBaseline({ + reviews: [{ id: 1, submitted_at: "2026-06-01T00:00:00Z", body }], + headSha: HEAD, + mergeBaseSha: BASE, + priorHeadIsAncestor: true, + }); + assert.deepEqual(out, { + mode: "delta", + deltaBaseSha: PRIOR, + priorHeadSha: PRIOR, + priorBody: body, + reason: "prior-meta-ancestor", + }); +}); + +test("resolveDeltaBaseline: marker without meta → full", () => { + const body = "\n**✅ PASS**"; + const out = resolveDeltaBaseline({ + reviews: [{ id: 1, submitted_at: "2026-06-01T00:00:00Z", body }], + headSha: HEAD, + mergeBaseSha: BASE, + priorHeadIsAncestor: true, + }); + assert.equal(out.mode, "full"); + assert.equal(out.reason, "missing-or-unparseable-meta"); + assert.equal(out.priorBody, body); +}); + +test("resolveDeltaBaseline: base change → full even when ancestor", () => { + const body = [ + "", + formatReviewMeta({ headSha: PRIOR, baseSha: BASE, mode: "delta" }), + ].join("\n"); + const out = resolveDeltaBaseline({ + reviews: [{ id: 1, submitted_at: "2026-06-01T00:00:00Z", body }], + headSha: HEAD, + mergeBaseSha: BASE2, + priorHeadIsAncestor: true, + }); + assert.equal(out.mode, "full"); + assert.equal(out.reason, "base-sha-changed"); + assert.equal(out.priorHeadSha, PRIOR); +}); diff --git a/ai-review/lib/publish.js b/ai-review/lib/publish.js index b04a5d6..193c1df 100644 --- a/ai-review/lib/publish.js +++ b/ai-review/lib/publish.js @@ -10,9 +10,21 @@ // `github`/`context` objects actions/github-script injects at runtime, and // this module intentionally has zero I/O. +const { formatReviewMeta } = require("./delta.js"); + const STATUS_BLOCK_START = ""; const STATUS_BLOCK_END = ""; +/** + * Meta line immediately after `` (spec §6.1). + * @param {{ headSha?: string, baseSha?: string, mode?: string|null }|null|undefined} reviewMeta + * @returns {string[]} + */ +function metaLines(reviewMeta) { + if (!reviewMeta || !reviewMeta.headSha || !reviewMeta.baseSha) return []; + return [formatReviewMeta(reviewMeta)]; +} + // Some structured-output paths deliver comment_markdown with literal // two-character "\n" sequences instead of real newlines (observed on // EdulyCom/github-actions#52 review 4949356509). GitHub then renders the @@ -61,8 +73,10 @@ function stripLeadingBannerArtifacts(markdown) { * @param {{verdict: string, confidence: number, mergeRisk: string, * counts: {p0:number,p1:number,p2:number,p3:number}, intentDeviated: boolean, * modelVerdict: string|undefined, blockers: string[], commentBody: string, - * modelUsed?: string|null}} args + * modelUsed?: string|null, + * reviewMeta?: { headSha: string, baseSha: string, mode: 'full'|'delta' }|null}} args * `commentBody` must already be run through stripLeadingBannerArtifacts. + * `reviewMeta` stamps the delta baseline (spec §6.1); omit until Publish wires SHAs. */ function modelLine(modelUsed) { if (!modelUsed || typeof modelUsed !== "string" || !modelUsed.trim()) return []; @@ -85,6 +99,7 @@ function buildReviewBody({ blockers, commentBody, modelUsed, + reviewMeta, }) { const verdictLine = verdict === "pass" ? "**✅ PASS**" : "**❌ FAIL**"; const rejectedBanner = intentDeviated ? "❌ **Rejected — wrong solution**\n\n" : ""; @@ -112,6 +127,7 @@ function buildReviewBody({ return [ "", + ...metaLines(reviewMeta), `${rejectedBanner}${verdictLine}`, ...(mismatchNote ? [mismatchNote] : []), ...(reasonNote ? [reasonNote] : []), @@ -127,13 +143,22 @@ function buildReviewBody({ /** * @param {string} salvaged possibly-empty text recovered from a missed structured output. - * @param {{modelUsed?: string|null}} [opts] + * @param {{modelUsed?: string|null, + * reviewMeta?: { headSha: string, baseSha: string }|null}} [opts] + * Inconclusive publishes omit mode or set mode=inconclusive so the next run + * cannot treat the body as a delta baseline (spec §6.1). */ function buildInconclusiveBody(salvaged, opts = {}) { const modelUsed = opts && opts.modelUsed; + const reviewMeta = opts && opts.reviewMeta; + const inconclusiveMeta = + reviewMeta && reviewMeta.headSha && reviewMeta.baseSha + ? { headSha: reviewMeta.headSha, baseSha: reviewMeta.baseSha, mode: "inconclusive" } + : null; salvaged = unescapeLiteralNewlines(salvaged || ""); return [ "", + ...metaLines(inconclusiveMeta), "### ⚠️ AI Review — inconclusive (re-run required)", "", "The review model did not return a structured result after a", diff --git a/ai-review/lib/publish.test.js b/ai-review/lib/publish.test.js index 3fcd932..d05ce3d 100644 --- a/ai-review/lib/publish.test.js +++ b/ai-review/lib/publish.test.js @@ -166,6 +166,26 @@ test("buildReviewBody omits Model line when modelUsed is empty", () => { assert.doesNotMatch(body, /^Model:/m); }); +test("buildReviewBody stamps ai-review-meta immediately after the marker", () => { + const body = buildReviewBody({ + ...BASE_ARGS, + reviewMeta: { + headSha: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + baseSha: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + mode: "delta", + }, + }); + assert.match( + body, + /^\n\n/ + ); +}); + +test("buildReviewBody omits meta when reviewMeta is absent", () => { + const body = buildReviewBody(BASE_ARGS); + assert.doesNotMatch(body, /ai-review-meta/); +}); + // --- buildInconclusiveBody --------------------------------------------------- test("without salvaged text there is no details block", () => { @@ -190,6 +210,19 @@ test("buildInconclusiveBody includes Model line but not a second italic re-run h assert.doesNotMatch(body, /_Re-run this job if you need another review pass\._/); }); +test("buildInconclusiveBody stamps meta with mode=inconclusive", () => { + const body = buildInconclusiveBody("", { + reviewMeta: { + headSha: "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa", + baseSha: "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb", + }, + }); + assert.match( + body, + /^\n\n/ + ); +}); + // --- tickVerifiedBoxes -------------------------------------------------------- test("ticks a single matching unchecked box", () => { From 155d3b6def6f84f5a5630f76793dbe46c5141eec Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 13:29:20 +0300 Subject: [PATCH 04/16] feat(ai-review): review delta range since last ai-review Wire prep to resolve full vs delta from prior review meta, drive numstat/diff from the active range, stamp publish meta, and document force-full-review. Preserve prior-review.md on forced full for carry-forward. --- ai-review/README.md | 32 ++++- ai-review/action.yml | 119 +++++++++++++---- ai-review/lib/delta.js | 66 ++++++++-- ai-review/lib/delta.test.js | 59 +++++++++ ai-review/lib/prep.js | 26 +++- ai-review/lib/prep.test.js | 28 ++++ ai-review/lib/publish.js | 3 +- ai-review/lib/write-delta.js | 185 +++++++++++++++++++++++++++ ai-review/lib/write-delta.test.js | 114 +++++++++++++++++ ai-review/lib/write-manifest.js | 9 +- ai-review/lib/write-manifest.test.js | 15 +++ 11 files changed, 613 insertions(+), 43 deletions(-) create mode 100644 ai-review/lib/write-delta.js create mode 100644 ai-review/lib/write-delta.test.js diff --git a/ai-review/README.md b/ai-review/README.md index 574563d..993aa22 100644 --- a/ai-review/README.md +++ b/ai-review/README.md @@ -56,13 +56,19 @@ injection-safety rule. 6. **Draft/closed gate** — `gh pr view` the PR; a draft, closed, or merged PR skips every remaining step (logs `skipped — PR is draft or closed`). 7. **Checkout / deterministic prep and model routing** — checks out the PR - head commit, then resolves the merge base, the changed-file list, each - file's full byte size at HEAD, a symbol manifest from the diff hunk - headers, and the Conventional-Commits title check into - `.ai-review/manifest.json`. The review stage is told to trust those - values instead of re-deriving them in paid model turns — a model that - derives the diff base from a false premise reviews the wrong range and - reports confidently on it. + head commit, then resolves the merge base, decides **full vs delta** + review from the last published `` meta (unless + `force-full-review` is set), writes `.ai-review/delta.json` and optional + `.ai-review/prior-review.md`, and stages the changed-file list for the + *active* range (delta = commits after the prior review HEAD; full = + merge-base…HEAD), each file's full byte size at HEAD, a symbol manifest + from the diff hunk headers, and the Conventional-Commits title check into + `.ai-review/manifest.json`. Manifest `base_sha`/`head_sha` stay the PR + merge-base and HEAD for telemetry; `review_mode` / + `delta_base_sha` / `prior_head_sha` describe the active range. The review + stage is told to trust those values instead of re-deriving them in paid + model turns — a model that derives the diff base from a false premise + reviews the wrong range and reports confidently on it. The same step routes ordinary diffs to the locked Sonnet primary (`claude/claude-sonnet-5`), escalating to Opus (`claude/claude-opus-5`) @@ -218,6 +224,18 @@ injection-safety rule. | `test-hint` | **DEPRECATED — accepted but ignored.** Same reason as `test-command`. | No | — | | `update-pr-body` | When `true`, the Publish step ticks verified checklist boxes in the PR description and maintains a managed `` block. Never unchecks a human-checked box. | No | `true` | | `update-linked-issues` | When `true`, the Review stage resolves and evaluates the issues the PR closes. ai-review only reads them; it never mutates issue state. | No | `true` | +| `force-full-review` | When `true`, always review merge-base…HEAD instead of a delta since the last published ai-review. | No | `false` | + +## Delta reviews + +On re-runs, prep looks for the latest published review body with +`` and a parseable +`` line. +When that prior `head_sha` is an ancestor of the current HEAD and the +merge-base matches, the active range is **delta** (`prior_head…HEAD`) — +smaller numstat / must-read set, with `.ai-review/prior-review.md` for +finding carry-forward. Full mode is used on first run, missing/inconclusive +meta, force-push (non-ancestor), base SHA change, or `force-full-review: true`. ## Outputs diff --git a/ai-review/action.yml b/ai-review/action.yml index 78f7c35..84fab36 100644 --- a/ai-review/action.yml +++ b/ai-review/action.yml @@ -214,6 +214,15 @@ inputs: state (that is ai-qa's post-merge job). required: false default: "true" + force-full-review: + description: > + When 'true', always review the full merge-base…HEAD range instead of + a delta since the last published ``. Use after a + rebase, base-branch change you want to force, or when debugging a + missed finding. Default 'false' — re-runs use delta when a valid + prior meta baseline exists. + required: false + default: "false" outputs: verdict: @@ -413,7 +422,10 @@ runs: FILES_MAX: ${{ inputs.sonnet-files-threshold }} CHURN_MAX: ${{ inputs.sonnet-churn-threshold }} MANIFEST_CLI: ${{ github.action_path }}/lib/write-manifest.js - GH_TOKEN: ${{ inputs.github-token }} + DELTA_CLI: ${{ github.action_path }}/lib/write-delta.js + GH_TOKEN: ${{ steps.identity.outputs.author-token }} + AUTHOR_LOGIN: ${{ steps.identity.outputs.author-login }} + FORCE_FULL_REVIEW: ${{ inputs.force-full-review }} REPO: ${{ github.repository }} PR_NUMBER: ${{ steps.pr.outputs.pr-number }} run: | @@ -440,18 +452,39 @@ runs: OPUS_FALLBACK="${FREE_SO}" export SONNET OPUS HAIKU - # Resolve the review range once, deterministically. `git diff A...B` - # already means "merge base of A and B", but resolving the SHA - # explicitly puts it in the manifest and the job log, so a wrong-range - # review becomes diagnosable after the fact instead of invisible. + # Resolve the PR merge-base once (telemetry + meta base_sha). The + # active review range may be a delta since the last published review. BASE_SHA="$(git merge-base "origin/${BASE_REF}" HEAD)" HEAD_SHA="$(git rev-parse HEAD)" - echo "prep: range ${BASE_SHA}..${HEAD_SHA}" - git diff --numstat "${BASE_SHA}" HEAD > .ai-review/numstat.txt + # List prior reviews by this bot; write-delta.js decides full vs delta + # (ancestor check via git), writes .ai-review/delta.json and optional + # prior-review.md. Fail open to [] → full mode on API errors. + if ! gh api --paginate "repos/${REPO}/pulls/${PR_NUMBER}/reviews" 2>/dev/null \ + | jq -s 'add // []' > .ai-review/reviews.json; then + echo '[]' > .ai-review/reviews.json + fi + + DELTA_OUT="$( + BASE_SHA="${BASE_SHA}" HEAD_SHA="${HEAD_SHA}" \ + FORCE_FULL_REVIEW="${FORCE_FULL_REVIEW:-false}" \ + AUTHOR_LOGIN="${AUTHOR_LOGIN}" \ + REVIEWS_JSON_PATH=".ai-review/reviews.json" \ + node "${DELTA_CLI}" + )" + REVIEW_MODE="$(printf '%s\n' "${DELTA_OUT}" | sed -n 's/^mode=//p' | tail -n1)" + REVIEW_BASE_SHA="$(printf '%s\n' "${DELTA_OUT}" | sed -n 's/^review-base-sha=//p' | tail -n1)" + DELTA_BASE_SHA="$(printf '%s\n' "${DELTA_OUT}" | sed -n 's/^delta-base-sha=//p' | tail -n1)" + PRIOR_HEAD_SHA="$(printf '%s\n' "${DELTA_OUT}" | sed -n 's/^prior-head-sha=//p' | tail -n1)" + REVIEW_MODE="${REVIEW_MODE:-full}" + REVIEW_BASE_SHA="${REVIEW_BASE_SHA:-${BASE_SHA}}" + + echo "prep: merge-base ${BASE_SHA}..${HEAD_SHA} review_mode=${REVIEW_MODE} range ${REVIEW_BASE_SHA}..${HEAD_SHA}" + + git diff --numstat "${REVIEW_BASE_SHA}" HEAD > .ai-review/numstat.txt # -U0 keeps the hunk headers (which carry the enclosing declaration) # without any body lines; only `diff --git` and `@@` lines are parsed. - git diff -U0 "${BASE_SHA}" HEAD \ + git diff -U0 "${REVIEW_BASE_SHA}" HEAD \ | grep -E '^(diff --git|@@)' > .ai-review/diff-headers.txt || true # Written to a file, never a step output: a PR title is attacker- # influenceable text and can contain newlines, which $GITHUB_OUTPUT @@ -459,7 +492,11 @@ runs: gh pr view "${PR_NUMBER}" --repo "${REPO}" --json title \ --jq '.title' > .ai-review/pr-title.txt - BASE_SHA="${BASE_SHA}" HEAD_SHA="${HEAD_SHA}" node "${MANIFEST_CLI}" + BASE_SHA="${BASE_SHA}" HEAD_SHA="${HEAD_SHA}" \ + REVIEW_MODE="${REVIEW_MODE}" \ + DELTA_BASE_SHA="${DELTA_BASE_SHA}" \ + PRIOR_HEAD_SHA="${PRIOR_HEAD_SHA}" \ + node "${MANIFEST_CLI}" FILES="$(jq -r '.file_count' .ai-review/manifest.json)" CHURN="$(jq -r '.churn' .ai-review/manifest.json)" @@ -498,6 +535,10 @@ runs: echo "fallback-model=${FALLBACK}" echo "haiku-model=${HAIKU}" echo "haiku-fallback-model=${HAIKU_FALLBACK}" + echo "review-mode=${REVIEW_MODE}" + echo "merge-base-sha=${BASE_SHA}" + echo "head-sha=${HEAD_SHA}" + echo "delta-base-sha=${DELTA_BASE_SHA}" } >> "${GITHUB_OUTPUT}" - name: Resolve linked issues @@ -582,11 +623,13 @@ runs: --allowedTools "Read,Grep,Glob,Write,Bash(git diff:*),Bash(git symbolic-ref:*),Bash(git remote show:*),Bash(git merge-base:*),Bash(git log:*),Bash(git show:*)" prompt: | Read `.ai-review/manifest.json` first: a prior deterministic step - already staged `base_sha`, `head_sha` and `changed_files`. Trust them — + already staged `base_sha`, `head_sha`, `review_mode`, `changed_files`, + and (when `review_mode` is `delta`) `delta_base_sha`. Trust them — do not run `git symbolic-ref`, `git merge-base` or `git remote show` to rediscover the default branch or the diff base. Diff with - `git diff HEAD` using the staged SHA, then read - the full contents of every changed file. Grep the repository for callers, + `git diff HEAD` when `review_mode` is `delta`, else + `git diff HEAD`, then read the full contents of every + changed file. Grep the repository for callers, callees, and helper functions related to the changed code, so a reviewer has the surrounding context without re-deriving it. @@ -781,16 +824,29 @@ runs: Read `.ai-review/manifest.json` FIRST. A prior deterministic step already resolved the review range and staged the facts you would - otherwise re-derive: `base_sha` and `head_sha` (the exact range), - `changed_files` (the authoritative list), `symbol_manifest` - (declarations touched by the diff), plus `title_ok`, - `has_test_change` and `no_tests_for_changed_logic`. + otherwise re-derive: `base_sha` and `head_sha` (PR merge-base and + HEAD for telemetry), `review_mode` (`full` or `delta`), + `delta_base_sha` when mode is delta (the prior published review + HEAD — the start of this run's active range), `changed_files` + (the authoritative list for the active range only), + `symbol_manifest` (declarations touched by the diff), plus + `title_ok`, `has_test_change` and `no_tests_for_changed_logic`. TRUST THOSE VALUES. Do NOT run `git symbolic-ref`, `git merge-base`, `git remote show`, or `gh pr view` to work out the default branch, the diff base, or the changed-file list — they are already settled, and re-deriving them costs turns and risks reviewing the wrong range. - Diff with `git diff HEAD` using the staged SHA. + Diff with `git diff HEAD` when `review_mode` is + `delta`, otherwise `git diff HEAD`. + + When `review_mode` is `delta`, this run covers only commits after + the last published ai-review. If `.ai-review/prior-review.md` + exists, READ IT and carry findings forward: mark each prior + finding resolved (cite what fixed it) or still open (restate it + if it still applies). Do not drop open prior findings silently. + Neighbor files outside `changed_files` may be read when a prior + finding or an import requires it — that is targeted expansion, + not a second full-PR read. Read the COMPLETE contents of every file in `changed_files` with `Read` — never sample, truncate, or reason from the diff hunks alone. @@ -1097,10 +1153,13 @@ runs: scan + P0-P3 severities + checklist) and `context.md` at the repo root if present. The PR number is in the `PR_NUMBER` environment variable; the PR head is already checked out. Read `.ai-review/manifest.json` - for the staged `base_sha`/`head_sha`/`changed_files` and trust them - rather than re-deriving the range; diff with - `git diff HEAD`. Run - `gh pr view "$PR_NUMBER" --json title,body,url,number` for the PR + for the staged `review_mode`/`base_sha`/`head_sha`/`delta_base_sha`/ + `changed_files` and trust them rather than re-deriving the range; + diff with `git diff HEAD` when `review_mode` is + `delta`, otherwise `git diff HEAD`. When + `.ai-review/prior-review.md` is present (delta or forced-full with + a prior), read it and mark prior findings resolved vs still-open. + Run `gh pr view "$PR_NUMBER" --json title,body,url,number` for the PR description only. Read `.ai-review/linked-issues.json` (repo root) if present and use each linked issue's acceptance criteria as the intent contract for Angle @@ -1551,6 +1610,9 @@ runs: REVIEW_LOG: ${{ runner.temp }}/ai-review-exec-review-snapshot.json REPAIR_LOG: ${{ runner.temp }}/ai-review-exec-repair-snapshot.json RETRY_LOG: ${{ steps.review_retry.outputs.execution_file }} + HEAD_SHA: ${{ steps.route.outputs.head-sha }} + MERGE_BASE_SHA: ${{ steps.route.outputs.merge-base-sha }} + REVIEW_MODE: ${{ steps.route.outputs.review-mode }} with: github-token: ${{ steps.identity-refresh.outputs.author-token }} script: | @@ -1582,6 +1644,15 @@ runs: fallback: process.env.ROUTED_MODEL || "", }); + const reviewMeta = + process.env.HEAD_SHA && process.env.MERGE_BASE_SHA + ? { + headSha: process.env.HEAD_SHA, + baseSha: process.env.MERGE_BASE_SHA, + mode: process.env.REVIEW_MODE === "delta" ? "delta" : "full", + } + : null; + const prNumber = Number(process.env.PR_NUMBER); const passLabel = process.env.PASS_LABEL; const failLabel = process.env.FAIL_LABEL; @@ -1605,7 +1676,10 @@ runs: } catch (e) { // No salvage file (step skipped, or nothing recoverable). } - const inconclusiveBody = buildInconclusiveBody(salvaged, { modelUsed }); + const inconclusiveBody = buildInconclusiveBody(salvaged, { + modelUsed, + reviewMeta, + }); try { await github.rest.pulls.createReview({ owner: context.repo.owner, @@ -1678,6 +1752,7 @@ runs: blockers, commentBody, modelUsed, + reviewMeta, }); await github.rest.pulls.createReview({ diff --git a/ai-review/lib/delta.js b/ai-review/lib/delta.js index 35e64ca..689ca71 100644 --- a/ai-review/lib/delta.js +++ b/ai-review/lib/delta.js @@ -148,28 +148,30 @@ function resolveDeltaBaseline({ forceFull = false, priorHeadIsAncestor, }) { - if (forceFull) { + const prior = findLatestAiReview(reviews); + if (!prior) { return { mode: "full", deltaBaseSha: null, priorHeadSha: null, priorBody: null, - reason: "force-full-review", + reason: forceFull ? "force-full-review" : "no-prior-review", }; } - const prior = findLatestAiReview(reviews); - if (!prior) { + const meta = parseReviewMeta(prior.body); + // forceFull still carries priorBody / priorHeadSha so Prep can write + // prior-review.md for finding carry-forward on a forced full run. + if (forceFull) { return { mode: "full", deltaBaseSha: null, - priorHeadSha: null, - priorBody: null, - reason: "no-prior-review", + priorHeadSha: meta && meta.headSha ? meta.headSha : null, + priorBody: prior.body, + reason: "force-full-review", }; } - const meta = parseReviewMeta(prior.body); if (!meta || !meta.headSha) { return { mode: "full", @@ -207,6 +209,52 @@ function resolveDeltaBaseline({ }; } +/** + * Git base for numstat / unified diff this run. Telemetry `base_sha` stays + * the PR merge-base; this is the active review range start. + * + * @param {{ mode: string, mergeBaseSha: string, deltaBaseSha?: string|null }} args + * @returns {string} + */ +function resolveActiveReviewBase({ mode, mergeBaseSha, deltaBaseSha }) { + if (mode === "delta" && deltaBaseSha) return deltaBaseSha; + return mergeBaseSha; +} + +/** + * Frozen `.ai-review/delta.json` shape (spec §10). + * + * @param {{ + * mode: string, + * reason: string, + * deltaBaseSha: string|null, + * priorHeadSha: string|null, + * headSha: string, + * mergeBaseSha: string, + * priorBodyPath: string|null, + * }} args + */ +function buildDeltaArtifact({ + mode, + reason, + deltaBaseSha, + priorHeadSha, + headSha, + mergeBaseSha, + priorBodyPath, +}) { + return { + schema: 1, + mode, + reason, + delta_base_sha: deltaBaseSha, + prior_head_sha: priorHeadSha, + head_sha: headSha, + merge_base_sha: mergeBaseSha, + prior_body_path: priorBodyPath, + }; +} + module.exports = { AI_REVIEW_MARKER, parseReviewMeta, @@ -214,4 +262,6 @@ module.exports = { resolveReviewRange, findLatestAiReview, resolveDeltaBaseline, + resolveActiveReviewBase, + buildDeltaArtifact, }; diff --git a/ai-review/lib/delta.test.js b/ai-review/lib/delta.test.js index ce7f72c..abb4aef 100644 --- a/ai-review/lib/delta.test.js +++ b/ai-review/lib/delta.test.js @@ -9,6 +9,8 @@ const { resolveReviewRange, findLatestAiReview, resolveDeltaBaseline, + resolveActiveReviewBase, + buildDeltaArtifact, } = require("./delta.js"); const HEAD = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; @@ -212,3 +214,60 @@ test("resolveDeltaBaseline: base change → full even when ancestor", () => { assert.equal(out.reason, "base-sha-changed"); assert.equal(out.priorHeadSha, PRIOR); }); + +test("resolveDeltaBaseline: forceFull still preserves priorBody for carry-forward", () => { + const body = [ + "", + formatReviewMeta({ headSha: PRIOR, baseSha: BASE, mode: "full" }), + "**✅ PASS**", + ].join("\n"); + const out = resolveDeltaBaseline({ + reviews: [{ id: 1, submitted_at: "2026-06-01T00:00:00Z", body }], + headSha: HEAD, + mergeBaseSha: BASE, + priorHeadIsAncestor: true, + forceFull: true, + }); + assert.deepEqual(out, { + mode: "full", + deltaBaseSha: null, + priorHeadSha: PRIOR, + priorBody: body, + reason: "force-full-review", + }); +}); + +test("resolveActiveReviewBase: delta uses prior head; full uses merge-base", () => { + assert.equal( + resolveActiveReviewBase({ mode: "delta", mergeBaseSha: BASE, deltaBaseSha: PRIOR }), + PRIOR, + ); + assert.equal( + resolveActiveReviewBase({ mode: "full", mergeBaseSha: BASE, deltaBaseSha: PRIOR }), + BASE, + ); +}); + +test("buildDeltaArtifact: frozen §10 shape", () => { + assert.deepEqual( + buildDeltaArtifact({ + mode: "delta", + reason: "prior-meta-ancestor", + deltaBaseSha: PRIOR, + priorHeadSha: PRIOR, + headSha: HEAD, + mergeBaseSha: BASE, + priorBodyPath: ".ai-review/prior-review.md", + }), + { + schema: 1, + mode: "delta", + reason: "prior-meta-ancestor", + delta_base_sha: PRIOR, + prior_head_sha: PRIOR, + head_sha: HEAD, + merge_base_sha: BASE, + prior_body_path: ".ai-review/prior-review.md", + }, + ); +}); diff --git a/ai-review/lib/prep.js b/ai-review/lib/prep.js index 951dbde..04b760d 100644 --- a/ai-review/lib/prep.js +++ b/ai-review/lib/prep.js @@ -211,18 +211,33 @@ function classifyPaths(paths) { * Assemble the manifest the review stage is told to trust. * * @param {object} args - * @param {string} args.baseSha merge-base commit, resolved by the caller + * @param {string} args.baseSha PR merge-base (telemetry; not always the diff base) * @param {string} args.headSha PR head commit - * @param {string} args.numstat stdout of `git diff --numstat base...HEAD` - * @param {string} args.diff stdout of `git diff -U0 base...HEAD` (headers only are used) + * @param {string} args.numstat stdout of `git diff --numstat HEAD` + * where reviewBase is delta_base_sha in delta mode, else the merge-base + * @param {string} args.diff stdout of `git diff -U0 HEAD` (headers only) * @param {object} args.sizes path -> full-file byte size at HEAD (`git cat-file -s`) * @param {string} args.title PR title + * @param {'full'|'delta'} [args.reviewMode='full'] + * @param {string|null} [args.deltaBaseSha=null] prior published head when delta + * @param {string|null} [args.priorHeadSha=null] */ -function buildManifest({ baseSha, headSha, numstat, diff, sizes, title }) { +function buildManifest({ + baseSha, + headSha, + numstat, + diff, + sizes, + title, + reviewMode, + deltaBaseSha, + priorHeadSha, +}) { const { files, fileCount, churn } = parseNumstat(numstat); const changed = files.map((f) => f.path); const sizeMap = sizes && typeof sizes === "object" ? sizes : {}; const classes = classifyPaths(changed); + const mode = reviewMode === "delta" ? "delta" : "full"; const totalBytes = changed.reduce((sum, p) => { const n = Number(sizeMap[p]); @@ -233,6 +248,9 @@ function buildManifest({ baseSha, headSha, numstat, diff, sizes, title }) { schema: 1, base_sha: baseSha ?? null, head_sha: headSha ?? null, + review_mode: mode, + delta_base_sha: mode === "delta" ? deltaBaseSha ?? null : null, + prior_head_sha: priorHeadSha ?? null, changed_files: changed, file_count: fileCount, churn, diff --git a/ai-review/lib/prep.test.js b/ai-review/lib/prep.test.js index f4358d2..39ed985 100644 --- a/ai-review/lib/prep.test.js +++ b/ai-review/lib/prep.test.js @@ -227,6 +227,9 @@ test("buildManifest: assembles the frozen shape", () => { assert.equal(m.schema, 1); assert.equal(m.base_sha, "a".repeat(40)); assert.equal(m.head_sha, "b".repeat(40)); + assert.equal(m.review_mode, "full"); + assert.equal(m.delta_base_sha, null); + assert.equal(m.prior_head_sha, null); assert.deepEqual(m.changed_files, ["src/a.ts", "src/a.test.ts"]); assert.equal(m.file_count, 2); assert.equal(m.churn, 4); @@ -238,6 +241,31 @@ test("buildManifest: assembles the frozen shape", () => { assert.equal(m.symbol_manifest[0].name, "alpha"); }); +test("buildManifest: delta mode keeps merge-base telemetry but lists only delta-range files", () => { + const prior = "c".repeat(40); + // numstat already scoped to prior…HEAD by the prep step — only the file + // touched after the last review appears, not the full PR set. + const m = buildManifest({ + baseSha: "a".repeat(40), + headSha: "b".repeat(40), + numstat: "2\t0\tsrc/only-delta.ts\n", + diff: "diff --git a/src/only-delta.ts b/src/only-delta.ts\n@@ -0,0 +1 @@ function deltaOnly() {", + sizes: { "src/only-delta.ts": 40 }, + title: "fix: tighten delta", + reviewMode: "delta", + deltaBaseSha: prior, + priorHeadSha: prior, + }); + + assert.equal(m.review_mode, "delta"); + assert.equal(m.base_sha, "a".repeat(40)); + assert.equal(m.delta_base_sha, prior); + assert.equal(m.prior_head_sha, prior); + assert.deepEqual(m.changed_files, ["src/only-delta.ts"]); + assert.equal(m.file_count, 1); + assert.equal(m.symbol_manifest[0].name, "deltaOnly"); +}); + test("buildManifest: missing size entries do not produce NaN", () => { const m = buildManifest({ baseSha: "a", diff --git a/ai-review/lib/publish.js b/ai-review/lib/publish.js index 193c1df..1b0e16f 100644 --- a/ai-review/lib/publish.js +++ b/ai-review/lib/publish.js @@ -76,7 +76,8 @@ function stripLeadingBannerArtifacts(markdown) { * modelUsed?: string|null, * reviewMeta?: { headSha: string, baseSha: string, mode: 'full'|'delta' }|null}} args * `commentBody` must already be run through stripLeadingBannerArtifacts. - * `reviewMeta` stamps the delta baseline (spec §6.1); omit until Publish wires SHAs. + * `reviewMeta` stamps the delta baseline (spec §6.1); Publish passes + * current HEAD + PR merge-base + this run's full|delta mode. */ function modelLine(modelUsed) { if (!modelUsed || typeof modelUsed !== "string" || !modelUsed.trim()) return []; diff --git a/ai-review/lib/write-delta.js b/ai-review/lib/write-delta.js new file mode 100644 index 0000000..6f57858 --- /dev/null +++ b/ai-review/lib/write-delta.js @@ -0,0 +1,185 @@ +"use strict"; + +// I/O wrapper: list prior bot reviews → resolve full|delta → write +// `.ai-review/delta.json` and optional `.ai-review/prior-review.md`. +// Pure resolution lives in delta.js; this script only talks to git/fs. +// +// Env: +// HEAD_SHA, BASE_SHA (PR merge-base), FORCE_FULL_REVIEW ("true"|"false") +// AUTHOR_LOGIN (optional filter), REVIEWS_JSON_PATH (default .ai-review/reviews.json) +// +// Prints shell-friendly KEY=value lines for the prep step (mode, review-base-sha, …). + +const fs = require("node:fs"); +const path = require("node:path"); +const { execFileSync } = require("node:child_process"); + +const { + findLatestAiReview, + parseReviewMeta, + resolveDeltaBaseline, + resolveActiveReviewBase, + buildDeltaArtifact, +} = require("./delta.js"); + +const DIR = ".ai-review"; +const PRIOR_REL = ".ai-review/prior-review.md"; + +/** + * @param {string} priorSha + * @param {string} headSha + * @param {{ execFileSync?: typeof execFileSync }} [io] + */ +function priorHeadIsAncestorOf(priorSha, headSha, io) { + const exec = (io && io.execFileSync) || execFileSync; + try { + exec("git", ["merge-base", "--is-ancestor", priorSha, headSha], { + stdio: "ignore", + }); + return true; + } catch { + return false; + } +} + +/** + * @param {unknown} raw + * @param {string} authorLogin + */ +function filterBotReviews(raw, authorLogin) { + const list = Array.isArray(raw) ? raw : []; + if (!authorLogin) return list; + return list.filter((r) => r && r.user && r.user.login === authorLogin); +} + +/** + * @param {{ + * reviews?: unknown, + * headSha: string, + * mergeBaseSha: string, + * forceFull?: boolean, + * authorLogin?: string, + * dir?: string, + * io?: { + * writeFile?: typeof fs.writeFileSync, + * mkdir?: typeof fs.mkdirSync, + * execFileSync?: typeof execFileSync, + * log?: (s: string) => void, + * }, + * }} args + */ +function writeDeltaArtifacts({ + reviews, + headSha, + mergeBaseSha, + forceFull = false, + authorLogin = "", + dir = DIR, + io, +}) { + const writeFile = (io && io.writeFile) || fs.writeFileSync; + const mkdir = (io && io.mkdir) || ((p) => fs.mkdirSync(p, { recursive: true })); + const log = (io && io.log) || ((s) => process.stderr.write(s)); + + const filtered = filterBotReviews(reviews, authorLogin); + const prior = findLatestAiReview(filtered); + let ancestor = false; + if (prior) { + const meta = parseReviewMeta(prior.body); + if (meta && meta.headSha) { + ancestor = priorHeadIsAncestorOf(meta.headSha, headSha, io); + } + } + + const baseline = resolveDeltaBaseline({ + reviews: filtered, + headSha, + mergeBaseSha, + forceFull, + priorHeadIsAncestor: ancestor, + }); + + mkdir(dir); + + let priorBodyPath = null; + if (baseline.priorBody) { + const absPrior = path.join(dir, "prior-review.md"); + writeFile(absPrior, baseline.priorBody); + priorBodyPath = PRIOR_REL; + } + + const artifact = buildDeltaArtifact({ + mode: baseline.mode, + reason: baseline.reason, + deltaBaseSha: baseline.deltaBaseSha, + priorHeadSha: baseline.priorHeadSha, + headSha, + mergeBaseSha, + priorBodyPath, + }); + writeFile(path.join(dir, "delta.json"), `${JSON.stringify(artifact, null, 2)}\n`); + + const reviewBase = resolveActiveReviewBase({ + mode: baseline.mode, + mergeBaseSha, + deltaBaseSha: baseline.deltaBaseSha, + }); + + log( + `delta: mode=${baseline.mode} reason=${baseline.reason} ` + + `review_base=${reviewBase.slice(0, 12)}…` + + (baseline.priorBody ? " prior-review.md=yes" : " prior-review.md=no") + + "\n", + ); + + return { + baseline, + artifact, + reviewBaseSha: reviewBase, + }; +} + +function main() { + const reviewsPath = + process.env.REVIEWS_JSON_PATH || path.join(DIR, "reviews.json"); + let reviews = []; + try { + reviews = JSON.parse(fs.readFileSync(reviewsPath, "utf8")); + } catch { + reviews = []; + } + + const headSha = process.env.HEAD_SHA || ""; + const mergeBaseSha = process.env.BASE_SHA || ""; + if (!headSha || !mergeBaseSha) { + throw new Error("write-delta.js requires HEAD_SHA and BASE_SHA"); + } + + const { baseline, reviewBaseSha } = writeDeltaArtifacts({ + reviews, + headSha, + mergeBaseSha, + forceFull: process.env.FORCE_FULL_REVIEW === "true", + authorLogin: process.env.AUTHOR_LOGIN || "", + }); + + // Shell-readable lines for the prep step (not GITHUB_OUTPUT — caller copies). + process.stdout.write(`mode=${baseline.mode}\n`); + process.stdout.write(`reason=${baseline.reason}\n`); + process.stdout.write(`review-base-sha=${reviewBaseSha}\n`); + process.stdout.write( + `delta-base-sha=${baseline.deltaBaseSha || ""}\n`, + ); + process.stdout.write( + `prior-head-sha=${baseline.priorHeadSha || ""}\n`, + ); +} + +module.exports = { + priorHeadIsAncestorOf, + filterBotReviews, + writeDeltaArtifacts, + main, +}; + +if (require.main === module) main(); diff --git a/ai-review/lib/write-delta.test.js b/ai-review/lib/write-delta.test.js new file mode 100644 index 0000000..0915443 --- /dev/null +++ b/ai-review/lib/write-delta.test.js @@ -0,0 +1,114 @@ +"use strict"; + +const test = require("node:test"); +const assert = require("node:assert/strict"); +const fs = require("node:fs"); +const os = require("node:os"); +const path = require("node:path"); + +const { + filterBotReviews, + writeDeltaArtifacts, +} = require("./write-delta.js"); +const { formatReviewMeta } = require("./delta.js"); + +const HEAD = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; +const PRIOR = "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb"; +const BASE = "cccccccccccccccccccccccccccccccccccccccc"; + +test("filterBotReviews keeps only the author login", () => { + const reviews = [ + { user: { login: "bot[bot]" }, body: "a" }, + { user: { login: "human" }, body: "b" }, + ]; + assert.deepEqual(filterBotReviews(reviews, "bot[bot]"), [reviews[0]]); + assert.deepEqual(filterBotReviews(reviews, ""), reviews); +}); + +test("writeDeltaArtifacts: delta mode writes delta.json + prior-review.md", () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "write-delta-")); + const body = [ + "", + formatReviewMeta({ headSha: PRIOR, baseSha: BASE, mode: "full" }), + "finding: leak", + ].join("\n"); + const reviews = [ + { + id: 1, + submitted_at: "2026-06-01T00:00:00Z", + user: { login: "review-bot[bot]" }, + body, + }, + ]; + + const written = new Map(); + const { baseline, reviewBaseSha, artifact } = writeDeltaArtifacts({ + reviews, + headSha: HEAD, + mergeBaseSha: BASE, + authorLogin: "review-bot[bot]", + dir, + io: { + writeFile: (p, data) => { + written.set(p, data); + }, + mkdir: () => {}, + execFileSync: () => {}, // ancestor check succeeds + log: () => {}, + }, + }); + + assert.equal(baseline.mode, "delta"); + assert.equal(reviewBaseSha, PRIOR); + assert.equal(artifact.prior_body_path, ".ai-review/prior-review.md"); + assert.equal(written.get(path.join(dir, "prior-review.md")), body); + const delta = JSON.parse(written.get(path.join(dir, "delta.json"))); + assert.equal(delta.schema, 1); + assert.equal(delta.mode, "delta"); + assert.equal(delta.delta_base_sha, PRIOR); + assert.equal(delta.merge_base_sha, BASE); + assert.equal(delta.head_sha, HEAD); + + fs.rmSync(dir, { recursive: true, force: true }); +}); + +test("writeDeltaArtifacts: forceFull keeps prior-review.md but full range", () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "write-delta-ff-")); + const body = [ + "", + formatReviewMeta({ headSha: PRIOR, baseSha: BASE, mode: "full" }), + ].join("\n"); + + const written = new Map(); + const { baseline, reviewBaseSha } = writeDeltaArtifacts({ + reviews: [ + { + id: 1, + submitted_at: "2026-06-01T00:00:00Z", + user: { login: "bot[bot]" }, + body, + }, + ], + headSha: HEAD, + mergeBaseSha: BASE, + forceFull: true, + authorLogin: "bot[bot]", + dir, + io: { + writeFile: (p, data) => { + written.set(p, data); + }, + mkdir: () => {}, + execFileSync: () => {}, + log: () => {}, + }, + }); + + assert.equal(baseline.mode, "full"); + assert.equal(baseline.reason, "force-full-review"); + assert.equal(baseline.priorBody, body); + assert.equal(reviewBaseSha, BASE); + assert.ok(written.has(path.join(dir, "prior-review.md"))); + + fs.rmSync(dir, { recursive: true, force: true }); +}); diff --git a/ai-review/lib/write-manifest.js b/ai-review/lib/write-manifest.js index 0fdcc22..eab765c 100644 --- a/ai-review/lib/write-manifest.js +++ b/ai-review/lib/write-manifest.js @@ -126,6 +126,10 @@ function main() { if (size !== undefined) sizes[f.path] = size; } + const reviewMode = process.env.REVIEW_MODE === "delta" ? "delta" : "full"; + const deltaBaseSha = process.env.DELTA_BASE_SHA || null; + const priorHeadSha = process.env.PRIOR_HEAD_SHA || null; + const manifest = buildManifest({ baseSha: process.env.BASE_SHA || null, headSha: process.env.HEAD_SHA || null, @@ -133,12 +137,15 @@ function main() { diff, sizes, title, + reviewMode, + deltaBaseSha: deltaBaseSha || null, + priorHeadSha: priorHeadSha || null, }); atomicWriteJson(path.join(DIR, "manifest.json"), manifest); process.stdout.write( - `prep: ${manifest.file_count} files, churn ${manifest.churn}, ` + + `prep: mode=${manifest.review_mode} ${manifest.file_count} files, churn ${manifest.churn}, ` + `${manifest.total_fullfile_bytes} bytes at HEAD, ` + `${manifest.symbol_manifest.length} symbols, ` + `title_ok=${manifest.title_ok}, ` + diff --git a/ai-review/lib/write-manifest.test.js b/ai-review/lib/write-manifest.test.js index 026c5fb..8e8b42a 100644 --- a/ai-review/lib/write-manifest.test.js +++ b/ai-review/lib/write-manifest.test.js @@ -356,6 +356,9 @@ test("main: writes manifest.json AND assignments.json against a real diff on dis const prevCwd = process.cwd(); const prevBase = process.env.BASE_SHA; const prevHead = process.env.HEAD_SHA; + const prevMode = process.env.REVIEW_MODE; + const prevDelta = process.env.DELTA_BASE_SHA; + const prevPrior = process.env.PRIOR_HEAD_SHA; // Spies on the real fs.renameSync so this test proves the DEFAULT wiring -- // main() -> writeRoster()/manifest write -> atomicWriteJson -> fs.renameSync // -- actually runs in production, not just that atomicWriteJson behaves @@ -384,6 +387,9 @@ test("main: writes manifest.json AND assignments.json against a real diff on dis fs.writeFileSync("src/a.ts", "export function widget() {\n return 1;\n}\n"); process.env.BASE_SHA = "base000"; process.env.HEAD_SHA = "head111"; + process.env.REVIEW_MODE = "delta"; + process.env.DELTA_BASE_SHA = "prior222"; + process.env.PRIOR_HEAD_SHA = "prior222"; main(); @@ -392,6 +398,9 @@ test("main: writes manifest.json AND assignments.json against a real diff on dis assert.deepEqual(manifest.changed_files, ["src/a.ts"]); assert.equal(manifest.base_sha, "base000"); assert.equal(manifest.head_sha, "head111"); + assert.equal(manifest.review_mode, "delta"); + assert.equal(manifest.delta_base_sha, "prior222"); + assert.equal(manifest.prior_head_sha, "prior222"); const roster = JSON.parse(fs.readFileSync(".ai-review/assignments.json", "utf8")); assert.equal(roster.schema, 1); @@ -416,6 +425,12 @@ test("main: writes manifest.json AND assignments.json against a real diff on dis else process.env.BASE_SHA = prevBase; if (prevHead === undefined) delete process.env.HEAD_SHA; else process.env.HEAD_SHA = prevHead; + if (prevMode === undefined) delete process.env.REVIEW_MODE; + else process.env.REVIEW_MODE = prevMode; + if (prevDelta === undefined) delete process.env.DELTA_BASE_SHA; + else process.env.DELTA_BASE_SHA = prevDelta; + if (prevPrior === undefined) delete process.env.PRIOR_HEAD_SHA; + else process.env.PRIOR_HEAD_SHA = prevPrior; fs.rmSync(dir, { recursive: true, force: true }); } }); From 7f40eb31d6567b2656fc2b351ba6476f77b408d2 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 13:39:39 +0300 Subject: [PATCH 05/16] feat(ai-review): Test Plan CI coverage findings; stop checklist ticks Map PR Test Plan items to CI check inventory as findings; retire PR-body checklist tick write-back. --- ai-review/README.md | 45 ++++-- ai-review/action.yml | 174 ++++++++++---------- ai-review/lib/publish.js | 1 + ai-review/lib/testplan-ci.js | 254 ++++++++++++++++++++++++++++++ ai-review/lib/testplan-ci.test.js | 159 +++++++++++++++++++ ai-review/rubric.md | 31 ++-- 6 files changed, 547 insertions(+), 117 deletions(-) create mode 100644 ai-review/lib/testplan-ci.js create mode 100644 ai-review/lib/testplan-ci.test.js diff --git a/ai-review/README.md b/ai-review/README.md index 993aa22..e8d8df4 100644 --- a/ai-review/README.md +++ b/ai-review/README.md @@ -150,15 +150,20 @@ injection-safety rule. `timeout-minutes` is the wall-clock backstop — see the consumer guide.) Set `enable-context-stage: 'false'` to skip this stage entirely; the review reads `context.md` only "if present", so the gate is unaffected. -10. **CI signal (re-review only)** — on a `workflow_dispatch` re-review, - reads the PR's required-check conclusions (`pass`/`fail`/`timeout`/ - `no_ci`) so the Publish step can treat a failing/timed-out required - check as an automatic fail. +10. **CI inventory + signal** — on every PR event (not only + `workflow_dispatch`), inventories check runs for the PR HEAD into + `.ai-review/ci-checks.json` and parses the PR Test Plan / checklists into + `.ai-review/test-plan-items.json`. Also derives an aggregate + `pass`/`fail`/`timeout`/`no_ci` signal for Publish when every returned + check has completed (a first `pull_request` run usually stays `no_ci` + while sibling jobs are still running). 11. **Review stage (Sonnet/Opus)** — runs the full rubric scan against the diff and returns a schema-validated structured result (verdict, confidence, merge risk, intent alignment, P0-P3 counts, test-quality - signals, the review markdown body, and — new — a per-item `checklist` - verdict, `verification_evidence`, and a `test_execution` outcome). It + signals, the review markdown body, optional `verification_evidence`, and + a `test_execution` outcome). Uncovered Test Plan items vs CI become + normal findings; the `checklist` field is left empty (Publish no longer + ticks boxes). It reads **complete file contents** (never just diff hunks) and evaluates the diff against the linked issues' acceptance criteria. It does **not** run the project's tests — see @@ -195,13 +200,10 @@ injection-safety rule. review stage's structured output (a `pass` claiming green tests without any `verification_evidence` is penalized, not trusted) and posts it as a native PR review and the corresponding pass/fail label, then sets the - four job outputs. When `update-pr-body` is `true` it also **ticks the - PR description's checklist boxes** that the review verified (`- [ ]` → - `- [x]`, never unchecking a human's box) and maintains a managed - `` block with the per-item verification - evidence. Editing the body is safe against the default trigger set - (which excludes `edited`); do **not** add `pull_request: [edited]` to - the caller or the review will loop on its own body edits. + four job outputs. It does **not** tick PR description checklist boxes + or write an `` block (`update-pr-body` is + accepted but is a no-op for that path). Test Plan gaps are already + findings from the review stage. ## Inputs @@ -222,7 +224,7 @@ injection-safety rule. | `api-timeout-ms` | Per-request timeout (ms) for every Claude stage, passed as `API_TIMEOUT_MS` (CLI default `600000`). **Does not bound the ~27.5-min stall** — a run with this set to `180000` still stalled 27m36s. It is a genuine per-request bound and fails a wedged request faster than the default, nothing more. | No | `180000` | | `test-command` | **DEPRECATED — accepted but ignored.** The Review stage no longer runs tests; see [Why the review no longer runs tests](#why-the-review-no-longer-runs-tests). | No | — | | `test-hint` | **DEPRECATED — accepted but ignored.** Same reason as `test-command`. | No | — | -| `update-pr-body` | When `true`, the Publish step ticks verified checklist boxes in the PR description and maintains a managed `` block. Never unchecks a human-checked box. | No | `true` | +| `update-pr-body` | Accepted for compatibility. Checklist tick / status-block write-back is **retired**; the input is a no-op. Test Plan gaps are findings vs CI instead. | No | `true` | | `update-linked-issues` | When `true`, the Review stage resolves and evaluates the issues the PR closes. ai-review only reads them; it never mutates issue state. | No | `true` | | `force-full-review` | When `true`, always review merge-base…HEAD instead of a delta since the last published ai-review. | No | `false` | @@ -237,6 +239,21 @@ smaller numstat / must-read set, with `.ai-review/prior-review.md` for finding carry-forward. Full mode is used on first run, missing/inconclusive meta, force-push (non-ancestor), base SHA change, or `force-full-review: true`. +## Test Plan ↔ CI + +The review does **not** execute the Test Plan and does **not** mark checklist +items verified in the PR body. Prep inventories: + +- `.ai-review/test-plan-items.json` — items from a `Test Plan` section and/or + `- [ ]` / `- [x]` checkboxes in the PR body +- `.ai-review/ci-checks.json` — check runs for the PR HEAD (`name`, + `conclusion`, `status`), fetched on all PR events + +The model maps items to CI coverage; uncovered or weakly covered items become +normal `findings[]` with severity P0–P3 via the rubric (then `recompute.js`). +Checklist tick write-back is retired (`update-pr-body` is a no-op for that +path). `test_execution` stays `"skipped"` — no test runners in the allowlist. + ## Outputs | Name | Description | diff --git a/ai-review/action.yml b/ai-review/action.yml index 84fab36..edd4494 100644 --- a/ai-review/action.yml +++ b/ai-review/action.yml @@ -198,11 +198,10 @@ inputs: required: false update-pr-body: description: > - When 'true' (default), the Publish step ticks verified checklist - boxes (`- [ ]` → `- [x]`) in the PR description in place and - maintains a managed `` block with per-item - verification evidence. It never unchecks a human-checked box. Set to - 'false' to leave the PR body untouched. + Accepted for compatibility. Checklist tick / status-block write-back + is retired — Test Plan gaps become findings vs CI inventory instead. + The input is a no-op either way; PR body checkboxes are never ticked + or unticked by this action. required: false default: "true" update-linked-issues: @@ -683,40 +682,64 @@ runs: echo "--- context.md preview (first 20 lines) ---" head -n 20 context.md - - name: CI signal (re-review only) + - name: CI inventory and signal id: ci-signal if: steps.fork-guard.outputs.is-fork != 'true' && steps.pr-state.outputs.skip != 'true' shell: bash env: GH_TOKEN: ${{ steps.identity.outputs.author-token }} - EVENT_NAME: ${{ github.event_name }} REPO: ${{ github.repository }} HEAD_SHA: ${{ steps.pr-state.outputs.head-sha }} + PR_NUMBER: ${{ steps.pr.outputs.pr-number }} + TESTPLAN_CLI: ${{ github.action_path }}/lib/testplan-ci.js run: | # Best-effort only — this step must never hard-fail the job, so it - # deliberately omits `set -e`. A normal pull_request run is the - # *first* CI job: no required check has a completed conclusion yet, - # so a real CI signal only exists on a manual workflow_dispatch - # re-review, after CI has already run once. + # deliberately omits `set -e`. Inventories check runs on every PR + # event (not only workflow_dispatch) into .ai-review/ci-checks.json + # and parses the PR Test Plan into .ai-review/test-plan-items.json + # for the review prompt. Aggregate pass/fail SIGNAL still requires + # every returned check to be completed — a first pull_request run + # usually yields no_ci while sibling jobs are still in progress. set -uo pipefail + mkdir -p .ai-review SIGNAL="no_ci" - if [ "${EVENT_NAME}" = "workflow_dispatch" ]; then - CHECK_RUNS="$(gh api "repos/${REPO}/commits/${HEAD_SHA}/check-runs" --jq '.check_runs' 2>/dev/null)" - if [ -n "${CHECK_RUNS}" ] && [ "${CHECK_RUNS}" != "[]" ] && [ "${CHECK_RUNS}" != "null" ]; then - # If any returned check-run has not yet reached "completed" - # (queued/in_progress), the signal is inconclusive — never guess - # a pass/fail from a partial subset of still-running checks. - if echo "${CHECK_RUNS}" | jq -e 'any(.[]; .status != "completed")' >/dev/null 2>&1; then - SIGNAL="no_ci" - else - CONCLUSIONS="$(echo "${CHECK_RUNS}" | jq '[.[] | .conclusion]')" - if echo "${CONCLUSIONS}" | jq -e 'any(.[]; . == "timed_out")' >/dev/null 2>&1; then - SIGNAL="timeout" - elif echo "${CONCLUSIONS}" | jq -e 'any(.[]; . == "failure" or . == "cancelled" or . == "action_required")' >/dev/null 2>&1; then - SIGNAL="fail" - elif echo "${CONCLUSIONS}" | jq -e 'all(.[]; . == "success" or . == "neutral" or . == "skipped")' >/dev/null 2>&1; then - SIGNAL="pass" - fi + CHECK_RUNS='[]' + + if RAW="$(gh api --paginate "repos/${REPO}/commits/${HEAD_SHA}/check-runs" 2>/dev/null)"; then + MERGED="$(printf '%s' "${RAW}" | jq -s '[.[].check_runs // [] | .[]]' 2>/dev/null || true)" + if [ -n "${MERGED}" ] && [ "${MERGED}" != "null" ]; then + CHECK_RUNS="${MERGED}" + fi + fi + printf '%s\n' "${CHECK_RUNS}" > .ai-review/check-runs.raw.json + + PR_BODY="" + if [ -n "${PR_NUMBER:-}" ]; then + PR_BODY="$(gh pr view "${PR_NUMBER}" --repo "${REPO}" --json body --jq '.body // ""' 2>/dev/null || true)" + fi + # Written to a file (never env): PR bodies can contain newlines and + # attacker-influenceable text. + printf '%s' "${PR_BODY}" > .ai-review/pr-body.md + + PR_BODY_PATH=".ai-review/pr-body.md" \ + CHECK_RUNS_PATH=".ai-review/check-runs.raw.json" \ + AI_REVIEW_DIR=".ai-review" \ + node "${TESTPLAN_CLI}" || true + + if [ -n "${CHECK_RUNS}" ] && [ "${CHECK_RUNS}" != "[]" ] && [ "${CHECK_RUNS}" != "null" ]; then + # If any returned check-run has not yet reached "completed" + # (queued/in_progress), the signal is inconclusive — never guess + # a pass/fail from a partial subset of still-running checks. + if echo "${CHECK_RUNS}" | jq -e 'any(.[]; .status != "completed")' >/dev/null 2>&1; then + SIGNAL="no_ci" + else + CONCLUSIONS="$(echo "${CHECK_RUNS}" | jq '[.[] | .conclusion]')" + if echo "${CONCLUSIONS}" | jq -e 'any(.[]; . == "timed_out")' >/dev/null 2>&1; then + SIGNAL="timeout" + elif echo "${CONCLUSIONS}" | jq -e 'any(.[]; . == "failure" or . == "cancelled" or . == "action_required")' >/dev/null 2>&1; then + SIGNAL="fail" + elif echo "${CONCLUSIONS}" | jq -e 'all(.[]; . == "success" or . == "neutral" or . == "skipped")' >/dev/null 2>&1; then + SIGNAL="pass" fi fi fi @@ -882,18 +905,16 @@ runs: exercise), report `no_tests_for_changed_logic: false` — that row requires no tests, so none missing is not a defect. - Evaluate the PR description's checklist. If the PR body (or a - linked issue) contains `- [ ]`/`- [x]` checklist items, return a - `checklist` array: one entry per item with its `text` copied - verbatim (without the `- [ ]`/`- [x]` prefix), a `status` of - `verified` (you confirmed it is satisfied — cite how in - `evidence`), `failed` (you confirmed it is NOT satisfied), or - `unverifiable` (cannot be checked from code/tests, e.g. a manual - or post-merge step). Only mark `verified` with real evidence per - `/verification-before-completion`. A later deterministic step uses - this to tick verified boxes in the PR body; do not edit the PR body - yourself. Per the rubric, checklist status is a P2 reminder at - most — do not fail the review solely on incomplete checklist items. + Read `.ai-review/test-plan-items.json` and `.ai-review/ci-checks.json` + (written by Prep). Map each Test Plan / checklist item to the + inventoried CI checks. For each item that is **not covered** (or + only weakly covered) by CI, emit a normal `findings[]` entry with + severity P0–P3 judged via the rubric — CI is the source of truth + for "was this exercised?", not PR-body ticks. Leave the structured + `checklist` array empty (or omit it); Publish no longer ticks + checklist boxes and must not edit the PR body. Manual/post-merge + items with no plausible CI mapping may be omitted or filed at low + severity at your judgment — do not invent coverage. Write a complete review as `comment_markdown`: findings grouped by severity (P0 Blockers, P1 Should Fix, P2 Nice-to-Have, P3 Nits — @@ -1150,7 +1171,7 @@ runs: your own `counts`; they do not affect this run's verdict. Read `${{ github.action_path }}/rubric.md` (the authoritative 8-angle - scan + P0-P3 severities + checklist) and `context.md` at the repo root + scan + P0-P3 severities + review checklist) and `context.md` at the repo root if present. The PR number is in the `PR_NUMBER` environment variable; the PR head is already checked out. Read `.ai-review/manifest.json` for the staged `review_mode`/`base_sha`/`head_sha`/`delta_base_sha`/ @@ -1163,13 +1184,16 @@ runs: description only. Read `.ai-review/linked-issues.json` (repo root) if present and use each linked issue's acceptance criteria as the intent contract for Angle - H. Attempt to load `/verification-before-completion` and + H. Read `.ai-review/test-plan-items.json` and + `.ai-review/ci-checks.json` if present: uncovered/weakly covered + Test Plan items become normal `findings[]` (P0–P3 via the rubric); + leave `checklist` empty — Publish does not tick PR-body boxes. + Attempt to load `/verification-before-completion` and `/requesting-code-review` (best-effort). Read complete file contents, never just the diff hunks. Do NOT run tests or probe for a toolchain — set `test_execution: "skipped"` (you have no shell - access to a test runner). You MAY populate the optional - `checklist` and `verification_evidence` fields; omit them if you - cannot assess them. + access to a test runner). You MAY populate `verification_evidence` + when you have real command output; omit it if you cannot. Perform the full review per the rubric, then write `comment_markdown` (findings grouped by severity P0-P3 with "_None._" for empty sections, @@ -1622,9 +1646,6 @@ runs: stripLeadingBannerArtifacts, buildReviewBody, buildInconclusiveBody, - tickVerifiedBoxes, - buildStatusBlock, - upsertStatusBlock, } = require(process.env.PUBLISH_LIB_PATH); const { resolveModelUsed } = require(process.env.METRICS_PATH); @@ -1708,11 +1729,8 @@ runs: return; } - // verification_evidence is still needed below by the PR-body status - // block, independently of the verdict computation. - const verificationEvidence = Array.isArray(review.verification_evidence) - ? review.verification_evidence - : []; + // verification_evidence is consumed inside recompute (when present); + // no PR-body status-block write-back remains. const result = recompute(review, { confidenceThreshold, @@ -1788,46 +1806,16 @@ runs: } } - // --- PR-body checklist verification (feature: update PR body) ------ - // Reflect verified checklist items back into the PR description: tick - // boxes the model VERIFIED (with evidence) in place, and maintain a - // managed block with per-item status + - // evidence. Never unticks a human-checked box; never fails the job on - // error. Gated by the update-pr-body input. The PR body is re-fetched - // immediately before writing to minimize races with human edits. - if ( - process.env.UPDATE_PR_BODY === "true" && - Array.isArray(review.checklist) && - review.checklist.length > 0 - ) { - try { - const { data: prData } = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: prNumber, - }); - const originalBody = prData.body || ""; - - const { newBody: tickedBody, ticks } = tickVerifiedBoxes(originalBody, review.checklist); - const block = buildStatusBlock({ checklist: review.checklist, verificationEvidence, verdict }); - const newBody = upsertStatusBlock(tickedBody, block); - - if (newBody !== originalBody) { - await github.rest.pulls.update({ - owner: context.repo.owner, - repo: context.repo.repo, - pull_number: prNumber, - body: newBody, - }); - core.info( - `Updated PR body (ticked ${ticks} checkbox(es), refreshed status block).` - ); - } else { - core.info("PR body already current; no change."); - } - } catch (err) { - core.warning(`Could not update PR body: ${err.message}`); - } + // --- PR-body checklist write-back (retired) ------------------------ + // Spec §7.4: stop ticking verified boxes / status-block checklist + // write-back. update-pr-body remains accepted for compatibility but + // is a no-op — never tick or untick human boxes. Linked-issue + // updates are independent and unchanged. + if (process.env.UPDATE_PR_BODY === "true") { + core.info( + "update-pr-body: checklist tick / status-block write-back is retired; " + + "Test Plan gaps are findings vs CI (.ai-review/test-plan-items.json)." + ); } core.setOutput("verdict", verdict); diff --git a/ai-review/lib/publish.js b/ai-review/lib/publish.js index 1b0e16f..d527c93 100644 --- a/ai-review/lib/publish.js +++ b/ai-review/lib/publish.js @@ -186,6 +186,7 @@ function buildInconclusiveBody(salvaged, opts = {}) { /** * Ticks unchecked PR-body checkboxes whose text matches a VERIFIED checklist * item. Never unchecks a human-checked box (the regex only matches "[ ]"). + * Kept for unit tests / compatibility; Publish no longer calls this (spec §7.4). * @param {string} originalBody * @param {{text: string, status: string, evidence?: string}[]} checklist * @returns {{newBody: string, ticks: number}} diff --git a/ai-review/lib/testplan-ci.js b/ai-review/lib/testplan-ci.js new file mode 100644 index 0000000..5e9eee2 --- /dev/null +++ b/ai-review/lib/testplan-ci.js @@ -0,0 +1,254 @@ +"use strict"; + +// Test Plan ↔ CI helpers (spec §7 / frozen §10). +// +// Pure extract/summarize for Prep artifacts: +// .ai-review/test-plan-items.json +// .ai-review/ci-checks.json +// Mapping quality is model-assisted in the review prompt; optional +// findObviousUncoveredItems is a coarse keyword heuristic for tests. + +const fs = require("node:fs"); +const path = require("node:path"); + +const DIR = ".ai-review"; + +const CHECKBOX_RE = /^\s*[-*]\s*\[(?: |x|X)\]\s+(.+?)\s*$/; +const PLAIN_BULLET_RE = /^\s*[-*]\s+(?!\[)(.+?)\s*$/; +const NUMBERED_RE = /^\s*\d+[.)]\s+(.+?)\s*$/; +const HEADING_TEST_PLAN_RE = /^(#{1,6})\s*test\s*plan\b/i; +const BARE_TEST_PLAN_RE = /^\s*\*{0,2}test\s*plan\*{0,2}\s*:?\s*$/i; + +/** + * @param {string|null|undefined} prBody + * @returns {string[]} + */ +function extractTestPlanItems(prBody) { + if (typeof prBody !== "string" || !prBody.trim()) return []; + + const section = extractTestPlanSection(prBody); + if (section != null) { + const boxes = extractCheckboxes(section); + if (boxes.length) return unique(boxes); + const plain = extractPlainListItems(section); + return unique(plain); + } + + return unique(extractCheckboxes(prBody)); +} + +/** + * @param {string} body + * @returns {string|null} section text after the heading, or null if absent + */ +function extractTestPlanSection(body) { + const lines = body.split(/\r?\n/); + let start = -1; + let startLevel = 2; + + for (let i = 0; i < lines.length; i++) { + const hm = lines[i].match(HEADING_TEST_PLAN_RE); + if (hm) { + start = i + 1; + startLevel = hm[1].length; + break; + } + if (BARE_TEST_PLAN_RE.test(lines[i])) { + start = i + 1; + startLevel = 2; + break; + } + } + if (start < 0) return null; + + const out = []; + for (let i = start; i < lines.length; i++) { + const hm = lines[i].match(/^(#{1,6})\s+\S/); + if (hm && hm[1].length <= startLevel) break; + out.push(lines[i]); + } + return out.join("\n"); +} + +/** + * @param {string} text + * @returns {string[]} + */ +function extractCheckboxes(text) { + const items = []; + for (const line of text.split(/\r?\n/)) { + const m = line.match(CHECKBOX_RE); + if (m) items.push(m[1].trim()); + } + return items; +} + +/** + * Non-checkbox bullets / numbered items (used inside a Test Plan section). + * @param {string} text + * @returns {string[]} + */ +function extractPlainListItems(text) { + const items = []; + for (const line of text.split(/\r?\n/)) { + const b = line.match(PLAIN_BULLET_RE); + if (b) { + items.push(b[1].trim()); + continue; + } + const n = line.match(NUMBERED_RE); + if (n) items.push(n[1].trim()); + } + return items; +} + +/** + * @param {unknown} checkRuns GitHub Checks API `.check_runs` array + * @returns {{ name: string, conclusion: string|null, status: string }[]} + */ +function summarizeCiChecks(checkRuns) { + if (!Array.isArray(checkRuns)) return []; + const out = []; + for (const r of checkRuns) { + if (!r || typeof r !== "object") continue; + const name = typeof r.name === "string" ? r.name.trim() : ""; + if (!name) continue; + const status = typeof r.status === "string" ? r.status : ""; + const conclusion = + r.conclusion == null || r.conclusion === "" + ? null + : String(r.conclusion); + out.push({ name, conclusion, status }); + } + return out; +} + +/** + * Coarse keyword overlap: item tokens (≥3 chars) vs check names. + * True misses for tests / prompt hints — not authoritative coverage. + * + * @param {string[]} items + * @param {{ name: string }[]} checks + * @returns {string[]} + */ +function findObviousUncoveredItems(items, checks) { + if (!Array.isArray(items) || !items.length) return []; + const checkTokenSets = (Array.isArray(checks) ? checks : []).map((c) => + new Set(tokenize(c && c.name)) + ); + return items.filter((item) => { + const tokens = tokenize(item); + if (!tokens.length) return true; + for (const cts of checkTokenSets) { + if (tokens.some((t) => cts.has(t))) return false; + } + return true; + }); +} + +/** + * @param {string} s + * @returns {string[]} + */ +function tokenize(s) { + return String(s || "") + .toLowerCase() + .replace(/[^a-z0-9]+/g, " ") + .split(/\s+/) + .filter((w) => w.length >= 3); +} + +/** + * @param {string[]} arr + * @returns {string[]} + */ +function unique(arr) { + const seen = new Set(); + const out = []; + for (const x of arr) { + if (!x || seen.has(x)) continue; + seen.add(x); + out.push(x); + } + return out; +} + +/** + * @param {{ + * prBody?: string, + * checkRuns?: unknown, + * dir?: string, + * io?: { + * writeFile?: typeof fs.writeFileSync, + * mkdir?: typeof fs.mkdirSync, + * log?: (s: string) => void, + * }, + * }} args + */ +function writeTestPlanCiArtifacts({ + prBody = "", + checkRuns = [], + dir = DIR, + io, +} = {}) { + const writeFile = (io && io.writeFile) || fs.writeFileSync; + const mkdir = + (io && io.mkdir) || ((p) => fs.mkdirSync(p, { recursive: true })); + const log = (io && io.log) || ((s) => process.stderr.write(s)); + + const items = extractTestPlanItems(prBody); + const checks = summarizeCiChecks(checkRuns); + + mkdir(dir); + writeFile( + path.join(dir, "test-plan-items.json"), + `${JSON.stringify({ schema: 1, items }, null, 2)}\n`, + ); + writeFile( + path.join(dir, "ci-checks.json"), + `${JSON.stringify({ schema: 1, checks }, null, 2)}\n`, + ); + + log( + `testplan-ci: ${items.length} test-plan item(s), ${checks.length} check(s)\n`, + ); + + return { items, checks }; +} + +function main() { + const dir = process.env.AI_REVIEW_DIR || DIR; + const bodyPath = + process.env.PR_BODY_PATH || path.join(dir, "pr-body.md"); + const checksPath = + process.env.CHECK_RUNS_PATH || path.join(dir, "check-runs.raw.json"); + + let prBody = ""; + try { + prBody = fs.readFileSync(bodyPath, "utf8"); + } catch { + prBody = ""; + } + + let checkRuns = []; + try { + checkRuns = JSON.parse(fs.readFileSync(checksPath, "utf8")); + } catch { + checkRuns = []; + } + if (!Array.isArray(checkRuns)) checkRuns = []; + + writeTestPlanCiArtifacts({ prBody, checkRuns, dir }); +} + +module.exports = { + extractTestPlanItems, + extractTestPlanSection, + extractCheckboxes, + summarizeCiChecks, + findObviousUncoveredItems, + writeTestPlanCiArtifacts, + main, +}; + +if (require.main === module) main(); diff --git a/ai-review/lib/testplan-ci.test.js b/ai-review/lib/testplan-ci.test.js new file mode 100644 index 0000000..06353d2 --- /dev/null +++ b/ai-review/lib/testplan-ci.test.js @@ -0,0 +1,159 @@ +"use strict"; + +const test = require("node:test"); +const assert = require("node:assert/strict"); +const path = require("node:path"); + +const { + extractTestPlanItems, + extractTestPlanSection, + summarizeCiChecks, + findObviousUncoveredItems, + writeTestPlanCiArtifacts, +} = require("./testplan-ci.js"); + +// --- extractTestPlanItems -------------------------------------------------- + +test("extractTestPlanItems: Test Plan section checkboxes", () => { + const body = [ + "## Summary", + "- [ ] Not a plan item", + "", + "## Test Plan", + "- [ ] Unit tests for parser", + "- [x] CI green on PR", + "", + "## Notes", + "- [ ] Ignored after section", + ].join("\n"); + + assert.deepEqual(extractTestPlanItems(body), [ + "Unit tests for parser", + "CI green on PR", + ]); +}); + +test("extractTestPlanItems: plain bullets under Test Plan when no checkboxes", () => { + const body = [ + "## Test Plan", + "- Run unit suite", + "1. Manual smoke on staging", + ].join("\n"); + + assert.deepEqual(extractTestPlanItems(body), [ + "Run unit suite", + "Manual smoke on staging", + ]); +}); + +test("extractTestPlanItems: falls back to all body checkboxes without section", () => { + const body = [ + "Please verify:", + "- [ ] Handles empty input", + "* [x] Docs updated", + ].join("\n"); + + assert.deepEqual(extractTestPlanItems(body), [ + "Handles empty input", + "Docs updated", + ]); +}); + +test("extractTestPlanItems: empty / non-string → []", () => { + assert.deepEqual(extractTestPlanItems(""), []); + assert.deepEqual(extractTestPlanItems(null), []); + assert.deepEqual(extractTestPlanItems(undefined), []); +}); + +test("extractTestPlanSection stops at next same-or-higher heading", () => { + const body = [ + "### Test Plan", + "- [ ] A", + "#### Nested", + "- [ ] B", + "## Other", + "- [ ] C", + ].join("\n"); + const section = extractTestPlanSection(body); + assert.match(section, /A/); + assert.match(section, /B/); + assert.doesNotMatch(section, /C/); + assert.deepEqual(extractTestPlanItems(body), ["A", "B"]); +}); + +// --- summarizeCiChecks ----------------------------------------------------- + +test("summarizeCiChecks maps name/conclusion/status", () => { + assert.deepEqual( + summarizeCiChecks([ + { name: "lint", conclusion: "success", status: "completed" }, + { name: "test", conclusion: null, status: "in_progress" }, + { name: "", status: "completed" }, + null, + ]), + [ + { name: "lint", conclusion: "success", status: "completed" }, + { name: "test", conclusion: null, status: "in_progress" }, + ], + ); +}); + +test("summarizeCiChecks: non-array → []", () => { + assert.deepEqual(summarizeCiChecks(null), []); + assert.deepEqual(summarizeCiChecks({}), []); +}); + +// --- findObviousUncoveredItems --------------------------------------------- + +test("findObviousUncoveredItems: keyword overlap vs obvious miss", () => { + const items = [ + "Unit tests for auth login", + "Deploy to production manually", + ]; + const checks = [ + { name: "unit / auth", conclusion: "success", status: "completed" }, + ]; + assert.deepEqual(findObviousUncoveredItems(items, checks), [ + "Deploy to production manually", + ]); +}); + +test("findObviousUncoveredItems: empty checks → all items uncovered", () => { + assert.deepEqual(findObviousUncoveredItems(["A thing"], []), ["A thing"]); +}); + +// --- writeTestPlanCiArtifacts ---------------------------------------------- + +test("writeTestPlanCiArtifacts writes both JSON shapes", () => { + const written = new Map(); + const { items, checks } = writeTestPlanCiArtifacts({ + prBody: "## Test Plan\n- [ ] Parser edge cases\n", + checkRuns: [ + { name: "ci / test", conclusion: "success", status: "completed" }, + ], + dir: "/tmp/fake-ai-review", + io: { + writeFile: (p, data) => { + written.set(p, data); + }, + mkdir: () => {}, + log: () => {}, + }, + }); + + assert.deepEqual(items, ["Parser edge cases"]); + assert.equal(checks.length, 1); + + const plan = JSON.parse( + written.get(path.join("/tmp/fake-ai-review", "test-plan-items.json")), + ); + const ci = JSON.parse( + written.get(path.join("/tmp/fake-ai-review", "ci-checks.json")), + ); + assert.equal(plan.schema, 1); + assert.deepEqual(plan.items, ["Parser edge cases"]); + assert.equal(ci.schema, 1); + assert.deepEqual(ci.checks, [ + { name: "ci / test", conclusion: "success", status: "completed" }, + ]); +}); diff --git a/ai-review/rubric.md b/ai-review/rubric.md index 6b95b31..b158040 100644 --- a/ai-review/rubric.md +++ b/ai-review/rubric.md @@ -28,10 +28,14 @@ these establish the **stated goal**: what was this change supposed to do? (If `gh issue view ` is available if you need more detail on a linked issue.) Extract acceptance criteria in this priority order: -1. Explicit checklist items (`- [ ]` / `- [x]`) in a linked issue or the PR body +1. Explicit Test Plan / checklist items (`- [ ]` / `- [x]`) in a linked issue or the PR body + (Prep may also stage them in `.ai-review/test-plan-items.json`) 2. Numbered requirements or "must/should" statements 3. Inferred intent from title + description narrative +For Test Plan items specifically, also read `.ai-review/ci-checks.json` when present: +uncovered or weakly covered items are findings (P0–P3), not PR-body ticks. + Compare against the diff: | Signal | Finding | Severity | @@ -176,15 +180,15 @@ Examples: formatting preference, typo in comment, minor refactoring opportunity. - [ ] Description explains the WHY, not just the WHAT. - [ ] Breaking changes called out explicitly. - [ ] `Closes #NNN` link present when the PR closes an issue. -- [ ] If a linked issue or the PR description has a checklist, evaluate each item - and return it in the structured `checklist` array with a `status` of `verified` - (confirmed satisfied — cite how in `evidence`), `failed` (confirmed NOT - satisfied), or `unverifiable` (a manual/post-merge step you cannot check from - code or tests). Only mark `verified` with real evidence — a later deterministic - step ticks verified boxes in the PR body, so an unfounded `verified` writes a - false claim into the description. Still report the completed-vs-incomplete ratio - as a P2 reminder; do NOT fail the review on checklist status alone (checklists - often track post-merge activities). +- [ ] If a linked issue or the PR description has a Test Plan / checklist, + map each item to CI coverage using `.ai-review/test-plan-items.json` and + `.ai-review/ci-checks.json` when present. Uncovered or only weakly covered + items become normal `findings[]` entries with severity P0–P3 via this + rubric (model-judged; do not floor all gaps at P1). Leave the structured + `checklist` array empty — Publish no longer ticks PR-body boxes. Never + invent CI coverage; manual/post-merge items with no plausible check mapping + may be omitted or filed at low severity. Do NOT fail the review solely + because a human left boxes unchecked. ### 2. Merge Conflict Handling @@ -296,6 +300,13 @@ Flag as P0 (breaks feature) or P1 (incorrect behavior). ### 12. Testing +**Test Plan ↔ CI (do not tick PR checklists):** Prep writes +`.ai-review/test-plan-items.json` and `.ai-review/ci-checks.json`. Map each +Test Plan item to inventoried checks. Gaps (uncovered / weakly covered) are +normal findings with P0–P3 severity per this rubric. Leave `checklist` empty — +Publish does not tick PR-body boxes. Still report `test_execution: "skipped"`; +do not run tests. + **Coverage expectations** (flag misses at the severity shown): | Scope | Minimum | Flag as | From d82f0b9beee15ff4d7e37e03c179fe1976bf9229 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 13:43:44 +0300 Subject: [PATCH 06/16] fix(ai-review): union Test Plan checkboxes with plain list items Keep mixed checkbox + bullet/numbered items; fall back to body checkboxes when the section is empty. --- ai-review/lib/testplan-ci.js | 13 +++++++--- ai-review/lib/testplan-ci.test.js | 43 +++++++++++++++++++++++++++++++ 2 files changed, 52 insertions(+), 4 deletions(-) diff --git a/ai-review/lib/testplan-ci.js b/ai-review/lib/testplan-ci.js index 5e9eee2..afd5062 100644 --- a/ai-review/lib/testplan-ci.js +++ b/ai-review/lib/testplan-ci.js @@ -28,10 +28,15 @@ function extractTestPlanItems(prBody) { const section = extractTestPlanSection(prBody); if (section != null) { - const boxes = extractCheckboxes(section); - if (boxes.length) return unique(boxes); - const plain = extractPlainListItems(section); - return unique(plain); + // Union checkboxes and plain/numbered bullets (neither extractor alone). + const fromSection = unique([ + ...extractCheckboxes(section), + ...extractPlainListItems(section), + ]); + if (fromSection.length) return fromSection; + // Empty Test Plan section → still honor body checkboxes elsewhere + // (spec: section and/or checklist items). + return unique(extractCheckboxes(prBody)); } return unique(extractCheckboxes(prBody)); diff --git a/ai-review/lib/testplan-ci.test.js b/ai-review/lib/testplan-ci.test.js index 06353d2..3dc7db5 100644 --- a/ai-review/lib/testplan-ci.test.js +++ b/ai-review/lib/testplan-ci.test.js @@ -46,6 +46,39 @@ test("extractTestPlanItems: plain bullets under Test Plan when no checkboxes", ( ]); }); +test("extractTestPlanItems: unions checkboxes with plain/numbered in same section", () => { + const body = [ + "## Test Plan", + "- [ ] Automated unit suite", + "- Manual smoke on staging", + "2. Load-test the API", + ].join("\n"); + + assert.deepEqual(extractTestPlanItems(body), [ + "Automated unit suite", + "Manual smoke on staging", + "Load-test the API", + ]); +}); + +test("extractTestPlanItems: empty Test Plan section falls back to body checkboxes", () => { + const body = [ + "## Summary", + "- [ ] Outside checklist item", + "", + "## Test Plan", + "See checklist above.", + "", + "## Notes", + "- [x] Another outside item", + ].join("\n"); + + assert.deepEqual(extractTestPlanItems(body), [ + "Outside checklist item", + "Another outside item", + ]); +}); + test("extractTestPlanItems: falls back to all body checkboxes without section", () => { const body = [ "Please verify:", @@ -59,6 +92,16 @@ test("extractTestPlanItems: falls back to all body checkboxes without section", ]); }); +test("extractTestPlanItems: dedupes same text checked and unchecked", () => { + const body = [ + "## Test Plan", + "- [ ] Handles empty input", + "- [x] Handles empty input", + ].join("\n"); + + assert.deepEqual(extractTestPlanItems(body), ["Handles empty input"]); +}); + test("extractTestPlanItems: empty / non-string → []", () => { assert.deepEqual(extractTestPlanItems(""), []); assert.deepEqual(extractTestPlanItems(null), []); From 26d8716bdd948d0e8310ff9f3e1c269f7cc9d659 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 15:09:29 +0300 Subject: [PATCH 07/16] feat(ai-review): Opus-managed Sonnet/Haiku subagent review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Route review model by roster K from assignments.json (K≤1 Sonnet collapse, K>1 Opus fan-out). Deprecate file/churn thresholds for selection. Parent SO remains the Publish gate; multi-role aggregate.js stays shadow-only. --- ai-review/README.md | 52 ++-- ai-review/action.yml | 267 ++++++++++++------ ...-17-ai-review-osh-delta-testplan-design.md | 52 +++- 3 files changed, 262 insertions(+), 109 deletions(-) diff --git a/ai-review/README.md b/ai-review/README.md index e8d8df4..067bc52 100644 --- a/ai-review/README.md +++ b/ai-review/README.md @@ -8,9 +8,10 @@ workflow can gate its own heavier build/test/deploy jobs on `verdict == 'pass'`. The verdict comes from a real two-stage AI review: a Haiku context stage -summarizes the diff, then a diff-size-routed Sonnet/Opus review stage -performs the full rubric scan (see `ai-review/rubric.md`) and returns a -schema-validated structured result. Model IDs are **locked in the action** +summarizes the diff, then a roster-K-routed Sonnet (collapse) or Opus +(OSH fan-out) review stage performs the full rubric scan (see +`ai-review/rubric.md`) and returns a schema-validated structured result. +Model IDs are **locked in the action** (Claude primary → for context, Cursor `composer-2.5` then free; for schema reviews, structured_output free models only — Cursor has no SO on this gateway). The posted PR review footer names the model that @@ -70,16 +71,13 @@ injection-safety rule. model turns — a model that derives the diff base from a false premise reviews the wrong range and reports confidently on it. - The same step routes ordinary diffs to the locked Sonnet primary - (`claude/claude-sonnet-5`), escalating to Opus (`claude/claude-opus-5`) - once a diff exceeds **either** `sonnet-files-threshold` - (25) or `sonnet-churn-threshold` (800). These were briefly 3/60, which - sent nearly every real PR to Opus and moved the review stage from - ~10-13 min to a 35-min median. Widened 15/400 → 25/800 after measuring - 682 review jobs across the consumer repos: 89% of all traffic still - routed to Opus, and Opus runs cost 3x the wall-clock and 4x the spend of - Sonnet ones. Still a **stopgap** — the real fix is reviewing in parallel - rather than in series, and both thresholds are removed once that lands. + The same step routes by roster **K** from `.ai-review/assignments.json` + (not file/churn size): **K≤1** → locked Sonnet (`claude/claude-sonnet-5`) + collapse — one session emits `--json-schema` directly; **K>1** → locked + Opus (`claude/claude-opus-5`) parent that fans out native Sonnet/Haiku + subagents per the roster (only the parent emits structured output). + Inputs `sonnet-files-threshold` / `sonnet-churn-threshold` remain accepted + for backward compatibility but are **deprecated for model routing**. It also writes `.ai-review/assignments.json`: the review roster — related changed files clustered, then packed into @@ -94,11 +92,8 @@ injection-safety rule. independent of total bytes, and a cluster exceeding *either* budget is split at file boundaries (never inside a file — there is no byte-range field in the schema) with the affected paths recorded in - `split_clusters` for the tracer. **Nothing reads it yet** — the review - below is still one serial session. It ships early, and best-effort, so - the partition it asserts (bins pairwise disjoint, no stray path, union - equal to `changed_files`) is exercised on real diffs before any model - stage depends on it. + `split_clusters` for the tracer. Prep writes `.ai-review/osh-policy.md` + (`collapse` vs `fanout`) for the review prompt. Cap remains **K≤4**. The emitted `k` is how many coverage reviewers actually exist — `min(formula, piece count after splitting)`, and `0` on an empty diff — @@ -132,7 +127,9 @@ injection-safety rule. `.ai-review/scores.json`. Each role states its own path in `artifact`, and `findings_roles` is the pre-filtered list to hand aggregation as its `roster` — passing all of `roles[]` would demand a findings file from - the scorer and fail every run. + the scorer and fail every run. Publish still gates on the parent (or + collapsed Sonnet) structured output; multi-role `aggregate.js` remains + shadow/non-gating. 8. **Resolve linked issues** — deterministically resolves every issue the PR closes (closing keywords *and* GitHub's linked-issue graph, via the PR's `closingIssuesReferences`) into `.ai-review/linked-issues.json`. @@ -218,8 +215,8 @@ injection-safety rule. | `qa-pass-label` | Post-merge `ai-qa` pass label; cleared (not applied) by this action on every new commit. | No | `✓ /ai-qa` | | `qa-fail-label` | Post-merge `ai-qa` fail label; cleared (not applied) by this action on every new commit. | No | `✗ /ai-qa` | | `confidence-threshold` | Minimum **blocking-finding** confidence (0-100) required for a pass. The Publish step recomputes confidence from the review stage's P0/P1 counts and test-quality signals and compares it against this threshold. P2/P3 findings lower the *reported* confidence but are advisory and never block. | No | `90` | -| `sonnet-files-threshold` | Max changed-file count for a diff to still route to the locked Sonnet primary (must hold together with `sonnet-churn-threshold`); larger diffs route to Opus. | No | `25` | -| `sonnet-churn-threshold` | Max changed-line count (adds + deletes) for a diff to still route to Sonnet. | No | `800` | +| `sonnet-files-threshold` | **DEPRECATED** — accepted but ignored for model routing. Roster K selects Sonnet collapse vs Opus fan-out. | No | `25` | +| `sonnet-churn-threshold` | **DEPRECATED** — accepted but ignored for model routing. Same as above. | No | `800` | | `enable-context-stage` | When `false`, skips the Haiku context stage (and its `context.md` verification) entirely. The stage is best-effort and its output optional, so disabling it removes a wall-clock risk without changing the gate contract. | No | `true` | | `api-timeout-ms` | Per-request timeout (ms) for every Claude stage, passed as `API_TIMEOUT_MS` (CLI default `600000`). **Does not bound the ~27.5-min stall** — a run with this set to `180000` still stalled 27m36s. It is a genuine per-request bound and fails a wedged request faster than the default, nothing more. | No | `180000` | | `test-command` | **DEPRECATED — accepted but ignored.** The Review stage no longer runs tests; see [Why the review no longer runs tests](#why-the-review-no-longer-runs-tests). | No | — | @@ -254,6 +251,19 @@ normal `findings[]` with severity P0–P3 via the rubric (then `recompute.js`). Checklist tick write-back is retired (`update-pr-body` is a no-op for that path). `test_execution` stays `"skipped"` — no test runners in the allowlist. +## OSH routing (roster K) + +Prep packs the active-range files into coverage bins and writes +`.ai-review/assignments.json` with `.k`: + +| K | Mode (`osh-mode`) | Review session | +| --- | --- | --- | +| ≤1 (incl. empty-diff `0`) | `collapse` | Single Sonnet + `--json-schema` (no Opus parent, no Haiku scorer) | +| >1 (max 4) | `fanout` | Opus parent + native Sonnet/Haiku Task subagents; only Opus emits SO | + +Publish still gates on that session's structured output. Multi-role +`aggregate.js` stays shadow/non-gating. + ## Outputs | Name | Description | diff --git a/ai-review/action.yml b/ai-review/action.yml index edd4494..8e68cad 100644 --- a/ai-review/action.yml +++ b/ai-review/action.yml @@ -9,9 +9,9 @@ description: > job output rather than an identity-pinned Check Run, and why the review identity is cosmetic. The verdict is produced by a real two-stage AI review pipeline: a Haiku context stage summarizes the diff, then a - diff-size-routed Sonnet/Opus review stage performs the full rubric scan - and returns a schema-validated result that a deterministic step - recomputes the pass/fail decision from. + roster-K-routed Sonnet (collapse) or Opus (OSH fan-out) review stage + performs the full rubric scan and returns a schema-validated result that + a deterministic step recomputes the pass/fail decision from. inputs: app-id: @@ -96,24 +96,18 @@ inputs: default: "90" sonnet-files-threshold: description: > - Max changed-file count for a diff to still be considered "tiny" and - routed to the locked Sonnet primary. A diff qualifies for Sonnet only when it - is at or under BOTH this and `sonnet-churn-threshold`; anything - larger routes to the locked Opus primary. Widened to 25 (from 15) — measured over - 682 review jobs, 89% of all traffic routed to Opus, and Opus runs cost - 3x the wall-clock (28.1 min median vs 9.2) and 4x the spend ($13.22 vs - $3.27) of Sonnet ones. This is NOT a depth concession: the parallel - design puts every coverage reviewer on Sonnet under the approved OSH - tiering, so widening early previews that end state. Both thresholds go - away entirely once the review stage fans out. + DEPRECATED — accepted for backward compatibility but ignored for model + routing. Review model selection uses roster K from + `.ai-review/assignments.json` (K≤1 → Sonnet collapse; K>1 → Opus OSH + parent). Formerly: max changed-file count for Sonnet when paired with + `sonnet-churn-threshold`. required: false default: "25" sonnet-churn-threshold: description: > - Max changed-line count (additions + deletions) for a diff to still - be considered "tiny" and routed to the locked Sonnet primary. Applied together - with `sonnet-files-threshold` (both must hold for Sonnet). Widened to - 800 alongside `sonnet-files-threshold` — see the note there. + DEPRECATED — accepted for backward compatibility but ignored for model + routing. See `sonnet-files-threshold`. Formerly: max changed-line count + (additions + deletions) for Sonnet when paired with that input. required: false default: "800" enable-context-stage: @@ -403,11 +397,10 @@ runs: # # It also emits `.ai-review/assignments.json` — the review roster from # lib/roster.js: which reviewer reads which files, and under which model. - # NOTHING CONSUMES IT YET; the review below is still one serial session. - # It ships now, and best-effort, so the partition it asserts (bins - # pairwise disjoint, union equal to changed_files) is exercised on real - # diffs across every consumer before a model stage depends on it. Same - # shadow-mode discipline the aggregation step shipped under. + # Slice 3 (OSH): routing and the review prompt consume `.k` and the role + # list. K≤1 → single Sonnet session (collapse); K>1 → Opus parent that + # fans out native Sonnet/Haiku subagents per the roster. No Node + # Promise.race orchestrator — concurrency is Claude Code's scheduler. # # All parsing lives in lib/prep.js and lib/roster.js behind unit tests; # this step only runs git and hands stdout over. Nothing here is @@ -418,8 +411,7 @@ runs: BASE_REF: ${{ steps.pr-state.outputs.base-ref }} # Locked at action level — keep in sync with ai-qa cascade comments. # Claude primary → Cursor if Claude blocked → free if Cursor blocked. - FILES_MAX: ${{ inputs.sonnet-files-threshold }} - CHURN_MAX: ${{ inputs.sonnet-churn-threshold }} + # sonnet-*-threshold inputs are deprecated for routing (kept unused). MANIFEST_CLI: ${{ github.action_path }}/lib/write-manifest.js DELTA_CLI: ${{ github.action_path }}/lib/write-delta.js GH_TOKEN: ${{ steps.identity.outputs.author-token }} @@ -499,37 +491,105 @@ runs: FILES="$(jq -r '.file_count' .ai-review/manifest.json)" CHURN="$(jq -r '.churn' .ai-review/manifest.json)" - - # Sonnet for ordinary diffs; Opus once a diff exceeds EITHER - # threshold. 7168977 narrowed the Sonnet band from 15/400 to 3/60, - # which routed nearly every real PR to Opus and took the review - # stage from ~10-13 min to a 35-min median (59-min max, jobs killed - # at callers' 60-min cap). Every consumer picked that up at once - # because they track this action at @main. - # - # Widened 15/400 -> 25/800. Measured across 682 review jobs in 7 - # consumer repos: 89% of ALL traffic still routed to Opus at 15/400, - # and Opus runs cost 3x the wall-clock (28.1 min median vs 9.2) and - # 4x the spend ($13.22 vs $3.27) of Sonnet ones. - # - # This is NOT a depth concession. The approved parallel design puts - # every coverage reviewer on Sonnet under the OSH tiering, so this - # previews the agreed end state rather than deviating from it. - # Still a STOPGAP: the real fix is reviewing in parallel rather than - # in series, and BOTH thresholds are deleted when the review stage - # fans out. Not the long-term answer. - FILES_MAX="${FILES_MAX:-25}" - CHURN_MAX="${CHURN_MAX:-800}" - if [ "${FILES}" -le "${FILES_MAX}" ] && [ "${CHURN}" -le "${CHURN_MAX}" ]; then + # Roster K drives OSH topology (spec §5.2 / Appendix A). Not file/churn + # thresholds — those inputs are deprecated for model selection. + K="$(jq -r '.k // 0' .ai-review/assignments.json)" + K="${K:-0}" + + # K≤1 (including empty-diff k:0): single Sonnet session emits + # --json-schema directly — no Opus parent, no Haiku scorer. + # K>1: Opus parent + native Sonnet/Haiku subagents; only Opus emits SO. + if [ "${K}" -le 1 ]; then MODEL="${SONNET}" FALLBACK="${SONNET_FALLBACK}" + OSH_MODE="collapse" else MODEL="${OPUS}" FALLBACK="${OPUS_FALLBACK}" + OSH_MODE="fanout" + fi + echo "prep: osh-mode=${OSH_MODE} k=${K} model=${MODEL} files=${FILES} churn=${CHURN}" + + # Policy file the review/retry prompts must obey (keeps one shared + # prompt body with mode-specific orchestration rules). + if [ "${OSH_MODE}" = "fanout" ]; then + cat > .ai-review/osh-policy.md <<'EOF' +# OSH fan-out policy (K>1) + +You are the **Opus parent** for this review. Only YOU emit the required +`--json-schema` structured output for Publish. Do not ask subagents to emit +that schema blob. + +## Roster + +Read `.ai-review/assignments.json` first. Honor its partition: do not invent +a second file split. Cap is K≤4 coverage reviewers. + +Locked model IDs (match `roles[].model` when spawning): + +- Opus parent / intent: `claude/claude-opus-5` +- Sonnet coverage + tracer: `claude/claude-sonnet-5` +- Haiku history + scorer: `claude/claude-haiku-4-5-20251001` + +## Subagents (native Claude Code Task/Agent tool) + +Spawn concurrent subagents: + +1. **Sonnet** — one per `reviewer-*` (`kind: coverage`): read 100% of every + `assigned_files` path with `Read`; run per-file rubric angles; propose + findings with P0–P3 severity. Write JSON to the role's `artifact` path + under `.ai-review/` when useful. +2. **Sonnet** — `tracer` (`kind: coherence`): zero assigned files; follow + `symbol_manifest` / `split_clusters` for cross-file breakage (Angle C). +3. **Haiku** — `history` (`kind: perspective`): git blame / prior comments / + code-comment angles; cheap gathers, not full-file exhaustiveness. +4. **Haiku** — `scorer` (`kind: scoring`): independent confidence 0/25/50/75/100 + and `severity_confirmed` for findings you (or workers) raised. Finder ≠ + scorer. Prefer writing `.ai-review/scores.json`. +5. **Intent** (`kind: frame`): you own Angle H (or dispatch an Opus-tier + subagent). Frame from linked issues + PR body **before** treating coverage + findings as settled. + +Workers return freeform or JSON under `.ai-review/`; they must not emit +Publish structured output. + +## What you must not re-read + +Do **not** exhaustively re-read every file workers already covered unless +conflict resolution or a spot-check needs it. Must-read-all for the active +range is satisfied when coverage workers' `assigned_files` union equals +`changed_files` (plus targeted neighbor reads when prior findings or imports +require it). + +## Your job after workers return + +Resolve conflicts, prioritize, own intent, map Test Plan↔CI gaps to findings, +apply delta prior-review carry-forward rules, then emit the single structured +output (`comment_markdown`, `findings`, `files_reviewed`, `counts`, `intent`, +…). Aggregate worker outputs yourself into that schema — Publish gates on your +SO only. +EOF + else + cat > .ai-review/osh-policy.md <<'EOF' +# OSH collapse policy (K≤1) + +You are a **single Sonnet** reviewer session. Emit `--json-schema` structured +output directly. + +- Do **not** spawn Task/Agent subagents for coverage / scorer / history fan-out. +- Do **not** expect an Opus parent or an independent Haiku scorer on this path. +- Read the COMPLETE contents of every file in `manifest.json` `changed_files` + with `Read` — never sample, truncate, or reason from diff hunks alone. +- Perform the full rubric review yourself (all angles that apply), including + intent, Test Plan↔CI findings, and delta carry-forward when present. +EOF fi + { echo "files=${FILES}" echo "churn=${CHURN}" + echo "k=${K}" + echo "osh-mode=${OSH_MODE}" echo "model=${MODEL}" echo "fallback-model=${FALLBACK}" echo "haiku-model=${HAIKU}" @@ -793,6 +853,7 @@ runs: API_TIMEOUT_MS: ${{ inputs.api-timeout-ms }} ANTHROPIC_CUSTOM_HEADERS: "${{ inputs.anthropic-auth-token != '' && format('Authorization: Bearer {0}', inputs.anthropic-auth-token) || '' }}" PR_NUMBER: ${{ steps.pr.outputs.pr-number }} + OSH_MODE: ${{ steps.route.outputs.osh-mode }} with: anthropic_api_key: ${{ inputs.anthropic-api-key != '' && inputs.anthropic-api-key || inputs.anthropic-auth-token }} # Deliberately NOT steps.identity.outputs.author-token, for the same @@ -808,7 +869,7 @@ runs: --model ${{ steps.route.outputs.model }} --fallback-model ${{ steps.route.outputs.fallback-model }} --max-turns 200 - --allowedTools "Read,Grep,Glob,Bash(gh pr view:*),Bash(gh issue view:*),Bash(git diff:*),Bash(git symbolic-ref:*),Bash(git merge-base:*),Bash(git remote show:*),Bash(git log:*),Bash(git show:*),Bash(cat:*),Bash(head:*),Bash(tail:*),Bash(ls:*),Bash(find:*),Bash(wc:*)" + --allowedTools "Read,Grep,Glob,Write,Task,Bash(gh pr view:*),Bash(gh issue view:*),Bash(git diff:*),Bash(git symbolic-ref:*),Bash(git merge-base:*),Bash(git remote show:*),Bash(git log:*),Bash(git show:*),Bash(cat:*),Bash(head:*),Bash(tail:*),Bash(ls:*),Bash(find:*),Bash(wc:*)" --json-schema '{"type":"object","additionalProperties":false,"required":["verdict","confidence","merge_risk","intent","counts","review_event","comment_markdown"],"properties":{"verdict":{"type":"string","enum":["pass","fail"]},"confidence":{"type":"integer","minimum":0,"maximum":100},"merge_risk":{"type":"string","enum":["low","med","high"]},"intent":{"type":"string","enum":["aligned","partial","deviated","skipped"]},"counts":{"type":"object","additionalProperties":false,"required":["p0","p1","p2","p3"],"properties":{"p0":{"type":"integer","minimum":0},"p1":{"type":"integer","minimum":0},"p2":{"type":"integer","minimum":0},"p3":{"type":"integer","minimum":0}}},"review_event":{"type":"string","enum":["APPROVE","REQUEST_CHANGES"]},"comment_markdown":{"type":"string"},"findings":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["id","file","line","severity","summary","failure_scenario","reason","evidence","confidence"],"properties":{"id":{"type":"string"},"file":{"type":"string"},"line":{"type":"integer","minimum":0},"severity":{"type":"string","enum":["P0","P1","P2","P3"]},"summary":{"type":"string"},"failure_scenario":{"type":"string"},"reason":{"type":"string"},"evidence":{"type":"string"},"confidence":{"type":"integer","enum":[0,25,50,75,100]},"severity_confirmed":{"type":"string","enum":["P0","P1","P2","P3"]}}}},"files_reviewed":{"type":"array","items":{"type":"string"}},"tests_failing":{"type":"boolean"},"coverage_below_threshold_on_critical_paths":{"type":"boolean"},"no_tests_for_changed_logic":{"type":"boolean"},"test_execution":{"type":"string","enum":["passed","failed","skipped","not_run"]},"verification_evidence":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["claim","command","result"],"properties":{"claim":{"type":"string"},"command":{"type":"string"},"result":{"type":"string"}}}},"checklist":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["text","status"],"properties":{"text":{"type":"string"},"status":{"type":"string","enum":["verified","failed","unverifiable"]},"evidence":{"type":"string"}}}}}}' prompt: | You are performing an automated code review. Attempt to load the @@ -820,11 +881,20 @@ runs: command you ran and its actual output/exit code. If you assert a check passed, you must have run it and read the result this session. + Read `.ai-review/osh-policy.md` FIRST and obey it. Prep set + `OSH_MODE` (`collapse` or `fanout`) to match that file: + - collapse: single-session Sonnet review; no subagent fan-out. + - fanout: you are the Opus parent; spawn Sonnet/Haiku Task + subagents per `.ai-review/assignments.json`; only YOU emit + structured output. Workers may Write under `.ai-review/`. + Read `${{ github.action_path }}/rubric.md`. This is the authoritative review methodology: an 8-angle scan (intent alignment first, then line-by-line diff, removed-behavior audit, cross-file tracing, reuse, simplification, efficiency, altitude), a severity - scale (P0-P3), and a full review checklist. Follow it. + scale (P0-P3), and a full review checklist. Follow it — under + fanout, coverage/coherence/scoring work is delegated per + osh-policy.md while you still own the final judgment. The PR number is available in the `PR_NUMBER` environment variable. Run `gh pr view "$PR_NUMBER" --json title,body,url,number` to read @@ -845,13 +915,13 @@ runs: context-gathering stage) for a summary of what changed and its callers/callees/related helpers. - Read `.ai-review/manifest.json` FIRST. A prior deterministic step - already resolved the review range and staged the facts you would - otherwise re-derive: `base_sha` and `head_sha` (PR merge-base and - HEAD for telemetry), `review_mode` (`full` or `delta`), - `delta_base_sha` when mode is delta (the prior published review - HEAD — the start of this run's active range), `changed_files` - (the authoritative list for the active range only), + Read `.ai-review/manifest.json` FIRST (after osh-policy). A prior + deterministic step already resolved the review range and staged the + facts you would otherwise re-derive: `base_sha` and `head_sha` (PR + merge-base and HEAD for telemetry), `review_mode` (`full` or + `delta`), `delta_base_sha` when mode is delta (the prior published + review HEAD — the start of this run's active range), + `changed_files` (the authoritative list for the active range only), `symbol_manifest` (declarations touched by the diff), plus `title_ok`, `has_test_change` and `no_tests_for_changed_logic`. @@ -871,17 +941,19 @@ runs: finding or an import requires it — that is targeted expansion, not a second full-PR read. - Read the COMPLETE contents of every file in `changed_files` with - `Read` — never sample, truncate, or reason from the diff hunks alone. - There is no size cap on changed files; large files are why this diff - was routed to the stronger model. `changed_files` is the coverage - contract: every entry gets read in full. For callers/callees outside - the diff (Angle C), start from `symbol_manifest` and follow the - rubric's risk-based bound instead of reading every call site in - full — that angle has no natural stopping point and is the one place - exhaustiveness is not the goal. - - Perform the full 8-angle scan from the rubric against the diff. + Must-read-all for the active range: under **collapse**, YOU read + the COMPLETE contents of every file in `changed_files` with `Read` + — never sample, truncate, or reason from the diff hunks alone. + Under **fanout**, coverage subagents satisfy that contract for + their `assigned_files`; you do not re-read every file unless + osh-policy conflict resolution needs a spot check. For + callers/callees outside the diff (Angle C), start from + `symbol_manifest` and follow the rubric's risk-based bound (or the + tracer role under fanout) — that angle has no natural stopping + point and is the one place exhaustiveness is not the goal. + + Perform the full 8-angle scan from the rubric against the diff + (directly under collapse; via roster roles under fanout). Classify every confirmed or plausible finding into P0/P1/P2/P3 per the rubric's severity definitions, and count them. @@ -927,10 +999,12 @@ runs: structured output as well. `findings` is one entry per finding you report, with your own confidence 0/25/50/75/100 and, where you would reclassify it, `severity_confirmed`. `files_reviewed` lists every file - you read in full — it is cross-checked against the staged manifest, so - a short list reads as incomplete coverage. A later non-gating step - aggregates these deterministically and compares the result against - your own `counts`; they do not affect this run's verdict. + read in full for the active range (yours under collapse; union of + coverage workers under fanout) — it is cross-checked against the + staged manifest, so a short list reads as incomplete coverage. A + later non-gating step aggregates these deterministically and + compares the result against your own `counts`; they do not affect + this run's verdict. Return your full assessment via the required structured output schema. `counts` must be accurate — a later deterministic step @@ -1144,6 +1218,7 @@ runs: API_TIMEOUT_MS: ${{ inputs.api-timeout-ms }} ANTHROPIC_CUSTOM_HEADERS: "${{ inputs.anthropic-auth-token != '' && format('Authorization: Bearer {0}', inputs.anthropic-auth-token) || '' }}" PR_NUMBER: ${{ steps.pr.outputs.pr-number }} + OSH_MODE: ${{ steps.route.outputs.osh-mode }} with: anthropic_api_key: ${{ inputs.anthropic-api-key != '' && inputs.anthropic-api-key || inputs.anthropic-auth-token }} github_token: ${{ inputs.github-token }} @@ -1154,21 +1229,27 @@ runs: --model ${{ steps.route.outputs.model }} --fallback-model ${{ steps.route.outputs.fallback-model }} --max-turns 200 - --allowedTools "Read,Grep,Glob,Bash(gh pr view:*),Bash(gh issue view:*),Bash(git diff:*),Bash(git symbolic-ref:*),Bash(git merge-base:*),Bash(git remote show:*),Bash(git log:*),Bash(git show:*),Bash(cat:*),Bash(head:*),Bash(tail:*),Bash(ls:*),Bash(find:*),Bash(wc:*)" + --allowedTools "Read,Grep,Glob,Write,Task,Bash(gh pr view:*),Bash(gh issue view:*),Bash(git diff:*),Bash(git symbolic-ref:*),Bash(git merge-base:*),Bash(git remote show:*),Bash(git log:*),Bash(git show:*),Bash(cat:*),Bash(head:*),Bash(tail:*),Bash(ls:*),Bash(find:*),Bash(wc:*)" --json-schema '{"type":"object","additionalProperties":false,"required":["verdict","confidence","merge_risk","intent","counts","review_event","comment_markdown"],"properties":{"verdict":{"type":"string","enum":["pass","fail"]},"confidence":{"type":"integer","minimum":0,"maximum":100},"merge_risk":{"type":"string","enum":["low","med","high"]},"intent":{"type":"string","enum":["aligned","partial","deviated","skipped"]},"counts":{"type":"object","additionalProperties":false,"required":["p0","p1","p2","p3"],"properties":{"p0":{"type":"integer","minimum":0},"p1":{"type":"integer","minimum":0},"p2":{"type":"integer","minimum":0},"p3":{"type":"integer","minimum":0}}},"review_event":{"type":"string","enum":["APPROVE","REQUEST_CHANGES"]},"comment_markdown":{"type":"string"},"findings":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["id","file","line","severity","summary","failure_scenario","reason","evidence","confidence"],"properties":{"id":{"type":"string"},"file":{"type":"string"},"line":{"type":"integer","minimum":0},"severity":{"type":"string","enum":["P0","P1","P2","P3"]},"summary":{"type":"string"},"failure_scenario":{"type":"string"},"reason":{"type":"string"},"evidence":{"type":"string"},"confidence":{"type":"integer","enum":[0,25,50,75,100]},"severity_confirmed":{"type":"string","enum":["P0","P1","P2","P3"]}}}},"files_reviewed":{"type":"array","items":{"type":"string"}},"tests_failing":{"type":"boolean"},"coverage_below_threshold_on_critical_paths":{"type":"boolean"},"no_tests_for_changed_logic":{"type":"boolean"},"test_execution":{"type":"string","enum":["passed","failed","skipped","not_run"]},"verification_evidence":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["claim","command","result"],"properties":{"claim":{"type":"string"},"command":{"type":"string"},"result":{"type":"string"}}}},"checklist":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["text","status"],"properties":{"text":{"type":"string"},"status":{"type":"string","enum":["verified","failed","unverifiable"]},"evidence":{"type":"string"}}}}}}' prompt: | This is a RETRY of an automated code review whose previous attempt ended without returning the required structured output. Emitting the structured output as your FINAL action is mandatory this time. + Read `.ai-review/osh-policy.md` and obey it (`OSH_MODE` is + `collapse` or `fanout`). Under fanout you are the Opus parent — + spawn Sonnet/Haiku Task subagents per `.ai-review/assignments.json`; + only YOU emit structured output. Under collapse, do not fan out. + Populate the optional `findings` and `files_reviewed` fields of the structured output as well. `findings` is one entry per finding you report, with your own confidence 0/25/50/75/100 and, where you would reclassify it, `severity_confirmed`. `files_reviewed` lists every file - you read in full — it is cross-checked against the staged manifest, so - a short list reads as incomplete coverage. A later non-gating step - aggregates these deterministically and compares the result against - your own `counts`; they do not affect this run's verdict. + read in full for the active range — it is cross-checked against the + staged manifest, so a short list reads as incomplete coverage. A + later non-gating step aggregates these deterministically and compares + the result against your own `counts`; they do not affect this run's + verdict. Read `${{ github.action_path }}/rubric.md` (the authoritative 8-angle scan + P0-P3 severities + review checklist) and `context.md` at the repo root @@ -1189,17 +1270,20 @@ runs: Test Plan items become normal `findings[]` (P0–P3 via the rubric); leave `checklist` empty — Publish does not tick PR-body boxes. Attempt to load `/verification-before-completion` and - `/requesting-code-review` (best-effort). Read complete file contents, - never just the diff hunks. Do NOT run tests or probe for a - toolchain — set `test_execution: "skipped"` (you have no shell - access to a test runner). You MAY populate `verification_evidence` - when you have real command output; omit it if you cannot. - - Perform the full review per the rubric, then write `comment_markdown` - (findings grouped by severity P0-P3 with "_None._" for empty sections, - plus strengths; no leading verdict token / confidence line / HTML - marker — the caller prepends its own banner). `counts` must be - accurate; a deterministic step recomputes the pass/fail decision. + `/requesting-code-review` (best-effort). Under collapse, read + complete file contents (never just diff hunks). Under fanout, + coverage workers do that for assigned paths. Do NOT run tests or + probe for a toolchain — set `test_execution: "skipped"` (you have + no shell access to a test runner). You MAY populate + `verification_evidence` when you have real command output; omit it + if you cannot. + + Perform the full review per the rubric (and osh-policy), then write + `comment_markdown` (findings grouped by severity P0-P3 with + "_None._" for empty sections, plus strengths; no leading verdict + token / confidence line / HTML marker — the caller prepends its own + banner). `counts` must be accurate; a deterministic step recomputes + the pass/fail decision. Return your assessment via the required structured output schema. Do NOT end your turn without emitting the structured output. @@ -1481,6 +1565,13 @@ runs: # Shadow aggregation — OBSERVATION ONLY, gates nothing. # + # Publish still consumes parent/collapsed-session structured_output (same + # as today) for both osh-mode=collapse and fanout. Under fanout the Opus + # parent is prompted to gather worker artifacts into that SO; we do NOT + # wire multi-role findings files into the live gate yet — avoiding a + # half-broken aggregate path. This step still derives review-serial from + # the SO and compares aggregate.js in shadow only. + # # PR-C's plan is to replace structured_output with findings files on disk # and delete the repair/back-off/retry/salvage chain. That is the first # change in this migration that can wrongly BLOCK a developer's PR, and it @@ -1490,7 +1581,9 @@ runs: # This step runs the new deterministic aggregation next to the live path # and records whether the two agree. It reads the files the Derive step # above produces from the schema-validated structured output (the model - # holds no Write grant — see that step's comment), runs lib/aggregate.js, feeds the result through the + # holds no Write grant on the collapse path for gate inputs — see that + # step's comment; fanout may Write worker JSON under `.ai-review/` for + # the parent to read), runs lib/aggregate.js, feeds the result through the # SAME lib/recompute.js the real verdict uses, and compares. Any divergence # is a warning in the log and a row in the job summary. # diff --git a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md index 743a3e1..58c6a99 100644 --- a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md +++ b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md @@ -1,6 +1,6 @@ # `ai-review` OSH + delta + Test-Plan/CI — design -**Status:** approved shape, not yet implemented. Sequencing in §8 governs how it lands. +**Status:** approved shape; Slices 1–2 landed; Slice 3 (OSH fan-out) landing on this branch. **Extends:** [`2026-08-07-ai-review-parallel-review-design.md`](./2026-08-07-ai-review-parallel-review-design.md) (parallel OSH / coverage–coherence / rubric scoring). This document does **not** supersede that design’s §1 binding constraints or its scoring/`recompute.js` contract. @@ -348,3 +348,53 @@ This design is complete when: - Non-goals explicitly exclude test execution, separate stage orchestrators, and fixing #43 here. - Ship order is Slice 1 delta → Slice 2 test-plan/CI → Slice 3 OSH, with K≤4 collapse at K=1. - `recompute.js`, fail-closed Publish, and SO-free cascade for schema stages remain in force. + +--- + +## Appendix A — Opus parent prompt contract (Slice 3) + +Frozen for the live `claude-code-action` parent session when fan-out is active. +Prep still emits `.ai-review/assignments.json`; the parent **must** consume that roster +and must not invent a second partition. + +### Locked model IDs (action-level; keep in sync with `ai-review` / `ai-qa`) + +| Tier | Primary ID used in action / roster | +|---|---| +| Opus (parent / intent tier) | `claude/claude-opus-5` | +| Sonnet (coverage + tracer) | `claude/claude-sonnet-5` | +| Haiku (history + independent scorer) | `claude/claude-haiku-4-5-20251001` | + +Subagents use the `model` field on each role in `assignments.json` (same strings). Cap remains +**K ≤ 4** coverage reviewers (`roster.js` `MAX_K`). + +### Fan-out path (`assignments.json` `.k` > 1) + +1. Parent session `--model` is Opus. Only this session may use `--json-schema` / emit + structured output for Publish. +2. Read `.ai-review/assignments.json`. Spawn native Claude Code **Task/Agent** subagents: + - **Sonnet** for each `reviewer-*` (coverage) and for `tracer` (coherence). + - **Haiku** for `history` and for `scorer` (independent confidence / `severity_confirmed`). + - **Intent** (`kind: frame`): Opus owns it (parent may run it itself or dispatch an Opus-tier + subagent). Keep intent isolated from coverage analysis. +3. Workers may write freeform or JSON under `.ai-review/` (e.g. `findings/.json`, + `scores.json`). They must **not** emit the Publish `--json-schema` blob. +4. Opus **must not** exhaustively re-read every file workers already covered unless conflict + resolution or a spot-check needs it. Must-read-all for the active range is satisfied by the + coverage workers' union of `assigned_files` (plus tracer / neighbor expansion rules). +5. Parent aggregates worker outputs into the schema contract (`comment_markdown`, `findings`, + `counts`, `intent`, etc.). Publish continues to consume **parent structured output** (same + fail-closed gate as today); multi-file `aggregate.js` remains shadow/non-gating until a + follow-up wires it live. + +### Collapse path (`assignments.json` `.k` ≤ 1, including empty-diff `k: 0`) + +- Single **Sonnet** session with `--json-schema` — **no** Opus parent, **no** independent Haiku + scorer, **no** subagent fan-out instructions. +- Artifact / SO shape may match today's serial review. Fail-closed path unchanged. + +### Routing note + +Once Slice 3 is live, model selection is **roster K**, not `sonnet-files-threshold` / +`sonnet-churn-threshold`. Those inputs remain accepted for backward compatibility but do not +choose the review model. From 27d430d84013115ff916a1758286923500191307 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 15:42:20 +0300 Subject: [PATCH 08/16] fix(ai-review): fail-closed roster; mode-conditional allowlists; scorer confidence Roster write failures now fail prep (Slice 3 routes on assignments.json). Collapse restores Read-only allowlist; fanout adds Task only (no Write). Fan-out SO confidence/severity_confirmed come from the Haiku scorer. --- ai-review/action.yml | 86 +++++++++++------ ai-review/lib/roster.js | 2 +- ai-review/lib/write-manifest.js | 79 ++++++---------- ai-review/lib/write-manifest.test.js | 94 ++++++++++--------- ...-17-ai-review-osh-delta-testplan-design.md | 16 ++-- 5 files changed, 144 insertions(+), 133 deletions(-) diff --git a/ai-review/action.yml b/ai-review/action.yml index 8e68cad..40390a6 100644 --- a/ai-review/action.yml +++ b/ai-review/action.yml @@ -510,6 +510,18 @@ runs: fi echo "prep: osh-mode=${OSH_MODE} k=${K} model=${MODEL} files=${FILES} churn=${CHURN}" + # Mode-conditional review allowlist (collapse = prior least-privilege; + # fanout adds Task only). No Write on either path: Claude Code cannot + # scope Write to `.ai-review/` (see Derive step), so workers return + # findings via Task results and the parent aggregates into SO. + REVIEW_TOOLS_COLLAPSE='Read,Grep,Glob,Bash(gh pr view:*),Bash(gh issue view:*),Bash(git diff:*),Bash(git symbolic-ref:*),Bash(git merge-base:*),Bash(git remote show:*),Bash(git log:*),Bash(git show:*),Bash(cat:*),Bash(head:*),Bash(tail:*),Bash(ls:*),Bash(find:*),Bash(wc:*)' + REVIEW_TOOLS_FANOUT='Read,Grep,Glob,Task,Bash(gh pr view:*),Bash(gh issue view:*),Bash(git diff:*),Bash(git symbolic-ref:*),Bash(git merge-base:*),Bash(git remote show:*),Bash(git log:*),Bash(git show:*),Bash(cat:*),Bash(head:*),Bash(tail:*),Bash(ls:*),Bash(find:*),Bash(wc:*)' + if [ "${OSH_MODE}" = "fanout" ]; then + REVIEW_ALLOWED_TOOLS="${REVIEW_TOOLS_FANOUT}" + else + REVIEW_ALLOWED_TOOLS="${REVIEW_TOOLS_COLLAPSE}" + fi + # Policy file the review/retry prompts must obey (keeps one shared # prompt body with mode-specific orchestration rules). if [ "${OSH_MODE}" = "fanout" ]; then @@ -531,28 +543,27 @@ Locked model IDs (match `roles[].model` when spawning): - Sonnet coverage + tracer: `claude/claude-sonnet-5` - Haiku history + scorer: `claude/claude-haiku-4-5-20251001` -## Subagents (native Claude Code Task/Agent tool) +## Subagents (native Claude Code Task tool) -Spawn concurrent subagents: +Spawn concurrent Task subagents (no Write tool — return results in the Task +response text/JSON; you aggregate into structured output): 1. **Sonnet** — one per `reviewer-*` (`kind: coverage`): read 100% of every `assigned_files` path with `Read`; run per-file rubric angles; propose - findings with P0–P3 severity. Write JSON to the role's `artifact` path - under `.ai-review/` when useful. + findings with P0–P3 severity (finder labels only — do not self-score). 2. **Sonnet** — `tracer` (`kind: coherence`): zero assigned files; follow `symbol_manifest` / `split_clusters` for cross-file breakage (Angle C). 3. **Haiku** — `history` (`kind: perspective`): git blame / prior comments / code-comment angles; cheap gathers, not full-file exhaustiveness. -4. **Haiku** — `scorer` (`kind: scoring`): independent confidence 0/25/50/75/100 - and `severity_confirmed` for findings you (or workers) raised. Finder ≠ - scorer. Prefer writing `.ai-review/scores.json`. +4. **Haiku** — `scorer` (`kind: scoring`): independent confidence + 0/25/50/75/100 and `severity_confirmed` for every finding raised by + finders. **Finder ≠ scorer** — you must copy these into the SO + `findings[].confidence` / `severity_confirmed` fields; do not invent + your own scores when the scorer ran. 5. **Intent** (`kind: frame`): you own Angle H (or dispatch an Opus-tier subagent). Frame from linked issues + PR body **before** treating coverage findings as settled. -Workers return freeform or JSON under `.ai-review/`; they must not emit -Publish structured output. - ## What you must not re-read Do **not** exhaustively re-read every file workers already covered unless @@ -565,9 +576,9 @@ require it). Resolve conflicts, prioritize, own intent, map Test Plan↔CI gaps to findings, apply delta prior-review carry-forward rules, then emit the single structured -output (`comment_markdown`, `findings`, `files_reviewed`, `counts`, `intent`, -…). Aggregate worker outputs yourself into that schema — Publish gates on your -SO only. +output. For each finding in SO: take `confidence` and `severity_confirmed` +from the Haiku scorer; severity labels may originate from workers. Publish +gates on your SO only. EOF else cat > .ai-review/osh-policy.md <<'EOF' @@ -582,6 +593,8 @@ output directly. with `Read` — never sample, truncate, or reason from diff hunks alone. - Perform the full rubric review yourself (all angles that apply), including intent, Test Plan↔CI findings, and delta carry-forward when present. +- Self-report finding `confidence` 0/25/50/75/100 and optional + `severity_confirmed` (no independent scorer on this path). EOF fi @@ -590,6 +603,7 @@ EOF echo "churn=${CHURN}" echo "k=${K}" echo "osh-mode=${OSH_MODE}" + echo "review-allowed-tools=${REVIEW_ALLOWED_TOOLS}" echo "model=${MODEL}" echo "fallback-model=${FALLBACK}" echo "haiku-model=${HAIKU}" @@ -857,10 +871,13 @@ EOF with: anthropic_api_key: ${{ inputs.anthropic-api-key != '' && inputs.anthropic-api-key || inputs.anthropic-auth-token }} # Deliberately NOT steps.identity.outputs.author-token, for the same - # least-privilege reason as the Context stage: this step only reads the - # repo, calls the Anthropic API, and returns structured output — it - # never calls the GitHub API to post anything. The App-minted identity - # is reserved for the deterministic Publish review step below. + # least-privilege reason as the Context stage: this step reads the + # repo (and on fanout may spawn Task subagents), calls the Anthropic + # API, and returns structured output — it never calls the GitHub API + # to post anything. Allowlist is mode-conditional + # (`steps.route.outputs.review-allowed-tools`): collapse has no + # Write/Task; fanout adds Task only (no Write — unscopeable; see + # Derive). The App-minted identity is reserved for Publish below. github_token: ${{ inputs.github-token }} allowed_bots: ${{ inputs.allowed-bots }} plugins: superpowers@superpowers-marketplace @@ -869,7 +886,7 @@ EOF --model ${{ steps.route.outputs.model }} --fallback-model ${{ steps.route.outputs.fallback-model }} --max-turns 200 - --allowedTools "Read,Grep,Glob,Write,Task,Bash(gh pr view:*),Bash(gh issue view:*),Bash(git diff:*),Bash(git symbolic-ref:*),Bash(git merge-base:*),Bash(git remote show:*),Bash(git log:*),Bash(git show:*),Bash(cat:*),Bash(head:*),Bash(tail:*),Bash(ls:*),Bash(find:*),Bash(wc:*)" + --allowedTools "${{ steps.route.outputs.review-allowed-tools }}" --json-schema '{"type":"object","additionalProperties":false,"required":["verdict","confidence","merge_risk","intent","counts","review_event","comment_markdown"],"properties":{"verdict":{"type":"string","enum":["pass","fail"]},"confidence":{"type":"integer","minimum":0,"maximum":100},"merge_risk":{"type":"string","enum":["low","med","high"]},"intent":{"type":"string","enum":["aligned","partial","deviated","skipped"]},"counts":{"type":"object","additionalProperties":false,"required":["p0","p1","p2","p3"],"properties":{"p0":{"type":"integer","minimum":0},"p1":{"type":"integer","minimum":0},"p2":{"type":"integer","minimum":0},"p3":{"type":"integer","minimum":0}}},"review_event":{"type":"string","enum":["APPROVE","REQUEST_CHANGES"]},"comment_markdown":{"type":"string"},"findings":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["id","file","line","severity","summary","failure_scenario","reason","evidence","confidence"],"properties":{"id":{"type":"string"},"file":{"type":"string"},"line":{"type":"integer","minimum":0},"severity":{"type":"string","enum":["P0","P1","P2","P3"]},"summary":{"type":"string"},"failure_scenario":{"type":"string"},"reason":{"type":"string"},"evidence":{"type":"string"},"confidence":{"type":"integer","enum":[0,25,50,75,100]},"severity_confirmed":{"type":"string","enum":["P0","P1","P2","P3"]}}}},"files_reviewed":{"type":"array","items":{"type":"string"}},"tests_failing":{"type":"boolean"},"coverage_below_threshold_on_critical_paths":{"type":"boolean"},"no_tests_for_changed_logic":{"type":"boolean"},"test_execution":{"type":"string","enum":["passed","failed","skipped","not_run"]},"verification_evidence":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["claim","command","result"],"properties":{"claim":{"type":"string"},"command":{"type":"string"},"result":{"type":"string"}}}},"checklist":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["text","status"],"properties":{"text":{"type":"string"},"status":{"type":"string","enum":["verified","failed","unverifiable"]},"evidence":{"type":"string"}}}}}}' prompt: | You are performing an automated code review. Attempt to load the @@ -886,7 +903,8 @@ EOF - collapse: single-session Sonnet review; no subagent fan-out. - fanout: you are the Opus parent; spawn Sonnet/Haiku Task subagents per `.ai-review/assignments.json`; only YOU emit - structured output. Workers may Write under `.ai-review/`. + structured output. Workers return findings via Task results + (no Write tool). Read `${{ github.action_path }}/rubric.md`. This is the authoritative review methodology: an 8-angle scan (intent alignment @@ -997,8 +1015,12 @@ EOF Populate the optional `findings` and `files_reviewed` fields of the structured output as well. `findings` is one entry per finding you - report, with your own confidence 0/25/50/75/100 and, where you would - reclassify it, `severity_confirmed`. `files_reviewed` lists every file + report. Under **collapse**, include your own confidence 0/25/50/75/100 + and, where you would reclassify it, `severity_confirmed`. Under + **fanout**, copy each finding's `confidence` and `severity_confirmed` + from the Haiku scorer (finder ≠ scorer) — do not self-score when a + scorer ran; worker severity labels may still seed the finding. + `files_reviewed` lists every file read in full for the active range (yours under collapse; union of coverage workers under fanout) — it is cross-checked against the staged manifest, so a short list reads as incomplete coverage. A @@ -1229,7 +1251,7 @@ EOF --model ${{ steps.route.outputs.model }} --fallback-model ${{ steps.route.outputs.fallback-model }} --max-turns 200 - --allowedTools "Read,Grep,Glob,Write,Task,Bash(gh pr view:*),Bash(gh issue view:*),Bash(git diff:*),Bash(git symbolic-ref:*),Bash(git merge-base:*),Bash(git remote show:*),Bash(git log:*),Bash(git show:*),Bash(cat:*),Bash(head:*),Bash(tail:*),Bash(ls:*),Bash(find:*),Bash(wc:*)" + --allowedTools "${{ steps.route.outputs.review-allowed-tools }}" --json-schema '{"type":"object","additionalProperties":false,"required":["verdict","confidence","merge_risk","intent","counts","review_event","comment_markdown"],"properties":{"verdict":{"type":"string","enum":["pass","fail"]},"confidence":{"type":"integer","minimum":0,"maximum":100},"merge_risk":{"type":"string","enum":["low","med","high"]},"intent":{"type":"string","enum":["aligned","partial","deviated","skipped"]},"counts":{"type":"object","additionalProperties":false,"required":["p0","p1","p2","p3"],"properties":{"p0":{"type":"integer","minimum":0},"p1":{"type":"integer","minimum":0},"p2":{"type":"integer","minimum":0},"p3":{"type":"integer","minimum":0}}},"review_event":{"type":"string","enum":["APPROVE","REQUEST_CHANGES"]},"comment_markdown":{"type":"string"},"findings":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["id","file","line","severity","summary","failure_scenario","reason","evidence","confidence"],"properties":{"id":{"type":"string"},"file":{"type":"string"},"line":{"type":"integer","minimum":0},"severity":{"type":"string","enum":["P0","P1","P2","P3"]},"summary":{"type":"string"},"failure_scenario":{"type":"string"},"reason":{"type":"string"},"evidence":{"type":"string"},"confidence":{"type":"integer","enum":[0,25,50,75,100]},"severity_confirmed":{"type":"string","enum":["P0","P1","P2","P3"]}}}},"files_reviewed":{"type":"array","items":{"type":"string"}},"tests_failing":{"type":"boolean"},"coverage_below_threshold_on_critical_paths":{"type":"boolean"},"no_tests_for_changed_logic":{"type":"boolean"},"test_execution":{"type":"string","enum":["passed","failed","skipped","not_run"]},"verification_evidence":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["claim","command","result"],"properties":{"claim":{"type":"string"},"command":{"type":"string"},"result":{"type":"string"}}}},"checklist":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["text","status"],"properties":{"text":{"type":"string"},"status":{"type":"string","enum":["verified","failed","unverifiable"]},"evidence":{"type":"string"}}}}}}' prompt: | This is a RETRY of an automated code review whose previous attempt @@ -1243,8 +1265,11 @@ EOF Populate the optional `findings` and `files_reviewed` fields of the structured output as well. `findings` is one entry per finding you - report, with your own confidence 0/25/50/75/100 and, where you would - reclassify it, `severity_confirmed`. `files_reviewed` lists every file + report. Under **collapse**, include your own confidence 0/25/50/75/100 + and, where you would reclassify it, `severity_confirmed`. Under + **fanout**, copy each finding's `confidence` and `severity_confirmed` + from the Haiku scorer (finder ≠ scorer) — do not self-score when a + scorer ran. `files_reviewed` lists every file read in full for the active range — it is cross-checked against the staged manifest, so a short list reads as incomplete coverage. A later non-gating step aggregates these deterministically and compares @@ -1567,7 +1592,7 @@ EOF # # Publish still consumes parent/collapsed-session structured_output (same # as today) for both osh-mode=collapse and fanout. Under fanout the Opus - # parent is prompted to gather worker artifacts into that SO; we do NOT + # parent is prompted to gather worker Task results into that SO; we do NOT # wire multi-role findings files into the live gate yet — avoiding a # half-broken aggregate path. This step still derives review-serial from # the SO and compares aggregate.js in shadow only. @@ -1580,12 +1605,11 @@ EOF # # This step runs the new deterministic aggregation next to the live path # and records whether the two agree. It reads the files the Derive step - # above produces from the schema-validated structured output (the model - # holds no Write grant on the collapse path for gate inputs — see that - # step's comment; fanout may Write worker JSON under `.ai-review/` for - # the parent to read), runs lib/aggregate.js, feeds the result through the - # SAME lib/recompute.js the real verdict uses, and compares. Any divergence - # is a warning in the log and a row in the job summary. + # above produces from the schema-validated structured output (Review has + # no Write grant on collapse or fanout — workers return via Task; Derive + # writes the shadow artifacts), runs lib/aggregate.js, feeds the result + # through the SAME lib/recompute.js the real verdict uses, and compares. + # Any divergence is a warning in the log and a row in the job summary. # # Deliberately non-gating: `continue-on-error`, no outputs consumed by any # later step, and Publish below is untouched. Deleting --json-schema and the diff --git a/ai-review/lib/roster.js b/ai-review/lib/roster.js index 3c09cbd..92853ba 100644 --- a/ai-review/lib/roster.js +++ b/ai-review/lib/roster.js @@ -818,7 +818,7 @@ function buildRoster({ // claude-haiku-4-5`), but not a gateway alias that ROUTES to Haiku under an // unrelated string (e.g. `gw-fast-1`) — `anthropic-base-url`'s own // description documents that such aliases exist. No live effect either way: - // nothing consumes the roster yet, so nothing reads `effort`. + // No live effect either way until a consumer reads `effort` from the roster. if (spec.effort && !/haiku/i.test(String(model ?? ""))) out.effort = spec.effort; return out; }; diff --git a/ai-review/lib/write-manifest.js b/ai-review/lib/write-manifest.js index eab765c..113b1d0 100644 --- a/ai-review/lib/write-manifest.js +++ b/ai-review/lib/write-manifest.js @@ -17,10 +17,8 @@ // exactly the byte count a reviewer reading the file will face. A deleted path // has no entry and contributes 0. // -// The roster is emitted on every run, including K=1, and nothing consumes it -// yet — the review stage is still serial. It is written now so the partition it -// asserts is exercised on real diffs, across all 7 consumers, before any model -// stage depends on it. +// The roster is load-bearing for Slice 3 OSH routing (K → collapse vs fanout). +// A failed build/write must fail the prep step — never silently skip. const fs = require("node:fs"); const path = require("node:path"); @@ -33,15 +31,13 @@ const DIR = ".ai-review"; * Write JSON to `p` atomically: temp file in the same directory, then rename. * * A bare `fs.writeFileSync(p, ...)` can leave a truncated file at `p` if it - * fails partway through (ENOSPC, a killed process). `writeRoster`'s catch - * already reconciles four signals on a write failure — return value, log - * line, `::warning::`, telemetry — but none of them touch the artifact on - * disk, so a truncated `assignments.json` could outlive a failure the rest of - * the system already reported as NOT EMITTED. `rename(2)` on the same - * filesystem is atomic: `p` either has the old content (nothing existed) or - * the complete new content, never a partial write, regardless of where the - * temp write failed. `writeFile`/`renameSync` are injectable for the same - * reason the rest of this module's I/O is. + * fails partway through (ENOSPC, a killed process). On a write failure + * `writeRoster` logs then rethrows (fail-closed); without an atomic rename a + * truncated `assignments.json` could still sit on disk after prep already + * failed. `rename(2)` on the same filesystem is atomic: `p` either has the + * old content (nothing existed) or the complete new content, never a partial + * write. `writeFile`/`renameSync` are injectable for the same reason the rest + * of this module's I/O is. */ function atomicWriteJson(p, obj, io) { const writeFile = (io && io.writeFile) || fs.writeFileSync; @@ -158,16 +154,11 @@ function main() { /** * Emit `.ai-review/assignments.json`. * - * Best-effort **for now, deliberately**: nothing consumes this file yet, so a - * defect in brand-new roster code must not fail a review on seven consumer - * repos that all track `@main`. The failure is named in the job log, never - * swallowed — and it is the same shadow-mode discipline `lib/aggregate.js` - * shipped under. - * - * When the review stage actually fans out, `assertPartition` becomes - * load-bearing and this guard must be removed: at that point an unassigned file - * is a file nobody reads, and continuing would be the fail-open the assertion - * exists to prevent. + * Fail-closed: Slice 3 routes the review model on `.k` and the review prompt + * consumes the role list. A build or write failure is logged (including + * scrapable `ai-review-roster` telemetry) then rethrown so the prep step exits + * non-zero. Atomic write is preserved — a failed rename must not leave a + * truncated assignments file. */ function writeRoster(manifest, sizes, io) { const readText = (io && io.readText) || ((p) => fs.readFileSync(p, "utf8")); @@ -176,12 +167,8 @@ function writeRoster(manifest, sizes, io) { let roster = null; try { - // Required inside the try, not at module scope: a load-time throw from - // roster.js — a syntax error surviving to production, or any future - // module-scope code that throws — would otherwise crash this require() - // before writeRoster ever runs, taking manifest.json and the whole prep - // step down with it. That is exactly the failure this function's own - // JSDoc promises cannot happen. + // Required inside the try so a load-time throw from roster.js is logged + // with the same FAILED / telemetry / rethrow path as a build failure. const { buildRoster, resolveImportEdges } = require("./roster.js"); const specifiers = collectSpecifiers(manifest.changed_files, sizes, readText); @@ -236,23 +223,19 @@ function writeRoster(manifest, sizes, io) { } log(`${rosterTelemetry(roster)}\n`); } catch (err) { - // `roster` may already be a built object here — buildRoster can succeed and - // writeJson can still throw. Null it out: the log line, the annotation and - // the telemetry all say NOT EMITTED, and the return value has to agree. - // Latent while main() is the only caller and ignores it; load-bearing once - // PR-D/2 reads this to decide whether to fan out. + // buildRoster can succeed and writeJson can still throw — null the local + // so telemetry says failed, then rethrow so prep fails closed. roster = null; // one(), like the two lines below it — a multi-line error message would // otherwise split this record across lines, the exact failure one()'s own // comment exists to prevent. - log(`roster: NOT EMITTED — ${one(err)}\n`); - // An annotation as well as a log line: a `run:` step's stdout is not - // surfaced anywhere a maintainer looks unless they open the job. + log(`roster: FAILED — ${one(err)}\n`); log( - `::warning::ai-review could not build the review roster: ${one(err)}. ` + - "Nothing consumes it yet, so the review is unaffected — but PR-D/2 will.\n", + `::error::ai-review could not build the review roster: ${one(err)}. ` + + "Prep fails closed — Slice 3 routes on assignments.json.\n", ); log(`${rosterTelemetry(null, err)}\n`); + throw err; } return roster; } @@ -273,9 +256,8 @@ function one(v) { */ function collectSpecifiers(changedFiles, sizes, readText) { // Lazy, matching writeRoster's require above — a load-time throw in - // roster.js must not crash this module before writeRoster's try/catch is - // even reached. Cheap: Node caches the module, so this is a second lookup, - // not a second load. + // roster.js is handled by writeRoster's fail-closed path (log + rethrow). + // Cheap: Node caches the module, so this is a second lookup, not a second load. const { extractImports } = require("./roster.js"); const out = Object.create(null); for (const p of Array.isArray(changedFiles) ? changedFiles : []) { @@ -313,15 +295,10 @@ function overBudget(roster) { /** * One scrapable line per run, in the `ai-review-metrics {json}` shape. * - * The case for shipping the roster before anything reads it rests entirely on - * exercising the partition across seven consumers first. A lone unstructured - * stdout line cannot carry that: the 682-job latency baseline was gathered by - * grepping `ai-review-metrics {...}` out of raw logs, and a roster failure needs - * to be countable the same way. A systematic `buildRoster` defect that produced - * nothing aggregatable would hold for as long as nobody happened to open a job. - * - * Failure is a record, not a blank — absence and cleanliness must never be the - * same byte pattern. + * Roster failures must be countable the same way as successes (the 682-job + * latency baseline was gathered by grepping `ai-review-metrics {...}` out of + * raw logs). Failure is a record, not a blank — absence and cleanliness must + * never be the same byte pattern. Prep then exits non-zero (fail-closed). */ function rosterTelemetry(roster, err) { const payload = roster diff --git a/ai-review/lib/write-manifest.test.js b/ai-review/lib/write-manifest.test.js index 8e8b42a..156c485 100644 --- a/ai-review/lib/write-manifest.test.js +++ b/ai-review/lib/write-manifest.test.js @@ -21,7 +21,7 @@ const { // filesystem, and its header called itself a "thin I/O wrapper" as the reason it // had no sibling test. `writeRoster` stretched that past the point where the // claim held: the size gate, the specifiers map, the byte fallback and the -// best-effort catch are all decisions, not plumbing. The I/O is injected so they +// fail-closed rethrow are all decisions, not plumbing. The I/O is injected so they // can be pinned without a temp directory or a chdir. // // `main` is the one exception, deliberately: it is real fs.readFileSync / @@ -101,8 +101,8 @@ test("collectSpecifiers: binary is skipped by extension, not by hoping read thro // // The success path is exercised end to end in ai-review/action.yml against a // real repo (see the commit history); these pin the wiring around it — the I/O -// is injected so the best-effort catch is reachable without a temp directory or -// a chdir. +// is injected so the fail-closed rethrow path is reachable without a temp +// directory or a chdir. test("writeRoster: writes assignments.json and logs a summary on success", () => { const writes = []; @@ -124,10 +124,10 @@ test("writeRoster: writes assignments.json and logs a summary on success", () => assert.equal(writes[0][1].k, roster.k); assert.ok(logs.some((l) => l.startsWith("roster: K="))); assert.ok(logs.some((l) => l.startsWith("ai-review-roster {"))); - assert.ok(!logs.some((l) => l.includes("NOT EMITTED"))); + assert.ok(!logs.some((l) => l.includes("FAILED"))); }); -test("writeRoster: a buildRoster throw is caught, logged, and never written", () => { +test("writeRoster: a buildRoster throw is logged then rethrown (fail-closed)", () => { // Duplicate changed_files entries make buildRoster's own assertPartition // throw ("assigned twice") — a real failure mode, not a synthetic one, since // a caller could hand this a manifest with a duplicated path. @@ -140,19 +140,22 @@ test("writeRoster: a buildRoster throw is caught, logged, and never written", () has_logic_change: true, modifies_reviewer_guidance: false, }; - const result = writeRoster(manifest, { "src/a.ts": 10 }, { - readText: () => "", - writeJson: (p, obj) => writes.push([p, obj]), - log: (line) => logs.push(line), - }); - assert.equal(result, null); + assert.throws( + () => + writeRoster(manifest, { "src/a.ts": 10 }, { + readText: () => "", + writeJson: (p, obj) => writes.push([p, obj]), + log: (line) => logs.push(line), + }), + /assigned twice/, + ); assert.equal(writes.length, 0, "a failed build must never reach disk"); - assert.ok(logs.some((l) => l.startsWith("roster: NOT EMITTED"))); - assert.ok(logs.some((l) => l.startsWith("::warning::"))); + assert.ok(logs.some((l) => l.startsWith("roster: FAILED"))); + assert.ok(logs.some((l) => l.startsWith("::error::"))); assert.ok(logs.some((l) => l.startsWith("ai-review-roster {") && l.includes('"status":"failed"'))); }); -test("writeRoster: a multi-line error message doesn't split the NOT EMITTED log line", () => { +test("writeRoster: a multi-line error message doesn't split the FAILED log line", () => { // The other two lines in this catch already went through one(); this one // didn't, so a multi-line error would break a scraper reading the log // line-by-line — the exact failure one()'s own comment describes. @@ -166,24 +169,24 @@ test("writeRoster: a multi-line error message doesn't split the NOT EMITTED log has_logic_change: true, modifies_reviewer_guidance: false, }; - writeRoster(manifest, { "src/a.ts": 10 }, { - readText: () => "", - writeJson: () => { - throw new Error("line one\nline two\nline three"); - }, - log: (line) => logs.push(line), - }); - const notEmitted = logs.find((l) => l.startsWith("roster: NOT EMITTED")); - assert.ok(notEmitted, "no NOT EMITTED line found"); - assert.equal(notEmitted.trimEnd().includes("\n"), false, `split across lines: ${JSON.stringify(notEmitted)}`); + assert.throws(() => + writeRoster(manifest, { "src/a.ts": 10 }, { + readText: () => "", + writeJson: () => { + throw new Error("line one\nline two\nline three"); + }, + log: (line) => logs.push(line), + }), + ); + const failed = logs.find((l) => l.startsWith("roster: FAILED")); + assert.ok(failed, "no FAILED line found"); + assert.equal(failed.trimEnd().includes("\n"), false, `split across lines: ${JSON.stringify(failed)}`); }); -test("writeRoster: a throw from writeJson (build succeeded, the write didn't) returns null too", () => { +test("writeRoster: a throw from writeJson (build succeeded, the write didn't) fails closed", () => { // buildRoster can succeed and writeJson can still throw (disk full, bad path). - // The catch logs NOT EMITTED / a ::warning:: / status:"failed" telemetry in - // every case — the return value has to agree, or a caller reading it back - // would get a fully-built roster object for a run the log just called a - // failure. + // The catch logs FAILED / a ::error:: / status:"failed" telemetry, then + // rethrows — prep must not continue without assignments.json. const logs = []; const manifest = { changed_files: ["src/a.ts"], @@ -192,15 +195,19 @@ test("writeRoster: a throw from writeJson (build succeeded, the write didn't) re has_logic_change: true, modifies_reviewer_guidance: false, }; - const result = writeRoster(manifest, { "src/a.ts": 10 }, { - readText: () => "", - writeJson: () => { - throw new Error("ENOSPC"); - }, - log: (line) => logs.push(line), - }); - assert.equal(result, null, "a write failure must not return a built roster"); - assert.ok(logs.some((l) => l.startsWith("roster: NOT EMITTED") && l.includes("ENOSPC"))); + assert.throws( + () => + writeRoster(manifest, { "src/a.ts": 10 }, { + readText: () => "", + writeJson: () => { + throw new Error("ENOSPC"); + }, + log: (line) => logs.push(line), + }), + /ENOSPC/, + ); + assert.ok(logs.some((l) => l.startsWith("roster: FAILED") && l.includes("ENOSPC"))); + assert.ok(logs.some((l) => l.startsWith("::error::"))); }); // The comment above the `why` ternary exists because a real miscategorisation @@ -271,16 +278,15 @@ test("writeRoster: a readText throw for one file degrades to a weaker cluster, n log: (line) => logs.push(line), }); assert.notEqual(result, null); - assert.ok(!logs.some((l) => l.includes("NOT EMITTED"))); + assert.ok(!logs.some((l) => l.includes("FAILED"))); }); // --- atomicWriteJson ---------------------------------------------------------- // -// writeRoster's catch reconciles four signals on a write failure — return -// value, log line, ::warning::, telemetry — but a bare fs.writeFileSync could -// still leave a truncated assignments.json on disk that the other four agree -// doesn't exist. Real files on a real temp directory, not mocks: the property -// under test is what actually lands on disk after a failure. +// On a write failure writeRoster logs then rethrows (fail-closed). A bare +// fs.writeFileSync could still leave a truncated assignments.json on disk +// after prep already failed. Real files on a real temp directory, not mocks: +// the property under test is what actually lands on disk after a failure. test("atomicWriteJson: writes the JSON, then removes the temp file", () => { const dir = fs.mkdtempSync(path.join(os.tmpdir(), "atomic-write-")); diff --git a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md index 58c6a99..43a448e 100644 --- a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md +++ b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md @@ -126,7 +126,8 @@ flowchart TD | **H**aiku (helpers) | History / mechanical gathers + **independent confidence scoring** | Cheap collection and scoring that must not be the same model that found the issue (parallel design §3). | Only the **Opus parent** emits `--json-schema` structured output for Publish. Workers return freeform -or JSON files under `.ai-review/`; the parent aggregates into the schema contract. Aggregation +or JSON **via Task results** (no unscopeable Write on the review allowlist); the parent aggregates into +the schema contract. Aggregation (`lib/aggregate.js` when landed) and `recompute.js` remain deterministic consumers — model-reported counts are never trusted as gate inputs. @@ -377,15 +378,18 @@ Subagents use the `model` field on each role in `assignments.json` (same strings - **Haiku** for `history` and for `scorer` (independent confidence / `severity_confirmed`). - **Intent** (`kind: frame`): Opus owns it (parent may run it itself or dispatch an Opus-tier subagent). Keep intent isolated from coverage analysis. -3. Workers may write freeform or JSON under `.ai-review/` (e.g. `findings/.json`, - `scores.json`). They must **not** emit the Publish `--json-schema` blob. +3. Workers return freeform or JSON **via Task tool results** (Review allowlist + has Task on fanout only — **no Write**; Claude Code cannot scope Write to + `.ai-review/`). They must **not** emit the Publish `--json-schema` blob. 4. Opus **must not** exhaustively re-read every file workers already covered unless conflict resolution or a spot-check needs it. Must-read-all for the active range is satisfied by the coverage workers' union of `assigned_files` (plus tracer / neighbor expansion rules). 5. Parent aggregates worker outputs into the schema contract (`comment_markdown`, `findings`, - `counts`, `intent`, etc.). Publish continues to consume **parent structured output** (same - fail-closed gate as today); multi-file `aggregate.js` remains shadow/non-gating until a - follow-up wires it live. + `counts`, `intent`, etc.). On fan-out, each finding's `confidence` / + `severity_confirmed` must come from the Haiku **scorer** (finder ≠ scorer) — + the parent must not self-score when a scorer ran. Publish continues to consume + **parent structured output** (same fail-closed gate as today); multi-file + `aggregate.js` remains shadow/non-gating until a follow-up wires it live. ### Collapse path (`assignments.json` `.k` ≤ 1, including empty-diff `k: 0`) From b7286d5f4016a9e76768b883b5a8a777b7139dc9 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 15:49:36 +0300 Subject: [PATCH 09/16] docs: document delta review, CI test-plan findings, OSH roles --- README.md | 3 + ai-review/README.md | 4 +- ...-alignment-test-execution-and-writeback.md | 4 +- ...6-test-plan-ci-findings-and-osh-routing.md | 79 +++++++++++++++++++ docs/consumer-integration.md | 26 +++--- docs/plan.md | 4 +- ...-17-ai-review-osh-delta-testplan-design.md | 2 +- 7 files changed, 105 insertions(+), 17 deletions(-) create mode 100644 docs/adr/0006-test-plan-ci-findings-and-osh-routing.md diff --git a/README.md b/README.md index 3b5f610..257b717 100644 --- a/README.md +++ b/README.md @@ -71,6 +71,9 @@ owning organization. identity model, the composite-actions-vs-reusable-workflow choice, the parameterization surface, the public/generalized posture, and the supply-chain and least-privilege posture this repo follows. +- [`docs/adr/0006-test-plan-ci-findings-and-osh-routing.md`](docs/adr/0006-test-plan-ci-findings-and-osh-routing.md) + — Test Plan ↔ CI findings (checklist ticks retired), delta reviews, and + roster-K Sonnet collapse / Opus fan-out routing. - [`docs/plan.md`](docs/plan.md) — the full build-out plan this repo is being implemented against, including the phase/task breakdown and the locked design decisions behind it. diff --git a/ai-review/README.md b/ai-review/README.md index 067bc52..3c8f725 100644 --- a/ai-review/README.md +++ b/ai-review/README.md @@ -249,7 +249,9 @@ items verified in the PR body. Prep inventories: The model maps items to CI coverage; uncovered or weakly covered items become normal `findings[]` with severity P0–P3 via the rubric (then `recompute.js`). Checklist tick write-back is retired (`update-pr-body` is a no-op for that -path). `test_execution` stays `"skipped"` — no test runners in the allowlist. +path); see +[`docs/adr/0006-test-plan-ci-findings-and-osh-routing.md`](../docs/adr/0006-test-plan-ci-findings-and-osh-routing.md). +`test_execution` stays `"skipped"` — no test runners in the allowlist. ## OSH routing (roster K) diff --git a/docs/adr/0003-intent-alignment-test-execution-and-writeback.md b/docs/adr/0003-intent-alignment-test-execution-and-writeback.md index d583f93..90d5293 100644 --- a/docs/adr/0003-intent-alignment-test-execution-and-writeback.md +++ b/docs/adr/0003-intent-alignment-test-execution-and-writeback.md @@ -1,10 +1,12 @@ # ADR 0003 — Linked-issue intent, real test execution, and PR/issue write-back -- **Status:** Accepted +- **Status:** Accepted (partially superseded by ADR 0006 for `ai-review` + checklist tick / status-block write-back and size-based Sonnet/Opus routing) - **Date:** 2026-07-21 - **Relates to:** ADR 0001 (gate/identity/injection posture) and ADR 0002 (`ai-qa` agentic review). Those decisions still hold; this ADR layers new capability onto both actions without changing the gate contract. + **Superseded in part by** ADR 0006 (Test Plan ↔ CI findings; roster-K OSH). ## Context diff --git a/docs/adr/0006-test-plan-ci-findings-and-osh-routing.md b/docs/adr/0006-test-plan-ci-findings-and-osh-routing.md new file mode 100644 index 0000000..ceb2b70 --- /dev/null +++ b/docs/adr/0006-test-plan-ci-findings-and-osh-routing.md @@ -0,0 +1,79 @@ +# ADR 0006 — Test Plan ↔ CI findings, delta reviews, and roster-K OSH routing + +- **Status:** Accepted +- **Date:** 2026-08-17 +- **Relates to:** ADR 0001 (gate contract — unchanged), ADR 0003 (partially + supersedes §4 `ai-review` checklist write-back and §5 size-based Opus routing), + ADR 0005 (roster / aggregation prerequisites). Implements + `docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md`. + +## Context + +ADR 0003 taught `ai-review` to tick PR-description checklist boxes it marked +verified and to prefer Opus except on trivially tiny diffs +(`sonnet-files-threshold` / `sonnet-churn-threshold`). Both choices aged poorly: + +1. **Checklist ticks were dishonest.** The review does not execute the Test + Plan. Ticking boxes implied human-visible “verified” when the signal was + model judgment, not CI. Consumers also had to avoid `pull_request: edited` + to prevent write-back loops. +2. **Size-based Sonnet/Opus routing fought the roster.** Parallel OSH work + already packs coverage into K bins (`assignments.json`). Diff file/churn + thresholds selected the wrong axis once an Opus parent fans out Sonnet + workers for K>1 and collapses to a single Sonnet session at K≤1. + +Design slices on this branch also add **delta reviews** (re-runs review only +commits after the last published `` meta unless forced full). + +## Decision + +### 1. Retire `ai-review` checklist tick / status-block write-back + +Publish no longer ticks `- [ ]` → `- [x]` in the PR body and no longer maintains +an `` block. Input `update-pr-body` stays accepted for +compatibility and is a **no-op** on that path. + +`ai-qa`'s managed PR-body / linked-issue write-back is unchanged. + +### 2. Test Plan gaps become ordinary findings vs CI + +Prep stages `.ai-review/test-plan-items.json` (Test Plan section and/or +checkboxes) and `.ai-review/ci-checks.json` (check runs for the PR HEAD). The +review maps items to CI coverage; uncovered or weakly covered items are normal +`findings[]` (P0–P3 via the rubric and `recompute.js`). The schema `checklist` +field stays empty. `test_execution` remains `"skipped"` — no test runners in the +allowlist (see also the later retirement of in-review test execution documented +in `ai-review/README.md`). + +### 3. Route the review session by roster K, not diff size + +| K | Mode | Review session | +| --- | --- | --- | +| ≤1 (incl. empty-diff `0`) | `collapse` | Single Sonnet + `--json-schema` | +| >1 (max 4) | `fanout` | Opus parent + native Sonnet/Haiku subagents; only Opus emits structured output | + +`sonnet-files-threshold` / `sonnet-churn-threshold` remain accepted but are +**deprecated for model routing**. + +### 4. Delta reviews with an explicit full-review escape hatch + +On re-runs, prep prefers the range after the prior published review’s +`head_sha` when ancestry and merge-base allow. Full merge-base…HEAD is used on +first run, missing/inconclusive meta, force-push, base change, or +`force-full-review: true`. + +## Consequences + +- **Gate contract unchanged.** The four job outputs and deterministic recompute + still decide pass/fail; Publish still posts a native review + labels. +- **Honest Test Plan signal.** Gaps show up as findings against CI, not as + checked boxes in the description. +- **No `edited`-loop caveat for `ai-review`.** Body write-back from this action + is gone; callers no longer need that warning for checklist ticks (they may + still avoid `edited` for other reasons). +- **Consumers need no required input changes.** Deprecated inputs keep working + as no-ops / ignored routing knobs. Optional `force-full-review` opts out of + delta. +- **ADR 0003** remains the historical record for linked-issue intent resolution + and the original write-back design; treat §4’s `ai-review` tick path and §5’s + size routing as superseded by this ADR. diff --git a/docs/consumer-integration.md b/docs/consumer-integration.md index 62d93d8..d261d37 100644 --- a/docs/consumer-integration.md +++ b/docs/consumer-integration.md @@ -51,7 +51,7 @@ lives inside your own CI's job graph and shares its concurrency group: permissions: contents: read - pull-requests: write # post the review; tick PR-body checkboxes + pull-requests: write # post the native PR review + labels issues: read # resolve + read the issues this PR closes concurrency: @@ -63,10 +63,10 @@ jobs: runs-on: ${{ vars.RUNNER_LABEL || 'ubuntu-latest' }} # Wall-clock backstop. Composite-action steps can't set timeout-minutes, # so this job-level cap is the only bound on a hung Anthropic gateway / - # plugin-marketplace load. Give it headroom for an Opus review that reads - # full files and runs your tests, but keep it tight enough to fail fast on - # a stall (the internal Haiku context stage is best-effort and won't sink - # the review on its own). + # plugin-marketplace load. Give it headroom for a roster-K review (Sonnet + # collapse or Opus fan-out) that reads full files, but keep it tight enough + # to fail fast on a stall (the Haiku context stage is best-effort and won't + # sink the review on its own). timeout-minutes: 25 outputs: verdict: ${{ steps.review.outputs.verdict }} @@ -83,7 +83,7 @@ jobs: anthropic-base-url: ${{ vars.ANTHROPIC_BASE_URL }} app-id: ${{ vars.APP_ID }} private-key: ${{ secrets.APP_PRIVATE_KEY }} - # test-command: npm test # optional; empty ⇒ auto-detect / skip + # force-full-review: true # optional; skip delta and review merge-base…HEAD review-gate: runs-on: ubuntu-latest @@ -107,12 +107,14 @@ jobs: ``` This version of `ai-review` also **reads the issues your PR closes** (to -judge intent against their acceptance criteria — hence `issues: read`) and, -when `update-pr-body` is left on, **ticks verified checklist boxes** in the -PR description and maintains a managed `` block. It -never unchecks a human's box. Keep your `on: pull_request` trigger at its -default event types — do **not** add `edited`, or the body edits it makes -would re-trigger the review in a loop. +judge intent against their acceptance criteria — hence `issues: read`). It +maps the PR **Test Plan** / checklists to CI coverage and turns gaps into +normal findings; it does **not** tick checklist boxes or write an +`` block (`update-pr-body` is accepted but a no-op +for that path — see ADR 0006). On re-runs it prefers a **delta** range since +the last published `` unless `force-full-review` is set. +Roster **K** selects Sonnet collapse (K≤1) vs Opus fan-out (K>1); size-based +`sonnet-*-threshold` inputs are deprecated for routing. Then, in branch protection, require the `review-gate` job's status (and your terminal CI job, e.g. `build`) as required status checks. If this repo diff --git a/docs/plan.md b/docs/plan.md index e215c27..7a3c659 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -29,7 +29,7 @@ Today `ai-review`/`ai-qa` are autopilot **skills** a developer runs in a local C - **Public & generalized:** the repo is **public** and org-agnostic. No action ships an eduly-specific default — eduly values (App ID/key, runner label, health URL, test command, staging URL) are **caller-supplied**; the action's own defaults are neutral (`runner-label: ubuntu-latest`, no health-url/test-command default). READMEs, ADR 0001, and `docs/consumer-integration.md` are written generically with **eduly as one example consumer**, not the subject. A `LICENSE` is a **deferred, blocking decision**: a public repo with no license is "all rights reserved," so external reuse cannot begin until a license is chosen (see D12) — internal EdulyCom consumption is unaffected. - **Process:** all changes via PR (no direct push to protected branches); **merge commits, not squash**. TypeScript/no-`any` for any committed scripts; prefer inline `github-script`. Docs land in the same PR as their change. - **Secrets & variables (org-level, human-supplied):** every secret/var the actions consume is set **at the EdulyCom org level** (`gh secret set --org EdulyCom …` / `gh variable set --org EdulyCom …`, repo-visibility scoped to the consuming repos incl. `github-actions` itself so its `selftest.yml` can run). The **implementer never invents secret values**: at each point one is needed (Phase 1 selftest, Phase 4 eduly adoption) the implementer **pauses and asks the human for the value**, then sets it at org level. Org-level secrets: `ANTHROPIC_AUTH_TOKEN` (eduly gateway bearer), `MTM_BOT_APP_PRIVATE_KEY`. Org-level vars: `ANTHROPIC_BASE_URL` (eduly gateway URL), `MTM_BOT_APP_ID`, `RUNNER_LABEL`, `AI_REVIEW_LABEL` (+ model-ID overrides if a gateway aliases them). (`GITHUB_TOKEN` is auto-provided per repo; not set here.) -- **Model IDs** (locked in the action, not repo-var overridable): context `claude/claude-haiku-4-5-20251001`; review routes tiny diffs to `claude/claude-sonnet-5` and larger diffs to `claude/claude-opus-5`. Cascade on overload: Cursor (`claude/cursor/...`) then pinned OpenCode free models then `auto/best-free`. Tiny = ≤`sonnet-files-threshold` (default 25) files AND ≤`sonnet-churn-threshold` (default 800) changed lines. ai-qa uses locked Sonnet with the same Cursor→free fallback list. See `docs/superpowers/plans/2026-08-17-claude-cursor-free-model-cascade.md` / #51. +- **Model IDs** (locked in the action, not repo-var overridable): context `claude/claude-haiku-4-5-20251001`; **review routes by roster K** — K≤1 → `claude/claude-sonnet-5` collapse; K>1 → `claude/claude-opus-5` parent with native Sonnet/Haiku fan-out (see ADR 0006 / `ai-review/README.md`). Cascade on overload: Cursor (`claude/cursor/...`) then pinned OpenCode free models then `auto/best-free`. Inputs `sonnet-files-threshold` / `sonnet-churn-threshold` remain accepted but are **deprecated for model routing**. ai-qa uses locked Sonnet with the same Cursor→free fallback list. See `docs/superpowers/plans/2026-08-17-claude-cursor-free-model-cascade.md` / #51 and `docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md`. - **Anthropic endpoint — caller-supplied, defaults to Anthropic:** `anthropic-base-url` is an **optional caller input**; when omitted the action leaves `ANTHROPIC_BASE_URL` unset so `claude-code-action` uses the **standard Anthropic endpoint** (no gateway baked in — keeps the action generic per D12). When the caller passes it (eduly → its **custom gateway**), the value is plumbed to **every** `claude-code-action` invocation — Haiku context, Sonnet/Opus review, ai-qa triage — together with the caller's auth: either `anthropic-api-key` (→ `ANTHROPIC_API_KEY`/`x-api-key`) or `anthropic-auth-token` (→ `ANTHROPIC_AUTH_TOKEN` bearer). Model IDs are locked in the action with a Claude→Cursor→free `--fallback-model` cascade rather than caller overrides. ## Locked decisions @@ -41,7 +41,7 @@ Today `ai-review`/`ai-qa` are autopilot **skills** a developer runs in a local C - **D6** Structure → **composite actions + thin caller**. - **D7** Rubric → **vendored markdown** `ai-review/rubric.md` (single source of truth after skill deletion). `/requesting-code-review` (superpowers) still installed live in CI. - **D8** Versioning → **floating `@main`** + CODEOWNERS. -- **D9** Opus → **yes**; large diffs use `claude-opus-4-8`. +- **D9** Opus → **yes**; originally "large diffs use Opus via size thresholds". **Superseded for routing:** roster K selects Sonnet collapse vs Opus fan-out (ADR 0006); size thresholds remain accepted but ignored for routing. - **D10** ai-qa → **post-merge non-blocking signal** (comment + labels) on push to default branch. - **D11** Gate mechanism → **CI-gating via ordinary job status** (the action exposes a `verdict` job output; a `review-gate` job fails red unless `verdict == 'pass'`; heavy lanes `needs: [review-gate]`), **not** an App-authored identity-pinned Check Run. No #3414 (the job runs on `pull_request`, so its status enters the PR rollup). Human approval + the gated required tests remain the merge gate. (Supersedes the earlier "advisory, CI runs regardless" framing: the review DOES gate CI — for cost savings — but via a job dependency, not a label cascade or an identity-pinned check.) - **D12** Publish/distribution → **public monorepo, path-referenced** (keep D6's one-dir-per-action layout; consumed as `EdulyCom/github-actions/@`; **no** GitHub Marketplace listing — leave that as a documented future extraction). Versioning stays **floating `@main` + CODEOWNERS** (D8; no semver tags) **for the first-party composites**; third-party actions inside them are SHA-pinned (D13). All defaults generic (eduly values in the caller). `LICENSE` **deferred**: publish the repo, but external reuse waits on the license choice; document this in the README and ADR 0001. diff --git a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md index 43a448e..7ce0f7f 100644 --- a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md +++ b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md @@ -1,6 +1,6 @@ # `ai-review` OSH + delta + Test-Plan/CI — design -**Status:** approved shape; Slices 1–2 landed; Slice 3 (OSH fan-out) landing on this branch. +**Status:** Slices 1–3 landed on branch `feat/ai-review-osh-delta-testplan` (delta, Test Plan ↔ CI, OSH fan-out); consumer docs + ADR 0006 record the checklist-tick retirement. **Extends:** [`2026-08-07-ai-review-parallel-review-design.md`](./2026-08-07-ai-review-parallel-review-design.md) (parallel OSH / coverage–coherence / rubric scoring). This document does **not** supersede that design’s §1 binding constraints or its scoring/`recompute.js` contract. From 4de764c95323c0f7c74147663f2e12b194e4f970 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 16:03:01 +0300 Subject: [PATCH 10/16] fix(ai-review): stop inlining osh-policy via YAML heredocs Column-0 heredoc bodies inside `run: |` terminate the block scalar, so GitHub Actions failed to load action.yml (line 532: expected ':'). Copy checked-in fanout/collapse policy files instead. --- ai-review/action.yml | 75 +++----------------------------- ai-review/osh-policy-collapse.md | 13 ++++++ ai-review/osh-policy-fanout.md | 53 ++++++++++++++++++++++ 3 files changed, 71 insertions(+), 70 deletions(-) create mode 100644 ai-review/osh-policy-collapse.md create mode 100644 ai-review/osh-policy-fanout.md diff --git a/ai-review/action.yml b/ai-review/action.yml index 40390a6..06ad631 100644 --- a/ai-review/action.yml +++ b/ai-review/action.yml @@ -414,6 +414,7 @@ runs: # sonnet-*-threshold inputs are deprecated for routing (kept unused). MANIFEST_CLI: ${{ github.action_path }}/lib/write-manifest.js DELTA_CLI: ${{ github.action_path }}/lib/write-delta.js + ACTION_PATH: ${{ github.action_path }} GH_TOKEN: ${{ steps.identity.outputs.author-token }} AUTHOR_LOGIN: ${{ steps.identity.outputs.author-login }} FORCE_FULL_REVIEW: ${{ inputs.force-full-review }} @@ -524,78 +525,12 @@ runs: # Policy file the review/retry prompts must obey (keeps one shared # prompt body with mode-specific orchestration rules). + # Checked-in files — do NOT inline via YAML heredoc: column-0 heredoc + # bodies terminate the `run: |` block (CI: "could not find expected ':'"). if [ "${OSH_MODE}" = "fanout" ]; then - cat > .ai-review/osh-policy.md <<'EOF' -# OSH fan-out policy (K>1) - -You are the **Opus parent** for this review. Only YOU emit the required -`--json-schema` structured output for Publish. Do not ask subagents to emit -that schema blob. - -## Roster - -Read `.ai-review/assignments.json` first. Honor its partition: do not invent -a second file split. Cap is K≤4 coverage reviewers. - -Locked model IDs (match `roles[].model` when spawning): - -- Opus parent / intent: `claude/claude-opus-5` -- Sonnet coverage + tracer: `claude/claude-sonnet-5` -- Haiku history + scorer: `claude/claude-haiku-4-5-20251001` - -## Subagents (native Claude Code Task tool) - -Spawn concurrent Task subagents (no Write tool — return results in the Task -response text/JSON; you aggregate into structured output): - -1. **Sonnet** — one per `reviewer-*` (`kind: coverage`): read 100% of every - `assigned_files` path with `Read`; run per-file rubric angles; propose - findings with P0–P3 severity (finder labels only — do not self-score). -2. **Sonnet** — `tracer` (`kind: coherence`): zero assigned files; follow - `symbol_manifest` / `split_clusters` for cross-file breakage (Angle C). -3. **Haiku** — `history` (`kind: perspective`): git blame / prior comments / - code-comment angles; cheap gathers, not full-file exhaustiveness. -4. **Haiku** — `scorer` (`kind: scoring`): independent confidence - 0/25/50/75/100 and `severity_confirmed` for every finding raised by - finders. **Finder ≠ scorer** — you must copy these into the SO - `findings[].confidence` / `severity_confirmed` fields; do not invent - your own scores when the scorer ran. -5. **Intent** (`kind: frame`): you own Angle H (or dispatch an Opus-tier - subagent). Frame from linked issues + PR body **before** treating coverage - findings as settled. - -## What you must not re-read - -Do **not** exhaustively re-read every file workers already covered unless -conflict resolution or a spot-check needs it. Must-read-all for the active -range is satisfied when coverage workers' `assigned_files` union equals -`changed_files` (plus targeted neighbor reads when prior findings or imports -require it). - -## Your job after workers return - -Resolve conflicts, prioritize, own intent, map Test Plan↔CI gaps to findings, -apply delta prior-review carry-forward rules, then emit the single structured -output. For each finding in SO: take `confidence` and `severity_confirmed` -from the Haiku scorer; severity labels may originate from workers. Publish -gates on your SO only. -EOF + cp "${ACTION_PATH}/osh-policy-fanout.md" .ai-review/osh-policy.md else - cat > .ai-review/osh-policy.md <<'EOF' -# OSH collapse policy (K≤1) - -You are a **single Sonnet** reviewer session. Emit `--json-schema` structured -output directly. - -- Do **not** spawn Task/Agent subagents for coverage / scorer / history fan-out. -- Do **not** expect an Opus parent or an independent Haiku scorer on this path. -- Read the COMPLETE contents of every file in `manifest.json` `changed_files` - with `Read` — never sample, truncate, or reason from diff hunks alone. -- Perform the full rubric review yourself (all angles that apply), including - intent, Test Plan↔CI findings, and delta carry-forward when present. -- Self-report finding `confidence` 0/25/50/75/100 and optional - `severity_confirmed` (no independent scorer on this path). -EOF + cp "${ACTION_PATH}/osh-policy-collapse.md" .ai-review/osh-policy.md fi { diff --git a/ai-review/osh-policy-collapse.md b/ai-review/osh-policy-collapse.md new file mode 100644 index 0000000..f76ee40 --- /dev/null +++ b/ai-review/osh-policy-collapse.md @@ -0,0 +1,13 @@ +# OSH collapse policy (K≤1) + +You are a **single Sonnet** reviewer session. Emit `--json-schema` structured +output directly. + +- Do **not** spawn Task/Agent subagents for coverage / scorer / history fan-out. +- Do **not** expect an Opus parent or an independent Haiku scorer on this path. +- Read the COMPLETE contents of every file in `manifest.json` `changed_files` + with `Read` — never sample, truncate, or reason from diff hunks alone. +- Perform the full rubric review yourself (all angles that apply), including + intent, Test Plan↔CI findings, and delta carry-forward when present. +- Self-report finding `confidence` 0/25/50/75/100 and optional + `severity_confirmed` (no independent scorer on this path). diff --git a/ai-review/osh-policy-fanout.md b/ai-review/osh-policy-fanout.md new file mode 100644 index 0000000..b8847f4 --- /dev/null +++ b/ai-review/osh-policy-fanout.md @@ -0,0 +1,53 @@ +# OSH fan-out policy (K>1) + +You are the **Opus parent** for this review. Only YOU emit the required +`--json-schema` structured output for Publish. Do not ask subagents to emit +that schema blob. + +## Roster + +Read `.ai-review/assignments.json` first. Honor its partition: do not invent +a second file split. Cap is K≤4 coverage reviewers. + +Locked model IDs (match `roles[].model` when spawning): + +- Opus parent / intent: `claude/claude-opus-5` +- Sonnet coverage + tracer: `claude/claude-sonnet-5` +- Haiku history + scorer: `claude/claude-haiku-4-5-20251001` + +## Subagents (native Claude Code Task tool) + +Spawn concurrent Task subagents (no Write tool — return results in the Task +response text/JSON; you aggregate into structured output): + +1. **Sonnet** — one per `reviewer-*` (`kind: coverage`): read 100% of every + `assigned_files` path with `Read`; run per-file rubric angles; propose + findings with P0–P3 severity (finder labels only — do not self-score). +2. **Sonnet** — `tracer` (`kind: coherence`): zero assigned files; follow + `symbol_manifest` / `split_clusters` for cross-file breakage (Angle C). +3. **Haiku** — `history` (`kind: perspective`): git blame / prior comments / + code-comment angles; cheap gathers, not full-file exhaustiveness. +4. **Haiku** — `scorer` (`kind: scoring`): independent confidence + 0/25/50/75/100 and `severity_confirmed` for every finding raised by + finders. **Finder ≠ scorer** — you must copy these into the SO + `findings[].confidence` / `severity_confirmed` fields; do not invent + your own scores when the scorer ran. +5. **Intent** (`kind: frame`): you own Angle H (or dispatch an Opus-tier + subagent). Frame from linked issues + PR body **before** treating coverage + findings as settled. + +## What you must not re-read + +Do **not** exhaustively re-read every file workers already covered unless +conflict resolution or a spot-check needs it. Must-read-all for the active +range is satisfied when coverage workers' `assigned_files` union equals +`changed_files` (plus targeted neighbor reads when prior findings or imports +require it). + +## Your job after workers return + +Resolve conflicts, prioritize, own intent, map Test Plan↔CI gaps to findings, +apply delta prior-review carry-forward rules, then emit the single structured +output. For each finding in SO: take `confidence` and `severity_confirmed` +from the Haiku scorer; severity labels may originate from workers. Publish +gates on your SO only. From ef2d4b338527179b35b13fb558a0577e621938f4 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 16:39:33 +0300 Subject: [PATCH 11/16] fix(ai-review): Sonnet Task agents under Opus; drop Model footer Haiku Task children inherit Opus adaptive thinking and 400 on this gateway. Pre-register osh-* Sonnet --agents and spawn them in one wave. Stop printing Model: on published review bodies. --- ai-review/README.md | 9 ++- ai-review/action.yml | 36 +++++++++--- ai-review/lib/publish.js | 12 ++-- ai-review/lib/publish.test.js | 13 +++-- ai-review/lib/write-manifest.js | 8 ++- ai-review/osh-agents.json | 26 +++++++++ ai-review/osh-policy-fanout.md | 58 ++++++++++--------- ...-17-ai-review-osh-delta-testplan-design.md | 28 ++++++--- 8 files changed, 132 insertions(+), 58 deletions(-) create mode 100644 ai-review/osh-agents.json diff --git a/ai-review/README.md b/ai-review/README.md index 3c8f725..2f37a90 100644 --- a/ai-review/README.md +++ b/ai-review/README.md @@ -74,8 +74,11 @@ injection-safety rule. The same step routes by roster **K** from `.ai-review/assignments.json` (not file/churn size): **K≤1** → locked Sonnet (`claude/claude-sonnet-5`) collapse — one session emits `--json-schema` directly; **K>1** → locked - Opus (`claude/claude-opus-5`) parent that fans out native Sonnet/Haiku - subagents per the roster (only the parent emits structured output). + Opus (`claude/claude-opus-5`) parent that fans out native Sonnet Task + subagents (`osh-coverage` / `osh-tracer` / `osh-history` / `osh-scorer` + via `--agents`; only the parent emits structured output). Haiku is not + used as a Task child of Opus (inherits adaptive thinking → gateway 400); + the optional context stage still uses Haiku as a top-level session. Inputs `sonnet-files-threshold` / `sonnet-churn-threshold` remain accepted for backward compatibility but are **deprecated for model routing**. @@ -261,7 +264,7 @@ Prep packs the active-range files into coverage bins and writes | K | Mode (`osh-mode`) | Review session | | --- | --- | --- | | ≤1 (incl. empty-diff `0`) | `collapse` | Single Sonnet + `--json-schema` (no Opus parent, no Haiku scorer) | -| >1 (max 4) | `fanout` | Opus parent + native Sonnet/Haiku Task subagents; only Opus emits SO | +| >1 (max 4) | `fanout` | Opus parent + Sonnet Task agents (`--agents`); only Opus emits SO | Publish still gates on that session's structured output. Multi-role `aggregate.js` stays shadow/non-gating. diff --git a/ai-review/action.yml b/ai-review/action.yml index 06ad631..4f7decf 100644 --- a/ai-review/action.yml +++ b/ai-review/action.yml @@ -436,13 +436,18 @@ runs: # inconclusive "success but no structured_output" failure mode. SONNET="claude/claude-sonnet-5" OPUS="claude/claude-opus-5" + # Context stage (top-level session) can use Haiku. HAIKU="claude/claude-haiku-4-5-20251001" + # Task subagents under Opus inherit adaptive thinking; Haiku returns + # HTTP 400 on this gateway. Helper roster roles (history/scorer) use + # Sonnet so fan-out workers actually run (finder ≠ scorer still holds). + HELPER_MODEL="${SONNET}" CURSOR="claude/cursor/composer-2.5" FREE_SO="oc/nemotron-3.5-lightning-free,oc/deepseek-v4-flash-free,auto/best-free" HAIKU_FALLBACK="${CURSOR},oc/nemotron-3.5-lightning-free,auto/best-free" SONNET_FALLBACK="${FREE_SO}" OPUS_FALLBACK="${FREE_SO}" - export SONNET OPUS HAIKU + export SONNET OPUS HAIKU HELPER_MODEL # Resolve the PR merge-base once (telemetry + meta base_sha). The # active review range may be a delta since the last published review. @@ -533,6 +538,11 @@ runs: cp "${ACTION_PATH}/osh-policy-collapse.md" .ai-review/osh-policy.md fi + # Pre-register Task agents (Sonnet) so the Opus parent can fan out in + # one parallel wave without improvising agent defs mid-review. + jq -c . "${ACTION_PATH}/osh-agents.json" > .ai-review/osh-agents.json + OSH_AGENTS="$(cat .ai-review/osh-agents.json)" + { echo "files=${FILES}" echo "churn=${CHURN}" @@ -547,6 +557,10 @@ runs: echo "merge-base-sha=${BASE_SHA}" echo "head-sha=${HEAD_SHA}" echo "delta-base-sha=${DELTA_BASE_SHA}" + # Multiline-safe: agents JSON is one jq -c line but may be large. + echo "osh-agents<> "${GITHUB_OUTPUT}" - name: Resolve linked issues @@ -822,6 +836,7 @@ runs: --fallback-model ${{ steps.route.outputs.fallback-model }} --max-turns 200 --allowedTools "${{ steps.route.outputs.review-allowed-tools }}" + --agents '${{ steps.route.outputs.osh-agents }}' --json-schema '{"type":"object","additionalProperties":false,"required":["verdict","confidence","merge_risk","intent","counts","review_event","comment_markdown"],"properties":{"verdict":{"type":"string","enum":["pass","fail"]},"confidence":{"type":"integer","minimum":0,"maximum":100},"merge_risk":{"type":"string","enum":["low","med","high"]},"intent":{"type":"string","enum":["aligned","partial","deviated","skipped"]},"counts":{"type":"object","additionalProperties":false,"required":["p0","p1","p2","p3"],"properties":{"p0":{"type":"integer","minimum":0},"p1":{"type":"integer","minimum":0},"p2":{"type":"integer","minimum":0},"p3":{"type":"integer","minimum":0}}},"review_event":{"type":"string","enum":["APPROVE","REQUEST_CHANGES"]},"comment_markdown":{"type":"string"},"findings":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["id","file","line","severity","summary","failure_scenario","reason","evidence","confidence"],"properties":{"id":{"type":"string"},"file":{"type":"string"},"line":{"type":"integer","minimum":0},"severity":{"type":"string","enum":["P0","P1","P2","P3"]},"summary":{"type":"string"},"failure_scenario":{"type":"string"},"reason":{"type":"string"},"evidence":{"type":"string"},"confidence":{"type":"integer","enum":[0,25,50,75,100]},"severity_confirmed":{"type":"string","enum":["P0","P1","P2","P3"]}}}},"files_reviewed":{"type":"array","items":{"type":"string"}},"tests_failing":{"type":"boolean"},"coverage_below_threshold_on_critical_paths":{"type":"boolean"},"no_tests_for_changed_logic":{"type":"boolean"},"test_execution":{"type":"string","enum":["passed","failed","skipped","not_run"]},"verification_evidence":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["claim","command","result"],"properties":{"claim":{"type":"string"},"command":{"type":"string"},"result":{"type":"string"}}}},"checklist":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["text","status"],"properties":{"text":{"type":"string"},"status":{"type":"string","enum":["verified","failed","unverifiable"]},"evidence":{"type":"string"}}}}}}' prompt: | You are performing an automated code review. Attempt to load the @@ -836,10 +851,12 @@ runs: Read `.ai-review/osh-policy.md` FIRST and obey it. Prep set `OSH_MODE` (`collapse` or `fanout`) to match that file: - collapse: single-session Sonnet review; no subagent fan-out. - - fanout: you are the Opus parent; spawn Sonnet/Haiku Task - subagents per `.ai-review/assignments.json`; only YOU emit - structured output. Workers return findings via Task results - (no Write tool). + - fanout: you are the Opus parent; spawn Task subagents using the + pre-registered `--agents` (`osh-coverage`, `osh-tracer`, + `osh-history`, `osh-scorer`) per `.ai-review/assignments.json` in + one parallel wave; only YOU emit structured output. Do not spawn + Haiku (inherits adaptive thinking → gateway 400). Workers return + findings via Task results (no Write tool). Read `${{ github.action_path }}/rubric.md`. This is the authoritative review methodology: an 8-angle scan (intent alignment @@ -1187,6 +1204,7 @@ runs: --fallback-model ${{ steps.route.outputs.fallback-model }} --max-turns 200 --allowedTools "${{ steps.route.outputs.review-allowed-tools }}" + --agents '${{ steps.route.outputs.osh-agents }}' --json-schema '{"type":"object","additionalProperties":false,"required":["verdict","confidence","merge_risk","intent","counts","review_event","comment_markdown"],"properties":{"verdict":{"type":"string","enum":["pass","fail"]},"confidence":{"type":"integer","minimum":0,"maximum":100},"merge_risk":{"type":"string","enum":["low","med","high"]},"intent":{"type":"string","enum":["aligned","partial","deviated","skipped"]},"counts":{"type":"object","additionalProperties":false,"required":["p0","p1","p2","p3"],"properties":{"p0":{"type":"integer","minimum":0},"p1":{"type":"integer","minimum":0},"p2":{"type":"integer","minimum":0},"p3":{"type":"integer","minimum":0}}},"review_event":{"type":"string","enum":["APPROVE","REQUEST_CHANGES"]},"comment_markdown":{"type":"string"},"findings":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["id","file","line","severity","summary","failure_scenario","reason","evidence","confidence"],"properties":{"id":{"type":"string"},"file":{"type":"string"},"line":{"type":"integer","minimum":0},"severity":{"type":"string","enum":["P0","P1","P2","P3"]},"summary":{"type":"string"},"failure_scenario":{"type":"string"},"reason":{"type":"string"},"evidence":{"type":"string"},"confidence":{"type":"integer","enum":[0,25,50,75,100]},"severity_confirmed":{"type":"string","enum":["P0","P1","P2","P3"]}}}},"files_reviewed":{"type":"array","items":{"type":"string"}},"tests_failing":{"type":"boolean"},"coverage_below_threshold_on_critical_paths":{"type":"boolean"},"no_tests_for_changed_logic":{"type":"boolean"},"test_execution":{"type":"string","enum":["passed","failed","skipped","not_run"]},"verification_evidence":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["claim","command","result"],"properties":{"claim":{"type":"string"},"command":{"type":"string"},"result":{"type":"string"}}}},"checklist":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["text","status"],"properties":{"text":{"type":"string"},"status":{"type":"string","enum":["verified","failed","unverifiable"]},"evidence":{"type":"string"}}}}}}' prompt: | This is a RETRY of an automated code review whose previous attempt @@ -1195,15 +1213,17 @@ runs: Read `.ai-review/osh-policy.md` and obey it (`OSH_MODE` is `collapse` or `fanout`). Under fanout you are the Opus parent — - spawn Sonnet/Haiku Task subagents per `.ai-review/assignments.json`; - only YOU emit structured output. Under collapse, do not fan out. + spawn Task subagents via pre-registered `--agents` (`osh-coverage`, + `osh-tracer`, `osh-history`, `osh-scorer`) per assignments.json in + one parallel wave; only YOU emit structured output. Do not spawn + Haiku. Under collapse, do not fan out. Populate the optional `findings` and `files_reviewed` fields of the structured output as well. `findings` is one entry per finding you report. Under **collapse**, include your own confidence 0/25/50/75/100 and, where you would reclassify it, `severity_confirmed`. Under **fanout**, copy each finding's `confidence` and `severity_confirmed` - from the Haiku scorer (finder ≠ scorer) — do not self-score when a + from the `osh-scorer` agent (finder ≠ scorer) — do not self-score when a scorer ran. `files_reviewed` lists every file read in full for the active range — it is cross-checked against the staged manifest, so a short list reads as incomplete coverage. A diff --git a/ai-review/lib/publish.js b/ai-review/lib/publish.js index d527c93..5630ff7 100644 --- a/ai-review/lib/publish.js +++ b/ai-review/lib/publish.js @@ -79,15 +79,13 @@ function stripLeadingBannerArtifacts(markdown) { * `reviewMeta` stamps the delta baseline (spec §6.1); Publish passes * current HEAD + PR merge-base + this run's full|delta mode. */ -function modelLine(modelUsed) { - if (!modelUsed || typeof modelUsed !== "string" || !modelUsed.trim()) return []; - return ["", `Model: \`${modelUsed.trim()}\``]; +function modelLine(_modelUsed) { + // Model attribution removed from published review bodies (consumer ask). + return []; } -function modelFooter(modelUsed) { - const line = modelLine(modelUsed); - if (!line.length) return []; - return [...line, "_Re-run this job if you need another review pass._"]; +function modelFooter(_modelUsed) { + return ["", "_Re-run this job if you need another review pass._"]; } function buildReviewBody({ diff --git a/ai-review/lib/publish.test.js b/ai-review/lib/publish.test.js index d05ce3d..057e114 100644 --- a/ai-review/lib/publish.test.js +++ b/ai-review/lib/publish.test.js @@ -152,18 +152,20 @@ test("always leads with the marker", () => { assert.match(body, /^\n/); }); -test("buildReviewBody includes model used and re-review hint", () => { +test("buildReviewBody omits Model line but keeps re-review hint", () => { const body = buildReviewBody({ ...BASE_ARGS, modelUsed: "claude/claude-sonnet-5", }); - assert.match(body, /Model: `claude\/claude-sonnet-5`/); + assert.doesNotMatch(body, /^Model:/m); + assert.doesNotMatch(body, /Model: `claude\/claude-sonnet-5`/); assert.match(body, /Re-run this job if you need another review pass/); }); -test("buildReviewBody omits Model line when modelUsed is empty", () => { +test("buildReviewBody still omits Model line when modelUsed is empty", () => { const body = buildReviewBody({ ...BASE_ARGS, modelUsed: "" }); assert.doesNotMatch(body, /^Model:/m); + assert.match(body, /Re-run this job if you need another review pass/); }); test("buildReviewBody stamps ai-review-meta immediately after the marker", () => { @@ -201,11 +203,12 @@ test("with salvaged text the details block contains it", () => { assert.match(body, /<\/details>/); }); -test("buildInconclusiveBody includes Model line but not a second italic re-run hint", () => { +test("buildInconclusiveBody omits Model line and keeps job re-run instruction", () => { const body = buildInconclusiveBody("salvaged text", { modelUsed: "claude/cursor/composer-2.5", }); - assert.match(body, /Model: `claude\/cursor\/composer-2.5`/); + assert.doesNotMatch(body, /Model: `claude\/cursor\/composer-2.5`/); + assert.doesNotMatch(body, /^Model:/m); assert.match(body, /\*\*Re-run the `ai-review` job\*\*/); assert.doesNotMatch(body, /_Re-run this job if you need another review pass\._/); }); diff --git a/ai-review/lib/write-manifest.js b/ai-review/lib/write-manifest.js index 113b1d0..39b515d 100644 --- a/ai-review/lib/write-manifest.js +++ b/ai-review/lib/write-manifest.js @@ -178,7 +178,13 @@ function writeRoster(manifest, sizes, io) { models: { opus: process.env.OPUS || "claude/claude-opus-5", sonnet: process.env.SONNET || "claude/claude-sonnet-5", - haiku: process.env.HAIKU || "claude/claude-haiku-4-5-20251001", + // Helper tier (history/scorer): Task subagents under Opus inherit + // adaptive thinking, so Haiku 400s on this gateway. Prefer HELPER_MODEL + // (Sonnet) when set; fall back to HAIKU for older callers. + haiku: + process.env.HELPER_MODEL || + process.env.HAIKU || + "claude/claude-sonnet-5", }, importEdges: resolveImportEdges(specifiers, manifest.changed_files), symbolManifest: manifest.symbol_manifest, diff --git a/ai-review/osh-agents.json b/ai-review/osh-agents.json new file mode 100644 index 0000000..106b1ca --- /dev/null +++ b/ai-review/osh-agents.json @@ -0,0 +1,26 @@ +{ + "osh-coverage": { + "description": "Coverage reviewer for assigned changed files. Use one Task per reviewer-* role from assignments.json.", + "model": "claude/claude-sonnet-5", + "tools": ["Read", "Grep", "Glob", "Bash"], + "prompt": "You are an OSH coverage reviewer. Read 100% of every path in your assigned_files with Read — never sample from hunks alone. Apply the per-file rubric angles. Propose findings with P0-P3 severity labels only. Do not self-score confidence. Return JSON: {role, assigned_files, files_reviewed, findings[]}." + }, + "osh-tracer": { + "description": "Cross-file coherence tracer (Angle C). Use for the tracer role in assignments.json.", + "model": "claude/claude-sonnet-5", + "tools": ["Read", "Grep", "Glob", "Bash"], + "prompt": "You are the OSH coherence tracer. You have zero assigned files. Follow symbol_manifest and split_clusters for cross-file breakage. Return JSON: {role:\"tracer\", findings[]}." + }, + "osh-history": { + "description": "History / perspective gatherer. Use for the history role in assignments.json.", + "model": "claude/claude-sonnet-5", + "tools": ["Read", "Grep", "Glob", "Bash"], + "prompt": "You are the OSH history helper. Cheap gathers: git blame, prior review comments, code-comment angles. Not full-file exhaustiveness. Return JSON: {role:\"history\", notes, findings[]}." + }, + "osh-scorer": { + "description": "Independent confidence scorer. Use after finders return. Finder must not equal scorer.", + "model": "claude/claude-sonnet-5", + "tools": ["Read", "Grep", "Glob"], + "prompt": "You are the independent OSH scorer. You did not find the issues. For each finding from coverage/tracer/history/intent, rate confidence 0/25/50/75/100 and severity_confirmed P0-P3. Return JSON: {role:\"scorer\", scores:[{id, confidence, severity_confirmed}]}." + } +} diff --git a/ai-review/osh-policy-fanout.md b/ai-review/osh-policy-fanout.md index b8847f4..3094de4 100644 --- a/ai-review/osh-policy-fanout.md +++ b/ai-review/osh-policy-fanout.md @@ -9,31 +9,37 @@ that schema blob. Read `.ai-review/assignments.json` first. Honor its partition: do not invent a second file split. Cap is K≤4 coverage reviewers. -Locked model IDs (match `roles[].model` when spawning): - -- Opus parent / intent: `claude/claude-opus-5` -- Sonnet coverage + tracer: `claude/claude-sonnet-5` -- Haiku history + scorer: `claude/claude-haiku-4-5-20251001` - -## Subagents (native Claude Code Task tool) - -Spawn concurrent Task subagents (no Write tool — return results in the Task -response text/JSON; you aggregate into structured output): - -1. **Sonnet** — one per `reviewer-*` (`kind: coverage`): read 100% of every - `assigned_files` path with `Read`; run per-file rubric angles; propose - findings with P0–P3 severity (finder labels only — do not self-score). -2. **Sonnet** — `tracer` (`kind: coherence`): zero assigned files; follow - `symbol_manifest` / `split_clusters` for cross-file breakage (Angle C). -3. **Haiku** — `history` (`kind: perspective`): git blame / prior comments / - code-comment angles; cheap gathers, not full-file exhaustiveness. -4. **Haiku** — `scorer` (`kind: scoring`): independent confidence - 0/25/50/75/100 and `severity_confirmed` for every finding raised by - finders. **Finder ≠ scorer** — you must copy these into the SO - `findings[].confidence` / `severity_confirmed` fields; do not invent - your own scores when the scorer ran. +Locked model IDs for **Task** subagents (also registered via `--agents`): + +- Opus parent / intent: `claude/claude-opus-5` (you — this session) +- All Task workers: `claude/claude-sonnet-5` via agents `osh-coverage`, + `osh-tracer`, `osh-history`, `osh-scorer` + +**Do not spawn Haiku as a Task subagent.** Claude Code Task subagents inherit +this Opus session's adaptive/extended thinking; Haiku returns +`400 adaptive thinking is not supported on this model` on this gateway. +Helper roles still run as **independent Sonnet sessions** (finder ≠ scorer). + +## Subagents — spawn one concurrent wave + +Use the Task tool with the pre-registered `--agents` names. Launch **all** of +the following in **one parallel wave** (do not serialize coverage then tracer +then helpers — wall-clock is max(worker), not sum): + +1. **`osh-coverage`** — one Task per `reviewer-*` (`kind: coverage`): pass + that role's `assigned_files` in the Task prompt; read 100% of every path + with `Read`; propose findings with P0–P3 severity (finder labels only). +2. **`osh-tracer`** — `tracer` (`kind: coherence`): follow `symbol_manifest` + / `split_clusters` for cross-file breakage (Angle C). +3. **`osh-history`** — `history` (`kind: perspective`): git blame / prior + comments / code-comment angles; cheap gathers. +4. **`osh-scorer`** — `scorer` (`kind: scoring`): after finders return (or in + the same wave if you pass provisional findings, then re-score once), + independent confidence 0/25/50/75/100 and `severity_confirmed`. + **Finder ≠ scorer** — copy these into SO `findings[].confidence` / + `severity_confirmed`; do not invent your own scores when the scorer ran. 5. **Intent** (`kind: frame`): you own Angle H (or dispatch an Opus-tier - subagent). Frame from linked issues + PR body **before** treating coverage + Task). Frame from linked issues + PR body **before** treating coverage findings as settled. ## What you must not re-read @@ -49,5 +55,5 @@ require it). Resolve conflicts, prioritize, own intent, map Test Plan↔CI gaps to findings, apply delta prior-review carry-forward rules, then emit the single structured output. For each finding in SO: take `confidence` and `severity_confirmed` -from the Haiku scorer; severity labels may originate from workers. Publish -gates on your SO only. +from the `osh-scorer` result; severity labels may originate from workers. +Publish gates on your SO only. diff --git a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md index 7ce0f7f..f827d1e 100644 --- a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md +++ b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md @@ -363,19 +363,31 @@ and must not invent a second partition. | Tier | Primary ID used in action / roster | |---|---| | Opus (parent / intent tier) | `claude/claude-opus-5` | -| Sonnet (coverage + tracer) | `claude/claude-sonnet-5` | -| Haiku (history + independent scorer) | `claude/claude-haiku-4-5-20251001` | - -Subagents use the `model` field on each role in `assignments.json` (same strings). Cap remains +| Sonnet (coverage + tracer + Task helpers) | `claude/claude-sonnet-5` | +| Haiku (context stage only — top-level session) | `claude/claude-haiku-4-5-20251001` | + +**Task subagents must not use Haiku under an Opus parent.** Claude Code Task +subagents inherit the parent's adaptive/extended thinking; Haiku returns +`400 adaptive thinking is not supported on this model` on this gateway. +History + independent scorer therefore run as **Sonnet** Task agents +(`osh-history`, `osh-scorer` via `--agents`); finder ≠ scorer still holds +because they are separate sessions. Context-stage Haiku remains valid (not a +Task child of Opus). + +Subagents use pre-registered `--agents` (`osh-coverage`, `osh-tracer`, +`osh-history`, `osh-scorer`) plus `assignments.json` role partition. Cap remains **K ≤ 4** coverage reviewers (`roster.js` `MAX_K`). ### Fan-out path (`assignments.json` `.k` > 1) 1. Parent session `--model` is Opus. Only this session may use `--json-schema` / emit structured output for Publish. -2. Read `.ai-review/assignments.json`. Spawn native Claude Code **Task/Agent** subagents: - - **Sonnet** for each `reviewer-*` (coverage) and for `tracer` (coherence). - - **Haiku** for `history` and for `scorer` (independent confidence / `severity_confirmed`). +2. Read `.ai-review/assignments.json`. Spawn native Claude Code **Task** subagents + in **one parallel wave** using `--agents`: + - **`osh-coverage`** (Sonnet) — one Task per `reviewer-*`. + - **`osh-tracer`** (Sonnet) — coherence. + - **`osh-history`** (Sonnet) — perspective gathers. + - **`osh-scorer`** (Sonnet) — independent confidence / `severity_confirmed`. - **Intent** (`kind: frame`): Opus owns it (parent may run it itself or dispatch an Opus-tier subagent). Keep intent isolated from coverage analysis. 3. Workers return freeform or JSON **via Task tool results** (Review allowlist @@ -386,7 +398,7 @@ Subagents use the `model` field on each role in `assignments.json` (same strings coverage workers' union of `assigned_files` (plus tracer / neighbor expansion rules). 5. Parent aggregates worker outputs into the schema contract (`comment_markdown`, `findings`, `counts`, `intent`, etc.). On fan-out, each finding's `confidence` / - `severity_confirmed` must come from the Haiku **scorer** (finder ≠ scorer) — + `severity_confirmed` must come from **`osh-scorer`** (finder ≠ scorer) — the parent must not self-score when a scorer ran. Publish continues to consume **parent structured output** (same fail-closed gate as today); multi-file `aggregate.js` remains shadow/non-gating until a follow-up wires it live. From a5d718d1e10b9650945c5d3bbe0fed21b6e89881 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 16:40:03 +0300 Subject: [PATCH 12/16] fix(ai-review): point fanout prompt at osh-scorer, not Haiku --- ai-review/action.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ai-review/action.yml b/ai-review/action.yml index 4f7decf..ae887c4 100644 --- a/ai-review/action.yml +++ b/ai-review/action.yml @@ -503,8 +503,8 @@ runs: K="${K:-0}" # K≤1 (including empty-diff k:0): single Sonnet session emits - # --json-schema directly — no Opus parent, no Haiku scorer. - # K>1: Opus parent + native Sonnet/Haiku subagents; only Opus emits SO. + # --json-schema directly — no Opus parent, no independent scorer. + # K>1: Opus parent + Sonnet Task --agents; only Opus emits SO. if [ "${K}" -le 1 ]; then MODEL="${SONNET}" FALLBACK="${SONNET_FALLBACK}" @@ -970,7 +970,7 @@ runs: report. Under **collapse**, include your own confidence 0/25/50/75/100 and, where you would reclassify it, `severity_confirmed`. Under **fanout**, copy each finding's `confidence` and `severity_confirmed` - from the Haiku scorer (finder ≠ scorer) — do not self-score when a + from the `osh-scorer` agent (finder ≠ scorer) — do not self-score when a scorer ran; worker severity labels may still seed the finding. `files_reviewed` lists every file read in full for the active range (yours under collapse; union of From 1e1be5f4c2ab30ccaa30ff10f7f8f56eaf9c3e38 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 17:03:20 +0300 Subject: [PATCH 13/16] fix(ai-review): bypass flaky collaborator permission API in selftest Claude stages abort when GET .../collaborators/.../permission returns 503, before any model work. Pass allowed_non_write_users through and scope selftest to the PR author so Context/Review can run; Publish still uses the App token. --- .github/workflows/selftest.yml | 5 +++++ ai-review/action.yml | 16 ++++++++++++++++ 2 files changed, 21 insertions(+) diff --git a/.github/workflows/selftest.yml b/.github/workflows/selftest.yml index d7b6bd1..b7afcc6 100644 --- a/.github/workflows/selftest.yml +++ b/.github/workflows/selftest.yml @@ -66,6 +66,11 @@ jobs: private-key: ${{ secrets.MTM_BOT_APP_PRIVATE_KEY }} anthropic-auth-token: ${{ secrets.ANTHROPIC_AUTH_TOKEN }} anthropic-base-url: ${{ vars.ANTHROPIC_BASE_URL }} + # Skip collaborator permission API (has been 503ing and aborting + # Claude stages before the model runs). Scoped to this PR author, + # not "*". Context/Review use read-scoped github.token; Publish + # posts with the App token. + allowed-non-write-users: eduly-haldarwish gated-demo: needs: [review] diff --git a/ai-review/action.yml b/ai-review/action.yml index ae887c4..2a9324a 100644 --- a/ai-review/action.yml +++ b/ai-review/action.yml @@ -54,6 +54,18 @@ inputs: controls — scope this to named bots, not `'*'`. required: false default: "" + allowed-non-write-users: + description: > + Passed through to `claude-code-action`'s `allowed_non_write_users`. + Comma-separated usernames (or `*`) allowed to run without a successful + collaborator write-permission lookup. Only takes effect when + `github-token` is provided (the default). Use when that GitHub API is + flaky (503) or for workflows whose Claude stages already use a + read-scoped token and never post via that token. WARNING: bypasses a + security check — prefer named users; `*` only in tightly scoped + callers (e.g. this repo's selftest). + required: false + default: "" pr-number: description: > Pull request number to review. Optional — defaults to the @@ -629,6 +641,7 @@ runs: # steps later in this action. github_token: ${{ inputs.github-token }} allowed_bots: ${{ inputs.allowed-bots }} + allowed_non_write_users: ${{ inputs.allowed-non-write-users }} # Root cause of the context.md handoff failure (Task 2.4 fix round): this # step ran with NO --allowedTools at all, so claude-code-action's headless # permission model silently denied every Write and most Bash tool calls @@ -829,6 +842,7 @@ runs: # Derive). The App-minted identity is reserved for Publish below. github_token: ${{ inputs.github-token }} allowed_bots: ${{ inputs.allowed-bots }} + allowed_non_write_users: ${{ inputs.allowed-non-write-users }} plugins: superpowers@superpowers-marketplace plugin_marketplaces: https://github.com/obra/superpowers-marketplace.git claude_args: | @@ -1108,6 +1122,7 @@ runs: anthropic_api_key: ${{ inputs.anthropic-api-key != '' && inputs.anthropic-api-key || inputs.anthropic-auth-token }} github_token: ${{ inputs.github-token }} allowed_bots: ${{ inputs.allowed-bots }} + allowed_non_write_users: ${{ inputs.allowed-non-write-users }} claude_args: | --resume ${{ steps.retry_budget.outputs.session_id }} --model ${{ steps.route.outputs.model }} @@ -1197,6 +1212,7 @@ runs: anthropic_api_key: ${{ inputs.anthropic-api-key != '' && inputs.anthropic-api-key || inputs.anthropic-auth-token }} github_token: ${{ inputs.github-token }} allowed_bots: ${{ inputs.allowed-bots }} + allowed_non_write_users: ${{ inputs.allowed-non-write-users }} plugins: superpowers@superpowers-marketplace plugin_marketplaces: https://github.com/obra/superpowers-marketplace.git claude_args: | From 5fda68d3139ceedf3998bbc9fafb27ed46ac7fa7 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 17:34:16 +0300 Subject: [PATCH 14/16] fix(ai-review): unblock publish after permission bypass and harden OSH Opt out of CCA subprocess env scrub (invalid package.json broke Publish), stop silencing testplan-ci failures, tighten Task tools, and align docs with Sonnet-only workers plus the two-axis K formula. Co-authored-by: Cursor --- ai-review/action.yml | 37 ++++++++++--- ai-review/lib/publish.js | 7 --- ai-review/lib/testplan-ci.test.js | 53 +++++++++++++++++++ ai-review/lib/write-manifest.js | 10 ++-- ai-review/osh-agents.json | 31 +++++++++-- ai-review/osh-policy-fanout.md | 10 ++++ ...6-test-plan-ci-findings-and-osh-routing.md | 2 +- docs/plan.md | 2 +- ...-17-ai-review-osh-delta-testplan-design.md | 24 +++++---- 9 files changed, 138 insertions(+), 38 deletions(-) diff --git a/ai-review/action.yml b/ai-review/action.yml index 2a9324a..8edbfb2 100644 --- a/ai-review/action.yml +++ b/ai-review/action.yml @@ -411,7 +411,7 @@ runs: # lib/roster.js: which reviewer reads which files, and under which model. # Slice 3 (OSH): routing and the review prompt consume `.k` and the role # list. K≤1 → single Sonnet session (collapse); K>1 → Opus parent that - # fans out native Sonnet/Haiku subagents per the roster. No Node + # fans out native Sonnet Task workers per the roster. No Node # Promise.race orchestrator — concurrency is Claude Code's scheduler. # # All parsing lives in lib/prep.js and lib/roster.js behind unit tests; @@ -632,6 +632,10 @@ runs: ANTHROPIC_BASE_URL: ${{ inputs.anthropic-base-url }} API_TIMEOUT_MS: ${{ inputs.api-timeout-ms }} ANTHROPIC_CUSTOM_HEADERS: "${{ inputs.anthropic-auth-token != '' && format('Authorization: Bearer {0}', inputs.anthropic-auth-token) || '' }}" + # allowed_non_write_users enables CCA subprocess isolation by + # default; that writes an invalid root package.json and breaks + # later node requires (Publish/telemetry). Opt out. + CLAUDE_CODE_SUBPROCESS_ENV_SCRUB: "0" with: anthropic_api_key: ${{ inputs.anthropic-api-key != '' && inputs.anthropic-api-key || inputs.anthropic-auth-token }} # Deliberately NOT steps.identity.outputs.author-token: this stage only @@ -760,7 +764,16 @@ runs: PR_BODY_PATH=".ai-review/pr-body.md" \ CHECK_RUNS_PATH=".ai-review/check-runs.raw.json" \ AI_REVIEW_DIR=".ai-review" \ - node "${TESTPLAN_CLI}" || true + node "${TESTPLAN_CLI}" || { + echo "::warning::testplan-ci failed; writing empty Test Plan/CI artifacts so the review can still map coverage" + echo '[]' > .ai-review/test-plan-items.json + echo '[]' > .ai-review/ci-checks.json + } + if [ ! -f .ai-review/test-plan-items.json ] || [ ! -f .ai-review/ci-checks.json ]; then + echo "::warning::Test Plan/CI artifacts missing after inventory; writing empty stubs" + echo '[]' > .ai-review/test-plan-items.json + echo '[]' > .ai-review/ci-checks.json + fi if [ -n "${CHECK_RUNS}" ] && [ "${CHECK_RUNS}" != "[]" ] && [ "${CHECK_RUNS}" != "null" ]; then # If any returned check-run has not yet reached "completed" @@ -788,7 +801,7 @@ runs: # exits 1 (upstream flakiness). Don't crash the job — the retry step # below re-attempts, and Publish degrades gracefully if both miss. # - # --max-turns 200 (in claude_args below) was intended as a mechanical + # --max-turns 250 (in claude_args below) was intended as a mechanical # backstop for rubric.md's Angle C bound. DO NOT TRUST IT AS A CEILING: # a measured run (EdulyCom/eduly 31294407787) reported num_turns 208 # under this cap with subtype "success", and grepping the eight @@ -828,6 +841,10 @@ runs: ANTHROPIC_BASE_URL: ${{ inputs.anthropic-base-url }} API_TIMEOUT_MS: ${{ inputs.api-timeout-ms }} ANTHROPIC_CUSTOM_HEADERS: "${{ inputs.anthropic-auth-token != '' && format('Authorization: Bearer {0}', inputs.anthropic-auth-token) || '' }}" + # allowed_non_write_users enables CCA subprocess isolation by + # default; that writes an invalid root package.json and breaks + # later node requires (Publish/telemetry). Opt out. + CLAUDE_CODE_SUBPROCESS_ENV_SCRUB: "0" PR_NUMBER: ${{ steps.pr.outputs.pr-number }} OSH_MODE: ${{ steps.route.outputs.osh-mode }} with: @@ -848,7 +865,7 @@ runs: claude_args: | --model ${{ steps.route.outputs.model }} --fallback-model ${{ steps.route.outputs.fallback-model }} - --max-turns 200 + --max-turns 250 --allowedTools "${{ steps.route.outputs.review-allowed-tools }}" --agents '${{ steps.route.outputs.osh-agents }}' --json-schema '{"type":"object","additionalProperties":false,"required":["verdict","confidence","merge_risk","intent","counts","review_event","comment_markdown"],"properties":{"verdict":{"type":"string","enum":["pass","fail"]},"confidence":{"type":"integer","minimum":0,"maximum":100},"merge_risk":{"type":"string","enum":["low","med","high"]},"intent":{"type":"string","enum":["aligned","partial","deviated","skipped"]},"counts":{"type":"object","additionalProperties":false,"required":["p0","p1","p2","p3"],"properties":{"p0":{"type":"integer","minimum":0},"p1":{"type":"integer","minimum":0},"p2":{"type":"integer","minimum":0},"p3":{"type":"integer","minimum":0}}},"review_event":{"type":"string","enum":["APPROVE","REQUEST_CHANGES"]},"comment_markdown":{"type":"string"},"findings":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["id","file","line","severity","summary","failure_scenario","reason","evidence","confidence"],"properties":{"id":{"type":"string"},"file":{"type":"string"},"line":{"type":"integer","minimum":0},"severity":{"type":"string","enum":["P0","P1","P2","P3"]},"summary":{"type":"string"},"failure_scenario":{"type":"string"},"reason":{"type":"string"},"evidence":{"type":"string"},"confidence":{"type":"integer","enum":[0,25,50,75,100]},"severity_confirmed":{"type":"string","enum":["P0","P1","P2","P3"]}}}},"files_reviewed":{"type":"array","items":{"type":"string"}},"tests_failing":{"type":"boolean"},"coverage_below_threshold_on_critical_paths":{"type":"boolean"},"no_tests_for_changed_logic":{"type":"boolean"},"test_execution":{"type":"string","enum":["passed","failed","skipped","not_run"]},"verification_evidence":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["claim","command","result"],"properties":{"claim":{"type":"string"},"command":{"type":"string"},"result":{"type":"string"}}}},"checklist":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["text","status"],"properties":{"text":{"type":"string"},"status":{"type":"string","enum":["verified","failed","unverifiable"]},"evidence":{"type":"string"}}}}}}' @@ -1027,7 +1044,7 @@ runs: exit 0 # Retry blast-radius gate. The retry below re-runs the ENTIRE review at - # --max-turns 200 — it is not a cheap re-emit. After a long first attempt + # --max-turns 250 — it is not a cheap re-emit. After a long first attempt # it cannot finish inside any caller's timeout-minutes, so it only burns # tokens until the job is killed: review-stage durations up to 79.5 min # were measured, and 32 of 682 jobs died on the caller's clock. @@ -1118,6 +1135,10 @@ runs: ANTHROPIC_BASE_URL: ${{ inputs.anthropic-base-url }} API_TIMEOUT_MS: ${{ inputs.api-timeout-ms }} ANTHROPIC_CUSTOM_HEADERS: "${{ inputs.anthropic-auth-token != '' && format('Authorization: Bearer {0}', inputs.anthropic-auth-token) || '' }}" + # allowed_non_write_users enables CCA subprocess isolation by + # default; that writes an invalid root package.json and breaks + # later node requires (Publish/telemetry). Opt out. + CLAUDE_CODE_SUBPROCESS_ENV_SCRUB: "0" with: anthropic_api_key: ${{ inputs.anthropic-api-key != '' && inputs.anthropic-api-key || inputs.anthropic-auth-token }} github_token: ${{ inputs.github-token }} @@ -1206,6 +1227,10 @@ runs: ANTHROPIC_BASE_URL: ${{ inputs.anthropic-base-url }} API_TIMEOUT_MS: ${{ inputs.api-timeout-ms }} ANTHROPIC_CUSTOM_HEADERS: "${{ inputs.anthropic-auth-token != '' && format('Authorization: Bearer {0}', inputs.anthropic-auth-token) || '' }}" + # allowed_non_write_users enables CCA subprocess isolation by + # default; that writes an invalid root package.json and breaks + # later node requires (Publish/telemetry). Opt out. + CLAUDE_CODE_SUBPROCESS_ENV_SCRUB: "0" PR_NUMBER: ${{ steps.pr.outputs.pr-number }} OSH_MODE: ${{ steps.route.outputs.osh-mode }} with: @@ -1218,7 +1243,7 @@ runs: claude_args: | --model ${{ steps.route.outputs.model }} --fallback-model ${{ steps.route.outputs.fallback-model }} - --max-turns 200 + --max-turns 250 --allowedTools "${{ steps.route.outputs.review-allowed-tools }}" --agents '${{ steps.route.outputs.osh-agents }}' --json-schema '{"type":"object","additionalProperties":false,"required":["verdict","confidence","merge_risk","intent","counts","review_event","comment_markdown"],"properties":{"verdict":{"type":"string","enum":["pass","fail"]},"confidence":{"type":"integer","minimum":0,"maximum":100},"merge_risk":{"type":"string","enum":["low","med","high"]},"intent":{"type":"string","enum":["aligned","partial","deviated","skipped"]},"counts":{"type":"object","additionalProperties":false,"required":["p0","p1","p2","p3"],"properties":{"p0":{"type":"integer","minimum":0},"p1":{"type":"integer","minimum":0},"p2":{"type":"integer","minimum":0},"p3":{"type":"integer","minimum":0}}},"review_event":{"type":"string","enum":["APPROVE","REQUEST_CHANGES"]},"comment_markdown":{"type":"string"},"findings":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["id","file","line","severity","summary","failure_scenario","reason","evidence","confidence"],"properties":{"id":{"type":"string"},"file":{"type":"string"},"line":{"type":"integer","minimum":0},"severity":{"type":"string","enum":["P0","P1","P2","P3"]},"summary":{"type":"string"},"failure_scenario":{"type":"string"},"reason":{"type":"string"},"evidence":{"type":"string"},"confidence":{"type":"integer","enum":[0,25,50,75,100]},"severity_confirmed":{"type":"string","enum":["P0","P1","P2","P3"]}}}},"files_reviewed":{"type":"array","items":{"type":"string"}},"tests_failing":{"type":"boolean"},"coverage_below_threshold_on_critical_paths":{"type":"boolean"},"no_tests_for_changed_logic":{"type":"boolean"},"test_execution":{"type":"string","enum":["passed","failed","skipped","not_run"]},"verification_evidence":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["claim","command","result"],"properties":{"claim":{"type":"string"},"command":{"type":"string"},"result":{"type":"string"}}}},"checklist":{"type":"array","items":{"type":"object","additionalProperties":false,"required":["text","status"],"properties":{"text":{"type":"string"},"status":{"type":"string","enum":["verified","failed","unverifiable"]},"evidence":{"type":"string"}}}}}}' diff --git a/ai-review/lib/publish.js b/ai-review/lib/publish.js index 5630ff7..eeff95e 100644 --- a/ai-review/lib/publish.js +++ b/ai-review/lib/publish.js @@ -79,11 +79,6 @@ function stripLeadingBannerArtifacts(markdown) { * `reviewMeta` stamps the delta baseline (spec §6.1); Publish passes * current HEAD + PR merge-base + this run's full|delta mode. */ -function modelLine(_modelUsed) { - // Model attribution removed from published review bodies (consumer ask). - return []; -} - function modelFooter(_modelUsed) { return ["", "_Re-run this job if you need another review pass._"]; } @@ -148,7 +143,6 @@ function buildReviewBody({ * cannot treat the body as a delta baseline (spec §6.1). */ function buildInconclusiveBody(salvaged, opts = {}) { - const modelUsed = opts && opts.modelUsed; const reviewMeta = opts && opts.reviewMeta; const inconclusiveMeta = reviewMeta && reviewMeta.headSha && reviewMeta.baseSha @@ -167,7 +161,6 @@ function buildInconclusiveBody(salvaged, opts = {}) { "fails closed.", "", "**Re-run the `ai-review` job** to get a verdict.", - ...modelLine(modelUsed), ...(salvaged ? [ "", diff --git a/ai-review/lib/testplan-ci.test.js b/ai-review/lib/testplan-ci.test.js index 3dc7db5..48a3348 100644 --- a/ai-review/lib/testplan-ci.test.js +++ b/ai-review/lib/testplan-ci.test.js @@ -200,3 +200,56 @@ test("writeTestPlanCiArtifacts writes both JSON shapes", () => { { name: "ci / test", conclusion: "success", status: "completed" }, ]); }); + +test("main: reads env paths and writes Test Plan/CI artifacts", () => { + const fs = require("node:fs"); + const os = require("node:os"); + const { main } = require("./testplan-ci.js"); + + const dir = fs.mkdtempSync(path.join(os.tmpdir(), "testplan-ci-main-")); + try { + fs.writeFileSync( + path.join(dir, "pr-body.md"), + "## Test Plan\n- [ ] Unit tests for parser\n", + ); + fs.writeFileSync( + path.join(dir, "check-runs.raw.json"), + JSON.stringify([ + { + name: "ci / test", + conclusion: "success", + status: "completed", + }, + ]), + ); + + const prev = { + AI_REVIEW_DIR: process.env.AI_REVIEW_DIR, + PR_BODY_PATH: process.env.PR_BODY_PATH, + CHECK_RUNS_PATH: process.env.CHECK_RUNS_PATH, + }; + process.env.AI_REVIEW_DIR = dir; + process.env.PR_BODY_PATH = path.join(dir, "pr-body.md"); + process.env.CHECK_RUNS_PATH = path.join(dir, "check-runs.raw.json"); + try { + main(); + } finally { + for (const [k, v] of Object.entries(prev)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } + } + + const plan = JSON.parse( + fs.readFileSync(path.join(dir, "test-plan-items.json"), "utf8"), + ); + const ci = JSON.parse( + fs.readFileSync(path.join(dir, "ci-checks.json"), "utf8"), + ); + assert.deepEqual(plan.items, ["Unit tests for parser"]); + assert.equal(ci.checks.length, 1); + assert.equal(ci.checks[0].name, "ci / test"); + } finally { + fs.rmSync(dir, { recursive: true, force: true }); + } +}); diff --git a/ai-review/lib/write-manifest.js b/ai-review/lib/write-manifest.js index 39b515d..209564d 100644 --- a/ai-review/lib/write-manifest.js +++ b/ai-review/lib/write-manifest.js @@ -178,13 +178,9 @@ function writeRoster(manifest, sizes, io) { models: { opus: process.env.OPUS || "claude/claude-opus-5", sonnet: process.env.SONNET || "claude/claude-sonnet-5", - // Helper tier (history/scorer): Task subagents under Opus inherit - // adaptive thinking, so Haiku 400s on this gateway. Prefer HELPER_MODEL - // (Sonnet) when set; fall back to HAIKU for older callers. - haiku: - process.env.HELPER_MODEL || - process.env.HAIKU || - "claude/claude-sonnet-5", + // Helper tier (history/scorer): Task under Opus cannot use Haiku on + // this gateway (adaptive thinking 400). Always Sonnet unless overridden. + haiku: process.env.HELPER_MODEL || "claude/claude-sonnet-5", }, importEdges: resolveImportEdges(specifiers, manifest.changed_files), symbolManifest: manifest.symbol_manifest, diff --git a/ai-review/osh-agents.json b/ai-review/osh-agents.json index 106b1ca..0796d1e 100644 --- a/ai-review/osh-agents.json +++ b/ai-review/osh-agents.json @@ -2,25 +2,46 @@ "osh-coverage": { "description": "Coverage reviewer for assigned changed files. Use one Task per reviewer-* role from assignments.json.", "model": "claude/claude-sonnet-5", - "tools": ["Read", "Grep", "Glob", "Bash"], - "prompt": "You are an OSH coverage reviewer. Read 100% of every path in your assigned_files with Read — never sample from hunks alone. Apply the per-file rubric angles. Propose findings with P0-P3 severity labels only. Do not self-score confidence. Return JSON: {role, assigned_files, files_reviewed, findings[]}." + "tools": [ + "Read", + "Grep", + "Glob" + ], + "prompt": "You are an OSH coverage reviewer. Read 100% of every path in your assigned_files with Read \u2014 never sample from hunks alone. Apply the per-file rubric angles. Propose findings with P0-P3 severity labels only. Do not self-score confidence. Return JSON: {role, assigned_files, files_reviewed, findings[]}." }, "osh-tracer": { "description": "Cross-file coherence tracer (Angle C). Use for the tracer role in assignments.json.", "model": "claude/claude-sonnet-5", - "tools": ["Read", "Grep", "Glob", "Bash"], + "tools": [ + "Read", + "Grep", + "Glob" + ], "prompt": "You are the OSH coherence tracer. You have zero assigned files. Follow symbol_manifest and split_clusters for cross-file breakage. Return JSON: {role:\"tracer\", findings[]}." }, "osh-history": { "description": "History / perspective gatherer. Use for the history role in assignments.json.", "model": "claude/claude-sonnet-5", - "tools": ["Read", "Grep", "Glob", "Bash"], + "tools": [ + "Read", + "Grep", + "Glob", + "Bash(git log:*)", + "Bash(git blame:*)", + "Bash(git show:*)", + "Bash(gh pr list:*)", + "Bash(gh pr view:*)" + ], "prompt": "You are the OSH history helper. Cheap gathers: git blame, prior review comments, code-comment angles. Not full-file exhaustiveness. Return JSON: {role:\"history\", notes, findings[]}." }, "osh-scorer": { "description": "Independent confidence scorer. Use after finders return. Finder must not equal scorer.", "model": "claude/claude-sonnet-5", - "tools": ["Read", "Grep", "Glob"], + "tools": [ + "Read", + "Grep", + "Glob" + ], "prompt": "You are the independent OSH scorer. You did not find the issues. For each finding from coverage/tracer/history/intent, rate confidence 0/25/50/75/100 and severity_confirmed P0-P3. Return JSON: {role:\"scorer\", scores:[{id, confidence, severity_confirmed}]}." } } diff --git a/ai-review/osh-policy-fanout.md b/ai-review/osh-policy-fanout.md index 3094de4..68d8ec3 100644 --- a/ai-review/osh-policy-fanout.md +++ b/ai-review/osh-policy-fanout.md @@ -50,6 +50,16 @@ range is satisfied when coverage workers' `assigned_files` union equals `changed_files` (plus targeted neighbor reads when prior findings or imports require it). +## Worker failures (fail closed, do not burn turns) + +If any Task returns an API/tool error, empty result, or times out: + +- **Do not** re-spawn the same worker more than once. +- Prefer emitting structured output with what you have: lower confidence on + uncovered paths, and add a P1 finding that names the failed role(s). +- Never loop on Bash/gh retries hoping a dead worker recovers — that burns + `--max-turns` and yields `error_max_turns` with no Publishable SO. + ## Your job after workers return Resolve conflicts, prioritize, own intent, map Test Plan↔CI gaps to findings, diff --git a/docs/adr/0006-test-plan-ci-findings-and-osh-routing.md b/docs/adr/0006-test-plan-ci-findings-and-osh-routing.md index ceb2b70..b375ecb 100644 --- a/docs/adr/0006-test-plan-ci-findings-and-osh-routing.md +++ b/docs/adr/0006-test-plan-ci-findings-and-osh-routing.md @@ -50,7 +50,7 @@ in `ai-review/README.md`). | K | Mode | Review session | | --- | --- | --- | | ≤1 (incl. empty-diff `0`) | `collapse` | Single Sonnet + `--json-schema` | -| >1 (max 4) | `fanout` | Opus parent + native Sonnet/Haiku subagents; only Opus emits structured output | +| >1 (max 4) | `fanout` | Opus parent + Sonnet Task workers (`osh-*` agents); only Opus emits structured output | `sonnet-files-threshold` / `sonnet-churn-threshold` remain accepted but are **deprecated for model routing**. diff --git a/docs/plan.md b/docs/plan.md index 7a3c659..5b4ba14 100644 --- a/docs/plan.md +++ b/docs/plan.md @@ -29,7 +29,7 @@ Today `ai-review`/`ai-qa` are autopilot **skills** a developer runs in a local C - **Public & generalized:** the repo is **public** and org-agnostic. No action ships an eduly-specific default — eduly values (App ID/key, runner label, health URL, test command, staging URL) are **caller-supplied**; the action's own defaults are neutral (`runner-label: ubuntu-latest`, no health-url/test-command default). READMEs, ADR 0001, and `docs/consumer-integration.md` are written generically with **eduly as one example consumer**, not the subject. A `LICENSE` is a **deferred, blocking decision**: a public repo with no license is "all rights reserved," so external reuse cannot begin until a license is chosen (see D12) — internal EdulyCom consumption is unaffected. - **Process:** all changes via PR (no direct push to protected branches); **merge commits, not squash**. TypeScript/no-`any` for any committed scripts; prefer inline `github-script`. Docs land in the same PR as their change. - **Secrets & variables (org-level, human-supplied):** every secret/var the actions consume is set **at the EdulyCom org level** (`gh secret set --org EdulyCom …` / `gh variable set --org EdulyCom …`, repo-visibility scoped to the consuming repos incl. `github-actions` itself so its `selftest.yml` can run). The **implementer never invents secret values**: at each point one is needed (Phase 1 selftest, Phase 4 eduly adoption) the implementer **pauses and asks the human for the value**, then sets it at org level. Org-level secrets: `ANTHROPIC_AUTH_TOKEN` (eduly gateway bearer), `MTM_BOT_APP_PRIVATE_KEY`. Org-level vars: `ANTHROPIC_BASE_URL` (eduly gateway URL), `MTM_BOT_APP_ID`, `RUNNER_LABEL`, `AI_REVIEW_LABEL` (+ model-ID overrides if a gateway aliases them). (`GITHUB_TOKEN` is auto-provided per repo; not set here.) -- **Model IDs** (locked in the action, not repo-var overridable): context `claude/claude-haiku-4-5-20251001`; **review routes by roster K** — K≤1 → `claude/claude-sonnet-5` collapse; K>1 → `claude/claude-opus-5` parent with native Sonnet/Haiku fan-out (see ADR 0006 / `ai-review/README.md`). Cascade on overload: Cursor (`claude/cursor/...`) then pinned OpenCode free models then `auto/best-free`. Inputs `sonnet-files-threshold` / `sonnet-churn-threshold` remain accepted but are **deprecated for model routing**. ai-qa uses locked Sonnet with the same Cursor→free fallback list. See `docs/superpowers/plans/2026-08-17-claude-cursor-free-model-cascade.md` / #51 and `docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md`. +- **Model IDs** (locked in the action, not repo-var overridable): context `claude/claude-haiku-4-5-20251001`; **review routes by roster K** — K≤1 → `claude/claude-sonnet-5` collapse; K>1 → `claude/claude-opus-5` parent with Sonnet Task fan-out (see ADR 0006 / `ai-review/README.md`). Cascade on overload: Cursor (`claude/cursor/...`) then pinned OpenCode free models then `auto/best-free`. Inputs `sonnet-files-threshold` / `sonnet-churn-threshold` remain accepted but are **deprecated for model routing**. ai-qa uses locked Sonnet with the same Cursor→free fallback list. See `docs/superpowers/plans/2026-08-17-claude-cursor-free-model-cascade.md` / #51 and `docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md`. - **Anthropic endpoint — caller-supplied, defaults to Anthropic:** `anthropic-base-url` is an **optional caller input**; when omitted the action leaves `ANTHROPIC_BASE_URL` unset so `claude-code-action` uses the **standard Anthropic endpoint** (no gateway baked in — keeps the action generic per D12). When the caller passes it (eduly → its **custom gateway**), the value is plumbed to **every** `claude-code-action` invocation — Haiku context, Sonnet/Opus review, ai-qa triage — together with the caller's auth: either `anthropic-api-key` (→ `ANTHROPIC_API_KEY`/`x-api-key`) or `anthropic-auth-token` (→ `ANTHROPIC_AUTH_TOKEN` bearer). Model IDs are locked in the action with a Claude→Cursor→free `--fallback-model` cascade rather than caller overrides. ## Locked decisions diff --git a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md index f827d1e..211be3b 100644 --- a/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md +++ b/docs/superpowers/specs/2026-08-17-ai-review-osh-delta-testplan-design.md @@ -31,7 +31,7 @@ already specified — without replacing how findings are scored or gated. The 2026-08-07 document remains the source of truth for: - Binding constraints (§1): **rubric scoring stays**; **must read all** for the active review range. -- Coverage / coherence / intent / history roles, `K = clamp(ceil(bytes / BUDGET), 1, 4)`, cluster +- Coverage / coherence / intent / history roles, `K = clamp(max(ceil(bytes / BUDGET), ceil(files / 20)), 1, 4)`, cluster packing, independent Haiku scoring, deterministic aggregation. - Fail-closed matrix and frozen artifact shapes (`assignments.json`, per-role findings, gate input). - No Node `Promise.race` orchestrator — concurrency is Claude Code’s subagent scheduler inside one @@ -48,7 +48,7 @@ Where the two conflict on product intent, treat this document as the later refin Test-Plan/CI, and orchestration *delivery*; treat 2026-08-07 as authoritative for scoring vocabulary, must-read-all, roster math, and aggregation. **Exception (K=1 collapse):** §5.2 of this document overrides parallel design §5’s minimum roster `{R1,H}` — the collapse path is a single Sonnet session -with no Opus parent and no independent Haiku scorer (see §5.2). +with no Opus parent and no independent scorer Task (see §5.2). --- @@ -56,7 +56,7 @@ with no Opus parent and no independent Haiku scorer (see §5.2). | Decision | Choice | |---|---| -| Orchestration | Opus parent + native Sonnet/Haiku subagents in one `claude-code-action` invocation | +| Orchestration | Opus parent + Sonnet Task workers (`osh-*` agents) in one `claude-code-action` invocation | | Delta baseline | Commits after last published `` with meta `head_sha` | | Full review when | First run, missing meta, inconclusive prior, force-push/non-ancestor, base change, `force-full-review` | | Test plan | Map to CI; gaps → findings P0–P3; stop ticking checklist boxes in PR body | @@ -123,7 +123,7 @@ flowchart TD |---|---|---| | **O**pus (parent) | One `claude-code-action` session with `--model` Opus when fan-out is live | Intent isolation ownership (or dispatch of the intent role), prioritization, conflict resolution across worker outputs, final structured judgment (`comment_markdown` + schema fields). Does **not** exhaustively re-read every file on large diffs when workers already covered them. | | **S**onnet (workers) | Coverage cluster reviewers R1..Rk from `roster.js` / `assignments.json`; tracer / coherence as in 2026-08-07 §4 | Full-file reads of assigned paths; propose findings with severity. | -| **H**aiku (helpers) | History / mechanical gathers + **independent confidence scoring** | Cheap collection and scoring that must not be the same model that found the issue (parallel design §3). | +| **H**elpers (Sonnet Task) | History / mechanical gathers + **independent confidence scoring** (agents `osh-history`, `osh-scorer`) | Cheap collection and scoring that must not be the same model session that found the issue (parallel design §3). Haiku is **not** used as a Task child (adaptive-thinking 400 on this gateway). | Only the **Opus parent** emits `--json-schema` structured output for Publish. Workers return freeform or JSON **via Task results** (no unscopeable Write on the review allowlist); the parent aggregates into @@ -136,21 +136,23 @@ counts are never trusted as gate inputs. `K` is still the read-budget from parallel design §5: ``` -K = clamp(ceil(total_fullfile_bytes / BUDGET), 1, 4) +K = clamp(max(ceil(total_fullfile_bytes / BUDGET), ceil(file_count / 20)), 1, 4) ``` -Cap remains **K≤4**. Fan-out is an option when the work exceeds one reviewer’s comprehension budget, -not a fixed pipeline every PR pays for. +(`BUDGET` = 130 KiB; file axis matches `roster.js` `FILES_PER_REVIEWER`.) Cap remains **K≤4**. +Fan-out is an option when the work exceeds one reviewer’s comprehension budget, not a fixed +pipeline every PR pays for. **Collapse rule (this design — overrides parallel design §5 minimum `{R1,H}`):** - If the roster / `assignments.json` implies **K=1** (single coverage reviewer holds the whole active-range byte budget): run a **single Sonnet** review session that emits `--json-schema` - structured output directly — **no Opus parent**, **no independent Haiku scorer**. Artifact + structured output directly — **no Opus parent**, **no independent scorer Task**. Artifact contracts may match today’s single-session shape (not full findings/scores fan-in). Fail-closed path unchanged; lower cost. -- If **K>1**: Opus parent + native Sonnet/Haiku subagents consuming `assignments.json`. Independent - Haiku scoring applies only on this path (finder ≠ scorer), per parallel design §3 / §7b. +- If **K>1**: Opus parent + native Sonnet Task subagents (`osh-*` via `--agents`) consuming + `assignments.json`. Independent scoring (`osh-scorer`) applies only on this path (finder ≠ + scorer), per parallel design §3 / §7b. Prep always emits the manifest and roster. For **K>1**, topology still collapses by roster size inside the Opus parent; for **K=1**, the topology is the collapsed single-session path above (not @@ -270,7 +272,7 @@ stable enough that fan-out is not the first multiplier on an unfinished baseline |---|---|---| | **1 — Delta** | Meta marker, baseline resolver, manifest/prompt range wiring, `force-full-review` | Immediate token and wall-clock win on re-runs; works with today’s serial review | | **2 — Test-Plan/CI** | CI inventory + items artifacts; findings for gaps; stop checklist ticks | Honest Test Plan signal without waiting on OSH | -| **3 — OSH fan-out** | Opus parent + Sonnet/Haiku subagents from roster; K=1 collapse; deprecate size-based model routing | Largest stall-surface change; ship after cheaper slices reduce how often / how large full reviews are | +| **3 — OSH fan-out** | Opus parent + Sonnet Task subagents from roster; K=1 collapse; deprecate size-based model routing | Largest stall-surface change; ship after cheaper slices reduce how often / how large full reviews are | Follow-up docs (README / plan / short ADR note that checklist tick write-back is retired) land after the slices or alongside Slice 2–3 as consumer-facing copy catches up. From 9f8ee34dea63643df5536ba5647d945cd0058555 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 18:06:15 +0300 Subject: [PATCH 15/16] ci(selftest): raise ai-review job timeout to 55m Fan-out review regularly burns ~27m before SO; the 30m job ceiling was cancelling mid-Opus and publishing inconclusive. Co-authored-by: Cursor --- .github/workflows/selftest.yml | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/selftest.yml b/.github/workflows/selftest.yml index b7afcc6..b1860a5 100644 --- a/.github/workflows/selftest.yml +++ b/.github/workflows/selftest.yml @@ -19,7 +19,9 @@ concurrency: jobs: review: runs-on: ubuntu-latest - timeout-minutes: 30 + # Fan-out (Opus + Sonnet Tasks) regularly exceeds 25m wall-clock; 30m was + # cancelling mid-review before Publish. Budget covers review + repair/retry. + timeout-minutes: 55 permissions: contents: read pull-requests: write From 86fcdfd46fafc6af2784616df847b01d33387f27 Mon Sep 17 00:00:00 2001 From: Hussam Aldarwish Date: Mon, 17 Aug 2026 18:12:57 +0300 Subject: [PATCH 16/16] revert(ai-review): drop collaborator-permission bypass for GitHub flakiness Remove allowed-non-write-users (selftest + action wiring) and the CCA subprocess scrub opt-out that only existed to make that bypass safe. Keep the 55m selftest timeout for real Opus fan-out wall-clock. Co-authored-by: Cursor --- .github/workflows/selftest.yml | 5 ----- ai-review/action.yml | 32 -------------------------------- 2 files changed, 37 deletions(-) diff --git a/.github/workflows/selftest.yml b/.github/workflows/selftest.yml index b1860a5..d3600ba 100644 --- a/.github/workflows/selftest.yml +++ b/.github/workflows/selftest.yml @@ -68,11 +68,6 @@ jobs: private-key: ${{ secrets.MTM_BOT_APP_PRIVATE_KEY }} anthropic-auth-token: ${{ secrets.ANTHROPIC_AUTH_TOKEN }} anthropic-base-url: ${{ vars.ANTHROPIC_BASE_URL }} - # Skip collaborator permission API (has been 503ing and aborting - # Claude stages before the model runs). Scoped to this PR author, - # not "*". Context/Review use read-scoped github.token; Publish - # posts with the App token. - allowed-non-write-users: eduly-haldarwish gated-demo: needs: [review] diff --git a/ai-review/action.yml b/ai-review/action.yml index 8edbfb2..947e238 100644 --- a/ai-review/action.yml +++ b/ai-review/action.yml @@ -54,18 +54,6 @@ inputs: controls — scope this to named bots, not `'*'`. required: false default: "" - allowed-non-write-users: - description: > - Passed through to `claude-code-action`'s `allowed_non_write_users`. - Comma-separated usernames (or `*`) allowed to run without a successful - collaborator write-permission lookup. Only takes effect when - `github-token` is provided (the default). Use when that GitHub API is - flaky (503) or for workflows whose Claude stages already use a - read-scoped token and never post via that token. WARNING: bypasses a - security check — prefer named users; `*` only in tightly scoped - callers (e.g. this repo's selftest). - required: false - default: "" pr-number: description: > Pull request number to review. Optional — defaults to the @@ -632,10 +620,6 @@ runs: ANTHROPIC_BASE_URL: ${{ inputs.anthropic-base-url }} API_TIMEOUT_MS: ${{ inputs.api-timeout-ms }} ANTHROPIC_CUSTOM_HEADERS: "${{ inputs.anthropic-auth-token != '' && format('Authorization: Bearer {0}', inputs.anthropic-auth-token) || '' }}" - # allowed_non_write_users enables CCA subprocess isolation by - # default; that writes an invalid root package.json and breaks - # later node requires (Publish/telemetry). Opt out. - CLAUDE_CODE_SUBPROCESS_ENV_SCRUB: "0" with: anthropic_api_key: ${{ inputs.anthropic-api-key != '' && inputs.anthropic-api-key || inputs.anthropic-auth-token }} # Deliberately NOT steps.identity.outputs.author-token: this stage only @@ -645,7 +629,6 @@ runs: # steps later in this action. github_token: ${{ inputs.github-token }} allowed_bots: ${{ inputs.allowed-bots }} - allowed_non_write_users: ${{ inputs.allowed-non-write-users }} # Root cause of the context.md handoff failure (Task 2.4 fix round): this # step ran with NO --allowedTools at all, so claude-code-action's headless # permission model silently denied every Write and most Bash tool calls @@ -841,10 +824,6 @@ runs: ANTHROPIC_BASE_URL: ${{ inputs.anthropic-base-url }} API_TIMEOUT_MS: ${{ inputs.api-timeout-ms }} ANTHROPIC_CUSTOM_HEADERS: "${{ inputs.anthropic-auth-token != '' && format('Authorization: Bearer {0}', inputs.anthropic-auth-token) || '' }}" - # allowed_non_write_users enables CCA subprocess isolation by - # default; that writes an invalid root package.json and breaks - # later node requires (Publish/telemetry). Opt out. - CLAUDE_CODE_SUBPROCESS_ENV_SCRUB: "0" PR_NUMBER: ${{ steps.pr.outputs.pr-number }} OSH_MODE: ${{ steps.route.outputs.osh-mode }} with: @@ -859,7 +838,6 @@ runs: # Derive). The App-minted identity is reserved for Publish below. github_token: ${{ inputs.github-token }} allowed_bots: ${{ inputs.allowed-bots }} - allowed_non_write_users: ${{ inputs.allowed-non-write-users }} plugins: superpowers@superpowers-marketplace plugin_marketplaces: https://github.com/obra/superpowers-marketplace.git claude_args: | @@ -1135,15 +1113,10 @@ runs: ANTHROPIC_BASE_URL: ${{ inputs.anthropic-base-url }} API_TIMEOUT_MS: ${{ inputs.api-timeout-ms }} ANTHROPIC_CUSTOM_HEADERS: "${{ inputs.anthropic-auth-token != '' && format('Authorization: Bearer {0}', inputs.anthropic-auth-token) || '' }}" - # allowed_non_write_users enables CCA subprocess isolation by - # default; that writes an invalid root package.json and breaks - # later node requires (Publish/telemetry). Opt out. - CLAUDE_CODE_SUBPROCESS_ENV_SCRUB: "0" with: anthropic_api_key: ${{ inputs.anthropic-api-key != '' && inputs.anthropic-api-key || inputs.anthropic-auth-token }} github_token: ${{ inputs.github-token }} allowed_bots: ${{ inputs.allowed-bots }} - allowed_non_write_users: ${{ inputs.allowed-non-write-users }} claude_args: | --resume ${{ steps.retry_budget.outputs.session_id }} --model ${{ steps.route.outputs.model }} @@ -1227,17 +1200,12 @@ runs: ANTHROPIC_BASE_URL: ${{ inputs.anthropic-base-url }} API_TIMEOUT_MS: ${{ inputs.api-timeout-ms }} ANTHROPIC_CUSTOM_HEADERS: "${{ inputs.anthropic-auth-token != '' && format('Authorization: Bearer {0}', inputs.anthropic-auth-token) || '' }}" - # allowed_non_write_users enables CCA subprocess isolation by - # default; that writes an invalid root package.json and breaks - # later node requires (Publish/telemetry). Opt out. - CLAUDE_CODE_SUBPROCESS_ENV_SCRUB: "0" PR_NUMBER: ${{ steps.pr.outputs.pr-number }} OSH_MODE: ${{ steps.route.outputs.osh-mode }} with: anthropic_api_key: ${{ inputs.anthropic-api-key != '' && inputs.anthropic-api-key || inputs.anthropic-auth-token }} github_token: ${{ inputs.github-token }} allowed_bots: ${{ inputs.allowed-bots }} - allowed_non_write_users: ${{ inputs.allowed-non-write-users }} plugins: superpowers@superpowers-marketplace plugin_marketplaces: https://github.com/obra/superpowers-marketplace.git claude_args: |