Uh oh!
There was an error while loading. Please reload this page.
feat(providers): add runtime.tool_output limits for oversized MCP tool results - #313
Conversation
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
Thanks for the contribution. I reviewed and have some inline comments. Thanks!
| """ | ||
| try: | ||
| spill_dir_str = self._tool_output.spill_dir | ||
| if spill_dir_str: |
There was a problem hiding this comment.
This walks the raw, unresolved spill_dir for symlinks, but /tmp and /var/tmp are themselves symlinks on macOS (to /private/tmp and /private/var/tmp). So a totally ordinary spill_dir: /tmp/my-dir trips this check and the feature quietly falls back to a pathless marker on every call, on the exact platform most of us are developing on.
The default-dir branch below doesn't have this problem because it resolves before walking ancestors. Same fix should work here.
| ifspill_dir_str: | |
| ifspill_dir_str: | |
| spill_dir=Path(spill_dir_str).resolve() | |
| if_contains_symlink(spill_dir, stop_at=spill_dir.parent.parentifspill_dir.parent!=spill_direlseNone): |
There was a problem hiding this comment.
Fixed, thanks. The explicit spill_dir branch now resolves the path and applies a symlink policy: a symlink is only rejected when the resolved path lands outside the system temp root. That keeps the macOS layout working (/tmp and /var/tmp are themselves symlinks into /private/...) while still refusing a symlink that would redirect spilled output to an attacker-chosen location elsewhere. I kept the check on the raw path for the outside-temp case so the security guarantees the existing tests pin (symlinked leaf, symlink mid-path) still hold. Covered by test_explicit_spill_dir_under_symlinked_parent_is_allowed plus the updated symlink-rejection tests.
| if not response_text and result.structuredContent: | ||
| response_text = str(result.structuredContent) | ||
| response_text = self._maybe_truncate_response( |
There was a problem hiding this comment.
_maybe_truncate_response/_spill_full_output are documented as best-effort and never-raise, but this call sits inside call_tool's outer except Exception. If a bug ever slips into the truncation path, it gets caught here, logged as "MCP tool call failed", and re-raised as RuntimeError — even though the tool call itself already succeeded. Whoever debugs that later is going to go looking at the wrong server.
| response_text=self._maybe_truncate_response( | |
| try: | |
| response_text=self._maybe_truncate_response( | |
| response_text, | |
| server_name=server_name, | |
| original_name=original_name, | |
| ) | |
| exceptExceptionastruncation_err: | |
| logger.warning( | |
| "Failed to apply output truncation for %s; returning untruncated result: %s", | |
| prefixed_name, | |
| truncation_err, | |
| ) |
There was a problem hiding this comment.
Good catch. _maybe_truncate_response now runs in its own try/except inside call_tool, separate from the tool call's outer handler. A failure in the best-effort truncation path is logged as a warning (Failed to apply output truncation for ...) and the untruncated result is returned, instead of being re-raised as RuntimeError("MCP tool call failed") and pointing whoever debugs it at the wrong server. Covered by test_truncation_failure_does_not_fail_successful_tool_call.
| created and truncation is applied in-place. | ||
| """ | ||
| spill_dir: str | None = None |
There was a problem hiding this comment.
No normalization on this field, and the two consumers don't agree on what an empty string means: manager.py treats "" as falsy and falls back to the default temp dir, copilot.py checks is not None and forwards output_directory="" straight to the SDK. Same config value, two different behaviors depending on which provider you're running. A validator that turns "" into None here fixes both call sites at once.
| spill_dir: str|None=None | |
| spill_dir: str|None=None | |
| @field_validator("spill_dir") | |
| @classmethod | |
| def_normalize_spill_dir(cls, v: str|None) ->str|None: | |
| returnv.strip() orNoneifvisnotNoneelseNone |
There was a problem hiding this comment.
Done. ToolOutputConfig.spill_dir now has a field_validator that normalizes empty/whitespace strings to None, so both consumers see the same value: the MCP manager falls back to the default temp dir and the Copilot provider no longer forwards output_directory="" to the SDK. Covered by test_spill_dir_empty_string_normalizes_to_none.
| from conductor.config.schema import AgentDef, OutputField, ToolOutputConfig | ||
| from conductor.exceptions import ProviderError, ValidationError | ||
| from conductor.executor.output import validate_output | ||
| from conductor.mcp.manager import ( |
There was a problem hiding this comment.
manager.py's own docstring says the marker is "generated entirely inside this manager," and then this file imports four underscore-prefixed constants from it directly. Nothing in ruff or ty will catch someone renaming these during a manager.py refactor, since private-name imports aren't a flagged contract violation, so this can break silently. Promoting _FS_HINT, _GENERIC_HINT, _TAIL_WINDOW, and _TRUNCATION_MARKER_PREFIX to public names (or their own small module) would make the dependency explicit instead of accidental.
There was a problem hiding this comment.
Agreed — promoted the four constants to public names (TRUNCATION_MARKER_PREFIX, GENERIC_HINT, FS_HINT, TAIL_WINDOW) since they are the shared contract with ClaudeProvider's marker parser, and updated the import here (plus the two test files that referenced them) to match. The dependency is now explicit rather than a private-name import that a manager-side rename could silently break.
| logger.debug("auto-approved permission request: %s", request) | ||
| return PermissionHandler.approve_all(request, invocation) | ||
| def _large_output_config(self) -> dict[str, Any] | None: |
There was a problem hiding this comment.
Typed -> dict[str, Any] | None, and all three call sites guard with if large_output_cfg is not None:, but every branch here returns a dict — there's no None path anywhere in the method. Looks like a leftover from an earlier version where None meant "skip this SDK kwarg entirely." Either drop the | None and the three now-dead checks, or bring back whatever case was supposed to return None.
There was a problem hiding this comment.
Right, it was a leftover. Dropped the | None from the return type and removed the three now-dead is not None guards at the call sites — every branch returns a dict, so large_output is now forwarded unconditionally.
| directory. | ||
| """ | ||
| if not self._tool_output_config.enabled: | ||
| return {"enabled": False} |
There was a problem hiding this comment.
Worth calling out how different this is from Claude's behavior: on Claude, spill_to_file=False still truncates to max_chars, it just skips the file write. Here it disables output-size limiting entirely, because the SDK has no truncate-without-spill mode. Someone sets this to keep tool output off disk and ends up with unlimited tool output instead, with nothing at runtime telling them that happened — it's only in the docstring. A log line when this branch fires would close that gap cheaply.
There was a problem hiding this comment.
Good point — added a logger.warning when this branch fires, spelling out that spill_to_file=False disables tool output size limiting entirely on Copilot (the SDK has no truncate-without-spill mode) and that output will not be capped at max_chars, unlike Claude which still truncates in place. Also clarified the spill_to_file field description in docs/mcp-tools.md to spell out the provider difference. Covered by test_spill_to_file_false_logs_a_warning.
| if not tools: | ||
| return False | ||
| fs_names = ( |
There was a problem hiding this comment.
Substring containment, not word boundaries, so any tool name that happens to contain "ls" or "file" trips this and gets the filesystem-hint rewrite whether or not it can actually read a file off disk. Telling the model to go use "your filesystem tools" on a false positive just sends it down a dead end. re.search(r"\b(read|grep|glob|bash|shell|ls|find|view|edit)\b", lower) would tighten this without much extra code.
There was a problem hiding this comment.
Tightened. _has_fs_like_tool now splits the tool name on non-alphanumeric characters and requires a keyword to equal a whole segment, so translate/fileupload no longer match. I also narrowed the keyword set to read-by-path capability (read, grep, view, cat, open, load, file, bash, shell) and dropped the search/write keywords (find, ls, glob, edit) — the agent already has the exact spill path from the marker, so it only needs to read/grep by path, and hinting at search or edit tools would send it down the same kind of dead end you flagged. Calibrated the set against real MCP server tool names (@modelcontextprotocol/server-filesystem, git, github, bash servers) so common readers like read_file, read_multiple_files, view_code and bash still match.
| | **Agentic Loop** | SDK-managed | Manual (provider code) | SDK-managed (delegated to CLI) | SDK-managed (delegated to hermes) | | ||
| | **Structured Output** | Prompt injection | Native | Prompt injection | Prompt injection | | ||
| | **Session Resume** | Yes | No | No | Yes | | ||
| | **Tool Output Limits** | native SDK spill+compaction | conductor-side truncation+spill | native CLI env var | N/A | |
There was a problem hiding this comment.
"native SDK spill+compaction" reads like tool-output limiting also triggers context compaction, but it doesn't — large_output (what this PR forwards) only handles spill-to-file, and compaction is a separate SDK mechanism for the session's context window that _large_output_config never touches. Worth splitting those apart so nobody assumes a connection that isn't there.
| |**Tool Output Limits**| native SDK spill+compaction| conductor-side truncation+spill | native CLI env var | N/A | | |
| |**Tool Output Limits**| native SDK spill (large_output)| conductor-side truncation+spill | native CLI env var | N/A | |
There was a problem hiding this comment.
Applied — split the two apart so the row now reads native SDK spill (large_output), making clear that what this PR forwards only handles spill-to-file and is unrelated to the SDK's separate context-window compaction.
…ize limits Introduce ToolOutputConfig (enabled, max_chars, spill_to_file, spill_dir) under RuntimeConfig.tool_output as a per-result cap for individual MCP tool results. Plumb the config through create_provider / ProviderFactory (test-compat path) and ProviderRegistry (engine runtime path) into ClaudeProvider and CopilotProvider, which store it as _tool_output_config for the follow-up truncation work. No behavior change yet — defaults preserve existing behavior (enabled, 50000 chars, spill to $TMPDIR/conductor/tool-output). Refs: .omo/plans/mcp-tool-output-limits.md (todo 1)
…tput MCPManager.call_tool now truncates each individual tool result that exceeds runtime.tool_output.max_chars and appends a single-line marker at the end of the kept prefix. The marker is generated entirely inside the manager with a constant generic hint; no placeholder or shared mutable state is used. When spill_to_file is enabled, the full result is written to a process-private file (dir mode 0o700, file mode 0o600, O_EXCL create, sanitized mcp-<server>-<tool>-<uuid8>.txt name) under spill_dir or $TMPDIR/conductor/tool-output; write failures degrade to a marker without a path. The Claude agentic loop detects truncation by the '[output truncated:' prefix in the trailing ~400 chars of the local result string and, when the resolved request_tools include filesystem-like tools, replaces the generic hint with a filesystem hint via exact string substitution. emit_output is not affected. Refs: .omo/plans/mcp-tool-output-limits.md (todo 2)
Add _large_output_config() helper and wire it into all three session creation sites (create_session, resume_session, dialog sessions). Semantics: - tool_output.enabled=False -> large_output key omitted (SDK defaults). - spill_to_file=False -> large_output.enabled=False, since the SDK has no truncate-without-spill mode. - max_chars is forwarded as max_size_bytes on a 1:1 basis; multibyte UTF-8 (CJK/emoji) may therefore be truncated earlier than max_chars characters. Documented in the helper docstring. - output_directory is only sent when spill_dir is explicitly set. Verified against github-copilot-sdk==1.0.1 which natively supports large_output in create_session/resume_session. No event is emitted for Copilot truncations (the SDK owns the tool loop; the truncation event remains Claude-only). Refs: .omo/plans/mcp-tool-output-limits.md (todo 3)
Document runtime.tool_output in docs/mcp-tools.md (new 'Tool output limits' section with all four fields, defaults, and per-provider behavior), docs/workflow-syntax.md (reference entry), and docs/providers/comparison.md (capability table row). Add examples/tool-output-limits.yaml demonstrating the config. Extend the Provider Support table in docs/mcp-tools.md with a 'Tool output limits' row covering Claude (conductor-side), Copilot (native SDK), claude-agent-sdk (native CLI MAX_MCP_OUTPUT_TOKENS), and Hermes (N/A). The docs state explicitly: the cap is per-result, not a cumulative context budget; Copilot forwards max_chars as bytes (multibyte UTF-8 may truncate earlier); the agent_tool_output_truncated event is Claude-only; spill files contain raw tool output and are not deleted by Conductor. Refs: .omo/plans/mcp-tool-output-limits.md (todo 6)
…JSONL log When an MCP tool result is truncated by MCPManager (todo 2), the Claude agentic loop now parses the truncation marker from the local result string (tail-window detection, no shared mutable state) and emits agent_tool_output_truncated with tool_name, original_chars, kept_chars, and spill_path. The emission is wrapped in try/except like neighboring event callbacks so a faulty callback cannot break the loop, and fires at most once per truncated call. ConsoleEventSubscriber gains a matching branch that logs a verbose warning with the tool name, char counts, and spill path (or an honest 'not spilled' note). The JSONL EventLogSubscriber writes all events generically, so no filter change was needed (event_log.py:161-174). The event is Claude-only: the Copilot SDK owns its tool loop and exposes no truncation hook. This asymmetry is documented in docs/mcp-tools.md. Refs: .omo/plans/mcp-tool-output-limits.md (todo 4)
Add the agent_tool_output_truncated event type to the dashboard frontend: EventType union entry and AgentToolOutputTruncatedData interface in types/events.ts, a store handler in workflow-store.ts that appends a 'truncated' activity entry (scissors icon, char counts, optional spill-file path) with for-each itemKey pass-through, and a matching case in buildActivityLogEntry. Rebuild the bundled assets via make build-frontend. Refs: .omo/plans/mcp-tool-output-limits.md (todo 5)
…ndings Address all validated Final Wave findings: - Copilot: forward enabled=False explicitly when tool_output is disabled; the SDK defaults large_output to enabled, so omitting the key silently ignored the user's config (copilot.py, session.py:979 contract). - Claude: only rewrite the truncation hint to the filesystem variant when the marker actually advertises a spill file, so the model is never told to read a file that does not exist. - MCPManager: resolve spill_dir to an absolute path (relative configs previously produced relative marker paths unreachable from agents with a different working_dir); refuse symlink spill dirs; harden pre-existing loose dirs to 0o700 or degrade to no-spill; remove the double-close on fd write error and clean up partial spill files. - Claude: widen the marker tail window to 2000 chars and anchor the parse regex on either hint variant with a closing-bracket anchor, so long spill paths and paths containing '. ' parse correctly and parsing still works after the hint rewrite. - Tests: update integration serialization assertions for the new tool_output default; add regression coverage for all of the above (symlink rejection, chmod hardening, fdopen failure, no-path hint gating, long/dot-space paths, fs-hint parse after rewrite, Copilot enabled=False forwarding). - Example: remove trailing whitespace. Refs: .omo/plans/mcp-tool-output-limits.md (final wave fixes)
Address the second round of code-review findings on the spill/truncation path: - Close an fd leak when os.fdopen itself raises after os.open succeeded; the raw fd is now closed exactly once and the partial file removed. - Write spill files with explicit UTF-8 encoding and treat any write-side failure (including UnicodeEncodeError, which is not an OSError) as a graceful no-spill: partial file removed, warning logged, tool call does not crash. - Reject spill directories containing a symlink anywhere in the path, not just at the leaf. A new _contains_symlink helper walks path components with a stop_at anchor for the default <tmp>/conductor/tool-output path so a planted ancestor symlink is refused while a legitimately symlinked system temp root is not falsely rejected. - Gate the filesystem hint on the parsed marker's spill_path instead of a raw 'full output saved to:' substring scan of the tail window, which false-positived when the truncated payload itself contained that text. - Force the truncation-marker regex to match the rightmost real marker by prefixing a greedy '.*', so marker-like text inside the payload cannot spoof the parsed original/kept/spill_path metadata. Regression tests cover each case (fd leak, UTF-8 round-trip, Unicode cleanup, ancestor symlink rejection for both default and explicit dirs, payload-literal false positive, last-marker spoof). Refs: .omo/plans/mcp-tool-output-limits.md (final wave round 2)
Round 3 of code-review findings, scoped to correctness (theoretical TOCTOU race-hardening, the pre-existing '__' server-name split, and the documented fs-tool heuristic were triaged out-of-scope per the Copilot SDK parity rule D15): - _spill_full_output is now a strict never-raise contract: the entire body (symlink check, resolve, mkdir, chmod, open, write) runs inside a single try catching (OSError, ValueError), so an invalid path (NUL byte, ENAMETOOLONG) degrades to a warning + no-path marker instead of crashing the tool call. - Widen the truncation-marker tail window from 2000 to 8192 chars so a valid long POSIX spill path (PATH_MAX ~4096 + marker overhead) is never cut off; previously such a marker parsed to None and silently skipped the truncation event and the filesystem hint. - Replace the generic hint only in the trailing marker (rightmost occurrence via rfind + slice) instead of a global str.replace that corrupted any identical text inside the truncated payload. Regression tests cover NUL/over-long path graceful no-spill, a ~4000-char spill path parsing + emitting + hint rewrite, and payload-literal preservation when the payload contains the generic-hint text. Refs: .omo/plans/mcp-tool-output-limits.md (final wave round 3)
Address code review on MCP tool output truncation: - Resolve an explicit spill_dir before the symlink check and only reject a symlink that resolves outside the system temp root. On macOS /tmp and /var/tmp are themselves symlinks into /private/..., so the previous raw path walk rejected every ordinary spill dir under them and silently disabled the spill on that platform. - Isolate _maybe_truncate_response in its own try/except so a bug in the best-effort truncation path can no longer masquerade as a tool failure and discard an otherwise successful tool result. - Promote the truncation marker constants (TRUNCATION_MARKER_PREFIX, GENERIC_HINT, FS_HINT, TAIL_WINDOW) to public names; they are the shared contract with ClaudeProvider's marker parser, not private to the manager.
Address code review on the filesystem-hint rewrite: - Import the now-public truncation marker constants from the MCP manager instead of underscore-prefixed private names, so a manager-side rename can no longer silently break the marker parser. - Detect filesystem-like tools by splitting the tool name on non-alphanumeric characters and requiring a keyword to equal a whole segment, instead of whole-name substring containment. This stops false positives such as "translate" or "fileupload" (which merely contain "ls"/"file") from triggering a hint that advertises filesystem tools the agent does not have. - Restrict the keyword set to read-by-path capability (read, grep, view, cat, open, load, file, bash, shell). The agent already has the exact spill path from the marker, so search tools (find/ls/glob) and write tools (edit) no longer qualify. Calibrated against real MCP server tool names (@modelcontextprotocol/server-filesystem and others).
…ll caveat Address code review on tool output configuration: - Normalize an empty/whitespace spill_dir to None in ToolOutputConfig so the MCP manager and the Copilot provider agree on its meaning instead of one treating "" as unset and the other forwarding output_directory="" to the SDK. - Drop the dead `| None` return type and the three now-unreachable `is not None` guards from CopilotProvider._large_output_config; every branch returns a dict. - Log a warning when spill_to_file=False on Copilot, since the SDK has no truncate-without-spill mode and this disables tool output size limiting entirely (unlike Claude, which still truncates to max_chars in place).
…d assets - Correct the Tool Output Limits row in the provider comparison to say "native SDK spill (large_output)" instead of "spill+compaction"; large_output only handles spill-to-file, not context compaction. - Clarify the spill_to_file field description in mcp-tools.md: disabling it truncates in place on Claude but disables SDK large-output handling entirely (with a runtime warning) on Copilot. - Rebuild the dashboard static assets after rebasing onto main.
23fb1bb to
abbfc10CompareCodecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #313 +/- ##
=======================================
Coverage ? 89.81% =======================================
Files ? 72 Lines ? 13238 Branches ? 0 =======================================
Hits ? 11890 Misses ? 1348 Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Jason Robert (jrob5756)
left a comment
There was a problem hiding this comment.
LGTM. Approved!
Uh oh!
There was an error while loading. Please reload this page.
Summary
Workflows that call chatty MCP tools (web scrapers, code search, Git hosts) previously died with a fatal
request exceeded model token limitAPI error when a single tool result grew too large. This PR adds a configurable per-result cap: oversized tool results are truncated to a configurable size, the full text is spilled to a temp file the agent can page through, and a notice surfaces in the console, event log, and dashboard whenever a result is capped.Provider behavior:
MCPManager.call_tool(the single choke point through which every MCP result passes) before it reaches the API. The full text is written to a process-private spill file (mode0o600, dir0o700,O_EXCLcreate). A single-line marker is appended with a generic hint by default; when the agent has filesystem-like tools, the hint is rewritten to point at the spill file.create_session/resume_session/ dialog sessions aslarge_outputinstead of re-inventing it.MAX_MCP_OUTPUT_TOKENSenv var on theclaudeCLI (outside this config).Event:
agent_tool_output_truncated(tool_name, original_chars, kept_chars, spill_path) is emitted Claude-only (the Copilot SDK owns its tool loop and exposes no hook). It renders in the console (verbose), the JSONL event log, and the dashboard activity feed.Configuration
New
runtime.tool_outputblock (opt-out, safe defaults — existing workflows behave identically unless a result exceeds the cap):See
examples/tool-output-limits.yaml.Important semantics
max_chars/max_agent_iterations. Cumulative budgeting is out of scope.max_charsis forwarded to the SDK as bytes, so multibyte UTF-8 (CJK/emoji) may truncate earlier thanmax_charscharacters.What this does NOT do
engine/context.pytrim logicclaude-agent-sdkorhermestool_outputoverrideMCPManager.call_toolsignature unchanged (still returnsstr)Testing
make test— 4212 passed (1 pre-existing live-API failure onmain:test_large_create_tool_call_does_not_truncaterequires Copilot enterprise auth, unrelated to this PR)make check(ruff + ty) — cleanmake validate-examples— cleanmake build-frontend— cleanNew/updated tests cover: schema defaults/validation, truncation + spill (filename sanitization,
0o600/0o700, UTF-8 round-trip,O_EXCL, symlink rejection for explicit/default/ancestor dirs, chmod hardening, fd-leak onfdopenfailure, Unicode cleanup), hint rewrite gating (fs-tools + real spill path required), marker parsing (both hint variants, long paths, dot-space paths, payload-literal spoofing, rightmost-marker), event emission (exactly once per truncated call), console subscriber branch, Copilotlarge_outputforwarding for all 3 session sites (includingenabled=Falseforwarding since the SDK defaults to enabled), and dashboard store/type wiring.Docs
docs/mcp-tools.md— new "Tool output limits" section + Provider Support table rowdocs/workflow-syntax.md—runtime.tool_outputreferencedocs/providers/comparison.md— capability table rowAGENTS.md— Key Patterns entryexamples/tool-output-limits.yaml— new example