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
2 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.