Fix review findings in MCP tools and --json output

README.md

@@ -200,11 +200,11 @@ mcp_servers:                 # omitted by default; each entry runs a trusted loc
 
 ### Local MCP tools
 
-MCP is disabled unless `mcp_servers` is explicitly configured. Each configured executable is trusted code with its own host and network permissions; this is tool preauthorization, not a sandbox or an interactive approval prompt. Configuration is read only from the user config or the explicitly selected `-c` file, never from the repository or model output. Tinyagent launches the command directly (no shell), with `--cwd` as its working directory and a minimal environment (`PATH`, plus the Windows process variables when present) and only the named forwarded variables. Missing forwarded variables fail before launch. Do not forward secrets unless the server needs them.
+MCP is disabled unless `mcp_servers` is explicitly configured. Each configured executable is trusted code with its own host and network permissions; this is tool preauthorization, not a sandbox or an interactive approval prompt. Configuration is read only from the user config or the explicitly selected `-c` file, never from the repository or model output. Tinyagent launches the command directly (no shell), with `--cwd` as its working directory and a minimal environment (`PATH`, plus the Windows process variables when present) and only the named forwarded variables. Missing forwarded variables fail before launch. Do not forward secrets unless the server needs them. Read-only runs never start MCP servers.
 
-The Go MCP SDK v1.8.0 supports protocol revisions 2026-07-28, 2025-11-25, 2025-06-18, 2025-03-26, and 2024-11-05 over stdio. Tinyagent discovers the complete paginated catalog once per run, with a 15-second default discovery timeout, 100-page, 256-selected-tool, 64-KiB-per-schema, and 1-MiB-total-schema caps, and exposes only configured names. Tool names are sent to providers as `<alias>__<original-name>` and must fit provider-compatible naming rules. The accepted input schema subset supports object, array, string, number, integer, boolean and null types, nested properties/items, required, enum, simple bounds/length fields, and boolean `additionalProperties`; unsupported keywords and remote references are rejected. Each call defaults to a 60-second timeout. Text content is joined with newlines, structured output is appended as JSON, unsupported content is marked as omitted, tool errors remain visible, and model-visible output is capped at 256 KiB before the normal output cap is applied. Stdio protocol data is kept separate from server stderr.
+The Go MCP SDK v1.8.0 supports protocol revisions 2026-07-28, 2025-11-25, 2025-06-18, 2025-03-26, and 2024-11-05 over stdio. Tinyagent discovers the complete paginated catalog once per run, with a 15-second default discovery timeout, 100-page, 256-selected-tool, 64-KiB-per-schema, and 1-MiB-total-schema caps, and exposes only configured names. Tool names are sent to providers as `<alias>__<original-name>` and must fit provider-compatible naming rules. The accepted input schema subset supports object, array, string, number, integer, boolean and null types (a single type or an array such as `["string", "null"]`), nested properties/items, required, enum, const, numeric bounds and `multipleOf`, length, `pattern`, `format`, item and property counts, `uniqueItems`, annotations (`title`, `description`, `default`, `examples`), and boolean `additionalProperties`. A top-level `$schema` declaration is dropped. Other keywords, including `anyOf`/`oneOf`/`allOf` and references, are rejected, and a selected tool with such a schema stops startup. Each call defaults to a 60-second timeout; a timed-out call returns an error to the model and the server stays available, while any other call failure disables that server for the rest of the run. Text content is joined with newlines, structured output is appended as JSON, unsupported content is marked as omitted, tool errors remain visible, and model-visible output is capped at 256 KiB before the normal output cap is applied. Stdio protocol data is kept separate from server stderr.
 
-MCP tool calls and results are stored in trajectories along with the external tool names. Trajectories from MCP-enabled runs cannot be resumed, and MCP configuration is rejected when resuming a built-in-only checkpoint. Trajectories and server stderr may contain sensitive data supplied by tools or the server; no blanket redaction is provided. On shutdown tinyagent closes MCP sessions and asks their processes to exit, then the SDK applies its bounded termination. Unix process descendants and Windows child-process cleanup have platform-specific behavior; tinyagent does not promise process-tree cleanup on Windows.
+MCP tool calls and results are stored in trajectories along with the external tool names. Trajectories from MCP-enabled runs cannot be resumed. Resuming a built-in-only checkpoint works with `mcp_servers` configured; the servers are not started and the run keeps the built-in tools. Trajectories and server stderr may contain sensitive data supplied by tools or the server; no blanket redaction is provided. On shutdown tinyagent closes MCP sessions and asks their processes to exit, then the SDK applies its bounded termination. Unix process descendants and Windows child-process cleanup have platform-specific behavior; tinyagent does not promise process-tree cleanup on Windows.
 
 ## Long runs
 

cmd/tinyagent/main.go

@@ -157,13 +157,14 @@ func run() int {
 	}
 	ctx, stop := signal.NotifyContext(context.Background(), os.Interrupt)
 	defer stop()
-	started := time.Now()
+	// MCP discovery counts against the wall-clock limit. Agent.Run applies
+	// the same limit again, and the earlier deadline set here wins.
 	if cfg.WallTimeSec > 0 {
 		var cancel context.CancelFunc
 		ctx, cancel = context.WithTimeout(ctx, time.Duration(cfg.WallTimeSec)*time.Second)
 		defer cancel()
 	}
-	mcpRegistry, err := mcptools.Start(ctx, root, cfg.MCPServers, os.Stderr)
+	mcpRegistry, err := mcptools.Start(ctx, root, mcpServers(os.Stderr, cfg), os.Stderr)
 	if err != nil {
 		return fail(err)
 	}
@@ -202,19 +203,7 @@ func run() int {
 		SkipAgentsMD: o.noAgentsMD,
 		NoCompact:    o.noCompact,
 	}
-	events := newJSONEvents(os.Stdout, o.jsonOutput)
-	if o.jsonOutput {
-		events.emit("run.started", map[string]any{"schema_version": 1, "model": o.modelName, "root": root, "read_only": cfg.ReadOnly, "resumed": false})
-		a.OnEvent = func(kind string, fields map[string]any) {
-			events.emit(kind, fields)
-			if events.err != nil {
-				stop()
-			}
-		}
-		if cfg.CostLimit > 0 && model.Unpriced(o.modelName) {
-			events.emit("warning", map[string]any{"message": fmt.Sprintf("no pricing available for %s; --cost-limit cannot be enforced", o.modelName)})
-		}
-	}
+	events := startEvents(os.Stdout, o.jsonOutput, a, false, stop)
 	if events.err != nil {
 		return 1
 	}
@@ -230,29 +219,12 @@ func run() int {
 		}
 	}
 
-	if cfg.WallTimeSec > 0 {
-		remaining := time.Duration(cfg.WallTimeSec)*time.Second - time.Since(started)
-		if remaining <= 0 {
-			return fail(errors.New("wall-clock time limit expired during MCP discovery"))
-		}
-		cfg.WallTimeSec = int(remaining / time.Second)
-		if cfg.WallTimeSec == 0 {
-			cfg.WallTimeSec = 1
-		}
-		a.Config.WallTimeSec = cfg.WallTimeSec
-	}
-	runStart := time.Now()
+	start := time.Now()
 	t := a.Run(ctx, task)
-	elapsed := time.Since(runStart)
+	elapsed := time.Since(start)
 	reportSummary(os.Stderr, o.modelName, t, elapsed)
-	if o.jsonOutput {
-		if t.ExitStatus == agent.StatusError {
-			events.emit("error", map[string]any{"message": t.Submission})
-		}
-		events.emit("result", resultFields(t, elapsed))
-		if events.err != nil {
-			return 1
-		}
+	if events.finish(t, elapsed) != nil {
+		return 1
 	}
 	if t.ExitStatus == agent.StatusSubmitted || t.ExitStatus == agent.StatusCompleted {
 		return 0
@@ -289,8 +261,11 @@ func runResume(o *options, fs *flag.FlagSet) int {
 	if err != nil {
 		return fail(err)
 	}
-	if len(cfg.MCPServers) > 0 || src.MCPEnabled || len(src.ExternalTools) > 0 {
-		return fail(errors.New("resume: MCP-enabled runs cannot be resumed, and MCP configuration cannot be added to a built-in checkpoint; start a fresh task"))
+	if src.MCPEnabled || len(src.ExternalTools) > 0 {
+		return fail(errors.New("resume: MCP-enabled runs cannot be resumed; start a fresh task"))
+	}
+	if len(cfg.MCPServers) > 0 {
+		fmt.Fprintln(os.Stderr, "resume: configured MCP servers are not started; resumed runs use only the built-in tools")
 	}
 	applyFlagOverrides(fs, &cfg, o.stepLimit, o.costLimit, o.timeLimit)
 	if flagWasSet(fs, "read-only") && o.readOnly != src.ReadOnly {
@@ -374,19 +349,7 @@ func runResume(o *options, fs *flag.FlagSet) int {
 		Resume:       src,
 		ResumeSource: o.resume,
 	}
-	events := newJSONEvents(os.Stdout, o.jsonOutput)
-	if o.jsonOutput {
-		events.emit("run.started", map[string]any{"schema_version": 1, "model": modelName, "root": root, "read_only": src.ReadOnly, "resumed": true})
-		a.OnEvent = func(kind string, fields map[string]any) {
-			events.emit(kind, fields)
-			if events.err != nil {
-				stop()
-			}
-		}
-		if cfg.CostLimit > 0 && model.Unpriced(modelName) {
-			events.emit("warning", map[string]any{"message": fmt.Sprintf("no pricing available for %s; --cost-limit cannot be enforced", modelName)})
-		}
-	}
+	events := startEvents(os.Stdout, o.jsonOutput, a, true, stop)
 	if events.err != nil {
 		return 1
 	}
@@ -405,18 +368,12 @@ func runResume(o *options, fs *flag.FlagSet) int {
 	fmt.Fprintf(os.Stderr, "resuming %s from %s (previous status %s after %d steps)\n", modelName, o.resume, prevStatus, src.Steps)
 	fmt.Fprintln(os.Stderr, "note: inspect and revalidate the workspace; it may have changed and commands may have produced unrecorded side effects after the last save. Live processes are not restored")
 
-	started := time.Now()
+	start := time.Now()
 	t := a.Run(ctx, src.Task)
-	elapsed := time.Since(started)
+	elapsed := time.Since(start)
 	reportSummary(os.Stderr, modelName, t, elapsed)
-	if o.jsonOutput {
-		if t.ExitStatus == agent.StatusError {
-			events.emit("error", map[string]any{"message": t.Submission})
-		}
-		events.emit("result", resultFields(t, elapsed))
-		if events.err != nil {
-			return 1
-		}
+	if events.finish(t, elapsed) != nil {
+		return 1
 	}
 	if t.ExitStatus == agent.StatusSubmitted || t.ExitStatus == agent.StatusCompleted {
 		return 0
@@ -711,6 +668,47 @@ func (e *jsonEvents) emit(kind string, fields map[string]any) {
 	e.err = err
 }
 
+// startEvents writes run.started (and the unpriced cost-limit warning) and
+// routes a's events to out. A failed write cancels the run through stop.
+// When disabled it returns a no-op stream.
+func startEvents(out io.Writer, enabled bool, a *agent.Agent, resumed bool, stop func()) *jsonEvents {
+	events := newJSONEvents(out, enabled)
+	if !enabled {
+		return events
+	}
+	events.emit("run.started", map[string]any{"schema_version": 1, "model": a.ModelName, "root": a.Tools.Root, "read_only": a.Tools.ReadOnly, "resumed": resumed})
+	a.OnEvent = func(kind string, fields map[string]any) {
+		events.emit(kind, fields)
+		if events.err != nil {
+			stop()
+		}
+	}
+	if a.Config.CostLimit > 0 && model.Unpriced(a.ModelName) {
+		events.emit("warning", map[string]any{"message": fmt.Sprintf("no pricing available for %s; --cost-limit cannot be enforced", a.ModelName)})
+	}
+	return events
+}
+
+// finish writes the closing events for t and returns the first write error.
+func (e *jsonEvents) finish(t *trajectory.Trajectory, elapsed time.Duration) error {
+	if t.ExitStatus == agent.StatusError {
+		e.emit("error", map[string]any{"message": t.Submission})
+	}
+	e.emit("result", resultFields(t, elapsed))
+	return e.err
+}
+
+// mcpServers returns the MCP servers to start. Read-only runs start none:
+// a server is an arbitrary local executable, and its tools are never offered
+// there anyway.
+func mcpServers(w io.Writer, cfg config.AgentConfig) []config.MCPServer {
+	if cfg.ReadOnly && len(cfg.MCPServers) > 0 {
+		fmt.Fprintln(w, "read-only mode: configured MCP servers are not started")
+		return nil
+	}
+	return cfg.MCPServers
+}
+
 func runOutput(jsonMode bool) io.Writer {
 	if jsonMode {
 		return io.Discard

cmd/tinyagent/main_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"context"
 	"encoding/json"
 	"errors"
 	"flag"
@@ -17,6 +18,7 @@ import (
 	"github.com/vicgupta/tinyagent/internal/agent"
 	"github.com/vicgupta/tinyagent/internal/config"
 	"github.com/vicgupta/tinyagent/internal/model"
+	"github.com/vicgupta/tinyagent/internal/tools"
 	"github.com/vicgupta/tinyagent/internal/trajectory"
 )
 
@@ -162,6 +164,91 @@ func TestJSONEventsWriteOneJSONLine(t *testing.T) {
 	}
 }
 
+// replies is a model that returns its messages in order.
+type replies []model.Message
+
+func (r *replies) Query(context.Context, []model.Message, []model.ToolSpec) (model.Message, model.Cost, error) {
+	if len(*r) == 0 {
+		return model.Message{}, model.Cost{}, errors.New("out of replies")
+	}
+	msg := (*r)[0]
+	*r = (*r)[1:]
+	return msg, model.Cost{InputTokens: 10, OutputTokens: 5}, nil
+}
+
+func newTestAgent(t *testing.T, m model.Model) *agent.Agent {
+	t.Helper()
+	root := t.TempDir()
+	if err := os.Mkdir(filepath.Join(root, ".git"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	return &agent.Agent{Model: m, ModelName: "fake", Tools: &tools.Executor{Root: root}, Config: config.Default(), SkipAgentsMD: true}
+}
+
+// TestJSONRunEventStream drives a whole run through startEvents and finish:
+// stdout must be only JSONL, opening with run.started and closing with result.
+func TestJSONRunEventStream(t *testing.T) {
+	m := &replies{
+		{Role: "assistant", Content: "looking", ToolCalls: []model.ToolCall{{ID: "1", Type: "function", Function: model.FunctionCall{Name: "list_files", Arguments: `{}`}}}},
+		{Role: "assistant", Content: "all good\n" + agent.SubmitMarker},
+	}
+	a := newTestAgent(t, m)
+	var out strings.Builder
+	events := startEvents(&out, true, a, false, func() { t.Error("stop called on a healthy stream") })
+	tr := a.Run(context.Background(), "check")
+	if err := events.finish(tr, time.Second); err != nil {
+		t.Fatal(err)
+	}
+	var kinds []string
+	var last map[string]any
+	for i, line := range strings.Split(strings.TrimSuffix(out.String(), "\n"), "\n") {
+		if err := json.Unmarshal([]byte(line), &last); err != nil {
+			t.Fatalf("line %d is not JSON: %q", i, line)
+		}
+		kinds = append(kinds, last["type"].(string))
+	}
+	want := []string{"run.started", "assistant.message", "tool.call", "tool.result", "assistant.message", "result"}
+	if strings.Join(kinds, " ") != strings.Join(want, " ") {
+		t.Fatalf("events = %v, want %v", kinds, want)
+	}
+	if last["status"] != agent.StatusSubmitted || last["submission"] != "all good" || last["duration_ms"] != float64(1000) {
+		t.Errorf("result = %#v", last)
+	}
+}
+
+func TestJSONEventsDisabledWriteNothing(t *testing.T) {
+	a := newTestAgent(t, &replies{{Role: "assistant", Content: "done"}})
+	var out strings.Builder
+	events := startEvents(&out, false, a, false, func() {})
+	if err := events.finish(a.Run(context.Background(), "x"), 0); err != nil || out.Len() != 0 || a.OnEvent != nil {
+		t.Errorf("disabled stream wrote %q (err %v, OnEvent set %v)", out.String(), err, a.OnEvent != nil)
+	}
+}
+
+func TestReadOnlyStartsNoMCPServers(t *testing.T) {
+	servers := []config.MCPServer{{Alias: "docs", Command: "docs-mcp", Tools: []string{"search"}}}
+	var w strings.Builder
+	if got := mcpServers(&w, config.AgentConfig{MCPServers: servers}); len(got) != 1 || w.Len() != 0 {
+		t.Errorf("normal mode: servers %v, note %q", got, w.String())
+	}
+	if got := mcpServers(&w, config.AgentConfig{ReadOnly: true, MCPServers: servers}); got != nil || !strings.Contains(w.String(), "not started") {
+		t.Errorf("read-only mode: servers %v, note %q", got, w.String())
+	}
+}
+
+// TestDeadlineSpentBeforeRunIsTimeLimit covers a wall-clock limit used up by
+// MCP discovery: the run still ends as TimeLimitExceeded.
+func TestDeadlineSpentBeforeRunIsTimeLimit(t *testing.T) {
+	a := newTestAgent(t, &replies{{Role: "assistant", Content: "done"}})
+	a.Config.WallTimeSec = 60
+	ctx, cancel := context.WithTimeout(context.Background(), time.Nanosecond)
+	defer cancel()
+	<-ctx.Done()
+	if tr := a.Run(ctx, "x"); tr.ExitStatus != agent.StatusTimeLimit {
+		t.Errorf("status = %s, want %s", tr.ExitStatus, agent.StatusTimeLimit)
+	}
+}
+
 // wantUsage is the layout from issue #12, with its update: -h, --help listed
 // first and -V for --version.
 const wantUsage = `Usage: tinyagent -t "<prompt>" [flags]    (or --task "<prompt>")

internal/agent/loop_test.go

@@ -181,14 +181,14 @@ func TestRunDispatchesExternalToolsWithEvents(t *testing.T) {
 	}}
 	a := newAgent(t, m)
 	a.Tools.ExternalSpecs = []model.ToolSpec{{Type: "function", Function: model.FunctionSpec{Name: "docs__search"}}}
-	a.Tools.ExternalCall = func(_ context.Context, name, raw string) (string, bool) {
+	a.Tools.ExternalCall = func(_ context.Context, name, raw string) (string, bool, bool) {
 		if name != "docs__search" {
-			return "", false
+			return "", false, false
 		}
 		if raw == `{}` {
-			return "tool error: missing q", true
+			return "tool error: missing q", true, true
 		}
-		return "result for " + raw, true
+		return "result for " + raw, false, true
 	}
 	results := map[string]map[string]any{}
 	a.OnEvent = func(kind string, fields map[string]any) {