fix(cli): render trace details by id with validation + exit-code mapping (issue #17)

[mrgoonie]

Summary

Fix goclaw traces get <id> — restore readable TTY output, surface decode failures, validate ids strictly, and map distinct exit codes per failure class. Closes #17.

What was broken

traces get <id> was unusable as documented:

1. TTY output dumped raw JSON — printer.Print(map[string]any) falls back to JSON in table mode.
2. unmarshalMap silently swallowed errors — _ = json.Unmarshal(...) produced empty {} on malformed payload.
3. No id validation — args[0] concatenated raw into URL; path-traversal (.., /, \x00, whitespace) flowed straight to the gateway.
4. All failures collapsed to exit 1 — not-found / permission / server / malformed all indistinguishable from generic.

Fixes

cmd/traces.go

• tracesGetCmd.RunE — strict id validation → HTTP → decode-with-error → render dispatch.
• validateTraceID — regex allowlist ^[A-Za-z0-9._-]+$ + reserved-token block (., .., empty, whitespace). Returns typed *client.APIError{Code: INVALID_REQUEST} → exit 4 via central handler.
• renderTraceTable + buildSpanTree — header card + span tree + flat events list. Insertion-ordered children, cycle-safe (cycles silently drop, no infinite recursion), orphans attach to virtual root.

internal/client/http.go

Latent retry-body bug. The retry loop closed resp.Body on every iteration including the final attempt, then post-loop io.ReadAll failed with "http: read on closed response body". This silently collapsed typed APIError → opaque wrapped error → exit 1, regardless of HTTP status. Fix: skip the per-iteration Close() on the final attempt; outer defer handles cleanup exactly once.

Exit-code contract for traces get

HTTP / condition	Code
Success	0
403 permission denied	2
404 not found	3
Malformed id (pre-HTTP)	4
5xx server failure	5
429 rate-limit	6

Tests

10 new tests in cmd/traces_get_test.go:

• Wire contract (GET /v1/traces/{id}) and JSON round-trip.
• Table mode emits header + span tree markers (├ / └) + EVENTS section.
• 404 → ExitNotFound, 403 → ExitAuth, 5xx → ExitServer.
• Table-driven malformed id (empty, ".", "..", path-traversal, slashes, NUL, whitespace) — asserts exit 4 and zero HTTP calls.
• Malformed response (200 + non-JSON body) — surfaces wrapped decode error.

Fixture cmd/testdata/trace_detail_get.json is a scrubbed stub with _TODO_refresh marker. Auth-blocked smoke probe documented in plans/260528-1357-fix-trace-details-by-id/reports/repro-260528-issue17.md. Refresh against live gateway before merging this PR to main.

Verification

• go vet ./... — clean
• go build ./... — clean
• go test -count=1 ./... — all packages green
• code-reviewer subagent: DONE_WITH_CONCERNS, 0 Critical, 0 Important, 4 informational (all addressed inline or scheduled as follow-up). See plans/260528-1357-fix-trace-details-by-id/reports/code-review-260528-issue17.md.

Acceptance criteria mapping

AC (from issue #17)	Where verified
1. Read trace details by id	renderTraceTable + TestTracesGet_TableMode_HasHeaderAndSpanMarkers
2. JSON + human-readable output	TestTracesGet_JSONMode_PreservesStructure + TestTracesGet_TableMode_*
3. Distinct errors (not-found / perm / malformed / server)	TestTracesGet_NotFound_ExitCode3 / _PermissionDenied_ExitCode2 / _MalformedID_NoHTTPCall / _ServerError_ExitCode5
4. Link upstream issue if API root cause	n/a — classified CLI-side (see Phase 1 repro report)
5. Regression test	10 tests in cmd/traces_get_test.go

Out of scope

• Hardening tracesExportCmd id validation (pre-existing; same vulnerability) — filed as follow-up.
• Fixture refresh against live gateway — gated by the Phase 1 reviewer note; auth-blocked at plan time.

Auto-close timing

This PR targets dev. Closes #17 will auto-close the issue only when dev is promoted to main. A comment will be posted on #17 explaining this.

Plan + reports

• Plan: plans/260528-1357-fix-trace-details-by-id/plan.md
• Repro + root-cause classification: plans/260528-1357-fix-trace-details-by-id/reports/repro-260528-issue17.md
• Code review: plans/260528-1357-fix-trace-details-by-id/reports/code-review-260528-issue17.md

Closes #17

[mrgoonie]

Review (post-CI)

Verdict: Approve — safe to merge to dev.

Findings

Severity	Count
Critical	0
Important	0
Suggestion	1 (deferred — out of scope, already filed as follow-up)

Checks performed

• CI status: all 4 green
  • build (macos-latest, 1.25) ✅
  • build (ubuntu-latest, 1.25) ✅
  • build (windows-latest, 1.25) ✅
  • claude-review ✅
• Correctness: validated locally
  • validateTraceID rejects all 9 malformed-id table cases pre-HTTP (asserted via TestTracesGet_MalformedID_NoHTTPCall: server hit count = 0).
  • buildSpanTree cycle safety: cycles silently drop (never appear in children[""], never recursed). No infinite loop possible.
  • HTTP retry-body fix (internal/client/http.go:161-175): final-attempt Close() skipped so outer defer handles cleanup exactly once. No leak, no double-close.
  • Error mapping: 404→3, 403→2, 5xx→5, 429→6, malformed→4 — all asserted, all pass.
• Security:
  • grep -i 'eyJ|Bearer|sk-|token=' cmd/testdata/ → 0 hits (fixture is scrubbed).
  • Id allowlist ^[A-Za-z0-9._-]+$ + reserved-token block (., ..) gates path-traversal before any HTTP call.
• Hygiene:
  • No plan-artifact refs (Phase N, F1, Y1, audit A, §N) in changed source/comments.
  • Conventional commit format used.
  • PR targets dev (not main), per repo policy.

Suggestions (deferred)

• tracesExportCmd (cmd/traces.go:207) has the same pre-existing un-validated-id pattern that this PR fixes for traces get. Out of scope here; filed as a follow-up task to apply the same validateTraceID + url.PathEscape treatment + regression test.

Notes for merge

• Fixture cmd/testdata/trace_detail_get.json carries _TODO_refresh marker — refresh against live goclaw.zuey.me before promoting dev → main (per Phase 1 reviewer gate).
• Closes #17 will auto-fire only on dev → main promotion. Issue Bug: cannot read trace details by trace id #17 has been commented with this timing.

No actionable findings. Loop terminates (0 fix iterations needed).