Extend error context refactoring to remaining privileged tools - #775
Conversation
PR Summary by QodoStandardize error context across remaining privileged tools
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1.
|
![qodo-for-packit[bot]](https://avatars.githubusercontent.com/in/4066991?s=60&v=4){kind=small}
94aa49c to
5bd9aed
Compare
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit 5bd9aed |
I think this second example demonstrates the downside of this approach - |
Agree, I hadn't considered this risk. I have reviewed the current usage, and I realized that I can address this by replacing the For the |
Yes, but is there a reason not to include all Perhaps it would be best to be explicit and introduce e.g. a |
Yes, the main reason is that in many cases the But you're right. Your suggestion of |
5bd9aed to
6655111
Compare
| except OgrException as ex: | ||
| logger.warning(f"Failed to get branches for package {package}: {ex}") | ||
| raise ToolError(f"Failed to get branches for package {package}: {ex}") from ex |
There was a problem hiding this comment.
Why are we dropping handling of OrgException without including it in include_exception_message_for?
There was a problem hiding this comment.
You are right, I was too strict here. The OgrException message is beneficial for LLM. Since, I dropped include_exception_message_for completely and now the new explain_to_llm() only renders FrameworkError messages, the raw OgrException would lose its message. So I re-raise it as a ToolError (redacted) so it reaches the LLM.
f4910b1#diff-2692556a0912053b5dffc50c955e51f1893b9bb944968e563f433fb1be7c3e10R601-R604
| dist_git_branch = tool_input.dist_git_branch | ||
| jira_issue = tool_input.jira_issue | ||
| try: | ||
| with tool_error_context("Failed to initialize Kerberos ticket"): |
There was a problem hiding this comment.
Shouldn't KerberosError messages be included as well?
There was a problem hiding this comment.
I thought, that at this point, when the tool reaches the KerberosError, there is nothing the agent can act on, since it has nothing to do about wrong Kerberos configuration. Therefore I kept only the generic message for the LLM. Full exception is still in the cause and additional_context, available in logs and observability.
Correct me if, I am wrong.
| ) | ||
| _, stderr = await asyncio.wait_for(active_proc.communicate(), timeout=timeout) | ||
| if active_proc.returncode != 0: | ||
| raise RuntimeError(f"SCP failed: {stderr.decode().strip()}") |
There was a problem hiding this comment.
Should we not propagate these RuntimeErrors as well? Perhaps agent can benefit from them with faster recovery if it mistakenly attempts to create directory structure on path where file is located.
There was a problem hiding this comment.
Agreed, this one is useful for the agent. Because of my latest changes, a plain RuntimeError would not be forwarded to LLM as is (see my response to @opohorel's review), so I changed both to ToolError (redacted) so the stderr reaches the LLM.
|
I submitted one comment to thing that stood out a bit to me. Otherwise good. |
|
overall I like the idea. there is one finding from Codex, which pointed out an issue we already have in the repo: Codex identified a potential information leak in tool_error_context while validating this PR’s selective-forwarding contract. The underlying behavior predates this PR, but this change expands its use across additional privileged tools and now documents that unexpected exception details remain hidden from the LLM. Passing the original exception as cause=e causes BeeAI 0.1.83’s error.explain() to render the raw cause chain. Although ReasoningAgent removes the redacted additional_context before formatting the tool error, it does not remove the cause. The resulting text is then sent to the LLM in a ToolMessage. This was reproduced with a credential that appeared as [REDACTED] in additional_context but remained visible in the rendered RuntimeError cause. Consequently, the distinction introduced by include_exception_message_for is ineffective: exceptions not selected for forwarding can still expose their complete messages through the cause chain. Please provide an LLM-safe error formatter that excludes both observability context and the raw cause, and add a regression test covering the actual ReasoningAgent formatting path. |
6655111 to
f4910b1
Compare
Thanks, this was the core issue, and I reworked the whole approach around it.
This also resolves the conversation I had with @nforro regarding the There is a new separate commit with all of these changes so you can check it out. |
|
/agentic_review |
|
Code review by qodo was updated up to the latest commit f4910b1 |
f4910b1 to
27d34ef
Compare
nforro
left a comment
nforro
left a comment
There was a problem hiding this comment.
Nice! There is one issue though:
ymir/tools/base.py:39 catches BeeAI's AbortError and converts it to a retryable ToolErrorWithContext. AbortError inherits both FrameworkError and asyncio.CancelledError, but its MRO places Exception first, so except Exception catches it. This affects all newly wrapped operations and turns cancellation/timeouts into normal tool failures, allowing the agent to continue or retry after cancellation. Add except asyncio.CancelledError: raise before the broad handler and test with BeeAI AbortError.
27d34ef to
1caab34
Compare
I tested this case and you were right. The solution you proposed solves the problem, so it has been fixed. |
- Add parameter to selectively append exception messages to the LLM-facing error for specific exception types - Extract make_additional_context() helper for reuse outside tool_error_context - Add docstring into tool_error_context() Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Dalibor Kricka <dalidalk@seznam.cz>
- Add additional_context kwargs to existing tool_error_context calls for better observability - Use argument include_exception_message_for in the tool_error_context() wrapper where exception detail is useful for the LLM - Use ToolErrorWithContext directly for known error conditions - Replace remaining manual try/except ToolError patterns Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Dalibor Kricka <dalidalk@seznam.cz>
- Replace manual try/except ToolError patterns with tool_error_context() - Add additional_context kwargs for observability - Use include_exception_message_for where exception detail is useful for the LLM Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Dalibor Kricka <dalidalk@seznam.cz>
- Remove include_exception_message_for parameter from tool_error_context() in favor of explain_to_llm() which renders only the FrameworkError exceptions chain - Add explain_to_llm() and _format_error_message_for_llm() to errors.py, exposing only error type, message and allowlisted context keys to the LLM - Log the LLM-facing error message in _runner.py before it is rendered - Convert RuntimeError raises in testing_farm to ToolError and apply redact_credentials() to subprocess stderr on the LLM-facing path - Convert OgrException in gitlab to ToolError with redact_credentials() to safely surface library errors to the LLM Signed-off-by: Dalibor Kricka <dalidalk@seznam.cz>
1caab34 to
589f1f7
Compare
5 participants
Summary
Follow-up to #727 that extends the
tool_error_contextapproach to the remaining privileged tools (copr,gitlab,lookaside) and enhances the base infrastructure.tool_error_context()withinclude_exception_message_forparameter for selective exception message forwarding to the LLMadditional_contextarguments to existingtool_error_context()calls in tools already refactored in Enable enhancing tool exception context #727try/except ToolErrorpatterns withtool_error_contextincopr,gitlabandlookasidetoolsFeedback welcome on
include_exception_message_for:This PR adds an optional
include_exception_message_forparameter totool_error_context(). It allows selectively appending exception messages of specified types to the LLM-facingerror_message, while all other exceptions remain hidden behind the generic message (with details going only to observability).Here are two useful example scenarios from the code:
1.
ymir/tools/privileged/gitlab.pyToolErrormessages from within a wrapped block. For example, inForkRepositoryTool, multiple validation checks raiseToolErrorwith specific messages the LLM should see:"Failed to fork repository: Unexpected git forge, expected gitlab.com/redhat"instead of just"Failed to fork repository", while unexpected exceptions (e.g. from theget_projectcall) still produce only the generic message.2.
ymir/tools/privileged/copr.pyDownloadArtifactsTool, aValueErroris raised with HTTP status info that helps the LLM understand what went wrong:"Failed to download build artifact: 404 Not Found". Again, the status detail is useful context, while any other unexpected exception stays hidden behind the generic message.I realize this adds complexity to what is otherwise a straightforward wrapper. The alternative would be catching and re-raising as
ToolErrorWithContextmanually at each call site, but that defeats the purpose of the context manager. If you see a cleaner approach, I'm open to suggestions.