fix(cuga-lite): don't stop mid-task after stating a plan; warn on suspicious tool arguments; correct made-up tool names#416
Conversation
CugaLite occasionally emits a short first-person plan with no code on a
turn where it clearly intends to keep working (e.g. "We need to search
student_loan app."). The LLM-based auto-continue classifier sometimes
misfires on these and finalizes the plan as the answer, producing a turn
with zero tool calls (the "planning-text stall").
Add a deterministic fast-path, looks_like_planning_text(), that detects
short first-person intent statements and forces auto_continue=True before
the model call. It is conservative: length-capped, skips questions, and a
negation guard lets genuine results ("I could not find ...") fall through.
The pre-routing step limit bounds any over-fire against infinite loops.
Adds unit tests covering the real observed stall strings, false-positive
guards, and the LLM short-circuit behaviour.
Default-off guardrail for the NL auto-continue loop, gated by the
advanced_features.require_tool_call_before_final setting (behavior
unchanged when unset):
- When the model would emit a final answer but no tool/code block has
executed this task, refuse it: inject a directive ("call a tool before
answering; do not answer from prior knowledge") and continue instead.
Catches the "0 tool calls -> confident hallucination" pattern.
Bounded by the existing step limit.
The code-execution counter is carried in the adapter metadata dict (no
graph state schema change; works for lite/supervisor/core). The write is
skipped when the guard is off, so the disabled path is byte-identical to
before.
… guard Flag suspicious tool arguments without mutating them (a 200-task mining pass showed the target coercion case never occurs in agent execution, so no auto-fix is warranted), and surface a tool-name correction listing the closest real tools when the agent calls a fabricated tool name instead of a bare NameError. - arg_warning: cuga_lite_warn_suspect_args (default on) detects-only - unknown-tool correction in local_executor via difflib close-matches; falls back to a find_tools re-query hint when no close match exists
|
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:
📝 WalkthroughWalkthroughThis PR adds a detection-only warning layer for suspicious CugaLite tool-call kwargs behind a new settings flag, extends ChangesCugaLite argument warning and tool-name correction diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Planning-text fast-path for NL auto-continue classifier
Estimated code review effort: 2 (Simple) | ~12 minutes Metadata construction cleanup
Estimated code review effort: 1 (Trivial) | ~3 minutes Sequence Diagram(s)sequenceDiagram
participant CodeExecutor
participant LocalExecutor
participant difflib
CodeExecutor->>LocalExecutor: format_error(error, available_tools, code)
LocalExecutor->>difflib: get_close_matches(missing_name, available_tools)
difflib-->>LocalExecutor: close match or none
LocalExecutor-->>CodeExecutor: formatted error with correction hint
sequenceDiagram
participant classify_nl_auto_continue
participant looks_like_planning_text
participant LLM
classify_nl_auto_continue->>looks_like_planning_text: inspect visible text
looks_like_planning_text-->>classify_nl_auto_continue: match or no match
alt planning text matches
classify_nl_auto_continue-->>classify_nl_auto_continue: return True
else no match
classify_nl_auto_continue->>LLM: ainvoke classifier prompt
LLM-->>classify_nl_auto_continue: JSON result
end
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
the
complexity: high
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/cuga/backend/cuga_graph/nodes/cuga_lite/tests/test_arg_warning.py (1)
109-171: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a synchronous tool case here.
All end-to-end wrapper tests use an async callable, but
prepare_tools_and_appsalso wraps sync.func/._runtools. A one-test sync fixture would catch the current unconditional-awaitregression immediately. As per coding guidelines, "Write new unit tests in corresponding tests/ sub-folders ... to verify all new branches and 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/tests/test_arg_warning.py` around lines 109 - 171, Add a synchronous end-to-end wrapper test in test_arg_warning.py to cover the prepare_tools_and_apps sync-tool path, since make_arg_warning_callable currently only has async fixtures. Create a sync callable fixture alongside _recorder and verify the wrapped function preserves args/kwargs without awaiting, using the existing make_arg_warning_callable entry point and a model like DirectorModel or YearModel to catch the unconditional-await regression.Source: Coding guidelines
🤖 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/cuga_lite/adapter/arg_warning.py`:
- Around line 120-125: The wrapper in arg_warning.py always awaits tool_func,
but prepare_tools_and_apps also routes sync .func/_run callables through this
path, so the wrapper must support both sync and async tools. Update wrapper to
detect whether tool_func is awaitable (or call it first and then await only when
needed), while keeping the existing warn_suspect_kwargs behavior and preserving
the current wrapper signature and call flow.
In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/nl_auto_continue_classifier.py`:
- Around line 61-63: The `_NEGATION_RE` pattern in
`nl_auto_continue_classifier.py` uses a literal curly apostrophe, which triggers
Ruff’s RUF001 in `cuga_graph/nodes`. Update the regex in `_NEGATION_RE` to avoid
the raw `’` by using an escaped form such as `\u2019` or an ASCII-only
equivalent, then run `ruff format` and `ruff check` for the `cuga_graph/nodes/`
Python changes.
In
`@src/cuga/backend/cuga_graph/nodes/cuga_lite/tests/test_nl_auto_continue_classifier.py`:
- Around line 21-25: Add a regression test in test_nl_auto_continue_classifier
alongside the existing classifier tests that disables cuga_lite_nl_auto_continue
and verifies a planning-style utterance still returns False. Keep the autouse
_enable_auto_continue fixture for the current positive-path tests, but override
the flag off in the new test using the same mod.settings.advanced_features
setting so the fast-path branch is exercised explicitly.
---
Nitpick comments:
In `@src/cuga/backend/cuga_graph/nodes/cuga_lite/tests/test_arg_warning.py`:
- Around line 109-171: Add a synchronous end-to-end wrapper test in
test_arg_warning.py to cover the prepare_tools_and_apps sync-tool path, since
make_arg_warning_callable currently only has async fixtures. Create a sync
callable fixture alongside _recorder and verify the wrapped function preserves
args/kwargs without awaiting, using the existing make_arg_warning_callable entry
point and a model like DirectorModel or YearModel to catch the
unconditional-await regression.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ade3e1d-f74b-49ca-9088-684136851dd7
📒 Files selected for processing (13)
src/cuga/backend/cuga_graph/nodes/cuga_agent_core/graph/shared_nodes.pysrc/cuga/backend/cuga_graph/nodes/cuga_agent_core/tests/graph/test_shared_call_model.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/adapter/arg_warning.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/adapter/prepare_node.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/base_executor.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/code_executor.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/local/local_executor.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_code_executor.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/nl_auto_continue_classifier.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/tests/test_arg_warning.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/tests/test_nl_auto_continue_classifier.pysrc/cuga/config.pysrc/cuga/settings.toml
|
requires testing on appworld again and lets get help of @Sergey-Zeltyn on groq |
- arg_warning: the warning wrapper unconditionally awaited tool_func, but prepare_tools_and_apps also routes sync .func/._run callables through it. Call the tool and await only when the result is awaitable; add a sync-tool regression test. - nl_auto_continue_classifier: replace the literal curly apostrophe in _NEGATION_RE with ’ to clear Ruff RUF001 (ambiguous unicode). - tests: add a regression test asserting the classifier finalizes (returns False, no LLM call) on planning text when cuga_lite_nl_auto_continue is off.
coderabbitai
Bot
added
complexity: medium
test-easy results on litellm gpt-oss-120b: Evaluation ReportSummaryPass@1 28/43 (65.1%) Per-Task ResultsTask R Tokens Cost LLM Cache Duration Steps Difficulty BreakdownDiff Tasks Pass@1 Tokens Tok/Task LLM Calls LLM/Task Duration Dur/Task Test-Set Breakdown (AppWorld)Test Set Tasks Pass@1 Tokens Tok/Task LLM Calls LLM/Task Duration Dur/Task |
sami-marreed
left a comment
sami-marreed
left a comment
There was a problem hiding this comment.
Code review — overall: merge with fixes. Three independent, well-scoped fixes; defaults are backward-compatible and non-intrusive as claimed; all four new test files pass (78/78). One trivial must-fix plus two should-fix items, left inline.
Strengths
- Genuinely independent fixes on disjoint code paths, each individually gated.
- Arg-warning is provably non-mutating (forwards kwargs verbatim; tests assert "NOT coerced").
- Tests exercise real behavior (real code through
eval_with_tools_async, real pydantic models,assert_not_called()), not mocks. - Conservative classifier: negation/question guards, 400-char cap, anchored match, bounded by step limit.
Minor (not inlined)
- PR body says 76 tests; actually 78 passing.
meta_valuealiases the state metadata dict, someta_value["code_exec_count"] = ...mutates graph state beforeCommand(update=...)is returned. Works under LangGraph but adict(meta_value)copy is cleaner.require_tool_call_before_finalloops to the step limit on legitimate no-tool tasks (e.g. a greeting), returning a step-limit error instead of an answer. Opt-in, but worth noting in the flag's help text.
| # directive and continue instead. Bounded by the step limit. | ||
| if require_tool_call and code_exec_count == 0: | ||
| logger.warning( | ||
| "%s: require_tool_call_before_final — final answer with 0 tool calls; injecting directive and continuing", |
There was a problem hiding this comment.
Important — loguru %s regression. Loguru formats with {}, not printf %s. This logs the literal %s: ... and silently drops adapter.sender_name. This is exactly the defect class that #389 just removed across 22 files (merged 2 commits before this PR's base). Fix with an f-string:
logger.warning(
f"{adapter.sender_name}: require_tool_call_before_final — final answer with 0 tool calls; injecting directive and continuing"
)| if require_tool_call: | ||
| # Record that at least one tool/code block has run this task. | ||
| meta_value["code_exec_count"] = code_exec_count + 1 |
There was a problem hiding this comment.
Important — counts emitted code blocks, not successful tool calls. code_exec_count increments whenever the model emits any code block, regardless of whether it executed successfully or actually called a tool. A model can emit x = 1 (or a block that NameErrors) and thereby satisfy require_tool_call_before_final, then still hallucinate a final answer — weaker than the name implies. Default-off mitigates severity. Either increment only after the execute node reports a non-error tool invocation, or tighten the comment/flag wording to "at least one code block executed."
There was a problem hiding this comment.
Took the wording option in 2827681: the code comment and the settings.toml help now state it counts emitted/executed code blocks (a x = 1 block or an erroring block still counts) — a lightweight grounding proxy, not a confirmed-tool-call guarantee. Counting only non-error tool invocations would need the execute node to report back into the adapter metadata; given the flag is default-off I kept the cheap proxy and made the docs honest about it.
| tool_func = make_arg_warning_callable( | ||
| tool_func, | ||
| getattr(tool, "args_schema", None), | ||
| enable=_warn_args, | ||
| ) |
There was a problem hiding this comment.
Important — default-on warning silently moves sync tools onto the event loop. For a sync-only tool (no .coroutine, has .func/._run) with an args_schema, the old path was sync func → make_tool_awaitable → run_in_executor (worker thread). make_arg_warning_callable returns an async def wrapper that calls the sync tool_func inline; make_tool_awaitable then sees a coroutine function and awaits it directly — so the sync tool now runs in the event loop instead of a worker thread, and a blocking sync tool could stall the loop. Scope is narrow (only sync-only tools with a schema; .coroutine tools are unaffected) but it's a silent, default-on behavior change. Fix: dispatch sync callables via run_in_executor inside the wrapper, or only wrap when inspect.iscoroutinefunction(tool_func).
There was a problem hiding this comment.
Fixed in 2827681 — make_arg_warning_callable now preserves the sync/async nature of the wrapped tool: async tools get an async wrapper, sync tools a sync wrapper, so make_tool_awaitable still sees a non-coroutine for sync-only tools and dispatches them via run_in_executor as before. Regression test covers the sync path.
- shared_nodes: fix loguru `%s` placeholder — it logged a literal `%s` and
dropped `adapter.sender_name` (loguru formats with `{}`). Now an f-string.
Same defect class swept out in #389.
- shared_nodes: copy the metadata dict before bumping `code_exec_count` so the
increment updates only the returned Command payload, not live graph state.
- arg_warning: preserve the sync/async nature of the wrapped tool. The wrapper
was always `async def`, so `make_tool_awaitable` awaited sync tools inline on
the event loop instead of dispatching them via run_in_executor — a blocking
sync tool could stall the loop. Sync tools now get a sync wrapper.
- Tighten `require_tool_call_before_final` wording (comments + settings help):
it counts emitted/executed code blocks, not confirmed successful tool calls,
and legitimately tool-free tasks loop to the step limit.
|
@sami-marreed @Sergey-Zeltyn Re-ran appworld easy with increased timeout. Below results for gpt-oss-120b and gpt-4.1 GPT-4.1 is a bit below what Sergey had before (84.7%) |
|
There are several changes in PR, here I comment on one of them Deterministic False positives = wasted steps. Any genuine final answer that matches the regex (starts with we/I/let's/let me + optional discourse marker + a present-tense action/modal verb, ≤400 chars, no ?, no negation) now loops instead of finalizing. The design mitigates this well — past-tense finals ("I found…", "I determined…") don't match because the verb list is present-tense/infinitive with \b boundaries, and questions/negations are excluded. But a correct final phrased as "I'll return the list: …" would match and burn a step. With cuga_lite_max_steps=70 it can't loop forever, but repeated over-fire near the cap could convert a would-be pass into a step-limit error. My verdict when I consider AppWorld ground truth answers: it should not affect appworld negatively |
|
Doing some gpt-oss log analysis to find possible concerns Concern (misdirection from the unknown-tool correction): The new Example (real, bundle b6d1104, score 1.0 — a passing task): Agent runs a block referencing formatted_total, never defined → Rare (1 exposed passing task in 87; the other 84 NameErrors were genuinely fabricated tool names where the correction helps), but it's the one place the change can actively hurt. |
|
Second (and last) concern loose cutoff=0.4 in difflib.get_close_matches(missing, available_tools, n=5, cutoff=0.4) (_unknown_tool_correction, local_executor.py): The app-prefix structure usually keeps suggestions in the right app and often exact (e.g.
|
Address PR #416 review (Sergey-Zeltyn) on _unknown_tool_correction: - suppress the correction when the missing name is a plain variable reference — it now fires only when the name is called like a function, so an undefined variable keeps its bare NameError instead of getting misleading "call find_tools / do not retry" guidance - raise difflib cutoff 0.4 -> 0.6 so weak cross-app matches (e.g. simple_note_* -> spotify_*) are no longer offered as suggestions - add a caution line to suggestions: lookalike names can perform different actions (delete vs get), pick by intent not list order - pass the executed code through format_error so the call-shape check has something to inspect; update BaseExecutor signature accordingly - add regression tests for all three behaviors (82 tests pass)
|
@Sergey-Zeltyn addressed in cc2d8a8: Variable NameError misdirection — the correction now fires only when the missing name is called like a function in the executed code ( Loose cutoff=0.4 — raised to 0.6. Verified against your examples: Regression tests added for all three behaviors (variable NameError stays bare, cross-app junk filtered, caution present) — 82 tests pass. Planning-text short-circuit — no change; per your analysis the present-tense verb list and question/negation guards keep AppWorld finals out of the match, and the step cap bounds any residual over-fire. |
|
@sami-marreed swept your review — inline threads answered individually; the non-inlined minors:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_code_executor.py (1)
167-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the "no close match" assertion.
The
"find_tools" in hintcheck (line 184) is trivially true regardless ofavailable_toolscontent — the fallback branch's static text always says "Call find_tools again with a different query." It doesn't actually validate that no other weak match slipped through besides the one explicitly excluded. Consider asserting"Did you mean" not in hint(or"No close match" in hint) to directly pin the branch taken.✅ Suggested strengthened assertion
assert "tool-name correction" in hint assert "spotify_create_playlist_post" not in hint - assert "find_tools" in hint + assert "Did you mean" not in hint + assert "find_tools" in hint🤖 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_code_executor.py` around lines 167 - 185, The test in test_correction_cutoff_rejects_cross_app_junk is only checking for the static fallback text from _unknown_tool_correction, so it does not prove the no-close-match branch was taken. Strengthen the assertion by validating the fallback path directly in the hint returned from LocalExecutor._unknown_tool_correction, for example by checking that “Did you mean” is absent or that “No close match” is present, while keeping the existing exclusion of spotify_create_playlist_post.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@src/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_code_executor.py`:
- Around line 167-185: The test in test_correction_cutoff_rejects_cross_app_junk
is only checking for the static fallback text from _unknown_tool_correction, so
it does not prove the no-close-match branch was taken. Strengthen the assertion
by validating the fallback path directly in the hint returned from
LocalExecutor._unknown_tool_correction, for example by checking that “Did you
mean” is absent or that “No close match” is present, while keeping the existing
exclusion of spotify_create_playlist_post.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d7e42e29-9a7f-45d9-8260-3dfa55083a58
📒 Files selected for processing (4)
src/cuga/backend/cuga_graph/nodes/cuga_lite/executors/base_executor.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/code_executor.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/local/local_executor.pysrc/cuga/backend/cuga_graph/nodes/cuga_lite/executors/tests/test_code_executor.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/cuga/backend/cuga_graph/nodes/cuga_lite/executors/base_executor.py
- src/cuga/backend/cuga_graph/nodes/cuga_lite/executors/code_executor.py
- src/cuga/backend/cuga_graph/nodes/cuga_lite/executors/local/local_executor.py
|
@coderabbitai can you label this pr readiness ? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="cuga-project/cuga-agent"
echo "== Existing labels matching 'readiness' =="
gh label list --repo "$repo" --search "readiness" || true
echo "== All labels (for reference) =="
gh label list --repo "$repo" --limit 100Length of output: 2669 🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="cuga-project/cuga-agent"
pr=416
gh pr edit "$pr" --repo "$repo" --add-label "ready: maintainer-review"
gh pr view "$pr" --repo "$repo" --json labels --jq '.labels[].name'Length of output: 633
Could you apply |
| # Model wants to finalize. Guard: refuse a final answer produced without | ||
| # any tool/code execution (e.g. a confident hallucination); inject a | ||
| # directive and continue instead. Bounded by the step limit. | ||
| if require_tool_call and code_exec_count == 0: |
There was a problem hiding this comment.
Edge cases when require tool call is true:
- Saying to the agent
what can you do?-->no tools-->Error - Will affect working with skills and no tools
- Follow-up questions on previous threads
Problem: Cuga returns intermediate/hallucinated answers without doing actual computations based on tools
examples:
- Let me perform the second phase. ( intermediate )
- Cuga answers based on general LLM knowledge instead of tool computation
Open questions:
- Does the flag of autonomous mode relate to this flag require_tool_call?
Decision:
- Let's remove it and open issue on agent answering from general knowledge instead of tools, use might want to configure that when to answer in general/llm vs based on tools
| # Pre-flight arg WARNING: the sandbox calls tool.coroutine directly, | ||
| # bypassing the StructuredTool's args_schema, so nothing flags malformed | ||
| # kwargs before the registry. This detector logs (never mutates) suspect | ||
| # shapes — the dict-as-string bug and friends. The former coercion layer | ||
| # (Wave-1 Change #4) is WONTFIX: a 200-task M3 mining pass found the | ||
| # failure does not occur in the current corpus. See arg_warning.py. | ||
| _af = getattr(settings, "advanced_features", None) | ||
| _warn_args = bool(getattr(_af, "cuga_lite_warn_suspect_args", True)) |
There was a problem hiding this comment.
Provide issue with example input and output. delete M3 logs etc
| def _unknown_tool_correction( | ||
| error: Exception, | ||
| available_tools: Optional[list[str]], | ||
| code: Optional[str] = None, | ||
| ) -> str: | ||
| """Build a correction hint when the agent calls a non-existent tool. |
There was a problem hiding this comment.
Add issue for tool name hallucination, we might already have one
| if not missing: | ||
| return "" | ||
|
|
||
| if code is not None and not re.search(rf"\b{re.escape(missing)}\s*\(", code): |
There was a problem hiding this comment.
This can match many phrases like var = " agent_1"
There was a problem hiding this comment.
sometimes it can also catch a helper function that agent generated with current code
async def helper1(arg1):
pass
res = await helper1(arg1)
| async def classify_nl_auto_continue( | ||
| llm: BaseChatModel, | ||
| assistant_visible: Any, | ||
| reasoning_excerpt: Optional[Any], | ||
| ) -> bool: | ||
| """Return True if the graph should append a user ``continue`` message and re-invoke the coder model.""" | ||
| if not getattr(settings.advanced_features, "cuga_lite_nl_auto_continue", True): | ||
| return False | ||
| visible = normalize_assistant_text(assistant_visible) | ||
| reasoning = normalize_assistant_text(reasoning_excerpt) | ||
| if looks_like_planning_text(visible): | ||
| logger.info("NL auto-continue: planning-text fast-path matched; auto-continuing") | ||
| return True | ||
| combined = build_combined_content_and_reasoning(visible, reasoning) |
There was a problem hiding this comment.
Make sure you have issue created on looks like planning text and what's the best approach to solve it
| r"explore|examine|check|investigate|figure\s+out|determine|" | ||
| r"retrieve|gather|list|enumerate)\b", | ||
| re.IGNORECASE, | ||
| ) |
There was a problem hiding this comment.
@coderabbitai how this will react to Ok i will fetch the infromation, but first i require your ID
There was a problem hiding this comment.
@sami-marreed As written, that example would currently match the fast-path and return True for auto-continue.
Why:
- it starts with a permitted discourse marker + first person:
Ok i - then hits a planning/modal phrase:
will - then an action verb in the allowlist:
fetch - it does not end with
? - it does not contain a negation like
not,cannot,couldn't, etc.
So for:
Ok i will fetch the infromation, but first i require your ID
looks_like_planning_text(...) would likely treat it as “planning text”, and classify_nl_auto_continue(...) would short-circuit before the LLM and auto-continue.
The important nuance is that the tail clause (but first i require your ID) is ignored by the current heuristic once the earlier i will fetch shape matches. If the intent is “ask the user for missing info”, a version ending as a question, e.g. ... could you provide your ID?, would not match this fast-path because of the ? check.
So yes — this is a real false-positive shape for the current regex.
🧠 Learnings used
Learnt from: sami-marreed
Repo: cuga-project/cuga-agent PR: 326
File: src/system_tests/load/tests/test_isolation.py:61-61
Timestamp: 2026-06-17T20:57:37.760Z
Learning: In this repository, Ruff rule RUF059 (unused unpacked variable) is intentionally not enabled. When reviewing Python code, do not flag issues of the form “unused unpacked variables” matching RUF059 (e.g., unused variables from tuple unpacking such as `error`). This aligns with the project’s Ruff configuration where `ruff check` succeeds without reporting these cases.
There was a problem hiding this comment.
Will this enter endless loop because of this?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n "classify_nl_auto_continue" -C5 --type=pyLength of output: 4372
🏁 Script executed:
#!/bin/bash
rg -n "classify_nl_auto_continue\|nl_auto_continue\|step_limit\|max_steps\|require_tool_call_before_final" -C5 --type py 2>/dev/null
rg -rn "classify_nl_auto_continue" -C8 .
echo "---"
rg -n "nl_auto_continue" -C6 src/cuga/backend/cuga_graph/nodes/cuga_lite/Length of output: 579
🏁 Script executed:
#!/bin/bash
set -x
fd nl_auto_continue_classifier
echo "---calls---"
grep -rn "classify_nl_auto_continue" --include=*.py .
echo "---step limit---"
grep -rn "step_limit\|max_steps\|MAX_STEPS\|step_count" --include=*.py src/cuga/backend/cuga_graph/nodes/cuga_lite/ | head -50Length of output: 6696
🏁 Script executed:
#!/bin/bash
sed -n '140,260' src/cuga/backend/cuga_graph/nodes/cuga_lite/adapter/sandbox_node.py
echo "===graph_adapter==="
sed -n '150,200' src/cuga/backend/cuga_graph/nodes/cuga_lite/adapter/graph_adapter.py
echo "===cuga_lite_graph append_with_step_limit==="
sed -n '130,200' src/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.pyLength of output: 855
🏁 Script executed:
#!/bin/bash
awk 'NR>=140 && NR<=260' src/cuga/backend/cuga_graph/nodes/cuga_lite/adapter/sandbox_node.py
echo "===graph_adapter==="
awk 'NR>=150 && NR<=200' src/cuga/backend/cuga_graph/nodes/cuga_lite/adapter/graph_adapter.py
echo "===cuga_lite_graph==="
awk 'NR>=100 && NR<=200' src/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.pyLength of output: 11542
🏁 Script executed:
#!/bin/bash
grep -n "cuga_lite_max_steps" -r src/cuga/config.py src/cuga/settings.tomlLength of output: 356
@sami-marreed No — it won't hang forever, but it also isn't fully safe from wasting steps.
The auto-continue loop is bounded by the graph's step counter, not by this classifier. Here's the chain:
classify_nl_auto_continueis invoked fromAgentGraphAdapter.classify_auto_continueinsrc/cuga/backend/cuga_graph/nodes/cuga_lite/adapter/graph_adapter.py.- Every continue cycle (call_model + sandbox) increments
state.step_countthroughappend_chat_messages_with_step_limitinsrc/cuga/backend/cuga_graph/nodes/cuga_lite/cuga_lite_graph.py. - Once
step_countexceedssettings.advanced_features.cuga_lite_max_steps(default70, set insrc/cuga/settings.tomlLine 51),core_create_error_commandreturns aCommandtoENDwith an error message — the graph terminates.
So in the worst case where the false-positive fast-path keeps matching turn after turn (e.g., the model keeps repeating a variant of "Ok I will fetch the information, but first I require your ID"), the run doesn't hang indefinitely — it stops at the step limit (default 70 iterations) and surfaces an error. That's a real cost (burning most of the step budget with a stalled conversation instead of finalizing quickly on turn 1), but not an unbounded/endless loop.
Given the false-positive you identified earlier, it may still be worth tightening _PLANNING_INTENT_RE/looks_like_planning_text (e.g., bail out when a clause like "but first I require/need ..." follows), so genuine "I need info from you" turns finalize immediately rather than consuming step budget before hitting the cap.
🧬 Code Graph Analysis Results
Relevant snippets
src/cuga/backend/cuga_graph/nodes/cuga_lite/nl_auto_continue_classifier.py
Line 39-53 (comment about looping / step limit)
# This path is intentionally conservative: it only flips False -> True for short
# text that opens with a first-person intent ("we"/"I"/"let's"/"let me"),
# optionally behind a discourse marker, followed by a forward-looking action or
# modal verb. A genuine final answer rarely matches, and the surrounding graph
# already enforces a step limit before auto-continuing, so an over-fire cannot
# loop forever.Line 83-101 (looks_like_planning_text)
def looks_like_planning_text(visible: str) -> bool:
"""True for a short first-person intent statement that signals more work to come.
Conservative deterministic detector for the planning-text stall. Returns
False for empty text, anything longer than a couple of sentences, text
that reads as a question (clarifying questions should finalize, not loop),
or text that addresses the user in the second person (it may be requesting
input the user must supply).
"""
t = (visible or "").strip()
if not t or len(t) > _PLANNING_MAX_LEN:
return False
if t.rstrip().endswith("?"):
return False
if _NEGATION_RE.search(t):
return False
if _SECOND_PERSON_RE.search(t):
return False
return bool(_PLANNING_INTENT_RE.match(t))Line 144-165 (parse_auto_continue_json)
def parse_auto_continue_json(raw: str) -> Optional[bool]:
t = (raw or "").strip()
if not t:
return None
if t.startswith("```"):
t = re.sub(r"^```(?:json)?\s*", "", t, flags=re.IGNORECASE).strip()
t = re.sub(r"\s*```\s*$", "", t).strip()
start, end = t.find("{"), t.rfind("}")
if start == -1 or end == -1 or end <= start:
return None
try:
obj = json.loads(t[start : end + 1])
except json.JSONDecodeError:
return None
if not isinstance(obj, dict):
return None
v = obj.get("auto_continue")
if isinstance(v, bool):
return v
if isinstance(v, str) and v.lower() in ("true", "false"):
return v.lower() == "true"
return NoneLine 168-206 (classify_nl_auto_continue)
async def classify_nl_auto_continue(
llm: BaseChatModel,
assistant_visible: Any,
reasoning_excerpt: Optional[Any],
) -> bool:
"""Return True if the graph should append a user ``continue`` message and re-invoke the coder model."""
if not getattr(settings.advanced_features, "cuga_lite_nl_auto_continue", True):
return False
visible = normalize_assistant_text(assistant_visible)
reasoning = normalize_assistant_text(reasoning_excerpt)
if looks_like_planning_text(visible):
logger.info("NL auto-continue: planning-text fast-path matched; auto-continuing")
return True
combined = build_combined_content_and_reasoning(visible, reasoning)
if not combined.strip():
return False
user_block = (
"Classify this assistant output (content + reasoning below).\n\n"
f"{combined}\n\n"
'Respond with JSON only: {"auto_continue": true} or {"auto_continue": false}'
)
try:
from cuga.backend.cuga_graph.utils.langfuse_tracing import get_langfuse_invoke_config
resp = await llm.ainvoke(
[
{"role": "system", "content": CLASSIFIER_SYSTEM_PROMPT},
{"role": "user", "content": user_block},
],
config=get_langfuse_invoke_config(),
)
parsed = parse_auto_continue_json(getattr(resp, "content", "") or "")
if parsed is None:
logger.warning("NL auto-continue classifier returned unparsable output; treating as finalize")
return False
return parsed
except Exception as e:
logger.warning(f"NL auto-continue classifier failed: {e}")
return FalseReview decision on PR #416: the guard enforces a proxy (a code block was emitted) rather than the real property (the answer is grounded in observed data), and breaks legitimate tool-free turns — 'what can you do?', skills without tools, follow-ups on previous threads — by looping them to the step limit. Superseded by #451, which tracks the underlying problem (answering from general LLM knowledge instead of tools) with general candidate designs and a configurable answer-source policy.
Address PR #416 review (Sami) on _unknown_tool_correction: - the call-shape check was a text search (\bname\s*\() that could match inside string literals like var = " agent_1(" — now only real ast.Call nodes count - a name the agent's own code defines (def / async def / class / assignment / import) keeps its bare NameError: calling a self-written helper before its definition used to get 'call find_tools' guidance, steering the agent away from the real fix - the no-close-match hint no longer assumes a tool was meant — it covers the lost-helper possibility (re-include the definition) alongside the find_tools re-query, with the lookalike-action caution retained - regression tests for all three behaviors
Address PR #416 review (Sami): 'Ok i will fetch the infromation, but first i require your ID' matched the planning fast-path and would auto-continue — answering the agent's request with a synthetic 'continue' instead of the user's reply. A planning statement describes the agent's own next actions, so any second-person text (you/your) now falls through to the LLM classifier. Also hardens CLASSIFIER_SYSTEM_PROMPT with the recorded decision examples (the Slack follow-up): interim plans -> continue; input requests, results, and clarifying questions -> finalize. The mode-aware redesign of this decision is tracked on #445.
haroldship
changed the title
coderabbitai
Bot
added
the
readability: good
Keep both PR unknown-tool correction tests and main's syntax-error guard tests in test_code_executor.py.
AppWorld results
|
Vakra results
Notes:
|
| r"explore|examine|check|investigate|figure\s+out|determine|" | ||
| r"retrieve|gather|list|enumerate)\b", | ||
| re.IGNORECASE, | ||
| ) |
There was a problem hiding this comment.
Will this enter endless loop because of this?
|
will merge after ido's |
3 participants
What this PR fixes
Three robustness fixes for CugaLite, described in plain words with real examples. Independent of issue #272.
1. The agent could stop mid-task right after stating a plan
Problem. Sometimes the model replies with a short plan instead of code — and the run treats that plan as the final answer.
Example (observed). The agent replies:
…and stops. That sentence is returned to the user as the answer; the search it announced never happens.
Fix. Planning statements like this are now recognized (a deterministic check for the observed patterns, plus example-guided hardening of the existing auto-continue LLM classifier), and the agent keeps working instead of stopping.
The check is careful about the opposite mistake: a plan that also asks the user for something —
— is not auto-continued, because the user has to reply. (Second-person guard, added after review.)
The broader redesign of this decision (autonomous vs interactive, deferral handling) is tracked on #445.
2. Made-up tool names caused silent retry loops
Problem. Models sometimes invent plausible-looking tool names instead of using the exact names
find_toolsreturned. The resultingNameErrorcarried no guidance, so the agent kept inventing variants until the step limit.Example (observed, gpt-oss). The agent calls
mondial_geo_get_countries_countries_get(...)— a name it constructed. Before: bareNameError, repeated re-fabrication, task dies at the step limit. Now the error says:and the agent recovers in one step.
Precision (after review). The check is AST-based: a name inside a string like
var = " agent_1("can't trigger it, and a helper function the agent wrote itself keeps its plainNameError(the right fix there is repairing the helper, not hunting for a tool). When no similar tool exists, the message covers both possibilities instead of assuming a tool was meant.Tool-name hallucination itself is tracked in #312.
3. Suspicious tool arguments were invisible
Problem. The sandbox calls tools directly, bypassing schema validation — so a malformed argument reaches the registry with no earlier signal.
Example. The agent passes
amount="5"(a string) where the tool wants a number. Now the log shows:Fix. Detection-only: the warning logs the suspect shape and always forwards the argument unchanged. Mining of past eval runs showed these shapes don't currently cause agent-visible failures, so auto-fixing was deliberately rejected. Examples and evidence: #452.
Removed during review: require-tool-call-before-final guard
An earlier commit added an optional guard that refused a final answer until at least one code block had executed (aimed at "confident answer with zero tool calls"). Review found it enforces the wrong thing:
what can you do?, skills-without-tools, and follow-up questions would loop to the step limit, and any trivial code block (x = 1) satisfied it. It has been removed from this PR; the underlying problem — the agent answering from general LLM knowledge instead of tools — is now tracked comprehensively in #451, including a configurable answer-source policy.Testing
test_nl_auto_continue_classifier.py,test_shared_call_model.py,test_code_executor.py,test_arg_warning.py— all pass.cuga_lite+cuga_agent_coresuites: 414 passed; the only failures are 5 pre-existing ones unrelated to this PR (3 e2b-package-not-installed, 2workspace_fs), verified identical with this PR's changes stashed.ruff checkandruff formatclean.Summary by CodeRabbit
New Features
Bug Fixes