refactor: migrate review-orchestrator to skill-based architecture - #143
Conversation
…-review skill Converts review-orchestrator from a subagent to a skill that provides concrete review guidance to the main next-task orchestrator. This enables direct reviewer invocation in Phase 9, solving the nested subagent limitation in Claude Code. Changes: - Create orchestrate-review skill with review pass definitions (191 lines) - Rewrite Phase 9 to mandate using the skill (minimal command bloat) - Update Codex installer to install skill to $CODEX_HOME/skills/ - Delete review-orchestrator agent (replaced by skill + direct orchestration) - Update hooks.json to reflect Phase 9 runs inline - Update CLAUDE.md workflow agents list Benefits: - Works in Claude Code (no nested subagents required) - Multiple parallel reviewer Task agents reduce rubber-stamping - Skill provides reusable review knowledge - Clear separation: command mandates, skill implements
Updates all documentation, agent files, and examples to reference the new Phase 9 review loop architecture instead of the removed review-orchestrator agent. Files updated: - 6 agent files (workflow diagrams, prerequisites) - 5 documentation files (workflows, reference, cross-platform) - 4 enhancer agents (invocation context) - 3 agent-docs files (workflow, OpenCode examples) - README.md (agent count, flow diagram) - CHANGELOG.md (enforcement gates) - opencode-plugin/index.ts (thinking budget config) All references to review-orchestrator removed or updated to Phase 9 review loop. Agent count: 29 → 28
…ices - Add concrete few-shot example finding to task prompt template - Remove disable-model-invocation to allow auto-invocation on "deep review" - Replace "Be thorough. Report EVERY issue." with more specific instruction - Follows prompt engineering, multi-agent, and instruction following best practices
- Add note about scope-based signal detection (changes vs project) - Reference skill from audit-project-agents.md for canonical pass definitions
- Add diverse example findings (security, code-quality, performance, false-positive) - Clarify confidence threshold for reporting - Add context for MAX_ITERATIONS and MAX_STALLS values - Add context for architecture trigger threshold (20+ files) - Improve fix implementation comment with guidance
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 introduces a significant architectural change by replacing the Highlights
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
|
![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: df10bf1ac8
ℹ️ 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".
| { | ||
| "type": "prompt", | ||
| "prompt": "## WORKFLOW ENFORCEMENT - SubagentStop Hook\n\nA subagent has completed. You MUST determine and execute the next workflow phase.\n\n### Check Workflow State\n\n```javascript\nconst state = workflowState.readState();\nconst completedAgent = state.phases.current;\nconst agentResult = // from the subagent output\n```\n\n### Decision Tree (MUST FOLLOW)\n\n1. **If implementation-agent completed**:\n - MUST run deslop-work AND test-coverage-checker (parallel)\n - ```javascript\n await Promise.all([\n Task({ subagent_type: 'next-task:deslop-work' }),\n Task({ subagent_type: 'next-task:test-coverage-checker' })\n ]);\n ```\n - Then MUST run review-orchestrator\n\n2. **If pre-review gates completed**:\n - MUST run review-orchestrator\n - ```javascript\n await Task({ subagent_type: 'next-task:review-orchestrator', model: 'opus' });\n ```\n\n3. **If review-orchestrator completed**:\n - Check if approved: `agentResult.approved === true`\n - If APPROVED: MUST run delivery-validator\n ```javascript\n await Task({ subagent_type: 'next-task:delivery-validator' });\n ```\n - If NOT APPROVED: MUST run deslop-work on fixes, then retry review\n\n4. **If delivery-validator completed**:\n - Check if approved: `agentResult.approved === true`\n - If APPROVED: MUST run docs-updater\n ```javascript\n await Task({ subagent_type: 'next-task:docs-updater' });\n ```\n - If NOT APPROVED: Return to implementation with fix instructions\n\n5. **If docs-updater completed**:\n - MUST invoke /ship command\n - ```javascript\n await Skill({ skill: 'ship:ship' });\n ```\n\n### CRITICAL ENFORCEMENT\n\n```\n╔══════════════════════════════════════════════════════════════════╗\n║ ⛔ NEVER skip deslop-work or test-coverage-checker ║\n║ ⛔ NEVER skip review-orchestrator ║\n║ ⛔ NEVER skip delivery-validator ║\n║ ⛔ NEVER skip docs-updater ║\n║ ⛔ NEVER create PR before /ship is invoked ║\n║ ⛔ NEVER push to remote before /ship is invoked ║\n╚══════════════════════════════════════════════════════════════════╝\n```\n\n### Workflow Sequence (MANDATORY)\n\n```\nimplementation-agent\n ↓ (SubagentStop)\npre-review gates (deslop-work + test-coverage-checker)\n ↓ (SubagentStop)\nreview-orchestrator\n ↓ (SubagentStop, if approved)\ndelivery-validator\n ↓ (SubagentStop, if approved)\ndocs-updater\n ↓ (SubagentStop)\n/ship command\n```\n\nReturn: {\"ok\": true, \"nextPhase\": \"<phase-name>\"}" | ||
| "prompt": "## WORKFLOW ENFORCEMENT - SubagentStop Hook\n\nA subagent has completed. You MUST determine and execute the next workflow phase.\n\n### Check Workflow State\n\n```javascript\nconst state = workflowState.readState();\nconst completedAgent = state.phases.current;\nconst agentResult = // from the subagent output\n```\n\n### Decision Tree (MUST FOLLOW)\n\n1. **If implementation-agent completed**:\n - MUST run deslop-work AND test-coverage-checker (parallel)\n - ```javascript\n await Promise.all([\n Task({ subagent_type: 'next-task:deslop-work' }),\n Task({ subagent_type: 'next-task:test-coverage-checker' })\n ]);\n ```\n - Then Phase 9 (review loop) runs inline\n\n2. **If pre-review gates completed**:\n - Phase 9 (review loop) runs INLINE in main orchestrator\n - Review spawns parallel Task agents for each review pass\n - After review completes, MUST run delivery-validator\n - ```javascript\n await Task({ subagent_type: 'next-task:delivery-validator' });\n ```\n\n4. **If delivery-validator completed**:\n - Check if approved: `agentResult.approved === true`\n - If APPROVED: MUST run docs-updater\n ```javascript\n await Task({ subagent_type: 'next-task:docs-updater' });\n ```\n - If NOT APPROVED: Return to implementation with fix instructions\n\n5. **If docs-updater completed**:\n - MUST invoke /ship command\n - ```javascript\n await Skill({ skill: 'ship:ship' });\n ```\n\n### CRITICAL ENFORCEMENT\n\nNEVER skip deslop-work or test-coverage-checker.\nNEVER skip Phase 9 review loop.\nNEVER skip delivery-validator.\nNEVER skip docs-updater.\nNEVER create PR before /ship is invoked.\nNEVER push to remote before /ship is invoked.\n\n### Workflow Sequence (MANDATORY)\n\n```\nimplementation-agent\n ↓ (SubagentStop)\npre-review gates (deslop-work + test-coverage-checker)\n ↓ (SubagentStop)\nPhase 9: review loop (inline in main orchestrator)\n ↓ (SubagentStop)\ndelivery-validator\n ↓ (SubagentStop, if approved)\ndocs-updater\n ↓ (SubagentStop)\n/ship command\n```\n\nReturn: {\"ok\": true, \"nextPhase\": \"<phase-name>\"}" |
There was a problem hiding this comment.
The updated SubagentStop prompt now instructs the orchestrator to run delivery-validator immediately after the Phase 9 review loop completes, without checking whether the review loop approved or reported blocked: true. If the review loop finds open issues or hits its stall/iteration limit, this instruction still advances the workflow, effectively bypassing the review gate and allowing delivery validation (and potentially /ship) on unreviewed or blocked changes. The prior flow explicitly gated on approval. Consider reinstating the approval/blocked check (or re-run/override logic) before proceeding to delivery-validator.
Useful? React with 👍 / 👎.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
![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 pull request successfully migrates the review-orchestrator agent to a skill-based architecture, aiming to reduce codebase size and improve review quality. While the refactor is generally well-executed and consistent across the repository, a critical security risk has been identified. The automated iteration loop defined in orchestrate-review/SKILL.md currently allows for the automatic application of LLM-generated code suggestions without human oversight. This creates a vulnerability where malicious code could be injected via indirect prompt injection. It is strongly recommended to introduce a mandatory human-in-the-loop approval step before any code changes are applied to mitigate this risk.
| for (const issue of [...findings.bySeverity.critical, ...findings.bySeverity.high, | ||
| ...findings.bySeverity.medium, ...findings.bySeverity.low]) { | ||
| if (!issue.falsePositive) { | ||
| // Read file, locate issue.line, apply issue.suggestion via Edit tool | ||
| // For complex fixes, use simple-fixer agent pattern | ||
| } | ||
| } |
There was a problem hiding this comment.
The orchestrate-review skill defines an automated iteration loop that instructs the agent to apply code suggestions generated by LLM Task agents directly to the codebase using the Edit tool. Since these suggestions are derived from reviewing potentially untrusted file content (which can contain indirect prompt injections), an attacker could manipulate the LLM into providing malicious code suggestions. The orchestrator, following this skill's logic, would then automatically apply these malicious changes to the repository without human intervention, leading to arbitrary code execution or repository compromise. This is a classic case of Insecure Output Handling where LLM-generated content is passed to a sensitive sink (code modification) without validation or human approval.
|
|
||
| ```javascript | ||
| // 5 iterations balances thoroughness vs cost; 2 stalls indicates fixes aren't progressing | ||
| const MAX_ITERATIONS = 5, MAX_STALLS = 2; |
There was a problem hiding this comment.
The MAX_ITERATIONS and MAX_STALLS values are hardcoded within the skill definition. While these are reasonable defaults, it would enhance flexibility and maintainability if these limits could be configured dynamically, perhaps via environment variables or a workflow state parameter. This would allow for easier adjustments to review policies without modifying the core skill logic.
| const MAX_ITERATIONS = 5, MAX_STALLS = 2; | |
| const MAX_ITERATIONS = Number(process.env.REVIEW_MAX_ITERATIONS || 5); | |
| const MAX_STALLS = Number(process.env.REVIEW_MAX_STALLS || 2); |
Address Gemini code review feedback about automated fix application. Clarifies that orchestrator validates fixes before applying and that critical/high severity findings should be reviewed, not blindly applied.
|
Thanks for the security review @gemini-code-assist! I've addressed this concern in commit b3fc6b6:
The key safeguard is that fixes flow through the normal Edit tool permission system, which allows users to review and reject changes. The skill is guidance for the orchestrator, not an autonomous execution engine. |
|
Thanks for the update, @avifenesh! I appreciate you addressing the security considerations. The added Security Note in the |
Code reviewFound 2 documentation inconsistencies where agent counts were not updated after removing the Issue 1: CLAUDE.md agent count not updatedFile: Evidence:
CLAUDE.md reference: This violates Critical Rule #6 which requires checking completion requirements before delivery. The architecture documentation must stay accurate. Link to code: https://github.com/avifenesh/awesome-slash/blob/b3fc6b67066ed29cda8643e14b2e0eef93469e40/CLAUDE.md#L51-L53 Issue 2: AGENTS.md counts not updatedFile: Evidence:
Actual counts after this PR:
CLAUDE.md reference: This violates Critical Rule #6 about checking completion requirements and Critical Rule #1 about maintaining accurate documentation in a production project. Links to code:
|
- CLAUDE.md: 14 → 12 agents in next-task - AGENTS.md: 32 → 31 total agents - AGENTS.md: 22 → 21 file-based agents
|
Fixed both documentation inconsistencies in commit 531f0ba: Issue 1 - CLAUDE.md agent count:
Issue 2 - AGENTS.md counts:
Thanks for catching these @github-actions! |
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
The mcp-server tests require @modelcontextprotocol/sdk to run. This was a pre-existing issue where the dependency was missing.
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
1 similar comment
Code reviewNo issues found. Checked for bugs and CLAUDE.md compliance. |
1 participant
Summary
review-orchestratoragent toorchestrate-reviewskillChanges
Created:
plugins/next-task/skills/orchestrate-review/SKILL.md(215 lines)Modified: Phase 9 in
next-task.mdto use skill instead of agentModified:
audit-project-agents.mdto reference skill for canonical pass definitionsUpdated: Codex installer to install skill to
$CODEX_HOME/skills/Deleted:
review-orchestrator.mdagent (811 lines)Updated: 20+ files to remove all review-orchestrator references
Architecture
Test Plan
npm test- All tests pass (except pre-existing MCP SDK issue)npm run validate:cross-platform- No issues found/enhanceanalysis applied