Fix human_review status bugs (#1149, #509) #1425
Conversation
This PR fixes two related bugs where tasks incorrectly show "Needs Resume" or get stuck in human_review status: ## Fix 1: Planning crash leaves task in human_review (AndyMik90#1149) When planning crashes and leaves no phases/subtasks, `update_status_from_subtasks()` was preserving the human_review status even though there was no valid plan. This caused tasks to show "Incomplete + Needs Resume" in the UI. **Solution:** Check if status is human_review but phases list is empty - this indicates planning failed. Reset to backlog with a recovery note instead. ## Fix 2: Task stuck in human_review after approval (AndyMik90#509) After a user approves a plan, clicking "Resume" would not start execution because there was no code to transition the status from human_review to in_progress when continuing a build. **Solution:** In the "continuing build" path of coder.py, check if the plan is in human_review status and if so, validate the approval and transition to in_progress before starting execution.
|
Matthew Larson seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
Summary of ChangesHello @elmateo487, 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 two critical bugs related to task status management, specifically addressing scenarios where tasks would incorrectly remain in "human_review" or display misleading statuses. The changes enhance the robustness of the task workflow by ensuring proper state transitions after planning failures and user approvals, leading to a more reliable and predictable user experience. 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
|
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughThe changes restore correct plan state transitions: recover tasks left in Changes
Sequence Diagram(s)sequenceDiagram
participant Agent as Agent (coder)
participant FS as FileSystem (implementation_plan.json, review_state.json)
participant Plan as ImplementationPlan
participant Frontend as Frontend / IPC
Agent->>FS: load implementation_plan.json
Agent->>Plan: parse plan (status, planStatus, phases)
alt plan.status == "human_review" and plan.planStatus == "review"
Agent->>FS: load review_state.json
alt review.approved == true
Agent->>Plan: set status/planStatus -> "in_progress"
Agent->>FS: save implementation_plan.json
Agent->>Frontend: emit build/resume start
Frontend->>Agent: begin execution / dispatch subtasks
else review not approved
Agent->>Frontend: warn "re-approval required", abort resume
end
else
Agent->>Frontend: continue resume or start as normal
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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 addresses two important bugs that cause tasks to get stuck in the human_review status. The fix in implementation_plan/plan.py for handling planning crashes is well-implemented. However, the change in apps/backend/agents/coder.py introduces a critical bug. It incorrectly handles the ImplementationPlan, treating a dictionary as an object, which would lead to a runtime error. I've provided a detailed comment with a code suggestion to correct this issue.
| plan = load_implementation_plan(spec_dir) | ||
| if plan and plan.status == "human_review" and plan.planStatus == "review": | ||
| from review.state import ReviewState | ||
| review_state = ReviewState.load(spec_dir) | ||
| if review_state.is_approval_valid(spec_dir): | ||
| # User approved the plan - transition to in_progress now that execution begins | ||
| plan.status = "in_progress" | ||
| plan.planStatus = "in_progress" | ||
| plan_file = spec_dir / "implementation_plan.json" | ||
| plan.save(plan_file) | ||
| print_status("Transitioned from human_review to in_progress after approval", "success") | ||
| else: | ||
| # Plan changed after approval or not approved - stay in human_review | ||
| print_status("Plan requires re-approval before execution can continue", "warning") | ||
| return |
There was a problem hiding this comment.
This block has a bug: load_implementation_plan from agents.utils returns a dictionary, but the code attempts to access attributes (.status) and methods (.save()) as if plan were an object. This will raise an AttributeError and cause a crash.
The logic should be updated to correctly load an ImplementationPlan object instance to allow for attribute access and use of the .save() method.
| plan = load_implementation_plan(spec_dir) | |
| if plan and plan.status == "human_review" and plan.planStatus == "review": | |
| from review.state import ReviewState | |
| review_state = ReviewState.load(spec_dir) | |
| if review_state.is_approval_valid(spec_dir): | |
| # User approved the plan - transition to in_progress now that execution begins | |
| plan.status = "in_progress" | |
| plan.planStatus = "in_progress" | |
| plan_file = spec_dir / "implementation_plan.json" | |
| plan.save(plan_file) | |
| print_status("Transitioned from human_review to in_progress after approval", "success") | |
| else: | |
| # Plan changed after approval or not approved - stay in human_review | |
| print_status("Plan requires re-approval before execution can continue", "warning") | |
| return | |
| from implementation_plan.plan import ImplementationPlan | |
| plan_file = spec_dir / "implementation_plan.json" | |
| if plan_file.exists(): | |
| plan = ImplementationPlan.load(plan_file) | |
| if plan.status == "human_review" and plan.planStatus == "review": | |
| from review.state import ReviewState | |
| review_state = ReviewState.load(spec_dir) | |
| if review_state.is_approval_valid(spec_dir): | |
| # User approved the plan - transition to in_progress now that execution begins | |
| plan.status = "in_progress" | |
| plan.planStatus = "in_progress" | |
| plan.save(plan_file) | |
| print_status("Transitioned from human_review to in_progress after approval", "success") | |
| else: | |
| # Plan changed after approval or not approved - stay in human_review | |
| print_status("Plan requires re-approval before execution can continue", "warning") | |
| return |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| plan = load_implementation_plan(spec_dir) | ||
| if plan and plan.status == "human_review" and plan.planStatus == "review": | ||
| from review.state import ReviewState | ||
| review_state = ReviewState.load(spec_dir) | ||
| if review_state.is_approval_valid(spec_dir): | ||
| # User approved the plan - transition to in_progress now that execution begins | ||
| plan.status = "in_progress" | ||
| plan.planStatus = "in_progress" | ||
| plan_file = spec_dir / "implementation_plan.json" | ||
| plan.save(plan_file) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/backend/agents/coder.py`:
- Around line 195-205: The transition to in_progress is being overwritten
because ImplementationPlan.save() calls update_status_from_subtasks() which
recomputes status; modify the flow so the explicit in_progress transition
persists: either add a save(preserve_status=True) (or a
persist_without_recompute()/write_raw(path)) to ImplementationPlan and call that
here after setting plan.status and plan.planStatus, or update
ImplementationPlan.save to accept an option to skip update_status_from_subtasks;
then use that new API in the block that handles approval (the code that calls
load_implementation_plan, ReviewState.load, review_state.is_approval_valid) so
the in_progress value is written to disk without being recomputed away.
In `@apps/backend/implementation_plan/plan.py`:
- Around line 170-178: Run the Python formatter (ruff format) on the
implementation plan module to fix CI formatting failures: apply ruff format to
the file containing the code that checks self.status == "human_review" and
self.phases (the block that sets self.status, self.planStatus, and
self.recoveryNote), re-stage the reformatted
apps/backend/implementation_plan/plan.py, and re-commit so the CI ruff check
passes.
- Around line 170-178: The current check only resets when self.status ==
"human_review" and not self.phases, which misses cases where phases exist but
contain zero subtasks; update the condition in the same block (where
self.status, self.planStatus, and self.recoveryNote are set) to detect "no
subtasks" by verifying that none of the phases have subtasks (e.g.,
replace/not-only-check self.phases with a test like "no phase in self.phases has
subtasks" or using any()/all() over phase.subtasks) and then perform the same
reset to backlog, planStatus="pending" and set recoveryNote.
| plan = load_implementation_plan(spec_dir) | ||
| if plan and plan.status == "human_review" and plan.planStatus == "review": | ||
| from review.state import ReviewState | ||
| review_state = ReviewState.load(spec_dir) | ||
| if review_state.is_approval_valid(spec_dir): | ||
| # User approved the plan - transition to in_progress now that execution begins | ||
| plan.status = "in_progress" | ||
| plan.planStatus = "in_progress" | ||
| plan_file = spec_dir / "implementation_plan.json" | ||
| plan.save(plan_file) | ||
| print_status("Transitioned from human_review to in_progress after approval", "success") |
There was a problem hiding this comment.
plan.save() recomputes status and can undo the in_progress transition.
ImplementationPlan.save() calls update_status_from_subtasks(), which sets backlog/pending when all subtasks are still pending (common right after approval). That means this transition likely won’t persist to disk. Consider preserving explicit in_progress in the “all pending” branch or persisting without recomputing status.
🐛 One possible fix (apps/backend/implementation_plan/plan.py)
- if self.status == "human_review" and self.planStatus == "review":
- pass
- else:
- self.status = "backlog"
- self.planStatus = "pending"
+ if self.status == "human_review" and self.planStatus == "review":
+ pass
+ elif self.status == "in_progress" and self.planStatus == "in_progress":
+ # Preserve explicit transition after approval
+ pass
+ else:
+ self.status = "backlog"
+ self.planStatus = "pending"🧰 Tools
🪛 GitHub Actions: Lint
[error] 195-195: Ruff format check detected formatting changes. 2 files would be reformatted. Run 'ruff format' to fix formatting.
🤖 Prompt for AI Agents
In `@apps/backend/agents/coder.py` around lines 195 - 205, The transition to
in_progress is being overwritten because ImplementationPlan.save() calls
update_status_from_subtasks() which recomputes status; modify the flow so the
explicit in_progress transition persists: either add a
save(preserve_status=True) (or a persist_without_recompute()/write_raw(path)) to
ImplementationPlan and call that here after setting plan.status and
plan.planStatus, or update ImplementationPlan.save to accept an option to skip
update_status_from_subtasks; then use that new API in the block that handles
approval (the code that calls load_implementation_plan, ReviewState.load,
review_state.is_approval_valid) so the in_progress value is written to disk
without being recomputed away.
| # No subtasks yet - check if this is a failed planning state | ||
| # Fix for #1149: If status is human_review but no phases/subtasks exist, | ||
| # planning crashed - reset to backlog instead of preserving invalid state | ||
| if self.status == "human_review" and not self.phases: | ||
| # Planning failed - don't show as human_review with Resume button | ||
| self.status = "backlog" | ||
| self.planStatus = "pending" | ||
| self.recoveryNote = "Planning phase failed - no implementation plan created" | ||
| return |
There was a problem hiding this comment.
Ruff format is failing for this file.
CI indicates ruff format would reformat this file. Please run ruff format and re-commit.
🧰 Tools
🪛 GitHub Actions: Lint
[error] 174-174: Ruff format check detected formatting changes. Run 'ruff format' to fix formatting.
🤖 Prompt for AI Agents
In `@apps/backend/implementation_plan/plan.py` around lines 170 - 178, Run the
Python formatter (ruff format) on the implementation plan module to fix CI
formatting failures: apply ruff format to the file containing the code that
checks self.status == "human_review" and self.phases (the block that sets
self.status, self.planStatus, and self.recoveryNote), re-stage the reformatted
apps/backend/implementation_plan/plan.py, and re-commit so the CI ruff check
passes.
Broaden the failed-planning reset to cover empty phases with zero subtasks.
In this not all_subtasks branch, gating on not self.phases misses cases where phases exist but are empty (partial/crashed write). That still leaves human_review stuck with no actionable subtasks. Consider treating any human_review + no subtasks as a failed plan here.
🐛 Proposed fix
- if self.status == "human_review" and not self.phases:
+ if self.status == "human_review":📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # No subtasks yet - check if this is a failed planning state | |
| # Fix for #1149: If status is human_review but no phases/subtasks exist, | |
| # planning crashed - reset to backlog instead of preserving invalid state | |
| if self.status == "human_review" and not self.phases: | |
| # Planning failed - don't show as human_review with Resume button | |
| self.status = "backlog" | |
| self.planStatus = "pending" | |
| self.recoveryNote = "Planning phase failed - no implementation plan created" | |
| return | |
| # No subtasks yet - check if this is a failed planning state | |
| # Fix for `#1149`: If status is human_review but no phases/subtasks exist, | |
| # planning crashed - reset to backlog instead of preserving invalid state | |
| if self.status == "human_review": | |
| # Planning failed - don't show as human_review with Resume button | |
| self.status = "backlog" | |
| self.planStatus = "pending" | |
| self.recoveryNote = "Planning phase failed - no implementation plan created" | |
| return |
🧰 Tools
🪛 GitHub Actions: Lint
[error] 174-174: Ruff format check detected formatting changes. Run 'ruff format' to fix formatting.
🤖 Prompt for AI Agents
In `@apps/backend/implementation_plan/plan.py` around lines 170 - 178, The current
check only resets when self.status == "human_review" and not self.phases, which
misses cases where phases exist but contain zero subtasks; update the condition
in the same block (where self.status, self.planStatus, and self.recoveryNote are
set) to detect "no subtasks" by verifying that none of the phases have subtasks
(e.g., replace/not-only-check self.phases with a test like "no phase in
self.phases has subtasks" or using any()/all() over phase.subtasks) and then
perform the same reset to backlog, planStatus="pending" and set recoveryNote.
When the planning agent exits with a non-zero code (crash/failure), the exit handler was unconditionally setting status to human_review. This caused tasks to show "Needs Resume" even when planning hadn't created any phases/subtasks. The fix adds the same subtask check that exists for successful exits (code=0) to the failure branch. If no subtasks exist when planning fails, the task is reset to backlog so the user can retry, rather than being stuck in human_review. Fixes AndyMik90#1149, AndyMik90#509
2 participants
Summary
This PR fixes two related bugs where tasks incorrectly show "Needs Resume" or get stuck in
human_reviewstatus:Fix 1: Planning crash leaves task in human_review (#1149)
When planning crashes and leaves no phases/subtasks,
update_status_from_subtasks()was preserving thehuman_reviewstatus even though there was no valid plan. This caused tasks to show "Incomplete + Needs Resume" in the UI with 0 subtasks.Solution: Check if status is
human_reviewbut phases list is empty - this indicates planning failed. Reset tobacklogwith a recovery note instead.File:
apps/backend/implementation_plan/plan.pyFix 2: Task stuck in human_review after approval (#509)
After a user approves a plan, clicking "Resume" would not start execution because there was no code to transition the status from
human_reviewtoin_progresswhen continuing a build. The task would remain stuck forever.Solution: In the "continuing build" path of
coder.py, check if the plan is inhuman_reviewstatus and if so, validate the approval and transition toin_progressbefore starting execution.File:
apps/backend/agents/coder.pyTest plan
Related Issues
Fixes #1149
Fixes #509
Related to #457, #585
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.