Polish spill envelope: inject correct tool name via middleware, simplify filenames, doc process-global config (stacks on #4) #5

Background

PR #4 added server-side spilling of oversized tool results — landing the bulk of the work. Three follow-up gaps to close before merging, all small and contained to that branch.

This is a stacked change: it targets copilot/spill-oversize-tool-results (the head of PR #4), not phlax-custom. The diff should be additive on top of PR #4 so the two land together.

Gap 1 (functional) — populate envelope tool field via middleware

In PR #4, pkg/utils/result.go exposes NewToolResultTextWithMeta(message, ResultMeta{Tool: ...}) but nothing in the codebase calls it — every handler still uses NewToolResultText(message), which constructs ResultMeta{}. Result: every spill envelope ends up with "tool": "tool-result", defeating the field.

We don't want to thread context into NewToolResultText (would touch every call site, which is the opposite of the "single choke point" win we just secured). Instead, add a middleware in pkg/github/ that runs after the tool handler and rewrites the envelope's tool field using req.Params.Name.

To keep things simple, also drop the tool name from the spill filename — use a fully opaque name (spill-{unixMillis}-{hex}.{ext}). That way the middleware never has to rename a file on disk; it only rewrites the JSON envelope text in memory. The tool field inside the envelope remains the authoritative source of "which tool produced this".

Implementation sketch

New file pkg/github/spill_middleware.go:

package github

import (
    "context"
    "encoding/json"
    "strings"

    "github.com/modelcontextprotocol/go-sdk/mcp"
)

// spillEnvelopePrefix is the cheap discriminator used to avoid JSON-parsing
// every text response. The envelope is produced by utils.NewToolResultText and
// always starts with this prefix because encoding/json preserves struct field
// order and Spilled is the first field of spillEnvelope.
const spillEnvelopePrefix = `{"spilled":true,`

// SpillToolNameMiddleware rewrites the "tool" field inside spill envelopes
// produced by pkg/utils so it reflects the actual tool name from the
// CallToolRequest. The spill writer in pkg/utils has no access to the tool
// name, so this middleware injects it after the fact.
//
// Non-CallTool methods, non-envelope results, and unparseable envelopes are
// all passed through unchanged.
func SpillToolNameMiddleware(next mcp.MethodHandler) mcp.MethodHandler {
    return func(ctx context.Context, method string, req mcp.Request) (mcp.Result, error) {
        result, err := next(ctx, method, req)
        if err != nil {
            return result, err
        }
        callReq, ok := req.(*mcp.CallToolRequest)
        if !ok || callReq == nil || callReq.Params == nil || callReq.Params.Name == "" {
            return result, nil
        }
        callResult, ok := result.(*mcp.CallToolResult)
        if !ok || callResult == nil || len(callResult.Content) != 1 {
            return result, nil
        }
        text, ok := callResult.Content[0].(*mcp.TextContent)
        if !ok || !strings.HasPrefix(text.Text, spillEnvelopePrefix) {
            return result, nil
        }

        var envelope map[string]any
        if err := json.Unmarshal([]byte(text.Text), &envelope); err != nil {
            return result, nil
        }
        if _, hasSpilled := envelope["spilled"].(bool); !hasSpilled {
            return result, nil
        }
        envelope["tool"] = callReq.Params.Name

        rewritten, err := json.Marshal(envelope)
        if err != nil {
            return result, nil
        }
        text.Text = string(rewritten)
        return result, nil
    }
}

Wire in pkg/github/server.go::NewMCPServer, register after the other middlewares so it runs outermost (closest to client), seeing the final result text:

ghServer.AddReceivingMiddleware(SpillToolNameMiddleware)

(Per the existing comment in NewMCPServer: "middleware executes in reverse registration order, so handlers see this context before executing." Registering last means running first on the way out — which is what we want, so we see the result before any other after-handler logic.)

Actually re-reading that comment more carefully: "middleware executes in reverse registration order, so handlers see this context before executing." That refers to ingress ordering. For egress (return path) it's the opposite — register first to run last on the way out. Whoever picks this up: confirm the actual ordering empirically with a quick test before committing the registration order. The behavioural test below (Gap 1, test 2) will fail loudly if it's wrong.

Filename simplification

In pkg/utils/result.go:

• Change generateSpillFilename(tool, ext string) to generateSpillFilename(ext string) and produce spill-{unixMillis}-{hex}{ext}.
• Delete sanitizeSpillToolName (no callers after the rename).
• The Tool field on ResultMeta becomes purely informational — populated to "tool-result" by default, and overridden by the middleware. Keep the field for future direct callers.

Tests for Gap 1

In a new pkg/github/spill_middleware_test.go:

1. Non-spill text passes through unchanged.
2. A real spill envelope (call utils.NewToolResultText with a large body and a configured spill dir) routed through the middleware ends up with "tool":"<the test tool name>".
3. Non-CallToolRequest methods (use a stub request type) pass through unchanged.
4. Mangled envelope text ({"spilled":true,bogus}) passes through unchanged.
5. Multiple Content entries pass through unchanged.

Use a mcp.MethodHandler test double; do not stand up a real server.

Gap 2 (polish) — document that SpillConfig is process-global

PR #4 stores spill config in a package-level var spillConfig SpillConfig guarded by sync.RWMutex. That's intentional — both stdio and HTTP startup call utils.SetSpillConfig(cfg.SpillConfig) exactly once. But the MCPServerConfig struct also carries SpillConfig, and the HTTP handler constructs a fresh MCPServerConfig per request (see pkg/http/handler.go::ServeHTTP). If anyone ever sets a different spill config per request through that path, the per-request value will silently disagree with the process-global one — wrong behaviour, no error.

Fix: add a doc comment on SetSpillConfig making the design explicit, and matching comments on the SpillConfig field of MCPServerConfig, StdioServerConfig, and ServerConfig. No code change.

// SetSpillConfig updates the process-global oversized response spill
// configuration. It is intended to be called exactly once during server
// startup before requests are served. Calls after startup are race-safe
// but will affect all in-flight tool calls server-wide — there is no
// per-request override path.
func SetSpillConfig(cfg SpillConfig) { ... }

// SpillConfig controls optional spilling of oversized tool results to disk.
// This config is applied process-globally via utils.SetSpillConfig during
// server startup; per-request overrides are not supported.
SpillConfig utils.SpillConfig

Gap 3 (docs) — add a non-sandbox example

README section added by PR #4 currently shows --spill-dir /workspace/tmp, which is right for our sandboxed-agent case but might lead non-sandboxed devs to reach for /tmp by reflex (defeating the whole point of the feature in some setups). Add a second example for the non-sandboxed dev case:

# For local dev outside a sandbox, prefer a tmpfs that's tied to your session
./github-mcp-server stdio --spill-dir "${XDG_RUNTIME_DIR:-/tmp}/github-mcp-spill"

And a one-line note: "spilling is only useful when the directory you point us at is reachable by the agent that will read the envelope. In a containerized / sandboxed setup, pick a path that's bind-mounted into the agent's filesystem; on a single host, any writable path that both processes can see is fine."

Out of scope

• MIMEHint field on ResultMeta — currently exposed but never set by any caller. The auto-detect (json.Valid → application/json) covers ~all real responses. Leave the field alone; revisit when there's an actual caller.
• Per-tool threshold overrides.
• Structured summaries for known content types (diff stats, file lists).

Acceptance criteria

• Spill envelopes produced by handlers in pkg/github/* have their tool field set to the actual tool name (verified by new test).
• Spill filenames no longer embed the tool name; they are spill-{unixMillis}-{hex}.{ext}.
• SetSpillConfig and the three SpillConfig struct fields are documented as process-global / not per-request.
• README has a second example for the non-sandboxed dev case and a one-line note explaining when spilling is useful.
• All existing PR Spill oversized tool results to a configured shared filesystem path #4 tests still pass after the filename simplification (the test in result_test.go does not assert the tool name in the filename — confirmed by reading it).

Files most likely to change

• pkg/github/spill_middleware.go (new)
• pkg/github/spill_middleware_test.go (new)
• pkg/github/server.go — register middleware, add doc comment on SpillConfig
• pkg/utils/result.go — simplify generateSpillFilename, drop sanitizeSpillToolName, add doc comment on SetSpillConfig
• pkg/utils/result_test.go — remove the tool-result filename-prefix assertion if present
• internal/ghmcp/server.go and pkg/http/server.go — add doc comments on SpillConfig field
• README.md — second example + note