fix: reduce retained tool output memory

[dgageot]

• wait for lifecycle supervisor watcher goroutines during shutdown
• bound retained tool output in session history with a regression test
• spool large MCP media payloads to disk instead of retaining inline base64
• slim retained TUI tool results and avoid duplicating file contents in metadata

[docker-agent]

❌ PR Review Failed — The review agent encountered an error and could not complete the review. View logs.

[aheritier]

The PR attacks four distinct memory sources and the implementation is mostly clean. The WithoutPayload slimming, the watchDone shutdown guarantee, and the ReadFileMeta.Content removal are all sound. One blocking issue must be resolved before merge.

Blocking

Disk leak: spooled MCP media files are never cleaned up.
writeMediaFile creates temp directories under os.TempDir() with prefix docker-agent-mcp-media-* but nothing removes them when the session ends or the ToolCallResult is GC'd. The PR itself calls defer os.RemoveAll(filepath.Dir(img.FilePath)) in the test, which highlights the production gap. Over a long session with many large media responses this trades the memory leak for a disk leak.

Fix options:

• Register a cleanup func on the GatewayToolset lifecycle (cf. cleanUp pattern already used there), called on Stop.
• Use a single session-scoped temp directory created once at Toolset construction and removed atomically on teardown — simpler than one dir per payload and easier to reason about.

Non-blocking

1. Double WithoutPayload() in reasoningblock.go — messages.go already calls msg.Result.WithoutPayload() before passing the result to UpdateToolResult; reasoningblock.go then calls result.WithoutPayload() again internally. Harmless (idempotent on a stripped result) but confusing. Choose one call site and remove the other.

2. Missing test: disk-write fallback in encodeMedia — the slog.Warn + inline fallback branch is untested. Worth covering with an injected error (e.g. a package-level writeMediaFile var that tests can replace).

3. Missing test: threshold boundary — TestProcessMCPContentSpoolsLargeMedia only checks maxInlineMediaBytes+1. Add a case for exactly maxInlineMediaBytes bytes → inline, and maxInlineMediaBytes+1 → spooled.

4. TestSupervisor_StopWaitsForWatcher is weak coverage for the concurrency fix — the test exercises the sequential path (Start → Stop), not the concurrent-Stop path that the s.stopping guard is specifically designed to handle. A test calling Stop twice concurrently from two goroutines while the watcher is live would be a stronger regression guard. The < time.Second timing assertion is also brittle under heavy CI load; goleak would be more idiomatic.

5. readImageFile in filesystem.go still inlines large local images — up to chat.MaxImageBytes (~4.5 MB) as base64. This is inconsistent with the new MCP spooling strategy. Out of scope for this PR, but worth a follow-up issue.

[aheritier]

[BLOCKING] Disk leak — these temp directories are never removed.

os.MkdirTemp creates a directory in os.TempDir() on every call to writeMediaFile, but nothing cleans it up when the ToolCallResult is discarded or the session ends. Over a long session with many large media tool responses this will exhaust disk space.

Suggested fix: introduce a session-scoped temp directory (created once, removed on Stop) instead of a per-payload directory. Alternatively, register a cleanup closure on the Toolset lifecycle the same way GatewayToolset does today.

[aheritier]

The test correctly cleans up with defer os.RemoveAll(...), but this highlights that the production code does not. This defer will be unnecessary once a lifecycle cleanup hook is added on the production side.

[aheritier]

[Non-blocking] Double WithoutPayload() call.

The caller in messages.go already passes msg.Result.WithoutPayload() here, so result is already a stripped copy. Calling result.WithoutPayload() a second time is idempotent but redundant and may confuse future readers. Either remove WithoutPayload() from this line (since it's already stripped at the call site), or remove it from the call site in messages.go and let UpdateToolResult own the stripping — but not both.

[aheritier]

[Non-blocking] Brittle timing assertion and weak concurrency coverage.

This < time.Second bound will be flaky under heavy CI load. More importantly, the test only covers the sequential Start→Stop path, not the concurrent-Stop path that the s.stopping guard was added to fix. A test with two goroutines calling Stop concurrently while the watcher is alive (e.g. blocked in sess.Wait()) would be a stronger regression guard. Consider using goleak to assert no goroutines are left behind.