Extend error context refactoring to remaining privileged tools - #775
Extend error context refactoring to remaining privileged tools#775DaliborKr wants to merge 3 commits into
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. Exception messages bypass redaction
|
![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 |
- 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>
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?
| 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?
| ) | ||
| _, 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.
|
I submitted one comment to thing that stood out a bit to me. Otherwise good. |
4 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.