fix(config): resolve dead code and dictionary shallow update issues i…#5934
fix(config): resolve dead code and dictionary shallow update issues i…#59342bright wants to merge 3 commits into
Conversation
…n process_config - Remove the incorrect 'continue' statement that caused unreachable code for dict updates. - Implement a recursive `dict_deep_update` helper to support deep merging of nested YAML configurations. - Adjust update order to ensure user-explicit arguments properly override default configurations.
|
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 (1)
📝 WalkthroughWalkthroughprocess_config now deep-merges nested dictionaries when applying config values, deep-copies dict/list values, skips unknown model fields, preserves existing non-dict overrides, and uses a new recursive _dict_deep_update helper for in-place nested dict updates. ChangesConfiguration Deep Merge
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
lib/crewai/src/crewai/utilities/config.py (2)
22-27: ⚡ Quick winFix indentation to use 4 spaces per level.
The function body uses 2-space indentation increment instead of Python's conventional 4 spaces. This is inconsistent with the rest of the file (lines 18-19, 31-43) and violates PEP 8.
📐 Proposed fix for indentation
def dict_deep_update(to_dict: dict[str, Any], from_dict: dict[str, Any]) -> None: - """Internal helper to recursively update to_dict with from_dict values in-place.""" - for key, value in from_dict.items(): - if key in to_dict and isinstance(to_dict[key], dict) and isinstance(value, dict): - dict_deep_update(to_dict[key], value) - else: - to_dict[key] = value + """Internal helper to recursively update to_dict with from_dict values in-place.""" + for key, value in from_dict.items(): + if key in to_dict and isinstance(to_dict[key], dict) and isinstance(value, dict): + dict_deep_update(to_dict[key], value) + else: + to_dict[key] = value🤖 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/src/crewai/utilities/config.py` around lines 22 - 27, The function dict_deep_update has its body indented with 2 spaces; update the indentation to use 4 spaces per level throughout the function (the for loop, the nested if isinstance(...) and its recursive call dict_deep_update(to_dict[key], value), and the else branch assigning to_dict[key] = value) so it matches the rest of the file and PEP8 style.
34-38: ⚡ Quick winConsider deep copying override values to prevent aliasing bugs.
When both the override and config values are dictionaries, the current implementation deep copies the config value but not the override value before merging. This means
values[key]will contain references to mutable objects fromoverride_value, potentially causing aliasing bugs if the same override dict is passed to multiple model instances or modified elsewhere.For consistency with how config values are handled (line 36, 40) and to prevent unexpected shared mutable state, consider deep copying the override value before merging.
🔒 Proposed fix to deep copy override values
if (override_value := values.get(key)) is not None: if isinstance(override_value, dict) and isinstance(value, dict): config_value = copy.deepcopy(value) - dict_deep_update(config_value, override_value) + dict_deep_update(config_value, copy.deepcopy(override_value)) values[key] = config_valueAlternatively, if you want to deep copy override values consistently for dicts and lists:
if (override_value := values.get(key)) is not None: if isinstance(override_value, dict) and isinstance(value, dict): config_value = copy.deepcopy(value) - dict_deep_update(config_value, override_value) + override_copy = copy.deepcopy(override_value) if isinstance(override_value, (dict, list)) else override_value + dict_deep_update(config_value, override_copy) values[key] = config_value🤖 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/src/crewai/utilities/config.py` around lines 34 - 38, The merge currently deep-copies the original config (config_value = copy.deepcopy(value)) but passes override_value directly into dict_deep_update, leaving shared references; change the merge to deep-copy the override before merging (e.g., create override_copy = copy.deepcopy(override_value) and call dict_deep_update(config_value, override_copy)) so values[key] contains no references to the original override dict; reference symbols: values, key, override_value, config_value, dict_deep_update.
🤖 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.
Nitpick comments:
In `@lib/crewai/src/crewai/utilities/config.py`:
- Around line 22-27: The function dict_deep_update has its body indented with 2
spaces; update the indentation to use 4 spaces per level throughout the function
(the for loop, the nested if isinstance(...) and its recursive call
dict_deep_update(to_dict[key], value), and the else branch assigning
to_dict[key] = value) so it matches the rest of the file and PEP8 style.
- Around line 34-38: The merge currently deep-copies the original config
(config_value = copy.deepcopy(value)) but passes override_value directly into
dict_deep_update, leaving shared references; change the merge to deep-copy the
override before merging (e.g., create override_copy =
copy.deepcopy(override_value) and call dict_deep_update(config_value,
override_copy)) so values[key] contains no references to the original override
dict; reference symbols: values, key, override_value, config_value,
dict_deep_update.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b97d7154-db5a-4ebc-a054-f5f313572dcb
📒 Files selected for processing (1)
lib/crewai/src/crewai/utilities/config.py
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/crewai/src/crewai/utilities/config.py (1)
38-44: 💤 Low valueConsider using a well-established library for deep merging.
While the custom implementation is correct and straightforward, libraries like
mergedeepor similar provide battle-tested deep merge functionality with additional features (merge strategies, handling edge cases). This could reduce maintenance burden.🤖 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/src/crewai/utilities/config.py` around lines 38 - 44, The custom recursive updater _dict_deep_update should be replaced with a battle-tested deep-merge library to reduce maintenance; add a dependency such as mergedeep (or deepmerge), import its merge function, and replace calls to _dict_deep_update(to_dict, from_dict) with the library call that performs an in‑place merge (e.g., mergedeep.merge(to_dict, from_dict) or deepmerge.algo.merge(to_dict, from_dict) depending on chosen lib) to preserve the original in-place behavior; remove the custom _dict_deep_update implementation, update the module imports, and run/adjust any tests to ensure identical merge semantics (dict overwrites and nested dict merging) are maintained.
🤖 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/config.py`:
- Around line 38-44: The _dict_deep_update function body is indented with 2
spaces; reformat to use 4-space indentation per PEP 8, updating the docstring
line, the for key, value loop, the if/else block and the recursive
call/assignment lines so each block level uses 4 spaces (preserve existing
logic/function names exactly: _dict_deep_update, to_dict, from_dict).
---
Nitpick comments:
In `@lib/crewai/src/crewai/utilities/config.py`:
- Around line 38-44: The custom recursive updater _dict_deep_update should be
replaced with a battle-tested deep-merge library to reduce maintenance; add a
dependency such as mergedeep (or deepmerge), import its merge function, and
replace calls to _dict_deep_update(to_dict, from_dict) with the library call
that performs an in‑place merge (e.g., mergedeep.merge(to_dict, from_dict) or
deepmerge.algo.merge(to_dict, from_dict) depending on chosen lib) to preserve
the original in-place behavior; remove the custom _dict_deep_update
implementation, update the module imports, and run/adjust any tests to ensure
identical merge semantics (dict overwrites and nested dict merging) are
maintained.
🪄 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: 48a2a771-9e3c-44e1-ac20-2e6305effd35
📒 Files selected for processing (1)
lib/crewai/src/crewai/utilities/config.py
1 participant
…n process_config
dict_deep_updatehelper to support deep merging of nested YAML configurations.Summary by CodeRabbit