Enable enhancing tool exception context - #727
Open
DaliborKr wants to merge 2 commits into
Open
Conversation
Signed-off-by: Nikola Forró <nforro@redhat.com>
- Add tool_error_context context manager with automatic exception capture - Propagate error context through MCP via beeai-framework patch - Filter additional_context from LLM error messages in _runner.py - Refactor first batch of privileged tools to use tool_error_context Co-authored-by: Nikola Forró <nforro@redhat.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Dalibor Kricka <dalidalk@seznam.cz>
PR Summary by QodoAdd structured tool error context with observability-only details
AI Description
Diagram
High-Level Assessment
Files changed (16)
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
7 rules 1. Unredacted context leaks secrets
|
![qodo-for-packit[bot]](https://avatars.githubusercontent.com/in/4066991?s=60&v=4){kind=small}
Comment on lines
+249
to
+251
| if info.flags & git.remote.PushInfo.ERROR: | ||
| logger.error("Push to dist-git rejected: %s", info.summary.strip()) | ||
| raise ToolError("Push to dist-git was rejected") |
There was a problem hiding this comment.
1. distgit tests not updated 📘 Rule violation ▣ Testability
CreateZstreamBranchTool now raises ToolError("Push to dist-git was rejected"), but the unit test
still asserts the old error text (match="Push rejected"), indicating tests were not updated to
cover the changed behavior in privileged tools.
Agent Prompt
## Issue description
Privileged tool behavior in `ymir/tools/privileged/distgit.py` changed, but unit tests were not updated to assert the new error behavior/message.
## Issue Context
Compliance requires unit tests to be added/updated when modifying privileged tools under `ymir/tools/privileged/`, especially for error paths.
## Fix Focus Areas
- ymir/tools/privileged/distgit.py[246-251]
- ymir/tools/privileged/tests/unit/test_distgit.py[110-156]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+106
to
+110
| logger.error(f"Tool {meta.creator} additional context: {additional_context}") | ||
| span = trace_api.get_current_span() | ||
| if span.is_recording(): | ||
| for key, value in additional_context.items(): | ||
| span.set_attribute(f"metadata.{key}", str(value)) |
There was a problem hiding this comment.
3. Unredacted context leaks secrets 🐞 Bug ⛨ Security
_setup_tool_error_logging() logs tool additional_context verbatim and exports it as span attributes, but tool_error_context() also injects raw exception strings into that context. This can persist credentials/tokens into logs and OpenTelemetry traces (e.g., GitLab URLs containing GITLAB_TOKEN).
Agent Prompt
## Issue description
`ymir/agents/observability.py:_setup_tool_error_logging()` logs and exports `additional_context` without any credential redaction, and `ymir/tools/base.py:tool_error_context()` always adds `additional_context["exception"] = f"{type(e).__name__}: {e}"`.
This combination can emit secrets into logs and into OpenTelemetry span attributes (which are typically indexed/searchable), because exception strings and context values may contain tokens/credentials.
## Issue Context
The repo already has a shared `redact_credentials()` helper used by MCP gateways, but the new agent-side observability hook bypasses it. Also, some tools construct credential-bearing URLs (e.g., `https://oauth2:*******@gitlab.com/...`) inside `tool_error_context()`, so any exception that includes the URL/argv can leak tokens via the captured exception string.
## Fix Focus Areas
- ymir/agents/observability.py[93-112]
- ymir/tools/base.py[13-22]
- ymir/tools/gateway_utils.py[16-61]
- ymir/tools/privileged/distgit.py[190-196]
## Suggested fix
1) In `_setup_tool_error_logging()`:
- Apply `redact_credentials()` to the string representation of `additional_context` before logging.
- When setting span attributes, redact values (and consider allowlisting keys) before `span.set_attribute(...)`.
2) In `tool_error_context()`:
- Avoid capturing raw exception messages verbatim, or redact them before storing (e.g., store only exception type, or a redacted/shortened message).
3) Add/adjust tests or a small unit-level check to ensure known token patterns are redacted in both logs and span attributes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+13
to
+18
| + error_context = getattr(e, "context", None) or {} | ||
| + meta = {"error_context": error_context} if error_context else None | ||
| + return MCPCallToolResult( | ||
| + content=[MCPTextContent(type="text", text=f"Error executing tool {tool.name}: {e}")], | ||
| + isError=True, | ||
| + _meta=meta, |
There was a problem hiding this comment.
4. Mcp error echoes exception 🐞 Bug ⛨ Security
beeai-framework-error-context.patch constructs the MCP tool error content using the raw exception
string ({e}), which becomes LLM-visible tool output. This can expose sensitive details
(tokens/URLs/paths) and undermines the PR goal of keeping model-facing errors brief and
non-sensitive.
Agent Prompt
## Issue description
The MCP boundary patch returns `MCPTextContent(... text=f"Error executing tool {tool.name}: {e}")`, directly interpolating the exception string into the tool result content.
Because this content is what the agent/model receives for tool failures, any sensitive data present in exception messages can be exposed to the LLM.
## Issue Context
Some tools construct credential-bearing URLs (e.g., `https://oauth2:*******@gitlab.com/...`) or may raise exceptions containing request/command details. Even if most tools are migrated to raise sanitized `ToolErrorWithContext`, this patch is a catch-all for *any* exception, including unwrapped ones.
## Fix Focus Areas
- beeai-framework-error-context.patch[10-19]
## Suggested fix
Update the patch so the MCP `content` uses a safe, fixed message (or a sanitized/short error message) and does **not** include `str(e)`.
Example approach inside the patched `except`:
- Set `text=f"Error executing tool {tool.name}"` (optionally include only `type(e).__name__`).
- Keep detailed debugging context in `_meta` / `error_context` only (and ensure that context values are safe/serializable/redacted as needed).
This preserves debuggability via observability while preventing sensitive exception strings from reaching the LLM.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
DaliborKr
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
tool_error_contextcontext manager that automatically captures exception type and message alongside tool-specific context (URLs, parameters, etc.)beeai-framework-error-context.patch(pending upstream PR - by @nforro)additional_contextfrom LLM error messages in_runner.pyto prevent context pollutiontool_error_context: jira, errata, distgit, testing_farm, maintainer_rules, zstream_searchThis is the first batch of tool refactoring — remaining tools will be migrated based on feedback. The context fields provided per tool are not definitive and can be extended or reduced over time as we learn what's most useful for debugging.
How it works
The first argument is a brief error message visible to the LLM. The keyword arguments are additional context visible only in logs and observability tools. See this dummy example in 'GetJiraDetailsTool':
Trace server example (@nforro plans to highlight the additional context in the Trace Server later):
Phoenix example: