factory: reviewer verdict — structured output + review against the issue, not just the diff

.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 <review>{"verdict":"clean"|"blocking",
+#   ...}</review> — 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:

.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
+  // <review>{"verdict":"clean"|"blocking","blockingFindings":[...]}</review>.
+  // 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`).

.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 <review>{"verdict":"clean"|"blocking",
+//     "blockingFindings":[{file,line,summary,why}]}</review>; 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: <existing PR head> }) 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 <branch> 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) {

.sandcastle/main.ts

@@ -164,13 +164,22 @@ 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 <review> 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,
             agent: sandcastle.claudeCode("claude-opus-5"),
             promptFile: "./.sandcastle/review-prompt.md",
             promptArgs: {
               BRANCH: issue.branch,
+              ISSUE_NUMBER: issue.id,
+              ISSUE_TITLE: issue.title,
             },
           });