fix(tools): resolve a coworker that a model spelled as a quoted list - #7615
Lesereingrape wants to merge 4 commits into
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe change normalizes coworker values by removing surrounding whitespace and single or double quotes. A parameterized test verifies delegation for direct, list-style, JSON-style, and Python-repr spellings. ChangesCoworker resolution
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to The supported coworker spellings resolve through the shared normalization path, with no concrete behavior regression identified. The change is ready to merge after normal checks. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The change in ✨ 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 |
|
Heads-up for maintainers so this does not get reviewed twice: #7620 (opened a few hours after this one, same issue #7614) fixes the same defect in They are equivalent for every spelling in the issue. The strip here sits in the Either one closes #7614 — merging one and closing the other is fine by me. If the sanitizer route is preferred, the parametrized spelling table in this PR's test ( |
|
Two contributor-side items I cannot close myself — both need a maintainer with write access. 1. CI has not been allowed to run on this PR. On the current head, seven workflow runs are In the meantime, the equivalent evidence I can produce locally: the exact pytest and ruff commands 2. The Three sibling PRs from the same account were prepared the same way and need both items: #7610, #7612, One deliberate non-request: I have not pushed a merge of |
1c44fca to
89b0a57
Compare
Related issue
Fixes #7614
Summary
_get_coworker()already knows that a weak model may hand over a list where a string was expected, and unwraps[...]to its first entry — but it leaves that entry's quotes in place, and role matching (sanitize_agent_name(), same file) strips"only. So the JSON spelling["researcher"]delegates fine while the Python-repr spelling['researcher']— whatstr(list)produces — comes back as "coworker mentioned not found" while listing that very role as available:The unwrap now strips the wrapping quotes (and surrounding whitespace) once, after it, which covers the list form and the quoted-scalar form together for both
DelegateWorkToolandAskQuestionTool, since they share this method.Deliberately not done: teaching
sanitize_agent_name()to drop'too. Its own docstring ("quotes removed") invites that reading, but the function is applied to the agents' real role names as well as to the model's argument, so a role containing an apostrophe would stop matching._get_coworker()only ever sees the incoming argument.Behavior is unchanged for every spelling that works today:
.strip()and.strip("'\"")are no-ops on a bare name, on[researcher], and on["researcher"](the double quotes were already removed later, and the two paths now agree).Verification
Tests added or updated for the changed behavior
Relevant tests and quality checks pass locally
New
test_delegate_work_resolves_coworker_spellings, parametrized over the five spellings, withAgent.execute_taskstubbed so no LLM or cassette is involved. On3831e8b: 2 failed, 3 passed — the two single-quote spellings fail, and the three controls (researcher,[researcher],["researcher"]) pass both before and after, which is the no-regression evidence. On this branch: 5 passed.uv run pytest lib/crewai/tests/tools/agent_tools/test_agent_tools.py -q→ 13 passed (the 8 pre-existing VCR-replay tests included).Blast radius:
lib/crewai/tests/tools+lib/crewai/tests/agentsrun on the base and on this branch, fail-sets diffed → exactly the 2 new cases flip, 0 regressions. 32 failures remain identical on both sides in this environment (crewai_filesis not installed here, plus the platform/a2a suites that need it); CI installs the full workspace.uv run ruff check/ruff format --check→ clean on the changed source.uv run mypy→ no error inbase_agent_tools.py(the 10 reported errors are the pre-existingno-any-unimportedfindings increw_docling_source.py).Additional context
The comment above the matching code spells out why the tolerance exists — "less-powerful LLM's have difficulty producing valid JSON" — so a single-quoted argument is squarely inside the case this branch was written for, and it is the half that was missed. The user-visible symptom is the loop where the tool refuses a role and then prints it as an allowed option, which a model usually retries by passing the bare name; so it costs a wasted tool call rather than breaking the crew outright.
Follow-up this PR does not attempt:
coworker[1:-1].split(",")[0]silently drops every entry after the first, so['Researcher', 'Analyst']never considersAnalyst. Splitting a quoted name on,is also wrong for a role that contains a comma. Both are about which coworker a multi-entry list should pick, i.e. a design call rather than a missed guard, so they belong in their own issue if maintainers want them addressed.Authored with an AI coding assistant.
.github/CONTRIBUTING.mdrequires thellm-generatedlabel for agent-authored contributions and external contributors cannot apply it here — maintainers, please add it.Note
Low Risk
Small input-normalization change in shared coworker parsing for delegate/ask tools; existing spellings stay no-ops and behavior is covered by new tests.
Overview
Fixes coworker resolution when models pass Python-style quoted list strings (e.g.
['researcher']) to agent delegation tools.After
_get_coworker()unwraps bracketed list forms, it now strips surrounding whitespace and wrapping'/"on the chosen entry. That aligns single-quoted spellings with JSON-style["researcher"]before role matching, without changingsanitize_agent_name()(which would break roles with apostrophes).Adds a parametrized test so
DelegateWorkToolresolves bare names, bracket forms, and both quote styles to the same agent.Reviewed by Cursor Bugbot for commit 118a37f. Bugbot is set up for automated code reviews on this repo. Configure here.