Conversation
There was a problem hiding this comment.
4 issues found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/appctx/context.go">
<violation number="1" location="internal/appctx/context.go:211">
P1: `a.Hooks.SetTracer(t)` will panic if `a.Hooks` is nil. The lines just above already guard `a.Hooks` with a nil check before calling `SetLevel` — this call should follow the same pattern.</violation>
</file>
<file name="internal/commands/tui.go">
<violation number="1" location="internal/commands/tui.go:53">
P1: Missing nil guard on `app.Hooks` before calling `SetTracer`. The code below already checks `app.Hooks != nil` before `SetLevel(0)`, but this call site does not, risking a nil-pointer panic when `--trace` is passed.</violation>
</file>
<file name="internal/observability/tracer_test.go">
<violation number="1" location="internal/observability/tracer_test.go:19">
P3: Double-close on the tracer: `defer tr.Close()` and the explicit `tr.Close()` both run, but `Close()` doesn't nil-out `file` after closing, so the deferred call closes an already-closed fd. Remove the defer since the explicit close is needed before `ReadFile`.</violation>
</file>
<file name="internal/observability/tracer.go">
<violation number="1" location="internal/observability/tracer.go:73">
P2: The `cats` field is read/written without synchronization. `EnableCategories` performs `t.cats |= cats` while `Log` and `Enabled` read `t.cats`, which is a data race under the Go memory model. Consider using `atomic.Int32` (or `atomic.Int64`) for `cats` to make the read/write race-free, especially since the struct is designed for cross-goroutine use (nil-safe, slog-backed).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
This PR introduces a unified trace system for CLI and TUI observability. It adds a new observability.Tracer type backed by slog JSON output to a file, controlled via the BASECAMP_TRACE environment variable (with backward compatibility for BASECAMP_DEBUG). The TUI gains a --trace flag and 12 instrumentation sites, while SDK hooks log structured HTTP events to the same trace file. A bin/devtools script facilitates live trace viewing via tmux.
Changes:
- New
observability.Tracerwith bitmask-based category filtering (http,tui,all), nil-safe methods, env var parsing, and comprehensive tests - Integration of the tracer into
CLIHooks(structured HTTP event logging),Applifecycle (init inApplyFlags, cleanup inCloseviaPersistentPostRunE), and TUI workspace (12 trace call sites for key presses, navigation, sidebar, overlays, etc.) - New
bin/devtoolsbash script and--traceflag on thetuicommand for developer ergonomics
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
internal/observability/tracer.go |
New Tracer type with slog JSON handler, category bitmask, file management, env var parsing, and path utilities |
internal/observability/tracer_test.go |
Comprehensive tests for Tracer creation, categories, nil-safety, env var parsing, and backward compat |
internal/observability/hooks.go |
Adds tracer field to CLIHooks with SetTracer method; all hook methods now also log to the file-based tracer |
internal/appctx/context.go |
Adds Tracer field to App, initializes tracer in ApplyFlags, adds Close() for resource cleanup |
internal/cli/root.go |
Adds PersistentPostRunE to call app.Close() on exit |
internal/commands/tui.go |
Adds --trace flag, tracer setup/widening, bubbletea debug log separation, and workspace option wiring |
internal/tui/workspace/workspace.go |
Adds Option pattern with WithTracer, trace() helper, and 12 trace instrumentation sites |
bin/devtools |
Bash script for tmux-based live trace viewing during TUI development |
.surface |
Registers new --trace flag in API surface |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace ad-hoc BASECAMP_DEBUG with BASECAMP_TRACE — a structured, slog-backed JSON tracing system. Per-process file naming supports concurrent sessions. - TraceCategories bitmask (TraceHTTP, TraceTUI, TraceAll) with EnableCategories - ParseTraceEnv/ParseTraceEnvWithCacheDir reads env, BASECAMP_DEBUG backcompat tightened to only accept values the verbosity parser recognises (numeric, "true") - TracePath(cacheDir) respects CLI's resolved cache directory and profile scoping - CLIHooks gains SetTracer(); each hook method logs structured JSON independent of stderr verbosity level - App.Tracer field initialized in ApplyFlags; App.Close() for resource cleanup - PersistentPostRunE on root command calls App.Close()
…acing - Workspace gains Option/WithTracer pattern; 12 trace sites covering key presses, navigation, sidebar, account switching, relayout, overlays - `basecamp tui --trace` enables TraceAll (or widens an env-created tracer); bubbletea debug log writes to separate .debug.log to keep JSON trace parseable - bin/devtools launches tmux with TUI + tail -f in a split pane; uses direct argv form (no sh -c) to avoid shell metacharacter issues with URLs/paths
… warning - Guard a.Hooks and app.Hooks with nil checks before SetTracer calls (context.go, tui.go) for consistency with existing SetLevel guards - Use atomic.Int32 for Tracer.cats field so EnableCategories is safe to call concurrently with Log/Enabled from other goroutines - Remove defer/explicit double-close in tracer tests; use single explicit close before ReadFile - Log warning to stderr when NewTracer fails in --trace flag path
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Include "true" in ParseTraceEnv doc comment for TraceAll values - Switch ParseTraceEnv tests to use TempDir to avoid leaking files - Add TestCLIHooks_TracerIntegration verifying SetTracer + hooks write structured trace events to the log file
Summary
BASECAMP_TRACEenv var with structured, slog-backed JSON tracing to file — replaces ad-hocBASECAMP_DEBUG(backward compat preserved for1,2,true)TraceCategoriesbitmask (http,tui,all) controls what gets logged; trace files land in the CLI's resolved cache directory (respects--cache-dirand profiles)basecamp tui --traceenables all-category tracing; 12 instrumentation sites in the workspace (key presses, navigation, sidebar, account switching, relayout, overlays).debug.logfile to keep the JSON trace parseablebin/devtoolsscript launches tmux with TUI +tail -fin a split pane for live trace viewingTest plan
make fmt-check && make vet && make test && make test-e2eall passgo vet ./...cleanpicker.goconfirmed unrelatedBASECAMP_TRACE=http basecamp projectsproduces structured JSON in~/.cache/basecamp/trace-{pid}.logBASECAMP_TRACE=all basecamp tuiproduces both HTTP and TUI eventsBASECAMP_DEBUG=1 basecamp projectsbackward compat maps to TraceHTTPBASECAMP_DEBUG=yes basecamp projectsdoes NOT create a trace file (tightened)basecamp tui --traceprints trace path, enables all categoriesBASECAMP_TRACE=http basecamp tui --tracewidens to all categoriesbin/devtoolsopens tmux split with TUI + tail (requires tmux)bin/devtools 'https://3.basecamp.com/...'URL arg preserved without shell mangling