review-fix-signoff-loop

Review Fix Signoff Loop

Purpose

Use this pattern for high-stakes implementation workflows where a normal "implement, test, review once" flow is not enough. The workflow must keep repairing and re-reviewing until independent signoff agents agree the spec is fully wired end to end.

Pair this with writing-agent-relay-workflows for SDK syntax and relay-80-100-workflow for deterministic validation gates.

Required Shape

1. Run deterministic preflight before agents start.
   • Confirm repository root, required specs, declared write scope, credentials needed for PR comments, and whether commit/push/PR creation is in scope.
   • For cross-repo or package-release work, write a scope matrix before implementation: repositories, branches, PRs, packages, providers/features touched, published versions, consuming package manifests, lockfiles, and expected downstream bumps.
   • Probe the CLIs used by later agent steps. For Codex, codex login status is not enough; run a tiny codex exec --ephemeral --json --sandbox read-only -m <supported-model> prompt and fail early with a clear re-login instruction if it cannot return the expected token.
   • Write preflight evidence to .workflow-artifacts/<workflow>/iteration-N/preflight.md.

1. Implement with scoped owners.

   • Use Codex workers for code changes unless the codebase has a reason to prefer another CLI.
   • Split backend, frontend, desktop, tests, docs, or infrastructure into explicit non-overlapping ownership areas.
   • Each worker writes a durable summary artifact with changed files and commands run.

2. Reconcile before validation.

   • Add a deterministic implementation-reconcile gate that checks required files, expected API/UI/runtime surfaces, migrations, generated artifacts, and untracked files with git status --short -- <paths>.
   • For multi-provider changes, reconcile against the scope matrix: every touched provider/package must be classified as implemented, dependency-only, intentionally-deferred, or not-applicable, with proof. Do not let "we only bumped the package I remembered" pass this gate.
   • For package-release flows, reconcile producer and consumer state: npm view <pkg> version, package manifests, lockfile resolved tarballs/integrities, and npm ls <pkg> from every consuming workspace.
   • For CI failures, map each failing job to its exact local command or documented non-local equivalent. Distinguish similarly named gates (for example handler coverage vs acceptance route coverage) and replay the one that actually failed.
   • Use failOnError: false, then route the captured output to a repair owner.

3. Run repairable validation.

   • Use capture -> fix -> rerun for typecheck, targeted tests, integration or E2E tests, and regression checks.
   • Include exact failing CI commands when available before broader "nearby" checks. A nearby green gate is supporting evidence, not proof that the reported CI failure is fixed.
   • Red validation output is input for a repair agent, not an immediate workflow failure.
   • Write BLOCKED_NO_COMMIT.md only for true external blockers.

4. Run fresh-context signoff reviews.

   • Start a new workflow run, new agent names, or otherwise new agent contexts for each loop iteration.
   • Run Claude and Codex signoff reviews independently over the same post-validation repo state.
   • Reviewers must read specs, diff, validation logs, artifacts, and actual files.

5. Break only on dual signoff.

   • The loop may exit only when both reviewers write the exact satisfied verdict and final deterministic acceptance is green.
   • If either reviewer finds issues or is blocked, run a Codex fix pass and start a new fresh-context review iteration.
   • Make the Codex fix pass a non-interactive one-shot worker (preset: 'worker') with a file_exists verification for its durable report. Do not rely on interactive PTY idle detection or /exit for loop progress.

6. Report final signoff.

   • Write a final SIGNOFF.md that includes iteration count, validation evidence, Claude rationale, Codex rationale, remaining risks, and artifact paths.
   • Include the final scope matrix with every repository/package/provider row signed off, deferred with owner/date, or marked not applicable. For release flows, include published and consumed versions.
   • Post the same report to the PR. Resolve the PR from an explicit env var first, then from gh pr view.

Verdict Contract

Use a strict text contract so deterministic gates can parse the result:

VERDICT: COMPREHENSIVELY_SATISFIED | FINDINGS | BLOCKED
why_passed: required when VERDICT is COMPREHENSIVELY_SATISFIED
end_to_end_wiring_verified: required when VERDICT is COMPREHENSIVELY_SATISFIED
deterministic_evidence: required when VERDICT is COMPREHENSIVELY_SATISFIED
scope_matrix_verified: required when VERDICT is COMPREHENSIVELY_SATISFIED for cross-repo/provider/package work
remaining_risks: required when VERDICT is COMPREHENSIVELY_SATISFIED
finding_id: stable-id when VERDICT is FINDINGS
severity: blocker | high | medium | low
file: path
issue: concrete gap
fix_required: exact change needed
test_required: deterministic proof needed
evidence: commands, files, or spec clause

A deterministic dual-signoff gate should require:

• both review files contain VERDICT: COMPREHENSIVELY_SATISFIED
• neither review file contains VERDICT: FINDINGS, VERDICT: BLOCKED, or an open finding_id
• both review files include the required pass-rationale fields
• for cross-repo/provider/package work, both review files include scope_matrix_verified: and cite the matrix artifact
• the latest final acceptance artifact is green

Scope Matrix

Create a machine-readable and human-readable matrix before the first fix pass for work that spans repositories, packages, providers, or CI gates. Keep it updated every iteration.

Recommended columns:

repo | branch | PR | package/provider/surface | expected change | producer version | consumer version | files expected | gates required | status | evidence | owner

Use the matrix to prevent "last-mile" omissions:

• A package published from one repo must have a matching consumer row in every repo that should install it.
• A provider touched in adapter code must have Cloud ingest/write/digest/writeback/dependency rows, or an explicit not-applicable rationale.
• A route or API surface with both runtime tests and coverage metadata must list both gates separately.
• Untracked generated artifacts must be classified as expected workflow artifacts, ignored local output, or unexpected drift before commit.

Example package-consumption evidence:

npm view @scope/pkg version
rg -n '"@scope/pkg"' package.json package-lock.json packages -g package.json -g package-lock.json
npm ls @scope/pkg --workspace <consumer-workspace>
node -e 'const lock=require("./package-lock.json"); console.log(lock.packages["node_modules/@scope/pkg"].version)'

Fresh Context Implementation

Prefer an outer loop that starts a new Agent Relay workflow run per iteration:

for (let iteration = 1; ; iteration += 1) {
  await runIteration(iteration, runStamp); // new workflow name, channel, and agent names
  clearStartFromAfterResumedIteration();
  if (hasDualSignoff(iteration)) {
    writeAndPostSignoffReport(iteration);
    break;
  }
}

Within runIteration, suffix workflow name, channel, and agent names with runStamp-iteration:

const suffix = `${runStamp}-${iteration}`;
workflow(`my-feature-completion-${suffix}`)
  .channel(`wf-my-feature-${suffix}`)
  .agent(`claude-reviewer-${suffix}`, { cli: 'claude', preset: 'reviewer', role: 'Fresh signoff reviewer' })
  .agent(`codex-reviewer-${suffix}`, { cli: 'codex', preset: 'reviewer', role: 'Fresh signoff reviewer' });

This prevents reviewer memory from a previous loop from becoming the reason the loop exits.

If the outer loop supports --start-from, consume it for only the resumed iteration. START_FROM=fix-review-findings must not leak into the next fresh-context iteration, or the next iteration will skip review/validation and fail on missing artifacts. After a completed resumed iteration, delete process.env.START_FROM and process.env.PREVIOUS_RUN_ID before continuing the loop.

Codex Fixer Reliability

For review-fix loop steps, prefer this shape:

.agent(`codex-review-fixer-${suffix}`, {
  cli: 'codex',
  model: CodexModels.GPT_5_4,
  preset: 'worker',
  role: 'Review-finding fixer. Repairs valid findings and hardens tests/proofs.',
  retries: 2,
})
.step('fix-review-findings', {
  agent: `codex-review-fixer-${suffix}`,
  dependsOn: ['dual-signoff-gate'],
  task: `Read iteration artifacts. Fix every valid finding, rerun relevant checks, and write ${dir}/review-fix-report.md.`,
  verification: { type: 'file_exists', value: `${ROOT}/${dir}/review-fix-report.md` },
})

Use interactive PTY Codex only when the step genuinely needs live channel coordination. For bounded artifact-producing fix/review steps, preset: 'worker' exits through the subprocess lifecycle, and file_exists proves the required artifact exists.

PR Signoff Comment

Final signoff should be both a durable artifact and a PR comment.

Resolution order:

1. explicit env var such as SIGNOFF_PR_NUMBER, PR_NUMBER, or GITHUB_PR_NUMBER
2. gh pr view --json number --jq .number
3. if no PR is available, write PR_COMMENT_FAILED.md and fail unless the workflow has an explicit skip env var

Use a deterministic shell or Node step:

gh pr comment "$PR_NUMBER" --body-file .workflow-artifacts/my-workflow/pr-comment.md

The comment body should include:

• iteration count
• final validation status and command evidence
• Claude signoff rationale
• Codex signoff rationale
• remaining risks or explicit "none"
• artifact path for full logs

Blocked State

Do not spin forever when progress is impossible. If agents identify a true external blocker, write:

.workflow-artifacts/<workflow>/iteration-N/BLOCKED_NO_COMMIT.md

Include exact evidence, missing credentials or services, commands that failed, and the safest retry command. Do not commit, push, or post a success comment from a blocked run.

Common Mistakes

• Reusing the same reviewer context every loop. Start a new run or new reviewer agents for each iteration.
• Letting a reviewer write NO_ISSUES_FOUND without pass rationale. Require the full verdict contract.
• Treating green tests as signoff. Green deterministic gates are required evidence, not a substitute for fresh review.
• Hard-failing the first red validation gate. Capture it, repair it, then rerun.
• Posting a PR comment before both signoff agents agree on the same final state.
• Forgetting to count iterations. The final report must say how many loops it took.