diff --git a/clients/go/client.go b/clients/go/client.go index e8c235a..757a19b 100644 --- a/clients/go/client.go +++ b/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 diff --git a/clients/go/client_test.go b/clients/go/client_test.go index 4901715..49613fa 100644 --- a/clients/go/client_test.go +++ b/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). diff --git a/clients/go/errors.go b/clients/go/errors.go index ddbcc8f..c7c32b8 100644 --- a/clients/go/errors.go +++ b/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"` diff --git a/clients/go/models.go b/clients/go/models.go index f4aca3f..598e5fa 100644 --- a/clients/go/models.go +++ b/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"` }