refactor(agent-core): extract shared agent logic into cuga_agent_core module#250
Open
sami-marreed wants to merge 7 commits into
Open
refactor(agent-core): extract shared agent logic into cuga_agent_core module#250sami-marreed wants to merge 7 commits into
sami-marreed wants to merge 7 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughLarge refactor introducing a shared CoreGraphAdapter and reusable graph/node helpers, migrating CugaLite and CugaSupervisor to adapter-driven graphs. Adds execution planning, runtime tool orchestration, policy adapter seams, SDK variables exposure, configuration validators, and extensive test updates. ChangesModular Agent Graph Core + Adapters
Sequence Diagram(s)(omitted) Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/cuga/backend/cuga_graph/nodes/cuga_supervisor/cuga_supervisor_graph.py (1)
39-59: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDuplicate function:
_resolve_names_from_caller_frameexists in both files.This function is duplicated here and in
supervisor_graph_adapter.py(lines 36-56). The adapter module's docstring states it was "moved from cuga_supervisor_graph.py", but the original remains. Consider removing this copy and importing from the adapter module if still needed elsewhere.♻️ Remove duplicate and import if needed
If this function is used elsewhere in this module, import it:
from cuga.backend.cuga_graph.nodes.cuga_supervisor.supervisor_graph_adapter import ( SupervisorGraphAdapter, + _resolve_names_from_caller_frame, ) - - -def _resolve_names_from_caller_frame(variable_names: List[str]) -> Dict[str, Any]: - """Resolve names from the delegated code's caller frame. - ... - """ - resolved: Dict[str, Any] = {} - frame = inspect.currentframe() - try: - caller = frame.f_back if frame is not None else None - if caller is None: - return resolved - for name in variable_names: - if name in caller.f_locals: - resolved[name] = caller.f_locals[name] - elif name in caller.f_globals: - resolved[name] = caller.f_globals[name] - finally: - del frame - return resolvedIf not used in this module, simply delete lines 39-59.
🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_supervisor/cuga_supervisor_graph.py` around lines 39 - 59, The function _resolve_names_from_caller_frame is duplicated; remove the copy in cuga_supervisor_graph.py and import the canonical implementation from supervisor_graph_adapter (where it was moved). Delete the local definition of _resolve_names_from_caller_frame in this file and add an import for _resolve_names_from_caller_frame from supervisor_graph_adapter at the top of the module (or leave no import if nothing in this module references it); ensure any callers in cuga_supervisor_graph.py now use the imported symbol.src/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.py (3)
143-177: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDuplicate helper functions remain.
_tool_call_kwarg_literal(lines 143-147) and_extract_code_from_response_tool_calls(lines 150-177) are duplicated fromagent_graph_adapter.py. Since the adapter module already contains these, this file should import them instead.♻️ Suggested approach
Import from the adapter module:
from cuga.backend.cuga_graph.nodes.cuga_lite.agent_graph_adapter import ( _tool_call_kwarg_literal, _extract_code_from_response_tool_calls, )Or, if these are only used within
agent_graph_adapter.py, remove them from this file entirely.🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.py` around lines 143 - 177, This file duplicates the helper functions _tool_call_kwarg_literal and _extract_code_from_response_tool_calls that already exist in agent_graph_adapter.py; remove the duplicated definitions from cuga_lite_graph.py and import them from cuga.backend.cuga_graph.nodes.cuga_lite.agent_graph_adapter instead (or if they are only used in agent_graph_adapter.py, delete them here entirely), ensuring any calls in cuga_lite_graph.py reference the imported _tool_call_kwarg_literal and _extract_code_from_response_tool_calls symbols.
647-665: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDuplicate
format_task_todos_system_blockandformat_current_plan_section.These functions exist in both files. The docstring in
agent_graph_adapter.pymentions these were "moved from cuga_lite_graph.py" but both files still contain them.Also applies to: 762-768
🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.py` around lines 647 - 665, There are duplicate implementations of format_task_todos_system_block and format_current_plan_section across cuga_lite_graph.py and agent_graph_adapter.py; remove the redundant copy and centralize the functions in one module, then update callers to import the single source. Specifically, keep the canonical implementations (pick either cuga_lite_graph.format_task_todos_system_block and format_current_plan_section or the versions in agent_graph_adapter), delete the duplicate functions from the other file, update any local imports/usages to from <chosen_module> import format_task_todos_system_block, format_current_plan_section, and adjust the docstring in agent_graph_adapter.py that states "moved from cuga_lite_graph.py" if you moved them the other direction. Ensure no remaining references to the removed functions and run tests to confirm names resolve.
504-507: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winDuplicate
_clean_empty_response_retry_metafunction.This function is also defined in
agent_graph_adapter.py(lines 37-40). Consolidate to avoid divergence.🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.py` around lines 504 - 507, There is a duplicate function named _clean_empty_response_retry_meta in cuga_lite_graph.py and agent_graph_adapter.py; remove the copy from cuga_lite_graph.py and instead import and use the single canonical _clean_empty_response_retry_meta defined in agent_graph_adapter.py (or move the function to a shared utility module and import it from both places), updating any references in CugaLiteGraph methods to call the imported function name _clean_empty_response_retry_meta so the logic is centralized and avoids divergence.src/cuga/backend/cuga_graph/nodes/cuga_lite/tool_approval_handler.py (1)
296-308:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing metadata cleanup on denial.
handle_denialchecksuser_approvedbut doesn't clean the approval metadata before returning the END command. This could leave stale approval fields in state if the graph is reused.🐛 Suggested fix
`@staticmethod` def handle_denial(adapter: CoreGraphAdapter, state: Any) -> Optional[Command]: """Handle user denial of tool approval.""" - if adapter.get_metadata(state).get("user_approved") is False: + md = adapter.get_metadata(state) + if md.get("user_approved") is False: logger.warning("User denied tool approval - skipping execution") + cleaned_metadata = ToolApprovalHandler.clean_approval_metadata(md) return Command( goto=END, update={ "execution_complete": True, "final_answer": "Execution cancelled by user.", "step_count": state.step_count + 1, + adapter.metadata_key: cleaned_metadata, }, ) return None🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/tool_approval_handler.py` around lines 296 - 308, handle_denial currently returns the END Command when user_approved is False but leaves approval metadata on the state; before returning, remove or clear approval-related metadata (e.g., "user_approved" and any other "approval_*" keys) using the graph adapter API (e.g., call adapter.update_metadata or adapter.clear_metadata for the current state) so the state is not left with stale fields, then return the same Command (goto=END, update with execution_complete, final_answer, and step_count increment) from handle_denial.
🧹 Nitpick comments (12)
src/cuga/backend/cuga_graph/policy/enactment.py (1)
253-268: 💤 Low valueConsider list unpacking for cleaner concatenation.
Static analysis suggests using
[*base_messages, blocked_message]instead ofbase_messages + [blocked_message]for slightly better performance and clarity.♻️ Proposed fix
return ( Command( goto=END, update={ - messages_key: base_messages + [blocked_message], + messages_key: [*base_messages, blocked_message], "final_answer": policy_match.action.content,🤖 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 `@src/cuga/backend/cuga_graph/policy/enactment.py` around lines 253 - 268, The concatenation of messages currently uses base_messages + [blocked_message]; change this to list unpacking to produce [*base_messages, blocked_message] where the Command is constructed (the return that sets update with messages_key and base_messages), i.e. modify the update value built from base_messages and blocked_message in the block that checks adapter (symbols: adapter, base_messages, messages_key, blocked_message, Command, goto=END) to use the unpacking form for cleaner, slightly faster list construction.src/cuga/backend/cuga_graph/nodes/cuga_supervisor/supervisor_graph_adapter.py (1)
430-437: 💤 Low valueSimplify the dict comprehension —
nameis alreadyinfo["name"].Both sequences in
zip()derive fromagent_list, so lengths always match. However, the comprehension is redundant: you iterateagent_listto extract names, then zip it back withagent_list. A simpler form avoids the zip entirely.♻️ Suggested simplification
- "available_agents": { - name: AgentInfo( - name=name, type=info["type"], description=info["description"] - ).model_dump() - for name, info in zip([a["name"] for a in agent_list], agent_list) - }, + "available_agents": { + info["name"]: AgentInfo( + name=info["name"], type=info["type"], description=info["description"] + ).model_dump() + for info in agent_list + },🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_supervisor/supervisor_graph_adapter.py` around lines 430 - 437, The dict comprehension building "available_agents" is redundant because it zips a list of names with agent_list; simplify by iterating agent_list directly: for each info in agent_list, use info["name"] as the key and create AgentInfo(name=info["name"], type=info["type"], description=info["description"]).model_dump() — update the comprehension in supervisor_graph_adapter.py (the "available_agents" expression that constructs AgentInfo and calls model_dump) to remove zip and the separate name list.src/cuga/backend/cuga_graph/nodes/cuga_supervisor/cuga_supervisor_graph.py (1)
62-91: 💤 Low valueConsider reusing
SupervisorGraphAdapterinstead of a separate_CugaSupervisorLoopAdapter.
_CugaSupervisorLoopAdapterduplicates the class attributes and hook implementations already present inSupervisorGraphAdapter. Since the wrapper functions only need the adapter interface (not the factory methods), you could instantiateSupervisorGraphAdapter(agents={})for the module-level singleton.♻️ Potential simplification
-class _CugaSupervisorLoopAdapter(CoreGraphAdapter): - """Supervisor seam: messages live on ``supervisor_chat_messages`` - (None-safe); step limit from ``state.cuga_lite_max_steps`` else - ``settings.advanced_features`` (default 50).""" - - messages_key = "supervisor_chat_messages" - # Approval seams (override the Lite defaults on CoreGraphAdapter). - metadata_key = "supervisor_metadata" - execute_node_name = "execute_agent_tool" - sender_name = "CugaSupervisor" - - def get_messages(self, state: CugaSupervisorState) -> List[BaseMessage]: - return state.supervisor_chat_messages or [] - - def resolve_max_steps(self, state: CugaSupervisorState, override: Optional[int]) -> int: - if override is not None: - return override - return ( - state.cuga_lite_max_steps - if state.cuga_lite_max_steps is not None - else getattr(settings.advanced_features, 'cuga_lite_max_steps', 50) - ) - - def get_variable_manager(self, state: CugaSupervisorState): - # Supervisor stores execution vars on its own manager, not the - # root state.variables_manager (the phase-9 coupling fix builds here). - return state.supervisor_variables_manager - - -_SUPERVISOR_LOOP_ADAPTER = _CugaSupervisorLoopAdapter() +# Module-level adapter for backward-compatible wrapper functions +_SUPERVISOR_LOOP_ADAPTER = SupervisorGraphAdapter(agents={})🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_supervisor/cuga_supervisor_graph.py` around lines 62 - 91, The file defines a near-duplicate adapter class _CugaSupervisorLoopAdapter and module singleton _SUPERVISOR_LOOP_ADAPTER; replace this duplication by instantiating the existing SupervisorGraphAdapter instead: remove or stop using _CugaSupervisorLoopAdapter and change the module-level singleton _SUPERVISOR_LOOP_ADAPTER to SupervisorGraphAdapter(agents={}) (ensuring any required attributes like messages_key/metadata_key/execute_node_name/sender_name and methods get_messages/resolve_max_steps/get_variable_manager are satisfied by SupervisorGraphAdapter or configured via its constructor), so callers that rely on _SUPERVISOR_LOOP_ADAPTER keep the same interface but reuse the shared SupervisorGraphAdapter implementation.src/cuga/backend/cuga_graph/nodes/cuga_supervisor/tests/test_supervisor_graph_adapter.py (1)
101-121: ⚡ Quick winConsider adding a test for
get_variable_managerwhen the attribute is missing.There's a test for
get_variables_storagereturningNonewhen missing (line 118-121), but no equivalent test forget_variable_manager. Given the issue identified in the adapter whereNonecan cause crashes, having this test would document the expected behavior.🧪 Suggested test
def test_get_variable_manager_returns_none_when_missing(): adapter = _make_adapter() state = SimpleNamespace() # No supervisor_variables_manager attribute assert adapter.get_variable_manager(state) is None🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_supervisor/tests/test_supervisor_graph_adapter.py` around lines 101 - 121, Add a unit test mirroring the existing variables storage missing-case: create a test named something like test_get_variable_manager_returns_none_when_missing that calls _make_adapter() to get adapter, constructs state = SimpleNamespace() with no supervisor_variables_manager attribute, and asserts adapter.get_variable_manager(state) is None; this verifies get_variable_manager returns None when the supervisor_variables_manager attribute is absent and prevents potential None-related crashes.src/cuga/backend/cuga_graph/nodes/cuga_lite/tests/test_agent_graph_adapter.py (1)
164-169: ⚡ Quick winUnused variable in test.
The
reasoningvariable is unpacked but never used. Prefix with underscore to indicate intentional discard.♻️ Suggested fix
def test_normalize_response_strips_empty_content(): adapter = _make_adapter() response = SimpleNamespace(content=" hello ", additional_kwargs={}) - content, reasoning = adapter.normalize_response(response) + content, _reasoning = adapter.normalize_response(response) # normalize_assistant_text strips whitespace assert content.strip() == "hello"🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/tests/test_agent_graph_adapter.py` around lines 164 - 169, The test test_normalize_response_strips_empty_content unpacks a second value from adapter.normalize_response into a variable named reasoning that is never used; change the unpack to use a throwaway name (e.g., _reasoning or _) when calling adapter.normalize_response in test_normalize_response_strips_empty_content so the unused variable is explicitly discarded and linter warnings are avoided.src/cuga/backend/cuga_graph/nodes/cuga_lite/agent_graph_adapter.py (3)
193-207: ⚖️ Poor tradeoffCircular import inside method body.
The import of
resolve_model_with_bind_toolsfromcuga_lite_graphinside this method creates a runtime circular dependency. This works but is fragile - ifcuga_lite_graphimports more from this file at module level, it could break. Consider movingresolve_model_with_bind_toolsto a shared utility module.🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/agent_graph_adapter.py` around lines 193 - 207, The runtime circular import is caused by importing resolve_model_with_bind_tools from cuga_lite_graph inside resolve_bind_tools; extract resolve_model_with_bind_tools into a new shared utility module (e.g., cuga.backend.cuga_graph.utils or cuga_lite_utils), move the implementation there, update cuga_lite_graph and any other modules to import from that shared module, and then change AgentGraphAdapter.resolve_bind_tools to import resolve_model_with_bind_tools at module top-level (or import it from the new shared module) instead of inside the method; ensure the method still calls resolve_model_with_bind_tools with the same parameters (active_model, configurable=configurable, tools_context_ref=self._tools_context_ref, tool_provider=self._base_tool_provider) and update any import statements accordingly.
186-190: 💤 Low valueMutable class attribute in inner class.
additional_kwargs: dict = {}uses a mutable default as a class attribute. While this specific usage is safe (the instance is created and returned immediately), it's a code smell flagged by Ruff (RUF012). Using a property or instance attribute is cleaner.♻️ Suggested fix
class _FakeResponse: content = f"```python\n{code}\n```" - additional_kwargs: dict = {} + + `@property` + def additional_kwargs(self) -> dict: + return {}🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/agent_graph_adapter.py` around lines 186 - 190, The inner class _FakeResponse defines a mutable class attribute additional_kwargs: dict = {}, which trips RUF012; change additional_kwargs to be an instance attribute or a read-only property on _FakeResponse (e.g., remove the class-level dict and implement def __init__(self): self.additional_kwargs = {} or add a `@property` def additional_kwargs(self) -> dict: return {}), keeping content as-is and returning the instance from the function.
223-233: 💤 Low valueTracker errors silently swallowed.
While swallowing tracker errors is intentional (per the docstring), using
logger.debugfor exceptions means production issues might be hard to diagnose. Consider usinglogger.warningwithexc_info=Falseor at least logging the exception type at warning level when it's unexpected.🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/agent_graph_adapter.py` around lines 223 - 233, The except block in on_response_processed silently logs tracker failures at debug level; change it to log at warning level and include the exception type/message (but not full traceback) so production issues are visible: update the except clause in AgentGraphAdapter.on_response_processed to call logger.warning (instead of logger.debug) and include the exception type/message string (or formatted exc.__class__.__name__ and str(exc)) while keeping exc_info=False to avoid stack traces.src/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_execution_plan_wiring.py (1)
123-123: ⚡ Quick winUnused variable in test.
The
resultvariable is unpacked but never used. Prefix with underscore to indicate intentional discard.♻️ Suggested fix
- result, new_vars = await CodeExecutor.eval_with_tools_async( + _result, new_vars = await CodeExecutor.eval_with_tools_async(🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_execution_plan_wiring.py` at line 123, The test unpacks two values from CodeExecutor.eval_with_tools_async but never uses the first (result); change the assignment to discard it by prefixing with an underscore (e.g., rename the local from result to _result) so the intent is clear and the unused-variable warning is avoided; update the line where CodeExecutor.eval_with_tools_async is awaited (the tuple assignment) accordingly.src/cuga/backend/cuga_graph/nodes/cuga_lite/tool_approval_handler.py (1)
122-128: ⚡ Quick winPEP 484 prohibits implicit
Optional.The
configparameter should use explicit optional typing.♻️ Suggested fix
`@staticmethod` async def check_and_create_approval_interrupt( adapter: CoreGraphAdapter, state: Any, code: str, content: str, - config: dict = None, + config: Optional[dict] = None, ) -> Optional[Command]:🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/tool_approval_handler.py` around lines 122 - 128, Update the function signature of check_and_create_approval_interrupt to use explicit optional typing for the config parameter (e.g., change config: dict = None to config: Optional[dict] = None) and ensure Optional is imported from typing at the top of the module; adjust any other local type hints if necessary to use explicit Optional rather than implicit None defaults so the signature and imports remain consistent with PEP 484.src/cuga/backend/cuga_graph/nodes/cuga_agent_core/runtime_tools.py (1)
56-59: ⚡ Quick winLog swallowed tool-arg extraction failures in
prompt_tool_dicts.This fallback is useful, but silently discarding schema errors makes runtime prompt/tool drift hard to debug. Keep fallback behavior and add a debug log with tool name + exception.
♻️ Proposed tweak
- except Exception: + except Exception as exc: + logger.debug( + f"Failed to derive args for runtime tool {getattr(t, 'name', '<unknown>')}: {exc}" + ) arg_names = []🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/runtime_tools.py` around lines 56 - 59, In prompt_tool_dicts update the except block that swallows failures when extracting arg_names from t (the tool object) to still set arg_names = [] but also emit a debug log containing the tool identifier (e.g., getattr(t, "name", repr(t))) and the caught exception; use the module/logger used elsewhere (or logging.getLogger(__name__)) and include exc_info=True so the stacktrace is available for debugging while preserving the existing fallback behavior.src/cuga/backend/cuga_graph/nodes/cuga_agent_core/variable_bridge.py (1)
33-33: ⚡ Quick winConsider strengthening the type hint for
target_manager.The
Anytype weakens type safety. A Protocol or proper type annotation would make the expected interface explicit and enable better IDE support and static analysis.♻️ Proposed refinement with Protocol
Add a Protocol definition at the top of the file:
from typing import Any, Dict, List +from typing import Protocol + + +class VariableManagerProtocol(Protocol): + """Protocol for objects that can store bridged variables.""" + def add_variable(self, value: Any, *, name: str, description: str) -> None: + ...Then update the type hint:
`@staticmethod` def bridge( source_values: Dict[str, Any], - target_manager: Any, + target_manager: VariableManagerProtocol, description_prefix: str = "bridged", ) -> List[str]:🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/variable_bridge.py` at line 33, Replace the broad Any for target_manager with a concrete Protocol describing the methods/attributes the code uses: create a TargetManagerProtocol (or interface) in variable_bridge.py that lists the exact callables/properties accessed (e.g., send_event, get_state, update_variable — name each method actually used by VariableBridge), then change the parameter/type annotation for target_manager to use TargetManagerProtocol and import typing.Protocol; update any related function signatures (e.g., VariableBridge.__init__ or functions that accept target_manager) to use the new Protocol so IDEs and static checkers can validate usage.
🤖 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 @.claude/settings.json:
- Line 6: The allowlist entry "Bash(python -m *)" is too broad and allows
arbitrary Python module execution; replace this wildcard with an explicit list
of permitted python -m invocations (for example only include specific modules
you actually need such as "Bash(python -m http.server)" or "Bash(python -m
pip)") or remove the python -m form entirely and instead add discrete approved
commands; update the settings entry that currently contains "Bash(python -m *)"
to enumerate only the exact module names your app requires.
In `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph_nodes.py`:
- Around line 255-258: The injected guidance currently replaces the original
HumanMessage with a new HumanMessage containing only content, dropping metadata;
instead, use the original message's model_copy to preserve id and
additional_kwargs — locate the block that creates result = list(messages), reads
original = result[last_human_idx], and replaces result[last_human_idx] and
change the replacement to call original.model_copy(update={'content':
f"{original.content}\n\n## Task Guidance\n{guidance}"}) so the original metadata
(id, additional_kwargs) is retained.
In `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.py`:
- Around line 183-205: The approval interrupt can be returned before the hard
step-limit check; to fix, compute final_messages and new_step_count early (use
modified_messages + [AIMessage(content=content)] and state.step_count + 1), call
enforce_step_limit(adapter, state=state, messages=final_messages,
new_step_count=new_step_count, limit=adapter.resolve_max_steps(state,
configurable.get("cuga_lite_max_steps"))) and immediately return if it yields a
limit_cmd, and only after that call
ToolApprovalHandler.check_and_create_approval_interrupt(adapter, state, code,
content, config) when settings.policy.enabled; this ensures enforce_step_limit
runs before ToolApprovalHandler.check_and_create_approval_interrupt.
- Around line 159-165: The Loguru logger calls in call_model use %-style
placeholders which Loguru doesn't support; update the two problematic calls to
use brace-style placeholders: change the logger.warning that references i and
msg_role to use "{}" placeholders and pass i and msg_role as separate arguments,
and change the logger.info block that logs len(messages_for_model) and
adapter.sender_name to use "{}" placeholders and pass those values as arguments;
locate these in the call_model function where logger.warning(...) and
logger.info(...) reference msg_role, i, messages_for_model, and
adapter.sender_name and replace the formatting accordingly.
- Around line 149-150: The current replacement
modified_messages.append(HumanMessage(content=content) if modified else msg)
drops metadata from the original msg; instead preserve fields by using the
existing model_copy() pattern (e.g., use msg.model_copy(update={"content":
content}) when modified) so id, additional_kwargs, etc. are retained; update the
modified_messages append to use msg.model_copy(update={"content": content"})
(refer to modified_messages, HumanMessage, msg, and model_copy() and mirror the
approach used in context_summarizer.py) and keep messages_for_model logic
unchanged.
In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/agent_graph_adapter.py`:
- Around line 78-104: The helper function _extract_code_from_response_tool_calls
is duplicated between agent_graph_adapter.py and cuga_lite_graph.py; extract a
single shared implementation into a common utility module (e.g.,
cuga_graph.utils or similar), keep or unify the type hints and optional debug
logging, reuse the existing helper _tool_call_kwarg_literal from the new module,
replace the two local implementations with imports of the shared
_extract_code_from_response_tool_calls, and update any callers to use the
imported function so behavior and logging stay consistent across both locations.
In
`@src/cuga/backend/cuga_graph/nodes/cuga_supervisor/supervisor_graph_adapter.py`:
- Around line 474-479: The code assumes adapter.get_variable_manager(state)
returns a manager but it may be None; update the block around
get_variable_manager/state so you first assign var_manager =
adapter.get_variable_manager(state) and only call
var_manager.get_variable_names() and var_manager.get_variable(...) when
var_manager is not None (wrap the existing_vars population in an if var_manager
is not None guard), still assign adapter._shared_vm_ref[0] = var_manager (so
shared ref can be None), and likewise guard later usages: allow
variable_manager=var_manager to be passed to CodeExecutor (CodeExecutor must
accept None) but wrap the var_manager.add_variable(...) loop in an if
var_manager is not None before iterating/adding new variables.
---
Outside diff comments:
In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.py`:
- Around line 143-177: This file duplicates the helper functions
_tool_call_kwarg_literal and _extract_code_from_response_tool_calls that already
exist in agent_graph_adapter.py; remove the duplicated definitions from
cuga_lite_graph.py and import them from
cuga.backend.cuga_graph.nodes.cuga_lite.agent_graph_adapter instead (or if they
are only used in agent_graph_adapter.py, delete them here entirely), ensuring
any calls in cuga_lite_graph.py reference the imported _tool_call_kwarg_literal
and _extract_code_from_response_tool_calls symbols.
- Around line 647-665: There are duplicate implementations of
format_task_todos_system_block and format_current_plan_section across
cuga_lite_graph.py and agent_graph_adapter.py; remove the redundant copy and
centralize the functions in one module, then update callers to import the single
source. Specifically, keep the canonical implementations (pick either
cuga_lite_graph.format_task_todos_system_block and format_current_plan_section
or the versions in agent_graph_adapter), delete the duplicate functions from the
other file, update any local imports/usages to from <chosen_module> import
format_task_todos_system_block, format_current_plan_section, and adjust the
docstring in agent_graph_adapter.py that states "moved from cuga_lite_graph.py"
if you moved them the other direction. Ensure no remaining references to the
removed functions and run tests to confirm names resolve.
- Around line 504-507: There is a duplicate function named
_clean_empty_response_retry_meta in cuga_lite_graph.py and
agent_graph_adapter.py; remove the copy from cuga_lite_graph.py and instead
import and use the single canonical _clean_empty_response_retry_meta defined in
agent_graph_adapter.py (or move the function to a shared utility module and
import it from both places), updating any references in CugaLiteGraph methods to
call the imported function name _clean_empty_response_retry_meta so the logic is
centralized and avoids divergence.
In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/tool_approval_handler.py`:
- Around line 296-308: handle_denial currently returns the END Command when
user_approved is False but leaves approval metadata on the state; before
returning, remove or clear approval-related metadata (e.g., "user_approved" and
any other "approval_*" keys) using the graph adapter API (e.g., call
adapter.update_metadata or adapter.clear_metadata for the current state) so the
state is not left with stale fields, then return the same Command (goto=END,
update with execution_complete, final_answer, and step_count increment) from
handle_denial.
In `@src/cuga/backend/cuga_graph/nodes/cuga_supervisor/cuga_supervisor_graph.py`:
- Around line 39-59: The function _resolve_names_from_caller_frame is
duplicated; remove the copy in cuga_supervisor_graph.py and import the canonical
implementation from supervisor_graph_adapter (where it was moved). Delete the
local definition of _resolve_names_from_caller_frame in this file and add an
import for _resolve_names_from_caller_frame from supervisor_graph_adapter at the
top of the module (or leave no import if nothing in this module references it);
ensure any callers in cuga_supervisor_graph.py now use the imported symbol.
---
Nitpick comments:
In `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/runtime_tools.py`:
- Around line 56-59: In prompt_tool_dicts update the except block that swallows
failures when extracting arg_names from t (the tool object) to still set
arg_names = [] but also emit a debug log containing the tool identifier (e.g.,
getattr(t, "name", repr(t))) and the caught exception; use the module/logger
used elsewhere (or logging.getLogger(__name__)) and include exc_info=True so the
stacktrace is available for debugging while preserving the existing fallback
behavior.
In `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/variable_bridge.py`:
- Line 33: Replace the broad Any for target_manager with a concrete Protocol
describing the methods/attributes the code uses: create a TargetManagerProtocol
(or interface) in variable_bridge.py that lists the exact callables/properties
accessed (e.g., send_event, get_state, update_variable — name each method
actually used by VariableBridge), then change the parameter/type annotation for
target_manager to use TargetManagerProtocol and import typing.Protocol; update
any related function signatures (e.g., VariableBridge.__init__ or functions that
accept target_manager) to use the new Protocol so IDEs and static checkers can
validate usage.
In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/agent_graph_adapter.py`:
- Around line 193-207: The runtime circular import is caused by importing
resolve_model_with_bind_tools from cuga_lite_graph inside resolve_bind_tools;
extract resolve_model_with_bind_tools into a new shared utility module (e.g.,
cuga.backend.cuga_graph.utils or cuga_lite_utils), move the implementation
there, update cuga_lite_graph and any other modules to import from that shared
module, and then change AgentGraphAdapter.resolve_bind_tools to import
resolve_model_with_bind_tools at module top-level (or import it from the new
shared module) instead of inside the method; ensure the method still calls
resolve_model_with_bind_tools with the same parameters (active_model,
configurable=configurable, tools_context_ref=self._tools_context_ref,
tool_provider=self._base_tool_provider) and update any import statements
accordingly.
- Around line 186-190: The inner class _FakeResponse defines a mutable class
attribute additional_kwargs: dict = {}, which trips RUF012; change
additional_kwargs to be an instance attribute or a read-only property on
_FakeResponse (e.g., remove the class-level dict and implement def
__init__(self): self.additional_kwargs = {} or add a `@property` def
additional_kwargs(self) -> dict: return {}), keeping content as-is and returning
the instance from the function.
- Around line 223-233: The except block in on_response_processed silently logs
tracker failures at debug level; change it to log at warning level and include
the exception type/message (but not full traceback) so production issues are
visible: update the except clause in AgentGraphAdapter.on_response_processed to
call logger.warning (instead of logger.debug) and include the exception
type/message string (or formatted exc.__class__.__name__ and str(exc)) while
keeping exc_info=False to avoid stack traces.
In
`@src/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_execution_plan_wiring.py`:
- Line 123: The test unpacks two values from CodeExecutor.eval_with_tools_async
but never uses the first (result); change the assignment to discard it by
prefixing with an underscore (e.g., rename the local from result to _result) so
the intent is clear and the unused-variable warning is avoided; update the line
where CodeExecutor.eval_with_tools_async is awaited (the tuple assignment)
accordingly.
In
`@src/cuga/backend/cuga_graph/nodes/cuga_lite/tests/test_agent_graph_adapter.py`:
- Around line 164-169: The test test_normalize_response_strips_empty_content
unpacks a second value from adapter.normalize_response into a variable named
reasoning that is never used; change the unpack to use a throwaway name (e.g.,
_reasoning or _) when calling adapter.normalize_response in
test_normalize_response_strips_empty_content so the unused variable is
explicitly discarded and linter warnings are avoided.
In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/tool_approval_handler.py`:
- Around line 122-128: Update the function signature of
check_and_create_approval_interrupt to use explicit optional typing for the
config parameter (e.g., change config: dict = None to config: Optional[dict] =
None) and ensure Optional is imported from typing at the top of the module;
adjust any other local type hints if necessary to use explicit Optional rather
than implicit None defaults so the signature and imports remain consistent with
PEP 484.
In `@src/cuga/backend/cuga_graph/nodes/cuga_supervisor/cuga_supervisor_graph.py`:
- Around line 62-91: The file defines a near-duplicate adapter class
_CugaSupervisorLoopAdapter and module singleton _SUPERVISOR_LOOP_ADAPTER;
replace this duplication by instantiating the existing SupervisorGraphAdapter
instead: remove or stop using _CugaSupervisorLoopAdapter and change the
module-level singleton _SUPERVISOR_LOOP_ADAPTER to
SupervisorGraphAdapter(agents={}) (ensuring any required attributes like
messages_key/metadata_key/execute_node_name/sender_name and methods
get_messages/resolve_max_steps/get_variable_manager are satisfied by
SupervisorGraphAdapter or configured via its constructor), so callers that rely
on _SUPERVISOR_LOOP_ADAPTER keep the same interface but reuse the shared
SupervisorGraphAdapter implementation.
In
`@src/cuga/backend/cuga_graph/nodes/cuga_supervisor/supervisor_graph_adapter.py`:
- Around line 430-437: The dict comprehension building "available_agents" is
redundant because it zips a list of names with agent_list; simplify by iterating
agent_list directly: for each info in agent_list, use info["name"] as the key
and create AgentInfo(name=info["name"], type=info["type"],
description=info["description"]).model_dump() — update the comprehension in
supervisor_graph_adapter.py (the "available_agents" expression that constructs
AgentInfo and calls model_dump) to remove zip and the separate name list.
In
`@src/cuga/backend/cuga_graph/nodes/cuga_supervisor/tests/test_supervisor_graph_adapter.py`:
- Around line 101-121: Add a unit test mirroring the existing variables storage
missing-case: create a test named something like
test_get_variable_manager_returns_none_when_missing that calls _make_adapter()
to get adapter, constructs state = SimpleNamespace() with no
supervisor_variables_manager attribute, and asserts
adapter.get_variable_manager(state) is None; this verifies get_variable_manager
returns None when the supervisor_variables_manager attribute is absent and
prevents potential None-related crashes.
In `@src/cuga/backend/cuga_graph/policy/enactment.py`:
- Around line 253-268: The concatenation of messages currently uses
base_messages + [blocked_message]; change this to list unpacking to produce
[*base_messages, blocked_message] where the Command is constructed (the return
that sets update with messages_key and base_messages), i.e. modify the update
value built from base_messages and blocked_message in the block that checks
adapter (symbols: adapter, base_messages, messages_key, blocked_message,
Command, goto=END) to use the unpacking form for cleaner, slightly faster list
construction.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: fdb341f9-86db-4f0a-a00c-064b4918efcb
📒 Files selected for processing (42)
.claude/settings.jsonsrc/cuga/backend/cuga_graph/nodes/api/code_agent/code_act_agent.pysrc/cuga/backend/cuga_graph/nodes/api/code_agent/tests/test_code_act_agent.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/code_extraction.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/execution_policy.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph_nodes.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/runtime_tools.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_graph.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/test_code_extraction.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/test_execution_policy.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/test_graph_adapter_hooks.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/test_graph_nodes.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/test_policy_enactment_adapter.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/test_runtime_tools.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/test_shared_call_model.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/test_shared_graph_builder.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/test_supervisor_feature_parity.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/test_supervisor_tool_provider.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/test_tool_approval_adapter.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/test_variable_bridge.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/variable_bridge.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/agent_graph_adapter.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/code_executor.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_execution_plan_wiring.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_extract_codeblocks.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_sync_async_tools.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/tests/memory/test_cuga_lite_graph_evolve_integration.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/tests/test_agent_graph_adapter.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/tests/test_cuga_lite_graph_evolve_guidelines.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/tool_approval_handler.pysrc/cuga/backend/cuga_graph/nodes/cuga_supervisor/cuga_supervisor_graph.pysrc/cuga/backend/cuga_graph/nodes/cuga_supervisor/cuga_supervisor_node.pysrc/cuga/backend/cuga_graph/nodes/cuga_supervisor/supervisor_graph_adapter.pysrc/cuga/backend/cuga_graph/nodes/cuga_supervisor/tests/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_supervisor/tests/test_supervisor_graph_adapter.pysrc/cuga/backend/cuga_graph/policy/enactment.pysrc/cuga/config.pysrc/cuga/sdk.py
💤 Files with no reviewable changes (2)
- src/cuga/backend/cuga_graph/nodes/api/code_agent/code_act_agent.py
- src/cuga/backend/cuga_graph/nodes/api/code_agent/tests/test_code_act_agent.py
Comment on lines
+149
to
+150
| modified_messages.append(HumanMessage(content=content) if modified else msg) | ||
| messages_for_model.append({"role": "user", "content": content}) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.py | head -180 | tail -50Repository: cuga-project/cuga-agent
Length of output: 2704
🏁 Script executed:
head -50 src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.pyRepository: cuga-project/cuga-agent
Length of output: 1951
🏁 Script executed:
rg "from langchain" src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.py -A 2Repository: cuga-project/cuga-agent
Length of output: 195
🏁 Script executed:
rg "modified_messages" src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.py -B 2 -A 5Repository: cuga-project/cuga-agent
Length of output: 1580
🏁 Script executed:
# Check if HumanMessage or LangChain messages use model_copy
rg "model_copy" . -t py --max-count 20Repository: cuga-project/cuga-agent
Length of output: 339
🏁 Script executed:
# Look for imports and usage of HumanMessage to understand its structure
rg "HumanMessage\(" . -t py -A 3 --max-count 15Repository: cuga-project/cuga-agent
Length of output: 50381
🏁 Script executed:
# Check if HumanMessage is created with additional_kwargs or other fields elsewhere
rg "HumanMessage\(" . -t py -A 2 | grep -E "(additional_kwargs|id=|metadata)" -B 2 -A 1 | head -50Repository: cuga-project/cuga-agent
Length of output: 1890
🏁 Script executed:
# Check message_utils.py to see how HumanMessage is constructed with metadata
cat -n src/cuga/backend/cuga_graph/utils/message_utils.pyRepository: cuga-project/cuga-agent
Length of output: 3832
🏁 Script executed:
# Look for what fields msg might have when it's modified
grep -n "additional_kwargs\|msg.id\|msg\.metadata" src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.pyRepository: cuga-project/cuga-agent
Length of output: 49
🏁 Script executed:
# Check what types of messages are being processed - look at effective_messages source
grep -n "effective_messages" src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.py -B 5 -A 3Repository: cuga-project/cuga-agent
Length of output: 1534
🏁 Script executed:
# Look for where additional_kwargs might be set or used in the broader context
rg "additional_kwargs" src/cuga/backend/cuga_graph/nodes/cuga_agent_core/ -A 2 -B 2Repository: cuga-project/cuga-agent
Length of output: 3026
🏁 Script executed:
# Check if LangChain's HumanMessage supports model_copy by looking at actual usage patterns
rg "\.model_copy\(" . -t py -B 3 -A 3 | head -100Repository: cuga-project/cuga-agent
Length of output: 941
🏁 Script executed:
# Check if the code already handles cases where model_copy might not be available
# Look at context_summarizer to see the pattern
cat -n src/cuga/backend/cuga_graph/utils/context_summarizer.py | grep -A 5 -B 5 "model_copy"Repository: cuga-project/cuga-agent
Length of output: 829
🏁 Script executed:
# Verify that HumanMessage from langchain_core.messages is a Pydantic model with model_copy
# by checking imports and any isinstance checks
rg "from langchain" src/cuga/backend/cuga_graph/utils/context_summarizer.pyRepository: cuga-project/cuga-agent
Length of output: 263
🏁 Script executed:
# Check if there are any tests that verify message metadata preservation
rg "additional_kwargs.*HumanMessage\|HumanMessage.*additional_kwargs" . -t py -A 3 -B 1 | head -60Repository: cuga-project/cuga-agent
Length of output: 49
🏁 Script executed:
# Check if msg could be a dict rather than a message object
# Look at how is_human is determined and what types are possible
grep -n "is_human\|isinstance(msg" src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.py -B 2 -A 2Repository: cuga-project/cuga-agent
Length of output: 528
🏁 Script executed:
# Check what apply_context_summarization returns
rg "async def apply_context_summarization" . -t py -A 15Repository: cuga-project/cuga-agent
Length of output: 1755
🏁 Script executed:
# Look for any tests showing what effective_messages could contain
rg "test.*call_model\|def.*create_call_model" . -t py --max-count 5Repository: cuga-project/cuga-agent
Length of output: 49
🏁 Script executed:
# Look at what final_messages is used for - does it matter if fields are lost?
grep -n "final_messages" src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.py -A 5Repository: cuga-project/cuga-agent
Length of output: 1490
🏁 Script executed:
# Check test files to see if metadata is being tested
rg "additional_kwargs.*modified\|modified.*additional_kwargs" . -t py -B 3 -A 3Repository: cuga-project/cuga-agent
Length of output: 49
🏁 Script executed:
# Look at the actual return statement and what happens to final_messages
sed -n '180,210p' src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.pyRepository: cuga-project/cuga-agent
Length of output: 1441
Avoid dropping message metadata when rewriting human messages.
Line 149 replaces the entire message object and loses metadata (additional_kwargs, id, etc.). Use model_copy() to preserve these fields, following the pattern already established in context_summarizer.py.
Proposed fix
- modified_messages.append(HumanMessage(content=content) if modified else msg)
+ modified_messages.append(
+ msg.model_copy(update={"content": content}) if modified else msg
+ )📝 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.
Suggested change
| modified_messages.append(HumanMessage(content=content) if modified else msg) | |
| messages_for_model.append({"role": "user", "content": content}) | |
| modified_messages.append( | |
| msg.model_copy(update={"content": content}) if modified else msg | |
| ) | |
| messages_for_model.append({"role": "user", "content": content}) |
🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.py` around
lines 149 - 150, The current replacement
modified_messages.append(HumanMessage(content=content) if modified else msg)
drops metadata from the original msg; instead preserve fields by using the
existing model_copy() pattern (e.g., use msg.model_copy(update={"content":
content}) when modified) so id, additional_kwargs, etc. are retained; update the
modified_messages append to use msg.model_copy(update={"content": content"})
(refer to modified_messages, HumanMessage, msg, and model_copy() and mirror the
approach used in context_summarizer.py) and keep messages_for_model logic
unchanged.
Comment on lines
+159
to
+165
| logger.warning("call_model: skipping message %d with unknown role: %s", i, msg_role) | ||
|
|
||
| logger.info( | ||
| "call_model: %d messages → model (%s)", | ||
| len(messages_for_model), | ||
| adapter.sender_name, | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the correct message formatting syntax for Loguru logger methods (e.g., logger.info/logger.warning)?
💡 Result:
In Loguru, message formatting is handled through two distinct mechanisms: the message content itself and the sink output format. 1. Message Content Formatting Loguru logger methods (e.g., logger.info, logger.warning, logger.error) utilize Python’s brace-style formatting (str.format), which is more powerful and readable than the traditional %-style formatting [1][2][3]. You pass the template string as the first argument, followed by the values to be inserted [1][3]. Example: logger.info("If you're using Python {}, prefer {feature} of course!", 3.6, feature="f-strings") 2. Output Formatting (Sink Format) To customize how logs appear in your terminal, files, or other sinks, you use the format argument within the logger.add function [1][2][4]. This defines the template for the entire log line, including metadata like time, log level, and the message itself [2][4]. The format string uses curly braces to access attributes from the log record dictionary [2][4]. You can also apply standard Python str.format directives for padding and alignment [2]. Example: logger.add(sys.stderr, format="{time:YYYY-MM-DD at HH:mm:ss} | {level: <8} | {message}") Key Formatting Features: - Markup Tags: You can colorize or style specific parts of your log output using markup tags like , , , or (which automatically applies the color associated with that specific log level) [1][5][3]. Example: logger.add(sys.stderr, format="{time} {message}") - Datetime Formatting: The {time} field supports custom datetime formatting (e.g., {time:YYYY-MM-DD HH:mm:ss}) [2][4]. You can append!UTC to display the time in UTC [2][4]. - Extra Context: You can use the logger.bind method to add custom fields to the record, which can then be referenced in your format string using {extra[key_name]} [1][6]. If you wish to change the default format, you must first remove the default handler, as Loguru adds one automatically upon import [6]. logger.remove logger.add(sys.stderr, format="{time} | {level} | {message}")
Citations:
- 1: https://github.com/Delgan/loguru/blob/master/README.md
- 2: https://loguru.readthedocs.io/en/stable/api/logger.html
- 3: https://loguru.readthedocs.io/en/stable/overview.html
- 4: https://betterstack.com/community/guides/logging/loguru/
- 5: https://deepwiki.com/Delgan/loguru/3.1-log-levels-and-message-formatting
- 6: Can I customize log format on top of loguru format Delgan/loguru#109
🏁 Script executed:
# Read the specific file and lines to confirm the code
cat -n src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.py | sed -n '155,170p'Repository: cuga-project/cuga-agent
Length of output: 845
🌐 Web query:
Does Loguru logger support %-style string formatting or does it require brace-style formatting?
💡 Result:
Loguru does not support %-style (printf-style) string formatting [1][2][3]. It exclusively uses brace-style formatting (similar to Python's str.format method) [4][5][2]. Because Loguru is designed to modernize Python logging, it requires the use of {} placeholders in log messages [5][3]. If you are migrating code that heavily relies on %-style formatting, you will need to update your log strings to use {} instead [6]. Example of the required syntax: logger.info("Hello {}", "world") Loguru does not intend to add support for %-style formatting because it would create ambiguity regarding whether a % symbol in a message is intended for formatting or should be treated as literal text [3]. For projects that must maintain compatibility with external libraries that use the standard logging module's %-style formatting, it is recommended to use an InterceptHandler to route those logs into Loguru [6][2].
Citations:
- 1: How to log message with args like logger.warning('hello %s', 'world') Delgan/loguru#661
- 2: Handle %s formatted strings? Delgan/loguru#991
- 3: Feature request: Format log messages when using % and a comma Delgan/loguru#1367
- 4: https://loguru.readthedocs.io/en/stable/api/logger.html
- 5: https://loguru.readthedocs.io/en/stable/overview.html
- 6: Support printf-style formatting? Delgan/loguru#1151
Fix Loguru formatting: %-style placeholders are not supported, use {} brace-style instead.
Lines 159 and 162–164 use %-style formatting (%d, %s), which Loguru does not support. Loguru exclusively uses brace-style formatting with {} placeholders. The current code will not process the arguments correctly. Update to:
Suggested fix
# Line 159
logger.warning("call_model: skipping message {} with unknown role: {}", i, msg_role)
# Lines 161–165
logger.info(
"call_model: {} messages → model ({})",
len(messages_for_model),
adapter.sender_name,
)🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.py` around
lines 159 - 165, The Loguru logger calls in call_model use %-style placeholders
which Loguru doesn't support; update the two problematic calls to use
brace-style placeholders: change the logger.warning that references i and
msg_role to use "{}" placeholders and pass i and msg_role as separate arguments,
and change the logger.info block that logs len(messages_for_model) and
adapter.sender_name to use "{}" placeholders and pass those values as arguments;
locate these in the call_model function where logger.warning(...) and
logger.info(...) reference msg_role, i, messages_for_model, and
adapter.sender_name and replace the formatting accordingly.
Comment on lines
+183
to
+205
| if code and settings.policy.enabled: | ||
| approval_command = await ToolApprovalHandler.check_and_create_approval_interrupt( | ||
| adapter, state, code, content, config | ||
| ) | ||
| if approval_command: | ||
| return approval_command | ||
|
|
||
| # ── Build final message list + step count ────────────────────────── | ||
| final_messages: list = modified_messages + [AIMessage(content=content)] | ||
| new_step_count: int = state.step_count + 1 | ||
|
|
||
| # ── Step limit enforcement ───────────────────────────────────────── | ||
| max_steps = adapter.resolve_max_steps(state, configurable.get("cuga_lite_max_steps")) | ||
| limit_cmd = enforce_step_limit( | ||
| adapter, | ||
| state=state, | ||
| messages=final_messages, | ||
| new_step_count=new_step_count, | ||
| limit=max_steps, | ||
| ) | ||
| if limit_cmd is not None: | ||
| return limit_cmd | ||
|
|
Contributor
There was a problem hiding this comment.
Enforce step limit before creating tool-approval interrupts.
Right now, Lines 183–189 can return an approval interrupt before the limit check at Lines 195–204. That lets over-limit turns bypass the intended hard stop.
Proposed fix
- # ── Tool-approval interrupt for generated code ─────────────────────
- if code and settings.policy.enabled:
- approval_command = await ToolApprovalHandler.check_and_create_approval_interrupt(
- adapter, state, code, content, config
- )
- if approval_command:
- return approval_command
-
# ── Build final message list + step count ──────────────────────────
final_messages: list = modified_messages + [AIMessage(content=content)]
new_step_count: int = state.step_count + 1
# ── Step limit enforcement ─────────────────────────────────────────
max_steps = adapter.resolve_max_steps(state, configurable.get("cuga_lite_max_steps"))
limit_cmd = enforce_step_limit(
adapter,
state=state,
messages=final_messages,
new_step_count=new_step_count,
limit=max_steps,
)
if limit_cmd is not None:
return limit_cmd
+
+ # ── Tool-approval interrupt for generated code ─────────────────────
+ if code and settings.policy.enabled:
+ approval_command = await ToolApprovalHandler.check_and_create_approval_interrupt(
+ adapter, state, code, content, config
+ )
+ if approval_command:
+ return approval_command📝 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.
Suggested change
| if code and settings.policy.enabled: | |
| approval_command = await ToolApprovalHandler.check_and_create_approval_interrupt( | |
| adapter, state, code, content, config | |
| ) | |
| if approval_command: | |
| return approval_command | |
| # ── Build final message list + step count ────────────────────────── | |
| final_messages: list = modified_messages + [AIMessage(content=content)] | |
| new_step_count: int = state.step_count + 1 | |
| # ── Step limit enforcement ───────────────────────────────────────── | |
| max_steps = adapter.resolve_max_steps(state, configurable.get("cuga_lite_max_steps")) | |
| limit_cmd = enforce_step_limit( | |
| adapter, | |
| state=state, | |
| messages=final_messages, | |
| new_step_count=new_step_count, | |
| limit=max_steps, | |
| ) | |
| if limit_cmd is not None: | |
| return limit_cmd | |
| # ── Build final message list + step count ────────────────────────── | |
| final_messages: list = modified_messages + [AIMessage(content=content)] | |
| new_step_count: int = state.step_count + 1 | |
| # ── Step limit enforcement ───────────────────────────────────────── | |
| max_steps = adapter.resolve_max_steps(state, configurable.get("cuga_lite_max_steps")) | |
| limit_cmd = enforce_step_limit( | |
| adapter, | |
| state=state, | |
| messages=final_messages, | |
| new_step_count=new_step_count, | |
| limit=max_steps, | |
| ) | |
| if limit_cmd is not None: | |
| return limit_cmd | |
| # ── Tool-approval interrupt for generated code ───────────────────── | |
| if code and settings.policy.enabled: | |
| approval_command = await ToolApprovalHandler.check_and_create_approval_interrupt( | |
| adapter, state, code, content, config | |
| ) | |
| if approval_command: | |
| return approval_command |
🧰 Tools
🪛 Ruff (0.15.13)
[warning] 191-191: Consider [*modified_messages, AIMessage(content=content)] instead of concatenation
Replace with [*modified_messages, AIMessage(content=content)]
(RUF005)
🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/shared_nodes.py` around
lines 183 - 205, The approval interrupt can be returned before the hard
step-limit check; to fix, compute final_messages and new_step_count early (use
modified_messages + [AIMessage(content=content)] and state.step_count + 1), call
enforce_step_limit(adapter, state=state, messages=final_messages,
new_step_count=new_step_count, limit=adapter.resolve_max_steps(state,
configurable.get("cuga_lite_max_steps"))) and immediately return if it yields a
limit_cmd, and only after that call
ToolApprovalHandler.check_and_create_approval_interrupt(adapter, state, code,
content, config) when settings.policy.enabled; this ensures enforce_step_limit
runs before ToolApprovalHandler.check_and_create_approval_interrupt.
- Remove settings.json from git tracking (git rm --cached) - Add .claude/settings.json to .gitignore alongside existing settings.local.json entry
…entGraphAdapter cuga_lite_graph.py goes from ~1800 lines to 214: state class, loop adapter, two thin wrappers, and the wiring factory. All node logic (prepare, sandbox, bind-tools helpers, find-tools helpers, todos, knowledge, reflection) now lives in AgentGraphAdapter, matching the pattern already used by cuga_supervisor_graph.py / SupervisorGraphAdapter. Also fixes 7 CodeRabbit review items from PR #250: - graph_nodes.py: use model_copy(update=...) on HumanMessage to preserve id/additional_kwargs - shared_nodes.py: same model_copy fix; Loguru %d/%s placeholders -> {} - tool_approval_handler.py: handle_denial clears user_approved from metadata in state update - cuga_supervisor_graph.py: remove dead _resolve_names_from_caller_frame + inspect import - supervisor_graph_adapter.py: guard var_manager None; fix create_update_todos_tool import source
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph/shared_nodes.py (1)
39-39: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftRemove
cuga_agent_core’s dependency oncuga_liteinternals.
shared_nodes.pycurrently importsToolApprovalHandlerfromcuga_lite, which breaks the shared-core layering boundary. Move approval behavior behindCoreGraphAdapterhooks or relocate approval handling intocuga_agent_core.Based on learnings: Ensure downstream layers (like
cuga_supervisor) never depend on the internal helper files of upstream layers (likecuga_lite). Shared schemas must live incuga_agent_core.🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph/shared_nodes.py` at line 39, shared_nodes.py currently imports ToolApprovalHandler from cuga_lite, breaking layer boundaries; remove that import and the direct dependency and instead expose approval behavior through CoreGraphAdapter hooks (e.g., add methods like request_tool_approval / handle_tool_approval on CoreGraphAdapter) or relocate the approval logic into cuga_agent_core (create a ToolApprovalHandler equivalent class inside cuga_agent_core). Update any call sites in shared_nodes.py to call the new CoreGraphAdapter hook methods (or the new local ToolApprovalHandler) rather than importing from cuga_lite so downstream layers no longer depend on cuga_lite internals.src/cuga/backend/cuga_graph/nodes/cuga_lite/agent_graph_adapter.py (1)
160-1104: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftSplit this module into cohesive submodules before merge.
This file now exceeds 1,000 LOC and combines multiple responsibilities (adapter hooks, policy/tool preparation, runtime orchestration, knowledge injection, sandbox execution) in one unit. Please split into focused components and keep this file as thin wiring.
As per coding guidelines
src/cuga/backend/cuga_graph/nodes/**/*.py: No single file in this directory should exceed 1,000 lines of code. Large monolithic files are a severe anti-pattern and must be refactored into sub-packages with cohesive sub-components.🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/agent_graph_adapter.py` around lines 160 - 1104, The AgentGraphAdapter class has grown >1,000 LOC and must be split into cohesive submodules; extract heavy responsibilities from AgentGraphAdapter (file-scope class) into focused components: move tool/prompt preparation logic in build_prepare_node into a new prepare_tools module/class (e.g., PrepareToolsService with methods used in build_prepare_node), move sandbox execution logic in build_sandbox_node into a separate sandbox executor (e.g., SandboxExecutor.eval_with_tools_async wrapper), move knowledge-awareness and knowledge injection blocks into a knowledge_awareness module (functions used around _knowledge_scope_instruction, get_knowledge_summary, format_knowledge_context), and move policy-related logic/PolicyEnactment interactions into a policy_handler module. Keep AgentGraphAdapter as thin wiring that delegates to these new components (retain method names like build_prepare_node, build_sandbox_node, ainvoke_model, resolve_bind_tools but replace in-body large blocks with calls to the new services), and update imports/constructor (inject new dependencies or service factories) so tests and callers still use AgentGraphAdapter without behavioral changes.
♻️ Duplicate comments (1)
src/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph/shared_nodes.py (1)
183-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEnforce the hard step limit before tool-approval interrupt.
The node can return an approval interrupt before limit enforcement, allowing over-limit turns to avoid the hard stop.
🔧 Proposed minimal reorder
- # ── Tool-approval interrupt for generated code ───────────────────── - if code and settings.policy.enabled: - approval_command = await ToolApprovalHandler.check_and_create_approval_interrupt( - adapter, state, code, content, config - ) - if approval_command: - return approval_command - # ── Build final message list + step count ────────────────────────── final_messages: list = modified_messages + [AIMessage(content=content)] new_step_count: int = state.step_count + 1 # ── Step limit enforcement ───────────────────────────────────────── max_steps = adapter.resolve_max_steps(state, configurable.get("cuga_lite_max_steps")) limit_cmd = enforce_step_limit( adapter, state=state, messages=final_messages, new_step_count=new_step_count, limit=max_steps, ) if limit_cmd is not None: return limit_cmd + + # ── Tool-approval interrupt for generated code ───────────────────── + if code and settings.policy.enabled: + approval_command = await ToolApprovalHandler.check_and_create_approval_interrupt( + adapter, state, code, content, config + ) + if approval_command: + return approval_command🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph/shared_nodes.py` around lines 183 - 205, Build the final_messages (modified_messages + AIMessage(content=content)) and new_step_count first, then call enforce_step_limit(...) with those values and return immediately if it yields a limit_cmd; only after passing the step-limit check should you call ToolApprovalHandler.check_and_create_approval_interrupt(adapter, state, code, content, config) and return approval_command if present. In other words, move the enforce_step_limit check to occur before ToolApprovalHandler.check_and_create_approval_interrupt, preserving use of adapter.resolve_max_steps and the same arguments (adapter, state, messages=final_messages, new_step_count, limit=max_steps).
🤖 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 `@src/cuga/backend/cuga_graph/nodes/AGENTS.md`:
- Line 13: The markdown file has unlabeled fenced code blocks causing MD040;
update the two code fences in AGENTS.md (the directory-tree block and the ASCII
flow diagram block) by adding a language identifier (e.g., "text") after the
opening triple backticks so the blocks read ```text instead of ``` to satisfy
markdownlint; locate the two unlabeled fences around the directory listing and
the "START ──> prepare ..." diagram and change them both to use the same
language label.
In `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/execution/todos.py`:
- Around line 119-121: The fallback except Exception: in the todo parsing block
should not swallow all errors; narrow the catch to the specific parse/validation
exceptions raised when constructing Todo or normalizing input (e.g., TypeError,
ValueError and the validation exception used by your model library such as
pydantic.ValidationError) and import that ValidationError if needed; update the
except clause around the Todo(**input_data) fallback in todos.py to catch only
those specific exceptions so real bugs still surface while still allowing the
"wrap in a list" fallback to run for expected parsing/validation failures.
In `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/tools/runtime_tools.py`:
- Line 23: Replace the cross-layer import of AppDefinition from
cuga_lite.providers.base with a shared schema in cuga_agent_core: move the
AppDefinition class (and any related provider interfaces used by
runtime_tools.py) into a new or existing core/shared schema module within
cuga_agent_core, update the import in runtime_tools.py to import AppDefinition
from that new core module, and ensure any other modules that currently import
AppDefinition from cuga_lite.providers.base are updated to import from the
cuga_agent_core shared schema instead so upstream internals are not referenced
by downstream layers.
In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/agent_graph_adapter.py`:
- Around line 765-766: The two silent except blocks (the "except Exception:
pass" occurrences in the AgentGraphAdapter knowledge-fallback paths) are
swallowing errors; replace them with explicit logging and controlled handling:
capture the exception using the module/class logger (e.g., self.logger or the
existing logger used by AgentGraphAdapter), call logger.exception(...) or
logger.error(..., exc_info=True) with a clear message mentioning the fallback
step, and then either continue the fallback logic or re-raise if the error is
fatal; ensure both occurrences that match the literal "except Exception: pass"
in the AgentGraphAdapter knowledge-fallback code are updated so failures are
visible in logs and include stack traces.
In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/bind_tools.py`:
- Around line 24-25: Replace the blind `except Exception: pass` in
_bind_tools_mode_from_settings, _bind_tools_apps_from_settings, and
_bind_tools_tool_names_from_settings with targeted exception handlers (e.g.,
KeyError, TypeError, ValueError as appropriate) and emit a logger.debug or
logger.warning with contextual info and the caught exception; similarly locate
and replace other blind catches in this module (the remaining try/except blocks
that catch Exception) with specific exceptions and proper logging. Also update
all Loguru warning calls that currently use "%s" placeholders to use Loguru's
"{}" formatting (i.e., change logger.warning("...%s...", var) ->
logger.warning("...{}...", var)) so all warning messages at the noted call sites
follow the correct formatting. Ensure you import or use the existing logger
(Loguru) and do not swallow exceptions — re-raise or handle them appropriately
after logging when necessary.
- Around line 96-112: The Loguru logger calls in bind_tools use printf-style
"%s" which doesn't interpolate; update the calls (e.g., inside get_all_tools
where logger.warning("bind_tools: get_all_tools failed: %s", e) and the
duplicate-name logger.debug call) to use Loguru "{}" placeholders and pass the
values as arguments (e.g., logger.warning("bind_tools: get_all_tools failed:
{}", e) and logger.debug("bind_tools: duplicate tool names from provider (using
first): {}", sorted(duplicates))). Apply the same change for the other logger
invocations noted in this file (the other occurrences around the
duplicates/by_name logic and at the referenced ranges).
In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/find_tools.py`:
- Around line 83-90: The current branch sets filtered_tools = [] when
app_to_tools_map is not provided, causing find_tools to miss matches even though
all_tools exists; update the else branch so that when app_to_tools_map is falsy
you derive filtered_tools from all_tools by selecting tools that belong to
app_name (e.g., filter all_tools by a tool property like tool.app or membership
in tool.apps or another identifier used in your tool objects), keeping the
existing behavior when app_to_tools_map is present (use
app_to_tools_map[app_name] if found) and only falling back to an empty list if
neither map nor any matching tool in all_tools exist. Reference: function
find_tools, variables app_to_tools_map, app_name, all_tools, and filtered_tools.
In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/providers/langchain.py`:
- Around line 63-72: The condition that backfills StructuredTool.func is
inverted: currently the code only runs when both coroutine and _run are missing,
making the assignment branches unreachable; change the inner guard to check for
the presence of either implementation (use if hasattr(tool, "coroutine") or
hasattr(tool, "_run")) so the block runs when we can copy an existing callable
into tool.func, and keep the assignment order that prefers tool.coroutine then
tool._run and the logger.warning message in the same block.
---
Outside diff comments:
In `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph/shared_nodes.py`:
- Line 39: shared_nodes.py currently imports ToolApprovalHandler from cuga_lite,
breaking layer boundaries; remove that import and the direct dependency and
instead expose approval behavior through CoreGraphAdapter hooks (e.g., add
methods like request_tool_approval / handle_tool_approval on CoreGraphAdapter)
or relocate the approval logic into cuga_agent_core (create a
ToolApprovalHandler equivalent class inside cuga_agent_core). Update any call
sites in shared_nodes.py to call the new CoreGraphAdapter hook methods (or the
new local ToolApprovalHandler) rather than importing from cuga_lite so
downstream layers no longer depend on cuga_lite internals.
In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/agent_graph_adapter.py`:
- Around line 160-1104: The AgentGraphAdapter class has grown >1,000 LOC and
must be split into cohesive submodules; extract heavy responsibilities from
AgentGraphAdapter (file-scope class) into focused components: move tool/prompt
preparation logic in build_prepare_node into a new prepare_tools module/class
(e.g., PrepareToolsService with methods used in build_prepare_node), move
sandbox execution logic in build_sandbox_node into a separate sandbox executor
(e.g., SandboxExecutor.eval_with_tools_async wrapper), move knowledge-awareness
and knowledge injection blocks into a knowledge_awareness module (functions used
around _knowledge_scope_instruction, get_knowledge_summary,
format_knowledge_context), and move policy-related logic/PolicyEnactment
interactions into a policy_handler module. Keep AgentGraphAdapter as thin wiring
that delegates to these new components (retain method names like
build_prepare_node, build_sandbox_node, ainvoke_model, resolve_bind_tools but
replace in-body large blocks with calls to the new services), and update
imports/constructor (inject new dependencies or service factories) so tests and
callers still use AgentGraphAdapter without behavioral changes.
---
Duplicate comments:
In `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph/shared_nodes.py`:
- Around line 183-205: Build the final_messages (modified_messages +
AIMessage(content=content)) and new_step_count first, then call
enforce_step_limit(...) with those values and return immediately if it yields a
limit_cmd; only after passing the step-limit check should you call
ToolApprovalHandler.check_and_create_approval_interrupt(adapter, state, code,
content, config) and return approval_command if present. In other words, move
the enforce_step_limit check to occur before
ToolApprovalHandler.check_and_create_approval_interrupt, preserving use of
adapter.resolve_max_steps and the same arguments (adapter, state,
messages=final_messages, new_step_count, limit=max_steps).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7acc4a71-55a3-4b2e-9791-b500af352aa5
📒 Files selected for processing (77)
src/cuga/__init__.pysrc/cuga/backend/cuga_graph/graph.pysrc/cuga/backend/cuga_graph/nodes/AGENTS.mdsrc/cuga/backend/cuga_graph/nodes/chat/chat_agent/chat_agent.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/execution/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/execution/code_extraction.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/execution/todos.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/execution/variable_bridge.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph/graph_nodes.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph/shared_graph.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph/shared_nodes.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/policy/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/policy/execution_policy.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/execution/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/execution/test_code_extraction.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/execution/test_variable_bridge.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/graph/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/graph/test_graph_adapter_hooks.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/graph/test_graph_nodes.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/graph/test_shared_call_model.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/graph/test_shared_graph_builder.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/graph/test_supervisor_feature_parity.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/policy/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/policy/test_execution_policy.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/policy/test_policy_enactment_adapter.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/policy/test_tool_approval_adapter.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/tools/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/tools/test_runtime_tools.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/tools/test_supervisor_tool_provider.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tools/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tools/runtime_tools.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/agent_graph_adapter.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/code_executor.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/common/call_api_helper.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_execution_plan_wiring.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_extract_codeblocks.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_sync_async_tools.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_tool_call_timeout.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/bind_tools.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/find_tools.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/knowledge.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/prompt_utils.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/providers/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/providers/base.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/providers/combined.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/providers/langchain.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/providers/registry.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/tests/test_arguments.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/tests/test_cuga_lite_graph_evolve_guidelines.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/tool_approval_handler.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/tracking/__init__.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/tracking/arguments.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/tracking/tracker.pysrc/cuga/backend/cuga_graph/nodes/cuga_supervisor/cuga_supervisor_graph.pysrc/cuga/backend/cuga_graph/nodes/cuga_supervisor/cuga_supervisor_state.pysrc/cuga/backend/cuga_graph/nodes/cuga_supervisor/supervisor_graph_adapter.pysrc/cuga/backend/cuga_graph/policy/tests/helpers.pysrc/cuga/backend/cuga_graph/policy/tests/test_e2e_healthcare_family_claims.pysrc/cuga/backend/cuga_graph/policy/tests/test_e2e_output_formatter.pysrc/cuga/backend/cuga_graph/policy/tests/test_e2e_playbook_guidance.pysrc/cuga/backend/cuga_graph/policy/tests/test_e2e_playbook_refinement.pysrc/cuga/backend/cuga_graph/policy/tests/test_e2e_tool_enrichment.pysrc/cuga/backend/cuga_graph/policy/tests/test_tool_approval_full_graph.pysrc/cuga/backend/server/main.pysrc/cuga/sdk.pysrc/cuga/sdk_core/tests/test_sdk_integration.pysrc/cuga/supervisor_utils/supervisor_config.pytests/integration/test_llm_config_publish.pytests/integration/test_tool_call_tracking.pytests/system/test_manager_api_integration.pytests/unit/test_cuga_lite_bind_tools.pytests/unit/test_cuga_lite_knowledge_scopes.pytests/unit/test_find_tools_exception.py
💤 Files with no reviewable changes (2)
- src/cuga/backend/cuga_graph/nodes/cuga_agent_core/execution/variable_bridge.py
- src/cuga/backend/cuga_graph/nodes/cuga_agent_core/policy/execution_policy.py
✅ Files skipped from review due to trivial changes (13)
- src/cuga/backend/cuga_graph/nodes/cuga_lite/providers/base.py
- src/cuga/backend/cuga_graph/nodes/cuga_lite/tracking/init.py
- src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/init.py
- src/cuga/supervisor_utils/supervisor_config.py
- src/cuga/backend/cuga_graph/policy/tests/test_e2e_playbook_guidance.py
- src/cuga/backend/cuga_graph/nodes/cuga_lite/executors/common/call_api_helper.py
- tests/integration/test_llm_config_publish.py
- src/cuga/backend/server/main.py
- src/cuga/backend/cuga_graph/policy/tests/test_e2e_playbook_refinement.py
- src/cuga/backend/cuga_graph/policy/tests/test_e2e_healthcare_family_claims.py
- src/cuga/backend/cuga_graph/policy/tests/test_e2e_output_formatter.py
- src/cuga/backend/cuga_graph/nodes/cuga_lite/providers/init.py
- src/cuga/backend/cuga_graph/policy/tests/test_tool_approval_full_graph.py
|
|
||
| The directory is structured to separate shared core machinery, agent graph orchestrators, and specialized task agents. | ||
|
|
||
| ``` |
Contributor
There was a problem hiding this comment.
Add fence languages for markdownlint compliance.
Line 13 and Line 47 use unlabeled fenced code blocks (MD040). Add a language identifier to avoid docs-lint failures.
💡 Suggested patch
-```
+```text
src/cuga/backend/cuga_graph/nodes/
├── cuga_agent_core/ # Shared core adapter-seam engine & execution utilities
│ ├── graph/ # Node builders and adapter interfaces (e.g., CoreGraphAdapter)
@@
-```
+```
@@
- ```
+ ```text
START ──> prepare ──> call_model <──> execute (sandbox / tool loop) ──> END
- ```
+ ```Also applies to: 47-47
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 13-13: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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 `@src/cuga/backend/cuga_graph/nodes/AGENTS.md` at line 13, The markdown file
has unlabeled fenced code blocks causing MD040; update the two code fences in
AGENTS.md (the directory-tree block and the ASCII flow diagram block) by adding
a language identifier (e.g., "text") after the opening triple backticks so the
blocks read ```text instead of ``` to satisfy markdownlint; locate the two
unlabeled fences around the directory listing and the "START ──> prepare ..."
diagram and change them both to use the same language label.
Comment on lines
+119
to
+121
| except Exception: | ||
| # Last resort: wrap in a list | ||
| todos_list = [Todo(**input_data) if isinstance(input_data, dict) else input_data] |
Contributor
There was a problem hiding this comment.
Avoid blind exception handling in todo parsing fallback.
Line 119 catches all exceptions, which can mask real defects in payload normalization. Catch specific parse/validation exceptions instead.
💡 Suggested patch
-from pydantic import BaseModel, Field
+from pydantic import BaseModel, Field, ValidationError
@@
- except Exception:
+ except (TypeError, ValueError, ValidationError):
# Last resort: wrap in a list
todos_list = [Todo(**input_data) if isinstance(input_data, dict) else input_data]🧰 Tools
🪛 Ruff (0.15.13)
[warning] 119-119: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/execution/todos.py` around
lines 119 - 121, The fallback except Exception: in the todo parsing block should
not swallow all errors; narrow the catch to the specific parse/validation
exceptions raised when constructing Todo or normalizing input (e.g., TypeError,
ValueError and the validation exception used by your model library such as
pydantic.ValidationError) and import that ValidationError if needed; update the
except clause around the Todo(**input_data) fallback in todos.py to catch only
those specific exceptions so real bugs still surface while still allowing the
"wrap in a list" fallback to run for expected parsing/validation failures.
|
|
||
| from loguru import logger | ||
|
|
||
| from cuga.backend.cuga_graph.nodes.cuga_lite.providers.base import AppDefinition |
Contributor
There was a problem hiding this comment.
Move AppDefinition to a core/shared schema module.
cuga_agent_core now depends on cuga_lite.providers.base, which inverts the intended dependency direction and couples shared core code to Lite internals. Please relocate shared provider schemas (e.g., AppDefinition, related interfaces) into cuga_agent_core and import from there.
Based on learnings: Ensure downstream layers (like cuga_supervisor) never depend on the internal helper files of upstream layers (like cuga_lite). Shared schemas must live in cuga_agent_core.
🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_agent_core/tools/runtime_tools.py` at
line 23, Replace the cross-layer import of AppDefinition from
cuga_lite.providers.base with a shared schema in cuga_agent_core: move the
AppDefinition class (and any related provider interfaces used by
runtime_tools.py) into a new or existing core/shared schema module within
cuga_agent_core, update the import in runtime_tools.py to import AppDefinition
from that new core module, and ensure any other modules that currently import
AppDefinition from cuga_lite.providers.base are updated to import from the
cuga_agent_core shared schema instead so upstream internals are not referenced
by downstream layers.
Comment on lines
+24
to
+25
| except Exception: | ||
| pass |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
uv run ruff check src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/bind_tools.pyRepository: cuga-project/cuga-agent
Length of output: 110
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/bind_tools.py"
echo "=== File line count ==="
wc -l "$FILE"
echo "=== Check ruff availability ==="
(ruff --version && echo "ruff_available=1") || echo "ruff_available=0"
echo "=== Show around claimed bare except blocks ==="
# Print windows around lines ~1-80 and ~30-70 to catch 24/38/52
sed -n '1,90p' "$FILE" | cat -n
echo "=== Show around later claimed log lines (80-160) ==="
sed -n '80,170p' "$FILE" | cat -n
echo "=== Show around claimed log lines (200-280) ==="
sed -n '190,270p' "$FILE" | cat -n
echo "=== Show around claimed log lines (280-360) ==="
sed -n '270,360p' "$FILE" | cat -n
echo "=== Find bare except Exception: in file ==="
rg -n "except Exception:\s*pass" "$FILE" || true
echo "=== Find any except Exception blocks ==="
rg -n "except Exception\s*:" "$FILE" || true
echo "=== Find Loguru/logger calls that use %s ==="
# Look for percent-s formatting in the file (common in old-style formatting)
rg -n "%s" "$FILE" || true
# Narrow to logger/loguru calls containing %s
rg -n "(logger|loguru|log)\.[a-zA-Z_]*\([^)]*%s" "$FILE" || true
echo "=== Run ruff check if available ==="
if command -v ruff >/dev/null 2>&1; then
ruff check "$FILE"
fiRepository: cuga-project/cuga-agent
Length of output: 22840
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from loguru import logger
# Test whether Loguru uses printf-style placeholders (%s) or treats them literally.
# We'll set a deterministic sink and capture output by formatting to stdout.
logger.remove()
logger.add(lambda msg: print(msg, end=""), format="{message}")
logger.warning("test %s", 123)
logger.warning("test {}", 123)
PYRepository: cuga-project/cuga-agent
Length of output: 186
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from loguru import logger
logger.remove()
# Print only the rendered message for easy comparison
logger.add(lambda m: print(m, end=""), format="{message}")
logger.warning("loguru %s", 123)
logger.warning("loguru {}", 123)
PYRepository: cuga-project/cuga-agent
Length of output: 186
Fix lint-blocking exception handling in bind_tools.py
- Replace the silent
except Exception: passblocks in_bind_tools_mode_from_settings,_bind_tools_apps_from_settings, and_bind_tools_tool_names_from_settings(lines 24-25, 38-39, 52-53) with targeted exceptions and at least a debug/warning log. - Address BLE001 “Do not catch blind exception:
Exception” in this file beyond thosepassblocks (e.g., lines 62, 95, 230, 285, 338). - Update Loguru warnings that use
%splaceholders (lines 96, 110, 231, 250, 286, 335, 339) to Loguru’s{}formatting style.
🧰 Tools
🪛 Ruff (0.15.13)
[error] 24-25: try-except-pass detected, consider logging the exception
(S110)
[warning] 24-24: Do not catch blind exception: Exception
(BLE001)
🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/bind_tools.py` around
lines 24 - 25, Replace the blind `except Exception: pass` in
_bind_tools_mode_from_settings, _bind_tools_apps_from_settings, and
_bind_tools_tool_names_from_settings with targeted exception handlers (e.g.,
KeyError, TypeError, ValueError as appropriate) and emit a logger.debug or
logger.warning with contextual info and the caught exception; similarly locate
and replace other blind catches in this module (the remaining try/except blocks
that catch Exception) with specific exceptions and proper logging. Also update
all Loguru warning calls that currently use "%s" placeholders to use Loguru's
"{}" formatting (i.e., change logger.warning("...%s...", var) ->
logger.warning("...{}...", var)) so all warning messages at the noted call sites
follow the correct formatting. Ensure you import or use the existing logger
(Loguru) and do not swallow exceptions — re-raise or handle them appropriately
after logging when necessary.
Comment on lines
+96
to
+112
| logger.warning("bind_tools: get_all_tools failed: %s", e) | ||
| return {} | ||
| by_name: Dict[str, StructuredTool] = {} | ||
| duplicates: Set[str] = set() | ||
| for t in all_tools or []: | ||
| n = getattr(t, "name", None) or "" | ||
| if not n: | ||
| continue | ||
| if n in by_name: | ||
| duplicates.add(n) | ||
| continue | ||
| by_name[n] = t | ||
| if duplicates: | ||
| logger.debug( | ||
| "bind_tools: duplicate tool names from provider (using first): %s", | ||
| sorted(duplicates), | ||
| ) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/bind_tools.py"
echo "== File =="
wc -l "$FILE"
echo
echo "== Import/Logger definitions (top 120 lines) =="
sed -n '1,160p' "$FILE" | nl -ba
echo
echo "== Logger call occurrences in file (warning/debug) =="
rg -n 'logger\.(warning|debug|info|error|exception)\s*\(' "$FILE" || true
echo
echo "== Context around reported lines (85-130, 215-270, 280-310, 320-360) =="
sed -n '70,140p' "$FILE" | nl -ba
echo "----"
sed -n '200,270p' "$FILE" | nl -ba
echo "----"
sed -n '270,310p' "$FILE" | nl -ba
echo "----"
sed -n '310,360p' "$FILE" | nl -baRepository: cuga-project/cuga-agent
Length of output: 258
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/bind_tools.py"
echo "== File =="
wc -l "$FILE"
echo
echo "== Logger/import definitions (top 120 lines) =="
sed -n '1,160p' "$FILE" | cat -n
echo
echo "== Logger call occurrences in file (warning/debug/info/error/exception) =="
rg -n 'logger\.(warning|debug|info|error|exception)\s*\(' "$FILE" || true
echo
echo "== Context around reported lines (70-140, 200-310, 310-360) =="
sed -n '70,140p' "$FILE" | cat -n
echo "----"
sed -n '200,270p' "$FILE" | cat -n
echo "----"
sed -n '270,310p' "$FILE" | cat -n
echo "----"
sed -n '310,360p' "$FILE" | cat -nRepository: cuga-project/cuga-agent
Length of output: 17820
🌐 Web query:
Loguru logger formatting %s placeholders supported or not
💡 Result:
Loguru does not natively support %s-style formatting placeholders [1][2]. It is designed to use curly brace {} placeholders, functioning similarly to Python's str.format method [1][3][2]. If you pass a string containing %s placeholders to a Loguru logging function, it will treat the string as a literal and will not perform substitution on those placeholders. To achieve similar results in Loguru, you should replace %s with {} and pass the corresponding values as additional arguments [1][2]. For example: Instead of: logger.info("User: %s", username) Use: logger.info("User: {}", username) This approach leverages Loguru's lazy evaluation, which is more efficient because it avoids formatting the string if the log record is not ultimately emitted (e.g., if the log level is lower than the configured threshold) [4][2].
Citations:
- 1: https://github.com/Delgan/loguru/blob/7b6f6e3741eea7b56273e072a008dc2564592496/README.md
- 2: https://loguru.readthedocs.io/en/stable/overview.html
- 3: https://loguru.readthedocs.io/en/0.7.1/api/logger.html
- 4: Are f-strings supported? Delgan/loguru#289
🏁 Script executed:
python3 - <<'PY'
import re, pathlib
path = pathlib.Path("src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/bind_tools.py")
lines = path.read_text().splitlines()
pattern = re.compile(r'\blogger\.(warning|debug|info|error|exception)\s*\(')
hits=[]
for i,l in enumerate(lines, start=1):
if 'logger.' in l and pattern.search(l):
hits.append(i)
print("Logger call starting lines:", hits)
print()
for i in hits:
# print 6 lines starting at i-1
start=max(1,i-2)
end=min(len(lines), i+6)
print(f"--- {path}:{i} ---")
for j in range(start,end+1):
print(f"{j:4d}\t{lines[j-1]}")
print()
PYRepository: cuga-project/cuga-agent
Length of output: 8202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/bind_tools.py"
rg -n --hidden --no-ignore -S '"%s' "$FILE" || true
rg -n --hidden --no-ignore -S 'logger\.(warning|debug|info|error|exception)\(.*%s' "$FILE" || trueRepository: cuga-project/cuga-agent
Length of output: 414
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/bind_tools.py"
rg -n --fixed-strings '%s' "$FILE" || trueRepository: cuga-project/cuga-agent
Length of output: 819
Use Loguru {} placeholders instead of %s in logger calls.
These messages won’t interpolate values as intended with Loguru, so diagnostics lose key context.
Suggested fix
-logger.warning("bind_tools: get_all_tools failed: %s", e)
+logger.warning("bind_tools: get_all_tools failed: {}", e)
-logger.debug("bind_tools: duplicate tool names from provider (using first): %s", sorted(duplicates))
+logger.debug("bind_tools: duplicate tool names from provider (using first): {}", sorted(duplicates))Also applies to: 231-231, 250-251, 286-286, 324-325, 335-336, 339-339
🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/bind_tools.py` around
lines 96 - 112, The Loguru logger calls in bind_tools use printf-style "%s"
which doesn't interpolate; update the calls (e.g., inside get_all_tools where
logger.warning("bind_tools: get_all_tools failed: %s", e) and the duplicate-name
logger.debug call) to use Loguru "{}" placeholders and pass the values as
arguments (e.g., logger.warning("bind_tools: get_all_tools failed: {}", e) and
logger.debug("bind_tools: duplicate tool names from provider (using first): {}",
sorted(duplicates))). Apply the same change for the other logger invocations
noted in this file (the other occurrences around the duplicates/by_name logic
and at the referenced ranges).
Comment on lines
+83
to
+90
| if app_to_tools_map and app_name in app_to_tools_map: | ||
| filtered_tools = app_to_tools_map[app_name] | ||
| else: | ||
| logger.warning( | ||
| f"App '{app_name}' not found in app_to_tools_map. Available apps: {list(app_to_tools_map.keys()) if app_to_tools_map else 'N/A'}" | ||
| ) | ||
| filtered_tools = [] | ||
|
|
Contributor
There was a problem hiding this comment.
Fix empty-tool fallback when app_to_tools_map is not provided.
Line 83-Line 90 drops to filtered_tools = [] whenever the optional map is absent, so find_tools can return no matches even though all_tools is available.
Suggested fix
- if app_to_tools_map and app_name in app_to_tools_map:
- filtered_tools = app_to_tools_map[app_name]
- else:
+ if app_to_tools_map and app_name in app_to_tools_map:
+ filtered_tools = app_to_tools_map[app_name]
+ elif app_to_tools_map is None:
+ filtered_tools = all_tools
+ else:
logger.warning(
f"App '{app_name}' not found in app_to_tools_map. Available apps: {list(app_to_tools_map.keys()) if app_to_tools_map else 'N/A'}"
)
filtered_tools = []🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/helpers/find_tools.py` around
lines 83 - 90, The current branch sets filtered_tools = [] when app_to_tools_map
is not provided, causing find_tools to miss matches even though all_tools
exists; update the else branch so that when app_to_tools_map is falsy you derive
filtered_tools from all_tools by selecting tools that belong to app_name (e.g.,
filter all_tools by a tool property like tool.app or membership in tool.apps or
another identifier used in your tool objects), keeping the existing behavior
when app_to_tools_map is present (use app_to_tools_map[app_name] if found) and
only falling back to an empty list if neither map nor any matching tool in
all_tools exist. Reference: function find_tools, variables app_to_tools_map,
app_name, all_tools, and filtered_tools.
Comment on lines
+63
to
72
| if isinstance(tool, StructuredTool) and not hasattr(tool, "func"): | ||
| if not hasattr(tool, "coroutine") and not hasattr(tool, "_run"): | ||
| logger.warning( | ||
| f"StructuredTool '{tool.name}' is missing .func attribute. " | ||
| f"Adding it for CodeAct compatibility." | ||
| ) | ||
| if hasattr(tool, 'coroutine') and tool.coroutine: | ||
| if hasattr(tool, "coroutine") and tool.coroutine: | ||
| tool.func = tool.coroutine | ||
| elif hasattr(tool, '_run'): | ||
| elif hasattr(tool, "_run"): | ||
| tool.func = tool._run |
Contributor
There was a problem hiding this comment.
Fix inverted condition when backfilling StructuredTool.func.
The guard is inverted: the code only enters the block when both coroutine and _run are missing, then tries to assign from them. That assignment branch is unreachable, so .func never gets patched.
Suggested fix
- if isinstance(tool, StructuredTool) and not hasattr(tool, "func"):
- if not hasattr(tool, "coroutine") and not hasattr(tool, "_run"):
- logger.warning(
- f"StructuredTool '{tool.name}' is missing .func attribute. "
- f"Adding it for CodeAct compatibility."
- )
- if hasattr(tool, "coroutine") and tool.coroutine:
- tool.func = tool.coroutine
- elif hasattr(tool, "_run"):
- tool.func = tool._run
+ if isinstance(tool, StructuredTool) and not hasattr(tool, "func"):
+ logger.warning(
+ f"StructuredTool '{tool.name}' is missing .func attribute. "
+ f"Adding it for CodeAct compatibility."
+ )
+ if hasattr(tool, "coroutine") and tool.coroutine:
+ tool.func = tool.coroutine
+ elif hasattr(tool, "_run"):
+ tool.func = tool._run📝 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.
Suggested change
| if isinstance(tool, StructuredTool) and not hasattr(tool, "func"): | |
| if not hasattr(tool, "coroutine") and not hasattr(tool, "_run"): | |
| logger.warning( | |
| f"StructuredTool '{tool.name}' is missing .func attribute. " | |
| f"Adding it for CodeAct compatibility." | |
| ) | |
| if hasattr(tool, 'coroutine') and tool.coroutine: | |
| if hasattr(tool, "coroutine") and tool.coroutine: | |
| tool.func = tool.coroutine | |
| elif hasattr(tool, '_run'): | |
| elif hasattr(tool, "_run"): | |
| tool.func = tool._run | |
| if isinstance(tool, StructuredTool) and not hasattr(tool, "func"): | |
| logger.warning( | |
| f"StructuredTool '{tool.name}' is missing .func attribute. " | |
| f"Adding it for CodeAct compatibility." | |
| ) | |
| if hasattr(tool, "coroutine") and tool.coroutine: | |
| tool.func = tool.coroutine | |
| elif hasattr(tool, "_run"): | |
| tool.func = tool._run |
🤖 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 `@src/cuga/backend/cuga_graph/nodes/cuga_lite/providers/langchain.py` around
lines 63 - 72, The condition that backfills StructuredTool.func is inverted:
currently the code only runs when both coroutine and _run are missing, making
the assignment branches unreachable; change the inner guard to check for the
presence of either implementation (use if hasattr(tool, "coroutine") or
hasattr(tool, "_run")) so the block runs when we can copy an existing callable
into tool.func, and keep the assignment order that prefers tool.coroutine then
tool._run and the logger.warning message in the same block.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Chore
Closes #205
Summary
Extracts duplicated graph logic from
CugaLiteGraphandCugaSupervisorGraphinto a new sharedcuga_agent_coremodule, making both agents more modular and easier to contribute to.What changed:
New
cuga_agent_core/module — shared building blocks extracted from both agents:graph_nodes.py/shared_nodes.py— common LangGraph node implementationsshared_graph.py— reusable graph builder utilitiesexecution_policy.py— unified execution policy logicruntime_tools.py— shared tool resolution and wiringcode_extraction.py— code block extraction (moved fromcode_act_agent)variable_bridge.py— variable pass-through between graph layersCugaLiteGraphrefactored — now delegates toAgentGraphAdapter(cuga_lite/agent_graph_adapter.py); core graph logic slimmed from ~700 → ~150 linesCugaSupervisorGraphrefactored — now delegates toSupervisorGraphAdapter(cuga_supervisor/supervisor_graph_adapter.py); same pattern appliedDeleted
code_act_agent.py— logic absorbed intocuga_agent_coreFull test coverage — 13 new test files covering all extracted modules and both adapters
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores