fix: wave-9 bug sweep — FIFO-safe @-resource loads, MCP server-to-client request routing, envelope-with-notes rendering (#189)

Three RED-first fixes, each pinned by failing tests on main:

1. resource (FileResolver.Load, HIGH): opening a FIFO (or other special
   file) in the workspace with no writer blocked the @-resource load in
   the syscall forever — ctx is not observable there — and the FIFO's
   zero size passed the cap, leaving the subsequent ReadAll unbounded.
   The open now uses O_NONBLOCK (a no-op for regular files) and
   non-regular files are rejected outright.

2. mcpclient (readLoop routing, HIGH): any parseable line was routed by
   id — a spec-legal server-to-client REQUEST (JSON-RPC 2.0: carries
   "method") whose id collided with an in-flight call delivered
   {result:null} to the waiter and the real response was dropped when it
   arrived. Lines carrying "method" are now skipped: they are never
   responses to our calls.

3. mcpclient (CallTool envelope + notes, MED): a multi-item result
   [envelope, trailing note] was joined with "\n" before the envelope
   probe — the trailing data failed the JSON parse and the RAW envelope
   JSON, including artifact refs that must never reach the model
   unvalidated, was delivered as plain text, bypassing artifact-ref
   validation entirely. Each text item is now parsed for the envelope;
   non-envelope items are preserved after the rendered form (through the
   per-server result cap).

Dropped after honest verification: the claimed base64url blind spot in
the unread-script decode pass — for realistic payloads the leading
pre-boundary run still decodes the injection phrase, and no payload
could be constructed where every run stays under the 24-char candidate
threshold. No RED, no fix.

Author: jkyberneees

internal/mcpclient/calltool_multiitem_test.go

@@ -0,0 +1,64 @@
+package mcpclient
+
+import (
+	"bufio"
+	"context"
+	"fmt"
+	"io"
+	"strings"
+	"testing"
+	"time"
+)
+
+// A server returning [envelope item, trailing note] produced a JOINED text
+// "envelope\nnote": the envelope probe fails on the trailing data, the
+// envelope is treated as plain text, and the RAW envelope JSON — including
+// artifact refs that must never reach the model unvalidated — is delivered
+// verbatim, bypassing artifact-ref validation entirely.
+func TestClient_CallTool_EnvelopeWithTrailingContent(t *testing.T) {
+	clientRead, serverWrite := io.Pipe()
+	serverRead, clientWrite := io.Pipe()
+	go io.Copy(io.Discard, serverRead)
+
+	c := &Client{
+		name:      "multiitem",
+		stdin:     clientWrite,
+		stdout:    bufio.NewReader(clientRead),
+		lineCh:    make(chan lineResult, 10),
+		done:      make(chan struct{}),
+		writeCh:   make(chan []byte, 2),
+		writeDone: make(chan struct{}),
+		closed:    make(chan struct{}),
+		pending:   make(map[int]chan callResponse),
+		timeout:   5 * time.Second,
+	}
+	go c.readLoop()
+	go c.writeLoop()
+	defer func() {
+		c.closeOnce.Do(func() { close(c.closed) })
+		clientWrite.Close()
+		clientRead.Close()
+	}()
+
+	go func() {
+		// nextID starts at 0: the first call is id 0.
+		fmt.Fprint(serverWrite, `{"jsonrpc":"2.0","id":0,"result":{"content":[`+
+			`{"type":"text","text":"{\"schema\":\"odek.tool-result/v1\",\"text\":\"report ready\"}"},`+
+			`{"type":"text","text":"(generated 2 artifacts)"}`+
+			`]}}`+"\n")
+	}()
+
+	out, err := c.CallTool(context.Background(), "build_report", `{}`)
+	if err != nil {
+		t.Fatalf("CallTool: %v", err)
+	}
+	if strings.Contains(out, `"schema":"odek.tool-result/v1"`) {
+		t.Fatalf("raw envelope JSON delivered to the model (artifact-ref validation bypassed): %q", out)
+	}
+	if !strings.Contains(out, "report ready") {
+		t.Fatalf("rendered envelope text missing from result: %q", out)
+	}
+	if !strings.Contains(out, "(generated 2 artifacts)") {
+		t.Fatalf("trailing content note lost: %q", out)
+	}
+}

internal/mcpclient/client.go

@@ -95,6 +95,7 @@ type request struct {
 type response struct {
 	JSONRPC string          `json:"jsonrpc"`
 	ID      int             `json:"id"`
+	Method  string          `json:"method,omitempty"`
 	Result  json.RawMessage `json:"result,omitempty"`
 	Error   *rpcError       `json:"error,omitempty"`
 }
@@ -645,6 +646,53 @@ func (c *Client) CallTool(ctx context.Context, name string, argsJSON string) (st
 	if err != nil {
 		return "", fmt.Errorf("mcpclient %s: tool %s: %w", c.name, name, err)
 	}
+	if env == nil && len(parts) > 1 {
+		// A multi-item result carrying an envelope plus trailing notes: the
+		// joined text no longer parses as one JSON document, so the probe
+		// above treats it as plain text — delivering the RAW envelope JSON to
+		// the model and bypassing artifact-ref validation. Parse each item
+		// for the envelope instead; a schema-matched but malformed item still
+		// fails closed.
+		var envErr error
+		for _, p := range parts {
+			e, eerr := artifact.ParseEnvelope(p)
+			if eerr != nil {
+				envErr = eerr
+				continue
+			}
+			if e != nil {
+				env = e
+				break
+			}
+		}
+		if env == nil && envErr != nil {
+			return "", fmt.Errorf("mcpclient %s: tool %s: %w", c.name, name, envErr)
+		}
+		// Preserve the non-envelope items after the rendered form: the
+		// trailing notes are ordinary text the server meant the model to
+		// see alongside the artifact metadata.
+		if env != nil {
+			var extras []string
+			for _, p := range parts {
+				if e, eerr := artifact.ParseEnvelope(p); eerr == nil && e != nil {
+					continue // the envelope item(s) render above
+				}
+				if strings.TrimSpace(p) != "" {
+					extras = append(extras, p)
+				}
+			}
+			suffix := ""
+			if len(extras) > 0 {
+				suffix = "\n" + strings.Join(extras, "\n")
+			}
+			for i := range env.Artifacts {
+				if _, err := artifact.Validate(env.Artifacts[i], c.artifactRoots); err != nil {
+					return "", fmt.Errorf("mcpclient %s: tool %s: artifact ref rejected: %w", c.name, name, err)
+				}
+			}
+			return c.applyResultLimit(name, c.renderCappedEnvelope(name, env)+suffix), nil
+		}
+	}
 	if env != nil {
 		for i := range env.Artifacts {
 			// The resolved path is intentionally discarded here: it is
@@ -877,6 +925,13 @@ func (c *Client) readLoop() {
 		if err := json.Unmarshal([]byte(line), &resp); err != nil {
 			continue // skip malformed lines
 		}
+		// A line carrying a "method" field is a server→client request or
+		// notification (JSON-RPC 2.0), never a response to one of our calls.
+		// Routing it by id would deliver {result:null} to a waiting caller
+		// whose id collides — and drop the real response when it arrives.
+		if resp.Method != "" {
+			continue
+		}
 
 		// Route to the waiting caller, if any.
 		c.mu.Lock()

internal/mcpclient/readloop_route_test.go

@@ -0,0 +1,60 @@
+package mcpclient
+
+import (
+	"bufio"
+	"context"
+	"encoding/json"
+	"fmt"
+	"io"
+	"strings"
+	"testing"
+	"time"
+)
+
+// A spec-legal server→client REQUEST (JSON-RPC 2.0: carries "method") whose
+// id collides with an in-flight call was routed to that call's waiter as
+// {result: null} — the real response was then dropped and the call failed
+// with a parse error. Responses to OUR calls never carry "method".
+func TestClient_Call_IgnoresServerToClientRequests(t *testing.T) {
+	clientRead, serverWrite := io.Pipe()
+	serverRead, clientWrite := io.Pipe()
+	go io.Copy(io.Discard, serverRead) // drain client→server writes
+
+	c := &Client{
+		name:      "confused",
+		stdin:     clientWrite,
+		stdout:    bufio.NewReader(clientRead),
+		lineCh:    make(chan lineResult, 10),
+		done:      make(chan struct{}),
+		writeCh:   make(chan []byte, 2),
+		writeDone: make(chan struct{}),
+		closed:    make(chan struct{}),
+		pending:   make(map[int]chan callResponse),
+		timeout:   5 * time.Second,
+	}
+	go c.readLoop()
+	go c.writeLoop()
+	cleanup := func() {
+		c.closeOnce.Do(func() { close(c.closed) })
+		clientWrite.Close()
+		clientRead.Close()
+	}
+	defer cleanup()
+
+	go func() {
+		// 1) Spec-legal server→client request whose id collides with the call
+		// (nextID starts at 0, so the first call is id 0).
+		fmt.Fprint(serverWrite, `{"jsonrpc":"2.0","id":0,"method":"ping","params":{}}`+"\n")
+		time.Sleep(100 * time.Millisecond) // deterministic ordering
+		// 2) The real response to the client's call.
+		fmt.Fprint(serverWrite, `{"jsonrpc":"2.0","id":0,"result":{"ok":true}}`+"\n")
+	}()
+
+	res, err := c.call(context.Background(), "tools/call", json.RawMessage(`{}`))
+	if err != nil {
+		t.Fatalf("call failed after colliding server→client request: %v", err)
+	}
+	if res == nil || !strings.Contains(string(res), `"ok":true`) {
+		t.Fatalf("result = %s, want the real response payload", res)
+	}
+}

internal/resource/resource.go

@@ -333,7 +333,11 @@ func (f *FileResolver) Load(ctx context.Context, id string) (string, error) {
 	// Open with O_NOFOLLOW to atomically prevent the final component from
 	// being a symlink. If the path is a symlink, the open fails with ELOOP —
 	// closing the TOCTOU window between a separate Lstat check and the read.
-	fd, err := os.OpenFile(resolvedTarget, os.O_RDONLY|syscall.O_NOFOLLOW, 0)
+	// O_NONBLOCK guards against special files: opening a FIFO with no
+	// writer blocks in the syscall forever (ctx is not observable there),
+	// and the FIFO's zero size would pass the cap below leaving ReadAll
+	// unbounded. For regular files O_NONBLOCK is a no-op.
+	fd, err := os.OpenFile(resolvedTarget, os.O_RDONLY|syscall.O_NOFOLLOW|syscall.O_NONBLOCK, 0)
 	if err != nil {
 		return "", err
 	}
@@ -343,6 +347,9 @@ func (f *FileResolver) Load(ctx context.Context, id string) (string, error) {
 	if err != nil {
 		return "", err
 	}
+	if !info.Mode().IsRegular() {
+		return "", fmt.Errorf("resource: %q is not a regular file", id)
+	}
 	if info.Size() > maxResourceFileBytes {
 		return "", fmt.Errorf("resource: file too large (%d bytes, max %d)", info.Size(), maxResourceFileBytes)
 	}

internal/resource/resource_fifo_test.go

@@ -0,0 +1,48 @@
+package resource
+
+import (
+	"context"
+	"path/filepath"
+	"sync"
+	"syscall"
+	"testing"
+	"time"
+)
+
+func mkfifo(path string) error {
+	return syscall.Mkfifo(path, 0600)
+}
+
+// A FIFO (or any non-regular file) in the workspace must not hang or
+// over-read @-resource loads: the open was blocking (no writer → forever,
+// ctx ignored), and a FIFO's zero size passed the size gate leaving the
+// subsequent ReadAll unbounded.
+func TestFileResolver_Load_FIFODoesNotHang(t *testing.T) {
+	dir := t.TempDir()
+	fifo := filepath.Join(dir, "pipe")
+	if err := mkfifo(fifo); err != nil {
+		t.Skipf("mkfifo unavailable: %v", err)
+	}
+
+	r := NewFileResolver(dir)
+	type res struct {
+		content string
+		err     error
+	}
+	done := make(chan res, 1)
+	var once sync.Once
+	go func() {
+		content, err := r.Load(context.Background(), "pipe")
+		once.Do(func() { done <- res{content, err} })
+	}()
+
+	select {
+	case got := <-done:
+		if got.err == nil {
+			t.Fatalf("FIFO load unexpectedly succeeded: %d bytes", len(got.content))
+		}
+	case <-time.After(3 * time.Second):
+		once.Do(func() {})
+		t.Fatal("Load on a writerless FIFO hung — non-regular files must be rejected without blocking")
+	}
+}