fix(reasoning): detect a bare READY marker in text plans#6488
fix(reasoning): detect a bare READY marker in text plans#6488HumphreySun98 wants to merge 2 commits into
Conversation
The refine prompt (and one of the create prompts) tells the model to "Conclude with READY or NOT READY", so models — especially on the text-parsing path — emit a bare `READY`. But the parser only accepted the exact sentence "READY: I am ready to execute the task.", so `READY` was never detected as ready and the agent stayed stuck refining until max attempts. Add a _detect_plan_ready helper that matches the last READY / NOT READY marker (NOT READY wins over the bare READY it contains; the full sentence still counts as ready) and use it at all three text-detection sites. Fixes crewAIInc#6204 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughPlan readiness detection now uses a regex-based parser that identifies the last ChangesPlan readiness detection fix
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/crewai/tests/utilities/test_structured_planning.py (1)
28-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a false-positive guard test.
The suite covers positive/negative/precedence/case scenarios well. Consider adding a test to verify that common English words containing "ready" (e.g., "already", "readiness") do not trigger a false READY — this directly guards the regex word-boundary concern in the source.
✨ Suggested test
+ def test_word_containing_ready_is_not_a_marker(self): + assert _detect_plan_ready("I am already prepared to proceed.") is False + assert _detect_plan_ready("The readiness check is complete.") is False🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/crewai/tests/utilities/test_structured_planning.py` around lines 28 - 52, Add a false-positive guard test in test_structured_planning for _detect_plan_ready so that ordinary words containing “ready” like “already” or “readiness” do not match as READY. Update the test suite near the existing _detect_plan_ready and AgentReasoning._parse_planning_response cases to assert these inputs return False, protecting the word-boundary behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/crewai/src/crewai/utilities/reasoning_handler.py`:
- Around line 31-33: The _READY_MARKER regex in reasoning_handler is too broad
because READY matches inside other words like already or readiness. Update the
pattern used by _READY_MARKER to require word boundaries around the READY token
(while keeping the NOT\s+READY alternative) so _detect_plan_ready only returns
true for an actual standalone readiness signal and not embedded substrings.
---
Nitpick comments:
In `@lib/crewai/tests/utilities/test_structured_planning.py`:
- Around line 28-52: Add a false-positive guard test in test_structured_planning
for _detect_plan_ready so that ordinary words containing “ready” like “already”
or “readiness” do not match as READY. Update the test suite near the existing
_detect_plan_ready and AgentReasoning._parse_planning_response cases to assert
these inputs return False, protecting the word-boundary behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 50296fc4-8581-4111-9b4d-106cd2b7863f
📒 Files selected for processing (2)
lib/crewai/src/crewai/utilities/reasoning_handler.pylib/crewai/tests/utilities/test_structured_planning.py
ErenAta16
left a comment
There was a problem hiding this comment.
Checked the regex logic directly: _detect_plan_ready correctly picks up a bare READY where the old exact-substring check ("READY: I am ready to execute the task." in response_str) would not — confirmed this difference standalone (old check returns False on a bare READY, new one returns True). The last-marker-wins design is the right call too: an earlier "not ready yet" mentioned mid-plan (common phrasing while a model reasons through steps) shouldn't override the concluding verdict, and the NOT prefix check correctly makes "NOT READY" win over the bare "READY" substring it contains rather than the regex accidentally matching on "READY" first. Applied consistently across all three text-detection sites per the diff, which matters since a partial fix (only one of the three sites) would have left the bug intermittently reproducible depending on which code path a given model hit.
Without word boundaries the pattern matched "ready" inside words like
"already", so plain prose ("I am already prepared") registered as READY, and a
trailing "already" could even flip a concluding NOT READY to ready. Anchor both
markers with \b and add regression tests.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Problem
With reasoning enabled, the executor asks the model to conclude its plan with READY or NOT READY and loops "refine plan" until ready. But there's a mismatch between what the prompts request and what the parser accepts:
refine_plan_prompt(and one of thecreate_plan_promptvariants) say: "Conclude with READY or NOT READY" → models emit a bareREADY.So a bare
READYis never detected as ready — the agent stays stuck refining until max attempts. This bites text-parsing models (the reporter used Ollama GLM), where the trace clearly showsREADYbut it's read as not-ready.Fix
Add a
_detect_plan_readyhelper and use it at all three text-detection sites:READY.NOT READYwins over the bareREADYit contains."READY: I am ready to execute the task.") still counts as ready — backward compatible.Only the text-parsing path is affected; the structured/function-calling path already uses a boolean
readyfield.Behavior (old → new)
READYNOT READY: …READY: I am ready to execute the task.Tests
Added
TestDetectPlanReady(bare/full/none/mid-text/case-insensitive +_parse_planning_responseintegration). Fulltest_structured_planning.py(33) green;ruff+mypyclean. Branched off latestmain(v1.15.2).Fixes #6204
This PR was authored with Claude Code. Per
CONTRIBUTING.md, AI-generated contributions require thellm-generatedlabel — I don't have triage permission to set it, so could a maintainer please add it? 🤖 Generated with Claude Code