Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
| True | ||
| """ | ||
| return role_name.lower() in GENERIC_ROLE_NAMES | ||
| if not role_name or not str(role_name).strip(): |
There was a problem hiding this comment.
I ran this against b5d93ed in a clean python:3.10-slim container. Neither new branch is reachable from the three call sites in the package: all three pass a name that ContextUtility.sanitize_workflow_filename has already rewritten.
role_name sanitize() is_generic_role_name() same on master 13dc7a7d
'' -> 'agent' -> True True
' ' -> '___' -> False False
' worker ' -> '__worker__' -> False False
sanitize_workflow_filename returns agent when the sanitized string is empty (context_utils.py:308-309), and agent is already in GENERIC_ROLE_NAMES, so the empty case fell back correctly before this change. Spaces become underscores before the function is called, so .strip() finds no whitespace to remove and " " still resolves to a folder named ___, which is the symptom in your motivation.
The call sites are workforce.py:738 and :747 (both sanitize_workflow_filename output) and workflow_memory_manager.py:1368 (_get_sanitized_role_name, same function). I enumerated them with an ast pass over the package rather than a grep; the only other calls are the seven in your new test, which pass raw strings.
Moving the strip one level down, into sanitize_workflow_filename at line 301, gets both of your examples through the real path:
' ' -> 'agent' -> True
' worker ' -> 'worker' -> True
That was name.strip().lower().replace(" ", "_"), run at your head with the rest of the PR in place.
There was a problem hiding this comment.
Confirmed at 0b3f6fbb. Both of your motivating cases now reach the guard through the real call path, and the normal case is unchanged:
'' -> 'agent' -> True
' ' -> 'agent' -> True
' worker ' -> 'worker' -> True
'\t\n' -> 'agent' -> True
'Data Analyst' -> 'data_analyst' -> False
Ran in a clean python:3.10-slim container against sanitize_workflow_filename and is_generic_role_name as they stand at that SHA.
…ize_workflow_filename
|
Thanks @ebarkhordar for the detailed trace! You're completely right that I've updated |
Summary
Improves role name validation in
camel.societies.workforce.utils.is_generic_role_name():"") and whitespace-only strings as generic role names so that fallback logic to agent title/description is correctly triggered.GENERIC_ROLE_NAMESto handle inputs like" worker ".test_is_generic_role_nametotest/workforce/test_workforce.py.Motivation
When initializing workforce workers without an explicit role or with whitespace strings,
is_generic_role_name("")previously returnedFalsebecause""was not inGENERIC_ROLE_NAMES. This prevented the fallback identifier resolution from executing and caused empty folder paths during workflow organization.Changes
camel/societies/workforce/utils.py: Strip whitespace and returnTruefor empty/whitespace inputs inis_generic_role_name().test/workforce/test_workforce.py: Added test cases for standard, whitespace-padded, and empty role name handling.Tests
test_is_generic_role_namecovering case variations and edge cases.