fix: stop silently swallowing exceptions in forward_incremental#802
fix: stop silently swallowing exceptions in forward_incremental#802dashitongzhi wants to merge 3 commits into
Conversation
The retry loop in forward_incremental was catching all exceptions with a bare 'except Exception: pass', making it impossible to diagnose failures. This collects error messages, logs them when verbose mode is on, and reports failures to the tracer - matching the behavior of forward().
|
PR author is not in the allowed authors list. |
WalkthroughThe Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/notte-browser/src/notte_browser/tagging/action/llm_taging/base.py (2)
112-113: ⚡ Quick win
last_erroris assigned but never used after the retry loop.Unlike
forward(), which passeslast_erroras thefromcause inraise LLMParsingError(...) from last_error,forward_incrementalonly returns a fallback and never referenceslast_erroragain. The variable is dead code.🧹 Proposed fix — remove the unused variable
- errors: list[str] = [] - last_error: Exception | None = None + errors: list[str] = []except Exception as e: - last_error = e - errors.append(str(e)) + errors.append(str(e))🤖 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 `@packages/notte-browser/src/notte_browser/tagging/action/llm_taging/base.py` around lines 112 - 113, The variable last_error in the retry setup is declared but never used in forward_incremental (unlike forward which uses it in raise LLMParsingError from last_error); remove the dead variable to clean up the code: delete the declaration "last_error: Exception | None = None" and any subsequent unused references in the forward_incremental logic (leave the errors list and fallback behavior intact), ensuring only meaningful error aggregation (errors list) remains and behavior of forward_incremental is unchanged.
117-121: ⚡ Quick winMissing
ContextSizeTooLargeErrorearly-exit thatforward()has.
forward()short-circuits on context-too-large errors to avoid pointless retries.forward_incrementalwill retry the same callmax_triestimes for this error before falling back — wasting LLM tokens and time with zero chance of success.🛡️ Proposed fix — mirror the early-exit from `forward()`
except Exception as e: - last_error = e errors.append(str(e)) + if "Please reduce the length of the messages or completions" in str(e): + pattern = r"Current length is (\d+) while limit is (\d+)" + match = re.search(pattern, str(e)) + size = int(match.group(1)) if match else None + max_size = int(match.group(2)) if match else None + raise ContextSizeTooLargeError(size=size, max_size=max_size) from e if self.verbose: logger.debug(...)🤖 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 `@packages/notte-browser/src/notte_browser/tagging/action/llm_taging/base.py` around lines 117 - 121, forward_incremental currently catches all exceptions and keeps retrying, but should mirror forward() by short-circuiting on ContextSizeTooLargeError to avoid pointless retries; update the except block in forward_incremental to detect ContextSizeTooLargeError (the same exception class used in forward()), set last_error/errors as before, and immediately raise or return early (i.e. no retry loop) when the caught exception is a ContextSizeTooLargeError, preserving verbose logging behavior.
🤖 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 `@packages/notte-browser/src/notte_browser/tagging/action/llm_taging/base.py`:
- Around line 112-113: The variable last_error in the retry setup is declared
but never used in forward_incremental (unlike forward which uses it in raise
LLMParsingError from last_error); remove the dead variable to clean up the code:
delete the declaration "last_error: Exception | None = None" and any subsequent
unused references in the forward_incremental logic (leave the errors list and
fallback behavior intact), ensuring only meaningful error aggregation (errors
list) remains and behavior of forward_incremental is unchanged.
- Around line 117-121: forward_incremental currently catches all exceptions and
keeps retrying, but should mirror forward() by short-circuiting on
ContextSizeTooLargeError to avoid pointless retries; update the except block in
forward_incremental to detect ContextSizeTooLargeError (the same exception class
used in forward()), set last_error/errors as before, and immediately raise or
return early (i.e. no retry loop) when the caught exception is a
ContextSizeTooLargeError, preserving verbose logging behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bd741858-1cfa-423a-8818-746005768e6c
📒 Files selected for processing (1)
packages/notte-browser/src/notte_browser/tagging/action/llm_taging/base.py
|
2027 auto-runs evals against preview deployments of your docs. To enable this, install one of:
Once a preview is deployed, open a new PR and we'll run the eval automatically. Evaluating agent experience using 2027.dev · View dashboard |
![mendral-app[bot]](https://avatars.githubusercontent.com/in/1975249?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Fixes F841 (unused variable) in forward_incremental and reformats base.py to pass pre-commit ruff hooks.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@packages/notte-browser/src/notte_browser/tagging/action/llm_taging/base.py`:
- Line 113: The variable _last_error is unused dead code; remove its declaration
and any assignments to it (the class-level declaration "_last_error: Exception |
None = None" and the update inside the forward_incremental implementation) so
that only live variables remain; search for "_last_error" and delete its
definition and any lines that set it within the method(s) (e.g.,
forward_incremental) and run linters/tests to confirm no references remain.
🪄 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: 74d0a1c1-8190-4396-9088-aa7d84beb318
📒 Files selected for processing (1)
packages/notte-browser/src/notte_browser/tagging/action/llm_taging/base.py
| previous_action_list: list[InteractionAction], | ||
| ) -> PossibleActionSpace: | ||
| errors: list[str] = [] | ||
| _last_error: Exception | None = None |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
_last_error is dead code — remove it entirely.
The variable is assigned on line 113 and updated on line 118, but never read. Unlike forward(), forward_incremental does not re-raise on exhaustion, so there is no consumer for this value. The underscore prefix suppresses the lint warning (F841) but the assignment itself still adds noise.
♻️ Proposed fix
errors: list[str] = []
- _last_error: Exception | None = None
for _ in range(self.max_tries):
try:
return await self.pipe.forward_incremental(snapshot, previous_action_list)
except Exception as e:
- _last_error = e
errors.append(str(e))
if self.verbose:Also applies to: 118-118
🤖 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 `@packages/notte-browser/src/notte_browser/tagging/action/llm_taging/base.py`
at line 113, The variable _last_error is unused dead code; remove its
declaration and any assignments to it (the class-level declaration "_last_error:
Exception | None = None" and the update inside the forward_incremental
implementation) so that only live variables remain; search for "_last_error" and
delete its definition and any lines that set it within the method(s) (e.g.,
forward_incremental) and run linters/tests to confirm no references remain.
There was a problem hiding this comment.
LGTM
Previous feedback was addressed: last_error was renamed to _last_error, resolving the F841 linter error. The CI failure (Run unit tests) is caused by integration tests for personas/vault/sessions that depend on external API limits — unrelated to this PR's changes, and consistent with known flaky patterns in this repo. The code changes themselves are correct.
Tag @mendral-app with feedback or questions. View session
Summary
The retry loop in
forward_incrementalwas catching all exceptions with a bareexcept Exception: pass, silently discarding all error information. This made it impossible to diagnose why incremental action listing was failing.Changes
In
RetryPipeWrapper.forward_incremental():errorslist (matching the pattern inforward())self.tracer.trace()so they appear in telemetry/debug outputBefore
After
This matches the error handling pattern already used in the sibling
forward()method.Summary by CodeRabbit
Note
Fixes silent exception swallowing in
forward_incrementalby collecting errors, logging them in verbose mode, and reporting failures to the tracer — matching the pattern already used inforward(). A follow-up commit prefixeslast_errorwith_to resolve the ruff F841 warning and reformats the file.Written by Mendral for commit c238a3a.