Skip to content

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

Merged
ALLiDoizCode merged 2 commits into
mainfrom
epic270/275-reviewer-verdict
Aug 6, 2026
Merged

ALLiDoizCode merged 2 commits into
mainfrom
epic270/275-reviewer-verdict

Conversation

@ALLiDoizCode

Copy link
Copy Markdown
Contributor

Gives the factory reviewer a verdict channel and the missing Spec axis (#275, epic #270).

What changes

  • Structured verdict, enforced. Every reviewer run must end with <review>{"verdict":"clean"|"blocking","blockingFindings":[{file,line,summary,why}]}</review>. The schema is declared as sandcastle.Output.object({ tag: "review", schema }); extraction/validation is performed by the new .sandcastle/review-verdict.ts because — verified against the @ai-hero/sandcastle@0.12.0 dist — the engine's structured-output surface exists only on top-level run(), and the sandbox.run() the runners use silently ignores an output option. Extraction mirrors the engine's semantics exactly (last tag wins, fence-aware unwrap, JSON parse, zod schema validation).
  • Malformed verdicts fail the run. One retry via SandboxRunResult.resume() (the engine's own structured-output retry mechanism, exactly one iteration, with engine-style error feedback); if the verdict is still malformed the runner throws and the Actions job goes red — a missing verdict is never mistaken for a clean one.
  • Spec axis. The implement runner forwards ISSUE_NUMBER/ISSUE_TITLE via promptArgs; the standalone agent:review runner resolves the issue from the PR body's Closes #n (no reference → Standards-only review). The prompt instructs the reviewer to gh issue view the target FIRST and review the diff against its acceptance criteria.
  • Blocking side effects. Findings are posted as a PR review (event COMMENT) and needs:human is applied — pure REST via gh api (porcelain gh pr edit is broken here by the projectCards GraphQL deprecation). In auto-merge mode a blocking verdict refuses to merge and falls back to PR mode.
  • Standalone runner unbroken. Adopts connector#634's proven first-live-run fix: the workflow checks out main (git refuses one branch in two worktrees), and the runner materialises the PR head as a local branch so the engine's worktree add -b … HEAD fallback cannot silently review an empty diff.

Forensics steps (#278) and the ~200k context-budget prompt wording (#273) are preserved.

Verification

  • Extraction smoke-tested (valid clean/blocking, fenced JSON, last-tag-wins, missing tag, invalid JSON, bad enum, blocking-without-findings, clean-with-findings, null line) — all pass.
  • Both runners import-smoke-tested to their env guards; npm run gate green; actionlint green on the workflow.
  • Live proof (clean + blocking on real PRs) runs on the first fanned-out repo whose PR merges, since this label→runner executes main's code.

Part of #270
Closes #275

🤖 Generated with Claude Code

…sue (#275)

The reviewer could previously only edit or do nothing; a defect it could not
fix had no channel to the human. Now every reviewer run must emit
<review>{"verdict":"clean"|"blocking","blockingFindings":[{file,line,summary,
why}]}</review>, and reviews against the target issue's acceptance criteria
(the missing Spec axis), not just the diff.

- .sandcastle/review-verdict.ts (new): the shared verdict channel. Declares
  the exact sandcastle Output.object({tag:"review",schema}) definition, but
  consumes its tag+schema itself: verified against @ai-hero/sandcastle@0.12.0
  dist, the engine's structured-output surface exists ONLY on top-level run()
  — sandbox.run() silently ignores an `output` option. Extraction mirrors the
  engine exactly (last tag wins, fence unwrap, JSON parse, schema validate);
  a malformed verdict gets ONE engine-style session-resume retry, then FAILS
  the job — never a silent pass. Blocking verdicts post the findings as a PR
  review and apply needs:human via pure REST (porcelain gh pr edit is broken
  by the projectCards deprecation in repos with a classic Project).
- agent-implement-issue.ts: reviewer receives the issue via promptArgs;
  blocking verdicts land on the freshly opened PR; auto-merge mode refuses to
  merge a blocking verdict and falls back to PR mode.
- agent-review-pr.ts: resolves the Spec-axis issue from the PR body's
  `Closes #n`; standalone runs now check out main (connector#634's proven
  first-live-run fix — git refuses one branch in two worktrees) and
  materialise the PR head as a local branch so the engine's worktree-add
  fallback cannot silently review an empty diff.
- review-prompt.md: two-axis review (Spec first: gh issue view the target),
  fix-vs-blocking boundary, and the REQUIRED verdict block.
- main.ts: passes ISSUE_NUMBER/ISSUE_TITLE so the shared prompt resolves in
  the reserved autonomous loop (which does not yet consume the verdict).

Part of #270. Closes #275.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
ALLiDoizCode added a commit to toon-protocol/relay that referenced this pull request Aug 6, 2026
…sue, not just the diff (toon-meta#275) (#104)

Port of toon-protocol/toon-meta#299:

- New .sandcastle/review-verdict.ts: <review> verdict schema + engine-mirroring
  extraction (sandbox.run() ignores `output` in sandcastle 0.12.0), one resume
  retry then fail-loud, and host-side postBlockingVerdict (PR review via REST +
  needs:human label) / resolveIssueFromPrBody helpers.
- agent-implement-issue.ts: reviewer phase goes through runReviewerWithVerdict;
  auto-merge refuses a blocking verdict (falls back to PR mode); a blocking
  verdict lands findings + label on the verified-open PR.
- agent-review-pr.ts: materialise the PR head as a local branch (checkout is
  main now), resolve the Spec-axis issue from the PR body's `Closes #n`, and
  post a blocking verdict from the host after sandbox close.
- review-prompt.md: two-axis (Standards + Spec) framing, target-issue context,
  WHAT YOU FIX vs WHAT IS BLOCKING, and the REQUIRED VERDICT block; context
  budget section unchanged and still last.
- agent-review.yml: check out MAIN (sandcastle worktree branch conflict —
  proven on connector#634) and document the verdict contract; forensics steps
  untouched.
- main.ts: pass ISSUE_NUMBER/ISSUE_TITLE to the reviewer so the shared prompt
  resolves; the reserved loop does not consume the verdict yet.
- docs/factory-runbook.md: review-pass section updated to the new mechanics.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@ALLiDoizCode

ALLiDoizCode commented Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Live proof — both directions (relay, first repo to merge the port)

The relay port (toon-protocol/relay#104) merged first and hosted the acceptance-criteria proof via the standalone agent:review label runner, on real PRs against real issues:

Clean: relay PR #107 (correctly implements relay#105, Closes #105 in the body) — run 31098191838, job success. Log: Reviewer verdict: CLEAN → reviewer pushed 1 refinement commit to the PR branch → Verdict clean — no blocking findings. No review posted, no needs:human.

Blocking: relay PR #108 (deliberately defective against relay#106: contradicted two acceptance criteria and deleted a section the issue said must not be removed) — run 31098192016, job success. Log: Reviewer verdict: BLOCKING (3 blocking finding(s))PR review posted with all three findings each mapped to the violated criterion, and needs:human applied. The Spec axis demonstrably drove the verdict: the reviewer ran gh issue view 106 and cited the issue's criteria verbatim in the why fields.

The proof also exercised the standalone plumbing this PR fixes: ref: main checkout + local materialisation of the PR head produced a correct non-empty diff, Closes #n resolution, in-sandbox gh issue view, and push-back of reviewer commits.

Proof scaffolding cleaned up: relay PRs #107/#108 closed (branches deleted), issues relay#105/#106 closed.

ALLiDoizCode added a commit to toon-protocol/fractal that referenced this pull request Aug 6, 2026
…sue, not just the diff (#40)

Port of toon-protocol/toon-meta#299 (toon-meta#275, epic toon-meta#270):

- New .sandcastle/review-verdict.ts: declares the Output.object({tag:"review"})
  contract and does its own extraction (sandbox.run() in sandcastle 0.12.0
  ignores an `output` option), with one resume retry on a malformed verdict,
  then a hard failure. Blocking verdicts post findings as a PR review and
  apply `needs:human`, all via REST `gh api`.
- agent-implement-issue.ts: reviewer phase now runs through
  runReviewerWithVerdict with the target issue (Spec axis); auto-merge refuses
  a blocking verdict and falls back to PR mode; a blocking verdict lands on
  the opened PR.
- agent-review-pr.ts: resolves the Spec-axis issue from the PR body's
  `Closes #n`, materialises the PR head as a local branch (the workflow now
  checks out main), and posts blocking findings after sandbox close.
- review-prompt.md: two-axis framing (Standards + Spec), target-issue context,
  WHAT YOU FIX vs WHAT IS BLOCKING, and the machine-parsed REQUIRED VERDICT
  block; context-budget section unchanged.
- agent-review.yml: checkout pins ref: main (sandcastle checks the PR head out
  in its own worktree; git refuses one branch in two worktrees) and the header
  documents the verdict contract. Forensics steps untouched.
- main.ts: passes ISSUE_NUMBER/ISSUE_TITLE to the reviewer prompt so the
  reserved autonomous loop keeps working with the new prompt.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ALLiDoizCode added a commit to toon-protocol/store that referenced this pull request Aug 6, 2026
…sue, not just the diff (#81)

Port of toon-protocol/toon-meta#299 (toon-meta#275) to store:

- New .sandcastle/review-verdict.ts: declares the Output.object({tag:"review"})
  contract, does its own last-tag-wins/fence-aware extraction (sandbox.run()
  in sandcastle 0.12.0 ignores an output option), one resume() retry on a
  malformed verdict, then hard-fails; on blocking, posts findings as a PR
  review (REST) and applies needs:human.
- agent-implement-issue.ts: reviewer runs via runReviewerWithVerdict with the
  labeled issue as the Spec axis; auto-merge refuses a blocking verdict
  (falls back to PR mode); blocking findings land on the verified-open PR.
- agent-review-pr.ts: materialises the PR head as a local branch (checkout is
  main now), resolves the Spec-axis issue from the PR body's Closes #n, and
  posts a blocking verdict after sandbox close.
- review-prompt.md: two-axis framing, target-issue context, Spec-axis step,
  WHAT YOU FIX vs WHAT IS BLOCKING, REQUIRED VERDICT block; context-budget
  section unchanged and last.
- agent-review.yml: checkout pins ref: main (worktree conflict, proven on
  connector#634); header documents the verdict contract. Forensics steps
  untouched.
- main.ts: reviewer promptArgs gain ISSUE_NUMBER/ISSUE_TITLE so the prompt's
  placeholders resolve in the reserved autonomous loop.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ALLiDoizCode added a commit to toon-protocol/Forge that referenced this pull request Aug 6, 2026
…sue, not just the diff (#57)

Port of toon-protocol/toon-meta#299 (toon-meta#275) to Forge:

- New .sandcastle/review-verdict.ts: <review> verdict schema + engine-mirroring
  extraction (last tag wins, fence unwrap, zod validation), one resume retry,
  fail-loud on malformed; blocking → PR review (REST) + needs:human label.
- agent-implement-issue.ts / agent-review-pr.ts: reviewer phase runs through
  runReviewerWithVerdict; auto-merge refuses a blocking verdict; the standalone
  runner resolves the Spec-axis issue from the PR body's Closes #n and
  materialises the PR head as a local branch (workflow now checks out main).
- review-prompt.md: two-axis framing (Standards + Spec), target-issue context,
  WHAT YOU FIX vs WHAT IS BLOCKING, REQUIRED VERDICT block; Forge gate steps
  and the context-budget section preserved.
- main.ts: forwards ISSUE_NUMBER/ISSUE_TITLE so the shared prompt resolves.
- Forge-specific: forge-core runReview recovers the issue id from the
  sandcastle/issue-<id> branch convention (prompt placeholders must resolve);
  forge-cli review materialises the PR head before prepareForReview
  (injectable seam, tested). Verdict ENFORCEMENT in the forge-core path is
  follow-up toon-meta#270 work.
- zod added as a devDependency (v3; .sandcastle/main.ts already imported it).

Part of toon-protocol/toon-meta#270
Part of toon-protocol/toon-meta#275

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ALLiDoizCode added a commit to toon-protocol/swap that referenced this pull request Aug 6, 2026
…sue, not just the diff (#90)

Port of toon-protocol/toon-meta#299 (toon-meta#275) to swap:

- .sandcastle/review-verdict.ts: shared verdict channel — declares
  sandcastle.Output.object({ tag: "review", schema }) and does its own
  extraction (sandbox.run() in @ai-hero/sandcastle@0.12.0 ignores an
  output option); one resume() retry on a malformed verdict, then fail.
- agent-implement-issue.ts: reviewer phase runs runReviewerWithVerdict;
  blocking verdicts post findings + needs:human on the opened PR, and
  auto-merge mode refuses to merge a blocking verdict (falls back to PR
  mode).
- agent-review-pr.ts: resolves the Spec-axis issue from the PR body's
  Closes #n, materialises the PR head as a local branch (the workflow
  now checks out main), and posts blocking verdicts after sandbox close.
- review-prompt.md: two-axis framing (Standards + Spec), target-issue
  context, WHAT YOU FIX vs WHAT IS BLOCKING, and the REQUIRED VERDICT
  structured-output contract; context-budget section unchanged and last.
- agent-review.yml: checkout pins ref: main (sandcastle checks the PR
  head out in its own worktree; one branch cannot be in two worktrees);
  header documents the verdict contract. Forensics steps untouched.
- main.ts: reviewer promptArgs gain ISSUE_NUMBER/ISSUE_TITLE so the
  prompt's placeholders resolve in the reserved autonomous loop.

zod was already a devDependency (^3.25.0, v3 API) — no lockfile change.

Part of toon-protocol/toon-meta#270
Part of toon-protocol/toon-meta#275

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ALLiDoizCode added a commit to toon-protocol/toon that referenced this pull request Aug 6, 2026
…sue, not just the diff (#163)

Port of toon-protocol/toon-meta#299 (toon-meta#275) to toon:

- new .sandcastle/review-verdict.ts: <review> verdict extraction (last tag
  wins, fence unwrap, zod schema), one resume retry then hard fail, and
  host-side postBlockingVerdict (PR review via REST + needs:human label)
- agent-implement-issue.ts: reviewer runs via runReviewerWithVerdict; a
  blocking verdict refuses auto-merge (falls back to PR mode) and lands
  findings + label on the opened PR
- agent-review-pr.ts: resolves the Spec-axis issue from the PR body's
  Closes #n, materialises the PR head as a local branch (workflow now checks
  out main), and posts blocking findings after sandbox close
- review-prompt.md: two-axis framing (Standards + Spec), target-issue
  context, WHAT YOU FIX vs WHAT IS BLOCKING, and the REQUIRED VERDICT block
- agent-review.yml: checkout pinned to ref main (worktree conflict), header
  documents the verdict contract; forensics steps untouched
- main.ts: pass ISSUE_NUMBER/ISSUE_TITLE to the reviewer promptArgs

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ALLiDoizCode added a commit to toon-protocol/rig that referenced this pull request Aug 6, 2026
…sue, not just the diff (#72)

Port of toon-protocol/toon-meta#299 (toon-meta#275) to rig:

- New .sandcastle/review-verdict.ts: <review> verdict schema + extraction
  (sandbox.run() in sandcastle 0.12.0 ignores `output`, so the module does
  its own engine-identical extraction), one resume() retry on a malformed
  verdict then a hard failure, and host-side postBlockingVerdict /
  resolveIssueFromPrBody helpers (pure REST via gh api).
- agent-implement-issue.ts: reviewer runs via runReviewerWithVerdict with
  the target issue (Spec axis); a blocking verdict posts findings +
  needs:human on the opened PR and refuses auto-merge (falls back to PR
  mode).
- agent-review-pr.ts: resolves the Spec-axis issue from the PR body's
  `Closes #n`, materialises the PR head as a local branch (the workflow now
  checks out main), and posts blocking findings after sandbox close.
- review-prompt.md: two-axis framing (Standards + Spec), target-issue
  context, WHAT YOU FIX vs WHAT IS BLOCKING, and the REQUIRED VERDICT
  structured-output contract; context-budget section unchanged and last.
- agent-review.yml: checkout pins ref: main (sandcastle checks the PR head
  out in its own worktree; one branch cannot be in two worktrees); header
  documents the verdict contract. Forensics steps untouched.
- main.ts: reviewer promptArgs gain ISSUE_NUMBER/ISSUE_TITLE so the prompt's
  placeholders resolve in the reserved autonomous loop.
- docs/factory-runbook.md: review-pass section updated to the checkout-main
  + verdict behaviour.

Part of toon-protocol/toon-meta#270
Part of toon-protocol/toon-meta#275

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ALLiDoizCode added a commit to toon-protocol/connector that referenced this pull request Aug 6, 2026
…sue, not just the diff (#810)

Port of toon-protocol/toon-meta#299 (toon-meta#275) to connector:

- New .sandcastle/review-verdict.ts: <review> verdict schema + extraction
  (sandbox.run() ignores `output`, so the module consumes the Output.object
  declaration itself), one resume() retry on a malformed verdict then fail,
  and host-side helpers to resolve the Spec-axis issue from the PR body and
  post blocking findings as a PR review + `needs:human` label (pure REST).
- agent-implement-issue.ts: reviewer phase enforces the verdict; auto-merge
  refuses a blocking verdict (falls back to PR mode); a blocking verdict
  lands on the opened PR.
- agent-review-pr.ts: materialise the PR head as a local branch (the
  workflow checks out main), resolve the target issue from `Closes #n`,
  enforce the verdict, post blocking findings after sandbox close.
- review-prompt.md: two-axis (Standards + Spec) review, target-issue
  context, WHAT YOU FIX vs WHAT IS BLOCKING, REQUIRED VERDICT block.
- main.ts: pass ISSUE_NUMBER/ISSUE_TITLE so the shared prompt resolves.
- agent-review.yml: header comment updated to describe the verdict
  contract (checkout already pins ref: main since connector#634).

zod was already a devDependency (^3.25.76) — no lockfile change.

Part of toon-protocol/toon-meta#270
Part of toon-protocol/toon-meta#275

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
ALLiDoizCode added a commit to toon-protocol/toon-client that referenced this pull request Aug 6, 2026
…sue, not just the diff (#522)

Port of toon-protocol/toon-meta#299 (toon-meta#275) to toon-client:

- .sandcastle/review-verdict.ts: shared verdict channel — zod schema
  ({verdict: clean|blocking, blockingFindings:[{file,line,summary,why}]}),
  engine-mirroring <review> tag extraction (sandbox.run() ignores the
  engine's output option in @ai-hero/sandcastle@0.12.0), one resume retry
  with promptArgs reset, then hard failure; blocking verdicts post a PR
  review + needs:human label via REST.
- agent-implement-issue.ts: review phase enforces the verdict; auto-merge
  refuses a blocking verdict (falls back to PR mode); blocking findings
  land on the verified-open PR.
- agent-review-pr.ts: materialises the PR head as a local branch (checkout
  is main), resolves the Spec-axis issue from the PR body's Closes #n, and
  posts blocking verdicts after sandbox close.
- review-prompt.md: two-axis framing (Standards + Spec), target-issue
  context, blocking-vs-fix rules, and the REQUIRED VERDICT block; the
  context-budget section stays last and unchanged.
- agent-review.yml: header now documents the verdict contract and the
  proven ref:main standalone-review mechanics (checkout already ref: main).
- main.ts: reviewer promptArgs gain ISSUE_NUMBER/ISSUE_TITLE so the
  prompt's new placeholders resolve in the reserved autonomous loop.

zod ^3.25.0 was already a root devDependency; no lockfile change.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
@ALLiDoizCode
ALLiDoizCode merged commit d02ae2e into main Aug 6, 2026
3 checks passed
@ALLiDoizCode
ALLiDoizCode deleted the epic270/275-reviewer-verdict branch August 6, 2026 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

1 participant