Skip to content

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

Merged
ALLiDoizCode merged 1 commit into
mainfrom
epic270/275-reviewer-verdict
Aug 6, 2026
Merged

factory: reviewer verdict — structured output + review against the issue, not just the diff#57
ALLiDoizCode merged 1 commit 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 — port of toon-protocol/toon-meta#299 to Forge.

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 stage-0 runner resolves the issue from the PR body's Closes #n (no reference → Standards-only review). review-prompt.md instructs the reviewer to gh issue view the target FIRST and review the diff against its acceptance criteria; Forge's repo-specific review steps and gate commands are preserved, as is the context-budget section.
  • 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 in repos with a classic Project attached). 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: agent-review.yml now checks out main (git refuses one branch in two worktrees), and the runner materialises the PR head as a local branch (git fetch origin +head:head) so the engine's worktree add -b … HEAD fallback cannot silently review an empty diff.
  • main.ts passes ISSUE_NUMBER/ISSUE_TITLE to its reviewer so the shared prompt resolves; the reserved autonomous loop does not consume the verdict yet (auto-merge wiring is toon-meta#270 work).

Forge-specific adaptations

Forge's live agent:review workflow runs forge review (forge-cli → forge-core), not the stage-0 .sandcastle/agent-review-pr.ts runner, so two extra changes keep that path working with the new prompt and checkout:

  • packages/forge-core/src/sandcastle-runners.tsrunReview now passes ISSUE_NUMBER/ISSUE_TITLE (an unresolved {{…}} placeholder fails the run), recovering the issue id from the deterministic sandcastle/issue-<id> branch convention (the same id the factory PR body's Closes #n names); a branch outside the convention gets a Standards-only review.
  • packages/forge-cli/src/review.ts — materialises the PR head as a local branch before prepareForReview (injectable seam, unit-tested), matching the workflow's new ref: main checkout.
  • Deviation, stated openly: the forge-core review path does not yet consume the <review> verdict (extraction/enforcement lives in .sandcastle/review-verdict.ts, which a built package cannot import). The prompt contract and the Spec axis are live on that path; porting enforcement into forge-core (and into templates/sandcastle/ for newly-stamped factories) is follow-up toon-meta#270/#275 work.

zod was added as a root devDependency (^3.25.0, v3 — z.ZodIssueCode.custom kept): .sandcastle/main.ts already imported it but nothing declared it.

Verification

  • Extraction smoke-tested in-repo (valid clean/blocking, fenced JSON, last-tag-wins, missing tag, invalid JSON, bad enum, blocking-without-findings, clean-with-findings, null line) — all 10 pass; temp script deleted before commit.
  • Both stage-0 runners import-smoke-tested to their env guards (SANDCASTLE_PR_NUMBER / SANDCASTLE_ISSUE_NUMBER), nothing else.
  • Full gate green locally in CI order: pnpm lint, pnpm typecheck, pnpm test (19 files, 172 tests incl. the two new seam tests), pnpm build, pnpm format:check.
  • Workflow YAML parse-checked (actionlint unavailable locally); the workflow edit is header comments + the ref: main pin, with the #278 redact/upload forensics steps byte-identical.

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

🤖 Generated with Claude Code

…sue, not just the diff

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
ALLiDoizCode merged commit a44e6c1 into main Aug 6, 2026
2 checks passed
@ALLiDoizCode
ALLiDoizCode deleted the epic270/275-reviewer-verdict branch August 6, 2026 11:53
ALLiDoizCode pushed a commit that referenced this pull request Aug 10, 2026
… clock (toon-meta#248) (#61)

* RALPH: factory: fresh-push-credential fix + raise the wall clock (toon-meta#248)

Closes #59. Ports connector#463's fix — App installation tokens expire ONE
HOUR after mint; a runner that mints once at job start and pushes only at
the end of a long cycle dies at the final push after every expensive agent
iteration has already been spent (connector#462). Fresh-mint and
publish-early land first; the clock moves last, per the issue's ordering.

Key decisions:
- New `mint-app-token.ts` (forge-core, duplicated standalone in
  `.sandcastle/` — mirrors the existing sandbox-secrets.ts split between the
  live path and the stage-0 runner) signs the App JWT and exchanges it for
  an installation token via RS256 (`node:crypto`, no jsonwebtoken dep).
  `createTokenMinter` returns `undefined` when APP_ID/APP_PRIVATE_KEY are
  absent so local runs / ambient-GH_TOKEN runs are unchanged.
- `sandcastle-runners.ts`'s new `pushBranch` mints fresh immediately before
  every push, delivers the token via STDIN into a mode-600 file read by a
  one-shot `credential.helper`, and deletes it after. The leading
  `-c credential.helper=` reset is load-bearing (multi-valued config key —
  verified directly by the "resets the container-global helper" test, which
  asserts the empty reset precedes the one-shot helper in the pushed
  command) and is what stops the stale container-global helper from
  `gh auth setup-git` (SANDBOX_READY_HOOKS) from winning. Also refreshes the
  host's own `process.env.GH_TOKEN` for the `gh pr list`/`gh pr create`
  calls that follow.
- New `pushEarly` runner + `PushEarly` cycle.ts injection point: publishes
  best-effort right after the implement phase, before the pre-review gate
  and review, so a kill mid-review still leaves recoverable work on the
  remote. The primitive fails loud; `runCycle` is what makes it best-effort
  (catches and logs).
- `run.ts` wires `createTokenMinter()` into `createSandcastleRunners` and
  wraps it with `withMasking` (`::add-mask::`) as defence-in-depth alongside
  the existing redact-then-upload log artifact step.
- Workflow + template: job timeout 60->180, new step timeout 170 (was 50),
  APP_ID/APP_PRIVATE_KEY added to the HOST-only implement step env (never
  forwarded into the sandbox — sandboxSecrets/PASSTHROUGH_KEYS omit both).
  Template additionally gained the redact+upload steps it never had.
- `.sandcastle/agent-implement-issue.ts`: PR-mode push now goes through the
  same pushBranch, plus a new best-effort early-publish call after the
  implement phase.

Files changed: packages/forge-core/src/{mint-app-token.ts,
mint-app-token.test.ts} (new), sandcastle-runners.{ts,test.ts}, cycle.{ts,
test.ts}, index.ts; packages/forge-cli/src/run.{ts,test.ts};
.github/workflows/agent-implement.yml; templates/workflows/agent-implement.yml;
.sandcastle/{agent-implement-issue.ts, mint-app-token.ts (new)}.

Gate green: format, lint, typecheck, 189 tests (+34), build.

Notes for next iteration: `.sandcastle/` and `templates/` are excluded from
eslint/vitest/tsc by design (confirmed in eslint.config.js/vitest.config.ts),
so the two hand-authored .sandcastle/*.ts changes were syntax-checked with
esbuild (no type errors caught) rather than run through the typed gate —
matches the precedent set by #57's review-verdict.ts port. Per
`templates/sandcastle`'s locked six-file inventory (templates.test.ts:22-24),
runner .ts scripts are not currently part of the stamped bundle at all, so
"the template stamps agent-implement-issue.ts" in the issue is aspirational
for a future forge-new wiring, not current stamp.ts behavior — fixed this
repo's own real .sandcastle/agent-implement-issue.ts regardless, since it is
what actually runs today.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* factory: mask minted tokens on the stage-0 push path too (toon-meta#248)

The live forge-core/forge-cli push path wraps mintToken with withMasking
(::add-mask::) as defence-in-depth alongside the redact-then-upload log
step, but the mirrored stage-0 .sandcastle/agent-implement-issue.ts path
(what a stamped repo runs) didn't get the same treatment. Bring it in
line for consistency on a PUBLIC repo.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

---------

Co-authored-by: RALPH <ralph@toon-protocol.local>
Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
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.

1 participant