porch: max_iterations enforcement is dead code (getMaxIterations has zero callers)

[waleedkadous]

Reported by

An external Codev adopter's architect, via cross-workspace afx send, 2026-05-26. Verified independently against packages/codev/src/ HEAD.

The bug

getMaxIterations() is exported from packages/codev/src/commands/porch/protocol.ts:403, parses per-phase max_iterations config from protocol.json (default 1), but has zero callers anywhere in packages/codev/src/. It's dead code. The state machine flows the value through state.iteration but never consults the configured cap, so porch unconditionally re-iterates on every REQUEST_CHANGES (and in practice on COMMENT-level findings too).

The dead-code comment at next.ts:617-618 makes the intent obvious:

// No rebuttal yet — emit "write rebuttal" task (do NOT increment iteration)
// This MUST come before the max_iterations safety valve so the builder
// gets a chance to write a rebuttal before force-advance kicks in.

The safety valve referenced by the comment does not exist after this block. Someone wrote the rebuttal-first path but never landed the force-advance path it was supposed to precede.

Live symptom

Reporter observed on a real in-flight SPIR project: a phase went through 5 review iterations of 3-way consult before the architect had to do a direct override on iter-6 — final verdicts had converged to APPROVE + COMMENT + COMMENT but porch demanded another iter. Other phases of the same project showed 2-iter and mid-iter-2-on-codex-hang patterns. Every SPIR protocol.json phase has max_iterations: 1. Builders ignore it because porch ignores it.

Root cause

$ grep -rn "getMaxIterations" packages/codev/src/
packages/codev/src/commands/porch/protocol.ts:403:export function getMaxIterations(protocol: Protocol, phaseId: string): number {
# ↑ definition only, no callers

See the follow-up comment on this issue

The reporter's first fix proposal (a per-N iteration cap with default 1 or 2) is retracted. A natural experiment over 21 review iterations on the reporter's project produced data showing the cap is the wrong abstraction — real bugs surface in deep iterations and a count-based cap would have shipped critical bugs.

The superseding policy (see the issue comment for the data and rationale): treat COMMENT and REQUEST_CHANGES asymmetrically. ADVANCE when no reviewer returned REQUEST_CHANGES (all APPROVE-or-COMMENT). RE-ITER when any reviewer returned REQUEST_CHANGES, no count limit in normal flow. Keep max_iterations only as a high safety ceiling (~8) for runaway prevention.

References

• packages/codev/src/commands/porch/protocol.ts:403-406 — getMaxIterations definition
• packages/codev/src/commands/porch/protocol.ts:228 — default-1 parsing
• packages/codev/src/commands/porch/next.ts:617-618 — phantom-guarding comment

[waleedkadous]

On it! Working on a fix now.

[waleedkadous]

Reframe from the reporter — 2026-05-26

The reporter ran a natural experiment across 21 review iterations on a real in-flight SPIR project and is retracting their earlier "raise default to 2 and wire it up" suggestion. New finding: a per-N iteration cap is the wrong abstraction.

Data (21 iters, 7 phases, classified post-hoc by what each iter caught)

Phase	Iters	Real-bug iters	Polish-only
1	2	1	0
2	6	3	3 (COMMENT only)
3	3	1	2 (COMMENT spinning)
4	4	4	0
5	2	1	0
6	2	1	0
7	2	1	0

11/21 (52%) caught real bugs. 5/21 (24%) were polish-only COMMENT-cycling.

Why a cap is the wrong abstraction

• One phase had FOUR consecutive iters each catching a new correctness bug. max=2 would have shipped two critical bugs from that phase alone.
• Another phase caught real bugs at iter-3 AND iter-5. max=2 misses both.
• Bugs hide in deep iterations. Iteration count is not a reliable polish/real-bug signal.

Real bug

Porch is treating COMMENT and REQUEST_CHANGES symmetrically — both trigger re-iter. They shouldn't be. COMMENT = "FYI, I have notes" — not a change-request.

Proposed policy (supersedes the Option A / Option B framing in the issue body)

• ADVANCE: when no reviewer returned REQUEST_CHANGES this round (all APPROVE-or-COMMENT).
• RE-ITER: when any reviewer returned REQUEST_CHANGES (no count limit in normal flow).
• SAFETY CEILING: keep max_iterations as runaway-prevention only, set high (~8), invoked only when REQUEST_CHANGES recurs many rounds (bug condition, not normal flow).

Under this policy on the reporter's dataset: 16/21 iters still fire (all real-bug catchers), 5 polish iters auto-advance, zero real bugs missed.

Updated acceptance criteria (supersede the body's)

• Verdict-handling in next.ts advances the phase when no reviewer returned REQUEST_CHANGES, even if there are COMMENTs.
• Re-iter still fires on any REQUEST_CHANGES, with no count limit during normal flow.
• getMaxIterations() is wired in as a high safety ceiling (default 8 or per-phase config) — only invoked when REQUEST_CHANGES persists for many rounds.
• Audit trail: when force-advance fires (either policy-driven or safety-ceiling), the latest iter's rebuttal file is preserved and a user-visible signal communicates that force-advance happened.
• Unit tests cover: (a) all-APPROVE-or-COMMENT → advance; (b) any REQUEST_CHANGES → re-iter; (c) REQUEST_CHANGES on iter=ceiling → force-advance with audit trail.
• Phantom comment at next.ts:617-618 either describes the new live code or is removed.