Skip to content

Fix review findings in MCP tools and --json output - #63

Merged
vicgupta merged 1 commit into
mainfrom
fix/mcp-json-review-followups
Oct 25, 2026
Merged

vicgupta merged 1 commit into
mainfrom
fix/mcp-json-review-followups

Conversation

@vicgupta

Copy link
Copy Markdown
Owner

Summary

Follow-up to #58 and #61, fixing issues found reviewing them after merge.

MCP tools (#58)

  • Read-only runs no longer start MCP servers. Their tools were already hidden there, but the executables (and any forwarded secrets) were still launched.
  • mcp_servers no longer blocks --resume. Any MCP config used to make every resume fail; built-in checkpoints now resume and the servers are skipped with a note. MCP-enabled checkpoints are still rejected.
  • Schema check accepts SDK-generated schemas. A top-level $schema is dropped, nullable ["string","null"] types and standard keywords (format, pattern, const, exclusive bounds, multipleOf, uniqueItems, examples, …) are allowed. anyOf/oneOf/allOf and refs are still rejected.
  • A call timeout no longer disables the server for the rest of the run; only connection failures do.
  • Explicit error flag. ExternalCall now returns isErr instead of tool.result.is_error being inferred from output prefixes.
  • Cleanup. Per-server startup is its own function (no duplicated validation, no unreachable reserved-name check, no defer in a loop). The wall-clock recomputation after discovery is gone: the deadline set before discovery already bounds the run, and an exhausted budget now ends as TimeLimitExceeded instead of a generic exit-1 error.

--json (#61)

  • startEvents / jsonEvents.finish replace the event setup duplicated across fresh and resumed runs.
  • New end-to-end test of a run's JSONL stream (run.started … result, nothing else on stdout).

Verification

  • go build ./..., go vet ./..., gofmt -l ., go test -count=1 ./..., git diff --check
  • Cross-compiled for linux, windows, darwin (amd64)
  • New MCP fixture tests: SDK-style schema, anyOf rejected, isError flagged, timeout keeps the server usable (confirmed failing against the old behavior)
  • New cmd tests: JSON event stream, disabled stream writes nothing, read-only skips MCP, spent deadline → TimeLimitExceeded

🤖 Generated with Claude Code

MCP (#58):
- Read-only runs no longer start configured MCP servers.
- mcp_servers in the config no longer blocks --resume of built-in
  checkpoints; the servers are skipped instead.
- Accept common SDK-generated schemas: drop a top-level $schema, allow
  nullable type arrays and standard validation/annotation keywords.
- A call timeout or interrupt no longer disables the server for the
  rest of the run; only connection failures do.
- ExternalCall reports failures with an explicit flag instead of
  matching "error: " / "tool error: " prefixes.
- Move per-server startup into one function: drop checks duplicated
  from validateDefinitions, the unreachable reserved-name check, and
  the deferred cancel inside the loop.
- Drop the wall-clock recomputation after discovery; the deadline set
  before discovery already bounds the run and ends as TimeLimitExceeded.

--json (#61):
- Share run.started/result emission between fresh and resumed runs.
- Test a full run's event stream end to end.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@vicgupta
vicgupta merged commit ce57926 into main Oct 25, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant