fix: sandbox result shape, empty tool schemas, CLI exec 2s deadline, retrieve_tools response limit, tool-id normalization (#981–#985) - #988
Merged
Conversation
…exec deadline, retrieve_tools limit, tool-id normalization
- code_execution sandbox: call_tool() results are JSON round-tripped before
entering goja, so scripts see the documented wire shape
(res.result.content[0].text) instead of Go field names (Content/Text);
non-serializable results return {ok:false, code:SERIALIZATION_ERROR} (#981)
- tools list / REST tool endpoints: supervisor StateView now parses
ToolMetadata.ParamsJSON instead of fabricating an empty
{type:object, properties:{}} schema for every tool; malformed schemas omit
the field rather than faking an empty one (#982)
- mcpproxy code exec (daemon mode): the 2s daemon-detection context is no
longer reused for the exec POST; the client deadline is now --timeout +30s
slack, CodeExec uses an uncapped clone of the shared HTTP client (the 5-min
blanket timeout no longer caps 10-min budgets), and a client-side deadline
is reported as such instead of a bare 'context deadline exceeded' (#983)
- retrieve_tools: responses are now subject to tool_response_limit via the
standard truncate-and-cache path with a read_cache handle; the record path
is pinned to the tools array (never the disabled list or a nested array),
the code-execution surface (which has no read_cache tool) is exempt, and
CallToolDirect gained a read_cache case so the banner is followable from
REST/CLI/Web-UI/tray (#984)
- describe_tool / call_tool_*: tool ids are whitespace-trimmed (outer and
around the colon) on both paths via the shared splitServerTool; a
case-mismatched id gets a "did you mean '<canonical>'?" remediation gated
on session visibility so it can never confirm out-of-scope or quarantined
tools; exact-case matching is preserved everywhere (#985)
Fixes #981, fixes #982, fixes #983, fixes #984, fixes #985
Deploying mcpproxy-docs with
|
| Latest commit: |
a83c249
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://51e47794.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://fix-issues-981-985.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 31792700873 --repo smart-mcp-proxy/mcpproxy-go
|
…e/exec outlive the blanket HTTP deadlines
Two defects surfaced by cross-model review of the daemon-mode timeout fix:
- handleCodeExecution only accepted JSON-decoded option shapes (float64,
[]interface{}), so the Go-typed args built by the REST handler were
silently dropped: a request restricted with max_tool_calls or
allowed_servers ran unrestricted. Option parsing now accepts both shapes
(applyCodeExecutionOptions), and the REST handler forwards only options
the caller actually supplied so config defaults keep their meaning.
- POST /api/v1/code/exec inherited the blanket 60s /api/v1 middleware
deadline and the process-wide http.Server read/write deadlines (~120s),
so any execution past those died regardless of timeout_ms. The route now
carries its own 630s budget (600s tool ceiling + IO slack) at both
layers; every other API route keeps the 60s deadline.
…aware REST deadline, bounded terminal truncation - CodeExecOptions fields are now pointers so the REST handler distinguishes an option the caller sent from one they omitted: an explicit max_tool_calls:0 or allowed_servers:[] reaches the tool as written, and a present-but-invalid timeout_ms:0 is rejected with a 400 instead of being silently replaced by the configured budget. - When timeout_ms is omitted, the REST request context now covers the 600000ms ceiling the tool may resolve from code_execution_timeout_ms (previously a 120s fallback cancelled any higher configured budget); the tool still applies its precise deadline inside, and the 630s route budget bounds the request overall. - A retrieve_tools response that exceeds tool_response_limit but has nothing to paginate (0-1 tools, one sprawling schema) is now cut plainly with the standard notice instead of passing through unbounded; sub-limit responses remain byte-identical.
…; keep simple truncation under tiny limits - max_tool_calls default resolution used zero as its unset marker, but zero is the documented unlimited override — an explicit 0 was floored to the configured limit on every transport. MaxToolCalls now starts at a -1 sentinel (never expressible by a caller; negatives are rejected during parsing) and resolveCodeExecutionDefaults only fills genuinely unset options. - simpleTruncate appended its 50-byte notice after reserving only limit/2 for limits under 200, so limits under ~100 produced over-limit output. The limit is now a hard ceiling: the notice is squeezed in when it fits and dropped for a bare prefix when it cannot; SimpleTruncateBudget mirrors the same math (Spec 084 sync).
…anner truncation under the limit - A fractional or non-numeric timeout_ms/max_tool_calls is now rejected instead of silently int-truncated (0.5 became 0 — the unlimited override; 1.9 became a 1ms budget). - When tool_response_limit is too small to carry the ~300-byte read_cache banner, Truncate falls back to bounded simple truncation with no cache handle instead of returning an over-limit banner. Legacy tests that pinned over-limit banners at limits 30/100 now use banner-sized limits.
This was referenced Aug 14, 2026
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
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.
Fixes the five bugs filed against v0.56.0. Each fix was implemented TDD (failing test observed first), then the whole diff went through an adversarial multi-lens review; all confirmed findings are folded in.
Fixes #981, fixes #982, fixes #983, fixes #984, fixes #985
What changed
#981 — sandbox
call_tool()exposed Go field namesinternal/jsruntime/runtime.gonow JSON round-trips everycall_toolresult before handing it to goja, so scripts see the documented wire shape (res.result.content[0].text) instead of the live Go value (Content/Text, leakedMarshalJSONmethods). A non-serializable result returns{ok:false, error:{code:"SERIALIZATION_ERROR"}}instead of a live pointer. The server-side activity/history classification still receives the raw typed result — unchanged.Back-compat note: scripts that adapted to the bug with PascalCase access (
r.result.Content[0].Text) will break; that shape was never documented, and the tool description has always instructed the lowercase form. TheJSON.parse(JSON.stringify(r))workaround remains valid.#982 — empty
{"properties":{},"type":"object"}schema for all toolstoolInfosFromMetadata(supervisor StateView) fabricated a hardcoded empty schema stub (literal// TODO: Parse ParamsJSON) on every population path. It now parsesToolMetadata.ParamsJSON; a malformed schema omits the field rather than emitting a truthful-looking empty object. This single choke point fixesGET /api/v1/tools,GET /api/v1/servers/{name}/tools, and allmcpproxy tools listformats. No payload shape change, so no swagger/contracts regen.#983 —
code execdaemon mode hard ~2s abortThe 2s daemon-detection context was reused for the exec POST. Now: separate ping context (2s, as before) and exec context derived from
--timeout+ 30s slack;CodeExecdispatches through an uncapped clone of the shared HTTP client (same transport/auth), so the CLI-wide 5-minute blanket timeout no longer caps legal 10-minute budgets; a client-side deadline is reported as an explicit client-side timeout instead of a barecontext deadline exceededagainsthttp://localhost. Also hardened a pre-existing nil-config deref on the standalone fallback path.#984 —
retrieve_toolsexempt fromtool_response_limit(~55k chars/call)retrieve_toolsresponses now go through the standard truncate-and-cache path: over-limit output is cut attool_response_limitwith the usualread_cachebanner, and the full payload is retrievable viaread_cache. Details that came out of review:toolsarray — the truncator can no longer pick the Spec-049disabledlist or a nested schema array as the pagination target; if the pin can't resolve, the banner is dropped rather than advertising a wrong contract (TruncateWithRecordPathininternal/truncate).read_cachetool, and Spec 085 pins it to full mode).CallToolDirectgained aread_cachecase, so the banner is followable from REST/CLI/Web-UI/tray — this also cures the pre-existing dead-end for truncatedcall_tool_*results over REST.detailmode is unchanged (Spec 085 byte-identity guarantees hold;compactremains opt-in per call or viatool_response_mode).Behavior notes: with the default
tool_response_limit=20000, large catalogs will see truncated discovery responses by default — the truncated text is not parseable JSON (same long-standing contract as truncatedcall_tool_*responses), and each truncated call now writes the full payload to the BBolt cache (TTL-bounded, 2h). Anythingstrings.Contains-polling raw retrieve_tools output (e.g.release-gate'swaitToolDiscoverable) can miss tools that fall past the cut.#985 —
describe_toolexact-matchnot_foundfor near-miss idsTool ids are whitespace-trimmed (outer and around the
:) on bothdescribe_toolandcall_tool_*via the sharedsplitServerTool. A case-mismatched id (server or tool segment) now gets"Tool not found. Tool ids are case-sensitive — did you mean '<canonical>'?"— the suggestion is gated on session visibility for the corrected pair, so it can never confirm an out-of-scope, quarantined, or pending tool (pinned by a dedicated no-scope-leak test). Case is never silently accepted: every approval/quarantine/scope gate keys on exact names, and silent folding would bypass them.Verification (all local, Apple Silicon)
go buildboth editions (personal +-tags server)go test -race -count=1green:internal/jsruntime,internal/runtime/supervisor,cmd/mcpproxy,internal/cliclient,internal/server(full suite),internal/truncate,internal/cache,internal/serveredition(-tags server)golangci-lintv2 with.github/.golangci.yml(CI config): 0 issues repo-wide./scripts/test-api-e2e.sh: 65/65 passed~/.mcpproxy), plus a follow-up correctness pass that reported no surviving logic defects.Out of scope: #986 (stored scripts) and #987 (batched
call_tool) are feature requests that go through the spec process separately.Follow-up from cross-model review (second commit)
Codex review of the branch surfaced two adjacent defects on the REST code-exec path, both verified and fixed here because they gate the #983 fix:
handleCodeExecutiononly accepted JSON-decoded option shapes (float64,[]interface{}), so the Go-typed args built byPOST /api/v1/code/exec(and thereforemcpproxy code execdaemon mode) never matched —max_tool_calls/allowed_serversrestrictions ran unrestricted. Parsing now accepts both shapes, and the REST handler forwards only options the caller actually supplied so config defaults keep their meaning./api/v1/code/execwas capped by the blanket HTTP deadlines: the 60s/api/v1middleware timeout and the process-widehttp.Serverread/write deadlines (~120s) cancelled any execution past them regardless oftimeout_ms— defeating the raised client deadline. The route now carries its own 630s budget (600s tool ceiling + IO slack) at both layers; every other API route keeps the 60s deadline.