fix(next-task): enforce strict review loop exit conditions (#235)#238
fix(next-task): enforce strict review loop exit conditions (#235)#238
Conversation
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.
Replace workflowState.updateFlow({ reviewResult: ... }) with
workflowState.completePhase({ ... }) so the review-loop phase
properly advances to delivery-validation when done.
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.
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.
- 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 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)
- 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
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).
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
Summary of ChangesHello @avifenesh, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request resolves several critical issues within the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This is a comprehensive fix that addresses several root causes for the review loop exiting prematurely. The changes to workflow-state.js are robust, with good refactoring (like RESULT_FIELD_MAP) and defensive coding (guarding completePhase and allowing falsy results). The addition of extensive tests in workflow-state.test.js is excellent and provides confidence in the new phase transitions and logic.
However, I've found a critical issue in the OpenCode adapter transformation logic in lib/adapter-transforms.js. The new check for completePhase is nested incorrectly, causing it to fail for code blocks that only contain a completePhase call. This results in raw JavaScript being left in OpenCode adapter files, which will lead to runtime failures on that platform. Please see the specific comment for details on the issue and the recommended fix.
Once that transformation logic is corrected, this PR will be in great shape.
![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: a7a260ac52
ℹ️ 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".
…form 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.
|
Fixed in e44faa0 — moved |
There was a problem hiding this comment.
Pull request overview
This PR fixes /next-task Phase 9 review-loop behavior so it can’t “approve early” while findings remain open, and makes workflow phase tracking/state advancement consistent across core lib, plugins, and generated adapters.
Changes:
- Added missing workflow phases (
pre-review-gates,docs-update) and centralized phase→result-field mapping viaRESULT_FIELD_MAP, plus a guard for unknown phases incompletePhase(). - Updated OpenCode adapter transforms to preserve
completePhaseinstructions when JS fences are converted to non-executable guidance. - Updated Phase 8/9 orchestration docs and added tests covering the new phase transitions and edge cases.
Reviewed changes
Copilot reviewed 38 out of 38 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/sync-docs/lib/state/workflow-state.js | Sync copy of workflow-state updates (new phases, RESULT_FIELD_MAP, completePhase guard). |
| plugins/sync-docs/lib/adapter-transforms.js | Sync copy of OpenCode transform emitting completePhase instruction. |
| plugins/ship/lib/state/workflow-state.js | Sync copy of workflow-state updates. |
| plugins/ship/lib/adapter-transforms.js | Sync copy of OpenCode transform update. |
| plugins/repo-map/lib/state/workflow-state.js | Sync copy of workflow-state updates. |
| plugins/repo-map/lib/adapter-transforms.js | Sync copy of OpenCode transform update. |
| plugins/perf/lib/state/workflow-state.js | Sync copy of workflow-state updates. |
| plugins/perf/lib/adapter-transforms.js | Sync copy of OpenCode transform update. |
| plugins/next-task/skills/orchestrate-review/SKILL.md | Updates review-loop iteration rules, MAX_STALLS, and uses completePhase() for review results. |
| plugins/next-task/lib/state/workflow-state.js | Plugin-local workflow-state updated with new phases/mapping/guard. |
| plugins/next-task/lib/adapter-transforms.js | Plugin-local OpenCode transform emits completePhase instruction. |
| plugins/next-task/commands/next-task.md | Phase 8/9 workflow instructions updated (startPhase review-loop; completePhase at end of phases; stricter stopping text). |
| plugins/learn/lib/state/workflow-state.js | Sync copy of workflow-state updates. |
| plugins/learn/lib/adapter-transforms.js | Sync copy of OpenCode transform update. |
| plugins/enhance/lib/state/workflow-state.js | Sync copy of workflow-state updates. |
| plugins/enhance/lib/adapter-transforms.js | Sync copy of OpenCode transform update. |
| plugins/drift-detect/lib/state/workflow-state.js | Sync copy of workflow-state updates. |
| plugins/drift-detect/lib/adapter-transforms.js | Sync copy of OpenCode transform update. |
| plugins/deslop/lib/state/workflow-state.js | Sync copy of workflow-state updates. |
| plugins/deslop/lib/adapter-transforms.js | Sync copy of OpenCode transform update. |
| plugins/debate/lib/state/workflow-state.js | Sync copy of workflow-state updates. |
| plugins/debate/lib/adapter-transforms.js | Sync copy of OpenCode transform update. |
| plugins/audit-project/lib/state/workflow-state.js | Sync copy of workflow-state updates. |
| plugins/audit-project/lib/adapter-transforms.js | Sync copy of OpenCode transform update. |
| lib/state/workflow-state.js | Canonical workflow-state changes: new phases, RESULT_FIELD_MAP, unknown-phase guard, store falsy results. |
| lib/adapter-transforms.js | Canonical OpenCode adapter transform now emits a completePhase instruction when present in JS fences. |
| adapters/opencode/skills/orchestrate-review/SKILL.md | Regenerated OpenCode skill reflecting updated guidance/version. |
| adapters/opencode/skills/discover-tasks/SKILL.md | Regenerated OpenCode skill now includes completePhase instruction instead of dropping it. |
| adapters/opencode/commands/ship.md | Regenerated OpenCode command now includes completePhase instruction. |
| adapters/opencode/commands/next-task.md | Regenerated OpenCode command reflecting Phase 8/9 changes and completePhase instruction. |
| adapters/opencode/agents/implementation-agent.md | Regenerated OpenCode agent now includes completePhase instruction. |
| adapters/opencode/agents/exploration-agent.md | Regenerated OpenCode agent now includes completePhase instruction. |
| adapters/opencode/agents/ci-monitor.md | Regenerated OpenCode agent now includes completePhase instruction. |
| adapters/codex/skills/next-task/SKILL.md | Regenerated Codex skill reflecting Phase 8/9 changes. |
| tests/workflow-state.test.js | Adds test coverage for new phases, result persistence, and unknown-phase handling. |
| CHANGELOG.md | Documents the review-loop exit-condition fix and related workflow-state changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e44faa0bc1
ℹ️ 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".
- 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)
|
Addressed all 7 review comments in 871dfa1: Thread 1 (Gemini): Already fixed in e44faa0 — Thread 2 (Codex P1): Phase 8 now derives Thread 3 (Codex P2): By design — the OpenCode transform produces generic prose instructions since OpenCode cannot execute JavaScript. The Thread 4 (Copilot): Changed Thread 5 (Copilot): Phase 8 completePhase payload now includes Thread 6 (Copilot): Blocked review loop now uses Thread 7 (Codex): The OpenCode delivery-validator adapter inherits from the canonical plugin source via gen-adapters. The failure path ( |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 871dfa1e93
ℹ️ 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".
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.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
lib/adapter-transforms.js:108
transformBodyForOpenCodenow treats any fenced block containingcompletePhaseas “not executable” and converts it to bullet instructions, but the instruction extraction only emits acompletePhasebullet. This drops other critical state calls in the same block (notablyworkflowState.failPhase(...)), which causes generated OpenCode docs to lose failure-path guidance (e.g., validate-delivery / delivery-validator no longer mention failing the phase). Extend the extraction to detect and emit a bullet forfailPhase(and considerupdateState/updateFlowif present) so generated adapters preserve both success and failure state transitions.
if (code.includes('require(') || code.includes('Task(') ||
code.includes('const ') || code.includes('let ') ||
code.includes('function ') || code.includes('=>') ||
code.includes('async ') || code.includes('await ') ||
code.includes('completePhase')) {
let instructions = '';
const taskMatches = [...code.matchAll(/(?:await\s+)?Task\s*\(\s*\{[^}]*subagent_type:\s*["'](?:[^"':]+:)?([^"']+)["'][^}]*\}\s*\)/gs)];
for (const taskMatch of taskMatches) {
const agent = taskMatch[1];
instructions += `- Invoke \`@${agent}\` agent\n`;
}
const phaseMatches = code.match(/startPhase\s*\(\s*['"]([^'"]+)['"]\s*\)/g);
if (phaseMatches) {
for (const pm of phaseMatches) {
const phase = pm.match(/['"]([^'"]+)['"]/)[1];
instructions += `- Phase: ${phase}\n`;
}
}
if (code.includes('AskUserQuestion')) {
instructions += '- Use AskUserQuestion tool for user input\n';
}
if (code.includes('EnterPlanMode')) {
instructions += '- Use EnterPlanMode for user approval\n';
}
if (code.includes('completePhase')) {
instructions += '- Call `workflowState.completePhase(result)` to advance workflow state\n';
}
if (instructions) {
return instructions;
}
return '*(JavaScript reference - not executable in OpenCode)*';
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const question = `Review loop blocked (${reason}). Open issues remain. How should we proceed?`; | ||
| const response = AskUserQuestion({ | ||
| questions: [{ | ||
| question, | ||
| header: 'Review Blocked', | ||
| multiSelect: false, | ||
| options: [ | ||
| { label: 'Override and proceed', description: 'Advance to delivery-validation with unresolved issues (risky)' }, | ||
| { label: 'Abort workflow', description: 'Stop here; open issues must be fixed manually' } | ||
| ] | ||
| }] | ||
| }); | ||
| // AskUserQuestion returns { answers: { [questionText]: selectedLabel } } | ||
| const choice = response.answers?.[question] ?? response[question]; | ||
| if (choice === 'Override and proceed') { |
There was a problem hiding this comment.
AskUserQuestion is used here without await, and the code assumes a return shape of { answers: { [questionText]: selectedLabel } }. Elsewhere in this repo, examples typically await AskUserQuestion(...) and treat the result as a string or an array (e.g., const choice = decision[0]). To avoid misleading orchestration logic, update this snippet to await the tool call and parse the result in a way that matches the established patterns (e.g., handle string/array return), rather than relying on response.answers?.[...].
| completePhase({ passed: false, reason: 'lint-failure' }, testDir); | ||
|
|
||
| const flow = readFlow(testDir); | ||
| expect(flow.preReviewResult).toBeDefined(); | ||
| expect(flow.preReviewResult.passed).toBe(false); |
There was a problem hiding this comment.
This test claims to validate the new completePhase behavior for “falsy” results, but it passes an object ({ passed: false, ... }), which is truthy and would have been stored even before changing if (result) to the null/undefined check. If the intent is to prevent regressions where completePhase(false) / completePhase(0) / completePhase('') would be dropped, adjust the test to pass an actually-falsy result value and assert it is persisted.
| completePhase({ passed: false, reason: 'lint-failure' }, testDir); | |
| const flow = readFlow(testDir); | |
| expect(flow.preReviewResult).toBeDefined(); | |
| expect(flow.preReviewResult.passed).toBe(false); | |
| completePhase(false, testDir); | |
| const flow = readFlow(testDir); | |
| expect(flow.preReviewResult).toBe(false); |
Summary
Fixes #235 — the Phase 9 review loop was stopping prematurely even when unresolved issues remained.
Root causes fixed
workflow-state.jsPHASES array:pre-review-gatesanddocs-updatewere referenced bystartPhase()calls but absent from the array, causing runtime crashesupdateFlowinstead ofcompletePhaseinorchestrate-review/SKILL.md: the phase never formally advanced, blocking delivery-validationstartPhase('review-loop')in Phase 9: all other phases had this call; review-loop was the only missing oneMAX_STALLSmismatch: constant was 2 (requiring 3 identical consecutive iterations) but docs said "2 consecutive" — aligned toMAX_STALLS=1completePhase()in Phase 8 (pre-review-gates): result was silently discardedcompletePhaselost in OpenCode adapter transform: added extraction handler toadapter-transforms.jsChanges
lib/state/workflow-state.jspre-review-gates+docs-updateto PHASES; hoistRESULT_FIELD_MAP; guardcompletePhaseagainst unknown phaselib/adapter-transforms.jscompletePhaseextraction so OpenCode adapters preserve the instructionplugins/next-task/skills/orchestrate-review/SKILL.mdupdateFlow→completePhase;MAX_STALLS2→1; addversion: 5.1.0plugins/next-task/commands/next-task.mdstartPhase('review-loop');completePhaseat Phase 8 + 9 end; strict stopping conditionsadapters/opencode/commands/next-task.mdadapters/codex/skills/next-task/SKILL.mdplugins/*/lib/state/workflow-state.js(11 files)__tests__/workflow-state.test.jsTest Plan
npm test— 98 suites, 4034 tests pass, 0 failuresnpx agentsys-dev preflight --all— all 15 checks passnpx agentsys-dev validate— all validators pass"orchestrator may override"text anywhere in project filesstartPhase('review-loop')present in all 3 platform command filescompletePhaseused inorchestrate-review/SKILL.mdfor both approved and blocked paths/enhancerun on modified agents/skills