diff --git a/.github/workflows/agent-review.yml b/.github/workflows/agent-review.yml index 96c58366c32..b5b9afe6d35 100644 --- a/.github/workflows/agent-review.yml +++ b/.github/workflows/agent-review.yml @@ -3,20 +3,25 @@ # WHAT THIS DOES (and does NOT do): # Fires when a human applies the `agent:review` label to a PULL REQUEST. It # runs the single-pass reviewer (`pnpm sandcastle:review`) against the PR's -# head branch, pushing any clarity/standards refinements back onto the PR. It -# NEVER merges the PR and NEVER closes anything — a human still merges. This is -# the single-pass replacement for the old 4-round `review-round:*` loop. +# head branch, pushing any clarity/standards refinements back onto the PR, +# and requires a structured verdict (toon-meta#275): the reviewer resolves the +# PR's target issue from its body's `Closes #n`, reviews against the issue's +# acceptance criteria, and must emit {"verdict":"clean"|"blocking", +# ...} — a malformed verdict FAILS the job. On "blocking" the runner +# posts the findings as a PR review and applies `needs:human` (both via the +# App token below — this workflow's own GITHUB_TOKEN stays read-only). It +# NEVER merges the PR and NEVER closes anything — a human still merges. This +# is the single-pass replacement for the old 4-round `review-round:*` loop. # # Committing this file triggers nothing: pull_request-`labeled` workflows fire -# only from the default branch and only when someone applies the label. +# only when someone applies the label. # -# VERIFY ON FIRST RUN — the standalone-review path: -# Sandcastle 0.12.0 only exercises the reviewer inside its parallel loop, on a -# fresh branch it just created. Running it standalone against an existing PR -# head branch is our interpretation (see .sandcastle/agent-review-pr.ts). On -# the first live run confirm: (1) the sandbox checks out the existing PR head -# rather than erroring on the ref, and (2) review-prompt.md's built-in -# {{TARGET_BRANCH}} resolves to `main` so the diff is non-empty. +# STANDALONE-REVIEW MECHANICS (proven live on connector#634's first run): +# sandcastle checks the PR head branch out in its OWN worktree under +# .sandcastle/worktrees/, and git refuses the same branch in two worktrees — +# so the checkout below pins `ref: main`, never the PR head. The runner then +# materialises the PR head as a local branch itself (see +# .sandcastle/agent-review-pr.ts). # # Note: label events on PRs opened from FORKS run without secrets (GitHub # security), so this runner only works on same-repo PRs. That is expected for @@ -83,8 +88,14 @@ jobs: - uses: actions/checkout@v4 with: fetch-depth: 0 - # Check out the PR head so the reviewer operates on the right branch. - ref: ${{ github.event.pull_request.head.ref }} + # Check out MAIN, not the PR head: sandcastle's branch strategy checks + # the PR head out in its own worktree under .sandcastle/worktrees/, + # and git refuses to check out one branch in two worktrees at once + # (WorktreeError "already checked out" — connector#634's first live + # run). The runner fetches the PR head into a local branch itself, and + # review-prompt.md's {{TARGET_BRANCH}} resolves to this checkout + # (main), giving the right diff base. + ref: main - uses: actions/setup-node@v4 with: diff --git a/.sandcastle/agent-implement-issue.ts b/.sandcastle/agent-implement-issue.ts index 06049df1ea5..89cfbec0446 100644 --- a/.sandcastle/agent-implement-issue.ts +++ b/.sandcastle/agent-implement-issue.ts @@ -41,6 +41,10 @@ import { execFileSync } from "node:child_process"; import * as sandcastle from "@ai-hero/sandcastle"; import { docker } from "@ai-hero/sandcastle/sandboxes/docker"; +import { + postBlockingVerdict, + runReviewerWithVerdict, +} from "./review-verdict.ts"; import { sandboxSecrets } from "./sandbox-secrets.ts"; // --------------------------------------------------------------------------- @@ -163,18 +167,30 @@ try { process.exit(0); } - // Review (opus, 1 iteration) on the SAME branch. The engine supplies the - // built-in {{TARGET_BRANCH}} used inside review-prompt.md, so we pass only - // BRANCH (mirrors main.ts). - await sandbox.run({ - name: "reviewer", - maxIterations: 1, - agent: sandcastle.claudeCode("claude-opus-5"), - promptFile: "./.sandcastle/review-prompt.md", - promptArgs: { BRANCH: branch }, + // Review (opus, 1 iteration) on the SAME branch, with the structured + // verdict REQUIRED (toon-meta#275): the reviewer receives the issue via + // promptArgs (Spec axis — it reviews against the issue's acceptance + // criteria, not just the diff) and must emit + // {"verdict":"clean"|"blocking","blockingFindings":[...]}. + // A malformed verdict fails the run (one engine-style resume retry, then + // non-zero exit) — see ./review-verdict.ts. The engine supplies the + // built-in {{TARGET_BRANCH}} used inside review-prompt.md. + const review = await runReviewerWithVerdict(sandbox, { + branch, + issue: { number: issueNumber, title: issueTitle }, }); + const blocking = review.verdict.verdict === "blocking"; - if (autoMerge) { + if (autoMerge && blocking) { + // A blocking verdict must never be auto-merged: fall through to PR mode + // so the findings land on a PR for a human instead (toon-meta#275). + console.log( + "\nAuto-merge requested, but the reviewer verdict is BLOCKING — " + + "falling back to PR mode so a human decides.", + ); + } + + if (autoMerge && !blocking) { // RE-ENABLE path: merge this one branch into the checked-out base and close // the issue, using the stock merge prompt scoped to the single branch. console.log("\nAuto-merge enabled — merging branch and closing issue."); @@ -222,6 +238,14 @@ try { if (openPrs.length > 0) { const pr = openPrs[0]!; console.log(`\nVerified: PR #${pr.number} is open — ${pr.url}`); + // A blocking verdict lands on the PR now that it exists: findings as a + // PR review, plus the `needs:human` label (toon-meta#275). + if (blocking) { + postBlockingVerdict(String(pr.number), review.verdict, { + number: issueNumber, + title: issueTitle, + }); + } console.log("Awaiting human review."); } else { // No open PR. Gather diagnostics (all via the authenticated host `gh`). diff --git a/.sandcastle/agent-review-pr.ts b/.sandcastle/agent-review-pr.ts index 7626f6a31b6..4a1fb7407ce 100644 --- a/.sandcastle/agent-review-pr.ts +++ b/.sandcastle/agent-review-pr.ts @@ -3,29 +3,36 @@ // applied to ONE pull request. // // This is the single-pass replacement for the old 4-round `review-round:*` -// reviewer loop. It runs the reviewer role (review-prompt.md — refactor for -// clarity while preserving behavior, enforce CODING_STANDARDS.md) against the -// PR's head branch, and pushes any refinement commits back to the PR. It NEVER -// merges the PR and NEVER closes anything — a human still merges. +// reviewer loop. It runs the reviewer role (review-prompt.md — two axes: +// Standards refinement + Spec review against the PR's target issue) against +// the PR's head branch, pushes any refinement commits back to the PR, and +// REQUIRES a structured verdict (toon-meta#275): +// - the reviewer must emit {"verdict":"clean"|"blocking", +// "blockingFindings":[{file,line,summary,why}]}; a malformed +// verdict fails the run (one engine-style resume retry, then non-zero exit) +// - on "blocking", the findings are posted as a PR review and the +// `needs:human` label is applied +// It NEVER merges the PR and NEVER closes anything — a human still merges. // -// STANDALONE-REVIEW CAVEAT (verify on first run) -// ---------------------------------------------- -// Sandcastle 0.12.0 exercises the reviewer only INSIDE the parallel loop's -// Phase 2, on a fresh `sandcastle/issue-*` branch it just created. Driving the -// same reviewer standalone against an already-existing PR head branch is our -// interpretation, not a documented engine feature. Two things to confirm on the -// first live run: -// 1. createSandbox({ branch: }) checks out the EXISTING -// branch (rather than failing because the ref already exists / creating a -// divergent one). The workflow checks out the PR head first to help this. -// 2. The built-in {{TARGET_BRANCH}} inside review-prompt.md resolves to `main` -// for a standalone sandbox. If the diff comes back empty, the base may be -// resolving wrong — check the reviewer's logged `git diff` command. +// STANDALONE-REVIEW MECHANICS (proven live on connector#634's first run): +// Sandcastle checks the PR head branch out in its OWN worktree under +// .sandcastle/worktrees/, and git refuses one branch in two worktrees — so +// the workflow checks out MAIN, never the PR head. Because the local clone +// is then on main, this runner materialises the PR head as a LOCAL branch +// (git fetch origin +head:head) before createSandbox(): without it the +// engine's `worktree add` falls back to `-b HEAD`, silently +// reviewing an EMPTY diff off main. review-prompt.md's {{TARGET_BRANCH}} +// resolves to the checked-out branch (main), so the diff base is right. +// +// The target issue for the Spec axis is resolved from the PR body's +// `Closes #n` (the implement runner writes one into every factory PR body). +// PRs without a closing reference get a Standards-only review. // // Required env: // SANDCASTLE_PR_NUMBER the PR to review (github.event.pull_request.number) // CLAUDE_CODE_OAUTH_TOKEN Claude Max-plan credential (org secret) -// GH_TOKEN token with contents:write + pull-requests:write +// GH_TOKEN token with contents:write + pull-requests:write + +// issues:write (labels) // // Usage: // SANDCASTLE_PR_NUMBER=42 npx tsx .sandcastle/agent-review-pr.ts @@ -34,6 +41,12 @@ import { execFileSync } from "node:child_process"; import * as sandcastle from "@ai-hero/sandcastle"; import { docker } from "@ai-hero/sandcastle/sandboxes/docker"; +import { + postBlockingVerdict, + resolveIssueFromPrBody, + type ReviewVerdict, + runReviewerWithVerdict, +} from "./review-verdict.ts"; import { sandboxSecrets } from "./sandbox-secrets.ts"; const prNumber = process.env.SANDCASTLE_PR_NUMBER?.trim(); @@ -55,6 +68,21 @@ if (!headRef) { throw new Error(`Could not resolve head branch for PR #${prNumber}.`); } +// Materialise the PR head as a local branch at origin's tip (the host clone is +// on main — see the standalone-review mechanics note above). Forced so a +// re-labeled PR re-reviews the CURRENT head even after a force-push. +execFileSync("git", ["fetch", "origin", `+${headRef}:${headRef}`], { + stdio: "inherit", +}); + +// Resolve the Spec-axis target issue from the PR body's `Closes #n`. +const targetIssue = resolveIssueFromPrBody(prNumber); +console.log( + targetIssue + ? `Spec axis target: issue #${targetIssue.number} — ${targetIssue.title}` + : "No `Closes #n` in the PR body — Standards-only review.", +); + const hooks = { sandbox: { onSandboxReady: [ @@ -92,19 +120,19 @@ const sandbox = await sandcastle.createSandbox({ branch: headRef, // Forward CLAUDE_CODE_OAUTH_TOKEN + GH_TOKEN into the container (the engine's // env resolver does not — see ./sandbox-secrets.ts). GH_TOKEN is what the - // review-push step's in-sandbox `git push` to the PR branch authenticates with. + // review-push step's in-sandbox `git push` to the PR branch authenticates + // with, and what the reviewer's in-sandbox `gh issue view` (Spec axis) reads. sandbox: docker({ env: sandboxSecrets() }), hooks, }); +let verdict: ReviewVerdict; try { - const review = await sandbox.run({ - name: "reviewer", - maxIterations: 1, - agent: sandcastle.claudeCode("claude-opus-5"), - promptFile: "./.sandcastle/review-prompt.md", - promptArgs: { BRANCH: headRef }, + const review = await runReviewerWithVerdict(sandbox, { + branch: headRef, + issue: targetIssue, }); + verdict = review.verdict; if (review.commits.length > 0) { // Push the reviewer's refinement commits back onto the PR branch. No merge, @@ -171,14 +199,21 @@ try { `deliberately so this is not mistaken for success.`; } } else { - console.log( - "\nReviewer made no changes — the code was already clean. Nothing to push.", - ); + console.log("\nReviewer made no changes — nothing to push."); } } finally { await sandbox.close(); } +// The verdict's side effects run AFTER the sandbox is closed, from the +// authenticated host: findings must land on the PR even if the push +// verification below is about to fail the job. +if (verdict.verdict === "blocking") { + postBlockingVerdict(prNumber, verdict, targetIssue); +} else { + console.log("\nVerdict clean — no blocking findings."); +} + // Fail loud AFTER the sandbox is closed: a silently-failed review push must turn // the Actions job red, never green. if (reviewPushError) { diff --git a/.sandcastle/main.ts b/.sandcastle/main.ts index 311feef29e4..239f14551db 100644 --- a/.sandcastle/main.ts +++ b/.sandcastle/main.ts @@ -164,6 +164,13 @@ for (let iteration = 1; iteration <= MAX_ITERATIONS; iteration++) { // Only review if the implementer produced commits if (implement.commits.length > 0) { + // review-prompt.md now requires ISSUE_NUMBER/ISSUE_TITLE (the Spec + // axis, toon-meta#275) — an unresolved {{...}} placeholder fails the + // run, so pass them here too. This reserved autonomous loop does not + // yet CONSUME the reviewer's verdict; the label runners + // (agent-implement-issue.ts / agent-review-pr.ts) enforce it via + // ./review-verdict.ts, and wiring it into this merge phase is part + // of the auto-merge work (toon-meta#270). const review = await sandbox.run({ name: "reviewer", maxIterations: 1, @@ -171,6 +178,8 @@ for (let iteration = 1; iteration <= MAX_ITERATIONS; iteration++) { promptFile: "./.sandcastle/review-prompt.md", promptArgs: { BRANCH: issue.branch, + ISSUE_NUMBER: issue.id, + ISSUE_TITLE: issue.title, }, }); diff --git a/.sandcastle/review-prompt.md b/.sandcastle/review-prompt.md index f209983dbde..1ca51a179aa 100644 --- a/.sandcastle/review-prompt.md +++ b/.sandcastle/review-prompt.md @@ -1,9 +1,29 @@ # TASK -Review the code changes on branch `{{BRANCH}}` and improve code clarity, consistency, and maintainability while preserving exact functionality. +Review the code changes on branch `{{BRANCH}}` along two axes, then deliver a +structured verdict: + +1. **Standards** — improve code clarity, consistency, and maintainability while + preserving exact functionality. +2. **Spec** — does the change actually satisfy the issue it targets and its + acceptance criteria? # CONTEXT +## Target issue + +Issue: #{{ISSUE_NUMBER}} — {{ISSUE_TITLE}} + +If the issue number above is `none`, no target issue could be resolved for this +branch; skip the Spec axis and review the Standards axis only. + +Otherwise, BEFORE reading the diff, run: + + gh issue view {{ISSUE_NUMBER}} + +and read the issue body — especially its acceptance criteria. The Spec axis +reviews the diff AGAINST THOSE CRITERIA, not against the diff itself. + ## Branch diff !`git diff {{TARGET_BRANCH}}...{{BRANCH}}` @@ -14,9 +34,15 @@ Review the code changes on branch `{{BRANCH}}` and improve code clarity, consist # REVIEW PROCESS -1. **Understand the change**: Read the diff and commits above to understand the intent. +1. **Understand the change**: Read the issue (above) and the diff and commits + to understand the intent. + +2. **Spec axis**: Check the diff against the target issue: + - Does the change actually resolve what the issue asked for? + - Is every acceptance criterion met by the diff (not merely claimed)? + - Does anything in the diff contradict the issue? -2. **Analyze for improvements**: Look for opportunities to: +3. **Analyze for improvements** (Standards axis): Look for opportunities to: - Reduce unnecessary complexity and nesting - Eliminate redundant code and abstractions - Improve readability through clear variable and function names @@ -25,34 +51,74 @@ Review the code changes on branch `{{BRANCH}}` and improve code clarity, consist - Avoid nested ternary operators - prefer switch statements or if/else chains - Choose clarity over brevity - explicit code is often better than overly compact code -3. **Check correctness**: - - Does the implementation match the intent? Are edge cases handled? +4. **Check correctness**: + - Are edge cases handled? - Are new/changed behaviours covered by tests? - Are there unsafe casts, `any` types, unchecked `unwrap()`/`expect()` on fallible paths, or unchecked assumptions? - Does the change introduce injection vulnerabilities, credential leaks, or other security issues? -4. **Maintain balance**: Avoid over-simplification that could: +5. **Maintain balance**: Avoid over-simplification that could: - Reduce code clarity or maintainability - Create overly clever solutions that are hard to understand - Combine too many concerns into single functions or components - Remove helpful abstractions that improve code organization - Make the code harder to debug or extend -5. **Apply project standards**: Follow the coding standards defined in @.sandcastle/CODING_STANDARDS.md +6. **Apply project standards**: Follow the coding standards defined in @.sandcastle/CODING_STANDARDS.md + +7. **Preserve functionality**: Never change what the code does - only how it does it. All original features, outputs, and behaviors must remain intact. + +# WHAT YOU FIX vs WHAT IS BLOCKING -6. **Preserve functionality**: Never change what the code does - only how it does it. All original features, outputs, and behaviors must remain intact. +Fix yourself (Standards axis): presentation-level issues — naming, structure, +readability, redundant code, formatting, inconsistency with neighbouring code — +that preserve exact behaviour. + +BLOCKING (report in the verdict, do NOT rewrite): a defect that makes the +change wrong to merge — + +- the diff fails or contradicts the target issue's acceptance criteria +- the diff introduces a functional defect or breaks the gate +- the diff asserts something factually wrong (a comment, doc, or test that + misstates the actual behaviour) that you cannot verify a fix for + +Never rewrite the substance of the change to force a pass. Substance defects +are the author's to fix; your channel for them is the verdict below. # EXECUTION -If you find improvements to make: +If you find Standards improvements to make: 1. Make the changes directly on this branch 2. Run buzz's gate for the side(s) the diff touches to ensure nothing is broken — Rust: `just fmt-check`, `just desktop-tauri-fmt-check`, `just clippy`, `just test-unit`; JS: `just desktop-check`, `just desktop-test`, `just desktop-build`, `just web-check`, `just web-build`. (Flutter/mobile, `desktop/src-tauri` compile checks, e2e, and infra-backed integration tests are out of scope in the sandbox — upstream `ci.yml` covers them on the PR.) 3. Commit describing the refinements -If the code is already clean and well-structured, do nothing. +If the code is already clean and well-structured, make no commits. + +# REQUIRED VERDICT (structured output) + +You MUST end your final message with exactly one verdict block. The block is +machine-parsed; a missing or malformed block FAILS the run. Emit it even when +you made no changes. + +Format — JSON only inside the tag, no comments, no trailing commas, no code +fences: + + +{"verdict":"clean","blockingFindings":[]} + + +Rules: + +- `verdict` is `"clean"` or `"blocking"` — nothing else. +- `blockingFindings` MUST be empty for `clean` and non-empty for `blocking`. +- Each finding is + `{"file":"","line":<1-based number or null>,"summary":"","why":""}` + — `line` is `null` for file-level findings. +- Standards fixes you already made are NOT findings; findings are only the + blocking defects defined above. -Once complete, output COMPLETE. +Once complete, output the verdict block and then COMPLETE. ## Context budget diff --git a/.sandcastle/review-verdict.ts b/.sandcastle/review-verdict.ts new file mode 100644 index 00000000000..04aae079c84 --- /dev/null +++ b/.sandcastle/review-verdict.ts @@ -0,0 +1,374 @@ +// Reviewer verdict channel (toon-meta#275) — shared by the implement runner's +// review phase (agent-implement-issue.ts) and the standalone review runner +// (agent-review-pr.ts). +// +// WHY THIS MODULE DOES ITS OWN EXTRACTION (verified against the published +// @ai-hero/sandcastle@0.12.0 dist): +// - The engine's structured-output surface — `Output.object({ tag, schema })`, +// fence-aware tag extraction, `StructuredOutputError`, `maxRetries` via +// session resume — exists ONLY on the top-level `run()` entry point +// (dist/index.js `run()`; see engine ADR 0010). The long-lived +// `sandbox.run()` the runners use accepts NO `output` option: the runtime +// never reads it and the option would be silently ignored. +// - We therefore declare the exact `Output.object` definition the ticket +// specifies (`reviewOutput` below) and consume its tag + schema OURSELVES, +// mirroring the engine's extraction semantics precisely: the LAST +// `…` block in the run's stdout wins, ```json fences are +// unwrapped, the contents are JSON-parsed and schema-validated. +// - The retry mirrors the engine's `maxRetries` design: on a malformed +// verdict, `SandboxRunResult.resume()` continues the reviewer's captured +// session for exactly one iteration with a token-efficient description of +// the parse/validation error (the engine's own structured-output retry +// mechanism). ONE retry, then FAIL: a malformed verdict must fail the job, +// never pass silently (toon-meta#275 acceptance criterion). +// - `resume()` gotcha: it spreads the ORIGINAL runOptions (including +// promptArgs) into an inline-prompt run, and the engine rejects non-empty +// promptArgs on inline prompts — so the retry passes `promptArgs: {}` +// explicitly. + +import { execFileSync } from "node:child_process"; +import * as sandcastle from "@ai-hero/sandcastle"; +import { z } from "zod"; + +// --------------------------------------------------------------------------- +// Schema + Output declaration +// --------------------------------------------------------------------------- + +export const reviewVerdictSchema = z + .object({ + verdict: z.enum(["clean", "blocking"]), + blockingFindings: z.array( + z.object({ + /** Repo-relative path of the file the finding is about. */ + file: z.string().min(1), + /** 1-based line number, or null for a file-level finding. */ + line: z.number().int().min(1).nullable(), + /** One-line description of the defect. */ + summary: z.string().min(1), + /** Why this blocks the merge (which criterion/behaviour it violates). */ + why: z.string().min(1), + }), + ), + }) + .superRefine((value, ctx) => { + if (value.verdict === "blocking" && value.blockingFindings.length === 0) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: "a blocking verdict requires at least one blockingFinding", + }); + } + if (value.verdict === "clean" && value.blockingFindings.length > 0) { + ctx.addIssue({ + code: z.ZodIssueCode.custom, + message: "a clean verdict must carry zero blockingFindings", + }); + } + }); + +export type ReviewVerdict = z.infer; + +/** + * The structured-output declaration toon-meta#275 specifies. `sandbox.run()` + * cannot consume it (see module header), so `extractReviewVerdict()` consumes + * its tag + schema instead. Keeping the declaration in the engine's own shape + * means the runners can hand it straight to top-level `run({ output })` if the + * engine ever exposes structured output on the sandbox surface. + */ +export const reviewOutput = sandcastle.Output.object({ + tag: "review", + schema: reviewVerdictSchema, +}); + +// --------------------------------------------------------------------------- +// Extraction — mirrors the engine's extractStructuredOutput() semantics +// --------------------------------------------------------------------------- + +/** Last `…` content in `text`, engine-identical scan. */ +function findLastTagContent(text: string, tag: string): string | undefined { + const openTag = `<${tag}>`; + const closeTag = ``; + let lastContent: string | undefined; + let searchFrom = 0; + for (;;) { + const openIdx = text.indexOf(openTag, searchFrom); + if (openIdx === -1) break; + const contentStart = openIdx + openTag.length; + const closeIdx = text.indexOf(closeTag, contentStart); + if (closeIdx === -1) break; + lastContent = text.slice(contentStart, closeIdx); + searchFrom = closeIdx + closeTag.length; + } + return lastContent; +} + +/** Engine-identical ```/```json fence unwrap. */ +function unwrapFences(text: string): string { + const fenceMatch = text.match(/^```(?:json)?\s*\n([\s\S]*?)\n\s*```\s*$/); + return fenceMatch ? fenceMatch[1]!.trim() : text; +} + +export type VerdictAttempt = + | { ok: true; verdict: ReviewVerdict } + | { ok: false; error: string }; + +/** Extract + validate the `` verdict from a reviewer run's stdout. */ +export function extractReviewVerdict(stdout: string): VerdictAttempt { + const raw = findLastTagContent(stdout, reviewOutput.tag); + if (raw === undefined) { + return { + ok: false, + error: `structured output tag <${reviewOutput.tag}> not found in the reviewer's output`, + }; + } + let parsed: unknown; + try { + parsed = JSON.parse(unwrapFences(raw.trim())); + } catch (cause) { + return { + ok: false, + error: + `tag <${reviewOutput.tag}> contains invalid JSON ` + + `(${cause instanceof Error ? cause.message : String(cause)}). ` + + `Matched content: ${raw.trim().slice(0, 500)}`, + }; + } + const result = reviewVerdictSchema.safeParse(parsed); + if (!result.success) { + return { + ok: false, + error: + `tag <${reviewOutput.tag}> failed schema validation: ` + + result.error.issues + .map((i) => `${i.path.join(".") || "(root)"}: ${i.message}`) + .join("; "), + }; + } + return { ok: true, verdict: result.data }; +} + +// --------------------------------------------------------------------------- +// Reviewer run with enforced verdict +// --------------------------------------------------------------------------- + +export interface TargetIssue { + /** Issue number as a string, e.g. "275". */ + number: string; + title: string; +} + +/** Mirrors the engine's buildStructuredOutputRetryFeedback() wording. */ +function buildRetryFeedback(error: string): string { + return ( + `Your previous response did not produce a valid structured review verdict.\n\n` + + `Retries remaining after this attempt: 0.\n\n` + + `Problem:\n${error}\n\n` + + `Emit only a corrected block — JSON matching\n` + + `{"verdict":"clean"|"blocking","blockingFindings":[{"file":string,` + + `"line":number|null,"summary":string,"why":string}]}\n` + + `— and nothing else. Do not change files or run commands.` + ); +} + +/** + * Run the reviewer (opus, single iteration) on `branch` and REQUIRE the + * structured verdict. On a malformed verdict, retries exactly once by resuming + * the reviewer's session (the engine's structured-output retry mechanism); if + * the verdict is still malformed, THROWS so the job fails rather than passing + * silently. + */ +export async function runReviewerWithVerdict( + sandbox: sandcastle.Sandbox, + options: { + branch: string; + /** null → no target issue; the prompt skips the Spec axis. */ + issue: TargetIssue | null; + promptFile?: string; + }, +): Promise<{ verdict: ReviewVerdict; commits: { sha: string }[] }> { + const first = await sandbox.run({ + name: "reviewer", + maxIterations: 1, + agent: sandcastle.claudeCode("claude-opus-5"), + promptFile: options.promptFile ?? "./.sandcastle/review-prompt.md", + promptArgs: { + BRANCH: options.branch, + ISSUE_NUMBER: options.issue?.number ?? "none", + ISSUE_TITLE: options.issue?.title ?? "(no target issue resolved)", + }, + }); + + const commits = [...first.commits]; + let attempt = extractReviewVerdict(first.stdout); + + if (!attempt.ok) { + console.warn( + `\nReviewer verdict malformed (${attempt.error}) — retrying once via session resume.`, + ); + if (!first.resume) { + throw new Error( + `Reviewer emitted a malformed verdict and the session cannot ` + + `be resumed (no captured session id) — failing the run. ` + + `Detail: ${attempt.error}`, + ); + } + // promptArgs MUST be reset: resume() re-spreads the original run options, + // and inline prompts reject non-empty promptArgs (see module header). + const retry = await first.resume(buildRetryFeedback(attempt.error), { + name: "reviewer-verdict-retry", + promptArgs: {}, + }); + commits.push(...retry.commits); + attempt = extractReviewVerdict(retry.stdout); + if (!attempt.ok) { + throw new Error( + `Reviewer verdict still malformed after one resume retry — failing ` + + `the run so a missing verdict is never mistaken for a clean one. ` + + `Detail: ${attempt.error}`, + ); + } + } + + const { verdict } = attempt; + console.log( + `\nReviewer verdict: ${verdict.verdict.toUpperCase()}` + + (verdict.blockingFindings.length > 0 + ? ` (${verdict.blockingFindings.length} blocking finding(s))` + : ""), + ); + return { verdict, commits }; +} + +// --------------------------------------------------------------------------- +// Host-side helpers (authenticated `gh` via GH_TOKEN) +// --------------------------------------------------------------------------- + +function gh(args: string[]): string { + return execFileSync("gh", args, { encoding: "utf8" }); +} + +function repoNwo(): string { + return gh([ + "repo", + "view", + "--json", + "nameWithOwner", + "--jq", + ".nameWithOwner", + ]).trim(); +} + +/** + * Resolve the target issue for a PR from its body's closing keyword + * (`Closes #n` / `Fixes #n` / `Resolves #n`, same-repo only — the convention + * the implement runner writes into every factory PR body). Returns null when + * no closing reference exists or the referenced issue cannot be read; the + * reviewer then skips the Spec axis rather than failing. + */ +export function resolveIssueFromPrBody(prNumber: string): TargetIssue | null { + const body = gh(["pr", "view", prNumber, "--json", "body", "--jq", ".body"]); + const match = body.match( + /\b(?:clos(?:e|es|ed)|fix(?:es|ed)?|resolv(?:e|es|ed))[ \t]*:?[ \t]+#(\d+)/i, + ); + if (!match) return null; + const number = match[1]!; + try { + const title = gh([ + "issue", + "view", + number, + "--json", + "title", + "--jq", + ".title", + ]).trim(); + return { number, title }; + } catch { + console.warn( + `PR body references #${number} but the issue could not be read — ` + + `skipping the Spec axis.`, + ); + return null; + } +} + +const NEEDS_HUMAN_LABEL = "needs:human"; + +/** + * Post a blocking verdict's findings as a PR review (event COMMENT) and apply + * the `needs:human` label. Pure REST via `gh api` — porcelain `gh pr edit` is + * broken in repos with a classic Project attached (projectCards GraphQL + * deprecation), so label writes must not go through it. + */ +export function postBlockingVerdict( + prNumber: string, + verdict: ReviewVerdict, + issue: TargetIssue | null, +): void { + const nwo = repoNwo(); + + const findings = verdict.blockingFindings + .map( + (f, i) => + `${i + 1}. \`${f.file}${f.line === null ? "" : `:${f.line}`}\` — ${f.summary}\n` + + ` ${f.why}`, + ) + .join("\n"); + + const body = + `## Reviewer verdict: BLOCKING\n\n` + + (issue + ? `Reviewed against issue #${issue.number} ("${issue.title}") and its acceptance criteria.\n\n` + : `No target issue could be resolved from the PR body (no \`Closes #n\`); Standards review only.\n\n`) + + `The sandcastle reviewer found ${verdict.blockingFindings.length} blocking finding(s):\n\n` + + `${findings}\n\n` + + `Applied \`${NEEDS_HUMAN_LABEL}\` — a human must resolve these findings before merge. ` + + `(Structured reviewer verdict, toon-protocol/toon-meta#275.)`; + + execFileSync( + "gh", + [ + "api", + `repos/${nwo}/pulls/${prNumber}/reviews`, + "-f", + "event=COMMENT", + "-f", + `body=${body}`, + ], + { stdio: ["ignore", "ignore", "inherit"] }, + ); + + // Ensure the label exists (ignore "already exists"), then add it. Both are + // plain REST so a pre-existing label or a re-run stays idempotent. + try { + execFileSync( + "gh", + [ + "api", + `repos/${nwo}/labels`, + "-f", + `name=${NEEDS_HUMAN_LABEL}`, + "-f", + "color=B60205", + "-f", + "description=Factory reviewer found blocking defects - a human must decide", + ], + { stdio: "pipe" }, + ); + } catch { + // Label already exists — the normal case. + } + execFileSync( + "gh", + [ + "api", + `repos/${nwo}/issues/${prNumber}/labels`, + "-f", + `labels[]=${NEEDS_HUMAN_LABEL}`, + ], + { stdio: ["ignore", "ignore", "inherit"] }, + ); + + console.log( + `Posted blocking findings as a PR review and applied '${NEEDS_HUMAN_LABEL}' on PR #${prNumber}.`, + ); +}