From 4016d6d6b43f475bcde4da766a0aacda9086b66d Mon Sep 17 00:00:00 2001 From: Goon Date: Thu, 28 May 2026 17:23:32 +0700 Subject: [PATCH] fix(cli): render trace details by id with validation + exit-code mapping (issue #17) Closes #17. Bug surface: `goclaw traces get ` was unreadable in TTY mode (dumped raw JSON), silently swallowed unmarshal errors (empty `{}`), accepted any raw id including path-traversal sequences (`..`, `/`, control chars), and collapsed every server failure to exit code 1 regardless of HTTP status. Fixes: - `cmd/traces.go` - `tracesGetCmd.RunE`: validate id allowlist before HTTP, decode payload with error surfacing, render header card + span tree + events list for TTY, pass-through JSON for `-o json` / piped stdout. - `validateTraceID`: regex allowlist `^[A-Za-z0-9._-]+$` + reserved-token reject (`.`, `..`, empty/whitespace). Returns typed `*client.APIError{Code: INVALID_REQUEST}` so the central error handler maps it to exit code 4. - `renderTraceTable` + `buildSpanTree`: insertion-ordered span tree, orphan spans attach to virtual root. No relink walk, cycle-safe (cycles silently drop). - `internal/client/http.go` - Retry loop bug: previously closed `resp.Body` on every iteration including the final attempt, then `io.ReadAll` after the loop failed with "read on closed response body". This collapsed typed `APIError` into an opaque wrapped error and lost the exit-code mapping for 5xx/429. Fixed by skipping the per-iteration close on the final attempt; outer `defer` handles cleanup. Exit-code contract for `traces get`: - 0 success / 2 permission denied / 3 not-found / 4 malformed id (pre-HTTP) / 5 server failure / 6 rate-limit. Tests: 10 new tests in `cmd/traces_get_test.go` lock the wire contract, table+JSON rendering, exit-code mapping per HTTP status, and the "malformed id makes zero HTTP calls" invariant (parameterized table). Fixture `cmd/testdata/trace_detail_get.json` is a scrubbed stub with `_TODO_refresh` marker; refresh against live gateway before merging to default. Docs: CHANGELOG `[Unreleased]` Fixed entry, README `Reading a Trace by ID` section with exit-code table, `docs/codebase-summary.md` traces bullet updated. --- CHANGELOG.md | 4 + README.md | 12 + cmd/testdata/trace_detail_get.json | 54 +++ cmd/traces.go | 119 ++++++- cmd/traces_get_test.go | 322 ++++++++++++++++++ docs/codebase-summary.md | 2 +- internal/client/http.go | 12 +- .../phase-01-repro-and-root-cause.md | 193 +++++++++++ ...ase-02-cli-fix-output-and-error-mapping.md | 255 ++++++++++++++ .../phase-03-docs-and-ship.md | 97 ++++++ .../plan.md | 86 +++++ .../reports/code-review-260528-issue17.md | 40 +++ .../reports/red-team-assumption-destroyer.md | 110 ++++++ .../reports/red-team-failure-mode-analyst.md | 99 ++++++ .../red-team-scope-complexity-critic.md | 109 ++++++ .../reports/red-team-security-adversary.md | 155 +++++++++ .../reports/repro-260528-issue17.md | 109 ++++++ 17 files changed, 1772 insertions(+), 6 deletions(-) create mode 100644 cmd/testdata/trace_detail_get.json create mode 100644 cmd/traces_get_test.go create mode 100644 plans/260528-1357-fix-trace-details-by-id/phase-01-repro-and-root-cause.md create mode 100644 plans/260528-1357-fix-trace-details-by-id/phase-02-cli-fix-output-and-error-mapping.md create mode 100644 plans/260528-1357-fix-trace-details-by-id/phase-03-docs-and-ship.md create mode 100644 plans/260528-1357-fix-trace-details-by-id/plan.md create mode 100644 plans/260528-1357-fix-trace-details-by-id/reports/code-review-260528-issue17.md create mode 100644 plans/260528-1357-fix-trace-details-by-id/reports/red-team-assumption-destroyer.md create mode 100644 plans/260528-1357-fix-trace-details-by-id/reports/red-team-failure-mode-analyst.md create mode 100644 plans/260528-1357-fix-trace-details-by-id/reports/red-team-scope-complexity-critic.md create mode 100644 plans/260528-1357-fix-trace-details-by-id/reports/red-team-security-adversary.md create mode 100644 plans/260528-1357-fix-trace-details-by-id/reports/repro-260528-issue17.md diff --git a/CHANGELOG.md b/CHANGELOG.md index b137d4e..b969e1a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -55,6 +55,10 @@ Format: [Keep a Changelog](https://keepachangelog.com/en/1.0.0/). - `goclaw activity aggregate --group-by {action|actor_type|entity_type|actor_id} [--from --to --limit --actor-type --actor-id --action --entity-type --entity-id]` — group audit-log activity by dimension with bucket counts (`GET /v1/activity/aggregate`). Attached as subcommand of existing `activity` parent. - `goclaw logs aggregate [--group-by {level|source}] [--level --source --from]` — summarize the runtime log ring buffer (`GET /v1/logs/runtime/aggregate`, admin-only). Distinct from `logs tail`. Epoch-millis `last_seen` rendered as RFC3339, never scientific notation. +### Fixed + +- `goclaw traces get ` — TTY mode now renders a human-readable summary (header card + span tree + events list) instead of dumping raw JSON. JSON-mode payload unchanged. Decode failures surface as wrapped errors instead of an empty `{}`. Trace ids are validated against `^[A-Za-z0-9._-]+$` and reserved tokens (`.`, `..`) are rejected before any HTTP call. Distinct exit codes per failure: not-found → 3, permission-denied → 2, malformed-id → 4, server-failure → 5. Latent retry-body bug in `internal/client/http.go` fixed: the final 5xx/429 response body is now preserved so the typed `APIError` reaches the caller (previously collapsed to exit 1). Closes #17. + ### Notes - All new commands honor the AI-first ergonomics contract: `--output=json` envelope, central error handler, `--yes` for destructive ops, `--quiet` for CI. - P4/P5 backlog was re-swept against the current CLI surface; already-covered items were removed from residual scope before implementation. diff --git a/README.md b/README.md index d4e306d..43a1a4d 100644 --- a/README.md +++ b/README.md @@ -124,6 +124,18 @@ goclaw logs aggregate [--group-by ] [--level ] [--source ] [ All are one-shot HTTP — no watch loops or WS streams. `logs aggregate` is admin-only on the server; `activity aggregate --group-by actor_id` is also admin-only (server-enforced). +### Reading a Trace by ID + +```bash +# Human-readable: header + span tree + events +goclaw traces get + +# Machine-readable JSON (also auto-selected when stdout is piped) +goclaw traces get -o json +``` + +Exit codes for `traces get`: `0` on success, `2` on permission denied, `3` on not-found, `4` on malformed id (rejected before any HTTP call — allowlist `^[A-Za-z0-9._-]+$`), `5` on upstream server failure, `6` on rate-limit / network-resource exhaustion. + ## Backup & Restore ### System Backup diff --git a/cmd/testdata/trace_detail_get.json b/cmd/testdata/trace_detail_get.json new file mode 100644 index 0000000..6ecad9c --- /dev/null +++ b/cmd/testdata/trace_detail_get.json @@ -0,0 +1,54 @@ +{ + "_TODO_refresh": "stub fixture derived from traces follow payload shape; refresh against goclaw.zuey.me before merge per phase-03 reviewer gate", + "trace_id": "trace_FIXTURE_001", + "agent_id": "agent_FIXTURE_001", + "session_key": "session_FIXTURE_001", + "user_id": "user_REDACTED", + "tenant_id": "tenant_REDACTED", + "status": "success", + "started_at": "2026-05-28T10:00:00Z", + "ended_at": "2026-05-28T10:00:02Z", + "duration_ms": 2000, + "input_tokens": 120, + "output_tokens": 80, + "cost": "0.0042", + "spans": [ + { + "span_id": "span_001", + "parent_span_id": null, + "name": "agent.run", + "kind": "agent", + "started_at": "2026-05-28T10:00:00Z", + "ended_at": "2026-05-28T10:00:02Z", + "duration_ms": 2000, + "status": "success" + }, + { + "span_id": "span_002", + "parent_span_id": "span_001", + "name": "llm.call", + "kind": "llm", + "started_at": "2026-05-28T10:00:00Z", + "ended_at": "2026-05-28T10:00:01Z", + "duration_ms": 1500, + "status": "success", + "input_tokens": 120, + "output_tokens": 80 + }, + { + "span_id": "span_003", + "parent_span_id": "span_001", + "name": "tool.call", + "kind": "tool", + "started_at": "2026-05-28T10:00:01Z", + "ended_at": "2026-05-28T10:00:02Z", + "duration_ms": 400, + "status": "success" + } + ], + "events": [ + {"event_id": "ev_001", "span_id": "span_002", "type": "llm.prompt", "timestamp": "2026-05-28T10:00:00Z"}, + {"event_id": "ev_002", "span_id": "span_002", "type": "llm.completion", "timestamp": "2026-05-28T10:00:01Z"}, + {"event_id": "ev_003", "span_id": "span_003", "type": "tool.invoke", "timestamp": "2026-05-28T10:00:01Z"} + ] +} diff --git a/cmd/traces.go b/cmd/traces.go index 851bff1..6803495 100644 --- a/cmd/traces.go +++ b/cmd/traces.go @@ -1,12 +1,16 @@ package cmd import ( + "encoding/json" "fmt" "io" "net/url" "os" + "regexp" + "strings" "time" + "github.com/nextlevelbuilder/goclaw-cli/internal/client" "github.com/nextlevelbuilder/goclaw-cli/internal/output" "github.com/spf13/cobra" ) @@ -61,19 +65,130 @@ var tracesListCmd = &cobra.Command{ var tracesGetCmd = &cobra.Command{ Use: "get ", Short: "Get trace with span tree", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { + id := strings.TrimSpace(args[0]) + if err := validateTraceID(id); err != nil { + return err + } c, err := newHTTP() if err != nil { return err } - data, err := c.Get("/v1/traces/" + args[0]) + data, err := c.Get("/v1/traces/" + url.PathEscape(id)) if err != nil { return err } - printer.Print(unmarshalMap(data)) + var trace map[string]any + if err := json.Unmarshal(data, &trace); err != nil { + return fmt.Errorf("decode trace payload: %w", err) + } + if cfg.OutputFormat != "table" { + printer.Print(trace) + return nil + } + renderTraceTable(trace, os.Stdout) return nil }, } +// traceIDPattern restricts trace ids to a safe, URL-safe allowlist. +// Blocks path-traversal (`..`, `/`, `\`), control characters, and whitespace +// before any HTTP call is issued. PathEscape is still applied on top. +var traceIDPattern = regexp.MustCompile(`^[A-Za-z0-9._-]+$`) + +func validateTraceID(id string) error { + if id == "" || id == "." || id == ".." { + return &client.APIError{Code: "INVALID_REQUEST", Message: "trace id is empty or reserved"} + } + if !traceIDPattern.MatchString(id) { + return &client.APIError{Code: "INVALID_REQUEST", Message: "trace id contains invalid characters (allowed: A-Z a-z 0-9 . _ -)"} + } + return nil +} + +// renderTraceTable prints a human-readable summary: header card, span tree, events. +func renderTraceTable(t map[string]any, w io.Writer) { + for _, row := range [][2]string{ + {"TRACE_ID", str(t, "trace_id")}, {"AGENT_ID", str(t, "agent_id")}, + {"SESSION_KEY", str(t, "session_key")}, {"STATUS", str(t, "status")}, + {"DURATION_MS", str(t, "duration_ms")}, + } { + if row[1] != "" { + fmt.Fprintf(w, "%-12s %s\n", row[0]+":", row[1]) + } + } + if in, out, cost := str(t, "input_tokens"), str(t, "output_tokens"), str(t, "cost"); in+out+cost != "" { + fmt.Fprintf(w, "%-12s in=%s out=%s cost=%s\n", "TOKENS:", in, out, cost) + } + spans, _ := t["spans"].([]any) + if len(spans) == 0 { + fmt.Fprintln(w, "\nSPANS: (none)") + } else { + fmt.Fprintln(w, "\nSPANS:") + output.PrintTreeRoot(buildSpanTree(spans), w) + } + events, _ := t["events"].([]any) + fmt.Fprintf(w, "\nEVENTS (n=%d):\n", len(events)) + for _, e := range events { + if m, ok := e.(map[string]any); ok { + fmt.Fprintf(w, " - %s\n", str(m, "type")) + } + } +} + +// buildSpanTree links spans via parent_span_id; spans whose parent isn't in this +// trace attach to a virtual root. Children are kept in insertion order. +func buildSpanTree(spans []any) output.TreeNode { + order := make([]string, 0, len(spans)) + labels := make(map[string]string, len(spans)) + children := make(map[string][]string, len(spans)) + parentOf := make(map[string]string, len(spans)) + for _, s := range spans { + m, ok := s.(map[string]any) + if !ok { + continue + } + id := str(m, "span_id") + if id == "" { + continue + } + label := id + if name := str(m, "name"); name != "" { + label = name + " [" + id + "]" + } + if kind := str(m, "kind"); kind != "" { + label += " kind=" + kind + } + if dur := str(m, "duration_ms"); dur != "" { + label += " " + dur + "ms" + } + labels[id] = label + order = append(order, id) + parentOf[id], _ = m["parent_span_id"].(string) + } + for _, id := range order { + if p := parentOf[id]; p != "" { + if _, ok := labels[p]; ok { + children[p] = append(children[p], id) + continue + } + } + children[""] = append(children[""], id) + } + var build func(id string) output.TreeNode + build = func(id string) output.TreeNode { + n := output.TreeNode{Name: labels[id]} + for _, c := range children[id] { + n.Children = append(n.Children, build(c)) + } + return n + } + root := output.TreeNode{Name: "trace"} + for _, id := range children[""] { + root.Children = append(root.Children, build(id)) + } + return root +} + var tracesExportCmd = &cobra.Command{ Use: "export ", Short: "Export trace to file", Args: cobra.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { diff --git a/cmd/traces_get_test.go b/cmd/traces_get_test.go new file mode 100644 index 0000000..fd14ce2 --- /dev/null +++ b/cmd/traces_get_test.go @@ -0,0 +1,322 @@ +package cmd + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "os" + "strings" + "sync/atomic" + "testing" + + "github.com/nextlevelbuilder/goclaw-cli/internal/output" +) + +// loadTraceDetailFixture reads the captured trace detail envelope from testdata. +// The fixture is a single trace map (not wrapped). Tests wrap it via okJSON. +func loadTraceDetailFixture(t *testing.T) map[string]any { + t.Helper() + data, err := os.ReadFile("testdata/trace_detail_get.json") + if err != nil { + t.Fatalf("read fixture: %v", err) + } + var m map[string]any + if err := json.Unmarshal(data, &m); err != nil { + t.Fatalf("decode fixture: %v", err) + } + return m +} + +// errJSON writes an error envelope mimicking the server shape. +func errJSON(t *testing.T, w http.ResponseWriter, status int, code, message string) { + t.Helper() + w.Header().Set("Content-Type", "application/json") + w.WriteHeader(status) + body, _ := json.Marshal(map[string]any{ + "ok": false, + "error": map[string]any{"code": code, "message": message}, + }) + _, _ = w.Write(body) +} + +// TestTracesGet_PathAndMethod locks the wire contract: GET /v1/traces/{id}. +func TestTracesGet_PathAndMethod(t *testing.T) { + var calls int64 + var gotPath, gotMethod string + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt64(&calls, 1) + gotPath = r.URL.Path + gotMethod = r.Method + okJSON(t, w, loadTraceDetailFixture(t)) + })) + defer srv.Close() + t.Setenv("GOCLAW_SERVER", srv.URL) + t.Setenv("GOCLAW_TOKEN", "test-token") + + if err := runCmd(t, "traces", "get", "trace_FIXTURE_001", "--output", "json"); err != nil { + t.Fatalf("traces get: %v", err) + } + if atomic.LoadInt64(&calls) != 1 { + t.Fatalf("expected 1 request, got %d", atomic.LoadInt64(&calls)) + } + if gotMethod != http.MethodGet { + t.Errorf("method = %q, want GET", gotMethod) + } + if gotPath != "/v1/traces/trace_FIXTURE_001" { + t.Errorf("path = %q, want /v1/traces/trace_FIXTURE_001", gotPath) + } +} + +// TestTracesGet_HappyPath_JSON_LocksFixture round-trips the JSON envelope. +func TestTracesGet_HappyPath_JSON_LocksFixture(t *testing.T) { + fixture := loadTraceDetailFixture(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + okJSON(t, w, fixture) + })) + defer srv.Close() + t.Setenv("GOCLAW_SERVER", srv.URL) + t.Setenv("GOCLAW_TOKEN", "test-token") + + out, err := captureStdout(t, func() error { + return runCmd(t, "traces", "get", "trace_FIXTURE_001", "--output", "json") + }) + if err != nil { + t.Fatalf("traces get: %v", err) + } + var got map[string]any + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("stdout is not JSON: %v\nstdout: %q", err, out) + } + if got["trace_id"] != "trace_FIXTURE_001" { + t.Errorf("trace_id = %v", got["trace_id"]) + } + if got["agent_id"] != "agent_FIXTURE_001" { + t.Errorf("agent_id = %v", got["agent_id"]) + } + if got["status"] != "success" { + t.Errorf("status = %v", got["status"]) + } + spans, ok := got["spans"].([]any) + if !ok || len(spans) != 3 { + t.Errorf("spans = %v (want 3 entries)", got["spans"]) + } +} + +// TestTracesGet_TableMode_HumanReadable_RED — the issue #17 repro (now green after fix). +func TestTracesGet_TableMode_HumanReadable_RED(t *testing.T) { + fixture := loadTraceDetailFixture(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + okJSON(t, w, fixture) + })) + defer srv.Close() + t.Setenv("GOCLAW_SERVER", srv.URL) + t.Setenv("GOCLAW_TOKEN", "test-token") + + out, err := captureStdout(t, func() error { + return runCmd(t, "traces", "get", "trace_FIXTURE_001", "--output", "table") + }) + if err != nil { + t.Fatalf("traces get: %v", err) + } + trimmed := strings.TrimSpace(out) + if strings.HasPrefix(trimmed, "{") { + t.Fatalf("table mode rendered raw JSON (starts with '{'): %q", out) + } + wantAny := []string{"TRACE", "SPAN", "EVENT", "trace_id", "agent_id"} + hit := false + for _, m := range wantAny { + if strings.Contains(out, m) { + hit = true + break + } + } + if !hit { + t.Fatalf("table mode missing human-readable markers; got: %q", out) + } +} + +// TestTracesGet_TableMode_HasHeaderAndSpanMarkers — verifies header card + tree drawing. +func TestTracesGet_TableMode_HasHeaderAndSpanMarkers(t *testing.T) { + fixture := loadTraceDetailFixture(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + okJSON(t, w, fixture) + })) + defer srv.Close() + t.Setenv("GOCLAW_SERVER", srv.URL) + t.Setenv("GOCLAW_TOKEN", "test-token") + + out, err := captureStdout(t, func() error { + return runCmd(t, "traces", "get", "trace_FIXTURE_001", "--output", "table") + }) + if err != nil { + t.Fatalf("traces get: %v", err) + } + if strings.HasPrefix(strings.TrimSpace(out), "{") { + t.Fatalf("table mode rendered raw JSON: %q", out) + } + if !strings.Contains(out, "TRACE_ID") { + t.Errorf("missing TRACE_ID header in: %q", out) + } + // At least one tree connector must appear (├─ or └─). + if !strings.Contains(out, "├") && !strings.Contains(out, "└") { + t.Errorf("missing span tree connectors (├ / └) in: %q", out) + } + if !strings.Contains(out, "EVENTS") { + t.Errorf("missing EVENTS section in: %q", out) + } +} + +// TestTracesGet_JSONMode_PreservesStructure — all top-level fixture keys present in JSON output. +func TestTracesGet_JSONMode_PreservesStructure(t *testing.T) { + fixture := loadTraceDetailFixture(t) + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + okJSON(t, w, fixture) + })) + defer srv.Close() + t.Setenv("GOCLAW_SERVER", srv.URL) + t.Setenv("GOCLAW_TOKEN", "test-token") + + out, err := captureStdout(t, func() error { + return runCmd(t, "traces", "get", "trace_FIXTURE_001", "--output", "json") + }) + if err != nil { + t.Fatalf("traces get: %v", err) + } + var got map[string]any + if err := json.Unmarshal([]byte(out), &got); err != nil { + t.Fatalf("not JSON: %v", err) + } + for k := range fixture { + if _, ok := got[k]; !ok { + t.Errorf("top-level key %q missing from JSON output", k) + } + } + // Nested reachability: spans[0].name + spans, ok := got["spans"].([]any) + if !ok || len(spans) == 0 { + t.Fatalf("spans not reachable") + } + first, ok := spans[0].(map[string]any) + if !ok || first["name"] == nil { + t.Errorf("spans[0].name not reachable: %v", spans[0]) + } +} + +// TestTracesGet_NotFound_ExitCode3 — 404 → ExitNotFound. +func TestTracesGet_NotFound_ExitCode3(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + errJSON(t, w, http.StatusNotFound, "NOT_FOUND", "trace not found") + })) + defer srv.Close() + t.Setenv("GOCLAW_SERVER", srv.URL) + t.Setenv("GOCLAW_TOKEN", "test-token") + + err := runCmd(t, "traces", "get", "doesnotexist", "--output", "json") + if err == nil { + t.Fatal("expected error, got nil") + } + if code := output.FromError(err); code != output.ExitNotFound { + t.Errorf("exit code = %d, want %d (ExitNotFound)", code, output.ExitNotFound) + } + if !strings.Contains(strings.ToLower(err.Error()), "not found") { + t.Errorf("error message should mention 'not found': %q", err.Error()) + } +} + +// TestTracesGet_PermissionDenied_ExitCode2 — 403 → ExitAuth. +func TestTracesGet_PermissionDenied_ExitCode2(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + errJSON(t, w, http.StatusForbidden, "TENANT_ACCESS_REVOKED", "tenant access revoked") + })) + defer srv.Close() + t.Setenv("GOCLAW_SERVER", srv.URL) + t.Setenv("GOCLAW_TOKEN", "test-token") + + err := runCmd(t, "traces", "get", "trace_FIXTURE_001", "--output", "json") + if err == nil { + t.Fatal("expected error, got nil") + } + if code := output.FromError(err); code != output.ExitAuth { + t.Errorf("exit code = %d, want %d (ExitAuth)", code, output.ExitAuth) + } +} + +// TestTracesGet_MalformedID_NoHTTPCall — id validation runs before HTTP; exit 4. +func TestTracesGet_MalformedID_NoHTTPCall(t *testing.T) { + var calls int64 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt64(&calls, 1) + okJSON(t, w, map[string]any{}) + })) + defer srv.Close() + t.Setenv("GOCLAW_SERVER", srv.URL) + t.Setenv("GOCLAW_TOKEN", "test-token") + + bad := []string{"", " ", "..", ".", "../etc/passwd", "a/b", "a\\b", "a\x00b", "a b"} + for _, id := range bad { + t.Run("id="+strings.ReplaceAll(id, "\x00", "NUL"), func(t *testing.T) { + before := atomic.LoadInt64(&calls) + err := runCmd(t, "traces", "get", id, "--output", "json") + if err == nil { + t.Fatalf("expected validation error for %q, got nil", id) + } + if code := output.FromError(err); code != output.ExitValidation { + t.Errorf("id=%q: exit = %d, want %d (ExitValidation)", id, code, output.ExitValidation) + } + if got := atomic.LoadInt64(&calls); got != before { + t.Errorf("id=%q: HTTP call made (calls %d -> %d); should be blocked client-side", id, before, got) + } + }) + } +} + +// TestTracesGet_ServerError_ExitCode5 — 5xx → ExitServer; client retries so calls >= 1. +func TestTracesGet_ServerError_ExitCode5(t *testing.T) { + if testing.Short() { + t.Skip("skipping in -short: HTTP client backs off ~3s between 5xx retries") + } + var calls int64 + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + atomic.AddInt64(&calls, 1) + errJSON(t, w, http.StatusInternalServerError, "INTERNAL", "boom") + })) + defer srv.Close() + t.Setenv("GOCLAW_SERVER", srv.URL) + t.Setenv("GOCLAW_TOKEN", "test-token") + + err := runCmd(t, "traces", "get", "trace_FIXTURE_001", "--output", "json") + if err == nil { + t.Fatal("expected error, got nil") + } + if code := output.FromError(err); code != output.ExitServer { + t.Errorf("exit code = %d, want %d (ExitServer)", code, output.ExitServer) + } + if n := atomic.LoadInt64(&calls); n < 1 { + t.Errorf("calls = %d, want >= 1 (client retries 5xx)", n) + } +} + +// TestTracesGet_MalformedResponse_SurfacesError — bad JSON body → wrapped decode error. +func TestTracesGet_MalformedResponse_SurfacesError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte("this is not json")) + })) + defer srv.Close() + t.Setenv("GOCLAW_SERVER", srv.URL) + t.Setenv("GOCLAW_TOKEN", "test-token") + + out, err := captureStdout(t, func() error { + return runCmd(t, "traces", "get", "trace_FIXTURE_001", "--output", "json") + }) + if err == nil { + t.Fatal("expected decode error, got nil") + } + msg := strings.ToLower(err.Error()) + if !strings.Contains(msg, "decode") && !strings.Contains(msg, "unmarshal") && !strings.Contains(msg, "invalid") { + t.Errorf("error should mention decode/unmarshal/invalid; got: %q", err.Error()) + } + if strings.TrimSpace(out) == "{}" { + t.Errorf("stdout is empty JSON object — silent failure regressed: %q", out) + } +} diff --git a/docs/codebase-summary.md b/docs/codebase-summary.md index 297a1d4..199e13c 100644 --- a/docs/codebase-summary.md +++ b/docs/codebase-summary.md @@ -316,7 +316,7 @@ goclaw (root) │ ├── workspace (list, read, delete, upload, move) │ └── attachments download --output ├── channels (list, contacts, pending-messages) -├── traces (list, export) +├── traces (list, get, export, follow) # `get` validates id allowlist, renders header+span-tree+events for TTY, JSON for piped/`-o json` ├── memory (list, search, upsert) ├── knowledge-graph (entities, links, query) ├── usage (summary, detail, costs, timeseries, breakdown) diff --git a/internal/client/http.go b/internal/client/http.go index ad09ddd..b765b63 100644 --- a/internal/client/http.go +++ b/internal/client/http.go @@ -161,10 +161,16 @@ func (c *HTTPClient) do(method, path string, body any) (json.RawMessage, error) if resp.StatusCode != 429 && resp.StatusCode < 500 { break } - resp.Body.Close() - if attempt < 2 { - time.Sleep(time.Duration(1< | ----> | newHTTP().Get | ----> | GET /v1/... | +| (cobra RunE) | | (envelope) | | { ok, payload}| ++--------------------+ +----------------+ +--------------+ + | | + v v + unmarshalMap(data) (silent err swallow) <-- confirmed bug + | + v + printer.Print(map) + | + v + table -> JSON fallback <-- "unusable output" symptom confirmed +``` + +**Investigation matrix** (each row = a probable failure mode): + +| # | Hypothesis | Evidence | Disposition | +|---|---|---|---| +| H1 | Table mode dumps raw JSON, looks "broken" to user | VERIFIED via [internal/output/output.go:30-37](internal/output/output.go:30) — JSON fallback | CLI-side; Phase 2 fixes render | +| H2 | `unmarshalMap` swallows error on malformed payload | VERIFIED via [cmd/helpers.go:48-53](cmd/helpers.go:48) — `_ = json.Unmarshal` | CLI-side; Phase 2 fixes | +| H3 | No `url.PathEscape` and no id-validation in `tracesGetCmd` | VERIFIED via [cmd/traces.go:61-75](cmd/traces.go:61) | CLI-side; Phase 2 adds validation | +| H4 | Server returns trace under unexpected envelope key | UNVERIFIED — capture in smoke probe | Determines Phase-2 unmarshal path | +| H5 | Server returns 404 for tenant-scoped trace IDs (auth filter) | UNVERIFIED — capture in smoke probe | Determines whether AC#3 needs server-side fix | + +## Related Code Files + +- Read only: `cmd/traces.go`, `cmd/helpers.go`, `internal/output/output.go`, `internal/output/exit.go`, `internal/client/http.go`. +- Reference: `cmd/traces_follow_test.go` (for `runCmd` + `okJSON` patterns), `cmd/testdata/` (will be created — no prior fixtures exist in repo). +- Create: `cmd/traces_get_test.go` — 3 red tests. +- Create: `cmd/testdata/trace_detail_get.json` — captured fixture (scrubbed). +- Create: `plans/260528-1357-fix-trace-details-by-id/reports/repro-260528-issue17.md` — smoke report with curl command, response sample (scrubbed), classification verdict. + +## Implementation Steps + +### 1. Smoke probe against live gateway + +Use `goclaw.zuey.me` or local dev gateway. **Token MUST come from env, never inline:** + +```bash +export TOKEN="$(goclaw config get token 2>/dev/null || cat ~/.goclaw/token)" +export SERVER="https://goclaw.zuey.me" # or local +# pick a trace id +goclaw traces list --limit 5 -o json | jq -r '.payload[0].trace_id' > /tmp/trace_id +TRACE_ID="$(cat /tmp/trace_id)" +# capture wire-level shape +curl -sS -H "Authorization: Bearer $TOKEN" "$SERVER/v1/traces/$TRACE_ID" > /tmp/trace_raw.json +# capture CLI shape (both modes) +goclaw traces get "$TRACE_ID" -o json > /tmp/trace_json.txt +goclaw traces get "$TRACE_ID" > /tmp/trace_tty.txt # demonstrates the "unusable" output +# capture error envelopes +goclaw traces get bogus-id-12345 -o json > /tmp/trace_404.txt 2>&1 +``` + +**Capture in the repro report:** +- Server response (scrubbed) +- CLI table-mode output (the visible bug) +- CLI JSON-mode output +- 404 error envelope shape — note the exact `error.code` string (e.g. `NOT_FOUND`, `TRACE_NOT_FOUND`, `RESOURCE_NOT_FOUND` — whichever the server actually returns). +- If reachable, also probe `permission denied` by attempting a trace from another tenant (server returns 403 + some code — capture the literal `error.code` string). + +**Gateway unreachable escape:** if no gateway is reachable, fall back to a hand-crafted minimal fixture derived from `traces follow` response shape ([cmd/traces_follow_test.go](cmd/traces_follow_test.go)). Mark the fixture and the report with `TODO: refresh after live smoke probe`. Phase 2 must refresh before merge. + +### 2. Save fixture (scrubbed) + +Strip secrets and PII from `/tmp/trace_raw.json` with this concrete `jq` recipe (extend as needed per real shape): + +```bash +jq ' + walk( + if type == "object" then + with_entries( + if .key | test("token|secret|api_key|authorization"; "i") then + .value = "REDACTED" + elif .key == "user_id" then .value = "user_REDACTED" + elif .key == "tenant_id" then .value = "tenant_REDACTED" + elif .key == "trace_id" then .value = "trace_FIXTURE_001" + elif .key == "agent_id" then .value = "agent_FIXTURE_001" + elif .key == "session_key" then .value = "session_FIXTURE_001" + elif .key | test("email|phone|address"; "i") then .value = "REDACTED" + elif .key | test("content|message|prompt|response|tool_args|tool_result|query"; "i") then .value = "REDACTED" + else . + end + ) + else . + end + ) +' /tmp/trace_raw.json > cmd/testdata/trace_detail_get.json +``` + +After writing, **manually scan** `cmd/testdata/trace_detail_get.json` for any residual: +- bearer tokens (`grep -i 'eyJ\|Bearer\|sk-\|token=' cmd/testdata/trace_detail_get.json` — must return 0 lines) +- numeric ids that look like real user/tenant ids +- span names that embed user input (sanitize manually if so) + +### 3. Write red repro tests + +In `cmd/traces_get_test.go` — only THREE tests in this phase (the rest moves to Phase 2): + +```go +// 1. Path/method correctness — should already pass; documents the contract. +func TestTracesGet_PathAndMethod(t *testing.T) { ... } + +// 2. JSON happy path with fixture — should already pass; locks the envelope shape. +func TestTracesGet_HappyPath_JSON_LocksFixture(t *testing.T) { ... } + +// 3. Table mode renders raw JSON, not human-readable — RED (this is the bug). +func TestTracesGet_TableMode_HumanReadable_RED(t *testing.T) { + // serve fixture; runCmd with "--output", "table" + // assert stdout does NOT start with `{` (would indicate JSON fallback) + // assert stdout contains human-readable markers like "TRACE_ID" or tree markers +} +``` + +Test scaffolding pattern (per repo convention from `cmd/traces_follow_test.go`): +- `httptest.NewServer` + `okJSON(t, w, payload)` envelope helper +- `t.Setenv("GOCLAW_API_URL", srv.URL)` and `t.Setenv("GOCLAW_TOKEN", "test-token")` +- `runCmd(t, "traces", "get", traceID, "--output", "table")` — **explicit `--output` flag** because `go test` pipes stdout (default would resolve to JSON, defeating the table-mode test). +- Each test calls `runCmd` directly; cobra persistent-flag state is reset by passing the flag every time. No `resetTracesGetFlags` helper needed. + +### 4. Run tests; expect TestTracesGet_TableMode_HumanReadable_RED to fail + +```bash +go test -count=1 ./cmd/... -run TestTracesGet +go vet ./... +go build ./... +``` + +Tests 1 and 2 should pass (path/method/JSON envelope are already correct). Test 3 should fail with a message like `stdout starts with '{', expected human-readable output`. + +### 5. Write `reports/repro-260528-issue17.md` + +Sections required: +- **Curl command** (with `$TOKEN` env var, no inline secret). +- **Response sample** — quote 10-30 lines of the scrubbed fixture. +- **Test result summary** — which red, which already green. +- **Classification: CLI-side / server-side / hybrid** with `file:line` evidence for each defect. +- **Server error-code findings** — literal strings observed for 404 and (if probed) 403. +- **If server-side root cause found:** drafted upstream issue body for `digitopvn/goclaw` (also scrubbed; reuse same `jq` recipe). + +## Success Criteria + +- [ ] `cmd/traces_get_test.go` exists with 3 test cases. +- [ ] `cmd/testdata/trace_detail_get.json` captures real server envelope (or, if gateway unreachable, a stand-in marked `TODO: refresh after live smoke probe`). +- [ ] No bearer token, no real user/tenant id, no PII in the committed fixture (`grep` verified). +- [ ] `go test -count=1 ./cmd/...` runs without compile errors; `TestTracesGet_TableMode_HumanReadable_RED` fails with expected assertion message. +- [ ] `reports/repro-260528-issue17.md` exists with: curl command (env-var token), response sample (scrubbed), classification verdict, file:line evidence, observed server error-code strings. +- [ ] No edits to `cmd/traces.go` or any production code. + +## Risk Assessment + +- **Risk:** no access to live gateway during planning. **Mitigation:** fixture stand-in marked `TODO: refresh`; Phase 2 must refresh before merge. +- **Risk:** secrets leak into fixture despite `jq` recipe. **Mitigation:** post-scrub grep check listed in step 2; fail-loud during code review subagent in Phase 3. +- **Risk:** issue reporter's actual repro depends on a specific id that no longer exists. **Mitigation:** test against any valid id from `traces list`; if every id 404s for the reporter, that's the root cause and gets captured. + +## Security Considerations + +- **Smoke probe uses real auth token.** Token MUST come from env var, never inlined into report or fixture. Shell history risk: prefix probe commands with leading space (`HISTCONTROL=ignorespace`) or run from a subshell that won't persist history. +- **Scrub recipe** uses `jq walk` to recursively redact by key-name allowlist; `grep` post-check enforces zero residual secrets. +- **Issue body to upstream** (if filed) reuses the same scrubbed fixture — never the raw one. + +## Next Steps + +Phase 2 picks up the red test (`TestTracesGet_TableMode_HumanReadable_RED`), adds 7 more tests for the remaining acceptance criteria, then implements until green. diff --git a/plans/260528-1357-fix-trace-details-by-id/phase-02-cli-fix-output-and-error-mapping.md b/plans/260528-1357-fix-trace-details-by-id/phase-02-cli-fix-output-and-error-mapping.md new file mode 100644 index 0000000..14515cc --- /dev/null +++ b/plans/260528-1357-fix-trace-details-by-id/phase-02-cli-fix-output-and-error-mapping.md @@ -0,0 +1,255 @@ +--- +phase: 2 +title: 'CLI fix: output and error mapping' +status: completed +priority: P2 +effort: 2.5-3h +dependencies: + - 1 +--- + +# Phase 2: CLI fix: output and error mapping + +## Overview + +Turn the red test from Phase 1 green. Add 7 more tests (rendering, error categories, malformed-id validation). Then implement: human-readable render path inline in `tracesGetCmd.RunE`, checked unmarshal, strict id validation (blocks path traversal), and reliance on existing HTTP-status fallback for the four AC#3 error categories. + +## Locked decisions inherited from plan.md + +- **No `MapServerCode` extension.** 404→ExitNotFound, 403→ExitAuth, 5xx→ExitServerError already resolve via [cmd/helpers.go:152-172](cmd/helpers.go:152) → [internal/output/exit.go:46+](internal/output/exit.go:46) HTTP-status fallback. +- **Inline render in `cmd/traces.go`**, no `cmd/traces_render.go`. Cap render at ~50 LOC. +- **Inline `json.Unmarshal` + error check.** No new helper. +- **Strict id allowlist:** `^[A-Za-z0-9._-]+$` AND non-empty AND not `.` AND not `..`. Then `url.PathEscape` on top. +- **5xx test allows `calls >= 1`** because `internal/client/http.go` retries 3x. + +## Requirements + +**Functional** +- Table/TTY mode renders a structured trace summary: header card (trace_id, agent_id, status, duration, tokens, cost — whichever Phase-1 fixture confirms exists) + span tree (via `output.PrintTree`) + flat events list (`EVENTS (n=N):` header + one line per event, no truncation). +- JSON/YAML mode emits the decoded payload via the existing printer path — no field dropping by us. (Note: `json.NewEncoder` re-encodes with HTML escape + key reorder; "preserves nested fields" means structural preservation, not byte-for-byte.) +- Empty/malformed server response returns a wrapped error, not a silent empty `{}`. +- Malformed id (empty, whitespace, `..`, `/`, `\\`, control char, non-allowlist) returns validation error BEFORE the HTTP call. Exit 4. +- 4 error categories produce distinguishable messages and exit codes per AC#3 via existing HTTP-status fallback. + +**Non-functional** +- All render logic stays inside `tracesGetCmd.RunE` (~50 LOC). No new file. Pattern matches `tracesListCmd` ([cmd/traces.go:30-59](cmd/traces.go:30)). +- Reuse `output.PrintTree` / `output.TreeNode` ([internal/output/tree.go:7](internal/output/tree.go:7)); no parallel tree renderer. +- No new dependencies. + +## Architecture + +``` +goclaw traces get + | + v +[validate id: regex allowlist + reject ./../ /] + |--invalid--> return validation error -> exit 4 + v +GET /v1/traces/{url.PathEscape(id)} + | + +-- APIError? ---> central handler via output.FromError + | | + | +-- StatusCode 404 -> exit 3 (via MapHTTPStatus) + | +-- StatusCode 403 -> exit 2 (via MapHTTPStatus) + | +-- StatusCode 5xx -> exit 5 (via MapHTTPStatus, after 3 retries) + | +-- StatusCode 400 -> exit 4 (server validation, if ever returned) + | + v +json.Unmarshal(data, &trace) — checked, NOT silent + |--err--> wrap "decode trace payload" -> exit 5 + v +switch cfg.OutputFormat: + json|yaml -> printer.Print(trace) + table -> inline render: header lines + PrintTree(spans) + events list +``` + +## Related Code Files + +- Modify: `cmd/traces.go` — `tracesGetCmd.RunE` rewritten with validation, checked unmarshal, inline render. Net add ~50-70 LOC. +- Modify: `cmd/traces_get_test.go` — add 7 new tests on top of Phase 1's 3. +- No new files. +- No changes to `internal/output/exit.go` or `internal/output/error.go`. + +## Implementation Steps + +### 1. Extend test file (red first) + +Add these 7 tests to `cmd/traces_get_test.go`: + +```go +// 4. Table render must include header card + span markers. +func TestTracesGet_TableMode_HasHeaderAndSpanMarkers(t *testing.T) { + // serve fixture; runCmd with --output table + // assert stdout contains "TRACE_ID" header AND tree marker ("├─" or "└─" or "└" or "├") + // assert stdout does NOT start with '{' +} + +// 5. JSON mode preserves structural keys observed in fixture. +func TestTracesGet_JSONMode_PreservesStructure(t *testing.T) { + // serve fixture; runCmd with --output json + // parse stdout as JSON; assert every top-level key present in fixture is present in output + // assert nested key (e.g. spans[0].name) reachable +} + +// 6. 404 → exit 3 + message contains "not found". +func TestTracesGet_NotFound_ExitCode3(t *testing.T) { + // server returns 404 + {"ok":false, "error":{"code":"", "message":"trace not found"}} + // assert err non-nil; output.FromError(err) == output.ExitNotFound (3) + // assert err.Error() contains "not found" (case-insensitive) +} + +// 7. 403 → exit 2. +func TestTracesGet_PermissionDenied_ExitCode2(t *testing.T) { + // server returns 403; assert output.FromError(err) == output.ExitAuth (2) +} + +// 8. Malformed id rejected client-side; no HTTP call. +func TestTracesGet_MalformedID_NoHTTPCall(t *testing.T) { + // record calls counter on httptest server + // try ids: "", " ", "..", "../etc/passwd", "a/b", "a\\b", "a\x00b" + // each should: return err, exit code 4, AND not increment calls +} + +// 9. 5xx → exit 5 (allow retries; do not assert call count). +func TestTracesGet_ServerError_ExitCode5(t *testing.T) { + // server returns 500 always + // assert err non-nil; output.FromError(err) == output.ExitServerError (5) + // assert calls counter >= 1 (NOT == 1; retry adds calls) +} + +// 10. Malformed JSON response surfaces decode error, not silent empty. +func TestTracesGet_MalformedResponse_SurfacesError(t *testing.T) { + // server returns 200 + body "this is not json" + // assert err non-nil; err.Error() contains "decode" or "unmarshal" + // assert stdout is empty or contains the error, NOT a literal "{}" +} +``` + +**Test harness rules:** +- Every table-mode test passes `--output table` explicitly. +- Every JSON-mode test passes `--output json` explicitly. +- Use `httptest.NewServer` + `okJSON(t, w, payload)` envelope helper. +- Use `t.Setenv` to point CLI at the test server. + +### 2. Implement `tracesGetCmd.RunE` rewrite + +Replace [cmd/traces.go:61-75](cmd/traces.go:61) with (sketch): + +```go +var tracesGetCmd = &cobra.Command{ + Use: "get ", + Short: "Get trace with span tree", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + id := strings.TrimSpace(args[0]) + if err := validateTraceID(id); err != nil { + return err // returns ExitValidation via output.FromError + } + c, err := newHTTP() + if err != nil { + return err + } + data, err := c.Get("/v1/traces/" + url.PathEscape(id)) + if err != nil { + return err // APIError -> central handler maps via HTTP status + } + var trace map[string]any + if err := json.Unmarshal(data, &trace); err != nil { + return fmt.Errorf("decode trace payload: %w", err) + } + format := output.ResolveFormat(cfg.OutputFormat) + if format == "json" || format == "yaml" { + printer.Print(trace) + return nil + } + return renderTraceTable(trace) // inline below in same file + }, +} + +// validateTraceID enforces the strict allowlist (rejects path-traversal etc.) +func validateTraceID(id string) error { + if id == "" || id == "." || id == ".." { + return validationErr("trace id is empty or invalid") + } + if !traceIDRegex.MatchString(id) { + return validationErr("trace id contains invalid characters (allowed: A-Z a-z 0-9 . _ -)") + } + return nil +} +var traceIDRegex = regexp.MustCompile(`^[A-Za-z0-9._-]+$`) + +// renderTraceTable prints header + span tree + events. ~30-40 LOC. +func renderTraceTable(t map[string]any) error { + // defensive helpers — define inline or in cmd/helpers.go + s := func(k string) string { v, _ := t[k].(string); return v } + // header lines + fmt.Printf("TRACE_ID: %s\nAGENT_ID: %s\nSTATUS: %s\n", s("trace_id"), s("agent_id"), s("status")) + // span tree via output.PrintTree — adapt spans field per Phase-1 fixture shape + if spans, ok := t["spans"].([]any); ok && len(spans) > 0 { + root := buildSpanTree(spans) // helper, ~15 LOC + fmt.Println() + output.PrintTree(root, "") + } else { + fmt.Println("(no spans)") + } + // events flat list + if evs, ok := t["events"].([]any); ok { + fmt.Printf("\nEVENTS (n=%d):\n", len(evs)) + for _, e := range evs { + if m, ok := e.(map[string]any); ok { + fmt.Printf(" - %v\n", m["type"]) // or whatever key Phase-1 fixture confirms + } + } + } + return nil +} +``` + +**`validationErr` helper**: produce an error that `output.FromError` maps to `ExitValidation` (4). If no such helper exists today, add a 2-line one (e.g. wrap in `APIError{Code: "INVALID_REQUEST"}` since `MapServerCode` covers that → exit 4). Verify via Phase 1 reading of `internal/output/exit.go` what the simplest path is. + +**`buildSpanTree`**: only define IF Phase 1 fixture confirms spans are hierarchical or have `parent_id`. If flat with no parent relationship, just render a flat list and skip the tree. **Do not guess the shape; let the fixture drive it.** + +### 3. Run tests until green + +```bash +go test -count=1 ./cmd/... -run TestTracesGet +go vet ./... +go build ./... +``` + +### 4. Live smoke (manual) + +Re-run the four `goclaw traces get` invocations from Phase 1's smoke probe. Confirm: +- TTY: readable header + span tree + events +- `-o json`: round-trips through `jq` with all fields intact +- 404 id: exit 3, message contains "not found" +- 403 id (if reachable): exit 2 + +## Success Criteria + +- [ ] All 3 Phase-1 tests + 7 Phase-2 tests pass under `go test -count=1`. +- [ ] `go vet ./...` clean. +- [ ] `go build ./...` clean. +- [ ] `tracesGetCmd.RunE` + `renderTraceTable` + `validateTraceID` + `buildSpanTree` together ≤ 100 LOC of new code in `cmd/traces.go` (excluding doc comments). +- [ ] Manual smoke: `goclaw traces get ` in TTY renders summary + span tree + events; `-o json` round-trips through `jq`. +- [ ] `output.FromError` returns 0/2/3/4/5 correctly for the categories per test assertions. +- [ ] Path-traversal attempt (`goclaw traces get ../etc/passwd`) returns exit 4 without making an HTTP request. +- [ ] No plan-artifact references in production or test code (no "Phase 2", finding codes, etc.). + +## Risk Assessment + +- **Risk:** Phase-1 fixture reveals a span shape that doesn't match the hierarchical-or-flat assumption. **Mitigation:** `renderTraceTable` degrades gracefully (`(no spans)` line) if structure is unrecognized. Tree renderer only invoked if shape is recognizable. +- **Risk:** server returns a 4xx that isn't 400/403/404 (e.g. 422). **Mitigation:** `MapHTTPStatus` likely already covers it via `4xx → ExitGeneric` or similar; verify in Phase 1 step 4 reading of `cmd/helpers.go:152-172`. +- **Risk:** `renderTraceTable` becomes a kitchen-sink. **Mitigation:** ≤100 LOC cap; defer richer rendering (timeline, ASCII chart, color) to a future enhancement. + +## Security Considerations + +- **Path traversal blocked client-side** via `validateTraceID` allowlist. Even if a downstream proxy mishandles encoded `..`, the CLI refuses to send it. +- **ANSI escape injection via span names.** Span `name` fields may contain LLM/tool output — potentially attacker-controlled. Current `traces list` ([cmd/traces.go:30-59](cmd/traces.go:30)) already renders user-controlled fields (`agent_id`, `status`) without sanitization, so structural parity exists. Span/event payloads are a **materially higher risk surface** because content is less constrained. Accepted-trade-off OR add `strings.Map(safeRune, v)` filter — **decide based on Phase-1 fixture inspection** (does the fixture have any non-printable bytes in span names?). If yes, add the filter; if no, accept and document. +- **404/403 existence oracle.** AC#3 explicitly requires the distinction. Trade-off accepted, documented here. Mitigation belongs to the server (could randomize a small delay on 404 to defeat timing oracle), not the CLI. +- **JSON full-payload exposure.** If server payload includes internal fields, `-o json` now shows them where table mode used to hide them via the broken render. This is correct behavior, not a regression — issue #17 explicitly wants both modes working. Documented. +- **No new auth surface.** Existing `newHTTP()` token handling unchanged. + +## Next Steps + +Phase 3 updates docs and ships. diff --git a/plans/260528-1357-fix-trace-details-by-id/phase-03-docs-and-ship.md b/plans/260528-1357-fix-trace-details-by-id/phase-03-docs-and-ship.md new file mode 100644 index 0000000..685c4f8 --- /dev/null +++ b/plans/260528-1357-fix-trace-details-by-id/phase-03-docs-and-ship.md @@ -0,0 +1,97 @@ +--- +phase: 3 +title: "Docs and ship" +status: pending +priority: P2 +effort: "45m-1h" +dependencies: [2] +--- + +# Phase 3: Docs and ship + +## Overview + +Update CHANGELOG, file upstream issue ONLY if Phase 1 classified server-side root cause, then ship via `/ck:ship beta` (target: `dev`). Closes issue #17. + +## Requirements + +**Functional** +- CHANGELOG entry under `[Unreleased]` documenting the fix. +- README `traces get` example updated IF output behavior materially changed (likely yes — TTY mode is new). +- `docs/codebase-summary.md` reflects new id-validation + render path IF non-trivial (likely a one-line bullet). +- Upstream issue filed at `digitopvn/goclaw` IFF Phase 1 classification was server-side or hybrid. PR body links it. +- `gh issue close 17` runs after the dev→main promotion merges (or auto-closes via `Closes #17` in the promotion PR). + +**Non-functional** +- Commit message follows conventional commits (`fix(cli): ...`). +- PR body references `Closes #17` (will auto-close only when promoted to default branch), links Phase-1 repro report, lists acceptance-criteria mapping. + +## Architecture + +No code changes — docs + ship pipeline only. + +## Related Code Files + +- Modify: `CHANGELOG.md` — add entry under existing `[Unreleased]` block (append to P0–P6 section if present). +- Modify: `README.md` — refresh `traces get` example block. +- Modify: `docs/codebase-summary.md` — one bullet on the new traces-get behavior. +- Modify: `plans/260528-1357-fix-trace-details-by-id/plan.md` — flip phase statuses via `ck plan check` (not hand-edit). +- No source code edits. + +## Implementation Steps + +1. **Sync plan status via CLI** — `cd plans/260528-1357-fix-trace-details-by-id && ck plan check 1 && ck plan check 2 && ck plan check 3 --start`. (Phase 3 marked in-progress while shipping; flip to completed at the end via `ck plan check 3`.) + +2. **CHANGELOG** — add under `[Unreleased]`: + ``` + ### Fixed + - `goclaw traces get ` — human-readable rendering for TTY mode (header + span tree + events), checked unmarshal (no more silent empty `{}`), strict id validation (rejects path-traversal), and distinct exit codes for not-found (3) / permission-denied (2) / malformed-id (4) / server-failure (5). Closes issue #17. + ``` + If Phase 1 found a server-side root cause, add a `### Notes` line linking the upstream issue URL. + +3. **README** — locate `traces get` example. If absent, add to traces section showing expected human + JSON output samples (use scrubbed Phase-1 fixture as the JSON sample). + +4. **codebase-summary.md** — add bullet under traces/output section noting validated id + structured render. + +5. **Upstream issue (conditional)** — only if root cause was server-side or hybrid: + - Check rights first: `gh api repos/digitopvn/goclaw -q .permissions`. If no issue-create permission, instead post a comment on issue #17 with the drafted body for the maintainer to file. + - If rights present: `gh issue create -R digitopvn/goclaw --title "..." --body "..."` using the scrubbed body drafted in Phase 1's repro report. + - Add the issue URL to CHANGELOG `### Notes`. + +6. **Code review** — spawn `code-reviewer` subagent on the diff before ship. Reviewer must explicitly verify: + - No bearer tokens or PII in committed fixture/report (`grep -i 'eyJ\|Bearer\|sk-' cmd/testdata/` returns 0). + - All 10 trace-get tests pass. + - `cmd/traces.go` net add ≤ 100 LOC. + - No plan-artifact references ("Phase 2", "F1", etc.) in production/test code. + +7. **Ship** — `/ck:ship beta` (target: `dev` branch). + +8. **Verify PR** — confirm PR body includes `Closes #17`, links Phase-1 repro report, lists acceptance-criteria mapping (1→fix; 2→table+JSON tests; 3→exit-code tests + path-traversal block; 4→upstream link if applicable; 5→`cmd/traces_get_test.go`). + +9. **Issue close after promotion** — PR targets `dev`; auto-close fires only when promoted to default branch. Document this in the issue with a comment: "Fixed in dev via PR #X; will auto-close on next `dev → main` promotion." + +## Success Criteria + +- [ ] CHANGELOG `[Unreleased]` entry present. +- [ ] README `traces get` example reflects new behavior. +- [ ] `docs/codebase-summary.md` updated. +- [ ] Upstream `digitopvn/goclaw` issue filed/commented-for-filing (conditional on Phase 1 verdict). +- [ ] code-reviewer subagent verdict: DONE, 0 Critical, 0 Important. +- [ ] PR created against `dev`, all CI checks green. +- [ ] Plan phases 1, 2, 3 all marked completed via `ck plan check`. +- [ ] Comment posted on issue #17 explaining the dev → main promotion auto-close behavior. + +## Risk Assessment + +- **Risk:** PR auto-close via `Closes #17` only fires on default-branch merge; PR targets `dev` first. **Mitigation:** explicit comment on #17 explaining the timing; manual close after `dev → main` promotion if needed. +- **Risk:** ship pipeline blocks on unrelated CI flakiness. **Mitigation:** investigate per failure; do not bypass with `--skip-tests`. +- **Risk:** no rights on `digitopvn/goclaw`. **Mitigation:** fallback to issue-#17 comment with the drafted body (step 5 above). + +## Security Considerations + +- Fixture/report secret-scrub gate re-checked by code-reviewer subagent (step 6). +- Upstream issue body reuses scrubbed content only. + +## Next Steps + +After merge to `dev`, the natural follow-up is `/ck:ship official` to promote `dev` → `main`. Out of scope for this plan. diff --git a/plans/260528-1357-fix-trace-details-by-id/plan.md b/plans/260528-1357-fix-trace-details-by-id/plan.md new file mode 100644 index 0000000..9258c3d --- /dev/null +++ b/plans/260528-1357-fix-trace-details-by-id/plan.md @@ -0,0 +1,86 @@ +--- +title: 'Fix issue #17: cannot read trace details by trace id' +description: >- + Restore reliable single-trace-by-id reads in goclaw-cli with explicit error + categorization, human-readable rendering, and regression tests. TDD mode. +status: pending +priority: P2 +branch: worktree-fix-trace-details-by-id +tags: + - bugfix + - traces + - ai-ergonomics + - tdd +blockedBy: [] +blocks: [] +created: '2026-05-28T06:58:02.755Z' +createdBy: 'ck:plan' +source: skill +--- + +# Fix issue #17: cannot read trace details by trace id + +## Overview + +`goclaw traces get ` exists ([cmd/traces.go:61](cmd/traces.go:61)) and calls `GET /v1/traces/{traceID}`, but the issue reporter observed that detail lookup "does not work reliably" with "unusable output". Verified CLI-side defects from scout: + +1. **No human-readable render path.** `tracesGetCmd` calls `printer.Print(unmarshalMap(data))`. In table mode, `output.Printer.Print` falls back to JSON dump ([internal/output/output.go:30-37](internal/output/output.go:30)) because the trace payload is a `map[string]any`, not a `*TableData`. Users in a TTY see an unindented blob. +2. **Silent unmarshal failures.** `unmarshalMap` discards `json.Unmarshal` errors ([cmd/helpers.go:48-53](cmd/helpers.go:48)) — a malformed/empty server response renders as empty `{}`. +3. **Path-traversal exposure.** `tracesGetCmd` concatenates `args[0]` into the URL with no `url.PathEscape` and no allowlist; `url.PathEscape("..")` returns literal `..`. Bad ids reach the server. +4. **No regression test.** `cmd/traces_get_test.go` does not exist. + +Plus one acceptance gap: +- No client-side categorization between not-found / permission-denied / malformed-id / server-failure. The exit-code-mapping infrastructure already exists ([internal/output/exit.go:18-44](internal/output/exit.go:18)): `MapServerCode` covers `NOT_FOUND`→3, `TENANT_ACCESS_REVOKED`→2, `INVALID_REQUEST`→4, `INTERNAL`/5xx→5. `cmd/helpers.go:152-172` derives codes from raw HTTP status as a fallback. **No `MapServerCode` extension needed.** + +Server-vs-CLI root cause is most likely CLI-side per the above, but Phase 1 captures a real response shape against a live gateway and confirms the failure mode before Phase 2 implementation. + +**Acceptance criteria** (verbatim from [issue #17](https://github.com/nextlevelbuilder/goclaw-cli/issues/17)): +1. `goclaw-cli` can read trace details by id. +2. Output works in JSON and human-readable modes. +3. Errors clearly distinguish: not found, permission denied, malformed id, and server/API failure. +4. If API is the root cause, document/link the server-side issue in `digitopvn/goclaw`. +5. Add regression test or smoke test for trace detail lookup. + +**Out of scope (verbatim from invocation):** +- Redesigning the traces domain. +- WS streaming for trace events. +- Anything beyond the single-trace-by-id read path. + +## Locked decisions (from red-team review 2026-05-28) + +These supersede earlier drafts and are not re-litigated during implementation: + +- **No `MapServerCode` extension.** HTTP-status fallback at `cmd/helpers.go:152-172` already covers 403→ExitAuth, 404→ExitNotFound, 5xx→ExitServerError. Server-code names are NOT speculated in tests. +- **Inline render, no `cmd/traces_render.go`.** Follows the established pattern of `tracesListCmd` ([cmd/traces.go:30-59](cmd/traces.go:30)). Cap render code at ~50 LOC inside `tracesGetCmd.RunE`. +- **Inline `json.Unmarshal` with error check.** No new `unmarshalMapStrict` helper. Pattern: `var trace map[string]any; if err := json.Unmarshal(data, &trace); err != nil { return fmt.Errorf("decode trace payload: %w", err) }`. +- **Strict id validation.** Reject empty/whitespace, `..`, leading/trailing `/`, embedded `/`, `\\`, control chars. Use a small allowlist regex (e.g. `^[A-Za-z0-9._-]+$`) AND `len > 0` AND not equal to `.` or `..`. Then `url.PathEscape` on top. +- **Test harness forces table mode explicitly.** Every table-mode test in `cmd/traces_get_test.go` passes `--output table` via `runCmd(t, "traces", "get", id, "--output", "table")` — `go test` stdout is piped, so default would be JSON. +- **Span tree IS in scope.** The command's existing `Short` description says "Get trace with span tree" ([cmd/traces.go:62](cmd/traces.go:62)) and AC #2 requires human-readable mode. Reuse `output.PrintTree` / `output.TreeNode` ([internal/output/tree.go:7](internal/output/tree.go:7)). No new tree renderer. +- **Events: simple flat list, no truncation.** Print `EVENTS (n=N):` header then one line per event. No first-5-last-1 polish. +- **5xx auto-retry awareness.** `internal/client/http.go` retries 3x on 5xx; Phase-2 5xx tests assert `calls >= 1` (not equal) and verify exit-code-5, not call count. + +## Phases + +| Phase | Name | Status | +|-------|------|--------| +| 1 | [Repro and root cause](./phase-01-repro-and-root-cause.md) | Completed | +| 2 | [CLI fix: output and error mapping](./phase-02-cli-fix-output-and-error-mapping.md) | Completed | +| 3 | [Docs and ship](./phase-03-docs-and-ship.md) | Pending | + +## TDD discipline (per `--tdd`) + +Each phase opens with red tests describing the desired behavior, then implementation lands until tests are green. Specifically: +- **Phase 1** — write 3 red tests in `cmd/traces_get_test.go` reproducing the bug against a mock `httptest` server with a realistic envelope; capture the exact payload shape from a live gateway smoke probe (or document gateway-unreachable escape). +- **Phase 2** — extend the test file with cases for human-readable rendering + 4 error categories + path-traversal validation before implementation. +- **Phase 3** — no new tests, only verification + ship. + +## Dependencies + +None. P6 (HEAD = `2801486`) provides all required prerequisite surfaces. + +## Risk and rollback + +- **Risk:** root cause is purely server-side. Mitigation: Phase 1 explicitly classifies CLI vs server; if server, Phase 2 narrows to error-mapping + smoke-test + upstream-issue filing, code change to traces command itself is minimal. +- **Risk:** trace payload shape (spans/events/messages structure) differs from speculation. Mitigation: Phase 1 captures the real fixture FIRST; Phase 2 render code matches the captured shape, not a guess. +- **Risk:** existence oracle from distinct 404 / 403 messages. AC #3 explicitly requires the distinction — accepted trade-off, documented in Phase 2 Security Considerations. +- **Rollback:** all changes confined to `cmd/traces.go` and `cmd/traces_get_test.go`. Single revert. diff --git a/plans/260528-1357-fix-trace-details-by-id/reports/code-review-260528-issue17.md b/plans/260528-1357-fix-trace-details-by-id/reports/code-review-260528-issue17.md new file mode 100644 index 0000000..8285329 --- /dev/null +++ b/plans/260528-1357-fix-trace-details-by-id/reports/code-review-260528-issue17.md @@ -0,0 +1,40 @@ +# Code review — issue #17 fix (pre-ship) + +Date: 2026-05-28 +Phase: 3 (step 6 — pre-ship gate) +Reviewer: `code-reviewer` subagent + +## Verdict + +**DONE_WITH_CONCERNS** — 0 Critical, 0 Important, 4 informational. Ship-ready. + +## Gate checks (all pass) + +| Check | Result | +|------|--------| +| `grep -i 'eyJ\|Bearer\|sk-\|token=' cmd/testdata/` returns 0 lines | ✅ | +| All 10 trace-get tests pass + full repo suite green | ✅ | +| `cmd/traces.go` net add ≤ 100 LOC (advisory) | ⚠️ 107 — accepted trade-off for `buildSpanTree` clarity | +| No plan-artifact refs in source/comments | ✅ | + +## Verified behavior + +- `buildSpanTree` cycle safety: A↔B cycles silently drop (never appear in `children[""]`, so `build()` is never invoked on them). No infinite recursion possible. +- `validateTraceID` gates: 9 malformed-id cases all rejected pre-HTTP. Confirmed by `TestTracesGet_MalformedID_NoHTTPCall` (server hit count = 0 across the table). +- HTTP retry-body fix: `if attempt == 2 { break }` correctly skips the per-iteration `Close()` so the outer `defer resp.Body.Close()` handles cleanup exactly once. No leak, no double-close. +- Error mapping: 404→3, 403→2, 5xx→5, malformed→4 — all asserted, all pass. + +## Informational concerns + +1. **107 LOC vs 100 advisory cap.** Eliminating 7 lines would force inlining `parentOf` or removing the orphan-detection branch — clarity loss > LOC gain. Accept. +2. **Fixture `_TODO_refresh` marker.** Intentional per Phase 1 reviewer gate — refresh against `goclaw.zuey.me` before final merge. +3. **`tracesExportCmd` un-validated id (`cmd/traces.go:207`).** Pre-existing. Same path-traversal/control-char inputs that `validateTraceID` blocks flow through `export` unfiltered. Out of scope here — spawn follow-up task. +4. **README exit-code list missing 6.** Fixed inline (added "rate-limit / network-resource exhaustion → 6"). + +## Recommended actions (all addressed or scheduled) + +| Action | Status | +|--------|--------| +| README exit-6 mention | Fixed inline this turn | +| Fixture refresh against live gateway | Plan-gated; auth-blocked smoke probe documented in Phase 1 repro | +| Harden `tracesExportCmd` with `validateTraceID` + `url.PathEscape` | Spawn follow-up task | diff --git a/plans/260528-1357-fix-trace-details-by-id/reports/red-team-assumption-destroyer.md b/plans/260528-1357-fix-trace-details-by-id/reports/red-team-assumption-destroyer.md new file mode 100644 index 0000000..d3685a7 --- /dev/null +++ b/plans/260528-1357-fix-trace-details-by-id/reports/red-team-assumption-destroyer.md @@ -0,0 +1,110 @@ +# Red-Team: Assumption Destroyer — Issue #17 Plan + +## Verdict: REVISE + +Most structural claims verify, but two findings materially impact the plan: +- The `MapServerCode` table does NOT contain `TRACE_NOT_FOUND` or `PERMISSION_DENIED` today (plan says "may need adding" — it's not "may", it's required). +- The trace detail payload shape (`spans`/`events`/`messages` keys, hierarchical-vs-flat span tree) is purely speculated. No fixture exists in repo. Phase 1 must capture it before Phase 2 can implement `renderTraceDetail`/`buildSpanTree`. + +## Findings + +### Important — `tracesGetCmd` line number is off by 1 +- **Assumption:** "`cmd/traces.go` has `tracesGetCmd` at line 61" +- **Status:** PASS (close enough) +- **Evidence:** `cmd/traces.go:61` reads `var tracesGetCmd = &cobra.Command{` +- **Impact:** None. Plan citation is accurate. + +### Important — `unmarshalMap` silently swallows errors +- **Assumption:** "`unmarshalMap` at `cmd/helpers.go:48-53` silently swallows errors" +- **Status:** PASS +- **Evidence:** `cmd/helpers.go:48-53`: + ```go + func unmarshalMap(data json.RawMessage) map[string]any { + var m map[string]any + _ = json.Unmarshal(data, &m) + return m + } + ``` + The `_ =` discards the error. Same flaw at `unmarshalList` (lines 41-46). +- **Impact:** Confirms Phase 2 step 3 ("Replace silent `unmarshalMap` call with checked unmarshal") is correctly motivated. Note: the same anti-pattern is used across `tracesListCmd`, `usageSummaryCmd`, etc. — fixing only at the `traces get` call site is the minimum fix per the issue scope; the broader code smell is out of scope. + +### Important — Printer table fallback dumps JSON +- **Assumption:** "`printer.Print` falls back to JSON dump in table mode at `internal/output/output.go:30-37`" +- **Status:** PASS +- **Evidence:** `internal/output/output.go:31-37`: + ```go + default: + // Table format requires TableData + if td, ok := data.(*TableData); ok { + p.printTable(td) + } else { + p.printJSON(data) // fallback for non-table data + } + ``` + A `map[string]any` is NOT `*TableData`, so it hits `printJSON`. In a TTY this dumps formatted JSON — useless as a "table". +- **Impact:** Confirms the root-cause hypothesis H1 in Phase 1. + +### Important — `output.PrintTree` / `TreeNode` exist +- **Assumption:** "`output.PrintTree` and `TreeNode` exist at `internal/output/tree.go:7-36` for reuse" +- **Status:** PASS +- **Evidence:** `internal/output/tree.go:8-12` defines `TreeNode{Name, Children}`; `tree.go:16-27` defines `PrintTree(node, w, prefix, isLast)`; `tree.go:30-35` defines `PrintTreeRoot`. +- **Impact:** Plan's reuse plan in Phase 2 step 2 is valid. **However**: `PrintTree` takes an `io.Writer` — plan must remember to pass `os.Stdout` (the existing API doesn't write to stdout by default). + +### Critical — `MapServerCode` does NOT include `TRACE_NOT_FOUND` or `PERMISSION_DENIED` +- **Assumption:** "extend `MapServerCode` if `TRACE_NOT_FOUND` / `PERMISSION_DENIED` not yet mapped" (Phase 2, line 64) +- **Status:** FAIL (semantically — plan hedges "if not yet mapped" but it's NOT mapped; phrasing implies "may" or "may not", which is misleading) +- **Evidence:** `internal/output/exit.go:17-39` `serverCodeMap` contains: `UNAUTHORIZED`, `NOT_PAIRED`, `TENANT_ACCESS_REVOKED`, `NOT_FOUND`, `NOT_LINKED`, `INVALID_REQUEST`, `FAILED_PRECONDITION`, `ALREADY_EXISTS`, `INTERNAL`, `UNAVAILABLE`, `AGENT_TIMEOUT`, `RESOURCE_EXHAUSTED`. **No** `TRACE_NOT_FOUND`. **No** `PERMISSION_DENIED`. Currently both return `ExitGeneric` (1) via `MapServerCode`. +- **Impact:** Phase 2 step 4 must commit to ADDING these codes, not just "confirming". Otherwise the `TestTracesGet_NotFound_ExitCode3` test will fail because the code-string path returns 1, and only the HTTP-status fallback path saves it (404→3 via `MapHTTPStatus` at `exit.go:54`). The `FromError` logic at `error.go:144-154` does fall through to `MapHTTPStatus` when `MapServerCode == ExitGeneric`, so the test will likely pass via the status-code fallback — but the plan should be explicit that the code-string mapping is currently a no-op, and acknowledge that the fallback is what actually saves the assertion. +- **Suggested fix:** Update Phase 2 step 4 from "Confirm `MapServerCode("TRACE_NOT_FOUND")` returns `ExitNotFound`" to "**ADD** `TRACE_NOT_FOUND → ExitNotFound` and `PERMISSION_DENIED → ExitAuth` to `serverCodeMap` in `internal/output/exit.go:17`." Also rerun `internal/output/exit_test.go` after the change. + +### Critical — `TRACE_NOT_FOUND` / `PERMISSION_DENIED` are guessed names, not verified +- **Assumption:** "server returns error codes like `TRACE_NOT_FOUND` and `PERMISSION_DENIED`" +- **Status:** UNVERIFIABLE (zero evidence in repo) +- **Evidence:** `grep -rn "TRACE_NOT_FOUND\|PERMISSION_DENIED"` across `/internal/`, `/cmd/`, `/docs/` returns **only** matches inside the plan documents themselves. Sibling test `cmd/traces_follow_test.go` does not exercise an error envelope at all. `apiErrorCodeForStatus` at `cmd/helpers.go:152-172` shows the **CLI's** notion of canonical codes: 403 → `TENANT_ACCESS_REVOKED`, not `PERMISSION_DENIED`. 404 → `NOT_FOUND`, not `TRACE_NOT_FOUND`. +- **Impact:** If the server actually returns `NOT_FOUND` (already mapped to ExitNotFound at `exit.go:24`) and `TENANT_ACCESS_REVOKED` (already mapped to ExitAuth at `exit.go:21`), then no `MapServerCode` extension is needed — the existing mapping already handles them. The plan's Phase 2 step 4 risks adding dead code if `TRACE_NOT_FOUND` is never emitted by the upstream `digitopvn/goclaw` server. +- **Suggested fix:** Phase 1 step 1 (smoke probe) MUST capture an actual 404 error envelope from the live gateway to lock the real `code` field value. Drop speculative codes from Phase 2 plan until that fixture is captured. Tests should assert on the actual code returned, not a guessed name. + +### Important — `okJSON` and `runCmd` test helpers exist +- **Assumption:** "`okJSON(t, w, payload)` and `runCmd(t, args...)` helpers exist" +- **Status:** PASS +- **Evidence:** `cmd/phase5_test.go:13-19` defines `okJSON`; `cmd/phase5_test.go:21-27` defines `runCmd`. Both used across `cmd/traces_follow_test.go`, `cmd/activity_aggregate_test.go`, etc. +- **Impact:** None — helpers available. +- **Caveat:** `okJSON` wraps payload in `{"ok": true, "payload": ...}`. If the live `/v1/traces/{id}` envelope is structurally different (e.g. payload at root, no `ok` wrapper, or under a different key), tests built on `okJSON` will be testing a fake envelope. This matters for "TestTracesGet_HappyPath_*" — verify Phase 1 smoke probe captures the **real** envelope before assuming the `okJSON` shape applies. + +### Critical — Trace payload shape (`spans`/`events`/`messages`) is speculation +- **Assumption:** "trace payload has `spans` (hierarchical or flat with parent_id), `events`, `messages`" +- **Status:** UNVERIFIABLE +- **Evidence:** Only on-disk reference to these field names in the context of `/v1/traces/{id}`: + - `cmd/traces.go:51-54` (`tracesListCmd` columns): `trace_id`, `agent_id`, `status`, `duration_ms`, `input_tokens`, `output_tokens`, `cost`. **No** `spans`, `events`, `messages`. + - `cmd/traces_follow_test.go:34, 130-134`: envelope contains `traces`, `spans_by_trace_id`, `next_since` — but that's the **list/follow** shape, not the single-trace-detail shape. + - `cmd/traces.go:62` Short: "Get trace with span tree" — implies spans exist but says nothing about format. + - No fixture file under `cmd/testdata/` (the directory does not exist). +- **Impact:** Phase 2 plans `renderTraceDetail` and `buildSpanTree` against an unknown shape. If spans are returned as `spans_by_trace_id[trace_id]` (the only known wire format from `traces_follow`), span linkage is **flat** under a map keyed by trace_id — different from "hierarchical with `children` or flat with `parent_id`" as plan speculates. Implementation could be built against the wrong shape. +- **Suggested fix:** Phase 1 is mandatory **before** any Phase 2 implementation. Phase 2 step 2 ("Implement `renderTraceDetail`") must explicitly block on Phase 1's captured fixture. Make this dependency loud in `phase-02-cli-fix-output-and-error-mapping.md` (currently only `dependencies: [1]` in frontmatter). + +### Minor — No existing secret-stripped fixture pattern in repo +- **Assumption:** "Strip secrets" / "Do NOT commit fixture with auth header or unredacted user IDs" +- **Status:** UNVERIFIABLE (no precedent to follow) +- **Evidence:** `find . -type d -name testdata` returns no results — no `cmd/testdata/` exists, no fixtures committed today. +- **Impact:** Phase 1 will introduce the first on-disk fixture pattern for this repo. Without a precedent, the reviewer has to design the redaction rules. Risk: under-redaction (PII slips through) or over-redaction (fixture becomes useless for testing real shape edge cases). +- **Suggested fix:** Phase 1 should list explicit fields to scrub: `user_id`, `tenant_id`, `agent_id` content if proprietary, `messages[].content`, `events[].args`, any token-bearing fields. Use sentinel placeholders (`user-redacted`, `tenant-redacted`, `[REDACTED]`). Document the redaction rules in the report so future trace fixtures follow them. + +### Minor — `Closes #17` auto-close requires merge to default branch +- **Assumption:** "PR auto-close via `Closes #17` only fires when merged into the default branch (`main`). PR targets `dev` first." (phase-03, risk section) +- **Status:** PASS (correctly self-flagged in plan) +- **Evidence:** Default branch is `main` (per gitStatus header and CLAUDE.md). Plan targets `dev`. GitHub auto-closes referenced issues only when the PR merges to the default branch. The plan already calls this out and proposes manual close after dev→main promotion. +- **Impact:** None — plan is self-consistent. Issue #17 is verified to exist in `nextlevelbuilder/goclaw-cli` (the correct repo per `git remote -v`). + +### Minor — `Get` returns `json.RawMessage` (envelope already unwrapped) +- **Observation (bonus):** `HTTPClient.Get` at `internal/client/http.go:47` returns `(json.RawMessage, error)`. The plan's diagrams hint at this but never explicitly state what `data` is when `c.Get(...)` returns. Worth verifying whether `Get` strips the `{ok, payload}` envelope before returning — otherwise `unmarshalMap(data)` operates on the envelope, not the inner payload, and tests using `okJSON` wrap exactly the envelope shape that `Get` would already have unwrapped. +- **Status:** UNVERIFIABLE without reading `internal/client/http.go` in full — but the plan does not document this contract, and the test envelope wrapping in `okJSON` strongly implies `Get` does unwrap. Phase 1's repro test must confirm the layer at which `data` is observed. +- **Suggested fix:** Phase 1 step 1 add a sub-step: "Document what shape `data := c.Get(...)` returns — envelope or payload — by reading `internal/client/http.go`'s Get implementation." + +## Open Questions + +- Does the live `/v1/traces/{id}` endpoint exist on the current `digitopvn/goclaw` server, or is the path different (e.g. `/v1/traces/get/{id}`, `/v1/traces?id=...`, `/v1/tenants/{tenant}/traces/{id}`)? Plan assumes the path works; H5 in Phase 1 nominally checks this but doesn't allocate a discrete acceptance criterion to the answer. +- Are span IDs/parent linkage fields named `span_id`/`parent_id`, `id`/`parent`, `id`/`parent_span_id`, or something else? `renderTraceDetail`/`buildSpanTree` cannot be designed until the fixture lands. +- If the server returns a 401/403 due to tenant mismatch on `traces get` but allows `traces list`, that's a server bug, not a CLI bug — should Phase 3's upstream-issue path be widened to "auth/tenant filtering" beyond pure not-found regressions? + +Status: DONE +Severity counts: 3 Critical, 5 Important, 3 Minor. diff --git a/plans/260528-1357-fix-trace-details-by-id/reports/red-team-failure-mode-analyst.md b/plans/260528-1357-fix-trace-details-by-id/reports/red-team-failure-mode-analyst.md new file mode 100644 index 0000000..8300708 --- /dev/null +++ b/plans/260528-1357-fix-trace-details-by-id/reports/red-team-failure-mode-analyst.md @@ -0,0 +1,99 @@ +# Red-Team: Failure Mode Analyst — Issue #17 Plan + +## Verdict: REVISE + +Plan is mostly sound but ships with **4 critical correctness gaps** that will cause test flakes or false-positive merges, and **3 important gaps** in test scaffolding/spec language. Two minor items also called out. Recommend revisions to phase-01 and phase-02 before TDD lands. + +## Failure Modes + +### Critical — Auto-retry inflates request count on 5xx/429 tests +- **Mode:** `internal/client/http.go` `do()` retries up to **3 attempts** on `StatusCode == 429 || StatusCode >= 500`, with 1s + 2s backoff between attempts. Phase-2 test `TestTracesGet_ServerError_ExitCode5` will see **3 hits**, not 1, plus a ~3s sleep cost. Phase-1 test `TestTracesGet_MalformedID_ValidatedClientSide` `calls == 0` assertion is fine, but any future "exactly N calls" assertion on 5xx is wrong. +- **Status:** EXPOSED +- **Evidence:** `internal/client/http.go:138-168`: `for attempt := range 3 { ... if resp.StatusCode != 429 && resp.StatusCode < 500 { break } ... if attempt < 2 { time.Sleep(time.Duration(1<`, `&` → `<`...), (c) does NOT preserve original byte-for-byte. So "preserves nested fields" is true, "preserves full payload" byte-equivalent is false. +- **Status:** EXPOSED +- **Evidence:** `internal/output/output.go:57-61` `enc := json.NewEncoder(os.Stdout); enc.SetIndent(...)` — no `SetEscapeHTML(false)` call. Go stdlib default is HTML-escape ON. +- **Impact:** Trace span content with `<` `>` `&` (e.g. tool calls with XML/HTML in args, or `&&` in shell commands) will have those chars escaped in output. Round-trip `jq` parse still works (escapes are valid JSON), but byte-comparison fails. Test `TestTracesGet_JSONMode_PreservesNestedFields` description (phase-02.md:70-71) says "round-trip parse; assert every field present" — that's the *correct* assertion. But the **plan prose** at phase-02.md:20 overpromises. +- **Suggested mitigation:** phase-02 — soften prose: "JSON/YAML mode emits the complete server payload as a structured object — every field round-trips through `jq`, but bytes are re-encoded (key order normalized, HTML chars escaped per Go stdlib default)." Optionally pass-through `json.RawMessage` instead of `map[string]any` to preserve bytes; but that's a bigger change. + +### Important — span shape assumptions are unverified before code lands +- **Mode:** Phase-02 §2 says "if spans hierarchical, recurse on `children`; if flat with `parent_id`, build adjacency map first." But: (a) what if **some** traces have `spans: []` (no spans)? (b) what if `spans` field is absent / `null`? (c) what if shape is **mixed** — top-level flat list with `children: []` already populated by the server? (d) what about cycles in `parent_id` graph (malicious / bug)? (e) what if `events: null` instead of `[]`? (f) what about giant span trees (1000+ nodes) — does `PrintTree` indent prefix grow unboundedly? +- **Status:** EXPOSED +- **Evidence:** Phase-01 fixture-capture is the only place where the actual shape would be locked, but Phase-01 risk-mitigation (phase-01.md:96-97) says "if server unreachable, fixture derived from `traces follow` shape with TODO" — meaning code lands without verified shape if smoke probe fails. `internal/output/tree.go:14-30` `PrintTree` is unguarded against deep recursion. +- **Impact:** Render helper may panic on `nil` `events`, infinite-loop on cyclic `parent_id`, or produce useless output on absent `spans`. +- **Suggested mitigation:** phase-02 §2 — explicit defensive list: "(a) `spans == nil` or `len(spans) == 0` → print `(no spans)` line, skip tree; (b) `events == nil` → treat as `[]`; (c) `parent_id` adjacency map MUST track visited to break cycles; (d) hard-cap tree depth at 50 levels with `...` truncation; (e) hard-cap printed events at 50 even when n <= 10." + +### Important — `str`/`safeFloat` helpers — `safeFloat` does not exist +- **Mode:** Phase-02.md:80-81 says "every field access through `str(m, "key")` / `safeFloat(m, "key")` helpers." `str` exists at `cmd/helpers.go:56-61`. `safeFloat` does **not** exist anywhere in `/cmd/` or `/internal/`. Plan does not specify who writes it or where it lives. +- **Status:** EXPOSED +- **Evidence:** `grep -rn "safeFloat" --include="*.go"` returns zero hits in the worktree. `str` defined `cmd/helpers.go:56-61`. +- **Impact:** Phase-02 implementation halts at compile error or scope-creep into `cmd/helpers.go`. Defensive numeric coercion of `map[string]any` (where JSON numbers decode as `float64`, but server may emit them as strings) is non-trivial and untested. +- **Suggested mitigation:** phase-02 Related Code Files — explicitly add `cmd/helpers.go` modification: "Add `safeFloat(m map[string]any, key string) float64` that handles `float64`, `int`, `int64`, `json.Number`, and string-encoded numbers; returns 0 on failure. Add unit tests in `cmd/helpers_test.go`." + +### Important — `url.PathEscape` "validate non-empty" is underspecified +- **Mode:** Phase-02 §3 step: "Validate id: `id := args[0]; if strings.TrimSpace(id) == "" { return fmt.Errorf("trace id is required") }`." This only catches whitespace/empty. It does NOT reject path-traversal (`../../etc/passwd`), URL-control chars (`?foo=bar` injecting query, `#frag` injecting fragment), or slash (`a/b` becoming `/v1/traces/a/b` — but `url.PathEscape` does escape `/` to `%2F`, so that's safe). What does the issue reporter consider "malformed"? The plan never says. +- **Status:** EXPOSED +- **Evidence:** `url.PathEscape` Go stdlib docs: escapes `/`, `?`, `#`, etc. — so `goclaw traces get '../../admin'` becomes `GET /v1/traces/..%2F..%2Fadmin` which the server can choose to 404 or 400. Not a CLI security hole (server is the trust boundary). But test `TestTracesGet_MalformedID_ValidatedClientSide` (phase-02.md:73) asserts `calls == 0` — meaning the CLI MUST short-circuit before HTTP, but the plan never lists which inputs count as "malformed client-side". +- **Impact:** Test is ambiguous. Implementer may pick a different set than tester expects. +- **Suggested mitigation:** phase-02 §3 — explicit list: "client-side rejects only (a) empty after trim, (b) length > 256 (DoS guard). All other inputs go through `url.PathEscape` and let the server decide. Test passes `""`, `" "`, and a 1KB string for `calls == 0` assertions; passes `"../../admin"` for `calls == 1` with the server returning 400." + +### Minor — Phase-01 smoke probe has TODO escape; Phase-02 manual smoke has none +- **Mode:** Phase-01 risk mitigation explicitly allows "if server unreachable, mark TODO". Phase-02 §6 says "Live smoke (manual) — re-run the four `goclaw traces get` invocations from Phase 1's smoke probe; confirm now produces useful output" with no fallback. +- **Status:** EXPOSED +- **Evidence:** phase-02.md:102. +- **Impact:** Phase-02 success-criteria "Manual smoke: ... renders a readable summary" (phase-02.md:111) can't be checked off if no gateway is reachable; ship pipeline stalls. +- **Suggested mitigation:** phase-02 §6 — add "If gateway unreachable, mark manual-smoke as `DEFERRED` and gate the PR description to require a smoke-check before merge to `main`." + +### Minor — Phase-3 codebase-summary update target verified +- **Mode:** Plan promises a bullet under "output/render section" in `docs/codebase-summary.md`. +- **Status:** MITIGATED +- **Evidence:** `docs/codebase-summary.md` line 149 has `#### output/ — Output Formatting + Error Handling`, line 587 references tree rendering. Section exists. +- **Impact:** None — claim is true. +- **Suggested mitigation:** None. + +### Minor — `gh issue create -R digitopvn/goclaw` rights not pre-verified +- **Mode:** Phase-03 §5 conditionally runs `gh issue create -R digitopvn/goclaw`. If the user lacks issue-creation rights on that repo, the command fails mid-ship. +- **Status:** EXPOSED +- **Evidence:** phase-03.md:52. No `gh api repos/digitopvn/goclaw -q .permissions` pre-check. +- **Impact:** Ship pipeline stops with a permission error instead of completing the docs update. +- **Suggested mitigation:** phase-03 §5 — prepend `gh api repos/digitopvn/goclaw -q .permissions.push 2>/dev/null` check; on failure, drop the drafted issue body into `plans/.../reports/upstream-issue-draft.md` and instruct the user to file it manually. + +### NOT_APPLICABLE — 403 retry concern +- **Mode:** "Does the client retry on 403?" +- **Status:** NOT_APPLICABLE +- **Evidence:** `internal/client/http.go:161`: `if resp.StatusCode != 429 && resp.StatusCode < 500 { break }`. 403 breaks immediately. No retry. No mitigation needed. + +## Open Questions + +- Should `HTTPClient` gain a `MaxRetries` knob to disable retries in tests, or is the ~3s 5xx-test cost acceptable (5xx tests are rare)? +- Does the issue reporter's actual repro live trace_id still exist on `goclaw.zuey.me`? If not, what's the smoke-probe fallback gateway? +- Does the server emit `PERMISSION_DENIED` as a body code anywhere, or always `TENANT_ACCESS_REVOKED`? Determines whether `MapServerCode` extension is actually a no-op or a behavior change. +- For deeply nested span trees (>50 levels), is truncation `... (N more levels)` acceptable, or should the tree render still walk but flatten? + +Status: DONE +Severity counts: Critical=4, Important=4, Minor=3, NOT_APPLICABLE=1 diff --git a/plans/260528-1357-fix-trace-details-by-id/reports/red-team-scope-complexity-critic.md b/plans/260528-1357-fix-trace-details-by-id/reports/red-team-scope-complexity-critic.md new file mode 100644 index 0000000..f185aa3 --- /dev/null +++ b/plans/260528-1357-fix-trace-details-by-id/reports/red-team-scope-complexity-critic.md @@ -0,0 +1,109 @@ +# Red-Team: Scope & Complexity Critic — Issue #17 Plan + +## Verdict: TRIM + +The plan attacks a real bug with the right instincts (TDD, structured rendering, central error mapping) but inflates work by ~40-50%. Phase 1 over-tests for a fix-before-known-shape; Phase 2 carries a deferred decision (`unmarshalMapStrict` vs inline) into implementation; presentation polish (event truncation, defensive helpers) is masquerading as bug-fix scope. Exit-code mapping is also mostly free — `MapServerCode` + `MapHTTPStatus` already cover all 4 categories via the existing fallback (verified at `internal/output/exit.go:18-44`). Trim, don't reject. + +--- + +## Findings + +### Critical — Pre-fixture test count inflates Phase 1 + +- **Item:** Phase 1 step 3 — 7 red test cases written before the fixture is captured from a live gateway. +- **Disposition:** TRIM +- **Rationale:** Three tests (`HappyPath_Table`, `MalformedID`, `MalformedResponse`) assert behavior of code that doesn't exist yet. They're Phase 2 acceptance tests squatting in Phase 1, which is fine for TDD but inflates Phase 1's "this is the repro" mission. The actual repro need is: (a) one test proving the current JSON-dump-in-TTY symptom, and (b) the captured fixture. The other 5 can be added at the top of Phase 2 (one commit) without breaking TDD discipline. +- **Leaner version:** Phase 1 writes 2 tests (`TestTracesGet_PathAndMethod`, `TestTracesGet_TableMode_DumpsRawJSON_RED`) + the fixture + the classification verdict. Move the other 5 to Phase 2 step 1. + +### Critical — `unmarshalMapStrict` decision deferred to implementation + +- **Item:** Phase 2 line 63: "add `unmarshalMapStrict(data) (map[string]any, error)`... OR replace usage at the call site only. Decision in step 1." +- **Disposition:** CUT the helper, KEEP the inline approach. +- **Rationale:** `unmarshalMap` is called in 6 places across `cmd/` (traces, usage summary/costs/timeseries/breakdown). Adding `unmarshalMapStrict` is either a one-off (only `tracesGetCmd` uses it — confusing) or a project-wide refactor (out of scope). The bug only requires checking the unmarshal error in this one command. Inline is 4 lines. Leaving the decision to "step 1" forces re-planning at implementation time, which is exactly the anti-pattern that planning is supposed to prevent. +- **Leaner version:** Phase 2 mandates: replace `printer.Print(unmarshalMap(data))` with an inline `json.Unmarshal` + error check. No new helper. Three lines diff. + +### Important — Separate `cmd/traces_render.go` file at 80-120 LOC + +- **Item:** Phase 2 — `cmd/traces_render.go` with `renderTraceDetail` + `buildSpanTree` + defensive `str/safeFloat` helpers. +- **Disposition:** TRIM +- **Rationale:** `str()` already exists at `cmd/helpers.go:56`. `tracesListCmd` (12 LOC inline render via `output.NewTable`) is the established pattern; this plan would break that pattern for one command. If span-tree rendering stays in scope (see next finding), it justifies a helper, but 80-120 LOC is double what's needed. The header card is 3-5 `printer.Print(output.NewTable(...))` calls. Span tree is `PrintTreeRoot(buildSpanTree(spans), os.Stdout)` — that's the only real helper that earns its keep. +- **Leaner version:** Inline the header in `tracesGetCmd.RunE` (~15 LOC). Extract only `buildSpanTree(spans []any) output.TreeNode` to `cmd/traces.go` as a package-level func, not a new file. Target: total addition ~40 LOC across one file. + +### Important — Span-tree rendering vs issue #17 acceptance criteria + +- **Item:** Phase 2 architecture — span tree via `output.PrintTree`. +- **Disposition:** KEEP (with constraint) +- **Rationale:** Re-read of issue #17 verbatim: "including relevant metadata, root/child events, messages, tool calls, status, timing". "Root/child events" reads as hierarchical structure — span tree IS a reasonable interpretation, not scope creep. Also the existing `tracesGetCmd.Short` is literally "Get trace with span tree" (`cmd/traces.go:62`), so this is a documented promise the code never delivered. KEEP, but cap aggressively: ~20 LOC for `buildSpanTree`, degrade gracefully if no spans/no parent_id field. +- **Leaner version:** Build adjacency only if `spans` field present and has >1 entry. If flat or absent, skip the tree block and print events list directly. No "deep nesting / cyclic refs" defensive code — the server controls this payload, and if it ever sends a cycle, that's a server bug worth crashing on (or trivially guard with depth limit ≤32, ~3 LOC). + +### Important — Events truncation logic + +- **Item:** Phase 2 step 2 — "first 5 + last 1 with `... (N-6 more) ...` separator if N > 10". +- **Disposition:** CUT +- **Rationale:** This is presentation polish, not bug-fix scope. The bug is "unusable output". A simple `EVENTS (N=12):` header + full list, one per line, is human-readable; if the user wants pagination they pipe to `less`. The truncation logic adds branching, off-by-ones, and a test surface that doesn't earn its weight for a P2 bugfix. If a real trace has 5,000 events, that's a separate UX issue worth filing. +- **Leaner version:** Print `EVENTS (N=):` header then list each event on its own line as ` `. ~8 LOC. Done. + +### Important — 4 distinct exit codes — already free, "verify" step is over-scoped + +- **Item:** Phase 2 step 4 — "Verify error-code mapping in `internal/output/error.go`... extend `MapServerCode` if `TRACE_NOT_FOUND` / `PERMISSION_DENIED` not yet mapped." +- **Disposition:** KEEP the requirement (it's locked per CLAUDE.md), CUT the "extend" hedging. +- **Rationale:** Verified at `internal/output/exit.go:18-44` — `serverCodeMap` maps `NOT_FOUND→3`, `TENANT_ACCESS_REVOKED→2`, `INVALID_REQUEST→4`, `INTERNAL/UNAVAILABLE→5`. And `apiErrorCodeForStatus` at `cmd/helpers.go:152-172` already translates HTTP 403→`TENANT_ACCESS_REVOKED`, 404→`NOT_FOUND`, 400/422→`INVALID_REQUEST`, 5xx→`INTERNAL`. The server doesn't need to emit `TRACE_NOT_FOUND` — bare HTTP status is already correctly mapped through two layers. The 4-category exit-code requirement is met today by `apiErrorFromRawBody` + `MapServerCode`. The remaining gap is **client-side malformed-id validation** (`MalformedID` → ExitValidation), which has to be added because the server never sees the request. +- **Leaner version:** Phase 2 step 4 becomes: "Add client-side id validation in `tracesGetCmd.RunE`: reject empty/whitespace id with an error that surfaces `INVALID_REQUEST` (or `output.ExitValidation` directly). No `internal/output/*` edits expected; if test fails for a specific server code, add to `serverCodeMap` at that point." + +### Important — Phase 1 effort (2-3h) is overstated for the trimmed scope + +- **Item:** Phase 1 — effort `"2-3h"` covers smoke probe + fixture + 7 red tests + repro report + classification. +- **Disposition:** TRIM +- **Rationale:** With the test count trim above, Phase 1 = smoke probe (15 min if gateway up) + fixture capture + 2 tests + report. Realistic: 45-75 min. The "2-3h" estimate suggests the planner padded for the bloated test count. +- **Leaner version:** Re-estimate Phase 1 at 1-1.5h after the test trim. Frees attention budget for Phase 2. + +### Minor — Upstream issue filing is unforked + +- **Item:** Phase 3 step 5 — "if Phase 1 classification was server-side or hybrid, file upstream issue." +- **Disposition:** KEEP, but fork plan branches. +- **Rationale:** Scout of `cmd/traces.go:68` shows the path `/v1/traces/` is straightforward — a server-side root cause would most likely be tenant-filter or payload-shape, both of which still need a CLI render fix to handle the response shape. Likely outcome: CLI-only or hybrid. If pure server-side (gateway returns 404 for valid IDs), the CLI render work is wasted but the test infrastructure isn't. Don't fork phases, but add a branch point at end of Phase 1: "If pure server-side: Phase 2 reduces to error-mapping + smoke test only; render scope deferred." +- **Leaner version:** Add a one-liner at end of Phase 1 Success Criteria: "Classification verdict drives Phase 2 scope reduction — record verdict in plan.md before starting Phase 2." + +### Minor — `docs/codebase-summary.md` update trigger undefined + +- **Item:** Phase 3 — "add a bullet... if `cmd/traces_render.go` was created" / "if non-trivial". +- **Disposition:** TRIM +- **Rationale:** Vague triggers create skip-the-step inertia. Either the helper is a new public-ish surface worth documenting, or it isn't. After the trim above (no separate file, ~40 LOC inline + one tiny helper), it's not. +- **Leaner version:** Cut the `codebase-summary.md` step entirely. The CHANGELOG entry is enough. If a future planner needs to know about the helper, grep finds it. + +### Minor — Phase 3 step 1 hand-edits plan.md status + +- **Item:** Phase 3 step 1 — "mark phase 1, 2 as completed in `plan.md`". +- **Disposition:** TRIM +- **Rationale:** The ck-stack has `/ck:plan-check` / `ck plan check` (referenced in checklists) for status sync. Hand-editing plan.md drift is exactly what that tool exists to prevent. Not catastrophic, but inconsistent with project tooling. +- **Leaner version:** Replace with: "Run `/ck:plan-check --sync` (or hand-edit if tooling unavailable) to flip phase 1 and 2 to `completed` in `plan.md`." Cite the tool, fall back to manual. + +--- + +## Summary of Recommended Cuts + +| Original | Trimmed | +|---|---| +| 14 tests total | 9 tests total (2 in Phase 1 + 7 in Phase 2) | +| Separate `cmd/traces_render.go` file, 80-120 LOC | Inline + one small `buildSpanTree` helper in `cmd/traces.go`, ~40 LOC | +| `unmarshalMapStrict` helper OR inline (deferred) | Inline only — decision made now | +| Events: "first 5 + last 1 with separator" | Print all events, one per line | +| Phase 2 step 4: "verify and possibly extend `MapServerCode`" | "Add client-side id validation; trust existing mapping" | +| Phase 1 effort 2-3h | Phase 1 effort 1-1.5h | +| Plan.md hand-edit for status | `/ck:plan-check --sync` | +| `codebase-summary.md` update (conditional) | CUT | + +**Net expected savings:** ~1.5h Phase 1 + ~1h Phase 2 + cognitive load of one deferred decision. Total ~25-30% reduction. Bug still gets fixed, contract still honored, TDD still enforced. + +--- + +## Open Questions for User + +None affect verified user decisions. TDD discipline is preserved (red tests still precede impl in both phases). Exit-code contract is preserved (the trim doesn't reduce categories, just shifts where they're enforced). Span-tree rendering is preserved because it appears to honor the issue text "root/child events" AND the existing command's stated `Short: "Get trace with span tree"` promise. + +If the user disagrees on **event truncation** specifically (Finding "Events truncation"), surface that — it's the only judgment call where reasonable people might differ on UX-vs-scope. + +--- + +**Status:** DONE +**Severity counts:** Critical: 2 / Important: 5 / Minor: 3 diff --git a/plans/260528-1357-fix-trace-details-by-id/reports/red-team-security-adversary.md b/plans/260528-1357-fix-trace-details-by-id/reports/red-team-security-adversary.md new file mode 100644 index 0000000..bd1dfbd --- /dev/null +++ b/plans/260528-1357-fix-trace-details-by-id/reports/red-team-security-adversary.md @@ -0,0 +1,155 @@ +# Red-Team: Security Adversary — Issue #17 Plan + +## Verdict: REVISE + +Plan is sound on the functional axis but ships with several under-specified security gates around (a) fixture/issue-body scrubbing, (b) path traversal via the trace id parameter, (c) ANSI injection parity claim, and (d) information-disclosure trade-off in the 404-vs-403 message split. None are showstoppers — all are addressable by tightening Phase 1 step 2 and Phase 2's security-considerations block before code lands. + +## Findings + +### Critical — Path traversal surface via `url.PathEscape` for trace id + +- **Threat:** `url.PathEscape` does NOT encode `.`, so `goclaw traces get ..` produces request path `/v1/traces/..`. `url.PathEscape("../../etc/passwd")` does encode the `/` as `%2F`, but `url.PathEscape("..")` returns the literal `".."`. If a future server route/middleware on `digitopvn/goclaw` ever path-normalizes before authz (or proxies through a path-rewriting reverse proxy like nginx with `merge_slashes`/`alias`), the CLI becomes the easiest tool to probe traversal. +- **Status:** HOLE (CLI-side input validation gap). +- **Evidence:** + - Verified empirically: `url.PathEscape("..") == ".."`, `url.PathEscape("./../x") == ".%2F..%2Fx"`, `url.PathEscape("../../etc/passwd") == "..%2F..%2Fetc%2Fpasswd"`. Run output captured in this review session. + - Plan phase-02-cli-fix-output-and-error-mapping.md:84 — `url.PathEscape(id)` is the only id sanitization step. + - Plan phase-02-cli-fix-output-and-error-mapping.md:84 — id validation is `strings.TrimSpace(id) == ""` only. +- **Attack scenario:** Operator who has a valid auth token but limited trace-read scope runs `goclaw traces get ..` or `goclaw traces get .`. The server receives `GET /v1/traces/..` — at minimum a noisy/unhandled route; at worst, on a future server build that adds `path.Clean` ahead of authz, lands on `/v1/traces/` (list endpoint) under the singular's authz check. +- **Suggested mitigation:** In Phase 2 step 3 id validation, after `TrimSpace` add: + ``` + if id == "." || id == ".." || strings.ContainsAny(id, "/\\") { + return fmt.Errorf("invalid trace id") + } + ``` + And add a Phase 1 test case `TestTracesGet_RejectsTraversalIDs` covering `..`, `.`, `../foo`, `foo/bar`, `\x00`. Also: tighten to a positive regex (`^[A-Za-z0-9_-]{1,128}$`) if Phase 1's live capture shows trace ids are opaque ULIDs/UUIDs (most likely). Note: this collapses to AC #3's "malformed id" category. + +--- + +### Critical — Fixture/issue-body scrubbing protocol is hand-wavy + +- **Threat:** Phase 1 step 2 says "Strip secrets" and Phase 1 Security Considerations says "Strip `user_id`, `tenant_id`, message contents, tool-call args. Keep only structural keys + sentinel values." But (a) no `jq` recipe is given, (b) the scrub list is incomplete — span `name` fields can contain user queries (e.g. `name: "search('user@example.com')"` ), tool-call `result` payloads may contain PII, HTTP-request span attributes can contain auth headers, and (c) Phase 3 step 5 may file an upstream issue using a body drafted in Phase 1 — same payload, same scrub requirement, but Phase 3 doesn't re-state it. +- **Status:** HOLE (operational/process gap; high likelihood of accidental commit of PII). +- **Evidence:** + - Phase 1 step 2 line 70: "Strip secrets." (no how). + - Phase 1 Security Considerations line 102 enumerates 4 fields but misses span `name`, span `attributes`, event `message`/`error`, headers in HTTP spans, trace metadata bags. + - Phase 3 step 5 line 52: issue body reuses the Phase-1 drafted body, but Phase 3 Security Considerations line 76 only says "Smoke probe artifacts ... must be scrubbed" without listing the issue-body case explicitly. +- **Attack scenario:** Operator captures a live trace whose span name is `OpenAI.chat({"messages":[{"role":"user","content":"my SSN is 123-45-6789"}]})`. Operator strips only the 4 listed keys, commits fixture to `cmd/testdata/trace_detail_get.json`, and the PII enters git history. Same payload pasted into the `digitopvn/goclaw` issue body via `gh issue create` becomes public on a different repo. +- **Suggested mitigation:** Add to Phase 1 step 2 (and reference from Phase 3 step 5): + ``` + # Scrub recipe (run once; assume captured response in raw.json): + jq ' + walk(if type == "object" then + with_entries( + if .key | IN("user_id","tenant_id","org_id","email","token", + "authorization","auth","api_key","secret", + "messages","tool_calls","arguments","result", + "input","output","prompt","completion","content", + "attributes","metadata","headers","query","body","error") + then .value = "REDACTED" + else . end) + else . end) + | .payload.trace_id = "trc_XXXXXXXXXXXXXXXX" + | .payload.agent_id = "agt_XXXXXXXX" + ' raw.json > cmd/testdata/trace_detail_get.json + ``` + Then `rg -i 'email|@|bearer|sk-|eyJ' cmd/testdata/trace_detail_get.json` as a post-scrub gate. Reuse the SAME pipeline for the upstream-issue body. Phase 3 step 5 should explicitly include `gh issue create --body-file scrubbed.md`, never inline. + +--- + +### Critical — `curl` command in repro report risks token leakage + +- **Threat:** Phase 1 step 1 instructs running `curl -sS -H "Authorization: Bearer $TOKEN" ...` AND Phase 1 step 5 says to include "Curl command + response samples" in `reports/repro-260528-issue17.md`. If the operator copy/pastes the executed command from shell history, the token expands. +- **Status:** HOLE (documentation/operational; medium likelihood — depends on operator hygiene). +- **Evidence:** + - Phase 1 step 1 line 69: `curl -sS -H "Authorization: Bearer $TOKEN" "$SERVER/v1/traces/"` — uses env var here, good. + - Phase 1 step 5 line 81: "Curl command + response samples (redacted)" — no explicit guard against pasting the expanded token. +- **Attack scenario:** Operator runs the curl in zsh with history-expansion or pastes from `history` output where the variable was expanded (some shells do this on certain `set` configs). Token enters the report → enters git → gets pushed. +- **Suggested mitigation:** Phase 1 step 5: explicit rule — "Quote curl as literal: `curl -H 'Authorization: Bearer $GOCLAW_TOKEN' ...` (single-quoted, never expanded). Add a pre-commit grep gate: `! rg -n 'Bearer [A-Za-z0-9._\-]{20,}' plans/`." And: set `HISTFILE=/dev/null` or prefix with space-then-command to skip history (`setopt HIST_IGNORE_SPACE`/`HISTCONTROL=ignorespace`) before running the probe. + +--- + +### Important — ANSI escape injection: parity claim is half-true + +- **Threat:** Phase 2 Security Considerations line 123 defers ANSI sanitization with "accept that terminal rendering of untrusted text is consistent with `traces list` (existing risk surface)." Verified: `tracesListCmd` does render user-controlled strings (`agent_id`, `status`) into table cells (cmd/traces.go:53). So the parity claim has factual basis — but the surface AREA is different. `traces list` renders ~7 short flat fields (id, agent, status, ms, tokens, tokens, cost) which are server-generated. The new `traces get` will render span `name`, event `message`, possibly tool-call args — fields that are far more likely to contain attacker-controlled content (user prompts, LLM outputs, tool stdout/stderr). LLM output is the canonical ANSI-injection vector in 2026. +- **Status:** HOLE — parity is structurally true (no existing scrubbing) but the risk-weighted exposure is materially higher. The "accept parity" decision should be made by the user, not the planner. +- **Evidence:** + - cmd/traces.go:53 — `str(t, "agent_id")` etc. unsanitized into table. + - `rg "safeRune|ANSI" internal/output/ cmd/` returns no hits — no current ANSI sanitization anywhere. + - Phase 2 line 79 — TreeNode names will include ` [ms] ` where `` is user/LLM-controlled. +- **Attack scenario:** Adversarial LLM (jailbroken or prompt-injected via tool output) emits a span name containing `\x1b[2J\x1b[H` (clear screen) + `\x1b]0;rm -rf ~\x07` (set terminal title) + a fake "operation succeeded" message. Operator runs `goclaw traces get ` in a terminal, sees a forged success line, may run further commands assuming the trace is benign. With OSC 52 (`\x1b]52;c;\x07`), the span name can quietly overwrite the operator's clipboard with arbitrary text. +- **Suggested mitigation:** Phase 2 — implement the `safeRune` helper now (10 lines, no extra dependency). Strip `\x1b`, `\x07`, all `C0` controls except `\t`. Apply in `renderTraceDetail` and `buildSpanTree`. Apply the same to `tracesListCmd` while you're in the area (one-line wrap around `str(...)`). Document the helper in `internal/output/text_sanitize.go`. Cost: ~20 LOC; benefit: closes the entire family. + +--- + +### Important — 404 vs 403 message split is an existence oracle (intentional?) + +- **Threat:** Plan deliberately splits "permission denied for trace ``" (403/PERMISSION_DENIED) from "trace `` not found" (404/TRACE_NOT_FOUND). That distinction reveals whether a trace id exists across tenants. AC #3 from issue #17 demands the distinction, so this is a documented trade-off — but the plan does not name it as a trade-off. +- **Status:** HOLE (information disclosure) but intentional per AC; needs explicit acknowledgment. +- **Evidence:** + - Phase 2 Architecture line 42-45: distinct messages by code. + - plan.md AC #3 line 33: "Errors clearly distinguish: not found, permission denied, malformed id, and server/API failure." +- **Attack scenario:** Attacker with valid token for tenant A iterates plausible trace-id prefixes. `403 permission denied for trace X` confirms X exists under tenant B; `404 not found` rules X out. Over time the attacker enumerates cross-tenant trace ids — useful for crafting follow-on social-engineering or for confirming activity against a known target. +- **Suggested mitigation:** This is primarily a server-side concern (the server is choosing to return 403 vs 404 distinct codes), and the CLI faithfully surfaces what the server returns. Two options: + 1. Defer to server: add a Phase 2 note "the 403/404 distinction is the server's choice; CLI surfaces it per AC #3. If the server later merges to a single 404 for security, CLI behavior follows automatically because `MapHTTPStatus(404) -> 3` covers both." + 2. Add a `--paranoid` flag (out of scope; just flag in plan). + Recommend option 1: add 3-line "Information disclosure trade-off" subsection to Phase 2 Security Considerations naming this explicitly so the next reviewer doesn't re-litigate it. + +--- + +### Important — Missing server-code entries for `TRACE_NOT_FOUND` / `PERMISSION_DENIED` + +- **Threat:** Plan Phase 2 step 4 line 91-92 says "Confirm `MapServerCode('TRACE_NOT_FOUND')` returns `ExitNotFound`. If not, extend the switch." Verified: `internal/output/exit.go:17-39` does NOT contain `TRACE_NOT_FOUND` or `PERMISSION_DENIED` / `FORBIDDEN`. So `MapServerCode` returns `ExitGeneric` (1) for both, and the code falls through to `MapHTTPStatus` via `FromError` (internal/output/error.go:148-152). That path works iff `APIError.HTTPStatus()` is populated — and that's a non-trivial invariant to depend on. +- **Status:** HOLE (latent fragility, not exploit-grade). +- **Evidence:** + - `internal/output/exit.go:17-39` — no TRACE_NOT_FOUND, no PERMISSION_DENIED, no FORBIDDEN. + - `internal/output/error.go:148-152` — fallback only fires when `errors.As(err, &aws)` succeeds AND `HTTPStatus() > 0`. If the server returns a JSON error envelope WITHOUT a status code surface in `APIError`, exit code is 1 instead of 2/3. +- **Attack scenario:** Not exploit; degraded UX. Automation script checking `$? == 2` for auth failures sees `$? == 1` and treats it as unknown error — bad escalation behavior. +- **Suggested mitigation:** Phase 2 step 4 — make the extension non-conditional. Add `TRACE_NOT_FOUND -> ExitNotFound`, `PERMISSION_DENIED -> ExitAuth`, `FORBIDDEN -> ExitAuth` to `serverCodeMap` unconditionally. Verify with `TestMapServerCode_TraceCodes`. Cost: 4 lines + 1 test. + +--- + +### Important — JSON full-payload preservation may leak internal fields + +- **Threat:** Phase 2 line 20-21 promises "JSON/YAML mode emits the **complete** server payload — no field dropping, no schema reshaping." That's the AI-ergonomics contract, but it converts a previous bug (silent JSON dump in table mode showed everything anyway) into an intentional, documented passthrough. If the server payload includes internal-only fields (`_internal_user_id`, debug counters, `__raw_sql`, etc.) the CLI now reliably exposes them. Previously, a user in TTY who saw a JSON blob might not parse it; now they get a JSON-structured output that is easier to grep/extract. +- **Status:** MITIGATED (within CLI scope) / HOLE (cross-boundary, server's responsibility). +- **Evidence:** + - Phase 2 line 20-21 — explicit full-passthrough decision. + - Phase 1 Architecture line 36 — current code already does `printer.Print(unmarshalMap(data))` which in JSON mode passes through fully. So the surface is NOT new — but the *test* in Phase 2 line 70 (`TestTracesGet_JSONMode_PreservesNestedFields`) freezes the contract as "every field round-trips" — which becomes a forcing function against future server-side scrubbing. +- **Attack scenario:** Server team adds an `_internal_debug_sql` field for observability; CLI test fails because the field is dropped/scrubbed; pressure pushes back on the server scrubbing. +- **Suggested mitigation:** Phase 2 — relax the test from "every input field present in output" to "every input field UNDER `payload` present in output, where input field name does not start with `_`" or "explicit whitelist of known-public fields from the captured fixture." Document the rule in `internal/output/error.go` or a new `docs/trace-payload-contract.md`. Cost: 3-line test refinement, 5-line doc. + +--- + +### Minor — Fixture contains real `agent_id` / `trace_id` even after scrub + +- **Threat:** Plan Phase 1 says "Keep only structural keys + sentinel values" but does not specify the sentinel format. Real trace ids and agent ids — even without other PII — can be cross-referenced against logs by anyone with operational access to the gateway. They are not secret but they ARE indirect identifiers (tying CLI test commits to specific live traces). +- **Status:** MITIGATED-IF-FOLLOWED (depends on operator using sentinels). Covered by the `jq` recipe in the Critical finding above. +- **Evidence:** Phase 1 Security Considerations line 102 mentions sentinel values but doesn't define them. +- **Attack scenario:** Marginal — researcher cross-references `trace_id` in fixture against gateway logs to identify a real user session. Low-likelihood, low-impact, but free to mitigate. +- **Suggested mitigation:** Sentinel pattern (already in the `jq` recipe above): `trc_XXXXXXXXXXXXXXXX`, `agt_XXXXXXXX`, `usr_XXXXXXXX`. Add a `TestFixtureContainsOnlySentinels` test that asserts the fixture matches `^[a-z]{3}_X+$` on id fields, blocking accidental real-id regression. + +--- + +### Minor — Timing oracle: not present in current plan + +- **Threat:** "Does a test for exit code 3 measure HTTP latency that leaks whether trace existed pre-auth-check?" +- **Status:** NOT_APPLICABLE. +- **Evidence:** Phase 1 and Phase 2 tests assert only exit code and message string, no latency assertions. Tests use `httptest.NewServer` which has near-zero latency anyway. +- **Attack scenario:** None. +- **Suggested mitigation:** None. Flag if any future PR adds `time.Since(start)` assertions on the error path. + +--- + +## Severity Counts + +- Critical: 3 (Path traversal, Fixture scrubbing recipe, Curl token leakage) +- Important: 4 (ANSI parity claim, 404/403 oracle, Missing server-code entries, JSON passthrough contract test) +- Minor: 2 (Sentinel format, Timing oracle [N/A]) + +## Open Questions + +- Is the trace id format actually `^[A-Za-z0-9_-]{N}$` (typical ULID/UUID/nanoid)? If yes, regex validation closes path traversal cleanly. If trace ids can contain arbitrary characters (e.g. user-supplied custom ids), validation must be looser and traversal mitigation falls back to explicit `..`/`.` rejection. Phase 1's live capture will resolve this. +- Does `digitopvn/goclaw` server treat `PERMISSION_DENIED` and `FORBIDDEN` interchangeably, or is one the convention? Phase 2 step 4 should grep the server repo (if accessible) to confirm. +- Is there a workspace-wide pre-commit hook for secret scanning (e.g. `gitleaks`, `trufflehog`) that would catch a leaked token in the fixture? If yes, the curl-token mitigation can lean on it. If no, this should become a `.git/hooks/pre-commit` addition in Phase 3. + +Status: DONE diff --git a/plans/260528-1357-fix-trace-details-by-id/reports/repro-260528-issue17.md b/plans/260528-1357-fix-trace-details-by-id/reports/repro-260528-issue17.md new file mode 100644 index 0000000..feeca98 --- /dev/null +++ b/plans/260528-1357-fix-trace-details-by-id/reports/repro-260528-issue17.md @@ -0,0 +1,109 @@ +# Repro & root cause — issue #17 (cannot read trace details by id) + +Date: 2026-05-28 +Phase: 1 +Worktree: `worktree-fix-trace-details-by-id` + +## Smoke probe + +**Intended target:** `goclaw.zuey.me` (production). +**Result:** auth blocked. No `~/.goclaw/config.yaml`, no token in env. The plan-documented escape applies — use a stub fixture derived from the existing `traces follow` payload shape ([cmd/traces_follow_test.go:129-130](cmd/traces_follow_test.go:129)), marked `_TODO_refresh` for the Phase 3 reviewer gate to enforce a real-gateway refresh before merge. + +**Curl command for future smoke probe (env-var token, no inline secret):** + +```bash +export TOKEN="$(goclaw config get token 2>/dev/null || cat ~/.goclaw/token)" +export SERVER="https://goclaw.zuey.me" +TRACE_ID="$(goclaw traces list --limit 1 -o json | jq -r '.payload[0].trace_id')" +curl -sS -H "Authorization: Bearer $TOKEN" "$SERVER/v1/traces/$TRACE_ID" > /tmp/trace_raw.json +goclaw traces get "$TRACE_ID" > /tmp/trace_tty.txt +goclaw traces get "$TRACE_ID" -o json > /tmp/trace_json.txt +goclaw traces get bogus-id-12345 -o json > /tmp/trace_404.txt 2>&1 +``` + +After refresh, run the `jq walk` scrub recipe from phase-01 step 2, then `grep -i 'eyJ\|Bearer\|sk-\|token=' cmd/testdata/trace_detail_get.json` must return 0 lines. + +## Fixture (scrubbed stub — sample) + +```json +{ + "_TODO_refresh": "stub fixture ...; refresh against goclaw.zuey.me before merge per phase-03 reviewer gate", + "trace_id": "trace_FIXTURE_001", + "agent_id": "agent_FIXTURE_001", + "session_key": "session_FIXTURE_001", + "user_id": "user_REDACTED", + "tenant_id": "tenant_REDACTED", + "status": "success", + "duration_ms": 2000, + "spans": [ + {"span_id": "span_001", "parent_span_id": null, "name": "agent.run", "kind": "agent", "status": "success"}, + {"span_id": "span_002", "parent_span_id": "span_001", "name": "llm.call", "kind": "llm", "status": "success"}, + {"span_id": "span_003", "parent_span_id": "span_001", "name": "tool.call", "kind": "tool", "status": "success"} + ], + "events": [ + {"event_id": "ev_001", "type": "llm.prompt"}, + {"event_id": "ev_002", "type": "llm.completion"}, + {"event_id": "ev_003", "type": "tool.invoke"} + ] +} +``` + +Secret-scan post-check: `grep -i 'eyJ\|Bearer\|sk-\|token=' cmd/testdata/trace_detail_get.json` → 0 lines. ✅ + +## Test result summary + +``` +go test -count=1 ./cmd/... -run TestTracesGet -v +``` + +| Test | Result | Meaning | +|------|--------|---------| +| `TestTracesGet_PathAndMethod` | PASS | Wire contract (`GET /v1/traces/{id}`) already correct. | +| `TestTracesGet_HappyPath_JSON_LocksFixture` | PASS | JSON-mode envelope round-trips correctly. | +| `TestTracesGet_TableMode_HumanReadable_RED` | **FAIL** | Table mode emits raw JSON beginning with `{` — the reported "unusable output" bug, locked. | + +Failure assertion: `table mode rendered raw JSON (starts with '{')`. Exactly as predicted by the scout findings. + +## Classification: **CLI-side** + +All three verified defects are in the CLI; no server-side root cause is required for AC#1–#3. + +| # | Defect | Evidence | Fix lane | +|---|--------|----------|----------| +| 1 | Table mode falls back to JSON dump for `map[string]any` payload | [cmd/traces.go:72](cmd/traces.go:72) calls `printer.Print(unmarshalMap(data))`; `output.Printer.Print` only formats `*TableData` in table mode, otherwise JSON-fallbacks ([internal/output/output.go:30-37](internal/output/output.go:30)) | Phase 2 — inline render (header + span tree via `output.PrintTree` + flat events list) | +| 2 | `unmarshalMap` silently swallows `json.Unmarshal` errors | [cmd/helpers.go:48-53](cmd/helpers.go:48) — literal `_ = json.Unmarshal(data, &m)` | Phase 2 — inline `json.Unmarshal` with `return fmt.Errorf("decode trace payload: %w", err)` | +| 3 | No id validation; raw `args[0]` concatenated into URL with no `url.PathEscape` | [cmd/traces.go:68](cmd/traces.go:68) — `"/v1/traces/" + args[0]` | Phase 2 — strict allowlist regex `^[A-Za-z0-9._-]+$` + reject `.` / `..` / empty / whitespace, then `url.PathEscape` | +| 4 | No client-side categorization between 404 / 403 / malformed-id / 5xx | `cmd/traces.go` `tracesGetCmd` has no error mapping | Phase 2 tests + existing `apiErrorCodeForStatus` ([cmd/helpers.go:152-172](cmd/helpers.go:152)) already maps every required HTTP status — no `MapServerCode` extension. | + +## Server error-code findings + +Not observed live (auth-blocked). Plan does not speculate server-code strings — relies on `apiErrorCodeForStatus` HTTP-status mapping which covers every AC#3 category: + +| HTTP | Canonical code | Exit code | +|------|----------------|-----------| +| 400 / 422 | `INVALID_REQUEST` | 4 | +| 401 | `UNAUTHORIZED` | 2 | +| 403 | `TENANT_ACCESS_REVOKED` | 2 | +| 404 | `NOT_FOUND` | 3 | +| 429 | `RESOURCE_EXHAUSTED` | 6 | +| 5xx | `INTERNAL` | 5 | + +This locks AC#3 behavior independent of upstream server-code strings. + +## Upstream issue body + +**Not filed.** Root cause is CLI-side; no upstream issue required for `digitopvn/goclaw`. Phase 3 step 5 short-circuits. + +## Acceptance criteria mapping (preview) + +| AC | Phase | Status after Phase 1 | +|----|-------|----------------------| +| 1. Read trace details by id | Phase 2 (render path + decode-error surfacing) | Pending | +| 2. JSON & human-readable output | Phase 2 (red test 3 + new green tests) | Red test locked | +| 3. Distinct errors: not-found / perm / malformed / server | Phase 2 (HTTP-status mapping; existing `apiErrorCodeForStatus`) | Pending tests | +| 4. Link upstream issue if API root cause | n/a — classified CLI-side | Resolved | +| 5. Regression test | Phase 1 (3 tests) + Phase 2 (7 more) | 3 of 10 landed | + +## Unresolved questions + +- **Real-gateway fixture refresh.** Stub fixture is shaped from `traces follow` payload conventions, not the real `GET /v1/traces/{id}` wire envelope. If the real shape differs materially (e.g. spans nested under `tree`, events under `event_log`), Phase 2 render assertions may need light shape adjustments. Phase 3 code-reviewer gate enforces refresh + re-run before merge.