clients/go: apply audit findings — fmt + doc + test coverage (3c62613 → new)

- L1: gofmt fix on models.go:81
- L2: rewrite misleading RunFailure doc comment (didn't actually embed APIError)
- L3: tighten Client doc to warn against post-construction field mutation
- L4: errors.New for non-formatting Errorf calls
- L5: add TestUploadFile lifting coverage from 0% → 100% on UploadFile
- L7: add context cancellation mid-multipart test

Audit: memory/clawdforge-audits/go-3c62613.md

clients/go/client.go

@@ -30,8 +30,9 @@ import (
 	"strings"
 )
 
-// Client is an immutable handle to a clawdforge instance. Construct with
-// New or NewWithClient. Methods are safe for concurrent use.
+// Client is a handle to a clawdforge instance. Construct with New or
+// NewWithClient. Methods are safe for concurrent use; do not mutate
+// BaseURL, Token, or HTTPClient after construction.
 type Client struct {
 	BaseURL    string
 	Token      string

clients/go/client_test.go

@@ -1,6 +1,7 @@
 package clawdforge
 
 import (
+	"bytes"
 	"context"
 	"encoding/json"
 	"errors"
@@ -8,7 +9,10 @@ import (
 	"mime/multipart"
 	"net/http"
 	"net/http/httptest"
+	"os"
+	"path/filepath"
 	"strings"
+	"sync"
 	"testing"
 	"time"
 )
@@ -170,7 +174,7 @@ func TestRunEmptyPromptRejected(t *testing.T) {
 	}
 }
 
-func TestUploadFile(t *testing.T) {
+func TestUploadReader(t *testing.T) {
 	c, done := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
 		if r.URL.Path != "/files" {
 			t.Errorf("path = %q", r.URL.Path)
@@ -228,6 +232,76 @@ func TestUploadFile(t *testing.T) {
 	}
 }
 
+// L5: exercise the file-path wrapper so coverage on UploadFile lifts off 0%.
+// Writes a small tempfile, uploads it, and verifies the multipart envelope —
+// notably that filepath.Base() is what hits the wire, not the full tmp path.
+func TestUploadFile(t *testing.T) {
+	dir := t.TempDir()
+	fpath := filepath.Join(dir, "snippet.txt")
+	const payload = "uploaded-from-disk"
+	if err := os.WriteFile(fpath, []byte(payload), 0o600); err != nil {
+		t.Fatalf("WriteFile: %v", err)
+	}
+
+	c, done := newTestClient(t, func(w http.ResponseWriter, r *http.Request) {
+		if r.URL.Path != "/files" {
+			t.Errorf("path = %q", r.URL.Path)
+		}
+		mr, err := r.MultipartReader()
+		if err != nil {
+			t.Fatalf("MultipartReader: %v", err)
+		}
+		var (
+			gotTTL  string
+			gotFile string
+			gotData []byte
+		)
+		for {
+			part, err := mr.NextPart()
+			if err == io.EOF {
+				break
+			}
+			if err != nil {
+				t.Fatalf("NextPart: %v", err)
+			}
+			switch part.FormName() {
+			case "ttl_secs":
+				b, _ := io.ReadAll(part)
+				gotTTL = string(b)
+			case "file":
+				gotFile = part.FileName()
+				gotData, _ = io.ReadAll(part)
+			}
+		}
+		if gotTTL != "120" {
+			t.Errorf("ttl_secs = %q", gotTTL)
+		}
+		// Server should see basename only, not the full tmpdir path.
+		if gotFile != "snippet.txt" {
+			t.Errorf("filename = %q, want basename only", gotFile)
+		}
+		if string(gotData) != payload {
+			t.Errorf("data = %q", string(gotData))
+		}
+		w.Header().Set("Content-Type", "application/json")
+		_, _ = w.Write([]byte(`{"file_token":"ff_disk","ttl_secs":120,"size":18}`))
+	})
+	defer done()
+
+	ft, err := c.UploadFile(context.Background(), fpath, 120)
+	if err != nil {
+		t.Fatalf("UploadFile: %v", err)
+	}
+	if ft.FileToken != "ff_disk" || ft.TTLSecs != 120 || ft.Size != 18 {
+		t.Errorf("got %+v", ft)
+	}
+
+	// And the missing-file path must return a wrapped open error.
+	if _, err := c.UploadFile(context.Background(), filepath.Join(dir, "does-not-exist"), 0); err == nil {
+		t.Error("expected error opening nonexistent file")
+	}
+}
+
 func TestCreateAndListAndRevokeToken(t *testing.T) {
 	created := false
 	listed := false
@@ -284,6 +358,125 @@ func TestCreateAndListAndRevokeToken(t *testing.T) {
 	}
 }
 
+// L7: exercise the io.Pipe + goroutine + multipart writer interplay when
+// the caller's context is cancelled mid-upload. Verifies that:
+//  1. UploadReader returns a TransportError wrapping context.Canceled,
+//  2. the producer goroutine and pipe close cleanly (no leak / no hang),
+//  3. the source io.Reader stops being read once cancellation propagates.
+func TestUploadReaderContextCancelMidStream(t *testing.T) {
+	// Server stalls so the upload is still in flight when we cancel.
+	handlerEntered := make(chan struct{})
+	handlerDone := make(chan struct{})
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
+		defer close(handlerDone)
+		close(handlerEntered)
+		// Read whatever the client sends until the request context dies or
+		// the client disconnects. Either way, just drain.
+		_, _ = io.Copy(io.Discard, r.Body)
+	}))
+	defer srv.Close()
+	c := New(srv.URL, "tok")
+
+	// A producer that emits chunks slowly so we're definitely mid-stream
+	// when cancel fires. It tracks calls so we can assert it stopped being
+	// read after cancellation.
+	slow := &slowReader{chunk: bytes.Repeat([]byte("x"), 256), delay: 5 * time.Millisecond, total: 1 << 20}
+
+	ctx, cancel := context.WithCancel(context.Background())
+
+	// Run UploadReader on a goroutine so we can cancel from the outside
+	// once we know the request has reached the server.
+	type result struct {
+		err error
+	}
+	resCh := make(chan result, 1)
+	go func() {
+		_, err := c.UploadReader(ctx, "big.bin", slow, 60)
+		resCh <- result{err: err}
+	}()
+
+	// Wait until the server is actively handling the request, then cancel.
+	select {
+	case <-handlerEntered:
+	case <-time.After(3 * time.Second):
+		t.Fatal("server handler never entered — request did not reach server")
+	}
+	// Give the producer a moment to actually be writing into the pipe.
+	time.Sleep(50 * time.Millisecond)
+	cancel()
+
+	var got result
+	select {
+	case got = <-resCh:
+	case <-time.After(3 * time.Second):
+		t.Fatal("UploadReader did not return after cancel — pipe/goroutine likely leaked")
+	}
+
+	if got.err == nil {
+		t.Fatal("expected error from cancelled upload")
+	}
+	var te *TransportError
+	if !errors.As(got.err, &te) {
+		t.Fatalf("err is not *TransportError: %T %v", got.err, got.err)
+	}
+	if !errors.Is(got.err, context.Canceled) {
+		t.Errorf("err does not wrap context.Canceled: %v", got.err)
+	}
+
+	// Snapshot reads-so-far, wait, then re-check: if cleanup worked the
+	// producer should have stopped being polled for more data.
+	before := slow.reads()
+	time.Sleep(150 * time.Millisecond)
+	after := slow.reads()
+	if after > before {
+		t.Errorf("producer kept being read after cancel: before=%d after=%d (pipe likely leaked)", before, after)
+	}
+	if before == 0 {
+		t.Error("producer was never read — test did not exercise multipart streaming")
+	}
+
+	// Force the stalled connection closed so the server handler unblocks.
+	srv.CloseClientConnections()
+	select {
+	case <-handlerDone:
+	case <-time.After(3 * time.Second):
+		t.Error("server handler did not exit")
+	}
+}
+
+// slowReader emits chunk after delay each Read, up to total bytes. It
+// counts Read calls so tests can assert reads ceased after cancellation.
+type slowReader struct {
+	chunk    []byte
+	delay    time.Duration
+	total    int
+	mu       sync.Mutex
+	produced int
+	calls    int
+}
+
+func (s *slowReader) Read(p []byte) (int, error) {
+	time.Sleep(s.delay)
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	s.calls++
+	if s.produced >= s.total {
+		return 0, io.EOF
+	}
+	n := copy(p, s.chunk)
+	if s.produced+n > s.total {
+		n = s.total - s.produced
+	}
+	s.produced += n
+	return n, nil
+}
+
+func (s *slowReader) reads() int {
+	s.mu.Lock()
+	defer s.mu.Unlock()
+	return s.calls
+}
+
 func TestContextCancellation(t *testing.T) {
 	// Block server-side handler until the request context dies OR a hard
 	// safety timer fires (so a misbehaving client can't hang the test).

clients/go/errors.go

@@ -46,9 +46,8 @@ func (e *TransportError) Unwrap() error { return e.Err }
 // request but `claude -p` failed (timeout, non-zero exit, etc.). The body
 // fields mirror the server's failure shape.
 //
-// RunFailure satisfies APIError-like semantics by also embedding the status
-// code via the underlying APIError, but it's a distinct type so callers can
-// branch on errors.As(err, &cf.RunFailure{}).
+// RunFailure has its own StatusCode field (always 502) and is distinct from
+// APIError so callers can branch on errors.As(err, &rf).
 type RunFailure struct {
 	StatusCode int
 	Err        string `json:"error"`

clients/go/models.go

@@ -2,6 +2,7 @@ package clawdforge
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 )
 
@@ -43,7 +44,7 @@ type RunResult struct {
 //	if err := res.AsJSON(&data); err != nil { ... }
 func (r *RunResult) AsJSON(out any) error {
 	if len(r.Result) == 0 {
-		return fmt.Errorf("clawdforge: empty result")
+		return errors.New("clawdforge: empty result")
 	}
 	if err := json.Unmarshal(r.Result, out); err != nil {
 		return fmt.Errorf("clawdforge: result is not JSON: %w", err)
@@ -56,7 +57,7 @@ func (r *RunResult) AsJSON(out any) error {
 // representation as a string in that case).
 func (r *RunResult) AsText() (string, error) {
 	if len(r.Result) == 0 {
-		return "", fmt.Errorf("clawdforge: empty result")
+		return "", errors.New("clawdforge: empty result")
 	}
 	// First try to decode as a plain JSON string
 	var s string
@@ -78,7 +79,7 @@ type FileToken struct {
 // plaintext Token populated) by POST /admin/tokens.
 type AppToken struct {
 	Name      string   `json:"name"`
-	Token     string   `json:"token,omitempty"`     // only set on create
+	Token     string   `json:"token,omitempty"` // only set on create
 	IPCidrs   []string `json:"ip_cidrs,omitempty"`
 	CreatedAt int64    `json:"created_at,omitempty"`
 }