fix(ralplan): stop infinite consensus loops with reviewer ratchet + per-lane review budget - #3356
Conversation
…er-lane review budget Behavior-first fix for heavy-model ralplan runs looping indefinitely (evidence: 8 architect passes, revisions 20KB->99KB, one fresh HIGH per pass on previously-approved ground). Reviewer behavior contracts (primary surface): - critic.md: 5-rule re-review ratchet from pass 2 (delta-only review, novelty justification, verdict monotonicity, severity discipline, counter-review of Architect over-engineering); enrichment lane justification-gated from pass 2 - architect.md: ratchet rules 1-4 + rule-5 awareness; "Never approve CRITICAL/HIGH" scoped to carryover findings (fresh HIGHs on approved ground need why-not-visible-earlier justification) - ralplan SKILL.md: mandatory pass-2+ re-review context bundle with run-level pass ordinals, sequential Architect->Critic cadence from pass 2 (pass-1 parallel fan-out unchanged), per-lane budget operator contract, --lane-verdict assignment instruction Runtime backstop (minimal, reuses #3165 cap machinery): - Per-lane review budget: default 1 architect + 1 critic pass per opener iteration, gjc.ralplan.maxReviewPassesPerLane (1..10, schema-registered) - Lane-specific PLANNING-STUCK (exit 3), post-interview/adr/final always allowed; fail-closed on-disk lane artifact floor (ENOENT-only masking) - Crash-gap dedupe repair: identical retry repairs missing/short ledger rows exactly once; final-stage retries verify/recreate pending-approval.md - HUD: arch/crit pass-count chips + last verdict via optional --lane-verdict token (closed vocabularies, run-scoped, no artifact parsing); legacy verdict cleared on run-id change; cross-run repair cannot leak verdicts into the active run Accepted risks (documented, per consensus): no cross-process admission lock (#3165 parity), no every-writer HUD chip preservation, no transactional verdict replay. #3165 opener-cap tests byte-identical. Verification: 135 tests / 741 expectations green across ralplan-runtime, ralplan-decision-artifacts, state-runtime, workflow-hud-summary, and generate-json-schemas suites; schema --check green. Spec: deep-interview (10 rounds, final ambiguity 5%) Plan: ralplan consensus rev 5 (Architect CLEAR/APPROVE, Critic OKAY)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4c054a053e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "gjc.ralplan.maxReviewPassesPerLane": { | ||
| type: "number", | ||
| default: 1, | ||
| validate: (value: number) => Number.isInteger(value) && value >= 1 && value <= 10, | ||
| }, |
There was a problem hiding this comment.
This adds the user-visible gjc.ralplan.maxReviewPassesPerLane setting alongside new lane-budget and --lane-verdict behavior, but the commit leaves packages/coding-agent/CHANGELOG.md unchanged, so the next release notes will omit the new configuration and failure semantics. Add an entry under the coding-agent changelog's ## [Unreleased] section.
AGENTS.md reference: AGENTS.md:L144-L146
Useful? React with 👍 / 👎.
…g entry - Move native-dependent WI-6 HUD tests (artifact/state ordering, lane-verdict precedence) from state-runtime.test.ts to ralplan-runtime.test.ts so the gjc-state-gates runtime CI shard stays native-free - Add Unreleased changelog entry for gjc.ralplan.maxReviewPassesPerLane, per-lane review budget / lane PLANNING-STUCK, --lane-verdict HUD carriage, and the reviewer re-review ratchet contracts
root-check still failed with 5 Biome errors after 516a550 fixed the native-free state gate. These are formatting/import-only corrections with no behavior change: - ralplan-runtime.ts, workflow-hud.ts: formatter output - ralplan-runtime.test.ts: organizeImports, formatter output, and path.join instead of string concatenation for the index.jsonl fixture path (lint/style/useTemplate) - state-runtime.test.ts: drop the stray double blank lines Verified locally: - bun run check:tools: clean (was 5 errors) - ralplan-runtime + state-runtime + workflow-hud-summary + ralplan-decision-artifacts + generate-json-schemas: 135 pass / 0 fail - all four gjc-state-gates groups with pi_natives removed: pass, confirming the native-free contract restored by 516a550 still holds
Red-check repair pushed:
|
| Condition | Result |
|---|---|
PR head state-runtime.test.ts |
fails at import (Failed to load pi_natives native addon) |
origin/dev version of the same file |
31 pass / 0 fail |
516a55092 landed the same structural relocation while I was repairing, so I dropped my duplicate structural commit and rebased only the still-failing Biome fixes on top.
Verification on 59c8b9709
bun run check:tools— clean (was 5 errors)ralplan-runtime+state-runtime+workflow-hud-summary+ralplan-decision-artifacts+generate-json-schemas— 135 pass / 0 fail- All four
gjc-state-gatesgroups (static/runtime/integrity/read) withpi_nativesremoved — pass, confirming the native-free contract restored by516a55092still holds
Dev CI run 30321545956 started on exact head 59c8b9709fc009270a9935936b1c7fb7ddeb55a5.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Terminal verdict — PR #3356 repair verified at
|
| Check | Result |
|---|---|
git/refs/heads/feat/ralplan-infinite-loop-convergence |
59c8b9709fc009270a9935936b1c7fb7ddeb55a5 |
| Local repair branch head | 59c8b9709fc009270a9935936b1c7fb7ddeb55a5 (identical) |
| PR head / mergeable state | 59c8b970 · open · mergeable: true · clean |
| Failing check-runs | none |
| Commit scope | 4 files, +75 / −51, formatting-only |
1. root-check formatting repair
Biome reported 5 errors at 4c054a05; each was reproduced locally before being fixed, and every fix is formatter-authored with no behavior change:
ralplan-runtime.ts— formatter output (+11/−18)workflow-hud.ts— formatter output (+5/−1)ralplan-runtime.test.ts—organizeImports, formatter output, andpath.join(truncatedDir, "index.jsonl")replacing string concatenation (lint/style/useTemplate, line 1590) (+59/−30)state-runtime.test.ts— stray double blank lines (+0/−2)
bun run check:tools is clean at this head (was 5 errors).
2. Native-free state-runtime boundary
scripts/ci-gjc-state-gates.ts runs its runtime group without building @gajae-code/natives; the script's own comments state that suites which transitively load the addon belong in the heavier "Affected path validation" job. 4c054a05 added import { runNativeRalplanCommand } from ".../ralplan-runtime" to state-runtime.test.ts, so the entire native-free suite failed at import time. This was PR-caused, not a workflow precondition gap.
Controlled experiment with pi_natives physically removed:
| Condition | Result |
|---|---|
4c054a05 state-runtime.test.ts |
fails at import — Failed to load pi_natives native addon |
origin/dev version of the same file |
31 pass / 0 fail |
516a55092 (owner-side) landed the relocation of the two cross-runtime lane-verdict cases into ralplan-runtime.test.ts, which owns the ralplan helpers and runs in a native-capable job. My duplicate structural commit was dropped; only the still-failing Biome fixes were rebased onto it. The boundary is re-verified at 59c8b970: all four gjc-state-gates groups (static/runtime/integrity/read) pass with pi_natives removed.
Verification summary at 59c8b970
bun run check:tools— cleanralplan-runtime+state-runtime+workflow-hud-summary+ralplan-decision-artifacts+generate-json-schemas— 135 pass / 0 fail- All four
gjc-state-gatesgroups withpi_nativesremoved — pass - No rerun-only response, no retry loops, no timeout inflation, no
continue-on-error
Merge hold
Not merging. Current dev head e9ecc93ad0dd7d20a0d2bd4bd7e9dc435a1d3dfd (run 30321208841) is still in_progress. Per the never-merge-while-dev-is-red constraint, #3356 waits until that exact dev head is terminal green.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 59c8b9709f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (laneVerdict && (await applyLaneVerdictUpdate(cwd, resolved.sessionId, laneVerdict, resolved.runId))) { | ||
| appliedLaneVerdict = laneVerdict; | ||
| } | ||
| return buildDeduplicatedResult(resolved, repairedArtifact, sha256, cwd, repositoryBinding, appliedLaneVerdict); |
There was a problem hiding this comment.
When a lane artifact survived a crash before its ledger append, an identical retry can repair the ledger and persist --lane-verdict, but this early return skips writeSessionActivityMarker and syncRalplanHud below. The command therefore reports success while the active TUI summary still shows the preceding stage and omits the repaired pass/verdict until an unrelated later mutation rebuilds it; sync the repaired write before returning.
Useful? React with 👍 / 👎.
| const summary = summarizeRalplanIndex(rows); | ||
| iterationFromIndex = summary.iteration; | ||
| stages = formatRalplanStagePresence(summary.currentStages); | ||
| architectPasses = summary.currentStages.filter(stage => stage === "architect").length; |
There was a problem hiding this comment.
When crash-gap repair encounters a short parseable row such as {"stage":"architect","stage_n":2}, it intentionally appends a complete row with the same stage identity. The budget gate deduplicates those rows, but this HUD path summarizes the raw ledger, so the next ordinary write renders two Architect passes for the single repaired pass (and may show values such as 3/2 after the genuine second pass). Apply the same stage-identity deduplication before deriving the summary and lane counters.
Useful? React with 👍 / 👎.
1 participant
Summary
Fixes infinite-ralplan symptoms observed after enabling heavy models for the Critic/Architect roles: endless ITERATE/REJECT loops driven by review goalpost-moving (one fresh HIGH per pass on previously-approved ground), escalating revision sizes (20KB → 99KB), and lane passes escaping the #3165 opener-only cap (8 architect passes in one run).
Approach — behavior-first, per approved spec and consensus plan:
Reviewer behavior contracts (primary surface)
--lane-verdictassignment instruction.Runtime backstop (minimal, reuses #3165 cap machinery)
gjc.ralplan.maxReviewPassesPerLane(integer 1..10, registered in settings schema + regeneratedconfig.schema.json).post-interview/adr/finalalways allowed so stuck plans escalate visibly, never silently approve.pending-approval.md).arch/critpass-count chips + last verdict via optional--lane-verdicttoken (closed vocabularies, run-scoped, zero artifact parsing; legacy verdict cleared on run-id change; stale-run repairs cannot leak verdicts into the active run).No-regression boundaries (held)
Verdict vocabularies, receipt-only role contracts, full-depth pass-1 review, Critic enrichment lane, and all #3165 semantics unchanged — the existing opener-cap test block is byte-identical.
Accepted risks (documented per consensus, intentionally not implemented)
Provenance
The approved planning artifacts are session-local files under
.gjc/(gitignored by design) and are therefore not linkable from this PR; exact untruncated paths in the authoring worktree (research-enhance-infinite-ralplan-4ad850b0):.gjc/_session-019fa2eb-abd6-7000-9ad3-d563f3cc5787/specs/deep-interview-ralplan-infinite-loop-convergence.md.gjc/_session-019fa2eb-abd6-7000-9ad3-d563f3cc5787/plans/ralplan/019fa2eb-abd6-7000-9ad3-d563f3cc5787/pending-approval.mdThe plan's full ADR and Intent Reconciliation sections are embedded in that artifact and summarized in the first commit message on this branch.
Verification (focused, per plan)
Red-team highlights: pathological 20-stage replay refuses exactly at the degenerate passes (a2/a6/rev6/c6/a8) while
finalstill succeeds; healthy replays produce zero warnings/refusals; justified blockers still block; injected EIO fails closed; cross-run verdict-leak, short-row double-count, and final pending-approval crash-gap attacks all fixed and retested.