fix(next-task): enforce strict review loop exit conditions (#235) (#238)

* fix(workflow-state): add missing phases and result field mappings

Add 'pre-review-gates' and 'docs-update' to the PHASES array so that
completePhase() correctly advances through the full workflow sequence.
Add result field mappings for pre-review-gates, delivery-validation,
and docs-update so phase results are stored properly.

* fix(orchestrate-review): use completePhase instead of updateFlow

Replace workflowState.updateFlow({ reviewResult: ... }) with
workflowState.completePhase({ ... }) so the review-loop phase
properly advances to delivery-validation when done.

* fix(next-task): enforce strict review loop exit conditions

Add startPhase('review-loop') at the beginning of Phase 9 across all
3 platform command files. Remove the "orchestrator may override after
3+ iterations" escape hatch. The review loop now only exits on:
openCount===0, stall detection (same hash 2x), or 5-iteration hard
limit. This ensures all issues are actually resolved before delivery.

* test(workflow-state): add tests for new phase transitions

Verify that completePhase correctly advances through the new phases:
implementation -> pre-review-gates -> review-loop -> delivery-validation
-> docs-update -> shipping. Also test that result fields (preReviewResult,
reviewResult, deliveryResult, docsResult) are stored correctly.

* fix(review-loop): address review findings from iteration 1

- Guard completePhase against unknown current phase (returns null instead of
  silently resetting to first phase)
- Fix truthy check to use !== null && !== undefined for result storage
- Add completePhase() call at end of Phase 9 in all 3 platform command files
- Fix no-shortcuts-policy table: "3 iterations" -> "5 iterations, stall-safe"
- Add PHASES ordering assertions to test suite
- Add blocked review-loop test case (stall-detected path)
- Add shipping -> complete boundary test
- Add completePhase-with-unknown-phase guard test

* fix(review-loop): address review findings from iteration 2

- Fix MAX_STALLS=1 to match documented behavior: stall triggers after 2
  consecutive identical-hash iterations (was 3 with MAX_STALLS=2)
- Add [WARN] log before completePhase null return on unknown phase
- Hoist RESULT_FIELD_MAP to module scope (eliminates per-call allocation)
- Add iteration-limit blocked reason test
- Add falsy result storage test (validates result !== null fix)

* fix(review-loop): address review findings from iteration 3

- Add completePhase() call at end of Phase 8 (pre-review-gates) in all
  platform command files so preReviewResult is stored in flow.json
- Add completePhase extraction to adapter-transforms.js so OpenCode adapter
  preserves the completePhase instruction (was silently dropped by transformer)
- Regenerate all affected OpenCode adapters with completePhase instructions

* docs: add CHANGELOG entry for review loop fix (#235)

Document the Phase 9 review loop exit condition fixes: MAX_STALLS
reduction (2→1), completePhase usage in orchestrate-review, and new
workflow-state phases (pre-review-gates, docs-update).

* chore(enhance): add version field to orchestrate-review skill

Per enhance analysis: add missing version: 5.1.0 to
plugins/next-task/skills/orchestrate-review/SKILL.md and regenerate
adapters to propagate to adapters/opencode/skills/orchestrate-review/SKILL.md

* fix(adapter-transforms): ensure completePhase triggers JS block transform

Move completePhase check to outer conditional so code blocks containing
only workflowState.completePhase() (without other JS keywords) are still
transformed to prose instructions in OpenCode adapters.

Fixes Gemini code review comment on PR #238.

* fix(review-loop): address PR review comments

- console.error → console.warn for [WARN] log in completePhase (Copilot #4)
- Phase 8: derive passed from actual gate results; include coverageResult
  in completePhase payload (Copilot #5, Codex #2)
- Blocked review loop: ask user before advancing to delivery-validation
  with AskUserQuestion override/abort prompt (Copilot #6)

* fix(orchestrate-review): use correct AskUserQuestion answer key

Access blocked-path user response via response.answers[question] using
the full question text as key (not the header label), with fallback for
both old and new AskUserQuestion response shapes.

Author: avifenesh

CHANGELOG.md

@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ### Fixed
 
+- **`/next-task` review loop exit conditions** — The Phase 9 review loop now continues iterating until all issues are resolved or a stall is detected (MAX_STALLS reduced from 2 to 1: two consecutive identical-hash iterations = stall). The `orchestrate-review` skill now uses `completePhase()` instead of `updateFlow()` to properly advance workflow state. Added `pre-review-gates` and `docs-update` to the `PHASES` array and `RESULT_FIELD_MAP` in `workflow-state.js`, ensuring these phases can be tracked and resumed correctly. Fixes issue #235.
+
 - **`/debate` command inline orchestration** — The `/debate` command now manages the full debate workflow directly (parse → resolve → execute → verdict), following the `/consult` pattern. The `debate-orchestrator` agent is now the programmatic entry point for other agents/workflows that need to spawn a debate via `Task()`. Fixes issue #231.
 
 ## [5.1.0] - 2026-02-18

__tests__/workflow-state.test.js

@@ -50,6 +50,14 @@ describe('workflow-state', () => {
       expect(PHASES).toContain('complete');
       expect(PHASES.indexOf('policy-selection')).toBeLessThan(PHASES.indexOf('complete'));
     });
+
+    test('new phases are in correct order relative to neighbors', () => {
+      expect(PHASES.indexOf('implementation')).toBeLessThan(PHASES.indexOf('pre-review-gates'));
+      expect(PHASES.indexOf('pre-review-gates')).toBeLessThan(PHASES.indexOf('review-loop'));
+      expect(PHASES.indexOf('review-loop')).toBeLessThan(PHASES.indexOf('delivery-validation'));
+      expect(PHASES.indexOf('delivery-validation')).toBeLessThan(PHASES.indexOf('docs-update'));
+      expect(PHASES.indexOf('docs-update')).toBeLessThan(PHASES.indexOf('shipping'));
+    });
   });
 
   describe('tasks.json operations', () => {
@@ -205,6 +213,124 @@ describe('workflow-state', () => {
     });
   });
 
+  describe('new phases: pre-review-gates and docs-update', () => {
+    beforeEach(() => {
+      createFlow(
+        { id: '1', title: 'Test', source: 'manual' },
+        { stoppingPoint: 'merged' },
+        testDir
+      );
+    });
+
+    test('PHASES contains pre-review-gates and docs-update', () => {
+      expect(PHASES).toContain('pre-review-gates');
+      expect(PHASES).toContain('docs-update');
+    });
+
+    test('isValidPhase accepts pre-review-gates and docs-update', () => {
+      expect(isValidPhase('pre-review-gates')).toBe(true);
+      expect(isValidPhase('docs-update')).toBe(true);
+    });
+
+    test('completePhase from implementation advances to pre-review-gates', () => {
+      setPhase('implementation', testDir);
+      completePhase(null, testDir);
+
+      const flow = readFlow(testDir);
+      expect(flow.phase).toBe('pre-review-gates');
+      expect(flow.status).toBe('in_progress');
+    });
+
+    test('completePhase from pre-review-gates advances to review-loop', () => {
+      setPhase('pre-review-gates', testDir);
+      completePhase({ passed: true }, testDir);
+
+      const flow = readFlow(testDir);
+      expect(flow.phase).toBe('review-loop');
+      expect(flow.status).toBe('in_progress');
+      expect(flow.preReviewResult).toEqual({ passed: true });
+    });
+
+    test('completePhase from review-loop stores reviewResult and advances to delivery-validation', () => {
+      setPhase('review-loop', testDir);
+      completePhase({ approved: true, iterations: 2 }, testDir);
+
+      const flow = readFlow(testDir);
+      expect(flow.phase).toBe('delivery-validation');
+      expect(flow.status).toBe('in_progress');
+      expect(flow.reviewResult).toEqual({ approved: true, iterations: 2 });
+    });
+
+    test('completePhase from review-loop stores blocked result (stall-detected)', () => {
+      setPhase('review-loop', testDir);
+      completePhase({ approved: false, blocked: true, reason: 'stall-detected', remaining: { critical: 1 } }, testDir);
+
+      const flow = readFlow(testDir);
+      expect(flow.phase).toBe('delivery-validation');
+      expect(flow.reviewResult.approved).toBe(false);
+      expect(flow.reviewResult.reason).toBe('stall-detected');
+    });
+
+    test('completePhase from review-loop stores blocked result (iteration-limit)', () => {
+      setPhase('review-loop', testDir);
+      completePhase({ approved: false, blocked: true, reason: 'iteration-limit', remaining: { critical: 0, high: 2 } }, testDir);
+
+      const flow = readFlow(testDir);
+      expect(flow.phase).toBe('delivery-validation');
+      expect(flow.reviewResult.reason).toBe('iteration-limit');
+      expect(flow.reviewResult.remaining.high).toBe(2);
+    });
+
+    test('completePhase stores falsy result (result !== null fix)', () => {
+      setPhase('pre-review-gates', testDir);
+      completePhase({ passed: false, reason: 'lint-failure' }, testDir);
+
+      const flow = readFlow(testDir);
+      expect(flow.preReviewResult).toBeDefined();
+      expect(flow.preReviewResult.passed).toBe(false);
+    });
+
+    test('completePhase from shipping advances to complete', () => {
+      setPhase('shipping', testDir);
+      completePhase(null, testDir);
+
+      const flow = readFlow(testDir);
+      expect(flow.phase).toBe('complete');
+      expect(flow.status).toBe('completed');
+    });
+
+    test('completePhase returns null for unknown current phase', () => {
+      setPhase('review-loop', testDir);
+      // Manually corrupt the phase to an unknown value
+      const currentFlow = readFlow(testDir);
+      currentFlow.phase = 'nonexistent-phase';
+      writeFlow(currentFlow, testDir);
+
+      const result = completePhase(null, testDir);
+      expect(result).toBeNull();
+    });
+
+    test('completePhase from delivery-validation advances to docs-update', () => {
+      setPhase('delivery-validation', testDir);
+      completePhase({ passed: true }, testDir);
+
+      const flow = readFlow(testDir);
+      expect(flow.phase).toBe('docs-update');
+      expect(flow.status).toBe('in_progress');
+      expect(flow.deliveryResult).toEqual({ passed: true });
+    });
+
+    test('completePhase from docs-update advances to shipping', () => {
+      setPhase('docs-update', testDir);
+      completePhase({ docsUpdated: true }, testDir);
+
+      const flow = readFlow(testDir);
+      expect(flow.phase).toBe('shipping');
+      expect(flow.status).toBe('in_progress');
+      expect(flow.docsResult).toEqual({ docsUpdated: true });
+    });
+  });
+
   describe('convenience functions', () => {
     test('getFlowSummary returns summary object', () => {
       createFlow(

adapters/codex/skills/next-task/SKILL.md

@@ -17,7 +17,7 @@ This workflow exists because each step serves a purpose. Taking shortcuts defeat
 | Step | Purpose | What Happens If Skipped |
 |------|---------|------------------------|
 | Worktree creation | Parallel task isolation | Conflicts, lost work |
-| Review loop (3 iterations) | Catches bugs humans miss | Bugs ship to production |
+| Review loop (5 iterations, stall-safe) | Catches bugs humans miss | Bugs ship to production |
 | 3-minute CI wait | Auto-reviewers need time | Miss critical feedback |
 | Address all PR comments | Quality gate | Merge blocked, trust lost |
 
@@ -338,6 +338,9 @@ For each fix:
 Use Edit tool to apply. Commit message: "fix: clean up AI slop"`
   });
 }
+
+const gatesPassed = (deslop.fixes?.length || 0) === 0;
+workflowState.completePhase({ passed: gatesPassed, deslopFixes: deslop.fixes?.length || 0, coverageResult });
 ```
 </phase-8>
 
@@ -346,6 +349,10 @@ Use Edit tool to apply. Commit message: "fix: clean up AI slop"`
 
 **Blocking gate** - Must run iterations before delivery validation.
 
+```javascript
+workflowState.startPhase('review-loop');
+```
+
 **CRITICAL**: You MUST spawn multiple parallel reviewer agents. Do NOT use a single generic reviewer.
 
 ### Step 1: Get Changed Files
@@ -415,13 +422,13 @@ For each finding, use Edit tool to apply the suggested fix. Commit after each ba
 
 Repeat steps 3-5 until:
 - `openCount === 0` (all issues resolved) -> approved
-- 3+ iterations with only medium/low issues -> orchestrator may override
-- 5 iterations reached -> blocked
+- Same findings hash for 2 consecutive iterations (stall detected) -> blocked
+- 5 iterations reached (hard limit) -> blocked
 
 ### Review Iteration Rules
 - MUST run at least 1 full iteration with ALL 4 core reviewers
 - Do NOT use a single generic reviewer - spawn all specialists in parallel
-- Orchestrator may override after 3+ iterations if only medium/low issues remain
+- MUST continue while `openCount > 0`. Only stop on: openCount===0, stall detection, or 5-iteration hard limit
 - Do not skip directly to delivery validation
 - Do not claim "review passed" without spawning the reviewer agents
 
@@ -436,6 +443,11 @@ After review loop completes, output:
 - Findings resolved: X critical, Y high, Z medium
 - Status: approved | blocked
 ```
+
+Then advance the workflow state:
+```javascript
+workflowState.completePhase({ approved, iterations, remaining });
+```
 </phase-9>
 
 <phase-10>

adapters/opencode/agents/ci-monitor.md

@@ -92,6 +92,7 @@ Agent MUST exit monitoring loop when ANY of these occur:
 4. ci-fixer reports unfixable issue - ESCALATE
 
 - Phase: ci-wait
+- Call `workflowState.completePhase(result)` to advance workflow state
 
 
 ## Output Format

adapters/opencode/agents/delivery-validator.md

@@ -74,13 +74,8 @@ delivery-validator (YOU ARE HERE)
 
 ## State Updates
 
-```javascript
-// On success
-workflowState.completePhase({ approved: true, checks });
+- Call `workflowState.completePhase(result)` to advance workflow state
 
-// On failure
-workflowState.failPhase('Validation failed', { failedChecks, fixInstructions });
-```
 
 ## Output Format
 

adapters/opencode/agents/exploration-agent.md

@@ -117,6 +117,7 @@ git diff HEAD~20 -- ${RELEVANT_DIRS} --stat
 ## Phase 10: Update State
 
 - Phase: exploration
+- Call `workflowState.completePhase(result)` to advance workflow state
 
 
 ## Output Format

adapters/opencode/agents/implementation-agent.md

@@ -133,7 +133,8 @@ Make atomic, meaningful commits:
 
 After implementation completes:
 
-*(JavaScript reference - not executable in OpenCode)*
+- Call `workflowState.completePhase(result)` to advance workflow state
+
 
 ## [CRITICAL] WORKFLOW GATES - READ CAREFULLY
 

adapters/opencode/commands/next-task.md

@@ -50,7 +50,7 @@ This workflow exists because each step serves a purpose. Taking shortcuts defeat
 | Step | Purpose | What Happens If Skipped |
 |------|---------|------------------------|
 | Worktree creation | Parallel task isolation | Conflicts, lost work |
-| Review loop (3 iterations) | Catches bugs humans miss | Bugs ship to production |
+| Review loop (5 iterations, stall-safe) | Catches bugs humans miss | Bugs ship to production |
 | 3-minute CI wait | Auto-reviewers need time | Miss critical feedback |
 | Address all PR comments | Quality gate | Merge blocked, trust lost |
 
@@ -236,11 +236,9 @@ Spawn: `worktree-manager` (haiku)
 
 **Last human interaction point.** Present plan via EnterPlanMode/ExitPlanMode.
 
-```javascript
-EnterPlanMode();
-// User reviews and approves via ExitPlanMode
-workflowState.completePhase({ planApproved: true, plan });
-```
+- Use EnterPlanMode for user approval
+- Call `workflowState.completePhase(result)` to advance workflow state
+
 </phase-6>
 
 <phase-7>
@@ -260,6 +258,7 @@ workflowState.completePhase({ planApproved: true, plan });
 - Invoke `@deslop-agent` agent
 - Invoke `@test-coverage-checker` agent
 - Phase: pre-review-gates
+- Call `workflowState.completePhase(result)` to advance workflow state
 
 </phase-8>
 
@@ -268,6 +267,10 @@ workflowState.completePhase({ planApproved: true, plan });
 
 **Blocking gate** - Must run iterations before delivery validation.
 
+```javascript
+workflowState.startPhase('review-loop');
+```
+
 **CRITICAL**: You MUST spawn multiple parallel reviewer agents. Do NOT use a single generic reviewer.
 
 ### Step 1: Get Changed Files
@@ -307,13 +310,13 @@ For each finding, use Edit tool to apply the suggested fix. Commit after each ba
 
 Repeat steps 3-5 until:
 - `openCount === 0` (all issues resolved) -> approved
-- 3+ iterations with only medium/low issues -> orchestrator may override
-- 5 iterations reached -> blocked
+- Same findings hash for 2 consecutive iterations (stall detected) -> blocked
+- 5 iterations reached (hard limit) -> blocked
 
 ### Review Iteration Rules
 - MUST run at least 1 full iteration with ALL 4 core reviewers
 - Do NOT use a single generic reviewer - spawn all specialists in parallel
-- Orchestrator may override after 3+ iterations if only medium/low issues remain
+- MUST continue while `openCount > 0`. Only stop on: openCount===0, stall detection, or 5-iteration hard limit
 - Do not skip directly to delivery validation
 - Do not claim "review passed" without spawning the reviewer agents
 
@@ -328,6 +331,10 @@ After review loop completes, output:
 - Findings resolved: X critical, Y high, Z medium
 - Status: approved | blocked
 ```
+
+Then advance the workflow state:
+- Call `workflowState.completePhase(result)` to advance workflow state
+
 </phase-9>
 
 <phase-10>
@@ -349,6 +356,7 @@ Uses the unified sync-docs agent from the sync-docs plugin with `before-pr` scop
 
 - Invoke `@sync-docs-agent` agent
 - Phase: docs-update
+- Call `workflowState.completePhase(result)` to advance workflow state
 
 </phase-11>
 

adapters/opencode/commands/ship.md

@@ -78,7 +78,8 @@ Parse from $ARGUMENTS:
 
 ## State Integration
 
-*(JavaScript reference - not executable in OpenCode)*
+- Call `workflowState.completePhase(result)` to advance workflow state
+
 
 ## Phase 1: Pre-flight Checks
 

adapters/opencode/skills/discover-tasks/SKILL.md

@@ -66,7 +66,8 @@ done
 
 ### Phase 5: Update State
 
-*(JavaScript reference - not executable in OpenCode)*
+- Call `workflowState.completePhase(result)` to advance workflow state
+
 
 ### Phase 6: Post Comment (GitHub only)