From d8177c89c68e6c4317c5da5815b4f6078eb92089 Mon Sep 17 00:00:00 2001 From: Avinash Joshi Date: Thu, 25 Jun 2026 22:24:33 -0700 Subject: [PATCH 1/5] feat(agent+workspace): agent layer v1 foundation (classifier, swap, CurrentAgent state) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - new Classifier interface in internal/agent (IsRendering, IsTrustDialog) with real implementations for codex (table-driven against testdata fixtures), stub implementations for opencode/aider/claude - state.Workspace gains CurrentAgent + AgentLaunches[type]int — per-agent launch counter so first-time swap to a new agent gets the full fresh briefing even when another agent ran first. Migrated lazily from the legacy AgentLaunchCount onto CurrentAgent on first read; omitempty so pre-v0.22 state files are forward-compatible - agent.BuildBriefing(ws, cfg, hints, agentType) keys fresh-vs-resume on the per-agent counter (with legacy AgentLaunchCount fallback when the agent matches ws.CurrentAgent) - codex launcher uses positional [PROMPT] + 'resume --last' (CLI 0.142.2 dropped --instructions); strip-on-empty in PlanLaunch only pops the preceding arg when it starts with '-' so 'on-request' value survives - workspace.SwapAgent orchestrates the swap: capture window-layout, verify launcher installed BEFORE tearing down the pane, kill old agent pane, persist new CurrentAgent, split new pane off the IDE, restore byte-precise geometry, bump per-agent counter - canopy.json learns agents: [...] allowlist; legacy agent.type still honored as a single-agent fallback. Unknown keys preserved via raw JSON-map shim. AddAgentToCanopyJSON for auto-add on pick - tmux.CaptureWindowLayout + SelectLayout helpers for byte-precise geometry restore across kill-pane + split-window Tests: classifier_test (table-driven over launchers), briefing tests pinning swap-to-new-agent and resume gates, launchers tests pinning codex argv survival across the empty-briefing strip, agent_swap_test covering the lifecycle. Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/agent/briefing.go | 39 +- internal/agent/briefing_test.go | 91 +++-- internal/agent/classifier.go | 108 +++++ internal/agent/classifier_aider.go | 27 ++ internal/agent/classifier_claude.go | 43 ++ internal/agent/classifier_codex.go | 104 +++++ internal/agent/classifier_opencode.go | 25 ++ internal/agent/classifier_test.go | 208 ++++++++++ internal/agent/launchers.go | 189 ++++++++- internal/agent/launchers_test.go | 215 ++++++++++ internal/agent/state.go | 78 ++-- internal/agent/state_test.go | 19 +- internal/agent/testdata/README.md | 41 ++ .../agent/testdata/codex_awaiting_input.txt | 50 +++ internal/agent/testdata/codex_idle.txt | 50 +++ internal/agent/testdata/codex_thinking_a.txt | 50 +++ internal/agent/testdata/codex_thinking_b.txt | 50 +++ .../agent/testdata/codex_trust_dialog.txt | 50 +++ internal/config/config.go | 179 ++++++++- internal/config/config_test.go | 245 ++++++++++++ internal/state/listing.go | 9 + internal/state/state.go | 39 ++ internal/tmux/layout.go | 76 ++++ internal/tmux/layout_test.go | 110 +++++ internal/workspace/agent_swap.go | 270 +++++++++++++ internal/workspace/agent_swap_test.go | 377 ++++++++++++++++++ internal/workspace/export_test.go | 15 + internal/workspace/initprompt.go | 60 +-- internal/workspace/lifecycle.go | 150 ++++++- 29 files changed, 2855 insertions(+), 112 deletions(-) create mode 100644 internal/agent/classifier.go create mode 100644 internal/agent/classifier_aider.go create mode 100644 internal/agent/classifier_claude.go create mode 100644 internal/agent/classifier_codex.go create mode 100644 internal/agent/classifier_opencode.go create mode 100644 internal/agent/classifier_test.go create mode 100644 internal/agent/testdata/README.md create mode 100644 internal/agent/testdata/codex_awaiting_input.txt create mode 100644 internal/agent/testdata/codex_idle.txt create mode 100644 internal/agent/testdata/codex_thinking_a.txt create mode 100644 internal/agent/testdata/codex_thinking_b.txt create mode 100644 internal/agent/testdata/codex_trust_dialog.txt create mode 100644 internal/tmux/layout.go create mode 100644 internal/tmux/layout_test.go create mode 100644 internal/workspace/agent_swap.go create mode 100644 internal/workspace/agent_swap_test.go create mode 100644 internal/workspace/export_test.go diff --git a/internal/agent/briefing.go b/internal/agent/briefing.go index 3231b42..fc30c4b 100644 --- a/internal/agent/briefing.go +++ b/internal/agent/briefing.go @@ -14,13 +14,23 @@ import ( // Returns "" when no --append-system-prompt should be passed at all (the // "resume + no active hints" case from the hybrid strategy). // +// agentType is the launcher about to spawn (e.g. "claude", "codex"). The +// fresh/resume decision is keyed on the PER-AGENT launch counter +// (ws.AgentLaunches[agentType]) so a first-time swap from claude → codex +// gets the full fresh briefing — codex has never run in this workspace +// even though claude has. Falling back to the legacy global +// ws.AgentLaunchCount here is the bug codex review caught 2026-06-25 +// (agent_swap.go P1 #1): claude's prior run made the global count > 0, +// so codex's first spawn skipped onto the delta path with no context. +// // Strategy decision tree (per the v0.6 design doc): // -// if AgentLaunchCount == 0: -// # fresh launch — agent has never seen this workspace +// count := ws.AgentLaunches[agentType] +// if count == 0: +// # fresh launch — THIS agent has never seen this workspace // return full briefing (conventions + identity + variant + hints) // -// # resume launch (AgentLaunchCount > 0) — agent already has prior context +// # resume launch (count > 0) — this agent already has prior context // if no active hints: // return "" # don't pass --append-system-prompt at all // return delta briefing (active hints only, framed as "since you last saw") @@ -32,8 +42,8 @@ import ( // state. Hints, on the other hand, may have changed between sessions // (PR merged while detached, branch reachable from main now, ...) — those // the agent genuinely needs to learn about on resume. -func BuildBriefing(ws state.Workspace, cfg *config.Config, hints []state.Hint) string { - if ws.AgentLaunchCount == 0 { +func BuildBriefing(ws state.Workspace, cfg *config.Config, hints []state.Hint, agentType string) string { + if launchCountFor(ws, agentType) == 0 { return buildFullBriefing(ws, cfg, hints) } if len(hints) == 0 { @@ -42,6 +52,25 @@ func BuildBriefing(ws state.Workspace, cfg *config.Config, hints []state.Hint) s return buildDelta(hints) } +// launchCountFor returns the per-agent launch counter for agentType, +// falling back to ws.AgentLaunchCount when the per-agent map is missing +// AND the named agent matches ws.CurrentAgent. That last clause covers +// the brief window between state-file load and the lifecycle migration +// (lifecycle.go's Load populates AgentLaunches from AgentLaunchCount on +// first read, but during state-file rewrites or concurrent reads we may +// still see the pre-migration shape). Without the fallback, an old +// workspace's first post-upgrade launch would re-show the fresh briefing +// — annoying but not wrong, since fresh is a strict superset of delta. +func launchCountFor(ws state.Workspace, agentType string) int { + if n, ok := ws.AgentLaunches[agentType]; ok { + return n + } + if agentType == ws.CurrentAgent { + return ws.AgentLaunchCount + } + return 0 +} + // buildFullBriefing renders the fresh-launch briefing. Sections: // // 1. Header + workspace identity diff --git a/internal/agent/briefing_test.go b/internal/agent/briefing_test.go index a733759..36710ea 100644 --- a/internal/agent/briefing_test.go +++ b/internal/agent/briefing_test.go @@ -12,16 +12,22 @@ import ( ) // fixtureWorkspace returns a populated state.Workspace for tests. -// SourceKind defaults to "fresh"; tests override per-case. +// SourceKind defaults to "fresh"; tests override per-case. CurrentAgent +// is set to "claude" so the BuildBriefing migration-fallback path +// (lifecycle.launchCountFor) maps a non-zero AgentLaunchCount onto the +// "claude" per-agent counter when AgentLaunches is unset — which is the +// shape pre-v0.22 state files have. Tests that exercise the swap-to-codex +// behavior must override AgentLaunches explicitly. func fixtureWorkspace() state.Workspace { return state.Workspace{ - Name: "ancient-hornet", - Branch: "ancient-hornet", - ProjectRoot: "/home/avi/Work/canopy", - Path: "/home/avi/.canopy/workspaces/canopy/ancient-hornet", - Port: 40010, - Status: state.StatusReady, - SourceKind: "fresh", + Name: "ancient-hornet", + Branch: "ancient-hornet", + ProjectRoot: "/home/avi/Work/canopy", + Path: "/home/avi/.canopy/workspaces/canopy/ancient-hornet", + Port: 40010, + Status: state.StatusReady, + SourceKind: "fresh", + CurrentAgent: "claude", } } @@ -40,7 +46,7 @@ func fixtureConfig() *config.Config { func TestBuildBriefing_FreshFull(t *testing.T) { ws := fixtureWorkspace() ws.AgentLaunchCount = 0 - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") wantSections := []string{ "# Canopy workspace context", @@ -70,13 +76,44 @@ func TestBuildBriefing_FreshFull(t *testing.T) { func TestBuildBriefing_ResumeNoHintsReturnsEmpty(t *testing.T) { ws := fixtureWorkspace() ws.AgentLaunchCount = 1 // resumed - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") if out != "" { t.Errorf("resume + no hints should return empty string; got %d bytes:\n%s", len(out), out) } } +// TestBuildBriefing_SwapToNewAgent_GetsFreshBriefing pins the bug codex +// review flagged 2026-06-25 (P1 #1): the FIRST launch of a freshly +// swapped-in agent must get the FULL briefing, even when claude has +// already run in this workspace and the legacy global AgentLaunchCount +// is > 0. Without the per-agent gate, codex spawned with delta-or-empty +// context on its first appearance — invisible to humans but +// catastrophic for the new agent's onboarding. +func TestBuildBriefing_SwapToNewAgent_GetsFreshBriefing(t *testing.T) { + ws := fixtureWorkspace() + // Workspace has been running claude for a while. Legacy global + // counter says 3 launches, per-agent says claude=3, codex=0. + ws.CurrentAgent = "claude" + ws.AgentLaunchCount = 3 + ws.AgentLaunches = map[string]int{"claude": 3} + + // Now we're spawning codex for the first time in this workspace. + out := BuildBriefing(ws, fixtureConfig(), nil, "codex") + + // Must be the FULL briefing — codex needs the workspace context. + for _, want := range []string{ + "# Canopy workspace context", + "## This workspace", + "## Workspace lifecycle (canopy conventions", + "ancient-hornet", + } { + if !strings.Contains(out, want) { + t.Errorf("swap-to-codex first launch should get FULL briefing; missing %q:\n%s", want, out) + } + } +} + // TestBuildBriefing_ResumeWithHintsReturnsDelta: resume + at least one // hint returns the delta-only briefing. Must NOT include the static // lifecycle conventions (those were taught on the fresh launch). @@ -89,7 +126,7 @@ func TestBuildBriefing_ResumeWithHintsReturnsDelta(t *testing.T) { Action: "canopy rm ancient-hornet", DetectedAt: time.Now(), }} - out := BuildBriefing(ws, fixtureConfig(), hints) + out := BuildBriefing(ws, fixtureConfig(), hints, "claude") if !strings.Contains(out, "shipped") { t.Errorf("delta briefing missing hint kind: %s", out) @@ -115,7 +152,7 @@ func TestBuildBriefing_ResumeWithHintsReturnsDelta(t *testing.T) { func TestBuildBriefing_SourceKindPR(t *testing.T) { ws := fixtureWorkspace() ws.SourceKind = "pr" - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") for _, want := range []string{ "pull request", @@ -135,7 +172,7 @@ func TestBuildBriefing_SourceKindPR(t *testing.T) { func TestBuildBriefing_SourceKindIssue(t *testing.T) { ws := fixtureWorkspace() ws.SourceKind = "issue" - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") for _, want := range []string{ "implementing", @@ -156,7 +193,7 @@ func TestBuildBriefing_SourceContextWrapped(t *testing.T) { ws := fixtureWorkspace() ws.SourceKind = "pr" ws.SourceContext = "PR #42: Fix the bug\n\nBody talks about the bug and how to fix it." - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") if !strings.Contains(out, "PR #42: Fix the bug") { t.Errorf("source context body missing from briefing: %s", out) @@ -173,7 +210,7 @@ func TestBuildBriefing_SourceContextEmpty_NoDelimiter(t *testing.T) { ws := fixtureWorkspace() ws.SourceKind = "branch" ws.SourceContext = "" - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") if strings.Contains(out, "<<>>") { t.Errorf("delimiter rendered with no body: %s", out) @@ -184,7 +221,7 @@ func TestBuildBriefing_SourceContextEmpty_NoDelimiter(t *testing.T) { func TestBuildBriefing_SourceKindBranch(t *testing.T) { ws := fixtureWorkspace() ws.SourceKind = "branch" - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") if !strings.Contains(out, "picking up the existing branch") { t.Errorf("branch briefing missing pickup framing: %s", out) @@ -217,7 +254,7 @@ func TestBuildBriefing_RenameDirective_AppliedConditionally(t *testing.T) { ws.SourceKind = tc.sourceKind ws.NameAutoGenerated = tc.nameAuto - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") // The literal `git branch -m ` line is unique to // the rename directive section — robust signature even as the // surrounding copy evolves. @@ -238,7 +275,7 @@ func TestBuildBriefing_PR_TellsAgentNotToRename(t *testing.T) { ws := fixtureWorkspace() ws.SourceKind = "pr" ws.Branch = "pdx91/inbox-improvements" - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") if !strings.Contains(out, "DON'T rename") { t.Errorf("PR briefing must tell agent not to rename: %s", out) @@ -254,7 +291,7 @@ func TestBuildBriefing_Branch_TellsAgentNotToRename(t *testing.T) { ws := fixtureWorkspace() ws.SourceKind = "branch" ws.Branch = "feat/oauth" - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") if !strings.Contains(out, "DON'T rename") { t.Errorf("branch briefing must tell agent not to rename: %s", out) @@ -269,7 +306,7 @@ func TestBuildBriefing_Issue_AllowsLaterRename(t *testing.T) { ws := fixtureWorkspace() ws.SourceKind = "issue" ws.Branch = "issue-42" - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") // Should explicitly mention that rename is OK later but not urgent. if !strings.Contains(out, "not urgent") && !strings.Contains(out, "later") { @@ -290,7 +327,7 @@ func TestBuildBriefing_LegacySourceKindFallsBackToFresh(t *testing.T) { t.Run("legacy empty SourceKind", func(t *testing.T) { ws := fixtureWorkspace() ws.SourceKind = "" - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") // Legacy rows should get the rename directive (matches v0.5 // behavior where every workspace was treated as auto-named). if !strings.Contains(out, "rename the branch") { @@ -305,7 +342,7 @@ func TestBuildBriefing_LegacySourceKindFallsBackToFresh(t *testing.T) { t.Run("unknown future SourceKind", func(t *testing.T) { ws := fixtureWorkspace() ws.SourceKind = "future-unknown-kind" - out := BuildBriefing(ws, fixtureConfig(), nil) + out := BuildBriefing(ws, fixtureConfig(), nil, "claude") // Unknown kind: don't push to rename (conservative), but // still ask the user about intent. if strings.Contains(out, "rename the branch to reflect intent") { @@ -323,7 +360,7 @@ func TestBuildBriefing_ProjectBriefingInline(t *testing.T) { cfg := fixtureConfig() cfg.Agent.Briefing = "This is a Rails 7 app. RSpec for tests." - out := BuildBriefing(fixtureWorkspace(), cfg, nil) + out := BuildBriefing(fixtureWorkspace(), cfg, nil, "claude") if !strings.Contains(out, "## Project briefing") { t.Errorf("missing Project briefing section header") } @@ -351,7 +388,7 @@ func TestBuildBriefing_ProjectBriefingFile(t *testing.T) { }, } - out := BuildBriefing(fixtureWorkspace(), cfg, nil) + out := BuildBriefing(fixtureWorkspace(), cfg, nil, "claude") if !strings.Contains(out, "FROM-FILE-CONTENT") { t.Errorf("file-based briefing not used: %s", out) } @@ -372,7 +409,7 @@ func TestBuildBriefing_ProjectBriefingFileMissing(t *testing.T) { BriefingFile: "missing.md", }, } - out := BuildBriefing(fixtureWorkspace(), cfg, nil) + out := BuildBriefing(fixtureWorkspace(), cfg, nil, "claude") if !strings.Contains(out, "INLINE-FALLBACK") { t.Errorf("inline fallback didn't fire when briefing_file is missing: %s", out) } @@ -389,7 +426,7 @@ func TestBuildBriefing_FreshWithActiveHints(t *testing.T) { Message: "branch matches workspace name", Action: "git branch -m ", }} - out := BuildBriefing(ws, fixtureConfig(), hints) + out := BuildBriefing(ws, fixtureConfig(), hints, "claude") if !strings.Contains(out, "rename_suggested") { t.Errorf("fresh briefing with hints missing the hint") @@ -408,7 +445,7 @@ func TestBuildBriefing_NilConfig(t *testing.T) { t.Errorf("BuildBriefing panicked on nil cfg: %v", r) } }() - out := BuildBriefing(fixtureWorkspace(), nil, nil) + out := BuildBriefing(fixtureWorkspace(), nil, nil, "claude") if !strings.Contains(out, "Canopy workspace context") { t.Errorf("nil cfg produced empty briefing") } diff --git a/internal/agent/classifier.go b/internal/agent/classifier.go new file mode 100644 index 0000000..6a77010 --- /dev/null +++ b/internal/agent/classifier.go @@ -0,0 +1,108 @@ +package agent + +import "regexp" + +// Classifier owns the launcher-specific surfaces that drive the agent +// pane state machine: +// +// - IdleMarkers / AwaitingMarkers — regex slices the Detector matches +// against RAW captured content (not normalize'd) to recognize +// launcher-specific UI states. Pattern matching uses raw because +// normalize() strips footer/spinner/input-prompt lines that the +// idle/awaiting markers usually live IN. +// +// - IsRendering — "is the pane at the agent's own UI right now (vs. +// a fallback shell, vs. mid-spawn)?" Used as the Phase-3 settle +// gate in initprompt.go's trust-dialog state machine. Each launcher +// has its own marker characters (claude ❯, codex › + boxed banner, +// aider >, opencode whatever) so a single regex doesn't work. +// +// - IsTrustDialog — "is the pane currently showing the first-launch +// trust/onboarding prompt that needs a key dismissal?" Returns +// false for launchers without a trust-dialog concept; the +// initprompt.go state machine treats a constant-false result as +// "no trust phase, skip Phase-1's dismissal branch." +// +// One Classifier per registered launcher type. ClassifierFor returns +// an unknownClassifier for unregistered or empty-string launchers; the +// nil-slice convention below keeps that safe. +// +// Why an interface and not pure data (pattern tables keyed by launcher +// string): codex's settle-state isn't a single regex match, it's "is +// the boxed banner present AND no spinner line." Behavior that's more +// than a regex needs Go code; the data lives in unexported pattern +// slices each implementer carries. Hybrid is more code than pure data +// but less than a hand-rolled per-launcher branch in state.go. +type Classifier interface { + // IdleMarkers and AwaitingMarkers are matched against RAW pane + // content (the captured tmux pane string). A nil or empty slice + // is the canonical "no markers; nothing to match" return — the + // Detector ranges over the slice, and Go's range yields zero + // iterations on nil. Callers MUST NOT assume non-nil. + IdleMarkers() []*regexp.Regexp + AwaitingMarkers() []*regexp.Regexp + + // IsRendering reports whether the pane currently shows the + // launcher's own UI (vs. a fallback shell or a transient init + // state). Used as the --prompt Phase-3 settle gate AND by the + // agent_state badge column to distinguish "agent rendered, just + // no spinner" idle from "no agent at all (StateUnknown)" idle. + IsRendering(content string) bool + + // IsTrustDialog reports whether the pane currently shows the + // launcher's first-launch trust/consent prompt. Returns false for + // launchers without a trust-dialog concept (opencode, aider). The + // Phase-1 wait loop in initprompt.go uses this to decide whether + // to send-keys an Enter to dismiss. + IsTrustDialog(content string) bool +} + +// ClassifierFor returns the Classifier for the given launcher type. +// +// Empty launcher → unknownClassifier (defensive; the Detector's caller +// passes the value from LauncherFromRole(roleTag), which is "" when the +// role tag is malformed — we don't want a malformed role to crash the +// state machine). +// +// Unregistered launcher → unknownClassifier (same default; we don't +// fail loudly here because the caller may legitimately ask about a +// launcher whose patterns haven't been dogfooded yet — opencode and +// aider land as Unknown-returning stubs deliberately). +// +// Lookup is a Go map access plus a one-line switch; no allocation. The +// returned Classifier is stateless and safe to share across goroutines. +func ClassifierFor(launcher string) Classifier { + switch launcher { + case "claude": + return claudeClassifier{} + case "codex": + return codexClassifier{} + case "opencode": + return opencodeClassifier{} + case "aider": + return aiderClassifier{} + } + return unknownClassifier{} +} + +// unknownClassifier is the "no markers registered, no idle / awaiting / +// trust signals" fallback. Returns nil slices and constant false. Used +// for unrecognized launchers AND as the placeholder for opencode/aider +// during the transitional period before their patterns ship. +// +// Two important invariants this enforces: +// +// 1. range over a nil regex slice yields zero iterations (Go spec) — +// so the Detector's loop bodies are safe without a nil guard. +// +// 2. IsRendering/IsTrustDialog both return false — so unknown +// launchers neither trip the Phase-3 settle gate (canopy refuses +// to send-keys until SOMETHING claims the pane is the agent's UI) +// NOR get treated as showing a trust dialog (canopy doesn't blindly +// hammer Enter into an unknown pane). Defensive default for both. +type unknownClassifier struct{} + +func (unknownClassifier) IdleMarkers() []*regexp.Regexp { return nil } +func (unknownClassifier) AwaitingMarkers() []*regexp.Regexp { return nil } +func (unknownClassifier) IsRendering(content string) bool { return false } +func (unknownClassifier) IsTrustDialog(content string) bool { return false } diff --git a/internal/agent/classifier_aider.go b/internal/agent/classifier_aider.go new file mode 100644 index 0000000..7c48a87 --- /dev/null +++ b/internal/agent/classifier_aider.go @@ -0,0 +1,27 @@ +package agent + +import "regexp" + +// aiderClassifier is a deliberate Unknown-returning stub. aider's TUI +// markers aren't dogfooded yet AND aider's interaction model is +// different enough (--yes-always vs. interactive permission flow per +// git-remote-presence) that the awaiting-markers regex set probably +// needs a different shape than claude/codex. +// +// Per the original codex-support design's "Sequencing" note: aider +// ships LAST, after we've used codex + opencode classifiers long +// enough to be sure the Classifier interface signature actually +// suits aider's "permission as flag, not as dialog" world. If it +// doesn't, the interface gets extended before aider patterns land. +// +// Until then: badge column renders `·` for aider workspaces (honest +// over a wrong-positive 💤). +// +// Tracking: TODOS.md "OPEN — v0.16.x — Extend --prompt / background +// workspaces to codex + opencode" — aider is wave 3. +type aiderClassifier struct{} + +func (aiderClassifier) IdleMarkers() []*regexp.Regexp { return nil } +func (aiderClassifier) AwaitingMarkers() []*regexp.Regexp { return nil } +func (aiderClassifier) IsRendering(content string) bool { return false } +func (aiderClassifier) IsTrustDialog(content string) bool { return false } diff --git a/internal/agent/classifier_claude.go b/internal/agent/classifier_claude.go new file mode 100644 index 0000000..2add667 --- /dev/null +++ b/internal/agent/classifier_claude.go @@ -0,0 +1,43 @@ +package agent + +import "regexp" + +// claudeClassifier is the Classifier implementation for the claude +// launcher. It's a thin adapter over the existing package-level +// claudeIdleMarkers / claudeAwaitingPatterns slices and the +// IsClaudeRendering / IsTrustDialog helper functions in state.go — +// which are kept as deprecated package-level shims so that callers +// outside this package (workspace/initprompt.go etc.) keep compiling +// while the Classifier interface is the going-forward dispatch. +// +// Behavior MUST be byte-identical to pre-classifier-refactor canopy: +// every method here either returns the existing pattern slice +// unchanged or calls the existing helper unchanged. The existing +// claude E2E and unit tests are the regression pin (see CLAUDE.md's +// test-discipline section — a regression in claude classification +// breaks the most-used path). +// +// Why the wrapper instead of moving the patterns into this file: +// the patterns are tightly coupled to normalize() (state.go) and +// the bottomLines helper (state.go); moving them risks churn in the +// /diff for the same observable behavior. The hybrid interface + +// data-table convention (see the D2 design call in the codex-support +// design doc) explicitly anticipates this — patterns are data, the +// interface is the seam. +type claudeClassifier struct{} + +func (claudeClassifier) IdleMarkers() []*regexp.Regexp { + return claudeIdleMarkers +} + +func (claudeClassifier) AwaitingMarkers() []*regexp.Regexp { + return claudeAwaitingPatterns +} + +func (claudeClassifier) IsRendering(content string) bool { + return IsClaudeRendering(content) +} + +func (claudeClassifier) IsTrustDialog(content string) bool { + return IsTrustDialog(content) +} diff --git a/internal/agent/classifier_codex.go b/internal/agent/classifier_codex.go new file mode 100644 index 0000000..f4ef368 --- /dev/null +++ b/internal/agent/classifier_codex.go @@ -0,0 +1,104 @@ +package agent + +import "regexp" + +// codexClassifier is the Classifier implementation for the codex +// launcher (the `codex` CLI from OpenAI's Codex project, NOT the +// /codex gstack skill or any other "codex" naming collision). +// +// Patterns below were dogfooded 2026-06-17 against codex-cli 0.140.0 +// running model gpt-5.5. The literal pane captures used as fixtures +// live in internal/agent/testdata/codex_*.txt; classifier_codex_test.go +// asserts each State classification against those captures. +// +// Codex's UI shape vs. claude's, as of 2026-06: +// +// - Idle marker: boxed banner `╭─...─╮ │ >_ OpenAI Codex (v... │ ╰─...─╯`. +// The banner stays visible at the top of the pane between turns, +// so matching it anywhere on screen is reliable. The footer +// `gpt-5.5 default · ` ALSO marks idle but the model name is +// going to change — banner is the more durable signal. +// +// - Awaiting marker: numbered-option approval dialog ending in +// `Press enter to confirm or esc to cancel`. Only fires when codex +// is spawned with --ask-for-approval on-request|untrusted (default +// mode auto-applies edits with no UI). canopy spawns codex with +// on-request — see launchers.go's codex entry. +// +// - Trust dialog: first-launch `Do you trust the contents of this +// directory?` prompt with the same 1./2. numbered selector shape +// as the awaiting dialog. The "Do you trust" prefix disambiguates. +// +// - Spinner line: `• Working (Ns • esc to interrupt)`. The timer +// increments every second; normalize() in state.go strips this line +// so a stuck-mid-tool-call pane doesn't flip-flop the activity hash. +// See spinnerLine regex (state.go) for the stripping. +// +// Same "last verified" convention as launchers.go's codex comment: +// when codex-cli ships a UI overhaul, re-capture fixtures, update the +// regexes, and bump the date. +type codexClassifier struct{} + +// codexIdleMarkers are codex-only UI elements that prove the pane is +// rendering codex (not the keepAlive shell, not a stale scrollback). +// The boxed banner is the load-bearing match; the footer is a softer +// backup. Both stay visible between turns. +// +// Last verified: 2026-06-17 against codex-cli 0.140.0. +var codexIdleMarkers = []*regexp.Regexp{ + regexp.MustCompile(`>_ OpenAI Codex \(v`), // banner header + regexp.MustCompile(`/model to change`), // banner row + regexp.MustCompile(`(?m)^\s+gpt-[\d.]+\s+\w+\s+·\s+`), // footer (model · cwd) +} + +// codexAwaitingPatterns are codex-TUI markers that mean a user action +// is required RIGHT NOW (approve/deny a proposed action). Only fires +// in --ask-for-approval on-request|untrusted modes; canopy uses +// on-request by default (see launchers.go). +// +// The numbered-Yes-option pattern (`› 1. Yes,`) is intentionally NOT +// here even though it appears in awaiting dialogs — codex's trust +// dialog uses the same numbered selector with `› 1. Yes, continue`, +// and we don't want to flag the trust state as "awaiting input" +// (canopy auto-dismisses trust dialogs; awaiting needs user action). +// The footer + edit-prefix patterns below are specific to the +// approval-required-action dialog and don't collide with trust. +// +// Last verified: 2026-06-17 against codex-cli 0.140.0. +var codexAwaitingPatterns = []*regexp.Regexp{ + regexp.MustCompile(`Press enter to confirm or esc to cancel`), // approval dialog footer + regexp.MustCompile(`Would you like to make the following edits\?`), // file-edit approval prefix +} + +func (codexClassifier) IdleMarkers() []*regexp.Regexp { return codexIdleMarkers } +func (codexClassifier) AwaitingMarkers() []*regexp.Regexp { return codexAwaitingPatterns } + +// codexRenderingMarkers is the subset of idle markers used for the +// Phase-3 settle check. Same patterns as IdleMarkers; codex's UI +// doesn't have a separate "rendering but not idle" footer the way +// claude's `⏵⏵ auto mode on` distinguishes mode states. +// +// Matched against the bottom 12 lines (same bottomLines helper as +// claude) so stale banner in scrollback doesn't pass the check after +// codex crashes back to a shell. +func (codexClassifier) IsRendering(content string) bool { + tail := bottomLines(content, 12) + for _, p := range codexIdleMarkers { + if p.MatchString(tail) { + return true + } + } + return false +} + +// codexTrustDialogPattern matches codex's first-launch consent prompt +// for the working directory. The "Do you trust" prefix is highly +// specific; it can't collide with codex's other numbered-selector +// dialogs (file-edit approval, /model picker, etc.). +// +// Last verified: 2026-06-17 against codex-cli 0.140.0. +var codexTrustDialogPattern = regexp.MustCompile(`Do you trust the contents of this directory\?`) + +func (codexClassifier) IsTrustDialog(content string) bool { + return codexTrustDialogPattern.MatchString(content) +} diff --git a/internal/agent/classifier_opencode.go b/internal/agent/classifier_opencode.go new file mode 100644 index 0000000..a083fed --- /dev/null +++ b/internal/agent/classifier_opencode.go @@ -0,0 +1,25 @@ +package agent + +import "regexp" + +// opencodeClassifier is a deliberate Unknown-returning stub. opencode's +// TUI markers haven't been dogfooded yet; until they are, the pane's +// badge column (⚡💤✋·) will render `·` for opencode workspaces — which +// is honest: canopy can't classify what it hasn't observed. Better than +// a wrong badge. +// +// To fill in: spawn opencode in a real workspace, capture the boot +// state / idle state / approval state (opencode's tool-call permission +// has a different shape than codex's per @docs), save raw captures as +// internal/agent/testdata/opencode_*.txt, replace the nil slices with +// real regex patterns, and ship a classifier_opencode_test.go pair. +// +// Tracking: TODOS.md "OPEN — v0.16.x — Extend --prompt / background +// workspaces to codex + opencode" — opencode is the wave-2 launcher +// after codex parity ships. +type opencodeClassifier struct{} + +func (opencodeClassifier) IdleMarkers() []*regexp.Regexp { return nil } +func (opencodeClassifier) AwaitingMarkers() []*regexp.Regexp { return nil } +func (opencodeClassifier) IsRendering(content string) bool { return false } +func (opencodeClassifier) IsTrustDialog(content string) bool { return false } diff --git a/internal/agent/classifier_test.go b/internal/agent/classifier_test.go new file mode 100644 index 0000000..87ab1df --- /dev/null +++ b/internal/agent/classifier_test.go @@ -0,0 +1,208 @@ +package agent + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestClassifierFor_RegistryLookup walks every known + every stub + +// the empty-string + a hand-crafted unknown launcher, and asserts the +// returned concrete type via Go's type switch. The point isn't the +// behavior (other tests cover that); it's that the dispatch table in +// ClassifierFor stays in sync as launchers are added. +func TestClassifierFor_RegistryLookup(t *testing.T) { + cases := []struct { + launcher string + typeCheck func(c Classifier) bool + typeName string + }{ + {"claude", func(c Classifier) bool { _, ok := c.(claudeClassifier); return ok }, "claudeClassifier"}, + {"codex", func(c Classifier) bool { _, ok := c.(codexClassifier); return ok }, "codexClassifier"}, + {"opencode", func(c Classifier) bool { _, ok := c.(opencodeClassifier); return ok }, "opencodeClassifier"}, + {"aider", func(c Classifier) bool { _, ok := c.(aiderClassifier); return ok }, "aiderClassifier"}, + {"", func(c Classifier) bool { _, ok := c.(unknownClassifier); return ok }, "unknownClassifier"}, + {"future-gpt-pilot", func(c Classifier) bool { _, ok := c.(unknownClassifier); return ok }, "unknownClassifier"}, + } + for _, tc := range cases { + t.Run(tc.launcher, func(t *testing.T) { + c := ClassifierFor(tc.launcher) + if !tc.typeCheck(c) { + t.Errorf("ClassifierFor(%q) returned %T; want %s", tc.launcher, c, tc.typeName) + } + }) + } +} + +// TestClassifierFor_StubsReturnNilSlicesAndFalse pins the contract for +// stub + unknown launchers. The nil-slice convention is what lets the +// Detector range-iterate without a nil guard at every callsite — if a +// stub ever started returning a non-nil empty slice (which would be +// equivalent semantically but waste an allocation), that's a code-smell +// worth catching here. +func TestClassifierFor_StubsReturnNilSlicesAndFalse(t *testing.T) { + for _, launcher := range []string{"", "opencode", "aider", "future-gpt-pilot"} { + t.Run(launcher, func(t *testing.T) { + c := ClassifierFor(launcher) + if c.IdleMarkers() != nil { + t.Errorf("IdleMarkers() = %v; want nil for stub %q", c.IdleMarkers(), launcher) + } + if c.AwaitingMarkers() != nil { + t.Errorf("AwaitingMarkers() = %v; want nil for stub %q", c.AwaitingMarkers(), launcher) + } + if c.IsRendering("any content") { + t.Errorf("IsRendering = true; want false for stub %q", launcher) + } + if c.IsTrustDialog("Do you trust the contents of this directory?") { + t.Errorf("IsTrustDialog = true; want false for stub %q (even on text that LOOKS like a trust prompt)", launcher) + } + }) + } +} + +// TestClassifierFor_NilSafeRangeIteration is the load-bearing safety +// guarantee: ranging over a nil regex slice yields zero iterations and +// does NOT panic. Without this, the Detector's loops in state.go would +// need an `if markers != nil` guard at every callsite. Pin it in a test +// so a refactor that switches to returning sentinel empty slices (which +// would also work) is a deliberate choice, not an accidental one. +func TestClassifierFor_NilSafeRangeIteration(t *testing.T) { + c := ClassifierFor("opencode") // any stub + count := 0 + for range c.IdleMarkers() { + count++ + } + for range c.AwaitingMarkers() { + count++ + } + if count != 0 { + t.Errorf("ranged %d times over stub markers; want 0", count) + } +} + +// TestClassifierFor_ClaudeWrapsExistingPatterns proves the refactor is +// pure: claudeClassifier returns the SAME slice pointers (not just +// equal contents) as the package-level claudeIdleMarkers / +// claudeAwaitingPatterns vars. If a future refactor accidentally +// allocates new slices, the test catches it — that'd silently break +// any code that holds a long-lived reference to the package-level vars +// (none today, but the contract matters for the deprecated shims). +func TestClassifierFor_ClaudeWrapsExistingPatterns(t *testing.T) { + c := ClassifierFor("claude") + if &c.IdleMarkers()[0] != &claudeIdleMarkers[0] { + t.Errorf("claudeClassifier.IdleMarkers() didn't return the package-level slice") + } + if &c.AwaitingMarkers()[0] != &claudeAwaitingPatterns[0] { + t.Errorf("claudeClassifier.AwaitingMarkers() didn't return the package-level slice") + } +} + +// TestCodexClassifier_AgainstRealFixtures runs the codex classifier +// against the captures saved during the 2026-06-17 dogfood spike. The +// fixtures are the load-bearing ground truth for codex pattern +// correctness — when codex-cli ships a UI change that drifts the +// regexes, the fixtures get re-captured (see testdata/README.md) and +// the patterns are bumped together. +// +// Each row asserts the SEMANTIC outcome (what ClassifyOneShot returns, +// plus IsTrustDialog), not the per-marker hit. Per-marker asserts are +// brittle: the awaiting fixture still contains the codex banner at +// the top (so IdleMarkers also match), but ClassifyOneShot returns +// AwaitingInput because awaiting beats idle in match order. Testing +// the state, not the individual matchers, captures the real contract. +func TestCodexClassifier_AgainstRealFixtures(t *testing.T) { + cases := []struct { + fixture string + wantState State + wantTrust bool + wantRender bool + }{ + { + // Trust dialog hides the banner from the bottom of the pane + // and matches neither idle nor (post-tightening) awaiting + // patterns. IsTrustDialog is the only positive signal. + fixture: "codex_trust_dialog.txt", + wantState: StateUnknown, + wantTrust: true, + wantRender: false, + }, + { + fixture: "codex_idle.txt", + wantState: StateIdle, + wantTrust: false, + wantRender: true, + }, + { + // ClassifyOneShot can't see motion — it can only do pattern + // matching. The thinking fixture's banner + footer still + // match idle markers, so static classification falls into + // Idle. The motion-based StateThinking only comes from + // ClassifyTwoShot / Detector.Observe (see spinner-stripped + // test below for that path). + fixture: "codex_thinking_a.txt", + wantState: StateIdle, + wantTrust: false, + wantRender: true, + }, + { + // Awaiting dialog: ClassifyOneShot returns AwaitingInput + // even though the banner at the top of the pane also + // matches an idle marker — awaiting beats idle in order. + fixture: "codex_awaiting_input.txt", + wantState: StateAwaitingInput, + wantTrust: false, + wantRender: false, // approval dialog takes over bottom 12 lines + }, + } + for _, tc := range cases { + t.Run(tc.fixture, func(t *testing.T) { + content := readFixture(t, tc.fixture) + if got := ClassifyOneShot("codex", content); got != tc.wantState { + t.Errorf("ClassifyOneShot(codex, %s) = %v; want %v", + tc.fixture, got, tc.wantState) + } + c := ClassifierFor("codex") + if got := c.IsTrustDialog(content); got != tc.wantTrust { + t.Errorf("IsTrustDialog(%s) = %v; want %v", + tc.fixture, got, tc.wantTrust) + } + if got := c.IsRendering(content); got != tc.wantRender { + t.Errorf("IsRendering(%s) = %v; want %v", + tc.fixture, got, tc.wantRender) + } + }) + } +} + +// TestCodexClassifier_ThinkingSpinnerStripped verifies that normalize() +// strips codex's `• Working (Ns • esc to interrupt)` line. The two +// thinking fixtures differ ONLY in the timer (1s vs 3s); after +// normalization, the rest of the content is identical, so the +// hash-based stability check in Detector.Observe stays stable across +// the second-by-second timer flips. Pre-refactor, this exact bug would +// have flagged codex as Thinking forever any time it was rendering its +// own UI (because the timer in the spinner line always changes). +func TestCodexClassifier_ThinkingSpinnerStripped(t *testing.T) { + a := readFixture(t, "codex_thinking_a.txt") + b := readFixture(t, "codex_thinking_b.txt") + if a == b { + t.Fatal("fixtures identical pre-normalize — captures may have been re-taken without motion") + } + if na, nb := normalize(a), normalize(b); na != nb { + t.Errorf("normalize() didn't strip the codex spinner timer:\nA=%q\nB=%q", na, nb) + } +} + +// readFixture loads internal/agent/testdata/. Centralized so the +// path lookup only lives in one place. +func readFixture(t *testing.T, name string) string { + t.Helper() + path := filepath.Join("testdata", name) + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read fixture %s: %v", path, err) + } + return strings.TrimRight(string(data), "\n") +} + diff --git a/internal/agent/launchers.go b/internal/agent/launchers.go index 1142ebc..4b1e02c 100644 --- a/internal/agent/launchers.go +++ b/internal/agent/launchers.go @@ -31,6 +31,40 @@ var log = clog.Pkg("agent") // list of known types so users can fix the typo without re-reading docs. var ErrUnknownAgent = errors.New("agent: unknown agent type") +// ErrAgentNotAllowed is returned by callers that validate a user- +// supplied agent name against the project's canopy.json `agents:` +// allowlist. The agent IS a valid registered launcher (otherwise the +// caller would have returned ErrUnknownAgent first), but the current +// project hasn't declared it as available. Wrappers include the +// rejected type + the project's allowed list in the error message. +// +// Returned by: +// - cmd/canopy/new.go when --agent is not in Cfg.Agents +// - cmd/canopy/agent.go (canopy agent swap) for the same gate +var ErrAgentNotAllowed = errors.New("agent: not allowed by this project's canopy.json `agents` list") + +// ErrLauncherNoExec is returned by Launcher.ResolveExec when the chosen +// launcher's Exec field is nil — i.e., it has no registered one-shot +// mode. Returned by `canopy ask ` when the target is a launcher +// like opencode that doesn't (yet) have a ` exec`-style entry +// point. v0.22. +var ErrLauncherNoExec = errors.New("agent: launcher has no one-shot exec mode registered") + +// ResolveExec returns the ExecMode for this launcher (used by +// `canopy ask `). Returns ErrLauncherNoExec when the launcher +// has no registered one-shot mode — the caller surfaces this so the +// user sees "this agent doesn't support `ask` yet" rather than a +// confusing exec.LookPath failure later. +// +// The returned *ExecMode is a pointer into the package-level defaults +// map and must NOT be mutated by callers. Treated as read-only. +func (l Launcher) ResolveExec() (*ExecMode, error) { + if l.Exec == nil { + return nil, fmt.Errorf("%w: %q", ErrLauncherNoExec, l.Cmd) + } + return l.Exec, nil +} + // BriefingMode describes how a launcher accepts the canopy-assembled // briefing. Different agents have different conventions: // @@ -73,6 +107,43 @@ type Launcher struct { Resume []string Fresh []string BriefingMode BriefingMode + + // Exec describes how to invoke this launcher in one-shot, non- + // interactive mode for `canopy ask ` (v0.22). Nil means + // the launcher has no known one-shot mode and `canopy ask` returns + // ErrLauncherNoExec. Distinct from Resume/Fresh (which spawn the + // interactive TUI in the agent pane) because exec mode skips the + // approval / state-machine surface entirely. + Exec *ExecMode +} + +// PromptMode picks how the user's question reaches the launcher's exec +// invocation. Each one-shot CLI accepts the prompt body differently: +// +// - claude -p → PromptArg (positional) +// - codex exec → PromptArg (positional) +// - aider --message → PromptArg (positional after the flag) +// +// PromptStdin is reserved for future launchers whose exec mode reads +// from stdin instead of a positional arg. None ship in v0.22. +type PromptMode int + +const ( + PromptArg PromptMode = iota + PromptStdin +) + +// ExecMode describes a launcher's one-shot invocation. Args is the +// pre-prompt argv tail (everything between Cmd and the prompt body). +// PromptMode decides where the prompt body goes: +// +// - PromptArg → final positional argv element +// - PromptStdin → piped to the child's stdin +// +// Used by Launcher.ResolveExec + cmd/canopy/ask.go. +type ExecMode struct { + Args []string + PromptMode PromptMode } // defaults is the registry of canopy-supported agents. Order is @@ -102,29 +173,75 @@ var defaults = map[string]Launcher{ Resume: []string{"--continue", "--append-system-prompt", "{{briefing}}"}, Fresh: []string{"--append-system-prompt", "{{briefing}}"}, BriefingMode: BriefingInline, + // `claude -p ` is claude's non-interactive "print + // mode" — answers the prompt once and exits. No TUI, no + // session continuity, fast. Used by `canopy ask claude`. + Exec: &ExecMode{Args: []string{"-p"}, PromptMode: PromptArg}, }, "codex": { Cmd: "codex", - // Codex's CLI is still moving; --instructions is the closest - // equivalent to claude's --append-system-prompt as of 2026-04. - // Update this entry when codex stabilizes its system-prompt - // surface. No --resume distinction today — codex resume - // requires a thread ID we don't track. - Resume: []string{"--instructions", "{{briefing}}"}, - Fresh: []string{"--instructions", "{{briefing}}"}, + // Codex's CLI keeps moving. As of codex-cli 0.142.2 (2026-06-25), + // the system-prompt surface is GONE — `--instructions` no longer + // exists. The only way to pass content at launch is the + // positional [PROMPT] arg, which codex treats as the user's + // first turn (not a system instruction). We use it anyway: + // the canopy briefing as first-turn user message is the best + // available substitute. + // + // Resume now uses `codex resume --last [PROMPT]` (added between + // 0.140 and 0.142). --last continues the most recent codex + // session — same behavior as `claude --continue`, with the same + // caveat: "most recent" is GLOBAL, not per-cwd. If the user + // runs codex in another directory between two canopy-driven + // codex launches in this workspace, --last picks up the wrong + // session. Per-session-ID tracking (codex resume ) would + // fix this; tracking the UUID requires parsing codex's session + // list or output. Filed as TODO. + // + // --ask-for-approval on-request: codex's default mode auto- + // applies file edits with no UI dialog at all. canopy's agent- + // pane state machine relies on observing an "awaiting input" + // dialog (the AwaitingMarkers Classifier) to render the ✋ + // badge and gate --prompt delivery. Forcing on-request makes + // codex pause for user confirmation before mutating, which is + // (a) the same gating UX claude has by default and (b) what + // canopy needs to classify pane state at all. Awaiting dialog + // shape captured in internal/agent/testdata/codex_awaiting_input.txt. + Resume: []string{"resume", "--last", "--ask-for-approval", "on-request", "{{briefing}}"}, + Fresh: []string{"--ask-for-approval", "on-request", "{{briefing}}"}, BriefingMode: BriefingInline, + // `codex exec ` is codex's non-interactive mode. + // Intentionally OMITs --ask-for-approval (which lives on the + // interactive Resume/Fresh argv): exec mode has no UI to + // surface approval dialogs through, so the flag would either + // be ignored or hang. Dogfooded 2026-06-25 — exec mode + // completes synchronously without an approval prompt. + Exec: &ExecMode{Args: []string{"exec"}, PromptMode: PromptArg}, }, "opencode": { - Cmd: "opencode", + Cmd: "opencode", + // opencode's resume verb wiring is TODO — the installed binary + // on 2026-06-25 fails to start ("Could not resolve npm bin for + // opencode-ai"), so we can't dogfood the flag surface to wire + // it correctly. Resume kept empty until verified; the agent + // will spawn fresh every time, no different from today. + // Same TODO for the Exec field below (no `canopy ask opencode`). Resume: []string{}, Fresh: []string{}, BriefingMode: BriefingAgentsMd, + Exec: nil, }, "aider": { Cmd: "aider", Resume: []string{"--restore-chat-history", "--message-file", "{{briefing}}"}, Fresh: []string{"--message-file", "{{briefing}}"}, BriefingMode: BriefingFile, + // `aider --message ` runs a single-turn aider + // invocation that exits after the response. The Args here + // only carries --message; --no-stream / --no-pretty are + // useful for non-TTY captures but skipped to keep the v1 + // argv minimal (the caller can pipe through `cat` if needed). + Exec: &ExecMode{Args: []string{"--message"}, PromptMode: PromptArg}, }, } @@ -169,6 +286,29 @@ func LauncherFromRole(role string) string { return parts[0] // may be "" if rest starts with ':' } +// InstalledLaunchers returns the subset of KnownAgents whose binary is +// currently on PATH. Used by the TUI agent-swap + ask pickers to show +// only launchers the user could actually run RIGHT NOW. v0.22. +// +// Why "installed" not "known": showing all registered launchers in the +// picker lets the user pick something that would fail at spawn time +// with a "binary not found" error — bad UX. Pre-filtering to installed +// keeps the picker honest about what the user can use. +// +// The check is a cheap exec.LookPath per launcher; for the four +// shipped launchers this is sub-millisecond total. Re-checked on +// every picker open (cheap enough; matches the picker open cadence). +func InstalledLaunchers() []string { + out := make([]string, 0, len(defaults)) + for _, name := range KnownAgents() { + l := defaults[name] + if err := l.VerifyInstalled(); err == nil { + out = append(out, name) + } + } + return out +} + // KnownAgents returns the sorted list of built-in agent type names. // Used by config.validate's error messages and `canopy init --with-scripts // --agent ` to list valid choices. @@ -290,9 +430,19 @@ func (l Launcher) PlanLaunch(briefingPath string, resume bool, worktreePath stri } // Token at position i. if briefingPath == "" { - // Drop this token AND the preceding flag, if any. + // Drop this token. Also drop the preceding arg ONLY when + // it looks like a flag name (starts with "-"). For codex's + // post-0.142.2 positional briefing the preceding arg is the + // flag VALUE of --ask-for-approval (e.g. "on-request") and + // MUST be kept; popping it would produce + // `codex resume --last --ask-for-approval` and codex would + // reject the missing value. Same shape and same reason as + // the guard in BuildArgv. (codex review P1 #2, 2026-06-25.) if len(parts) > 1 { - parts = parts[:len(parts)-1] + prev := parts[len(parts)-1] + if strings.HasPrefix(prev, "-") { + parts = parts[:len(parts)-1] + } } continue } @@ -412,8 +562,11 @@ func (l Launcher) BuildArgv(resume bool, briefing string) []string { // Walk the tail. For each "{{briefing}}" token: if briefing is // non-empty, replace inline and keep the preceding flag. If empty, - // drop both the flag and the token (the prior arg is assumed to be - // the flag this token is the value for). + // drop the token; AND drop the preceding arg if it's a flag (looks + // like "--xxx" or "-x"). For purely-positional briefing arrangements + // (e.g., codex post-0.142.2 where the briefing is just [PROMPT] + // with no preceding flag name), we keep the preceding arg intact + // because it's a flag VALUE, not a flag NAME. out := make([]string, 0, len(tail)+1) out = append(out, l.Cmd) for i := 0; i < len(tail); i++ { @@ -422,11 +575,17 @@ func (l Launcher) BuildArgv(resume bool, briefing string) []string { out = append(out, arg) continue } - // Token at position i. If briefing is empty, drop it AND the - // preceding flag (out's last element). + // Token at position i. If briefing is empty, drop it. ALSO drop + // the preceding arg if it looks like a flag — that's the + // claude/aider case where the briefing is a flag value. For + // codex's positional briefing the preceding arg is a flag value + // (e.g., "on-request") and must be kept. if briefing == "" { if len(out) > 1 { - out = out[:len(out)-1] // pop the preceding flag + prev := out[len(out)-1] + if strings.HasPrefix(prev, "-") { + out = out[:len(out)-1] // pop the preceding flag + } } continue } diff --git a/internal/agent/launchers_test.go b/internal/agent/launchers_test.go index 9237419..0a12ec5 100644 --- a/internal/agent/launchers_test.go +++ b/internal/agent/launchers_test.go @@ -49,6 +49,69 @@ func TestResolve_UnknownReturnsError(t *testing.T) { } } +// TestResolveExec_RegisteredLaunchers covers the v0.22 ExecMode wiring. +// Each launcher with an Exec entry must return its mapped Args + +// PromptMode without error. opencode (no Exec mode known as of 2026-06) +// returns ErrLauncherNoExec — that's the contract `canopy ask` uses +// to surface "this agent doesn't support quick-query yet." +func TestResolveExec_RegisteredLaunchers(t *testing.T) { + cases := []struct { + launcher string + wantArgs []string + wantMode PromptMode + wantErrSnt error // when non-nil, expect errors.Is(err, snt) + }{ + {"claude", []string{"-p"}, PromptArg, nil}, + {"codex", []string{"exec"}, PromptArg, nil}, + {"aider", []string{"--message"}, PromptArg, nil}, + {"opencode", nil, 0, ErrLauncherNoExec}, + } + for _, tc := range cases { + t.Run(tc.launcher, func(t *testing.T) { + l, err := Resolve(tc.launcher) + if err != nil { + t.Fatalf("Resolve(%q): %v", tc.launcher, err) + } + exec, err := l.ResolveExec() + if tc.wantErrSnt != nil { + if !errors.Is(err, tc.wantErrSnt) { + t.Fatalf("ResolveExec err = %v; want errors.Is(..., %v)", err, tc.wantErrSnt) + } + return + } + if err != nil { + t.Fatalf("ResolveExec: %v", err) + } + if !equalSlice(exec.Args, tc.wantArgs) { + t.Errorf("Args = %v; want %v", exec.Args, tc.wantArgs) + } + if exec.PromptMode != tc.wantMode { + t.Errorf("PromptMode = %v; want %v", exec.PromptMode, tc.wantMode) + } + }) + } +} + +// TestResolveExec_CodexOmitsApprovalFlag pins the design decision: the +// codex Exec.Args must NOT include --ask-for-approval. The interactive +// pane uses on-request mode (so canopy can detect awaiting-input), but +// exec mode is non-interactive and the approval flag would either be +// ignored or hang waiting for a UI that doesn't exist. Pin the absence +// so a future refactor that "consolidates" codex args doesn't +// reintroduce it. +func TestResolveExec_CodexOmitsApprovalFlag(t *testing.T) { + l, _ := Resolve("codex") + exec, err := l.ResolveExec() + if err != nil { + t.Fatalf("ResolveExec(codex): %v", err) + } + for _, a := range exec.Args { + if a == "--ask-for-approval" { + t.Errorf("codex Exec.Args contains --ask-for-approval (%v); exec mode must omit it", exec.Args) + } + } +} + func TestRoleForType(t *testing.T) { tests := []struct { name, in, want string @@ -131,6 +194,105 @@ func TestBuildArgv_ClaudeFreshNoBriefing(t *testing.T) { } } +// TestBuildArgv_CodexCarriesApprovalFlag: codex's argv must include +// --ask-for-approval on-request in both Fresh and Resume, followed by +// the briefing as POSITIONAL prompt. Resume additionally prefixes +// `resume --last` (codex's continue-most-recent verb, equivalent to +// `claude --continue`). Without the approval flag, codex auto-applies +// edits and the AwaitingMarkers classifier never fires. +// +// As of codex-cli 0.142.2 (2026-06-25), codex removed --instructions +// entirely and added `codex resume --last`. Last verified 2026-06-25. +func TestBuildArgv_CodexCarriesApprovalFlag(t *testing.T) { + l, _ := Resolve("codex") + for _, tc := range []struct { + name string + resume bool + want []string + }{ + {"fresh", false, []string{"codex", "--ask-for-approval", "on-request", "BRIEFING-TEXT"}}, + {"resume", true, []string{"codex", "resume", "--last", "--ask-for-approval", "on-request", "BRIEFING-TEXT"}}, + } { + t.Run(tc.name, func(t *testing.T) { + got := l.BuildArgv(tc.resume, "BRIEFING-TEXT") + if !equalSlice(got, tc.want) { + t.Errorf("BuildArgv codex %s = %v; want %v", tc.name, got, tc.want) + } + }) + } +} + +// TestBuildArgv_CodexResumeEmptyBriefing pins the post-0.142.2 Resume +// argv when no briefing is set (the "Resume + no hints" hybrid case): +// `codex resume --last --ask-for-approval on-request` — no positional +// PROMPT, so codex opens to its interactive shell on the resumed +// session. Without this assertion, a regression that dropped --last +// or ate the approval flag would slip through. +func TestBuildArgv_CodexResumeEmptyBriefing(t *testing.T) { + l, _ := Resolve("codex") + got := l.BuildArgv(true, "") + want := []string{"codex", "resume", "--last", "--ask-for-approval", "on-request"} + if !equalSlice(got, want) { + t.Errorf("BuildArgv codex resume empty briefing = %v; want %v", got, want) + } +} + +// TestBuildArgv_CodexFreshEmptyBriefing: Fresh path with no briefing +// drops the positional [PROMPT] (last arg), leaving the approval flag +// + value intact. The Resume equivalent is covered by +// TestBuildArgv_CodexResumeEmptyBriefing (different argv shape since +// Resume prefixes `resume --last`). +func TestBuildArgv_CodexFreshEmptyBriefing(t *testing.T) { + l, _ := Resolve("codex") + got := l.BuildArgv(false, "") + want := []string{"codex", "--ask-for-approval", "on-request"} + if !equalSlice(got, want) { + t.Errorf("BuildArgv codex fresh empty briefing = %v; want %v", got, want) + } +} + +// TestBuildArgv_EmptyBriefingOnlyPopsFlagPrefix is a regression-pin +// for the v0.22 BuildArgv fix: the strip-on-empty-briefing logic must +// pop the preceding arg ONLY if it looks like a flag (starts with `-`). +// Without this guard, codex's positional briefing (where the preceding +// arg is a flag VALUE like "on-request") gets the value eaten and the +// resulting argv is malformed. +// +// Built from a synthetic Launcher so we don't depend on any of the +// shipped agents' argv staying any particular shape over time. +func TestBuildArgv_EmptyBriefingOnlyPopsFlagPrefix(t *testing.T) { + cases := []struct { + name string + tail []string + want []string + }{ + { + name: "preceding is flag → strip both", + tail: []string{"--instructions", "{{briefing}}"}, + want: []string{"FAKE"}, + }, + { + name: "preceding is flag VALUE → keep it, drop only briefing", + tail: []string{"--mode", "single", "{{briefing}}"}, + want: []string{"FAKE", "--mode", "single"}, + }, + { + name: "preceding is plain positional → keep it, drop only briefing", + tail: []string{"foo", "{{briefing}}"}, + want: []string{"FAKE", "foo"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + l := Launcher{Cmd: "FAKE", Fresh: tc.tail, Resume: tc.tail} + got := l.BuildArgv(false, "") + if !equalSlice(got, tc.want) { + t.Errorf("BuildArgv = %v; want %v", got, tc.want) + } + }) + } +} + // TestBuildArgv_OpencodeMode: opencode uses BriefingAgentsMd, so // BuildArgv has no {{briefing}} token to substitute. Verify both fresh // and resume return just [opencode]. @@ -211,6 +373,59 @@ func TestPlanLaunch_ClaudeResumeNoBriefing(t *testing.T) { } } +// TestPlanLaunch_CodexFresh: fresh codex launch with briefing inlines +// the cat shell substitution as the positional [PROMPT] argv. +// `--ask-for-approval on-request` is preserved as the leading flag pair. +func TestPlanLaunch_CodexFresh(t *testing.T) { + l, _ := Resolve("codex") + plan := l.PlanLaunch("/tmp/briefing.md", false, "/tmp/worktree") + want := `codex --ask-for-approval on-request "$(cat /tmp/briefing.md)"` + if plan.ShellCommand != want { + t.Errorf("ShellCommand = %q\nwant %q", plan.ShellCommand, want) + } + if plan.PreRun != "" { + t.Errorf("codex PreRun should be empty; got %q", plan.PreRun) + } +} + +// TestPlanLaunch_CodexResume: resumed codex with briefing produces +// `codex resume --last --ask-for-approval on-request "$(cat ...)"`. +func TestPlanLaunch_CodexResume(t *testing.T) { + l, _ := Resolve("codex") + plan := l.PlanLaunch("/tmp/briefing.md", true, "/tmp/worktree") + want := `codex resume --last --ask-for-approval on-request "$(cat /tmp/briefing.md)"` + if plan.ShellCommand != want { + t.Errorf("ShellCommand = %q\nwant %q", plan.ShellCommand, want) + } +} + +// TestPlanLaunch_CodexResumeNoBriefing pins the bug codex review caught +// 2026-06-25 (P1 #2): when briefing is empty AND the {{briefing}} token +// is a positional argument preceded by a flag VALUE (not a flag NAME), +// PlanLaunch must NOT pop the preceding arg. Otherwise codex spawns with +// `codex resume --last --ask-for-approval` (no value) and fails to parse. +// +// Mirrors the BuildArgv regression test for the same shape. +func TestPlanLaunch_CodexResumeNoBriefing(t *testing.T) { + l, _ := Resolve("codex") + plan := l.PlanLaunch("", true, "/tmp/worktree") + want := `codex resume --last --ask-for-approval on-request` + if plan.ShellCommand != want { + t.Errorf("ShellCommand = %q\nwant %q\n(if 'on-request' is missing, the strip-on-empty guard is broken)", + plan.ShellCommand, want) + } +} + +// TestPlanLaunch_CodexFreshNoBriefing: same shape, fresh path. +func TestPlanLaunch_CodexFreshNoBriefing(t *testing.T) { + l, _ := Resolve("codex") + plan := l.PlanLaunch("", false, "/tmp/worktree") + want := `codex --ask-for-approval on-request` + if plan.ShellCommand != want { + t.Errorf("ShellCommand = %q\nwant %q", plan.ShellCommand, want) + } +} + // TestPlanLaunch_AiderFile: aider uses --message-file with the path // directly (no $(cat) — aider reads the file itself). func TestPlanLaunch_AiderFile(t *testing.T) { diff --git a/internal/agent/state.go b/internal/agent/state.go index fd815ae..616d3cf 100644 --- a/internal/agent/state.go +++ b/internal/agent/state.go @@ -121,24 +121,32 @@ func (d *Detector) Observe(paneID, launcher, current string) (State, int) { // Hash stable. Pattern matching uses RAW content (codex M2) so the // footer-living idle markers aren't normalized away. - if launcher == "claude" { - for _, p := range claudeAwaitingPatterns { - if p.MatchString(current) { - return StateAwaitingInput, 9 - } + // + // Per-launcher dispatch via Classifier (see classifier.go). The + // behavioral contract: a launcher with REGISTERED markers (claude, + // codex) gets full classification; a stub launcher with empty + // marker slices (opencode, aider) AND any unrecognized launcher + // fall through to StateUnknown — we have motion data but no UI + // signal to interpret a stable pane. + classifier := ClassifierFor(launcher) + for _, p := range classifier.AwaitingMarkers() { + if p.MatchString(current) { + return StateAwaitingInput, 9 } - for _, p := range claudeIdleMarkers { - if p.MatchString(current) { - return StateIdle, 8 - } + } + for _, p := range classifier.IdleMarkers() { + if p.MatchString(current) { + return StateIdle, 8 } - // Stable + claude + no markers — probably idle but we have weak - // evidence. Lower confidence so dogfood logs surface it. + } + // Stable, no marker matched. If the launcher has REGISTERED markers + // (real classifier, just none hit), call it Idle with low confidence + // so dogfood logs surface it — same behavior as pre-refactor claude. + // If markers are empty (stub or unknown), it's StateUnknown — we + // don't fabricate an Idle signal for launchers we haven't dogfooded. + if len(classifier.IdleMarkers()) > 0 || len(classifier.AwaitingMarkers()) > 0 { return StateIdle, 5 } - - // Unknown launcher at rest: we can detect motion (Thinking) but no - // idle patterns are registered for codex/opencode/aider yet. return StateUnknown, 4 } @@ -158,14 +166,14 @@ func (d *Detector) Observe(paneID, launcher, current string) (State, int) { // produce the same badge for the same pane content): // // 1. prev=="" OR cur=="" OR launcher=="" → Unknown -// 2. launcher != "claude" → Unknown (no idle markers registered for -// codex/opencode/aider yet — same as ClassifyOneShot) +// 2. launcher with NO registered Classifier markers (opencode/aider +// stubs, unknown launchers) → Unknown. Codex + claude are real. // 3. normalize(prev) != normalize(cur) → Thinking (motion detected) // 4. stable + awaiting pattern match on RAW cur → AwaitingInput // 5. stable + idle marker match on RAW cur → Idle // 6. stable + no marker → Unknown (we have motion data but no idle -// signal — same low-confidence behavior as Detector.Observe at -// this point) +// signal — same fallback as Detector.Observe's confidence-4 path +// for stub launchers) // // Pattern matching uses RAW cur, not normalize(cur), so footer-living // markers aren't stripped before they can be matched (codex review M2). @@ -173,18 +181,22 @@ func ClassifyTwoShot(launcher, prev, cur string) State { if prev == "" || cur == "" || launcher == "" { return StateUnknown } - if launcher != "claude" { + classifier := ClassifierFor(launcher) + // Stub-launcher early-out: empty markers ↔ we haven't dogfooded this + // launcher's UI. Even if motion is present, we don't promise more + // than Unknown — the remote badge column should stay honest. + if len(classifier.IdleMarkers()) == 0 && len(classifier.AwaitingMarkers()) == 0 { return StateUnknown } if normalize(prev) != normalize(cur) { return StateThinking } - for _, p := range claudeAwaitingPatterns { + for _, p := range classifier.AwaitingMarkers() { if p.MatchString(cur) { return StateAwaitingInput } } - for _, p := range claudeIdleMarkers { + for _, p := range classifier.IdleMarkers() { if p.MatchString(cur) { return StateIdle } @@ -216,17 +228,18 @@ func ClassifyOneShot(launcher, content string) State { if content == "" || launcher == "" { return StateUnknown } - if launcher != "claude" { - // Other launchers (codex, opencode, aider) have no registered - // idle/awaiting patterns yet — can't classify without motion. + classifier := ClassifierFor(launcher) + // Stub-launcher early-out: empty markers ↔ undogfooded launcher. + // Stays Unknown so the remote-row badge column doesn't lie. + if len(classifier.IdleMarkers()) == 0 && len(classifier.AwaitingMarkers()) == 0 { return StateUnknown } - for _, p := range claudeAwaitingPatterns { + for _, p := range classifier.AwaitingMarkers() { if p.MatchString(content) { return StateAwaitingInput } } - for _, p := range claudeIdleMarkers { + for _, p := range classifier.IdleMarkers() { if p.MatchString(content) { return StateIdle } @@ -298,7 +311,18 @@ var ansiCSI = regexp.MustCompile(`\x1b\[[0-9;?]*[A-Za-z]`) // (Baked, Cooking, Simmering, Brewing, Churned) — match all known // shapes. The number changes every second so this line WILL flip the // hash if not stripped. -var spinnerLine = regexp.MustCompile(`(?i)(churned|baked|cooking|simmering|brewing|thinking|musing|pondering) for \d+s`) +// +// Also matches codex's spinner: `• Working (Ns • esc to interrupt)`. +// Codex's spinner is a single fixed verb ("Working") wrapped in a +// parenthesized timer, structurally different from claude's "Baked +// for Ns", so it gets its own alternation arm rather than trying to +// jam it into the (verb) for Ns grammar. Last verified for codex: +// 2026-06-17 against codex-cli 0.140.0. +var spinnerLine = regexp.MustCompile( + `(?i)(churned|baked|cooking|simmering|brewing|thinking|musing|pondering) for \d+s` + + `|` + + `• Working \(\d+s • esc to interrupt\)`, +) // footerLine matches claude's mode-toggle footer that toggles when the // user hits shift+tab. We strip it from normalized content (avoids diff --git a/internal/agent/state_test.go b/internal/agent/state_test.go index f2dde86..9751d69 100644 --- a/internal/agent/state_test.go +++ b/internal/agent/state_test.go @@ -175,20 +175,27 @@ func TestDetector_StableContent_ClaudeNoMarkers_IsIdleLowConfidence(t *testing.T } func TestDetector_UnknownLauncher_StableContent_IsUnknown(t *testing.T) { + // Use opencode — a real Classifier stub with empty marker slices. + // codex used to be the canonical "no markers registered" case but + // shipped real patterns post-2026-06; opencode + aider remain the + // canonical stubs and exercise the empty-slice fallthrough path. d := NewDetector() - d.Observe("%0", "codex", "stable content") - state, _ := d.Observe("%0", "codex", "stable content") + d.Observe("%0", "opencode", "stable content") + state, _ := d.Observe("%0", "opencode", "stable content") if state != StateUnknown { - t.Errorf("unknown launcher stable = %v, want StateUnknown (no markers known)", state) + t.Errorf("stub launcher stable = %v, want StateUnknown (no markers known)", state) } } func TestDetector_UnknownLauncher_ChangingContent_IsThinking(t *testing.T) { + // Motion check fires BEFORE the launcher classifier check, so + // motion → StateThinking is launcher-agnostic. Use opencode (stub) + // to assert this still holds for stub launchers post-refactor. d := NewDetector() - d.Observe("%0", "codex", "first") - state, _ := d.Observe("%0", "codex", "different") + d.Observe("%0", "opencode", "first") + state, _ := d.Observe("%0", "opencode", "different") if state != StateThinking { - t.Errorf("unknown launcher changing = %v, want StateThinking (motion is launcher-agnostic)", state) + t.Errorf("stub launcher changing = %v, want StateThinking (motion is launcher-agnostic)", state) } } diff --git a/internal/agent/testdata/README.md b/internal/agent/testdata/README.md new file mode 100644 index 0000000..ccbcfa2 --- /dev/null +++ b/internal/agent/testdata/README.md @@ -0,0 +1,41 @@ +# Agent classifier test fixtures + +Raw `tmux capture-pane` snapshots of real agent TUI states, used as ground truth +for `internal/agent/classifier_*_test.go`. When the upstream CLI ships UI +changes, re-capture these files against a known-good version and bump the +"last verified" comment next to each regex pattern in the matching +`classifier_.go`. + +## codex (captured 2026-06-17 against codex-cli 0.140.0, model gpt-5.5) + +- `codex_trust_dialog.txt` — first-launch trust prompt + ("Do you trust the contents of this directory?") +- `codex_idle.txt` — codex at input prompt, no activity (banner + footer) +- `codex_thinking_a.txt` + `codex_thinking_b.txt` — mid-response, captured 2s apart; + the only line that changed is the spinner timer (`• Working (1s ...)` → + `• Working (3s ...)`). Used to prove motion-based Thinking detection AND + to validate that `normalize()` strips the spinner line so a long-idle pane + doesn't flip-flop on timer. +- `codex_awaiting_input.txt` — codex's approval dialog for a file-write + ("Would you like to make the following edits?" / "Press enter to confirm or + esc to cancel"). Only appears under `--ask-for-approval untrusted` or + `on-request`; in default mode codex auto-applies edits with no dialog. + +## Re-capture procedure + +```bash +SPIKE=/tmp/codex-spike-cwd +mkdir -p "$SPIKE" && cd "$SPIKE" && git init -q +tmux -L codex-spike new-session -d -s spike -x 200 -y 50 \ + -c "$SPIKE" "codex --ask-for-approval untrusted" +sleep 5 +# Trust dialog: +tmux -L codex-spike capture-pane -t spike -p > codex_trust_dialog.txt +tmux -L codex-spike send-keys -t spike Enter +sleep 4 +# Idle: +tmux -L codex-spike capture-pane -t spike -p > codex_idle.txt +# Thinking: send any prompt and capture twice during execution +# Awaiting: ask codex to write a file in untrusted mode +tmux -L codex-spike kill-server +``` diff --git a/internal/agent/testdata/codex_awaiting_input.txt b/internal/agent/testdata/codex_awaiting_input.txt new file mode 100644 index 0000000..66a105f --- /dev/null +++ b/internal/agent/testdata/codex_awaiting_input.txt @@ -0,0 +1,50 @@ +╭───────────────────────────────────────╮ +│ >_ OpenAI Codex (v0.140.0) │ +│ │ +│ model: gpt-5.5 /model to change │ +│ directory: /tmp/codex-spike-cwd │ +╰───────────────────────────────────────╯ + + Tip: GPT-5.5 is now available in Codex. It's our strongest agentic coding model yet, built to reason through large codebases, check assumptions with tools, and keep going until the work is done. + + Learn more: https://openai.com/index/introducing-gpt-5-5/ + + +› Write a tiny file at /tmp/codex-spike-cwd/hello.txt that says hi + + +• I’ll create the requested file directly in the workspace. + +• Added hello.txt (+1 -0) + 1 +hi + + + Would you like to make the following edits? + + +› 1. Yes, proceed (y) + 2. Yes, and don't ask again for these files (a) + 3. No, and tell Codex what to do differently (esc) + + Press enter to confirm or esc to cancel + + + + + + + + + + + + + + + + + + + + + diff --git a/internal/agent/testdata/codex_idle.txt b/internal/agent/testdata/codex_idle.txt new file mode 100644 index 0000000..b47246b --- /dev/null +++ b/internal/agent/testdata/codex_idle.txt @@ -0,0 +1,50 @@ +╭───────────────────────────────────────╮ +│ >_ OpenAI Codex (v0.140.0) │ +│ │ +│ model: gpt-5.5 /model to change │ +│ directory: /tmp/codex-spike-cwd │ +╰───────────────────────────────────────╯ + + Tip: GPT-5.5 is now available in Codex. It's our strongest agentic coding model yet, built to reason through large codebases, check assumptions with tools, and keep going until the work is done. + + Learn more: https://openai.com/index/introducing-gpt-5-5/ + + +› Explain this codebase + + gpt-5.5 default · /tmp/codex-spike-cwd + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/internal/agent/testdata/codex_thinking_a.txt b/internal/agent/testdata/codex_thinking_a.txt new file mode 100644 index 0000000..93a4219 --- /dev/null +++ b/internal/agent/testdata/codex_thinking_a.txt @@ -0,0 +1,50 @@ +╭───────────────────────────────────────╮ +│ >_ OpenAI Codex (v0.140.0) │ +│ │ +│ model: gpt-5.5 /model to change │ +│ directory: /tmp/codex-spike-cwd │ +╰───────────────────────────────────────╯ + + Tip: GPT-5.5 is now available in Codex. It's our strongest agentic coding model yet, built to reason through large codebases, check assumptions with tools, and keep going until the work is done. + + Learn more: https://openai.com/index/introducing-gpt-5-5/ + + +› Count to ten very slowly, one number per second, explaining each digit's history + + +• Working (1s • esc to interrupt) + + +› Explain this codebase + + gpt-5.5 default · /tmp/codex-spike-cwd + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/internal/agent/testdata/codex_thinking_b.txt b/internal/agent/testdata/codex_thinking_b.txt new file mode 100644 index 0000000..8d2d6c6 --- /dev/null +++ b/internal/agent/testdata/codex_thinking_b.txt @@ -0,0 +1,50 @@ +╭───────────────────────────────────────╮ +│ >_ OpenAI Codex (v0.140.0) │ +│ │ +│ model: gpt-5.5 /model to change │ +│ directory: /tmp/codex-spike-cwd │ +╰───────────────────────────────────────╯ + + Tip: GPT-5.5 is now available in Codex. It's our strongest agentic coding model yet, built to reason through large codebases, check assumptions with tools, and keep going until the work is done. + + Learn more: https://openai.com/index/introducing-gpt-5-5/ + + +› Count to ten very slowly, one number per second, explaining each digit's history + + +• Working (3s • esc to interrupt) + + +› Explain this codebase + + gpt-5.5 default · /tmp/codex-spike-cwd + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/internal/agent/testdata/codex_trust_dialog.txt b/internal/agent/testdata/codex_trust_dialog.txt new file mode 100644 index 0000000..f710c12 --- /dev/null +++ b/internal/agent/testdata/codex_trust_dialog.txt @@ -0,0 +1,50 @@ +> You are in /tmp/codex-spike-cwd + + Do you trust the contents of this directory? Working with untrusted contents comes with higher risk of prompt injection. Trusting the directory allows project-local config, hooks, and exec policies + to load. + +› 1. Yes, continue + 2. No, quit + + Press enter to continue + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + diff --git a/internal/config/config.go b/internal/config/config.go index 4996932..fbabb44 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -95,11 +95,27 @@ type Config struct { // Scripts comes from the JSON. Scripts Scripts `json:"scripts"` - // Agent comes from the JSON. Empty block (no agent.type set) is - // treated as Type="claude" by validate(), so existing canopy.json - // files without an agent block keep working unchanged. + // Agent is the LEGACY singular agent block. Empty block (no + // agent.type set) is treated as Type="claude" by validate(). When + // the newer Agents array is also present, Agents wins and Agent is + // silently ignored — see validate() for the precedence dance. Agent Agent `json:"agent,omitempty"` + // Agents is the v0.22 plural allowlist of launchers this project + // supports. canopy new --agent and canopy agent swap + // both validate against this list via AllowsAgent. The first entry + // is the project's DEFAULT for new workspaces. + // + // Precedence rules (validate()): + // - Agents non-empty: use as-is; Agent legacy block ignored + // - Agents empty, Agent.Type set: promote to Agents=[Agent.Type] + // - Both empty: Agents=["claude"] (matches legacy default) + // + // The legacy Agent.Briefing/BriefingFile stays a separate concern — + // even when Agents is present, the briefing-text plumbing keeps + // reading from Agent. Per-launcher briefings are a follow-up. + Agents []string `json:"agents,omitempty"` + // ProjectRoot is set by Load (not from the JSON). Absolute path to the // directory containing canopy.json. ProjectRoot string `json:"-"` @@ -110,6 +126,140 @@ type Config struct { Project string `json:"-"` } +// DefaultAgent returns the canonical default agent for a new workspace +// created from this project. Always returns the first entry in Agents +// (which validate() guarantees is non-empty). Use this instead of +// reading Cfg.Agent.Type directly — Agent.Type is the legacy field that +// validate() may not have populated when canopy.json declared Agents +// only. +func (c *Config) DefaultAgent() string { + if len(c.Agents) > 0 { + return c.Agents[0] + } + // validate() should have populated Agents; this is a defensive + // fallback for callers that bypass Load (test fixtures, mostly). + if c.Agent.Type != "" { + return c.Agent.Type + } + return "claude" +} + +// AllowsAgent reports whether the project's canopy.json declares the +// given agent type as runnable. Empty target → false. Used by the +// --agent CLI flag and the canopy agent swap verb to gate against +// ErrAgentNotAllowed at command time, before any side effects fire. +func (c *Config) AllowsAgent(t string) bool { + if t == "" { + return false + } + for _, a := range c.Agents { + if a == t { + return true + } + } + return false +} + +// AddAgentToCanopyJSON appends agentName to /canopy.json's +// `agents` array if it's not already present, and writes the file +// back atomically. Unknown top-level keys are preserved via raw-map +// round-trip (same pattern as userconfig.go) so user-added fields +// don't get clobbered. v0.22. +// +// Used by the in-TUI swap + ask pickers' D6=A "auto-add on pick" +// path: when a user picks an agent that's installed but not in the +// project's allowlist, this writes the config update silently. +// Idempotent — no-op + nil return if the agent is already listed. +// +// The function does NOT mutate any in-memory Config; the caller is +// expected to re-Load if they need the updated Cfg.Agents. (canopy +// agent swap's success path doesn't need it — the verb has already +// resolved the launcher by name; canopy.json is updated for the next +// invocation.) +// +// Errors propagate: file-missing, parse failure, malformed agents +// field (e.g., `"agents": "claude"` instead of an array), or write +// failure. Each is wrapped with the canopy.json path for diagnosis. +func AddAgentToCanopyJSON(projectRoot, agentName string) error { + if agentName == "" { + return fmt.Errorf("config.AddAgentToCanopyJSON: empty agent name") + } + path := filepath.Join(projectRoot, FileName) + data, err := os.ReadFile(path) + if err != nil { + return fmt.Errorf("config.AddAgentToCanopyJSON: read %s: %w", path, err) + } + + // Two-pass decode: raw map preserves unknown keys; typed agents + // extraction modifies just the field we care about. + raw := map[string]json.RawMessage{} + if err := json.Unmarshal(data, &raw); err != nil { + return fmt.Errorf("config.AddAgentToCanopyJSON: parse %s: %w", path, err) + } + var agents []string + if existing, ok := raw["agents"]; ok && len(existing) > 0 { + if err := json.Unmarshal(existing, &agents); err != nil { + return fmt.Errorf("config.AddAgentToCanopyJSON: parse agents in %s: %w", path, err) + } + } + // Legacy promotion: if `agents` is missing but the legacy `agent: {type}` + // block is present, seed the new array from that type. Without this + // seed, auto-adding a NEW agent to a project that implicitly declared + // claude via `agent.type` would write only the new agent and silently + // drop claude from the allowlist. (codex review P1, 2026-06-25.) + if len(agents) == 0 { + if rawAgent, ok := raw["agent"]; ok && len(rawAgent) > 0 { + var legacy struct { + Type string `json:"type"` + } + if err := json.Unmarshal(rawAgent, &legacy); err == nil && legacy.Type != "" { + agents = []string{legacy.Type} + } + } + } + // Last-resort default: a project with neither `agents` nor `agent.type` + // implicitly defaulted to claude (validate() does this on Load). + // Adding a new agent shouldn't drop the implicit claude default. + if len(agents) == 0 { + agents = []string{"claude"} + } + for _, a := range agents { + if a == agentName { + return nil // idempotent — already there + } + } + agents = append(agents, agentName) + + updated, err := json.Marshal(agents) + if err != nil { + return fmt.Errorf("config.AddAgentToCanopyJSON: marshal agents: %w", err) + } + raw["agents"] = updated + + // Re-marshal the whole doc with indent so the file stays + // human-readable. We can't use json.Marshal on the raw map and + // get stable key order; for canopy.json's small fixed surface + // (scripts + agent + agents) the indented output is acceptable + // even with map-iteration-order keys. If key order becomes + // load-bearing, swap to a custom encoder; for now boring wins. + out, err := json.MarshalIndent(raw, "", " ") + if err != nil { + return fmt.Errorf("config.AddAgentToCanopyJSON: marshal doc: %w", err) + } + out = append(out, '\n') + + tmp := path + ".tmp" + if err := os.WriteFile(tmp, out, 0o644); err != nil { + return fmt.Errorf("config.AddAgentToCanopyJSON: write tmp: %w", err) + } + if err := os.Rename(tmp, path); err != nil { + _ = os.Remove(tmp) + return fmt.Errorf("config.AddAgentToCanopyJSON: rename: %w", err) + } + log.Info("config.agent.added", "project_root", projectRoot, "agent", agentName) + return nil +} + // Discover walks up from startDir looking for canopy.json. It stops at the // first parent directory that contains a readable canopy.json, or at the // filesystem root (returning ErrNotFound). @@ -234,7 +384,28 @@ func LoadFrom(root string) (*Config, error) { // the check there means new agents land via one PR (add a launcher), // not two (add a launcher + update config validation). func validate(c *Config) error { - if c.Agent.Type == "" { + // Agents/Agent precedence (v0.22 schema dance): + // - Agents non-empty → use as-is; the legacy Agent block (if any) + // is silently ignored. Agent.Type is forced to Agents[0] so any + // code still reading the legacy field stays consistent. + // - Agents empty, Agent.Type set → promote to Agents=[Agent.Type]. + // This is the existing-canopy.json upgrade path: a project that + // only declares `agent.type` gains Agents=[that-type] without + // the user editing anything. + // - Both empty → default Agents=["claude"], matching legacy. + // + // The precedence is silent (no log noise on collision) per eng-review + // D6. The legacy `agent` block keeps working indefinitely; users + // migrate by editing canopy.json on their own schedule. + switch { + case len(c.Agents) > 0: + // Keep Agent.Type aligned with Agents[0] for any holdout + // readers; the canonical source going forward is Agents. + c.Agent.Type = c.Agents[0] + case c.Agent.Type != "": + c.Agents = []string{c.Agent.Type} + default: + c.Agents = []string{"claude"} c.Agent.Type = "claude" } if c.Agent.Briefing != "" && c.Agent.BriefingFile != "" { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index d158ba3..bf1554a 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -4,7 +4,9 @@ import ( "errors" "os" "path/filepath" + "strings" "testing" + "time" "github.com/avinashjoshi/canopy/internal/clog" "github.com/avinashjoshi/canopy/internal/config" @@ -240,6 +242,249 @@ func TestLoad_AgentExplicit(t *testing.T) { // goes to slog and is hard to assert in a unit test. } +// TestLoad_Agents_Precedence covers the v0.22 schema rules: the new +// `agents` plural array wins when both forms are present, the legacy +// `agent` form is promoted to a single-element list when alone, and +// neither falls back to ["claude"]. Each case asserts both Agents[] +// AND Agent.Type so the legacy field stays consistent with the +// canonical Agents source. +func TestLoad_Agents_Precedence(t *testing.T) { + t.Parallel() + cases := []struct { + name string + json string + wantAgents []string + wantPrimary string + }{ + { + name: "neither: default to claude", + json: `{"scripts":{"setup":"","run":"","archive":""}}`, + wantAgents: []string{"claude"}, + wantPrimary: "claude", + }, + { + name: "legacy agent.type alone is promoted to agents=[type]", + json: `{"scripts":{"setup":"","run":"","archive":""},"agent":{"type":"codex"}}`, + wantAgents: []string{"codex"}, + wantPrimary: "codex", + }, + { + name: "agents alone: round-trip + Agent.Type tracks agents[0]", + json: `{"scripts":{"setup":"","run":"","archive":""},"agents":["codex","claude"]}`, + wantAgents: []string{"codex", "claude"}, + wantPrimary: "codex", + }, + { + name: "both present: agents silently wins; Agent.Type aligns to agents[0]", + json: `{"scripts":{"setup":"","run":"","archive":""},"agent":{"type":"claude"},"agents":["aider","codex"]}`, + wantAgents: []string{"aider", "codex"}, + wantPrimary: "aider", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "canopy.json"), tc.json) + cfg, err := config.DiscoverAndLoad(dir) + if err != nil { + t.Fatalf("DiscoverAndLoad: %v", err) + } + if !equalStrings(cfg.Agents, tc.wantAgents) { + t.Errorf("Agents = %v; want %v", cfg.Agents, tc.wantAgents) + } + if cfg.Agent.Type != tc.wantPrimary { + t.Errorf("Agent.Type = %q; want %q", cfg.Agent.Type, tc.wantPrimary) + } + if got := cfg.DefaultAgent(); got != tc.wantPrimary { + t.Errorf("DefaultAgent() = %q; want %q", got, tc.wantPrimary) + } + }) + } +} + +// TestAddAgentToCanopyJSON_AppendsNewAgent: the auto-add path on a +// project whose canopy.json had no `agents` field AND no legacy +// `agent: {type}` block writes ["claude", "codex"] — the implicit +// "claude" default is preserved, and the new agent is appended. +// Unknown top-level keys must round-trip untouched. +func TestAddAgentToCanopyJSON_AppendsNewAgent(t *testing.T) { + t.Parallel() + dir := t.TempDir() + initial := `{ + "scripts": {"setup": "x", "run": "x", "archive": "x"}, + "custom_user_field": "preserve me" +}` + writeFile(t, filepath.Join(dir, "canopy.json"), initial) + + if err := config.AddAgentToCanopyJSON(dir, "codex"); err != nil { + t.Fatalf("AddAgentToCanopyJSON: %v", err) + } + + cfg, err := config.LoadFrom(dir) + if err != nil { + t.Fatalf("LoadFrom post-add: %v", err) + } + if !equalStrings(cfg.Agents, []string{"claude", "codex"}) { + t.Errorf("Agents = %v; want [claude codex] (implicit claude preserved + codex appended)", cfg.Agents) + } + // Unknown key survives. + raw, _ := os.ReadFile(filepath.Join(dir, "canopy.json")) + if !strings.Contains(string(raw), `"preserve me"`) { + t.Errorf("custom_user_field clobbered; full file:\n%s", string(raw)) + } +} + +// TestAddAgentToCanopyJSON_AppendsToExistingList: project already +// has an agents list; new entry gets appended (not replacing). +func TestAddAgentToCanopyJSON_AppendsToExistingList(t *testing.T) { + t.Parallel() + dir := t.TempDir() + initial := `{ + "scripts": {"setup": "x", "run": "x", "archive": "x"}, + "agents": ["claude"] +}` + writeFile(t, filepath.Join(dir, "canopy.json"), initial) + + if err := config.AddAgentToCanopyJSON(dir, "codex"); err != nil { + t.Fatalf("AddAgentToCanopyJSON: %v", err) + } + cfg, _ := config.LoadFrom(dir) + if !equalStrings(cfg.Agents, []string{"claude", "codex"}) { + t.Errorf("Agents = %v; want [claude codex]", cfg.Agents) + } +} + +// TestAddAgentToCanopyJSON_IdempotentNoOp: adding an agent that's +// already in the list is a no-op (returns nil, file unchanged). +// Catches the dumb regression where idempotency gets dropped during +// a refactor and we start duplicating entries. +func TestAddAgentToCanopyJSON_IdempotentNoOp(t *testing.T) { + t.Parallel() + dir := t.TempDir() + initial := `{ + "scripts": {"setup": "x", "run": "x", "archive": "x"}, + "agents": ["claude", "codex"] +}` + path := filepath.Join(dir, "canopy.json") + writeFile(t, path, initial) + + statBefore, err := os.Stat(path) + if err != nil { + t.Fatalf("stat before: %v", err) + } + // Sleep slightly so a mtime change would be detectable. + time.Sleep(20 * time.Millisecond) + + if err := config.AddAgentToCanopyJSON(dir, "codex"); err != nil { + t.Fatalf("AddAgentToCanopyJSON: %v", err) + } + statAfter, _ := os.Stat(path) + if !statBefore.ModTime().Equal(statAfter.ModTime()) { + t.Errorf("file mtime changed on idempotent add (before=%v after=%v); want no write", + statBefore.ModTime(), statAfter.ModTime()) + } +} + +// TestAddAgentToCanopyJSON_PreservesLegacyAgentType pins the codex +// review fix (P1 #5, 2026-06-25): a project whose canopy.json declared +// `agent: {type: "claude"}` and NO `agents:` array must end up with +// `agents: ["claude", "codex"]` after auto-adding codex — NOT +// `agents: ["codex"]` (which would silently drop claude). +func TestAddAgentToCanopyJSON_PreservesLegacyAgentType(t *testing.T) { + t.Parallel() + dir := t.TempDir() + initial := `{ + "scripts": {"setup": "x", "run": "x", "archive": "x"}, + "agent": {"type": "claude"} +}` + writeFile(t, filepath.Join(dir, "canopy.json"), initial) + + if err := config.AddAgentToCanopyJSON(dir, "codex"); err != nil { + t.Fatalf("AddAgentToCanopyJSON: %v", err) + } + cfg, err := config.LoadFrom(dir) + if err != nil { + t.Fatalf("LoadFrom: %v", err) + } + if !equalStrings(cfg.Agents, []string{"claude", "codex"}) { + t.Errorf("Agents = %v; want [claude codex] (claude must NOT be dropped)", cfg.Agents) + } +} + +// TestAddAgentToCanopyJSON_PreservesImplicitClaudeDefault: a project +// with NEITHER agents nor agent.type implicitly defaults to claude +// (validate() does this on Load). Auto-adding codex must produce +// ["claude", "codex"], not ["codex"]. Sister test to the legacy +// agent.type case above. +func TestAddAgentToCanopyJSON_PreservesImplicitClaudeDefault(t *testing.T) { + t.Parallel() + dir := t.TempDir() + initial := `{ + "scripts": {"setup": "x", "run": "x", "archive": "x"} +}` + writeFile(t, filepath.Join(dir, "canopy.json"), initial) + + if err := config.AddAgentToCanopyJSON(dir, "codex"); err != nil { + t.Fatalf("AddAgentToCanopyJSON: %v", err) + } + cfg, err := config.LoadFrom(dir) + if err != nil { + t.Fatalf("LoadFrom: %v", err) + } + if !equalStrings(cfg.Agents, []string{"claude", "codex"}) { + t.Errorf("Agents = %v; want [claude codex] (implicit claude must NOT be dropped)", cfg.Agents) + } +} + +// TestAddAgentToCanopyJSON_EmptyAgentRejected: empty agent name is +// an error (defensive — no way to add "" usefully). +func TestAddAgentToCanopyJSON_EmptyAgentRejected(t *testing.T) { + t.Parallel() + dir := t.TempDir() + writeFile(t, filepath.Join(dir, "canopy.json"), `{"scripts":{"setup":"x","run":"x","archive":"x"}}`) + if err := config.AddAgentToCanopyJSON(dir, ""); err == nil { + t.Error("AddAgentToCanopyJSON(\"\") = nil; want error") + } +} + +// TestConfig_AllowsAgent covers the gate for --agent / canopy agent swap. +// Empty argument is never allowed; declared types are; undeclared types +// are rejected so the caller can surface ErrAgentNotAllowed. +func TestConfig_AllowsAgent(t *testing.T) { + t.Parallel() + cfg := &config.Config{Agents: []string{"claude", "codex"}} + cases := []struct { + in string + want bool + }{ + {"", false}, + {"claude", true}, + {"codex", true}, + {"aider", false}, + {"future-thing", false}, + } + for _, tc := range cases { + t.Run(tc.in, func(t *testing.T) { + if got := cfg.AllowsAgent(tc.in); got != tc.want { + t.Errorf("AllowsAgent(%q) = %v; want %v", tc.in, got, tc.want) + } + }) + } +} + +func equalStrings(a, b []string) bool { + if len(a) != len(b) { + return false + } + for i := range a { + if a[i] != b[i] { + return false + } + } + return true +} + // TestLoad_AgentEmptyBlock: an explicitly-empty agent block (`"agent": {}`) // behaves the same as omitting the block — Type defaults to "claude". func TestLoad_AgentEmptyBlock(t *testing.T) { diff --git a/internal/state/listing.go b/internal/state/listing.go index f97c652..6e21d44 100644 --- a/internal/state/listing.go +++ b/internal/state/listing.go @@ -130,6 +130,14 @@ type GlobalRow struct { // leaves this empty for callers that don't need detector decoration. Hints []Hint + // CurrentAgent is the launcher type this workspace is currently + // running ("claude", "codex", etc). v0.22. Mirrors + // state.Workspace.CurrentAgent — surfaced into rows so the UI + // can render the agent name + the agent-swap picker can show + // the current selection dimmed. Empty for IsMain rows (main + // sessions have no agent pane) and for pre-v0.22 unmigrated rows. + CurrentAgent string + // Host is the registered host name this workspace lives on. Empty // string for local workspaces (the laptop's own state). Non-empty // for rows merged in from remote canopies via host.Refresher @@ -315,6 +323,7 @@ func (s *State) BuildGlobalRows(ctx context.Context, probe LivenessProbe) []Glob LastErrorHint: w.LastErrorHint, Owner: w.Owner, SourceKind: w.SourceKind, + CurrentAgent: w.CurrentAgent, }) } } diff --git a/internal/state/state.go b/internal/state/state.go index edf7959..4d23145 100644 --- a/internal/state/state.go +++ b/internal/state/state.go @@ -146,6 +146,26 @@ type Workspace struct { // store; same shape as Port. AgentLaunchCount int `json:"agent_launch_count,omitempty"` + // AgentLaunches tracks per-agent launch counts in this workspace. + // Keyed by launcher type (e.g., "claude", "codex"); value is the + // number of times we've spawned that specific agent in this + // workspace's lifetime. v0.22+. + // + // Used by workspace.SwapAgent to decide Resume vs Fresh when + // swapping in a different agent: count[target]==0 means this is + // the first time the target has ever run here (its --resume + // machinery would find no prior session and exit), so we use + // Fresh. count[target]>0 means prior history exists, so we use + // Resume and let the agent's own --continue / --resume verb + // reach the prior conversation. + // + // Migration: pre-v0.22 rows have nil AgentLaunches. The first + // canopy ls (or any read that goes through Manager.New) populates + // it from AgentLaunchCount under the assumption that all prior + // launches were of CurrentAgent — which was always true before + // swap existed. + AgentLaunches map[string]int `json:"agent_launches,omitempty"` + // SourceKind is set once at workspace creation and never changes. // Drives which AGENT.md briefing variant gets assembled and the // agent's framing for the work. Values: @@ -211,6 +231,25 @@ type Workspace struct { // rows (no `owner` key) reading back as "" — i.e. mine, or a legacy // review row if pr-sourced — with no schema migration. Owner string `json:"owner,omitempty"` + + // CurrentAgent is the launcher type this workspace is currently + // running ("claude", "codex", etc.). Snapshotted at workspace-create + // time from --agent (or canopy.json's `agents[0]` default) and + // mutated by `canopy agent swap `. The pane-role tag + // (`agent:`) and this field are kept in sync. + // + // v0.22 introduces this field. Empty value on a pre-v0.22 row means + // "needs migration"; the workspace manager's MigrateAgents pass on + // next read populates it from the project's canopy.json + // (`agents[0]` → legacy `agent.type` → "claude") under WithLock, + // converging on first post-upgrade access. See workspace package. + // + // Why on the row vs. computed from config every time: the workspace + // owns its identity. A user who swaps agents on workspace A + // shouldn't see workspace B follow along, even though both projects + // share the same canopy.json default. Snapshot at create, mutate + // only via explicit swap. + CurrentAgent string `json:"current_agent,omitempty"` } // OwnerSelfMarker is the reserved value Owner holds when the user has diff --git a/internal/tmux/layout.go b/internal/tmux/layout.go new file mode 100644 index 0000000..81e09f2 --- /dev/null +++ b/internal/tmux/layout.go @@ -0,0 +1,76 @@ +package tmux + +import ( + "context" + "fmt" + "os/exec" + "strings" +) + +// CaptureWindowLayout returns tmux's opaque window-layout descriptor for +// the active window of the given session. The string is what tmux's +// `select-layout` accepts: a checksumed, byte-precise serialization of +// every pane's pixel geometry within the window. Round-tripping it via +// SelectLayout restores the EXACT layout, not just the proportions. +// +// Used by canopy agent swap: capture before kill-pane → respawn the +// new agent pane → SelectLayout to restore. tmux's `split-window +// -l %` takes a PERCENTAGE of the target pane, which means a naive +// kill-and-resplit drifts the IDE/terminal/agent geometry slightly each +// swap (the remaining panes redistribute when one is killed, so a +// 30% split off the IDE post-kill is not the same geometry as the +// original 30% split). Capturing and restoring the layout sidesteps +// that drift entirely. +// +// Returns the layout string for the session's ACTIVE window. Multi- +// window sessions are out of scope: canopy currently lays out one +// window per workspace. +func (c *Client) CaptureWindowLayout(ctx context.Context, session string) (string, error) { + args := c.args("display-message", "-t", session, "-p", "#{window_layout}") + cmd := exec.CommandContext(ctx, "tmux", args...) + var stdout, stderr strings.Builder + cmd.Stdout = &stdout + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + return "", fmt.Errorf("tmux.CaptureWindowLayout(%s): %w (stderr: %s)", + session, err, strings.TrimSpace(stderr.String())) + } + layout := strings.TrimSpace(stdout.String()) + if layout == "" { + return "", fmt.Errorf("tmux.CaptureWindowLayout(%s): empty layout (session may not exist)", session) + } + return layout, nil +} + +// SelectLayout for restoring a captured window_layout string lives in +// session.go alongside the other layout primitives. Callers that pass +// the output of CaptureWindowLayout straight to SelectLayout restore +// byte-precise geometry. Empty strings come from a swallowed +// CaptureWindowLayout error; SelectLayout treats them as a tmux error +// rather than a no-op (consistent with session.go's existing contract), +// so the caller should drop empty layouts before calling. + +// KillPane terminates the pane with the given pane ID. Used by canopy +// agent swap to remove the existing agent pane before respawning the +// new agent in its place. Idempotent at the canopy semantics layer: +// if the pane is already gone (e.g., user manually killed it before +// the swap), tmux returns a "can't find pane" error which the swap +// path can choose to treat as fine. +// +// paneID must be the `%` form from a prior Create/SplitPane +// or LookupPane call. Invalid pane IDs are rejected by tmux with a +// clear error. +func (c *Client) KillPane(ctx context.Context, paneID string) error { + if paneID == "" { + return fmt.Errorf("tmux.KillPane: empty pane ID") + } + args := c.args("kill-pane", "-t", paneID) + cmd := exec.CommandContext(ctx, "tmux", args...) + var stderr strings.Builder + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + return fmt.Errorf("tmux.KillPane(%s): %w (stderr: %s)", + paneID, err, strings.TrimSpace(stderr.String())) + } + return nil +} diff --git a/internal/tmux/layout_test.go b/internal/tmux/layout_test.go new file mode 100644 index 0000000..0410b38 --- /dev/null +++ b/internal/tmux/layout_test.go @@ -0,0 +1,110 @@ +package tmux_test + +import ( + "context" + "os/exec" + "regexp" + "strings" + "testing" + + "github.com/avinashjoshi/canopy/internal/tmux" +) + +// stripPaneIndices replaces the trailing pane-index integer inside each +// tmux window_layout pane spec with a placeholder. The format is +// "checksum,WxH,X,Y[...pane specs...]" where each leaf spec ends in +// ",". Pane indices are assigned by tmux at creation time +// so they DIFFER across a kill+respawn even when geometry is identical. +// The byte-precise-geometry contract is "all the W/H/X/Y values match"; +// pane IDs are bookkeeping. +// +// The first two-digit field at the start (`,`) is the layout +// checksum + window dimensions, which DO depend on pane IDs (because +// tmux folds them into the checksum). We strip the checksum too — +// the checksum is what tmux uses to detect a corrupted layout string +// at parse time, not a stable identifier across mutations. +func stripPaneIndices(layout string) string { + // Pane-index suffix: a comma + digits immediately before `,`, `}`, or `]`. + idxRe := regexp.MustCompile(`,\d+([,}\]])`) + stripped := idxRe.ReplaceAllString(layout, ",X$1") + // Drop the leading "checksum," prefix. + if i := strings.Index(stripped, ","); i > 0 { + stripped = stripped[i+1:] + } + return stripped +} + +// TestCaptureWindowLayout_RoundTrip creates a multi-pane session, +// captures its layout, kills+respawns one pane, then SelectLayout- +// restores. The captured-before layout string must equal the +// captured-after layout string — otherwise the swap path drifts pane +// geometry across iterations. +func TestCaptureWindowLayout_RoundTrip(t *testing.T) { + cwd := t.TempDir() + name := "tmux-layout-roundtrip-" + strings.ReplaceAll(t.Name(), "/", "_") + ctx := context.Background() + c := tmux.WithSocket(testSocket) + + // 3-pane session: IDE (top-left), shell (bottom), agent (top-right). + // Matches canopy's buildSession layout. + idePane, err := c.Create(ctx, name, cwd, "sleep 120") + if err != nil { + t.Fatalf("Create: %v", err) + } + t.Cleanup(func() { + _ = exec.Command("tmux", "-L", testSocket, "kill-session", "-t", name).Run() + }) + _ = idePane + shellPane, err := c.SplitPane(ctx, name, cwd, "sleep 120", tmux.SplitVertical, 15) + if err != nil { + t.Fatalf("SplitPane shell: %v", err) + } + _ = shellPane + agentPane, err := c.SplitPane(ctx, name, cwd, "sleep 120", tmux.SplitHorizontal, 30) + if err != nil { + t.Fatalf("SplitPane agent: %v", err) + } + + before, err := c.CaptureWindowLayout(ctx, name) + if err != nil { + t.Fatalf("CaptureWindowLayout: %v", err) + } + if before == "" { + t.Fatal("CaptureWindowLayout returned empty layout") + } + + // Simulate canopy agent swap: kill agent pane and respawn. + if err := c.KillPane(ctx, agentPane); err != nil { + t.Fatalf("KillPane: %v", err) + } + newAgent, err := c.SplitPane(ctx, name, cwd, "sleep 120", tmux.SplitHorizontal, 30) + if err != nil { + t.Fatalf("SplitPane respawn: %v", err) + } + _ = newAgent + + // Without SelectLayout, geometry typically drifts (the 30% is now + // percent-of-redistributed-IDE, not percent-of-original). After + // SelectLayout, geometry should be byte-identical to `before`. + if err := c.SelectLayout(ctx, name, before); err != nil { + t.Fatalf("SelectLayout: %v", err) + } + after, err := c.CaptureWindowLayout(ctx, name) + if err != nil { + t.Fatalf("CaptureWindowLayout after: %v", err) + } + // Compare geometry (W/H/X/Y) ignoring pane IDs and checksum. + if got, want := stripPaneIndices(after), stripPaneIndices(before); got != want { + t.Errorf("layout drift after kill+respawn+restore:\nbefore (raw)=%q\nafter (raw)=%q\nbefore (geom)=%q\nafter (geom)=%q", + before, after, want, got) + } +} + +// TestKillPane_EmptyPaneID is a quick guard: empty pane ID must fail +// fast rather than asking tmux to kill nothing-in-particular. +func TestKillPane_EmptyPaneID(t *testing.T) { + c := tmux.WithSocket(testSocket) + if err := c.KillPane(context.Background(), ""); err == nil { + t.Error("KillPane(empty) returned nil; want error") + } +} diff --git a/internal/workspace/agent_swap.go b/internal/workspace/agent_swap.go new file mode 100644 index 0000000..724b51a --- /dev/null +++ b/internal/workspace/agent_swap.go @@ -0,0 +1,270 @@ +// Agent swap orchestration. Implements the v0.22 `canopy agent swap +// ` verb: take a running workspace that currently has agent X in +// its tmux pane, kill that pane, persist the new agent type in state, +// and respawn the new agent in the same pane geometry. +// +// Why kill-pane (not kill-the-running-process-inside-the-pane): eng- +// review D5 picked clean-visual-state over preserved-scrollback. The +// old agent's per-directory conversation history survives via the +// agent's OWN resume mechanism (claude --continue, codex equivalent), +// not via tmux scrollback. The new pane shows ONLY the new agent's UI; +// no claude TUI artifacts leaking into codex's startup render. +// +// Why save+restore window-layout (not naive split-window with the same +// percentage): tmux's `split-window -l %` takes N as percent of the +// TARGET PANE. After kill-pane the remaining panes redistribute, so a +// fresh 30% split off the IDE post-kill produces drifted geometry vs. +// the original 30% split. Capturing window_layout before kill and +// SelectLayout-ing after respawn restores byte-precise geometry. +// +// Atomicity story: if respawn fails after kill, the workspace is left +// without an agent pane. The persisted state.Workspace.CurrentAgent +// still reflects the user's intent (the new agent), and the next +// canopy switch resurrects the agent in the new shape. Eng-review +// Open Q #3 — acceptable v1 risk. +package workspace + +import ( + "context" + "errors" + "fmt" + + "github.com/avinashjoshi/canopy/internal/agent" + "github.com/avinashjoshi/canopy/internal/state" + "github.com/avinashjoshi/canopy/internal/tmux" +) + +// ErrSwapNoAgentPane is returned by SwapAgent when no pane in the +// workspace's tmux session is tagged with `agent:*`. Either the +// workspace's session is gone (in which case the user should run +// canopy switch to resurrect it first) or the agent pane was manually +// killed; either way the swap can't proceed without first restoring a +// pane to kill. +var ErrSwapNoAgentPane = errors.New("workspace.SwapAgent: no agent:* pane found in session") + +// ErrSwapAlreadyCurrent is returned when the user asks to swap to the +// agent that's already running. Treating it as an error (rather than a +// no-op) catches typos and prevents silent unnecessary churn (a kill + +// respawn of the same agent would still lose tmux scrollback). +var ErrSwapAlreadyCurrent = errors.New("workspace.SwapAgent: workspace is already running this agent") + +// SwapAgent replaces the workspace's running agent pane with a fresh +// pane of newType. Returns the updated *state.Workspace on success. +// +// Validates: +// - newType is non-empty and in m.Cfg.Agents allowlist → else +// agent.ErrAgentNotAllowed. +// - newType != ws.CurrentAgent → else ErrSwapAlreadyCurrent. +// +// Sequence (see file-level comment for the why): +// +// 1. capture the session's window_layout +// 2. look up the current agent:* pane +// 3. tmux kill-pane on it +// 4. update state: ws.CurrentAgent = newType, save under WithLock +// 5. tmux split-window from the IDE pane, run the new agent's +// command (resume mode = true; the launcher's Resume argv carries +// claude --continue / codex equivalent so per-directory history +// survives the swap from the AGENT's perspective) +// 6. tag the new pane with agent.RoleForType(newType) +// 7. select-layout to restore captured geometry byte-precise +// 8. select-pane onto the new pane so the user lands on it +// +// All log events have structured fields so downstream observability can +// correlate "swap attempted" with "swap completed" / "swap failed +// mid-flight". +func (m *Manager) SwapAgent(ctx context.Context, name, newType string) (*state.Workspace, error) { + // Validate newType FIRST, before any tmux state change. Cheap gate + // that fails fast on typos / disallowed types without leaving the + // session in a half-modified state. + if !m.Cfg.AllowsAgent(newType) { + return nil, fmt.Errorf("%w: %q (allowed: %v)", + agent.ErrAgentNotAllowed, newType, m.Cfg.Agents) + } + + // Look up the workspace row + verify session exists. + st, err := m.Store.Load() + if err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: load state: %w", err) + } + ws, err := st.Find(m.Cfg.ProjectRoot, name) + if err != nil { + return nil, fmt.Errorf("workspace.SwapAgent(%s): %w", name, ErrWorkspaceNotFound) + } + wsCopy := *ws // defensive copy so subsequent state.Load doesn't share + + oldType := currentAgent(&wsCopy, m.Cfg) + if oldType == newType { + return nil, fmt.Errorf("%w: %q (current = %q)", + ErrSwapAlreadyCurrent, newType, oldType) + } + + session := wsCopy.TmuxSessionName() + if has, err := m.Tmux.HasSession(ctx, session); err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: HasSession: %w", err) + } else if !has { + return nil, fmt.Errorf("workspace.SwapAgent: session %q not running (try `canopy switch %s` first)", session, name) + } + + // Step 1: capture window-layout before kill so respawn can restore + // byte-precise geometry (kill-pane redistributes the remaining + // panes, so a fresh percentage-based split drifts). + layout, err := m.Tmux.CaptureWindowLayout(ctx, session) + if err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: capture layout: %w", err) + } + + // Step 2: verify the target launcher is installed on PATH BEFORE + // any destructive tmux/state operations. Without this, an allowed- + // in-canopy.json but not-installed-locally agent name (e.g., codex + // on a host where codex isn't on PATH) would tear down the running + // agent pane and only THEN fail when agentPaneCmd's Resolve → + // VerifyInstalled tripped — leaving the session paneless and the + // state mid-swap. (codex review P1 #3, 2026-06-25.) + launcher, err := agent.Resolve(newType) + if err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: resolve launcher %q: %w", newType, err) + } + if err := launcher.VerifyInstalled(); err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: %w", err) + } + + // Step 3: find the current agent pane. There should be exactly one + // in v0.22 (concurrent multi-agent is deferred); reject otherwise + // so a future refactor doesn't silently mis-target. + agentPanes, err := m.Tmux.LookupAllPanes(ctx, session, "agent:*") + if err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: lookup agent pane: %w", err) + } + if len(agentPanes) == 0 { + return nil, ErrSwapNoAgentPane + } + if len(agentPanes) > 1 { + return nil, fmt.Errorf("workspace.SwapAgent: session %q has %d agent:* panes; expected 1", session, len(agentPanes)) + } + oldAgentPaneID := agentPanes[0].ID + + // Step 4: find the IDE pane (we'll respawn off it). It's the most + // stable anchor in canopy's 3-pane layout — never killed by + // workspace lifecycle ops in the normal case. + idePaneID, err := m.Tmux.LookupPane(ctx, session, "ide") + if err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: lookup ide pane: %w", err) + } + + log.Info("workspace.agent-swap.start", + "session", session, + "workspace", name, + "from_agent", oldType, + "to_agent", newType, + "old_pane", oldAgentPaneID) + + // Step 4: kill the old agent pane. + if err := m.Tmux.KillPane(ctx, oldAgentPaneID); err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: kill-pane %s: %w", oldAgentPaneID, err) + } + + // Step 5: persist the new agent type. Done BEFORE the respawn so + // agentPaneCmd (which reads ws.CurrentAgent via currentAgent) sees + // the new value when we call it. Persisted under WithLock for the + // usual race-safety against concurrent state mutations. + var updated state.Workspace + err = m.Store.WithLock(func(s *state.State) error { + row, ferr := s.Find(m.Cfg.ProjectRoot, name) + if ferr != nil { + return ferr + } + row.CurrentAgent = newType + updated = *row + return nil + }) + if err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: persist new agent: %w", err) + } + + // Step 6: spawn the new agent. Resume vs Fresh decided by the + // per-(workspace, agent) launch counter (v0.22+): + // - AgentLaunches[newType] == 0 → Fresh. The agent has never + // run in this workspace, so its --continue/--resume machinery + // would find no prior session and exit immediately ("No + // conversation found to continue"). Fresh dodges that. + // - AgentLaunches[newType] > 0 → Resume. The agent has prior + // history here from an earlier launch; the agent's own resume + // argv (claude --continue, etc.) reaches it. + // This makes "swap claude → codex → claude" work the way the + // design promised in D5: swap-back auto-resumes the original + // conversation. + resume := updated.AgentLaunches[newType] > 0 + newAgentCmd, err := m.agentPaneCmd(&updated, resume) + if err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: build new agent cmd: %w", err) + } + // The split direction + size match buildSession's original. After + // SelectLayout restores byte-precise geometry, the size argument + // here only matters for the brief window between split and select. + // + // SelectPane(idePaneID) first so the upcoming SplitPane (which + // targets the SESSION'S ACTIVE pane via `tmux split-window -t + // `) splits off the IDE pane specifically. Without this, + // a swap invoked while the user's focus was on the shell pane + // would split the new agent off the shell, producing a wrong + // layout that SelectLayout's geometry restore can't fix on its + // own. (codex review P1 #4, 2026-06-25.) + if err := m.Tmux.SelectPane(ctx, idePaneID); err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: select ide pane before split: %w", err) + } + newAgentPane, err := m.Tmux.SplitPane(ctx, session, updated.Path, keepAlive(newAgentCmd), tmux.SplitHorizontal, 30) + if err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: split new agent pane: %w", err) + } + if err := m.Tmux.SetRole(ctx, newAgentPane, agent.RoleForType(newType)); err != nil { + return nil, fmt.Errorf("workspace.SwapAgent: tag new agent pane: %w", err) + } + + // Step 7: restore byte-precise geometry. + if err := m.Tmux.SelectLayout(ctx, session, layout); err != nil { + // Non-fatal: layout drift is a UX paper cut, not a correctness + // bug. Log loudly so dogfood notices. + log.Warn("workspace.agent-swap.restore-layout-failed", + "session", session, "err", err.Error()) + } + + // Step 8: land focus on the new agent pane (same rationale as + // buildSession: user expects to interact with the agent first). + if err := m.Tmux.SelectPane(ctx, newAgentPane); err != nil { + log.Warn("workspace.agent-swap.select-pane-failed", + "session", session, "err", err.Error()) + } + + // Step 9: bump the per-agent launch counter now that the new + // agent has spawned successfully. Done AFTER spawn (not in + // step 5's Save) so a failed spawn doesn't lie about history. + // Next swap to this agent will see AgentLaunches[newType]>0 + // and use Resume. + err = m.Store.WithLock(func(s *state.State) error { + row, ferr := s.Find(m.Cfg.ProjectRoot, name) + if ferr != nil { + return ferr + } + bumpAgentLaunches(row, newType) + row.AgentLaunchCount++ // legacy total counter, kept in sync + updated = *row + return nil + }) + if err != nil { + // Non-fatal: the swap succeeded, but next-swap heuristics + // won't know. Log loud; user-visible state is correct. + log.Warn("workspace.agent-swap.bump-launches-failed", + "session", session, "err", err.Error()) + } + + log.Info("workspace.agent-swap.done", + "session", session, + "workspace", name, + "from_agent", oldType, + "to_agent", newType, + "new_pane", newAgentPane, + "agent_launches", updated.AgentLaunches[newType]) + + return &updated, nil +} diff --git a/internal/workspace/agent_swap_test.go b/internal/workspace/agent_swap_test.go new file mode 100644 index 0000000..edfe02f --- /dev/null +++ b/internal/workspace/agent_swap_test.go @@ -0,0 +1,377 @@ +package workspace_test + +import ( + "bytes" + "context" + "errors" + "os" + "path/filepath" + "testing" + + "github.com/avinashjoshi/canopy/internal/agent" + "github.com/avinashjoshi/canopy/internal/config" + "github.com/avinashjoshi/canopy/internal/settings" + "github.com/avinashjoshi/canopy/internal/state" + "github.com/avinashjoshi/canopy/internal/tmux" + "github.com/avinashjoshi/canopy/internal/workspace" +) + +// fixtureWithAgents is fixture(), but the canopy.json declares +// `agents: ["claude", "codex"]` so SwapAgent's allowlist gate has both +// real launchers available. claude is the default (first entry). +func fixtureWithAgents(t *testing.T) (*workspace.Manager, func()) { + t.Helper() + mgr, cleanup := fixture(t) + // Overwrite canopy.json with one that declares agents. fixture() + // already wrote a minimal one without an agents block. + cfgJSON := `{ + "scripts": {"setup": "bin/canopy-setup", "run": "bin/canopy-run", "archive": "bin/canopy-archive"}, + "agents": ["claude", "codex"] + }` + cfgPath := filepath.Join(mgr.Cfg.ProjectRoot, "canopy.json") + if err := os.WriteFile(cfgPath, []byte(cfgJSON), 0o644); err != nil { + t.Fatalf("rewrite canopy.json: %v", err) + } + // Re-Load so mgr.Cfg picks up the new schema. (DiscoverAndLoad + // re-resolves project root + populates Agents via validate().) + newCfg, err := config.DiscoverAndLoad(mgr.Cfg.ProjectRoot) + if err != nil { + t.Fatalf("DiscoverAndLoad: %v", err) + } + mgr.Cfg = newCfg + return mgr, cleanup +} + +// TestSwapAgent_HappyPath: a claude workspace swapped to codex ends up +// with codex as the persisted CurrentAgent AND the agent pane's role +// tag updated to "agent:codex". The IDE + shell panes survive untouched. +func TestSwapAgent_HappyPath(t *testing.T) { + requireGitAndTmux(t) + mgr, _ := fixtureWithAgents(t) + + var stdout, stderr bytes.Buffer + ws, err := mgr.Create(context.Background(), "swap-happy", workspace.CreateOptions{}, &stdout, &stderr) + if err != nil { + t.Fatalf("Create: %v\nstdout: %s\nstderr: %s", err, stdout.String(), stderr.String()) + } + if ws.CurrentAgent != "claude" { + t.Fatalf("pre-swap CurrentAgent = %q; want claude (project default)", ws.CurrentAgent) + } + + updated, err := mgr.SwapAgent(context.Background(), ws.Name, "codex") + if err != nil { + t.Fatalf("SwapAgent: %v", err) + } + if updated.CurrentAgent != "codex" { + t.Errorf("post-swap CurrentAgent = %q; want codex", updated.CurrentAgent) + } + + // Tmux: the agent:* pane's role tag should now be agent:codex. + panes, err := mgr.Tmux.LookupAllPanes(context.Background(), ws.TmuxSessionName(), "agent:*") + if err != nil { + t.Fatalf("LookupAllPanes: %v", err) + } + if len(panes) != 1 { + t.Fatalf("agent:* pane count = %d; want 1", len(panes)) + } + if panes[0].Role != "agent:codex" { + t.Errorf("agent pane role = %q; want agent:codex", panes[0].Role) + } + + // IDE + shell panes are still present (not killed by the swap). + idePanes, err := mgr.Tmux.LookupAllPanes(context.Background(), ws.TmuxSessionName(), "ide") + if err != nil || len(idePanes) != 1 { + t.Errorf("IDE pane after swap: count=%d err=%v; want exactly 1", len(idePanes), err) + } + shellPanes, err := mgr.Tmux.LookupAllPanes(context.Background(), ws.TmuxSessionName(), "terminal:shell") + if err != nil || len(shellPanes) != 1 { + t.Errorf("Shell pane after swap: count=%d err=%v; want exactly 1", len(shellPanes), err) + } + + // State.json reflects the new agent (durable, not just in the + // returned struct). + store, err := state.NewStore(mgr.CanopyHome) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + st, err := store.Load() + if err != nil { + t.Fatalf("state.Load: %v", err) + } + row, err := st.Find(mgr.Cfg.ProjectRoot, ws.Name) + if err != nil { + t.Fatalf("state.Find: %v", err) + } + if row.CurrentAgent != "codex" { + t.Errorf("state.json CurrentAgent = %q; want codex (persisted)", row.CurrentAgent) + } +} + +// TestSwapAgent_DisallowedAgent: trying to swap to an agent that's +// registered as a launcher but NOT in canopy.json's agents allowlist +// returns ErrAgentNotAllowed. The tmux session is left untouched. +func TestSwapAgent_DisallowedAgent(t *testing.T) { + requireGitAndTmux(t) + mgr, _ := fixtureWithAgents(t) + + var stdout, stderr bytes.Buffer + ws, err := mgr.Create(context.Background(), "swap-disallowed", workspace.CreateOptions{}, &stdout, &stderr) + if err != nil { + t.Fatalf("Create: %v", err) + } + + // aider is a known launcher but NOT in our agents list. + _, err = mgr.SwapAgent(context.Background(), ws.Name, "aider") + if !errors.Is(err, agent.ErrAgentNotAllowed) { + t.Fatalf("SwapAgent(aider) err = %v; want errors.Is(..., agent.ErrAgentNotAllowed)", err) + } + + // State unchanged. + store, _ := state.NewStore(mgr.CanopyHome) + st, _ := store.Load() + row, _ := st.Find(mgr.Cfg.ProjectRoot, ws.Name) + if row.CurrentAgent != "claude" { + t.Errorf("state.CurrentAgent after rejected swap = %q; want claude (unchanged)", row.CurrentAgent) + } + + // Agent pane is still claude. + panes, _ := mgr.Tmux.LookupAllPanes(context.Background(), ws.TmuxSessionName(), "agent:*") + if len(panes) != 1 || panes[0].Role != "agent:claude" { + t.Errorf("agent pane after rejected swap = %v; want exactly 1 agent:claude", panes) + } +} + +// TestSwapAgent_AlreadyCurrent: swapping to the same agent that's +// already running is rejected with ErrSwapAlreadyCurrent (typo guard + +// avoid pointless kill/respawn churn). +func TestSwapAgent_AlreadyCurrent(t *testing.T) { + requireGitAndTmux(t) + mgr, _ := fixtureWithAgents(t) + + var stdout, stderr bytes.Buffer + ws, err := mgr.Create(context.Background(), "swap-noop", workspace.CreateOptions{}, &stdout, &stderr) + if err != nil { + t.Fatalf("Create: %v", err) + } + + _, err = mgr.SwapAgent(context.Background(), ws.Name, "claude") + if !errors.Is(err, workspace.ErrSwapAlreadyCurrent) { + t.Fatalf("SwapAgent(claude→claude) err = %v; want ErrSwapAlreadyCurrent", err) + } +} + +// Note: an integration test for the codex review P1 #3 fix +// ("VerifyInstalled before kill-pane") was attempted but PATH poisoning +// also breaks the tmux/git binaries SwapAgent reaches BEFORE the +// launcher check, making it impossible to isolate "agent binary +// missing" from the test harness. The fix lives at agent_swap.go +// step 2 (launcher.Resolve + VerifyInstalled before any tmux op); +// future test could mock VerifyInstalled via an injectable Resolver +// on Manager. + +// TestSwapAgent_FirstSwapUsesFresh_SecondSwapResumes pins the v0.22 +// per-(workspace, agent) launch-counter semantics: +// +// - On the FIRST swap to an agent that's never run in this +// workspace, AgentLaunches[target]==0 → spawn with Fresh argv +// (claude without --continue, codex without resume), so the +// agent doesn't fail with "No conversation found to continue". +// - On the SECOND swap to that same agent (after at least one +// prior launch + a swap-away), AgentLaunches[target]>0 → spawn +// with Resume argv. The agent's own resume verb reaches its +// prior session. +// +// This test inspects state.json's AgentLaunches map after each swap +// rather than the actual argv (which would require capturing the +// spawned process's args — possible but heavier). The map is the +// source of truth that drives the decision. +func TestSwapAgent_FirstSwapUsesFresh_SecondSwapResumes(t *testing.T) { + requireGitAndTmux(t) + mgr, _ := fixtureWithAgents(t) + + var stdout, stderr bytes.Buffer + ws, err := mgr.Create(context.Background(), "swap-resume", workspace.CreateOptions{}, &stdout, &stderr) + if err != nil { + t.Fatalf("Create: %v", err) + } + + // After Create, AgentLaunches should record one claude launch. + { + store, _ := state.NewStore(mgr.CanopyHome) + st, _ := store.Load() + row, _ := st.Find(mgr.Cfg.ProjectRoot, ws.Name) + if got := row.AgentLaunches["claude"]; got != 1 { + t.Errorf("post-Create AgentLaunches[claude] = %d; want 1", got) + } + if got := row.AgentLaunches["codex"]; got != 0 { + t.Errorf("post-Create AgentLaunches[codex] = %d; want 0", got) + } + } + + // First swap claude → codex. codex has never launched in this + // workspace; AgentLaunches[codex] should be 0 BEFORE the swap + // fires (so SwapAgent picks Fresh), then bumped to 1 AFTER. + if _, err := mgr.SwapAgent(context.Background(), ws.Name, "codex"); err != nil { + t.Fatalf("SwapAgent claude→codex: %v", err) + } + { + store, _ := state.NewStore(mgr.CanopyHome) + st, _ := store.Load() + row, _ := st.Find(mgr.Cfg.ProjectRoot, ws.Name) + if got := row.AgentLaunches["codex"]; got != 1 { + t.Errorf("post-first-swap AgentLaunches[codex] = %d; want 1", got) + } + } + + // Swap back codex → claude. claude has 1 prior launch (from + // Create), so swap-back should pick Resume. + if _, err := mgr.SwapAgent(context.Background(), ws.Name, "claude"); err != nil { + t.Fatalf("SwapAgent codex→claude: %v", err) + } + { + store, _ := state.NewStore(mgr.CanopyHome) + st, _ := store.Load() + row, _ := st.Find(mgr.Cfg.ProjectRoot, ws.Name) + if got := row.AgentLaunches["claude"]; got != 2 { + t.Errorf("post-swap-back AgentLaunches[claude] = %d; want 2", got) + } + } +} + +// TestMigrateCurrentAgents_BackfillsAgentLaunches: a pre-v0.22 row +// with non-zero AgentLaunchCount and nil AgentLaunches should get +// AgentLaunches populated from the legacy total counter under the +// "all prior launches were CurrentAgent" assumption (strictly true +// before swap existed). +func TestMigrateCurrentAgents_BackfillsAgentLaunches(t *testing.T) { + stateDir := t.TempDir() + projA := t.TempDir() + + store, err := state.NewStore(stateDir) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + preMigration := &state.State{ + Workspaces: []state.Workspace{ + { + ProjectRoot: projA, + Name: "legacy", + Branch: "legacy", + Status: state.StatusReady, + CurrentAgent: "claude", + AgentLaunchCount: 5, + // AgentLaunches intentionally nil — pre-v0.22 shape + }, + }, + } + if err := store.Save(preMigration); err != nil { + t.Fatalf("Save: %v", err) + } + + cfgJSON := `{"scripts":{"setup":"x","run":"x","archive":"x"},"agents":["claude","codex"]}` + if err := os.WriteFile(filepath.Join(projA, "canopy.json"), []byte(cfgJSON), 0o644); err != nil { + t.Fatalf("write canopy.json: %v", err) + } + cfg, err := config.DiscoverAndLoad(projA) + if err != nil { + t.Fatalf("DiscoverAndLoad: %v", err) + } + + mgr := &workspace.Manager{ + Cfg: cfg, + Store: store, + Tmux: tmux.WithSocket("canopy-migrate-launches-test"), + CanopyHome: stateDir, + Settings: settings.Settings{}, + } + if err := workspace.RunMigrateCurrentAgentsForTest(mgr); err != nil { + t.Fatalf("RunMigrateCurrentAgentsForTest: %v", err) + } + + st, _ := store.Load() + row, _ := st.Find(projA, "legacy") + if row.AgentLaunches == nil { + t.Fatal("AgentLaunches still nil after migration") + } + if got := row.AgentLaunches["claude"]; got != 5 { + t.Errorf("AgentLaunches[claude] = %d; want 5 (mirrors AgentLaunchCount)", got) + } +} + +// TestMigrateCurrentAgents_PreV022Row: a row written before v0.22 (no +// CurrentAgent field, JSON omits it) gets populated to the project's +// default agent the first time a Manager constructs for that project. +// Cross-project rows are NOT touched — verifies the per-project scope +// of the migration. +func TestMigrateCurrentAgents_PreV022Row(t *testing.T) { + stateDir := t.TempDir() + projA := t.TempDir() + projB := t.TempDir() + + // Hand-write a state.json with two rows: one for projA (this + // manager's project), one for projB (someone else's). Both have + // empty CurrentAgent — simulating the pre-v0.22 on-disk shape. + store, err := state.NewStore(stateDir) + if err != nil { + t.Fatalf("NewStore: %v", err) + } + preMigration := &state.State{ + Workspaces: []state.Workspace{ + {ProjectRoot: projA, Name: "alpha", Branch: "alpha", Status: state.StatusReady}, + {ProjectRoot: projB, Name: "beta", Branch: "beta", Status: state.StatusReady}, + }, + } + if err := store.Save(preMigration); err != nil { + t.Fatalf("Save: %v", err) + } + + // Build a canopy.json for projA that declares agents: ["codex", "claude"] + // so the default agent is codex (not claude). + cfgJSON := `{ + "scripts": {"setup": "x", "run": "x", "archive": "x"}, + "agents": ["codex", "claude"] + }` + if err := os.WriteFile(filepath.Join(projA, "canopy.json"), []byte(cfgJSON), 0o644); err != nil { + t.Fatalf("write canopy.json: %v", err) + } + cfg, err := config.DiscoverAndLoad(projA) + if err != nil { + t.Fatalf("DiscoverAndLoad: %v", err) + } + + // Manually compose a Manager (bypassing workspace.New since New + // owns CanopyHome resolution and we want stateDir, not ~/.canopy). + // Call migrateCurrentAgents indirectly by constructing the same + // way New does, minus settings noise. + mgr := &workspace.Manager{ + Cfg: cfg, + Store: store, + Tmux: tmux.WithSocket("canopy-migrate-test"), + CanopyHome: stateDir, + Settings: settings.Settings{}, + } + // MigrateAgents is exported via the Manager's method on construction; + // New() calls it via workspace.New. Here we exercise the same code + // path by re-running it directly. The test helper exists so tests + // can invoke without going through the home-dir setup. + if err := workspace.RunMigrateCurrentAgentsForTest(mgr); err != nil { + t.Fatalf("MigrateCurrentAgents: %v", err) + } + + st, err := store.Load() + if err != nil { + t.Fatalf("Load post-migration: %v", err) + } + got := map[string]string{} + for _, w := range st.Workspaces { + got[w.Name] = w.CurrentAgent + } + if got["alpha"] != "codex" { + t.Errorf("projA row CurrentAgent = %q; want codex (project default)", got["alpha"]) + } + // projB row stays empty: this Manager isn't the right context to + // migrate it (would need projB's canopy.json to know the default). + if got["beta"] != "" { + t.Errorf("projB row CurrentAgent = %q; want empty (cross-project, untouched)", got["beta"]) + } +} diff --git a/internal/workspace/export_test.go b/internal/workspace/export_test.go new file mode 100644 index 0000000..c6a2f02 --- /dev/null +++ b/internal/workspace/export_test.go @@ -0,0 +1,15 @@ +package workspace + +// This file is _test.go-suffixed so its exports are visible ONLY to +// the workspace_test package (Go's convention for "internal test +// helpers without polluting the production API"). Production code +// can't see RunMigrateCurrentAgentsForTest, so the migration entry +// point stays unexported in the binary. + +// RunMigrateCurrentAgentsForTest exposes migrateCurrentAgents for test +// code that constructs a *Manager manually (bypassing New) to exercise +// the migration without setting up a fake ~/.canopy. Production callers +// hit migrateCurrentAgents through New() instead. +func RunMigrateCurrentAgentsForTest(m *Manager) error { + return m.migrateCurrentAgents() +} diff --git a/internal/workspace/initprompt.go b/internal/workspace/initprompt.go index 9a00a5b..3fd930d 100644 --- a/internal/workspace/initprompt.go +++ b/internal/workspace/initprompt.go @@ -93,29 +93,32 @@ func SendInitialPrompt( Reason: fmt.Sprintf("malformed @canopy-role tag %q (cannot derive launcher)", agentPane.Role), } } - if launcher != "claude" { - return &ErrPromptFailed{ - Reason: fmt.Sprintf( - "--prompt is only supported for agent:claude in v0.16.1 (got agent:%s)", - launcher), - } - } + // v0.22: dispatch by classifier instead of the v0.16-era launcher== + // "claude" gate. Each launcher's Classifier provides IsRendering / + // IsTrustDialog; --prompt now works for any launcher with + // registered patterns (claude, codex). Stub launchers (opencode, + // aider) return false from both helpers, so the Phase-1 trust-loop + // times out without sending anything — same observable outcome as + // the old gate but without the special case. + classifier := agent.ClassifierFor(launcher) - if err := awaitClaudeReady(ctx, tx, agentPane.ID, progress); err != nil { + if err := awaitAgentReady(ctx, tx, agentPane.ID, classifier, progress); err != nil { return err } - // Phase 3: re-verify claude is rendering, not the keepAlive shell. - // awaitClaudeReady already saw a claude marker once; this guards - // against the rare race where claude crashed BETWEEN Phase 2 exit - // and now. + // Phase 3: re-verify the agent is rendering, not the keepAlive + // shell. awaitAgentReady already saw an agent marker once; this + // guards against the rare race where the agent crashed BETWEEN + // Phase 2 exit and now. captured, err := capturePaneTimeout(ctx, tx, agentPane.ID, 500*time.Millisecond) if err != nil { return &ErrPromptFailed{Reason: "Phase 3 verify capture-pane failed: " + err.Error()} } - if !agent.IsClaudeRendering(captured) { + if !classifier.IsRendering(captured) { return &ErrPromptFailed{ - Reason: "agent pane is shell, not claude (claude may have crashed); refusing to send-keys to defend against command injection", + Reason: fmt.Sprintf( + "agent pane is shell, not %s (agent may have crashed); refusing to send-keys to defend against command injection", + launcher), } } @@ -164,13 +167,22 @@ func IsPromptFailed(err error) (*ErrPromptFailed, bool) { return nil, false } -// awaitClaudeReady runs Phase 1 + Phase 2 of the trust state machine. -// Returns nil when claude is verified rendering. Returns +// awaitAgentReady runs Phase 1 + Phase 2 of the trust state machine. +// Returns nil when the agent is verified rendering. Returns // *ErrPromptFailed for either phase timeout. -func awaitClaudeReady( +// +// classifier is the per-launcher Classifier (from +// agent.ClassifierFor(launcher)); its IsRendering / IsTrustDialog drive +// the dispatch. For stub launchers (opencode/aider), both return +// false, so the trust-dialog branch never fires and the ready-marker +// branch never matches — Phase 1 times out cleanly. The pre-v0.22 +// behavior was a hardcoded gate against `launcher != "claude"`; the +// timeout-with-classifier-stub is observationally equivalent. +func awaitAgentReady( ctx context.Context, tx *tmux.Client, paneID string, + classifier agent.Classifier, progress io.Writer, ) error { const ( @@ -185,11 +197,11 @@ func awaitClaudeReady( for time.Now().Before(phase1Deadline) { captured, err := capturePaneTimeout(ctx, tx, paneID, pollInterval) if err == nil { - if agent.IsClaudeRendering(captured) { + if classifier.IsRendering(captured) { clearProgress(progress) return nil } - if agent.IsTrustDialog(captured) { + if classifier.IsTrustDialog(captured) { trustSeen = true dismissCtx, dismissCancel := context.WithTimeout(ctx, 2*time.Second) err := tx.SendKeyName(dismissCtx, paneID, "Enter") @@ -212,11 +224,11 @@ func awaitClaudeReady( // One more capture in case the ready marker rendered right at // the deadline (avoids a flake-driven timeout). if captured, err := capturePaneTimeout(ctx, tx, paneID, pollInterval); err == nil && - agent.IsClaudeRendering(captured) { + classifier.IsRendering(captured) { return nil } return &ErrPromptFailed{ - Reason: "Phase 1 timeout: neither trust dialog nor claude ready marker appeared in 5s", + Reason: "Phase 1 timeout: neither trust dialog nor agent ready marker appeared in 5s", } } @@ -225,17 +237,17 @@ func awaitClaudeReady( phase2Deadline := phase2Start.Add(phaseBudget) for time.Now().Before(phase2Deadline) { captured, err := capturePaneTimeout(ctx, tx, paneID, pollInterval) - if err == nil && agent.IsClaudeRendering(captured) { + if err == nil && classifier.IsRendering(captured) { clearProgress(progress) return nil } elapsed := time.Since(phase2Start).Round(time.Second) - fmt.Fprintf(progress, "\rWaiting for claude (post-trust)... %s / %s ", elapsed, phaseBudget) + fmt.Fprintf(progress, "\rWaiting for agent (post-trust)... %s / %s ", elapsed, phaseBudget) time.Sleep(pollInterval) } clearProgress(progress) return &ErrPromptFailed{ - Reason: "Phase 2 timeout: claude ready marker never appeared in 5s after trust dismiss", + Reason: "Phase 2 timeout: agent ready marker never appeared in 5s after trust dismiss", } } diff --git a/internal/workspace/lifecycle.go b/internal/workspace/lifecycle.go index 764a3d6..d94a3a7 100644 --- a/internal/workspace/lifecycle.go +++ b/internal/workspace/lifecycle.go @@ -130,6 +130,18 @@ func New(cfg *config.Config) (*Manager, error) { if err := m.migrateAndGuard(); err != nil { return nil, err } + // v0.22: populate CurrentAgent on any pre-v0.22 row for THIS + // project. Cross-project rows get migrated when their own Manager + // constructs (canopy is usually invoked per project; the Global tab + // in the TUI iterates Managers per project too). Eager-on-construct + // means the first canopy ls / canopy switch / canopy new after the + // upgrade converges all of this project's rows in one flock- + // protected pass. Best-effort: failures are logged, not surfaced — + // the workspace still functions, just with the v0.22 field empty + // (downstream code falls back to Cfg.DefaultAgent()). + if err := m.migrateCurrentAgents(); err != nil { + log.Warn("workspace.migrate-current-agents", "err", err.Error()) + } return m, nil } @@ -161,6 +173,67 @@ func (m *Manager) migrateAndGuard() error { }) } +// migrateCurrentAgents fills the v0.22 state.Workspace.CurrentAgent +// field on any row for THIS Manager's project that's missing it. Runs +// once per Manager construction, under the state lock. Source +// precedence matches the eng-review D9 contract: +// +// 1. Cfg.Agents[0] — the new canonical allowlist's first entry +// 2. Cfg.Agent.Type — legacy single-agent block (handled by config +// validate() as the same value as Cfg.Agents[0], but defensive) +// 3. "claude" — last-resort default for the rare case where neither +// is set on a Cfg constructed outside Load (test code) +// +// Cross-project rows in the shared state.json are left alone: this +// Manager only has the right Cfg for its own project. Their migration +// happens when their own project's Manager constructs. +// +// Idempotent: rows that already have CurrentAgent set are skipped. The +// flock-protected Save fires unconditionally because WithLock always +// saves on a clean fn return — that's a tiny cost (the JSON is already +// what we'd write) and matches the existing migrate / reconcile +// pattern in this package. +func (m *Manager) migrateCurrentAgents() error { + defaultAgent := m.Cfg.DefaultAgent() + if defaultAgent == "" { + defaultAgent = "claude" + } + return m.Store.WithLock(func(s *state.State) error { + migratedCurrent := 0 + migratedLaunches := 0 + for i := range s.Workspaces { + w := &s.Workspaces[i] + if w.ProjectRoot != m.Cfg.ProjectRoot { + continue + } + if w.CurrentAgent == "" { + w.CurrentAgent = defaultAgent + migratedCurrent++ + } + // v0.22+: populate AgentLaunches from the legacy total + // counter under the "all prior launches were CurrentAgent" + // assumption — which was strictly true before swap + // existed. Only fires when AgentLaunches is nil (never + // migrated); subsequent runs leave it alone. + if w.AgentLaunches == nil { + w.AgentLaunches = map[string]int{} + if w.CurrentAgent != "" && w.AgentLaunchCount > 0 { + w.AgentLaunches[w.CurrentAgent] = w.AgentLaunchCount + } + migratedLaunches++ + } + } + if migratedCurrent > 0 || migratedLaunches > 0 { + log.Info("workspace.current-agent.migrated", + "project", m.Cfg.Project, + "current_rows", migratedCurrent, + "launches_rows", migratedLaunches, + "default_agent", defaultAgent) + } + return nil + }) +} + // workspacesDir returns /workspaces/. Created on // demand by Create; safe to call before the dir exists. func (m *Manager) workspacesDir() string { @@ -225,6 +298,18 @@ type CreateOptions struct { // to review); empty for fresh/issue/branch (the workspace is mine). // See state.Workspace.Owner for the render semantics. Owner string + + // Agent is the launcher type this workspace should run when it + // boots. v0.22 `canopy new --agent ` sets this. Empty value + // means "use the project's default" — Create falls back to + // Cfg.DefaultAgent() (Cfg.Agents[0] or "claude"). + // + // Validation happens at the CLI layer (cmd/canopy/new.go) via + // Cfg.AllowsAgent before Create is called; Create itself trusts + // the caller, since internal call sites may legitimately compose + // options with values that don't match the project's current + // allowlist (e.g. tests that exercise migration paths). + Agent string } // Create runs the full workspace setup lifecycle. name may be empty to @@ -351,6 +436,18 @@ func (m *Manager) Create(ctx context.Context, name string, opts CreateOptions, s sourceKind = "fresh" } + // Snapshot the workspace's launcher: opts.Agent if the caller + // supplied one (--agent on the CLI), else the project's default. + // Empty fallback to "claude" is defensive; Cfg.DefaultAgent() + // already guarantees non-empty for a Cfg that went through Load. + chosenAgent := opts.Agent + if chosenAgent == "" { + chosenAgent = m.Cfg.DefaultAgent() + } + if chosenAgent == "" { + chosenAgent = "claude" + } + ws = state.Workspace{ ProjectRoot: m.Cfg.ProjectRoot, // v2 authoritative key (basename derived via ProjectBasename()) Name: name, @@ -363,6 +460,7 @@ func (m *Manager) Create(ctx context.Context, name string, opts CreateOptions, s SourceContext: opts.SourceContext, NameAutoGenerated: nameWasEmpty, Owner: opts.Owner, + CurrentAgent: chosenAgent, } return s.Add(ws) }) @@ -398,6 +496,7 @@ func (m *Manager) Create(ctx context.Context, name string, opts CreateOptions, s row.Status = state.StatusReady row.LastError = "" row.AgentLaunchCount++ + bumpAgentLaunches(row, currentAgent(row, m.Cfg)) ws = *row return nil }) @@ -696,7 +795,7 @@ func (m *Manager) buildSession(ctx context.Context, ws *state.Workspace) error { if err != nil { return err } - if err := m.Tmux.SetRole(ctx, agentPane, agent.RoleForType(m.Cfg.Agent.Type)); err != nil { + if err := m.Tmux.SetRole(ctx, agentPane, agent.RoleForType(currentAgent(ws, m.Cfg))); err != nil { return fmt.Errorf("workspace.buildSession: tag agent pane: %w", err) } // Land the active pane on the agent pane — that's the thing the @@ -733,8 +832,32 @@ func (m *Manager) buildSession(ctx context.Context, ws *state.Workspace) error { // state changes between this call and the agent actually launching are // not reflected — that's acceptable; the agent gets a fresh briefing // on every relaunch. +// currentAgent returns the launcher type to spawn for this workspace. +// +// Precedence: +// 1. ws.CurrentAgent — the v0.22 per-workspace snapshot. The Manager's +// migrateCurrentAgents pass on New() populates this for any pre- +// v0.22 row, so production calls almost always take this branch. +// 2. cfg.DefaultAgent() — fallback for the brief window between row +// creation and the first Save (in-memory workspace.Create returns +// a *state.Workspace that hasn't been persisted yet) and for test +// code that constructs a *state.Workspace literal without going +// through the migration. +// +// Never returns empty: cfg.DefaultAgent() falls back to "claude" when +// the config has no Agents and no Agent.Type. +func currentAgent(ws *state.Workspace, cfg *config.Config) string { + if ws != nil && ws.CurrentAgent != "" { + return ws.CurrentAgent + } + if cfg != nil { + return cfg.DefaultAgent() + } + return "claude" +} + func (m *Manager) agentPaneCmd(ws *state.Workspace, resume bool) (string, error) { - launcher, err := agent.Resolve(m.Cfg.Agent.Type) + launcher, err := agent.Resolve(currentAgent(ws, m.Cfg)) if err != nil { // Typo in canopy.json's agent.type — fail loud. This is a // config error, not an environment gap; we don't want to @@ -763,7 +886,7 @@ func (m *Manager) agentPaneCmd(ws *state.Workspace, resume bool) (string, error) // TUI refresh will pick it up. hints := lifecycle.RunFast(context.Background(), *ws) - briefing := agent.BuildBriefing(*ws, m.Cfg, hints) + briefing := agent.BuildBriefing(*ws, m.Cfg, hints, currentAgent(ws, m.Cfg)) // Write briefing to a temp file. Empty briefing → empty path → the // launcher drops the flag entirely (handled in PlanLaunch). @@ -1021,7 +1144,7 @@ func (m *Manager) Resurrect(ctx context.Context, name string) (*state.Workspace, if err != nil { return nil, err } - if err := m.Tmux.SetRole(ctx, agentPane, agent.RoleForType(m.Cfg.Agent.Type)); err != nil { + if err := m.Tmux.SetRole(ctx, agentPane, agent.RoleForType(currentAgent(&wsCopy, m.Cfg))); err != nil { return nil, fmt.Errorf("workspace.Resurrect: tag agent pane: %w", err) } // Land active pane on the agent — same rationale as buildSession. @@ -1041,6 +1164,7 @@ func (m *Manager) Resurrect(ctx context.Context, name string) (*state.Workspace, row.Status = state.StatusReady row.LastError = "" row.AgentLaunchCount++ + bumpAgentLaunches(row, currentAgent(&wsCopy, m.Cfg)) wsCopy = *row return nil }) @@ -1050,6 +1174,24 @@ func (m *Manager) Resurrect(ctx context.Context, name string) (*state.Workspace, return &wsCopy, nil } +// bumpAgentLaunches increments the per-agent launch counter in +// state.Workspace.AgentLaunches. Lazily initializes the map; safe +// to call on any row regardless of migration state. v0.22. +// +// Used by buildSession, Resurrect, and SwapAgent — every site where +// canopy actually spawns an agent process. The counter is read by +// SwapAgent to decide Resume vs Fresh (count>0 → Resume; count==0 +// → Fresh, since the agent has no prior session in this workspace). +func bumpAgentLaunches(row *state.Workspace, agentName string) { + if row == nil || agentName == "" { + return + } + if row.AgentLaunches == nil { + row.AgentLaunches = map[string]int{} + } + row.AgentLaunches[agentName]++ +} + // BareAttach returns a tmux session name to attach to for a "diagnostic" // view of the workspace: a one-pane shell at the workspace dir with // CANOPY_* env vars set, but WITHOUT running scripts.setup or rebuilding From 13cac9fda4b041ad59f34c6045f5ba5444c7a708 Mon Sep 17 00:00:00 2001 From: Avinash Joshi Date: Thu, 25 Jun 2026 22:25:13 -0700 Subject: [PATCH 2/5] feat(cli+tui): canopy agent swap, canopy ask, TUI pickers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CLI surface: - 'canopy agent swap ' resolves the workspace from cwd (walks up through dot-prefixed subdirs like .github/.config/.gstack via the isInsideWorkspace helper — !strings.HasPrefix(rel, '..') instead of the old first-char-'.' test) and dispatches to workspace.SwapAgent - 'canopy ask [--file path | prompt]' one-shot non-interactive invocation against the launcher's Exec mode. ResolveExec gate runs BEFORE AddAgentToCanopyJSON so opencode (no Exec) errors cleanly without leaving a config side-effect - 'canopy new --agent ' picks the launcher at workspace creation; remote dispatch forwards --agent to the remote canopy - startup sweep clears stale ~/.canopy/tmp/ask-* files TUI surface: - 'A' opens agentSwapPickerMode (lists project's agents: allowlist plus any installed-but-unlisted launcher; picking the latter silently auto-adds it to canopy.json) - 'Q' opens askPickerMode → askInputMode (textarea) → Ctrl+S writes the question to ~/.canopy/tmp/ask-.md (atomic tmpfile + rename) and spawns 'canopy ask' inside a tmux display-popup - popup body wraps with '; read -r _' so fast answers don't vanish before the user can read them; tmpPath passed as a positional shell argument (bash -c '...' _ '') with POSIX single-quoting so a $HOME with spaces / $ / ' survives intact Tests: cmd/canopy/agent_test (isInsideWorkspace table over 10 dot/dotdot cases), ask_test (happy path + error paths), update_ask_test (popup command shape, posixShellQuote round-trip, positional-arg pattern), update_agent_swap_test (picker nav + Enter dispatch + cancel). Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/canopy/agent.go | 170 ++++++++++++ cmd/canopy/agent_test.go | 36 +++ cmd/canopy/ask.go | 349 +++++++++++++++++++++++ cmd/canopy/ask_test.go | 108 ++++++++ cmd/canopy/main.go | 8 + cmd/canopy/new.go | 26 ++ internal/ui/keymap.go | 26 ++ internal/ui/model.go | 40 +++ internal/ui/update.go | 12 + internal/ui/update_agent_swap.go | 329 ++++++++++++++++++++++ internal/ui/update_agent_swap_test.go | 246 +++++++++++++++++ internal/ui/update_ask.go | 381 ++++++++++++++++++++++++++ internal/ui/update_ask_test.go | 325 ++++++++++++++++++++++ internal/ui/view.go | 6 + 14 files changed, 2062 insertions(+) create mode 100644 cmd/canopy/agent.go create mode 100644 cmd/canopy/agent_test.go create mode 100644 cmd/canopy/ask.go create mode 100644 cmd/canopy/ask_test.go create mode 100644 internal/ui/update_agent_swap.go create mode 100644 internal/ui/update_agent_swap_test.go create mode 100644 internal/ui/update_ask.go create mode 100644 internal/ui/update_ask_test.go diff --git a/cmd/canopy/agent.go b/cmd/canopy/agent.go new file mode 100644 index 0000000..9cce908 --- /dev/null +++ b/cmd/canopy/agent.go @@ -0,0 +1,170 @@ +package main + +import ( + "context" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + + "github.com/spf13/cobra" + + "github.com/avinashjoshi/canopy/internal/agent" + "github.com/avinashjoshi/canopy/internal/workspace" +) + +// agentCmd returns the `canopy agent ` cobra group. The +// only subcommand in v0.22 is `swap `. The noun-space is +// deliberately reserved for future read commands (eng-review D11): +// +// canopy agent list # show current/available agents (TODO) +// canopy agent status # show running state (TODO) +// canopy agent reset # kill + relaunch same agent (TODO) +// canopy agent swap # swap to a different agent ← v0.22 +// +// Putting `swap` under the noun keeps the verb shape consistent across +// all future subcommands and avoids the verb-collision foot-gun a bare +// `canopy agent ` would have introduced. +func agentCmd() *cobra.Command { + c := &cobra.Command{ + Use: "agent", + Short: "Manage the running agent in the current workspace", + Long: "Subcommands for inspecting and manipulating the agent process running\n" + + "in the current workspace's tmux session. v0.22 ships `swap`; `list`,\n" + + "`status`, and `reset` are reserved noun-space for follow-ups.", + } + c.AddCommand(agentSwapCmd()) + return c +} + +// agentSwapCmd returns the `canopy agent swap ` subcommand. +// +// Locates the workspace by walking up from cwd to find canopy.json +// (same machinery as canopy ls / switch / rm), then finds the workspace +// row whose Path is an ancestor of cwd. Calls Manager.SwapAgent which +// validates the target against canopy.json's agents allowlist, kills +// the current agent pane, persists the new type, respawns the new +// agent, and restores byte-precise window-layout. +// +// Exits 0 on success, 1 on validation/state errors (e.g., +// ErrAgentNotAllowed, ErrSwapAlreadyCurrent, missing session). The +// session stays on whatever screen it was on before — the user reads +// the success line in their shell and tmux already shows the new +// agent's UI in the agent pane. +func agentSwapCmd() *cobra.Command { + return &cobra.Command{ + Use: "swap ", + Short: "Swap the running agent in this workspace to ", + Long: "Locates the workspace by walking up from cwd to find canopy.json,\n" + + "then kills the running agent's pane and respawns in the same\n" + + "layout. Window geometry is preserved byte-precise; the agent's own\n" + + "per-directory conversation history is preserved by the agent itself\n" + + "(claude --continue, codex equivalent), not by tmux scrollback.\n\n" + + " must be in canopy.json's `agents:` allowlist. Returns\n" + + "ErrAgentNotAllowed if not.", + Args: cobra.ExactArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() + newType := args[0] + + mgr, err := loadManager() + if err != nil { + return err + } + + // Locate this workspace by walking up from cwd until we hit + // a registered workspace path. Use the same cwd-walk that + // `canopy rm` and `canopy switch` use implicitly. + cwd, err := os.Getwd() + if err != nil { + return fmt.Errorf("canopy agent swap: getwd: %w", err) + } + name, err := findWorkspaceFromCwd(ctx, mgr, cwd) + if err != nil { + return err + } + + ws, err := mgr.SwapAgent(ctx, name, newType) + if err != nil { + // Pretty-print known sentinels so the user gets clean + // guidance instead of a wrapped chain. + if errors.Is(err, agent.ErrAgentNotAllowed) { + fmt.Fprintf(cmd.ErrOrStderr(), + "canopy agent swap: %q is not in this project's `agents:` list.\n"+ + "Allowed: %v\n"+ + "Edit canopy.json to add it, or pick from the allowed list.\n", + newType, mgr.Cfg.Agents) + return err + } + return err + } + + fmt.Fprintf(cmd.OutOrStdout(), + "Swapped %s → %s in workspace %q.\n"+ + "The new agent has been spawned in the same pane geometry.\n"+ + "Run `canopy switch %s` to attach if you're not already there.\n", + ws.CurrentAgent, newType, ws.Name, ws.Name) + return nil + }, + } +} + +// findWorkspaceFromCwd walks up from cwd looking for a registered +// workspace path. Returns the workspace name. If no workspace contains +// cwd, returns a clean error suggesting `canopy ls` to see available +// workspaces. +// +// Lives here (rather than in the workspace package) because it's +// CLI-shape concern: the workspace package operates on names; the CLI +// gets a name from the filesystem. +func findWorkspaceFromCwd(ctx context.Context, mgr *workspace.Manager, cwd string) (string, error) { + absCwd, err := filepath.Abs(cwd) + if err != nil { + return "", fmt.Errorf("canopy agent swap: abs cwd: %w", err) + } + if resolved, lerr := filepath.EvalSymlinks(absCwd); lerr == nil { + absCwd = resolved + } + list, err := mgr.List(ctx) + if err != nil { + return "", fmt.Errorf("canopy agent swap: list workspaces: %w", err) + } + for _, ws := range list { + wsPath := ws.Path + if resolved, lerr := filepath.EvalSymlinks(wsPath); lerr == nil { + wsPath = resolved + } + // Walk up from cwd: workspace match when cwd == wsPath OR cwd + // is inside wsPath. Use filepath.Rel to detect ancestry safely. + rel, rerr := filepath.Rel(wsPath, absCwd) + if rerr != nil { + continue + } + if isInsideWorkspace(rel) { + return ws.Name, nil + } + } + return "", fmt.Errorf( + "canopy agent swap: cwd %q is not inside any registered workspace; cd into a workspace first or run `canopy ls` to see them", + cwd) +} + +// isInsideWorkspace reports whether `rel` (output of filepath.Rel(wsPath, cwd)) +// indicates cwd is the workspace dir itself or a descendant of it. +// +// rel == "." → cwd IS wsPath. Otherwise the only signal that cwd is +// OUTSIDE wsPath is a leading "..". A naive "first char != '.'" test +// trips on dot-prefixed subdirectories (.github, .config, .gstack) +// because filepath.Rel returns ".github" for a cwd one level inside the +// workspace's .github directory — first char IS '.', but the path is +// still inside. (codex review P2 #3, 2026-06-25.) +func isInsideWorkspace(rel string) bool { + if rel == "." { + return true + } + if rel == ".." || strings.HasPrefix(rel, "../") || strings.HasPrefix(rel, `..\`) { + return false + } + return true +} diff --git a/cmd/canopy/agent_test.go b/cmd/canopy/agent_test.go new file mode 100644 index 0000000..551d975 --- /dev/null +++ b/cmd/canopy/agent_test.go @@ -0,0 +1,36 @@ +package main + +import "testing" + +// TestIsInsideWorkspace pins the rel-classifying logic that +// findWorkspaceFromCwd uses to decide whether cwd lives inside a +// registered workspace. The codex review (P2 #3, 2026-06-25) caught a +// false-reject on dot-prefixed subdirectories — when cwd is one level +// inside .github or .config, filepath.Rel returns ".github" / ".config" +// and the old `rel[0] != '.'` test rejected them. +func TestIsInsideWorkspace(t *testing.T) { + cases := []struct { + rel string + want bool + why string + }{ + {".", true, "rel == . → cwd IS the workspace"}, + {"docs", true, "plain subdir"}, + {"docs/api/v1", true, "deep subdir"}, + {".github", true, "dot-prefixed subdir (the codex bug — .github used to false-reject)"}, + {".github/workflows", true, "deep inside dot-prefixed subdir"}, + {".config/canopy", true, "another dot-prefixed subdir we'd expect to land in"}, + {".gstack/tmp", true, "canopy's own .gstack subdir"}, + {"..", false, "rel == .. → cwd is the parent of wsPath"}, + {"../sibling", false, "rel == ../sibling → cwd is a sibling, outside"}, + {"../../other-project", false, "deeper outside"}, + } + for _, c := range cases { + t.Run(c.rel, func(t *testing.T) { + got := isInsideWorkspace(c.rel) + if got != c.want { + t.Errorf("isInsideWorkspace(%q) = %v; want %v (%s)", c.rel, got, c.want, c.why) + } + }) + } +} diff --git a/cmd/canopy/ask.go b/cmd/canopy/ask.go new file mode 100644 index 0000000..e33622b --- /dev/null +++ b/cmd/canopy/ask.go @@ -0,0 +1,349 @@ +package main + +import ( + "context" + "errors" + "fmt" + "io" + "os" + "os/exec" + "path/filepath" + "strings" + "time" + + "github.com/spf13/cobra" + + "github.com/avinashjoshi/canopy/internal/agent" + "github.com/avinashjoshi/canopy/internal/config" + "github.com/avinashjoshi/canopy/internal/workspace" +) + +// askFlags holds the parsed CLI flags for `canopy ask`. v0.22. +var askFlags struct { + file string // --file : read question body from file (used by the TUI popup) + stdin bool // --stdin: read question body from stdin + timeout time.Duration // --timeout: per-invocation deadline (default 60s) +} + +// Exit codes for `canopy ask`. The TUI popup parses these to render +// distinct messages instead of trying to grep stderr. See design doc +// §1 step 7 (the exit-code-collision fix). +const ( + askExitOK = 0 + askExitGeneric = 1 + askExitAgentNotAllowed = 2 + askExitLauncherNoExec = 3 + askExitTimeout = 4 + askExitBinaryNotInstalled = 5 +) + +// askCmd returns the `canopy ask [question]` cobra subcommand. +// Pattern + behavior: design doc at +// ~/.gstack/projects/avinashjoshi-canopy/cassy-add-codex-support-concurrent-multi-agent-design-20260625-110939.md +// +// Three input modes: +// - positional inline: `canopy ask codex "question"` +// - --file : `canopy ask codex --file ./bug.md` (TUI popup uses this) +// - --stdin: `echo "question" | canopy ask codex --stdin` +// +// Exactly one must be supplied; mixing is an error so users don't +// accidentally lose intent (e.g., piping stdin while also passing a +// positional and not knowing which canopy actually used). +func askCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "ask [question]", + Short: "Ask a one-shot question to a different agent without leaving the workspace", + Long: "Runs ` exec` (or its non-interactive equivalent) with the question and\n" + + "a brief workspace-context prefix. Output streams to stdout; agent chatter to\n" + + "stderr. Atomic — no session continuity, no multi-turn. For multi-turn handoff,\n" + + "use `canopy agent swap ` instead.\n\n" + + "Input modes (exactly one):\n" + + " positional canopy ask codex \"what does this regex do?\"\n" + + " --file canopy ask codex --file ./context.md\n" + + " --stdin echo \"question\" | canopy ask codex --stdin\n\n" + + "Exit codes: 0 success, 1 generic error, 2 ErrAgentNotAllowed,\n" + + "3 ErrLauncherNoExec, 4 timeout, 5 binary missing.", + Args: cobra.MinimumNArgs(1), + RunE: func(cmd *cobra.Command, args []string) error { + ctx := cmd.Context() + agentName := args[0] + + // Load the question. Exactly one input mode must be active. + question, err := loadAskQuestion(args[1:], askFlags.file, askFlags.stdin, cmd.InOrStdin()) + if err != nil { + return err + } + if strings.TrimSpace(question) == "" { + return fmt.Errorf("canopy ask: question body is empty") + } + + mgr, err := loadManager() + if err != nil { + return err + } + + // D6=A: auto-add the agent to canopy.json if it's installed + // AND has a one-shot exec mode. `canopy ask` is invoked by + // the TUI popup AND directly by users; both want the same + // "any installed launcher just works" behavior. We check + // Resolve / VerifyInstalled / ResolveExec FIRST so a launcher + // that fails any of those gates doesn't leave a useless + // config side-effect on disk (codex review P2 #7). + if !mgr.Cfg.AllowsAgent(agentName) { + launcher, lerr := agent.Resolve(agentName) + if lerr != nil { + fmt.Fprintf(cmd.ErrOrStderr(), "canopy ask: %v\n", lerr) + exitWithCode(askExitAgentNotAllowed) + return nil + } + if err := launcher.VerifyInstalled(); err != nil { + fmt.Fprintf(cmd.ErrOrStderr(), "canopy ask: %v\n", err) + exitWithCode(askExitBinaryNotInstalled) + return nil + } + if _, err := launcher.ResolveExec(); err != nil { + if errors.Is(err, agent.ErrLauncherNoExec) { + fmt.Fprintf(cmd.ErrOrStderr(), + "canopy ask: launcher %q has no one-shot mode wired up yet.\n", + agentName) + exitWithCode(askExitLauncherNoExec) + return nil + } + return err + } + // All gates passed — safe to mutate canopy.json now. + if err := config.AddAgentToCanopyJSON(mgr.Cfg.ProjectRoot, agentName); err != nil { + fmt.Fprintf(cmd.ErrOrStderr(), "canopy ask: auto-add %q to canopy.json failed: %v\n", agentName, err) + exitWithCode(askExitGeneric) + return nil + } + // Re-load so the rest of this function sees the updated + // allowlist (not strictly required since we don't gate + // again, but keeps state consistent for any future + // downstream check). + updated, err := config.LoadFrom(mgr.Cfg.ProjectRoot) + if err == nil { + mgr.Cfg = updated + } + } + + // Resolve the launcher + its exec mode. + launcher, err := agent.Resolve(agentName) + if err != nil { + return err + } + execMode, err := launcher.ResolveExec() + if err != nil { + if errors.Is(err, agent.ErrLauncherNoExec) { + fmt.Fprintf(cmd.ErrOrStderr(), + "canopy ask: launcher %q has no one-shot mode wired up yet.\n", + agentName) + exitWithCode(askExitLauncherNoExec) + return nil + } + return err + } + + // Verify the binary is on PATH before spawning. Without + // this, exec.CommandContext below would surface a + // cryptic os/exec error; VerifyInstalled gives the + // canonical install hint. + if err := launcher.VerifyInstalled(); err != nil { + fmt.Fprintf(cmd.ErrOrStderr(), "canopy ask: %v\n", err) + exitWithCode(askExitBinaryNotInstalled) + return nil + } + + // Locate workspace via cwd-walk-up. Same machinery as + // canopy agent swap. The workspace gives us name + path + // + branch to render the context prefix. + cwd, _ := os.Getwd() + wsName, err := findWorkspaceFromCwd(ctx, mgr, cwd) + if err != nil { + // Soft-fail: if we're not inside a workspace, still + // support `canopy ask` with a less-rich prefix. + // (Open Q #3 in the design — for v1 we just emit a + // degraded prefix and proceed.) + wsName = "" + } + + // Build the assembled prompt. Four canopy-generated fields + // + the user's question. Premise 5. + assembled := buildAskPrefix(mgr, wsName, cwd) + "\n\n---\n\n" + question + + // Run the agent's exec mode under a timeout context. + ctxRun, cancel := context.WithTimeout(ctx, askFlags.timeout) + defer cancel() + + return runAskExec(ctxRun, launcher, execMode, assembled, mgr, + cmd.OutOrStdout(), cmd.ErrOrStderr()) + }, + } + cmd.Flags().StringVar(&askFlags.file, "file", "", + "read question body from (used by the TUI popup)") + cmd.Flags().BoolVar(&askFlags.stdin, "stdin", false, + "read question body from stdin (use with `echo ... | canopy ask --stdin`)") + cmd.Flags().DurationVar(&askFlags.timeout, "timeout", 60*time.Second, + "per-invocation timeout (default 60s)") + return cmd +} + +// loadAskQuestion picks exactly one input mode (positional, --file, +// --stdin) and returns the resulting question body. Mixing modes is a +// clean error — the user almost certainly didn't intend it and we'd +// rather fail loud than silently pick one. +func loadAskQuestion(positional []string, file string, useStdin bool, stdin io.Reader) (string, error) { + modes := 0 + if len(positional) > 0 { + modes++ + } + if file != "" { + modes++ + } + if useStdin { + modes++ + } + switch modes { + case 0: + return "", fmt.Errorf("canopy ask: provide a question (positional, --file , or --stdin)") + case 1: + // fall through + default: + return "", fmt.Errorf("canopy ask: provide exactly one of , --file, --stdin (got %d)", modes) + } + + switch { + case len(positional) > 0: + return strings.Join(positional, " "), nil + case file != "": + data, err := os.ReadFile(file) + if err != nil { + return "", fmt.Errorf("canopy ask: read --file %s: %w", file, err) + } + return string(data), nil + case useStdin: + data, err := io.ReadAll(stdin) + if err != nil { + return "", fmt.Errorf("canopy ask: read stdin: %w", err) + } + return string(data), nil + } + return "", fmt.Errorf("canopy ask: unreachable input-mode branch") +} + +// buildAskPrefix assembles the four-field canopy-generated context +// prefix. Premise 5 (D3 of office-hours): no primary-agent pane +// capture; only fixed-shape canopy data so prompt-injection surface +// stays minimal. +// +// wsName empty (caller wasn't inside a workspace) yields a degraded +// prefix that still names the project + cwd; that's the soft-fall +// behavior described in design Open Q #3. +func buildAskPrefix(mgr *workspace.Manager, wsName, cwd string) string { + var b strings.Builder + if wsName != "" { + fmt.Fprintf(&b, "You are being asked a quick question by a user in canopy workspace %q on branch %q.\n", + wsName, branchHint(cwd)) + } else { + fmt.Fprintf(&b, "You are being asked a quick question by a user in canopy project %q.\n", + mgr.Cfg.Project) + } + fmt.Fprintf(&b, "Working directory: %s\n", cwd) + fmt.Fprintf(&b, "Repo root: %s\n", mgr.Cfg.ProjectRoot) + b.WriteString("\nThe user's question follows.") + return b.String() +} + +// branchHint is a best-effort branch-name pull from `git symbolic-ref`. +// Returns empty string on any failure (detached HEAD, non-git dir, +// etc.) — the prefix renders "branch %q" with empty value, which is +// acceptable for the soft-fall path. +func branchHint(cwd string) string { + cmd := exec.Command("git", "-C", cwd, "symbolic-ref", "--short", "HEAD") + out, err := cmd.Output() + if err != nil { + return "" + } + return strings.TrimSpace(string(out)) +} + +// runAskExec dispatches the launcher's exec command with the assembled +// prompt + the canopy env subset, streams stdout/stderr, and maps +// timeout / generic failure into the exit-code contract. +func runAskExec( + ctx context.Context, + launcher agent.Launcher, + execMode *agent.ExecMode, + prompt string, + mgr *workspace.Manager, + stdout, stderr io.Writer, +) error { + // Build argv: [Cmd, Args..., (prompt as positional OR via stdin)] + argv := append([]string{launcher.Cmd}, execMode.Args...) + if execMode.PromptMode == agent.PromptArg { + argv = append(argv, prompt) + } + cmd := exec.CommandContext(ctx, argv[0], argv[1:]...) + cmd.Stdout = stdout + cmd.Stderr = stderr + cmd.Env = append(os.Environ(), + "CANOPY_WORKSPACE_PATH="+os.Getenv("CANOPY_WORKSPACE_PATH"), // pass-through if set + "CANOPY_ROOT_PATH="+mgr.Cfg.ProjectRoot, + "CANOPY_PORT="+os.Getenv("CANOPY_PORT"), + ) + if execMode.PromptMode == agent.PromptStdin { + cmd.Stdin = strings.NewReader(prompt) + } + + if err := cmd.Run(); err != nil { + // Distinguish timeout from generic error via the context. + if errors.Is(ctx.Err(), context.DeadlineExceeded) { + fmt.Fprintf(stderr, "canopy ask: timed out after %s\n", askFlags.timeout) + exitWithCode(askExitTimeout) + return nil + } + // Surface the child's exit code if possible; otherwise generic. + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + os.Exit(askExitGeneric) + } + return err + } + return nil +} + +// exitWithCode flushes any pending writes and calls os.Exit. Wrapper +// so tests can stub it (var exitWithCode = os.Exit) without touching +// every call site. +var exitWithCode = func(code int) { + os.Exit(code) +} + +// sweepAskTempFiles deletes stale `~/.canopy/tmp/ask-*.md` files older +// than 1 hour. Called from main.go init() once per CLI invocation; a +// backstop for the rare case where the TUI itself is SIGKILL'd before +// its defer-removed the temp file. v0.22. +func sweepAskTempFiles() { + home, err := os.UserHomeDir() + if err != nil { + return + } + tmpDir := filepath.Join(home, ".canopy", "tmp") + entries, err := os.ReadDir(tmpDir) + if err != nil { + return + } + cutoff := time.Now().Add(-1 * time.Hour) + for _, e := range entries { + if !strings.HasPrefix(e.Name(), "ask-") || !strings.HasSuffix(e.Name(), ".md") { + continue + } + info, err := e.Info() + if err != nil { + continue + } + if info.ModTime().Before(cutoff) { + _ = os.Remove(filepath.Join(tmpDir, e.Name())) + } + } +} diff --git a/cmd/canopy/ask_test.go b/cmd/canopy/ask_test.go new file mode 100644 index 0000000..ab73673 --- /dev/null +++ b/cmd/canopy/ask_test.go @@ -0,0 +1,108 @@ +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +// TestLoadAskQuestion_ExactlyOneMode covers the input-mode contract: +// positional, --file, or --stdin — exactly one. Zero modes errors; +// two or more modes errors. The "exactly one" guard prevents silent +// intent loss when a user accidentally combines flags. +func TestLoadAskQuestion_ExactlyOneMode(t *testing.T) { + tmp := t.TempDir() + file := filepath.Join(tmp, "q.md") + if err := os.WriteFile(file, []byte("from a file"), 0o644); err != nil { + t.Fatalf("write fixture: %v", err) + } + + cases := []struct { + name string + positional []string + file string + stdin bool + stdinBody string + wantQ string + wantErr bool + }{ + {name: "no modes errors", wantErr: true}, + {name: "positional alone", positional: []string{"hello", "world"}, wantQ: "hello world"}, + {name: "file alone", file: file, wantQ: "from a file"}, + {name: "stdin alone", stdin: true, stdinBody: "from stdin", wantQ: "from stdin"}, + {name: "positional+file errors", positional: []string{"a"}, file: file, wantErr: true}, + {name: "positional+stdin errors", positional: []string{"a"}, stdin: true, wantErr: true}, + {name: "file+stdin errors", file: file, stdin: true, wantErr: true}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got, err := loadAskQuestion(tc.positional, tc.file, tc.stdin, strings.NewReader(tc.stdinBody)) + if tc.wantErr { + if err == nil { + t.Errorf("want error; got nil (q=%q)", got) + } + return + } + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != tc.wantQ { + t.Errorf("got %q; want %q", got, tc.wantQ) + } + }) + } +} + +// TestSweepAskTempFiles_DeletesOldKeepsRecent: the startup sweep must +// remove stale leak files (>1h) while leaving recent ones alone. Pins +// the contract used by `cmd/canopy/main.go init()` and the popup's +// defer cleanup story. +func TestSweepAskTempFiles_DeletesOldKeepsRecent(t *testing.T) { + // Stash $HOME so the sweep operates on a test tmpdir, not the + // real ~/.canopy. + home := t.TempDir() + t.Setenv("HOME", home) + tmpDir := filepath.Join(home, ".canopy", "tmp") + if err := os.MkdirAll(tmpDir, 0o755); err != nil { + t.Fatalf("mkdir tmpDir: %v", err) + } + + old := filepath.Join(tmpDir, "ask-old.md") + recent := filepath.Join(tmpDir, "ask-recent.md") + other := filepath.Join(tmpDir, "agent-briefing-xyz.md") // not ask-* + for _, p := range []string{old, recent, other} { + if err := os.WriteFile(p, []byte("body"), 0o644); err != nil { + t.Fatalf("write %s: %v", p, err) + } + } + // Backdate old to 2h ago. + past := time.Now().Add(-2 * time.Hour) + if err := os.Chtimes(old, past, past); err != nil { + t.Fatalf("chtimes old: %v", err) + } + + sweepAskTempFiles() + + if _, err := os.Stat(old); !os.IsNotExist(err) { + t.Errorf("old file still exists (err=%v); want removed", err) + } + if _, err := os.Stat(recent); err != nil { + t.Errorf("recent file got removed: %v", err) + } + if _, err := os.Stat(other); err != nil { + t.Errorf("non-ask file got removed: %v (sweep should only touch ask-*.md)", err) + } +} + +// TestBranchHint_NonGitDirReturnsEmpty: branchHint should not surface +// git errors — it's a soft-fall that yields "" when git can't read +// the branch (detached HEAD, no git, not a repo). +func TestBranchHint_NonGitDirReturnsEmpty(t *testing.T) { + tmp := t.TempDir() // bare temp dir, no .git + got := branchHint(tmp) + if got != "" { + t.Errorf("branchHint(non-git dir) = %q; want \"\"", got) + } +} diff --git a/cmd/canopy/main.go b/cmd/canopy/main.go index a2b1e07..9cef773 100644 --- a/cmd/canopy/main.go +++ b/cmd/canopy/main.go @@ -129,6 +129,14 @@ func main() { root.AddCommand(projectCmd()) // v0.17.0 Phase 1a: project-on-host registry root.AddCommand(configCmd()) // v0.20: user-level config (~/.canopy/config.json) root.AddCommand(newClipboardServerCmd()) // v0.21: clipboard-bridge daemon subcommand + root.AddCommand(agentCmd()) // v0.22: agent swap + reserved noun-space + root.AddCommand(askCmd()) // v0.22: `canopy ask ` second-opinion verb + + // v0.22: sweep stale ~/.canopy/tmp/ask-*.md files older than 1 hour. + // Backstop for the popup's defer-removed temp file in case the TUI + // was SIGKILL'd before its cleanup ran. Cheap; runs once per CLI + // invocation; touches at most a handful of files in a known dir. + sweepAskTempFiles() if err := root.Execute(); err != nil { // Distinguish "workspace OK, prompt failed" (exit 2) from diff --git a/cmd/canopy/new.go b/cmd/canopy/new.go index 5deb80b..b9b9dde 100644 --- a/cmd/canopy/new.go +++ b/cmd/canopy/new.go @@ -11,6 +11,7 @@ import ( "github.com/spf13/cobra" + "github.com/avinashjoshi/canopy/internal/agent" "github.com/avinashjoshi/canopy/internal/host" "github.com/avinashjoshi/canopy/internal/workspace" ) @@ -31,6 +32,7 @@ var newWorkspaceFlags struct { promptFile string // --prompt-file: read --prompt content from file (multi-line) onHost string // --on : dispatch to remote canopy (v0.17.0 Phase 0) remoteCwd string // --remote-cwd : cwd on the remote before running canopy (Phase 0) + agent string // --agent : launcher to spawn (v0.22) } // newCmd returns the `canopy new` cobra subcommand. @@ -104,6 +106,22 @@ func newCmd() *cobra.Command { if err != nil { return err } + // v0.22: --agent validation against canopy.json's + // `agents:` allowlist. Empty flag value → use the project's + // default (Cfg.Agents[0]); validation skipped because the + // default is, by definition, allowed. ErrAgentNotAllowed is + // fatal here, BEFORE any side effect (port allocation, git + // worktree, scripts.setup) — the same shape as `canopy + // agent swap`'s gate. + if newWorkspaceFlags.agent != "" { + if !mgr.Cfg.AllowsAgent(newWorkspaceFlags.agent) { + return fmt.Errorf("%w: %q (allowed: %v)", + agent.ErrAgentNotAllowed, + newWorkspaceFlags.agent, + mgr.Cfg.Agents) + } + opts.Agent = newWorkspaceFlags.agent + } // Pick the workspace name. Explicit --name beats the // source-derived suggestion, which beats namegen (the // empty string case, handled inside Manager.Create). @@ -202,6 +220,8 @@ func newCmd() *cobra.Command { "dispatch to remote canopy at instead of running locally (v0.17.0 Phase 0)") cmd.Flags().StringVar(&newWorkspaceFlags.remoteCwd, "remote-cwd", "", "with --on: cd to on the remote before invoking canopy (Phase 0; Phase 1 absorbs into hosts.json project registry)") + cmd.Flags().StringVar(&newWorkspaceFlags.agent, "agent", "", + "launcher to spawn for this workspace (must be in canopy.json's `agents:` allowlist; default = agents[0])") return cmd } @@ -245,6 +265,12 @@ func dispatchNewToRemote(ctx context.Context, resolved resolvedHost, posArgs []s if newWorkspaceFlags.allowLoc { canopyArgs = append(canopyArgs, "--allow-local") } + // Forward --agent so the remote canopy creates the workspace with the + // requested launcher instead of falling back to the project's default. + // (codex review P2 #6, 2026-06-25.) + if newWorkspaceFlags.agent != "" { + canopyArgs = append(canopyArgs, "--agent", newWorkspaceFlags.agent) + } // Pass through any positional args (cobra collects unparsed; in // practice `canopy new` takes none today but future-proof the call). canopyArgs = append(canopyArgs, posArgs...) diff --git a/internal/ui/keymap.go b/internal/ui/keymap.go index ee94898..e495b2f 100644 --- a/internal/ui/keymap.go +++ b/internal/ui/keymap.go @@ -247,6 +247,32 @@ var listModeBindings = []Binding{ Available: availableInWorkspaceContext, Action: actionKill, }, + { + // A (capital) opens the agent-swap picker for the cursor + // workspace (v0.22). Lower-case a is "add project" / + // "host auth" depending on tab — the capital-key convention + // matches K (kill) and P (PR): destructive or significant + // state changes require a deliberate keypress. + // availableAgentSwap hides the binding when the row can't + // support a swap (Main rows, remote rows, projects with + // empty `agents:` allowlist). + K: key.NewBinding(key.WithKeys("A"), key.WithHelp("A", "swap agent")), + Group: "act", + Available: availableAgentSwap, + Action: actionAgentSwap, + }, + { + // Q opens the v0.22 "quick second opinion" popup. The popup + // itself runs `canopy ask --file ` inside a + // tmux display-popup; the TUI underneath stays present and + // returns when the popup is dismissed. Capital Q matches the + // same deliberate-keypress convention as K/P/B/A (lowercase + // q is quit). + K: key.NewBinding(key.WithKeys("Q"), key.WithHelp("Q", "ask agent")), + Group: "act", + Available: availableAsk, + Action: actionAsk, + }, { // `i` opens the diagnostic detail drawer for the selected // workspace. Read-only; scope-capped to "what's the state diff --git a/internal/ui/model.go b/internal/ui/model.go index 03f856b..873ef08 100644 --- a/internal/ui/model.go +++ b/internal/ui/model.go @@ -182,6 +182,23 @@ const ( // Manager.SetOwner for local rows or `canopy set-owner --on ` // for remote rows. v0.22 distinguish-my-workspaces. ownerFormMode + // agentSwapPickerMode is the v0.22 picker that opens on `A` from + // the workspaces tab. Lists the cursor row's project canopy.json + // `agents:` allowlist; arrow-nav + Enter dispatches into + // Manager.SwapAgent. Esc cancels back to listMode. agentSwapBusy + // flips true while the swap is in flight; the view renders + // "Swapping..." then the result message before auto-returning + // to listMode. See internal/ui/update_agent_swap.go. + agentSwapPickerMode + // askPickerMode + askInputMode are the v0.22 "quick second opinion" + // flow. `Q` opens askPickerMode → user picks the target agent → + // askInputMode (textarea for the question) → Ctrl+S submits → the + // TUI writes the question to ~/.canopy/tmp/ask-*.md and spawns + // `canopy ask --file ` inside a tmux display-popup. + // The answer renders in the popup; when the popup closes the TUI + // returns to listMode. See internal/ui/update_ask.go. + askPickerMode + askInputMode ) // inNewFlow reports whether the current mode is any step of the @@ -331,6 +348,29 @@ type Model struct { deleteTargetRoot string deleteHangs []string // v0.6 safety check results — populated when 'd' is pressed; non-empty triggers the force-required path in renderConfirmDelete + handleConfirmDeleteKey + // Agent-swap picker (mode == agentSwapPickerMode). Snapshotted at + // modal-open time so a refresh between open and Enter doesn't + // re-roll the (workspace, project, agent list) the user saw. + // Same scoping rationale as deleteTarget + deleteTargetRoot. + agentSwapTarget string // workspace name + agentSwapTargetRoot string // workspace's ProjectRoot + agentSwapCurrent string // workspace's current agent at open time (dimmed in picker) + agentSwapList []string // snapshot of Cfg.Agents at open time + agentSwapCursor int // index into agentSwapList + agentSwapBusy bool // SwapAgent call in flight; suppress further keypresses + render "Swapping..." + agentSwapResult string // post-swap message ("Swapped to codex." or error); shown until any key returns to listMode + + // Ask (second-opinion popup) state. Same snapshot rationale as + // the swap picker: row context captured at open time. Spans two + // modes (picker → input) so we share the state between them. + askTarget string // workspace name (for prefix) + askTargetRoot string // workspace's ProjectRoot + askList []string // snapshot of Cfg.Agents at open time + askCursor int // index into askList during picker + askAgent string // chosen agent after picker → input transition + askInput textarea.Model // multi-line question textarea (Ctrl+S submits) + askErr string // last error from temp-file write or popup spawn + // attachTarget snapshots the row the user pressed Enter on when its // session already has another client connected. confirmAttachMode // reads it to render the "already attached" prompt; y/Enter proceeds diff --git a/internal/ui/update.go b/internal/ui/update.go index 41356ae..871088d 100644 --- a/internal/ui/update.go +++ b/internal/ui/update.go @@ -360,6 +360,12 @@ func (m *Model) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } return m, progressTickCmd(msg.buf) + case agentSwapDoneMsg: + return m.handleAgentSwapDone(msg) + + case askDoneMsg: + return m.handleAskDone(msg) + case createDoneMsg: // Workspace creation finished. On success we auto-attach to the // new workspace's tmux session — that's what the user pressed `n` @@ -688,6 +694,12 @@ func (m *Model) handleKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m.handleUpgradeKey(msg) case hostUpgradeMode: return m.handleHostUpgradeKey(msg) + case agentSwapPickerMode: + return m.handleAgentSwapPickerKey(msg) + case askPickerMode: + return m.handleAskPickerKey(msg) + case askInputMode: + return m.handleAskInputKey(msg) } // Search-mode keystrokes: capture into searchQuery, refilter on each diff --git a/internal/ui/update_agent_swap.go b/internal/ui/update_agent_swap.go new file mode 100644 index 0000000..76b9b05 --- /dev/null +++ b/internal/ui/update_agent_swap.go @@ -0,0 +1,329 @@ +// Agent-swap picker (v0.22). `A` from the workspaces tab opens a list +// of the cursor row's project canopy.json `agents:` allowlist. Arrow +// nav + Enter dispatches into Manager.SwapAgent; Esc cancels back to +// listMode. +// +// Why a separate file (vs folding into update.go): same separation-by- +// flow pattern as update_new.go, update_delete.go, update_kill.go etc. +// The model fields live in model.go alongside the existing modal state; +// the action / handler / dispatch / view all live here. One mental +// model per file. +// +// Out-of-scope failure modes the picker INTENTIONALLY doesn't handle: +// +// - Remote rows (row.Host != ""). canopy agent swap doesn't yet +// dispatch over SSH; the action predicate hides the binding for +// remote rows so the user never sees a binding they can't use. +// +// - The Main row. Each project's main session has no agent pane — +// swap is workspace-scoped. Predicate hides too. +// +// - Cross-project Managers via managerForRow. Same machinery as +// the delete flow; no special-case needed here. + +package ui + +import ( + "context" + "errors" + "fmt" + "strings" + + tea "github.com/charmbracelet/bubbletea" + + "github.com/avinashjoshi/canopy/internal/agent" + "github.com/avinashjoshi/canopy/internal/config" + "github.com/avinashjoshi/canopy/internal/workspace" +) + +// availableAgentSwap gates the `A` keybind. Hidden unless: +// +// - we're on the workspace tab (not Hosts), +// - the cursor is on a real workspace row (not Main, not Loading), +// - the row is LOCAL (no Host — remote swap is a future feature), +// - at least one launcher is INSTALLED on PATH. +// +// We don't gate on canopy.json's agents allowlist anymore (D6=A): +// any installed launcher shows in the picker; picking one outside +// the allowlist auto-adds it to canopy.json. The allowlist is now a +// "remember what's been used" set rather than a "gate the picker" +// set. +func availableAgentSwap(m *Model) bool { + if m.tab == tabHosts { + return false + } + row, ok := m.list.CursorRow() + if !ok || row.Loading || row.IsMain || row.Host != "" { + return false + } + return len(agent.InstalledLaunchers()) > 0 +} + +// actionAgentSwap opens the agent-swap picker. Snapshots the +// installed-launchers list + current agent at open time so subsequent +// refresh ticks (which might re-read state.json) can't reshuffle the +// picker mid-decision. Picking a launcher outside canopy.json's +// `agents:` allowlist auto-adds it (D6=A); the picker doesn't pre- +// filter to allowed-only. +func actionAgentSwap(m *Model, _ tea.KeyMsg) (tea.Model, tea.Cmd) { + row, ok := m.list.CursorRow() + if !ok || row.Loading { + return m, nil + } + installed := agent.InstalledLaunchers() + if len(installed) == 0 { + m.err = fmt.Errorf("no agent launchers installed on PATH (claude / codex / aider / opencode)") + return m, nil + } + m.mode = agentSwapPickerMode + m.agentSwapTarget = row.Name + m.agentSwapTargetRoot = row.ProjectRoot + m.agentSwapCurrent = currentAgentOnRow(row) + m.agentSwapList = installed + m.agentSwapBusy = false + m.agentSwapResult = "" + + // Initial cursor: first agent in the list that is NOT the row's + // current agent. Asking the picker to land on the agent you're + // already running would be a mis-press magnet (Enter would no-op + // via ErrSwapAlreadyCurrent). + m.agentSwapCursor = 0 + for i, a := range m.agentSwapList { + if a != m.agentSwapCurrent { + m.agentSwapCursor = i + break + } + } + return m, nil +} + +// currentAgentOnRow extracts the row's current agent from +// state.GlobalRow. This UI knows about agents through the row data +// (state.BuildGlobalRows populates it from state.Workspace.CurrentAgent). +// Empty string is a defensive fallback for pre-v0.22 rows that haven't +// been migrated yet — the picker treats them as "no current," which +// means no entry gets dimmed. +func currentAgentOnRow(row Row) string { + return row.CurrentAgent +} + +// handleAgentSwapPickerKey is the keymap while the picker is open. +// +// Three states the handler distinguishes: +// +// - Busy (agentSwapBusy true): SwapAgent is in flight. The picker +// ignores keypresses except ctrl+c (which quits canopy entirely; +// can't safely cancel a partial swap mid-flight without leaving +// the tmux session in a half-swapped state). +// +// - Result shown (agentSwapResult non-empty): SwapAgent completed +// and the picker is rendering "Swapped to X." or an error. Any +// keypress dismisses back to listMode and clears the snapshot. +// +// - Picker (default): arrow nav + Enter dispatches; Esc / q cancels. +// +// q is suppressed in the picker proper so a stray q press doesn't +// accidentally quit canopy from inside a modal. ctrl+c is the +// always-available escape hatch. +func (m *Model) handleAgentSwapPickerKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + if m.agentSwapBusy { + if msg.String() == "ctrl+c" { + return m, tea.Quit + } + return m, nil + } + if m.agentSwapResult != "" { + // Any keypress dismisses the result. + m.mode = listMode + m.clearAgentSwapState() + return m, nil + } + + switch msg.String() { + case "esc": + m.mode = listMode + m.clearAgentSwapState() + return m, nil + case "ctrl+c": + return m, tea.Quit + case "up", "k": + if m.agentSwapCursor > 0 { + m.agentSwapCursor-- + } + return m, nil + case "down", "j": + if m.agentSwapCursor < len(m.agentSwapList)-1 { + m.agentSwapCursor++ + } + return m, nil + case "enter": + if m.agentSwapCursor < 0 || m.agentSwapCursor >= len(m.agentSwapList) { + return m, nil + } + target := m.agentSwapList[m.agentSwapCursor] + if target == m.agentSwapCurrent { + // Same-agent shortcut: don't bother dispatching, render the + // "already running" message immediately. + m.agentSwapResult = fmt.Sprintf("Already running %s; nothing to swap.", target) + return m, nil + } + // Resolve manager + kick off the swap as a tea.Cmd. Set busy + // so the UI shows "Swapping..." until the command completes. + // D6=A: if the chosen agent isn't in the project's allowlist, + // the cmd auto-adds it to canopy.json + re-Loads the Manager + // before the swap fires. The user just picked it from the + // installed-launchers list; that pick IS the consent. + mgr, err := m.resolveAgentSwapManager() + if err != nil { + m.agentSwapResult = "Couldn't resolve workspace: " + err.Error() + return m, nil + } + m.agentSwapBusy = true + return m, swapAgentCmd(mgr, m.agentSwapTarget, target) + } + return m, nil +} + +// resolveAgentSwapManager re-resolves the target workspace's Manager +// from the snapshotted (name, ProjectRoot) pair. Same shape as the +// delete flow's resolveTargetMgr — survives row reordering between +// open and Enter. +func (m *Model) resolveAgentSwapManager() (*workspace.Manager, error) { + for _, r := range m.filteredRows() { + if r.Name != m.agentSwapTarget { + continue + } + if m.agentSwapTargetRoot != "" && r.ProjectRoot != m.agentSwapTargetRoot { + continue + } + return m.managerForRow(r) + } + return nil, fmt.Errorf("workspace %q (root %q) is no longer in the row list", m.agentSwapTarget, m.agentSwapTargetRoot) +} + +// clearAgentSwapState resets the picker fields after dismiss. Mirrors +// clearNewTarget's pattern — explicit zero-value reset instead of +// relying on every keypress to leave fields in a sane state. +func (m *Model) clearAgentSwapState() { + m.agentSwapTarget = "" + m.agentSwapTargetRoot = "" + m.agentSwapCurrent = "" + m.agentSwapList = nil + m.agentSwapCursor = 0 + m.agentSwapBusy = false + m.agentSwapResult = "" +} + +// agentSwapDoneMsg is the tea.Msg posted by swapAgentCmd when the swap +// command finishes. err is nil on success; populated with the wrapped +// error chain on failure (including ErrAgentNotAllowed, +// ErrSwapAlreadyCurrent, etc.). newAgent echoes the target so the +// success message can render without re-reading state. +type agentSwapDoneMsg struct { + newAgent string + err error +} + +// swapAgentCmd dispatches Manager.SwapAgent and posts an +// agentSwapDoneMsg back to the Bubbletea event loop. Pattern matches +// the existing tea.Cmd dispatchers in update_new.go (createDoneCmd +// shape — kick off the long op in a goroutine, post the result message +// when done so the Update loop can transition state). +// +// D6=A: if the chosen agent isn't in canopy.json's `agents:` allowlist, +// auto-add it to canopy.json + re-load the Manager's Cfg before the +// swap fires. The user's pick from the installed-launchers picker IS +// the explicit consent. Writes to canopy.json are atomic + preserve +// unknown keys (see config.AddAgentToCanopyJSON). +func swapAgentCmd(mgr *workspace.Manager, wsName, newAgent string) tea.Cmd { + return func() tea.Msg { + if !mgr.Cfg.AllowsAgent(newAgent) { + if err := config.AddAgentToCanopyJSON(mgr.Cfg.ProjectRoot, newAgent); err != nil { + return agentSwapDoneMsg{newAgent: newAgent, err: fmt.Errorf("auto-add agent to canopy.json: %w", err)} + } + // Re-load the config so SwapAgent's AllowsAgent check passes. + // We mutate the existing Manager's Cfg in place rather than + // constructing a new Manager — same project, same paths. + updated, err := config.LoadFrom(mgr.Cfg.ProjectRoot) + if err != nil { + return agentSwapDoneMsg{newAgent: newAgent, err: fmt.Errorf("re-load canopy.json after auto-add: %w", err)} + } + mgr.Cfg = updated + } + _, err := mgr.SwapAgent(context.Background(), wsName, newAgent) + return agentSwapDoneMsg{newAgent: newAgent, err: err} + } +} + +// handleAgentSwapDone applies the agentSwapDoneMsg result: flips +// agentSwapBusy off and stages a result string for render. Called from +// update.go's main Update switch alongside the other tea.Msg cases. +func (m *Model) handleAgentSwapDone(msg agentSwapDoneMsg) (tea.Model, tea.Cmd) { + m.agentSwapBusy = false + if msg.err != nil { + // Tag known sentinels with a friendly prefix so the user doesn't + // have to parse the wrapped error chain in their head. + switch { + case errors.Is(msg.err, agent.ErrAgentNotAllowed): + m.agentSwapResult = fmt.Sprintf( + "%s is not in this project's agents allowlist. Edit canopy.json to add it.", + msg.newAgent) + case errors.Is(msg.err, workspace.ErrSwapAlreadyCurrent): + m.agentSwapResult = fmt.Sprintf("Already running %s; nothing to swap.", msg.newAgent) + case errors.Is(msg.err, workspace.ErrSwapNoAgentPane): + m.agentSwapResult = "No agent pane in this workspace's tmux session — run canopy switch first to resurrect it." + default: + m.agentSwapResult = "Swap failed: " + msg.err.Error() + } + return m, nil + } + m.agentSwapResult = fmt.Sprintf("Swapped to %s. Press any key.", msg.newAgent) + return m, nil +} + +// renderAgentSwapPicker draws the modal. Three render states match the +// keymap's three handler states: +// +// 1. Busy → spinner-ish "Swapping..." line, no list interactivity +// 2. Result shown → final message + "press any key" +// 3. Picker → list of agents with cursor + current-agent dim hint +// +// No fancy lipgloss styling — matches the existing confirm-modal density +// (renderConfirmDelete etc) to stay visually consistent with the rest +// of the modal family. +func (m *Model) renderAgentSwapPicker() string { + var b strings.Builder + fmt.Fprintf(&b, "\nSwap agent in workspace %q\n", m.agentSwapTarget) + if m.agentSwapCurrent != "" { + fmt.Fprintf(&b, "Currently running: %s\n\n", m.agentSwapCurrent) + } else { + b.WriteString("\n") + } + + if m.agentSwapBusy { + b.WriteString(" Swapping... (this kills the running agent pane and respawns)\n") + return b.String() + } + if m.agentSwapResult != "" { + b.WriteString(" ") + b.WriteString(m.agentSwapResult) + b.WriteString("\n\n Press any key to dismiss.\n") + return b.String() + } + + for i, a := range m.agentSwapList { + marker := " " + if i == m.agentSwapCursor { + marker = "▶ " + } + suffix := "" + if a == m.agentSwapCurrent { + suffix = " (current)" + } + fmt.Fprintf(&b, "%s%s%s\n", marker, a, suffix) + } + b.WriteString("\n") + b.WriteString(" ↑/↓ select • enter swap • esc cancel\n") + return b.String() +} + diff --git a/internal/ui/update_agent_swap_test.go b/internal/ui/update_agent_swap_test.go new file mode 100644 index 0000000..c6ddb23 --- /dev/null +++ b/internal/ui/update_agent_swap_test.go @@ -0,0 +1,246 @@ +package ui + +import ( + "errors" + "strings" + "testing" + + tea "github.com/charmbracelet/bubbletea" + + "github.com/avinashjoshi/canopy/internal/agent" + "github.com/avinashjoshi/canopy/internal/workspace" +) + +// TestHandleAgentSwapDone_SuccessRendersAndUnbusy: a clean swap clears +// busy state and stages a success message that mentions the new agent. +func TestHandleAgentSwapDone_SuccessRendersAndUnbusy(t *testing.T) { + m := &Model{mode: agentSwapPickerMode, agentSwapBusy: true} + _, _ = m.handleAgentSwapDone(agentSwapDoneMsg{newAgent: "codex", err: nil}) + if m.agentSwapBusy { + t.Error("agentSwapBusy still true after success") + } + if !strings.Contains(m.agentSwapResult, "codex") { + t.Errorf("agentSwapResult = %q; want it to mention 'codex'", m.agentSwapResult) + } +} + +// TestHandleAgentSwapDone_SentinelErrors maps each known sentinel to a +// friendly message prefix. Catches the case where a new sentinel gets +// added in the workspace package without a corresponding UI branch +// here — the default branch would render the raw error chain. +func TestHandleAgentSwapDone_SentinelErrors(t *testing.T) { + cases := []struct { + name string + err error + wantSub string + }{ + { + name: "ErrAgentNotAllowed", + err: agent.ErrAgentNotAllowed, + wantSub: "not in this project's agents allowlist", + }, + { + name: "ErrSwapAlreadyCurrent", + err: workspace.ErrSwapAlreadyCurrent, + wantSub: "Already running", + }, + { + name: "ErrSwapNoAgentPane", + err: workspace.ErrSwapNoAgentPane, + wantSub: "No agent pane", + }, + { + name: "generic error", + err: errors.New("kaboom"), + wantSub: "Swap failed:", + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := &Model{mode: agentSwapPickerMode, agentSwapBusy: true} + _, _ = m.handleAgentSwapDone(agentSwapDoneMsg{newAgent: "codex", err: tc.err}) + if m.agentSwapBusy { + t.Error("agentSwapBusy still true after err") + } + if !strings.Contains(m.agentSwapResult, tc.wantSub) { + t.Errorf("agentSwapResult = %q; want substring %q", m.agentSwapResult, tc.wantSub) + } + }) + } +} + +// TestHandleAgentSwapPickerKey_NavCursor: up/down/j/k move the cursor +// within bounds; out-of-bounds keys are no-ops. Picks the simplest +// invariant: cursor never goes negative, never exceeds list length-1. +func TestHandleAgentSwapPickerKey_NavCursor(t *testing.T) { + m := &Model{ + mode: agentSwapPickerMode, + agentSwapList: []string{"claude", "codex", "aider"}, + agentSwapCursor: 0, + } + + // up at top is a no-op + _, _ = m.handleAgentSwapPickerKey(tea.KeyMsg{Type: tea.KeyUp}) + if m.agentSwapCursor != 0 { + t.Errorf("up at top moved cursor to %d; want 0", m.agentSwapCursor) + } + + // down twice + _, _ = m.handleAgentSwapPickerKey(tea.KeyMsg{Type: tea.KeyDown}) + _, _ = m.handleAgentSwapPickerKey(tea.KeyMsg{Type: tea.KeyDown}) + if m.agentSwapCursor != 2 { + t.Errorf("down x2 cursor = %d; want 2", m.agentSwapCursor) + } + + // down at bottom is a no-op + _, _ = m.handleAgentSwapPickerKey(tea.KeyMsg{Type: tea.KeyDown}) + if m.agentSwapCursor != 2 { + t.Errorf("down at bottom moved cursor to %d; want 2", m.agentSwapCursor) + } + + // up returns toward top + _, _ = m.handleAgentSwapPickerKey(tea.KeyMsg{Type: tea.KeyUp}) + if m.agentSwapCursor != 1 { + t.Errorf("up cursor = %d; want 1", m.agentSwapCursor) + } +} + +// TestHandleAgentSwapPickerKey_EscCancels: pressing esc returns to +// listMode and clears the snapshotted state so the next open starts +// fresh. +func TestHandleAgentSwapPickerKey_EscCancels(t *testing.T) { + m := &Model{ + mode: agentSwapPickerMode, + agentSwapTarget: "foo", + agentSwapTargetRoot: "/tmp/proj", + agentSwapList: []string{"claude", "codex"}, + agentSwapCursor: 1, + } + _, _ = m.handleAgentSwapPickerKey(tea.KeyMsg{Type: tea.KeyEsc}) + if m.mode != listMode { + t.Errorf("mode = %v; want listMode", m.mode) + } + if m.agentSwapTarget != "" || m.agentSwapTargetRoot != "" || m.agentSwapList != nil { + t.Errorf("agentSwap state not cleared after esc: target=%q root=%q list=%v", + m.agentSwapTarget, m.agentSwapTargetRoot, m.agentSwapList) + } +} + +// TestHandleAgentSwapPickerKey_EnterOnSameAgentDoesntDispatch: pressing +// Enter when the highlighted agent IS the current agent surfaces an +// "already running" message inline WITHOUT dispatching SwapAgent (which +// would be a wasted call returning ErrSwapAlreadyCurrent). Catches the +// optimization wire. +func TestHandleAgentSwapPickerKey_EnterOnSameAgentDoesntDispatch(t *testing.T) { + m := &Model{ + mode: agentSwapPickerMode, + agentSwapList: []string{"claude", "codex"}, + agentSwapCursor: 0, // claude + agentSwapCurrent: "claude", + } + _, cmd := m.handleAgentSwapPickerKey(tea.KeyMsg{Type: tea.KeyEnter}) + if cmd != nil { + t.Errorf("Enter on same-agent dispatched a cmd; want nil (inline short-circuit)") + } + if !strings.Contains(m.agentSwapResult, "Already running") { + t.Errorf("agentSwapResult = %q; want 'Already running' message", m.agentSwapResult) + } +} + +// TestHandleAgentSwapPickerKey_BusyIgnoresKeys: while a swap is in +// flight, almost all keypresses are swallowed (the swap can't be +// safely cancelled mid-flight without leaving tmux in a half-swapped +// state). Only ctrl+c (quit canopy entirely) is allowed through. +func TestHandleAgentSwapPickerKey_BusyIgnoresKeys(t *testing.T) { + m := &Model{mode: agentSwapPickerMode, agentSwapBusy: true, agentSwapList: []string{"claude"}} + // Esc, j, k, enter all no-op + for _, key := range []tea.KeyMsg{ + {Type: tea.KeyEsc}, + {Type: tea.KeyDown}, + {Type: tea.KeyUp}, + {Type: tea.KeyEnter}, + } { + _, _ = m.handleAgentSwapPickerKey(key) + } + if m.mode != agentSwapPickerMode { + t.Errorf("mode = %v after busy keypresses; want agentSwapPickerMode (no exit)", m.mode) + } + // ctrl+c still works (returns tea.Quit) + _, cmd := m.handleAgentSwapPickerKey(tea.KeyMsg{Type: tea.KeyCtrlC}) + if cmd == nil { + t.Error("ctrl+c during busy returned nil cmd; want tea.Quit") + } +} + +// TestHandleAgentSwapPickerKey_ResultDismissedByAnyKey: after a swap +// completes (success or error), the picker enters "result shown" mode +// where any key dismisses back to listMode. Tests the dismiss-on-any-key +// contract specifically. +func TestHandleAgentSwapPickerKey_ResultDismissedByAnyKey(t *testing.T) { + m := &Model{ + mode: agentSwapPickerMode, + agentSwapResult: "Swapped to codex.", + agentSwapList: []string{"claude", "codex"}, + agentSwapCursor: 1, + } + _, _ = m.handleAgentSwapPickerKey(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'q'}}) + if m.mode != listMode { + t.Errorf("mode after dismiss = %v; want listMode", m.mode) + } + if m.agentSwapResult != "" { + t.Errorf("agentSwapResult not cleared after dismiss: %q", m.agentSwapResult) + } +} + +// TestRenderAgentSwapPicker_HappyPath: render the picker's three +// states and assert each one has the expected scaffolding lines. We +// don't pin exact glyphs / colors (too brittle); we check for the +// distinguishing text. +func TestRenderAgentSwapPicker_HappyPath(t *testing.T) { + cases := []struct { + name string + setup func(*Model) + wantSubs []string + }{ + { + name: "picker state lists agents + cursor + current marker", + setup: func(m *Model) { + m.agentSwapTarget = "feature-x" + m.agentSwapList = []string{"claude", "codex"} + m.agentSwapCursor = 1 + m.agentSwapCurrent = "claude" + }, + wantSubs: []string{"feature-x", "claude", "codex", "(current)", "swap", "cancel"}, + }, + { + name: "busy state shows swapping message", + setup: func(m *Model) { + m.agentSwapTarget = "feature-x" + m.agentSwapList = []string{"claude", "codex"} + m.agentSwapBusy = true + }, + wantSubs: []string{"feature-x", "Swapping"}, + }, + { + name: "result state shows message + dismiss hint", + setup: func(m *Model) { + m.agentSwapTarget = "feature-x" + m.agentSwapList = []string{"claude", "codex"} + m.agentSwapResult = "Swapped to codex. Press any key." + }, + wantSubs: []string{"Swapped to codex", "Press any key"}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := &Model{mode: agentSwapPickerMode} + tc.setup(m) + got := m.renderAgentSwapPicker() + for _, want := range tc.wantSubs { + if !strings.Contains(got, want) { + t.Errorf("renderAgentSwapPicker output missing %q\nfull output:\n%s", want, got) + } + } + }) + } +} diff --git a/internal/ui/update_ask.go b/internal/ui/update_ask.go new file mode 100644 index 0000000..15bd2ce --- /dev/null +++ b/internal/ui/update_ask.go @@ -0,0 +1,381 @@ +// Ask (second-opinion popup) flow. `Q` from the workspaces tab opens +// the picker (askPickerMode) → user selects target agent → the TUI +// transitions to askInputMode (multi-line textarea) → Ctrl+S writes +// the question to ~/.canopy/tmp/ask-.md and spawns +// `canopy ask --file ` inside a tmux display-popup. +// +// Why a tmux popup (vs Bubbletea modal): the actual answer can be 100+ +// lines and arrives over multiple seconds. The popup is its own pty, +// scrollable via the user's normal tmux key bindings, and dismissible +// with q. The TUI underneath stays present for instant return. +// +// Why temp-file vs piping stdin: tmux display-popup runs the command +// in a fresh pty inside the popup; the parent Bubbletea process's +// stdin doesn't pipe through to the popup'd command. The design doc +// reviewer caught this as a blocker; temp-file + --file plumbing is +// the fix. See ~/.gstack/projects/avinashjoshi-canopy/cassy-add-codex- +// support-concurrent-multi-agent-design-20260625-110939.md §3. + +package ui + +import ( + "context" + "crypto/rand" + "encoding/hex" + "errors" + "fmt" + "os" + "path/filepath" + "strings" + "time" + + "github.com/charmbracelet/bubbles/textarea" + tea "github.com/charmbracelet/bubbletea" + + "github.com/avinashjoshi/canopy/internal/agent" +) + +// askPromptHeight is the textarea height (rows). 12 rows fits a +// multi-line question comfortably while leaving room for the picker +// header and the "Ctrl+S to submit / Esc to cancel" footer in an +// 80%-height popup. +const askPromptHeight = 12 + +// availableAsk gates the `Q` keybind. Same conditions as the swap +// picker: workspaces tab, local row, real workspace (not Main/loading), +// at least one launcher installed. Allowlist is not a gate (D6=A). +func availableAsk(m *Model) bool { + if m.tab == tabHosts { + return false + } + row, ok := m.list.CursorRow() + if !ok || row.Loading || row.IsMain || row.Host != "" { + return false + } + return len(agent.InstalledLaunchers()) > 0 +} + +// actionAsk opens the ask picker. Snapshots the installed-launchers +// list at open time so a refresh tick can't reshuffle the picker mid- +// decision. Picking a launcher outside canopy.json's allowlist auto- +// adds it (D6=A); the picker doesn't pre-filter to allowed-only. +func actionAsk(m *Model, _ tea.KeyMsg) (tea.Model, tea.Cmd) { + row, ok := m.list.CursorRow() + if !ok || row.Loading { + return m, nil + } + installed := agent.InstalledLaunchers() + if len(installed) == 0 { + m.err = fmt.Errorf("no agent launchers installed on PATH (claude / codex / aider / opencode)") + return m, nil + } + m.mode = askPickerMode + m.askTarget = row.Name + m.askTargetRoot = row.ProjectRoot + m.askList = installed + m.askAgent = "" + m.askErr = "" + + // Cursor default: first agent that isn't the row's current. Same + // rationale as the swap picker — asking the agent you're already + // running is rarely what you wanted. + m.askCursor = 0 + for i, a := range m.askList { + if a != row.CurrentAgent { + m.askCursor = i + break + } + } + return m, nil +} + +// handleAskPickerKey is the keymap while the agent picker is up. +// Arrow nav + Enter → askInputMode. Esc cancels back to listMode. +func (m *Model) handleAskPickerKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + switch msg.String() { + case "esc": + m.mode = listMode + m.clearAskState() + return m, nil + case "ctrl+c": + return m, tea.Quit + case "up", "k": + if m.askCursor > 0 { + m.askCursor-- + } + return m, nil + case "down", "j": + if m.askCursor < len(m.askList)-1 { + m.askCursor++ + } + return m, nil + case "enter": + if m.askCursor < 0 || m.askCursor >= len(m.askList) { + return m, nil + } + m.askAgent = m.askList[m.askCursor] + m.mode = askInputMode + m.askInput = newAskTextarea() + return m, m.askInput.Focus() + } + return m, nil +} + +// newAskTextarea returns a textarea pre-configured for the question +// input stage. Multi-line, Ctrl+S to submit (handled in +// handleAskInputKey, not via textarea binding), Esc cancels. +func newAskTextarea() textarea.Model { + ta := textarea.New() + ta.Placeholder = "Type your question (Ctrl+S to submit, Esc to cancel)..." + ta.SetWidth(80) + ta.SetHeight(askPromptHeight) + ta.CharLimit = 32 * 1024 // 32KB ceiling, mirrors canopy new --prompt-file + return ta +} + +// handleAskInputKey is the keymap while the question textarea is up. +// Ctrl+S submits → dispatches the popup spawn cmd. Esc cancels back +// to listMode (loses the typed question — same as canopy new --prompt's +// esc behavior). +func (m *Model) handleAskInputKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { + switch msg.String() { + case "esc": + m.mode = listMode + m.clearAskState() + return m, nil + case "ctrl+c": + return m, tea.Quit + case "ctrl+s": + question := strings.TrimSpace(m.askInput.Value()) + if question == "" { + m.askErr = "Question is empty — type something or Esc to cancel." + return m, nil + } + // Resolve the workspace's worktree path for the popup cwd + // (codex review P1 #1: must be Path, not ProjectRoot — the + // `canopy ask` subprocess does cwd-walk-up to find its + // workspace, and we want it landing in the worktree, not + // the source checkout). + popupCwd, err := m.resolveAskCwd() + if err != nil { + m.askErr = "Couldn't resolve workspace: " + err.Error() + return m, nil + } + return m, askPopupCmd(m.askAgent, question, popupCwd, m.tc) + } + // Default: let the textarea handle the keypress (typing, nav, etc). + var cmd tea.Cmd + m.askInput, cmd = m.askInput.Update(msg) + return m, cmd +} + +// resolveAskCwd returns the cwd for the popup's `canopy ask` invocation. +// We want the WORKSPACE'S WORKTREE PATH (row.Path) — NOT the project root. +// `canopy ask` does its own cwd-walk-up to find the workspace from cwd; +// pointing it at the project root would land on the source checkout +// instead of the worktree, and the agent would answer against the +// wrong branch/files. (codex review P1 #1, 2026-06-25.) +// +// The snapshot (askTarget + askTargetRoot) was captured at open time; +// the row may have re-ordered or disappeared by submit time, so we +// re-walk filteredRows and only return when both (Name, ProjectRoot) +// still match. ProjectRoot is the (Project, Name) discriminator, NOT +// the popup cwd. +func (m *Model) resolveAskCwd() (string, error) { + for _, r := range m.filteredRows() { + if r.Name != m.askTarget { + continue + } + if m.askTargetRoot != "" && r.ProjectRoot != m.askTargetRoot { + continue + } + if r.Path == "" { + return "", fmt.Errorf("workspace %q has no Path set; cannot dispatch popup", r.Name) + } + return r.Path, nil + } + return "", fmt.Errorf("workspace %q (root %q) is no longer in the row list", m.askTarget, m.askTargetRoot) +} + +// clearAskState resets the ask fields after dismiss / popup completion. +func (m *Model) clearAskState() { + m.askTarget = "" + m.askTargetRoot = "" + m.askList = nil + m.askCursor = 0 + m.askAgent = "" + m.askErr = "" + // Don't bother resetting m.askInput — its zero value works and the + // next open re-Newzs it anyway. +} + +// askDoneMsg is posted by askPopupCmd after the tmux popup exits. +// err is nil on success; populated when temp-file write or popup spawn +// failed. successful popup exit (whether the agent answered cleanly or +// not) is success from canopy's perspective — the user saw the answer +// or error in the popup. +type askDoneMsg struct { + err error +} + +// askPopupCmd writes the question to ~/.canopy/tmp/ask-.md, +// spawns `canopy ask --file ` via tmux display-popup +// (blocks until the popup is dismissed), then deletes the temp file +// and posts an askDoneMsg. Runs in its own goroutine (Bubbletea's +// tea.Cmd contract) so the TUI event loop stays responsive. +// +// Tests on this command directly are hard — they'd need a running +// tmux server + a real canopy binary on PATH. The state-transition +// tests around it (mode flips, temp-file lifecycle from the unit +// helper below) cover what we can; the popup invocation itself is +// validated by manual smoke per the design doc's E2E step. +func askPopupCmd(agent, question, projectRoot string, tc tmuxClient) tea.Cmd { + return func() tea.Msg { + tmpPath, err := writeAskTempFile(question) + if err != nil { + return askDoneMsg{err: fmt.Errorf("ask: write temp file: %w", err)} + } + defer os.Remove(tmpPath) + + // Build the popup command. The `canopy` binary is whatever + // the user has on PATH (likely the dev symlink during testing, + // the released binary in production). Per CLAUDE.md, that's + // user state — we don't second-guess it. + // + // Wrap with `bash -c '; echo; read -p "..." _'` so the + // popup stays open after canopy ask exits. Without the wait, + // `tmux display-popup -E` closes the moment the child process + // finishes, vanishing fast answers (and error messages from + // agent-not-allowed / binary-missing / etc) before the user + // can read them. The trailing `read` blocks until the user + // presses enter or dismisses the popup with the normal tmux + // key. (codex review P1 #2, 2026-06-25.) + // + // Path safety: tmpPath lands in $HOME/.canopy/tmp/ask-.md. + // $HOME can contain spaces, `$`, or `'`, and the path then + // inherits them. We pass tmpPath as a POSITIONAL ARGUMENT to + // bash ($1) instead of interpolating it into the bash -c body — + // that way only the OUTER shell sees the raw path, and it gets + // single-quoted once via posixShellQuote. bash then expands + // "$1" with full word integrity. This dodges the nested-quoting + // problem that an unquoted `--file %s` interpolation had: a + // home like /home/Foo Bar/ split on the space. (codex review + // P2 #4, 2026-06-25.) + popupCmd := fmt.Sprintf( + `bash -c 'canopy ask %s --file "$1"; echo; echo "--- press enter to dismiss ---"; read -r _' _ %s`, + posixShellQuote(agent), posixShellQuote(tmpPath)) + + // Run the popup. tmux display-popup blocks until the wrapped + // bash process exits, which now happens only after the user + // presses enter on the dismiss prompt. + ctx := context.Background() + if err := tc.DisplayPopup(ctx, popupCmd, projectRoot); err != nil { + return askDoneMsg{err: fmt.Errorf("ask: display-popup: %w", err)} + } + return askDoneMsg{} + } +} + +// tmuxClient is the subset of *tmux.Client askPopupCmd needs. Lets us +// inject a fake in tests without spinning up a real tmux server. +type tmuxClient interface { + DisplayPopup(ctx context.Context, command, cwd string) error +} + +// writeAskTempFile creates ~/.canopy/tmp/ask-.md with the given +// body using an atomic tmpfile + rename. Returns the final path; the +// caller is responsible for os.Remove'ing it after the popup exits. +// The startup sweep (cmd/canopy/main.go init() → sweepAskTempFiles) +// is the backstop for the rare case where the caller's defer doesn't +// fire (TUI SIGKILL'd mid-popup). +func writeAskTempFile(body string) (string, error) { + home, err := os.UserHomeDir() + if err != nil { + return "", fmt.Errorf("ask: home dir: %w", err) + } + tmpDir := filepath.Join(home, ".canopy", "tmp") + if err := os.MkdirAll(tmpDir, 0o755); err != nil { + return "", fmt.Errorf("ask: mkdir %s: %w", tmpDir, err) + } + rand := newAskRandSuffix() + finalPath := filepath.Join(tmpDir, "ask-"+rand+".md") + tmpPath := finalPath + ".tmp" + if err := os.WriteFile(tmpPath, []byte(body), 0o600); err != nil { + return "", err + } + if err := os.Rename(tmpPath, finalPath); err != nil { + _ = os.Remove(tmpPath) + return "", err + } + return finalPath, nil +} + +// posixShellQuote wraps s in POSIX single quotes, escaping any +// embedded single quotes via the standard '\'' close-escape-reopen +// trick. Safe for any string — single-quoted POSIX strings have NO +// escape sequences except for that trick. +// +// Used by askPopupCmd to pass tmpPath (and the agent name) as +// positional arguments to bash. The OUTER shell that tmux runs the +// command through sees these single-quoted, preserving spaces and +// shell metacharacters that a $HOME with awkward characters might +// embed in the path. +func posixShellQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + +// newAskRandSuffix returns a short random hex string for the temp +// file's name. 12 hex chars = 48 bits of entropy — overkill for a +// per-popup unique name; the startup sweep + atomic rename are the +// load-bearing safety bits. +func newAskRandSuffix() string { + var b [6]byte + if _, err := rand.Read(b[:]); err != nil { + // Should never fail in practice (crypto/rand panics on + // /dev/urandom unavailable). Fall back to a timestamp-based + // suffix; collisions are still extremely unlikely. + return fmt.Sprintf("%d", time.Now().UnixNano()) + } + return hex.EncodeToString(b[:]) +} + +// handleAskDone applies the askDoneMsg result. Always returns to +// listMode (the popup IS the answer surface — there's nothing for the +// TUI to render afterward besides going back to the list). +func (m *Model) handleAskDone(msg askDoneMsg) (tea.Model, tea.Cmd) { + if msg.err != nil { + m.err = errors.New("ask popup failed: " + msg.err.Error()) + } + m.mode = listMode + m.clearAskState() + return m, nil +} + +// renderAskPicker draws the agent picker. Same density as the swap +// picker — simple text rows + cursor marker + footer hint. +func (m *Model) renderAskPicker() string { + var b strings.Builder + fmt.Fprintf(&b, "\nAsk an agent (quick second opinion) in workspace %q\n\n", m.askTarget) + for i, a := range m.askList { + marker := " " + if i == m.askCursor { + marker = "▶ " + } + fmt.Fprintf(&b, "%s%s\n", marker, a) + } + b.WriteString("\n ↑/↓ select • enter next • esc cancel\n") + return b.String() +} + +// renderAskInput draws the textarea + agent header + submit hint. +func (m *Model) renderAskInput() string { + var b strings.Builder + fmt.Fprintf(&b, "\nAsk %s a question (workspace %q)\n\n", m.askAgent, m.askTarget) + b.WriteString(m.askInput.View()) + b.WriteString("\n\n") + if m.askErr != "" { + b.WriteString(" " + m.askErr + "\n") + } + b.WriteString(" Ctrl+S submit • Esc cancel\n") + return b.String() +} diff --git a/internal/ui/update_ask_test.go b/internal/ui/update_ask_test.go new file mode 100644 index 0000000..216825b --- /dev/null +++ b/internal/ui/update_ask_test.go @@ -0,0 +1,325 @@ +package ui + +import ( + "context" + "errors" + "os" + "path/filepath" + "strings" + "testing" + + tea "github.com/charmbracelet/bubbletea" +) + +// TestHandleAskPickerKey_NavCursor: arrow nav stays within bounds. +func TestHandleAskPickerKey_NavCursor(t *testing.T) { + m := &Model{ + mode: askPickerMode, + askList: []string{"claude", "codex", "aider"}, + askCursor: 0, + } + _, _ = m.handleAskPickerKey(tea.KeyMsg{Type: tea.KeyUp}) + if m.askCursor != 0 { + t.Errorf("up at top moved cursor; got %d, want 0", m.askCursor) + } + _, _ = m.handleAskPickerKey(tea.KeyMsg{Type: tea.KeyDown}) + _, _ = m.handleAskPickerKey(tea.KeyMsg{Type: tea.KeyDown}) + _, _ = m.handleAskPickerKey(tea.KeyMsg{Type: tea.KeyDown}) // past end + if m.askCursor != 2 { + t.Errorf("down past end: got %d, want 2", m.askCursor) + } +} + +// TestHandleAskPickerKey_EscCancelsAndClearsState: Esc on the picker +// returns to listMode and clears snapshot state. +func TestHandleAskPickerKey_EscCancelsAndClearsState(t *testing.T) { + m := &Model{ + mode: askPickerMode, + askTarget: "foo", + askTargetRoot: "/tmp/proj", + askList: []string{"claude"}, + askCursor: 0, + } + _, _ = m.handleAskPickerKey(tea.KeyMsg{Type: tea.KeyEsc}) + if m.mode != listMode { + t.Errorf("mode = %v; want listMode", m.mode) + } + if m.askTarget != "" || m.askTargetRoot != "" || m.askList != nil { + t.Errorf("ask state not cleared: target=%q root=%q list=%v", + m.askTarget, m.askTargetRoot, m.askList) + } +} + +// TestHandleAskPickerKey_EnterTransitionsToInput: pressing Enter on +// the picker captures the chosen agent and transitions to input mode +// with a focused textarea. +func TestHandleAskPickerKey_EnterTransitionsToInput(t *testing.T) { + m := &Model{ + mode: askPickerMode, + askList: []string{"claude", "codex"}, + askCursor: 1, // codex + } + _, cmd := m.handleAskPickerKey(tea.KeyMsg{Type: tea.KeyEnter}) + if m.mode != askInputMode { + t.Errorf("mode = %v; want askInputMode", m.mode) + } + if m.askAgent != "codex" { + t.Errorf("askAgent = %q; want codex", m.askAgent) + } + if cmd == nil { + t.Error("Enter returned nil cmd; want textarea Focus() cmd") + } +} + +// TestWriteAskTempFile_RoundTrip: write a body, read it back from the +// returned path, verify it's under ~/.canopy/tmp/ with the ask- +// prefix + .md suffix shape the sweep depends on. +func TestWriteAskTempFile_RoundTrip(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + body := "what does this regex do?" + path, err := writeAskTempFile(body) + if err != nil { + t.Fatalf("writeAskTempFile: %v", err) + } + t.Cleanup(func() { _ = os.Remove(path) }) + + // Path shape + if !strings.HasPrefix(filepath.Base(path), "ask-") { + t.Errorf("path basename %q lacks ask- prefix", filepath.Base(path)) + } + if !strings.HasSuffix(path, ".md") { + t.Errorf("path %q lacks .md suffix", path) + } + if !strings.Contains(path, filepath.Join(home, ".canopy", "tmp")) { + t.Errorf("path %q not under HOME/.canopy/tmp", path) + } + // Body round-trip + got, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read back: %v", err) + } + if string(got) != body { + t.Errorf("body round-trip: got %q, want %q", string(got), body) + } +} + +// TestWriteAskTempFile_NoStaleTmpfile: an interrupted writeAskTempFile +// shouldn't leave the .tmp scratch file behind. We verify by reading +// the parent dir after a successful write: only the final .md exists. +func TestWriteAskTempFile_NoStaleTmpfile(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path, err := writeAskTempFile("body") + if err != nil { + t.Fatalf("writeAskTempFile: %v", err) + } + t.Cleanup(func() { _ = os.Remove(path) }) + + entries, _ := os.ReadDir(filepath.Dir(path)) + tmpCount := 0 + for _, e := range entries { + if strings.HasSuffix(e.Name(), ".tmp") { + tmpCount++ + } + } + if tmpCount != 0 { + t.Errorf("found %d leftover .tmp files in tmp dir", tmpCount) + } +} + +// fakeTmuxClient stubs tmux.Client's DisplayPopup so askPopupCmd tests +// don't need a running tmux server. +type fakeTmuxClient struct { + gotCommand string + gotCwd string + returnErr error +} + +func (f *fakeTmuxClient) DisplayPopup(ctx context.Context, command, cwd string) error { + f.gotCommand = command + f.gotCwd = cwd + return f.returnErr +} + +// TestAskPopupCmd_HappyPath: askPopupCmd writes a temp file, dispatches +// `canopy ask --file ` via DisplayPopup, then removes +// the temp file. Verifies the wire end-to-end with a fake tmux client. +func TestAskPopupCmd_HappyPath(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + fake := &fakeTmuxClient{} + + cmd := askPopupCmd("codex", "what does this do?", "/tmp/proj", fake) + msg := cmd() + done, ok := msg.(askDoneMsg) + if !ok { + t.Fatalf("msg type = %T; want askDoneMsg", msg) + } + if done.err != nil { + t.Errorf("done.err = %v; want nil", done.err) + } + + // DisplayPopup got the right invocation. Post codex-review P1 #2, + // the popup command is wrapped with `bash -c '... ; read -r _'` so + // it stays open after canopy ask exits; verify both the bash wrapper + // AND the inner canopy ask invocation are present. Post P2 #4 the + // tmpPath is passed as a positional argument ($1), not interpolated + // into the bash body, so we look for `--file "$1"` literally and + // the path appears as a single-quoted positional after the body. + if !strings.HasPrefix(fake.gotCommand, "bash -c '") { + t.Errorf("DisplayPopup command = %q; want 'bash -c ...' wrapper", fake.gotCommand) + } + if !strings.Contains(fake.gotCommand, `canopy ask 'codex' --file "$1"`) { + t.Errorf("DisplayPopup command = %q; missing `canopy ask 'codex' --file \"$1\"` invocation", fake.gotCommand) + } + if !strings.Contains(fake.gotCommand, "read -r _") { + t.Errorf("DisplayPopup command = %q; missing 'read -r _' dismiss prompt (P1 #2)", fake.gotCommand) + } + if fake.gotCwd != "/tmp/proj" { + t.Errorf("DisplayPopup cwd = %q; want /tmp/proj", fake.gotCwd) + } + + // Temp file was deleted after the popup completed. + tmpDir := filepath.Join(home, ".canopy", "tmp") + entries, _ := os.ReadDir(tmpDir) + for _, e := range entries { + if strings.HasPrefix(e.Name(), "ask-") { + t.Errorf("temp file %s not cleaned up", e.Name()) + } + } +} + +// TestPosixShellQuote_BasicCases: spot-check the inline shell-quote +// helper. Pins the contract askPopupCmd depends on for safe path +// passing — paths with spaces, dollars, and embedded single quotes +// must all survive the outer shell layer untouched. +func TestPosixShellQuote_BasicCases(t *testing.T) { + cases := []struct { + in, want string + }{ + {"plain", "'plain'"}, + {"/tmp/foo.md", "'/tmp/foo.md'"}, + {"/home/Foo Bar/.canopy/tmp/ask-abc.md", "'/home/Foo Bar/.canopy/tmp/ask-abc.md'"}, + {"with$var", "'with$var'"}, + {"has'quote", `'has'\''quote'`}, + {"", "''"}, + } + for _, c := range cases { + got := posixShellQuote(c.in) + if got != c.want { + t.Errorf("posixShellQuote(%q) = %q; want %q", c.in, got, c.want) + } + } +} + +// TestAskPopupCmd_TmpPathWithSpaces pins the bug codex review caught +// 2026-06-25 (P2 #4): a $HOME containing a space (which makes tmpPath +// land at something like /home/Foo Bar/.canopy/tmp/ask-abc.md) used to +// produce a popup command that bash re-split on the space, sending only +// the first half to --file. The fix passes tmpPath as a POSITIONAL +// argument to bash, single-quoted at the outer-shell layer. +// +// We can't easily set HOME for this test (the popup command builder +// reads HOME via writeAskTempFile, but we want to verify the QUOTING +// behavior of the assembled command independently). So we assert +// that the assembled command always ends with a single-quoted +// positional argument matching the temp file path — that's the +// contract that protects path safety regardless of $HOME shape. +func TestAskPopupCmd_TmpPathQuotedAsPositional(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + fake := &fakeTmuxClient{} + + cmd := askPopupCmd("codex", "what does this do?", "/tmp/proj", fake) + _ = cmd() + + // Command shape: ends with `_ ''` (sh placeholder for $0, + // then quoted $1). + got := fake.gotCommand + if !strings.HasSuffix(got, ".md'") { + t.Errorf("popup command should end with a single-quoted .md path; got %q", got) + } + // The positional must be wrapped in single quotes (not interpolated + // raw into the bash body). + if !strings.Contains(got, " _ '") { + t.Errorf("popup command missing `_ '...'` positional pattern; got %q", got) + } +} + +// TestAskPopupCmd_DisplayPopupErrorSurfaced: when tmux fails, the +// askDoneMsg carries the wrapped error. +func TestAskPopupCmd_DisplayPopupErrorSurfaced(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + fake := &fakeTmuxClient{returnErr: errors.New("tmux exploded")} + + cmd := askPopupCmd("codex", "q", "/tmp/proj", fake) + msg := cmd() + done := msg.(askDoneMsg) + if done.err == nil { + t.Fatal("done.err = nil; want wrapped DisplayPopup error") + } + if !strings.Contains(done.err.Error(), "tmux exploded") { + t.Errorf("err missing underlying message: %v", done.err) + } +} + +// TestHandleAskDone_AlwaysReturnsToListMode: success or error, the +// done handler sends the user back to the list (the popup IS the +// answer surface — nothing more to render in the TUI). +func TestHandleAskDone_AlwaysReturnsToListMode(t *testing.T) { + cases := []struct { + name string + err error + }{ + {"success", nil}, + {"error", errors.New("kaboom")}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + m := &Model{mode: askInputMode, askTarget: "foo"} + _, _ = m.handleAskDone(askDoneMsg{err: tc.err}) + if m.mode != listMode { + t.Errorf("mode = %v; want listMode", m.mode) + } + if tc.err != nil && m.err == nil { + t.Error("Model.err nil after errored popup; should carry surface message") + } + }) + } +} + +// TestRenderAskPicker_LinesPresent: the picker view shows agent names +// + workspace name + nav hint. +func TestRenderAskPicker_LinesPresent(t *testing.T) { + m := &Model{ + mode: askPickerMode, + askTarget: "feature-x", + askList: []string{"claude", "codex"}, + askCursor: 1, + } + out := m.renderAskPicker() + for _, want := range []string{"feature-x", "claude", "codex", "next", "cancel"} { + if !strings.Contains(out, want) { + t.Errorf("renderAskPicker missing %q\nfull:\n%s", want, out) + } + } +} + +// TestRenderAskInput_LinesPresent: the input view shows the chosen +// agent + workspace + submit hint. +func TestRenderAskInput_LinesPresent(t *testing.T) { + m := &Model{ + mode: askInputMode, + askTarget: "feature-x", + askAgent: "codex", + askInput: newAskTextarea(), + } + out := m.renderAskInput() + for _, want := range []string{"feature-x", "codex", "Ctrl+S", "Esc"} { + if !strings.Contains(out, want) { + t.Errorf("renderAskInput missing %q\nfull:\n%s", want, out) + } + } +} diff --git a/internal/ui/view.go b/internal/ui/view.go index 5512244..3a30cf9 100644 --- a/internal/ui/view.go +++ b/internal/ui/view.go @@ -70,6 +70,12 @@ func (m *Model) View() string { return m.renderUpgrade() case hostUpgradeMode: return m.renderHostUpgrade() + case agentSwapPickerMode: + return m.renderAgentSwapPicker() + case askPickerMode: + return m.renderAskPicker() + case askInputMode: + return m.renderAskInput() } if m.mode == confirmRetryMode { From 330d031b1addd7f0ca8d675df333d70d66771dd5 Mon Sep 17 00:00:00 2001 From: Avinash Joshi Date: Thu, 25 Jun 2026 22:25:21 -0700 Subject: [PATCH 3/5] chore: bump version and changelog (v0.22.0.0) Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 38 +++++++++++++++++++++++++++++++++++++- TODOS.md | 44 +++++++++++++++++++++++++++++++++++++++++++- VERSION | 2 +- 3 files changed, 81 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b060024..6abb39d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,43 @@ All notable changes to canopy are documented here. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and canopy adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [0.21.16.0] - 2026-06-25 — tell "my work" from "a PR I'm reviewing" at a glance +## [0.22.0.0] - 2026-06-25 — agent layer v1: codex parity, swap on the fly, second-opinion popup + +Canopy used to assume one agent per project: claude, period. Codex existed only as a known string. v0.22 turns "the agent" into a first-class field on every workspace and gives the user three new operating modes: pick a non-default agent at workspace creation, swap the running agent without losing per-agent conversation history, and pop open a second-opinion popup that bills against a different agent for a single throwaway question. + +The model change is small but load-bearing. `state.Workspace` grows `CurrentAgent` (the launcher this workspace is currently running) and `AgentLaunches` (a per-agent launch counter). Both are `omitempty`, so pre-v0.22 state files read back unchanged; the workspace manager populates them lazily on first read by promoting the legacy global `AgentLaunchCount` onto whichever agent matches `CurrentAgent`. `canopy.json` learns an `agents: [...]` array (the allowlist a project advertises); the legacy `agent: { type: ... }` block is still honored as a single-agent fallback. The config loader preserves unknown keys via a raw-JSON-map shim so an out-of-band edit doesn't get truncated on the next save. + +`canopy new --agent ` picks the launcher at creation; remote dispatch forwards the flag. `canopy agent swap ` (from cwd) and the TUI's `A` keybind (from any workspace row) replace the running agent pane with a fresh pane of the new launcher, preserving byte-precise tmux geometry via `capture-pane`/`select-layout`. The first swap to a new agent gets the FULL fresh briefing — even when claude has already run in the workspace — because the fresh/resume decision is now keyed on the per-agent counter, not the legacy global one. Subsequent swaps back to a previously-used agent hit Resume (claude's `--continue`, codex's `resume --last`), so swap-and-swap-back restores the prior conversation as the design promised. + +`canopy ask [--file path|prompt]` is a one-shot non-interactive invocation: send a prompt to the target launcher, get the answer, exit. The TUI's `Q` keybind opens a picker + textarea, writes the question to `~/.canopy/tmp/ask-*.md`, and spawns `canopy ask` inside a `tmux display-popup`. The popup stays open after the answer renders (a `read -r _` prompt at the end of the body) so fast answers don't vanish before the user can read them, and the temp-file path is passed as a positional shell argument (`bash -c '...' _ ''`) so a `$HOME` with spaces or shell metacharacters survives. + +Codex CLI flag drift broke the inline-prompt approach. As of `codex-cli 0.142.2` the `--instructions` flag is gone — the only way to deliver content at launch is the positional `[PROMPT]` argument (codex treats it as the user's first turn). Resume now uses `codex resume --last` (added between 0.140 and 0.142). The argv assembly's strip-on-empty-briefing logic now correctly distinguishes a flag NAME (pop) from a flag VALUE (keep), so `codex resume --last --ask-for-approval on-request` survives an empty briefing without losing the `on-request` value. + +### Added + +- **`canopy agent swap ` (CLI) and `A` (TUI) swap the running agent.** Kills the agent pane, persists `CurrentAgent`, respawns the new launcher via `tmux split-window` off the IDE pane, and restores the original window layout byte-for-byte. Allowed-but-not-installed agents refuse cleanly without tearing down the running pane (codex review P1 #3 caught this); the new pane always splits off the IDE regardless of which pane the user's focus was on (P1 #4). `cwd`-walk-up routes the CLI to the right workspace, including from dot-prefixed subdirectories like `.github`/`.config`/`.gstack` (P2 #3). +- **`canopy ask [--file path | prompt]` one-shot popup.** Dispatches a single non-interactive question. TUI `Q` writes the question to `~/.canopy/tmp/ask-*.md` (atomic tmpfile + rename + startup sweep backstop) and spawns the popup. The bash body wraps with `; read -r _` so the popup stays open after the answer renders; tmpPath is passed as a positional shell argument so a `$HOME` with spaces, `$`, or `'` survives (P2 #4). +- **Per-agent launch counter `AgentLaunches map[string]int`.** Tracks fresh-vs-resume independently per launcher in each workspace. First-time spawn of a new agent gets the FULL fresh briefing even when another agent ran first; subsequent spawns of the same agent resume its prior conversation. Migrated lazily from the legacy `AgentLaunchCount` (mapped onto `CurrentAgent` on first read) so old state files are forward-compatible. +- **`agents: [...]` allowlist in `canopy.json`.** First entry is the project default; legacy `agent.type` block still honored. Unknown launcher types fail fast with a clear error before any tmux/state mutation. `canopy new --agent ` validates against the allowlist. Picking an unlisted launcher in the swap/ask picker silently auto-adds it to the allowlist — preserving the legacy single-agent shape and the implicit-claude default (codex review P1 #5). +- **Agent classifier framework (`internal/agent/classifier*.go`).** Per-launcher pane-state detectors (idle / awaiting input / responding) for claude, codex, opencode, aider. Table-driven tests against fixture pane content in `internal/agent/testdata/`. Drives the TUI's agent-pane state badge. +- **`internal/tmux/layout.go` capture/restore helpers.** Wraps `tmux display-message '#{window_layout}'` + `select-layout ` so swap operations preserve geometry across kill-pane + split-window. + +### Changed + +- **`BuildBriefing` keys fresh-vs-resume on the per-agent counter.** Signature now takes `agentType`. Reads `ws.AgentLaunches[agentType]` (with legacy `AgentLaunchCount` fallback when the agent matches `ws.CurrentAgent`). Pins the regression that codex review caught (P1 #1): swap-to-codex on a workspace where claude has run no longer dropped onto the delta path with empty context. +- **`PlanLaunch` strip-on-empty mirrors `BuildArgv`'s guard.** Only pops the preceding arg when it starts with `-`. Codex's post-0.142.2 argv (`... --ask-for-approval on-request {{briefing}}`) survives the empty-briefing branch without losing `on-request`. Same guard as `BuildArgv` — they had drifted, and only `BuildArgv` had the fix (codex review P1 #2). +- **codex launcher uses `resume --last` and positional `[PROMPT]`.** `--instructions` was dropped in `codex-cli 0.142.2`; the new shape works against current codex. `--ask-for-approval on-request` is set explicitly so canopy's `AwaitingMarkers` classifier can render the ✋ badge consistently with claude. +- **`canopy.json` loader preserves unknown keys.** Round-trips a raw JSON map through Load/Save so future schema fields don't get truncated by an old version, and project-local edits the user makes between runs survive `AddAgentToCanopyJSON`'s auto-add path. +- **Remote dispatch forwards `--agent`.** `cmd/canopy/new.go`'s `dispatchNewToRemote` appends `--agent ` to `canopyArgs` (codex review P2 #6). + +### Fixed + +- **`canopy agent swap` from `.github`/`.config`/`.gstack` cwd no longer false-rejects.** The cwd-walk-up's "inside-workspace" check now uses `!strings.HasPrefix(rel, "..")` instead of `rel[0] != '.'`, so a dot-prefixed subdirectory of the workspace correctly resolves. Table-driven test in `cmd/canopy/agent_test.go` pins 10 cases (codex review P2 #3). +- **Agent swap validates launcher install BEFORE tearing down the agent pane.** Previously an allowed-in-`canopy.json`-but-not-on-PATH agent killed the running pane before failing. Now `agent.Resolve` + `launcher.VerifyInstalled` run as Step 2, ahead of any tmux/state mutation (codex review P1 #3). +- **Agent swap splits the new pane off the IDE, not the active pane.** `SelectPane(idePaneID)` before `SplitPane` so the geometry restore doesn't have to rescue a wrong-pane split (codex review P1 #4). +- **`canopy ask` for a launcher with no `Exec` mode wired errors before touching `canopy.json`.** `ResolveExec` check moved ahead of `AddAgentToCanopyJSON` so picking opencode (Exec=nil today) no longer leaves a config side-effect on the way to the error (codex review P2 #7). + + The Workspaces tab gave no signal for the one distinction that matters when you run many parallel worktrees: which rows are your own feature work, and which are checkouts of someone else's PR you pulled in to review. They looked identical. This adds an owner concept that marks the review rows and leaves your own quiet. diff --git a/TODOS.md b/TODOS.md index 16e1a8a..91e45ce 100644 --- a/TODOS.md +++ b/TODOS.md @@ -12,6 +12,40 @@ Each entry is self-contained for someone (you, future-Claude, or another AI agen --- +## 📋 OPEN (P2) — opencode launcher: wire `Resume` argv and `Exec` mode (added 2026-06-25, deferred from v0.22.0.0) + +**What:** `internal/agent/launchers.go` ships opencode with empty `Resume: []string{}` and `Exec: nil`. Result: opencode workspaces always launch FRESH (no resume verb), and `canopy ask opencode ` errors out cleanly with `ErrLauncherNoExec` instead of running the one-shot. + +**Why deferred:** Couldn't dogfood the right flags. The opencode binary on the dogfood machine on 2026-06-25 fails to start with `Could not resolve npm bin for opencode-ai`. The other launchers' Resume/Exec argv shapes were verified by running them; doing the same for opencode without a working install would be guessing. + +**Fix sketch:** +1. Get opencode running on a test machine (`npm install -g opencode-ai` or whatever fixes the bin resolution). +2. Confirm the resume flag/verb opencode supports today. claude uses `--continue`, codex uses `resume --last`, aider uses `--restore-chat-history`; opencode probably has one. Document at the top of the launcher entry with a "verified " comment, matching the existing convention. +3. Set `Resume` and `Exec` in `defaultLaunchers["opencode"]`. Add `PlanLaunch` tests pinning the resume argv shape (mirror `TestPlanLaunch_CodexResume*`). +4. Remove the `Exec: nil` and update `cmd/canopy/ask.go`'s opencode-no-exec branch (no `ErrLauncherNoExec` path needed anymore). + +**Where to look:** `internal/agent/launchers.go:221-233` (the opencode entry, with the explanatory comment), `cmd/canopy/ask.go` (the `ResolveExec` check that surfaces `ErrLauncherNoExec`), `internal/agent/launchers_test.go` (the codex tests as the shape template). + +--- + +## 📋 OPEN (P3) — codex per-session-ID resume for precise per-cwd continuity (added 2026-06-25, deferred from v0.22.0.0) + +**What:** codex Resume currently uses `codex resume --last`, which picks the GLOBAL most-recent codex session — not the one for this workspace's cwd. If the user runs codex in another directory between two canopy-driven codex launches in this workspace, `--last` grabs the wrong conversation. + +**Why deferred:** Lower-impact than the v0.22 swap/ask features; same caveat that claude's `--continue` has (also global-most-recent). The user-visible incident requires running codex outside canopy between two canopy launches AND noticing the resumed conversation is the wrong one. Both unusual, both recoverable. + +**Fix sketch:** +1. Parse `codex` session listing (CLI shape TBD — `codex sessions list --json` or similar; verify against the installed version). +2. After each `canopy`-driven codex launch, capture the session UUID (codex prints/persists it; need to figure out the durable lookup). +3. Persist `last_codex_session_id` per workspace in `state.Workspace`. +4. Change codex `Resume` argv to `resume ` when the UUID is known; fall back to `resume --last` when unset (first launch + workspaces from before this lands). + +**Why this is a behind-the-scenes fix:** No user-facing API change. The Resume verb is internal-only; the user always sees "the conversation I had in this workspace" either way — this just makes it correct under the cross-workspace contention case. + +**Where to look:** `internal/agent/launchers.go:181-220` (codex launcher entry, ResumeArgs `["resume", "--last", ...]`), `internal/state/state.go` (the Workspace struct where `LastCodexSessionID` would land), `internal/agent/launchers_test.go` (`TestPlanLaunch_CodexResume*` as the assertion template). + +--- + ## 📋 OPEN (P3) — `git.Sanitize` doesn't strip git-invalid dot sequences (added 2026-06-25) **What:** `Sanitize`'s character class is `[^A-Za-z0-9._-]+`, so dots survive (intentionally — `v1.2.3` is a valid branch). But git rejects a few dot patterns that Sanitize passes straight through: a ref can't contain `..`, can't end in `.`, and can't end in `.lock`. So `canopy new "a..b"` (no `--branch`) still produces an invalid ref `a..b` and `git worktree add -b` fails — the same class of bug as the spaces case fixed in PR `fix-branch-name-sanitize`, just rarer. @@ -292,7 +326,15 @@ Defer post-v0.1; this is polish that depends on the BYO flow first feeling solid --- -## 📋 OPEN — v0.16.x — Extend `--prompt` / background workspaces to codex + opencode (added 2026-05-11) +## 🔄 PARTIAL — v0.16.x — Extend `--prompt` / background workspaces to codex + opencode (added 2026-05-11, codex parity shipped v0.22.0.0) + +**Status:** Wave 1 (codex classifier + agent-state badge parity) shipped in v0.22.0.0. The `Classifier` interface lives in `internal/agent/classifier.go`; `classifier_codex.go` has a real implementation (`IsRendering` and `IsTrustDialog` keyed on codex's pane markers, table-driven tests against fixtures in `internal/agent/testdata/`); `classifier_opencode.go` and `classifier_aider.go` are stubbed `false`. Remaining work: + +- **opencode classifier** — needs fixture content + marker patterns. Blocked on opencode binary being broken on the dogfood machine (`Could not resolve npm bin for opencode-ai` 2026-06-25). Pick up when the install works. +- **aider classifier** — `--yes-always` interactive flow is a prompt-injection concern; defer until someone asks. +- **`SendInitialPrompt(paneID, text) error` per launcher** — `--prompt`/`--prompt-file` delivery is still claude-only. The `Classifier` framework is in place to dispatch by launcher; the actual paste-into-pane handler isn't. + +Below is the original 2026-05-11 entry, preserved for the architectural context: v0.16.1 shipped `canopy new --prompt`/`--prompt-file` + the agent-state badge column, but the prompt-delivery flow is claude-only. The agent registry in `internal/agent/launchers.go` already understands codex / opencode / aider as launcher types — the gap is in the kickoff path: diff --git a/VERSION b/VERSION index 238b38c..a194d17 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.21.16.0 +0.22.0.0 From 567458eb4cb74dbf90385dbaf4cbf58310ae16dd Mon Sep 17 00:00:00 2001 From: Avinash Joshi Date: Thu, 25 Jun 2026 22:43:31 -0700 Subject: [PATCH 4/5] fix(test): stub codex/claude in PATH for swap tests so CI passes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The codex review P1 #3 fix added launcher.VerifyInstalled (=exec.LookPath) as Step 2 of SwapAgent — runs BEFORE any tmux/state mutation so an allowed-but-not-installed launcher refuses cleanly. CI runners don't have @openai/codex installed, so the swap tests now fail at the VerifyInstalled gate with 'codex not found on PATH'. Fix: stubAgentBinaries(t, "claude", "codex") creates no-op executables in a t.TempDir() and prepends to PATH via t.Setenv. The stubs are read-only sentinels — VerifyInstalled doesn't care what the binary does, only that LookPath finds it. fixtureWithAgents calls stubAgentBinaries first so every existing test under it picks up the stubs automatically. No production-code change. Verified locally: tests pass both WITH real codex on PATH (no stub interference) and with PATH stripped to /usr/bin:/bin (CI simulation). Co-Authored-By: Claude Opus 4.7 (1M context) --- internal/workspace/agent_swap_test.go | 32 +++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/internal/workspace/agent_swap_test.go b/internal/workspace/agent_swap_test.go index edfe02f..8f2807f 100644 --- a/internal/workspace/agent_swap_test.go +++ b/internal/workspace/agent_swap_test.go @@ -16,11 +16,43 @@ import ( "github.com/avinashjoshi/canopy/internal/workspace" ) +// stubAgentBinaries creates no-op executables for "claude" and "codex" +// in a temp dir prepended to PATH for the duration of the test. The +// codex review P1 #3 fix (agent_swap.go step 2) calls +// launcher.VerifyInstalled (= exec.LookPath) BEFORE any tmux/state +// mutation. CI runners don't have @openai/codex installed; without +// these stubs every swap test fails at the launcher check. The stubs +// are read-only sentinels — actual launching of the agent pane in +// these tests already hits agentFallbackShell when the binary is +// truly missing in normal runs, but VerifyInstalled doesn't care +// what the binary does, only that LookPath finds it. +// +// Why per-test PATH stub (vs. installing in CI): the test models the +// CONTRACT — "swap fails fast when codex isn't on PATH" — without +// requiring CI to ship a real codex. A separate test could pin the +// VerifyInstalled-rejects-missing-launcher contract by NOT stubbing. +func stubAgentBinaries(t *testing.T, names ...string) { + t.Helper() + stubDir := t.TempDir() + for _, name := range names { + path := filepath.Join(stubDir, name) + // Minimal POSIX stub: exec a shell that immediately exits 0. + // tmux respawn-pane with -K keeps the pane open afterward, so + // the pane survives long enough for LookupAllPanes to find it. + body := "#!/bin/sh\nexec /bin/sh\n" + if err := os.WriteFile(path, []byte(body), 0o755); err != nil { + t.Fatalf("write stub %s: %v", name, err) + } + } + t.Setenv("PATH", stubDir+string(os.PathListSeparator)+os.Getenv("PATH")) +} + // fixtureWithAgents is fixture(), but the canopy.json declares // `agents: ["claude", "codex"]` so SwapAgent's allowlist gate has both // real launchers available. claude is the default (first entry). func fixtureWithAgents(t *testing.T) (*workspace.Manager, func()) { t.Helper() + stubAgentBinaries(t, "claude", "codex") mgr, cleanup := fixture(t) // Overwrite canopy.json with one that declares agents. fixture() // already wrote a minimal one without an agents block. From cd85c0f07b4562f36d369adec7636dcbda902ba8 Mon Sep 17 00:00:00 2001 From: Avinash Joshi Date: Fri, 26 Jun 2026 10:10:32 -0700 Subject: [PATCH 5/5] =?UTF-8?q?fix(agent):=20four=20ultrareview=20findings?= =?UTF-8?q?=20=E2=80=94=20empty=20briefing=20on=20swap,=20codex=20IsRender?= =?UTF-8?q?ing,=20codex=20chevron,=20swap=20CLI=20message?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit bug_001 (NORMAL) — SwapAgent gave new agent EMPTY briefing on first swap. Step 5 mutated CurrentAgent to newType before Step 6 called BuildBriefing, so launchCountFor's `agentType == CurrentAgent` fallback returned the legacy AgentLaunchCount (3 from claude) for codex. BuildBriefing saw count > 0, hints == nil, returned "". codex spawned via PlanLaunch with no positional [PROMPT] — zero context. The CHANGELOG's headline P1 #1 fix from v0.22.0.0 was silently broken end-to-end. Two-layer fix: - agent_swap.go Step 5 now initializes AgentLaunches[newType]=0 when absent, so launchCountFor finds the explicit zero. - briefing.go launchCountFor's legacy fallback only fires when AgentLaunches is nil entirely (genuine pre-v0.22 row, not just a missing key on a migrated map). Defense in depth — any caller forgetting to seed the per-agent key still gets the right briefing. - Test fixture updated to mirror production: CurrentAgent="codex" (the NEW agent), AgentLaunches with explicit "codex":0. Added a second test (BareSwapStillFresh) that documents the briefing-side robustness without the SwapAgent init. bug_008 (NORMAL) — codexClassifier.IsRendering failed in production. codex's idle markers live at the TOP of the visible pane (banner rows 1-6, footer row ~15), but the implementation matched against bottomLines(content, 12). tmux capture-pane -p preserves trailing blank rows that codex hasn't drawn into, so a typical 50-row pane yielded 12 blank lines and the markers never matched. canopy new --agent codex --prompt always timed out at Phase 1/2 with "Phase 1 timeout: neither trust dialog nor agent ready marker appeared in 5s". The unit test masked the bug because readFixture stripped trailing blanks before classification; production saw the raw shape. Fix: trim trailing blank rows and match against the full visible content. Added readFixtureRaw + TestCodexClassifier_IsRendering_HandlesRawPaneContent which uses the un-trimmed fixture, plus a non-codex-pane rejection test to guard against an over-eager match. bug_006 (NORMAL) — normalize() didn't strip codex's › chevron. The inputLine regex was hardcoded to claude's ❯ (U+276F), so every keystroke into an idle codex pane flipped the normalized-content hash; Detector.Observe ran its motion check before idle marker matching and returned StateThinking at confidence 9. The TUI badge column said ⚡ Thinking while the user was composing. Fix: extend regex to char class [❯›] — claude's ❯ + codex's › (U+203A). Added paired regression tests for both chevrons so an over-eager rewrite can't drop either. bug_002 (NIT) — `canopy agent swap ` printed `Swapped → ` instead of ` → `. The success Fprintf used ws.CurrentAgent (already mutated to newType by SwapAgent Step 5) as the FROM. Fix: capture the row's CurrentAgent BEFORE SwapAgent via a new currentAgentForWorkspace helper that walks mgr.List(ctx) the same way findWorkspaceFromCwd already does. A test expectation in TestCodexClassifier_AgainstRealFixtures (codex_awaiting_input case) flipped from wantRender:false to wantRender:true — the previous value was bug-compatible with the old bottomLines(12) shape, not the semantic-correct answer. Co-Authored-By: Claude Opus 4.7 (1M context) --- cmd/canopy/agent.go | 37 ++++++++++++++- internal/agent/briefing.go | 34 ++++++++++---- internal/agent/briefing_test.go | 51 ++++++++++++++++++-- internal/agent/classifier_codex.go | 38 +++++++++++---- internal/agent/classifier_test.go | 74 +++++++++++++++++++++++++++++- internal/agent/state.go | 24 +++++++--- internal/agent/state_test.go | 32 +++++++++++++ internal/workspace/agent_swap.go | 17 +++++++ 8 files changed, 274 insertions(+), 33 deletions(-) diff --git a/cmd/canopy/agent.go b/cmd/canopy/agent.go index 9cce908..6f11ecf 100644 --- a/cmd/canopy/agent.go +++ b/cmd/canopy/agent.go @@ -85,6 +85,16 @@ func agentSwapCmd() *cobra.Command { return err } + // Capture the CURRENT agent BEFORE SwapAgent runs. + // SwapAgent's returned *Workspace has CurrentAgent already + // mutated to newType (Step 5 in agent_swap.go), so we can't + // derive the "from" agent for the success message after the + // fact. (ultrareview bug_002, 2026-06-26.) + oldType, err := currentAgentForWorkspace(ctx, mgr, name) + if err != nil { + return fmt.Errorf("canopy agent swap: look up current agent: %w", err) + } + ws, err := mgr.SwapAgent(ctx, name, newType) if err != nil { // Pretty-print known sentinels so the user gets clean @@ -104,7 +114,7 @@ func agentSwapCmd() *cobra.Command { "Swapped %s → %s in workspace %q.\n"+ "The new agent has been spawned in the same pane geometry.\n"+ "Run `canopy switch %s` to attach if you're not already there.\n", - ws.CurrentAgent, newType, ws.Name, ws.Name) + oldType, ws.CurrentAgent, ws.Name, ws.Name) return nil }, } @@ -150,6 +160,31 @@ func findWorkspaceFromCwd(ctx context.Context, mgr *workspace.Manager, cwd strin cwd) } +// currentAgentForWorkspace returns the named workspace's CurrentAgent +// as currently persisted in state.json. Used by the swap CLI to render +// the "from" half of the success message — must be read BEFORE +// SwapAgent runs, because SwapAgent mutates the row in place. +// +// Falls back to the project's default agent (canopy.json `agents[0]`, +// then legacy `agent.type`, then "claude") when the row hasn't been +// migrated yet — same fallback shape that workspace.currentAgent uses +// internally. (ultrareview bug_002, 2026-06-26.) +func currentAgentForWorkspace(ctx context.Context, mgr *workspace.Manager, name string) (string, error) { + rows, err := mgr.List(ctx) + if err != nil { + return "", err + } + for _, r := range rows { + if r.Name == name && r.ProjectRoot == mgr.Cfg.ProjectRoot { + if r.CurrentAgent != "" { + return r.CurrentAgent, nil + } + return mgr.Cfg.DefaultAgent(), nil + } + } + return "", fmt.Errorf("workspace %q not found in this project", name) +} + // isInsideWorkspace reports whether `rel` (output of filepath.Rel(wsPath, cwd)) // indicates cwd is the workspace dir itself or a descendant of it. // diff --git a/internal/agent/briefing.go b/internal/agent/briefing.go index fc30c4b..a5fb836 100644 --- a/internal/agent/briefing.go +++ b/internal/agent/briefing.go @@ -52,20 +52,34 @@ func BuildBriefing(ws state.Workspace, cfg *config.Config, hints []state.Hint, a return buildDelta(hints) } -// launchCountFor returns the per-agent launch counter for agentType, -// falling back to ws.AgentLaunchCount when the per-agent map is missing -// AND the named agent matches ws.CurrentAgent. That last clause covers -// the brief window between state-file load and the lifecycle migration -// (lifecycle.go's Load populates AgentLaunches from AgentLaunchCount on -// first read, but during state-file rewrites or concurrent reads we may -// still see the pre-migration shape). Without the fallback, an old -// workspace's first post-upgrade launch would re-show the fresh briefing -// — annoying but not wrong, since fresh is a strict superset of delta. +// launchCountFor returns the per-agent launch counter for agentType. +// +// Returns 0 (fresh briefing) in two cases: +// - AgentLaunches[agentType] is recorded as 0. +// - AgentLaunches is non-nil but the key is missing — the workspace +// has been migrated to the per-agent map, so a missing entry IS a +// true zero, not pre-migration ambiguity. +// +// Falls back to the legacy global AgentLaunchCount only when +// AgentLaunches is nil entirely (genuine pre-v0.22 row that hasn't +// been touched by the migration in lifecycle.Load yet) AND the queried +// agent matches ws.CurrentAgent (the legacy total only describes the +// current agent's launches under the pre-v0.22 single-agent assumption). +// +// 2026-06-26 (ultrareview bug_001): the original implementation +// fell back to the legacy total whenever the per-agent KEY was +// missing, even on a populated AgentLaunches map. That broke +// SwapAgent's first-swap case: Step 5 mutated CurrentAgent to newType +// before BuildBriefing ran, so `agentType == CurrentAgent` matched +// and the fallback returned the prior agent's launch count → empty +// briefing for the new agent. The narrower fallback below is robust +// even if a caller forgets to seed AgentLaunches[newType]=0; the +// caller-side fix lives in agent_swap.go Step 5 as defense in depth. func launchCountFor(ws state.Workspace, agentType string) int { if n, ok := ws.AgentLaunches[agentType]; ok { return n } - if agentType == ws.CurrentAgent { + if ws.AgentLaunches == nil && agentType == ws.CurrentAgent { return ws.AgentLaunchCount } return 0 diff --git a/internal/agent/briefing_test.go b/internal/agent/briefing_test.go index 36710ea..5a0f361 100644 --- a/internal/agent/briefing_test.go +++ b/internal/agent/briefing_test.go @@ -90,15 +90,26 @@ func TestBuildBriefing_ResumeNoHintsReturnsEmpty(t *testing.T) { // is > 0. Without the per-agent gate, codex spawned with delta-or-empty // context on its first appearance — invisible to humans but // catastrophic for the new agent's onboarding. +// +// 2026-06-26 update: this test originally set ws.CurrentAgent = "claude" +// (the OLD agent), but ultrareview bug_001 caught that production +// behavior is different. SwapAgent mutates CurrentAgent to newType +// (e.g. "codex") in Step 5 BEFORE Step 6 calls BuildBriefing. So the +// fixture now models the production sequence by setting CurrentAgent +// to the NEW agent and adding an explicit AgentLaunches[newType]=0 +// entry — mirroring SwapAgent's Step 5 initialization. Without that +// init, launchCountFor's `agentType == CurrentAgent` fallback returns +// the legacy AgentLaunchCount (3 from claude) → BuildBriefing returns +// empty → codex spawns with no context. func TestBuildBriefing_SwapToNewAgent_GetsFreshBriefing(t *testing.T) { ws := fixtureWorkspace() - // Workspace has been running claude for a while. Legacy global - // counter says 3 launches, per-agent says claude=3, codex=0. - ws.CurrentAgent = "claude" + // Production sequence: claude ran 3 times, then SwapAgent flipped + // CurrentAgent to codex and ensured AgentLaunches[codex] == 0. + ws.CurrentAgent = "codex" ws.AgentLaunchCount = 3 - ws.AgentLaunches = map[string]int{"claude": 3} + ws.AgentLaunches = map[string]int{"claude": 3, "codex": 0} - // Now we're spawning codex for the first time in this workspace. + // Now BuildBriefing is called for the codex spawn. out := BuildBriefing(ws, fixtureConfig(), nil, "codex") // Must be the FULL briefing — codex needs the workspace context. @@ -114,6 +125,36 @@ func TestBuildBriefing_SwapToNewAgent_GetsFreshBriefing(t *testing.T) { } } +// TestBuildBriefing_SwapToNewAgent_RegressionWithoutZeroInit reproduces +// the ultrareview bug_001 failure shape: CurrentAgent already mutated +// to the new agent BUT AgentLaunches[newType] missing from the map. +// This is what the production code looked like before SwapAgent +// initialized AgentLaunches[newType]=0 in Step 5. The expected +// behavior is to still get the FULL fresh briefing — but the old +// code returned "" because launchCountFor's fallback found +// agentType == CurrentAgent and returned the legacy total. +// +// The fix is in agent_swap.go (Step 5 init), not in launchCountFor. +// This test pins that BuildBriefing's contract — given the production +// fixture shape — still produces the right output even if a caller +// forgot to init the per-agent counter. +func TestBuildBriefing_SwapToNewAgent_BareSwapStillFresh(t *testing.T) { + ws := fixtureWorkspace() + ws.CurrentAgent = "codex" + ws.AgentLaunchCount = 3 + ws.AgentLaunches = map[string]int{"claude": 3} // codex absent + + out := BuildBriefing(ws, fixtureConfig(), nil, "codex") + + // Currently FAILS without the agent_swap.go init fix. + // Documents the load-bearing contract from the SwapAgent side. + if !strings.Contains(out, "# Canopy workspace context") { + t.Errorf("BuildBriefing for never-launched agent should still " + + "be FULL; agent_swap.go Step 5 init is the production fix.\n" + + "Without that init this test documents the failure shape.") + } +} + // TestBuildBriefing_ResumeWithHintsReturnsDelta: resume + at least one // hint returns the delta-only briefing. Must NOT include the static // lifecycle conventions (those were taught on the fresh launch). diff --git a/internal/agent/classifier_codex.go b/internal/agent/classifier_codex.go index f4ef368..fc96646 100644 --- a/internal/agent/classifier_codex.go +++ b/internal/agent/classifier_codex.go @@ -1,6 +1,9 @@ package agent -import "regexp" +import ( + "regexp" + "strings" +) // codexClassifier is the Classifier implementation for the codex // launcher (the `codex` CLI from OpenAI's Codex project, NOT the @@ -73,18 +76,33 @@ var codexAwaitingPatterns = []*regexp.Regexp{ func (codexClassifier) IdleMarkers() []*regexp.Regexp { return codexIdleMarkers } func (codexClassifier) AwaitingMarkers() []*regexp.Regexp { return codexAwaitingPatterns } -// codexRenderingMarkers is the subset of idle markers used for the -// Phase-3 settle check. Same patterns as IdleMarkers; codex's UI -// doesn't have a separate "rendering but not idle" footer the way -// claude's `⏵⏵ auto mode on` distinguishes mode states. +// IsRendering is the Phase-3 settle gate for codex panes. Codex's +// idle markers live in the TOP portion of the visible pane (boxed +// banner at rows 1-6, footer at row ~15) — unlike claude, whose input +// chevron + auto-mode footer sit at the bottom. So we can't use the +// bottomLines helper claude uses; we match against the full visible +// content, after trimming trailing blank rows. // -// Matched against the bottom 12 lines (same bottomLines helper as -// claude) so stale banner in scrollback doesn't pass the check after -// codex crashes back to a shell. +// Why trim before matching: tmux `capture-pane -p` returns the +// VISIBLE pane verbatim, including blank rows that codex hasn't drawn +// into. A typical 50-row pane with codex content in rows 1-15 has 35 +// trailing blanks. Without the trim, bottomLines(content, 12) would +// return 12 blank lines and the markers would never match — which is +// exactly the bug ultrareview caught 2026-06-26 (bug_008). The unit +// test masked it because `readFixture` strips trailing blanks before +// classification, so the test saw the trimmed shape while production +// saw the raw shape. +// +// Why no scrollback-false-positive concern: CapturePane uses `-p` +// without `-S`, so we only see the VISIBLE pane. A crashed-to-shell +// codex doesn't show its banner anywhere in the visible area — the +// banner is gone the moment the codex process exits, replaced by +// whatever the shell renders. func (codexClassifier) IsRendering(content string) bool { - tail := bottomLines(content, 12) + // Strip trailing blank rows the visible pane preserves. + trimmed := strings.TrimRight(content, "\n\t ") for _, p := range codexIdleMarkers { - if p.MatchString(tail) { + if p.MatchString(trimmed) { return true } } diff --git a/internal/agent/classifier_test.go b/internal/agent/classifier_test.go index 87ab1df..dc91408 100644 --- a/internal/agent/classifier_test.go +++ b/internal/agent/classifier_test.go @@ -149,10 +149,17 @@ func TestCodexClassifier_AgainstRealFixtures(t *testing.T) { // Awaiting dialog: ClassifyOneShot returns AwaitingInput // even though the banner at the top of the pane also // matches an idle marker — awaiting beats idle in order. + // + // IsRendering returns TRUE here: codex's UI is still up, + // the approval dialog IS the codex UI in a different mode. + // Pre-ultrareview-bug_008, this was wantRender:false because + // the old bottomLines(12) implementation only saw the dialog + // at the bottom and missed the banner at the top — a bug + // that was bug-compatible with the test, not a feature. fixture: "codex_awaiting_input.txt", wantState: StateAwaitingInput, wantTrust: false, - wantRender: false, // approval dialog takes over bottom 12 lines + wantRender: true, }, } for _, tc := range cases { @@ -206,3 +213,68 @@ func readFixture(t *testing.T, name string) string { return strings.TrimRight(string(data), "\n") } +// readFixtureRaw loads internal/agent/testdata/ WITHOUT trimming +// trailing blank rows. Used by the production-shape regression tests +// (codex IsRendering, etc.) that need to model what tmux capture-pane +// actually returns: the visible pane verbatim, with blank rows that +// the agent hasn't drawn into preserved at the bottom. +// +// Origin: ultrareview bug_008 (2026-06-26). The codex classifier's +// IsRendering passed the unit tests only because readFixture stripped +// trailing blanks before classification; production left them in and +// the markers (top-of-pane in codex's UI) were never reached. +func readFixtureRaw(t *testing.T, name string) string { + t.Helper() + path := filepath.Join("testdata", name) + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read fixture %s: %v", path, err) + } + return string(data) +} + +// TestCodexClassifier_IsRendering_HandlesRawPaneContent pins the +// production shape: codex's idle markers live at the TOP of the +// visible pane, but tmux capture-pane preserves trailing blank rows +// at the bottom. IsRendering must trim those trailing blanks before +// matching so the top-of-pane markers stay reachable. +// +// Without the trim, a 50-row pane with codex in rows 1-15 and 35 +// blank rows below would never match any codex marker — IsRendering +// returns false and the `--prompt` flow's Phase 1/2/3 gates all time +// out. (ultrareview bug_008, 2026-06-26.) +func TestCodexClassifier_IsRendering_HandlesRawPaneContent(t *testing.T) { + // readFixtureRaw preserves the trailing blank rows that the actual + // codex_idle.txt fixture has. + raw := readFixtureRaw(t, "codex_idle.txt") + + // Sanity: the raw fixture really does end with blank rows, OR we + // don't actually exercise the regression. If a future capture is + // re-taken without trailing blanks, this test fails loudly so we + // know to re-create the production shape. + if !strings.HasSuffix(raw, "\n\n") { + t.Fatalf("raw codex_idle fixture lacks trailing blank rows " + + "(it ends with %q); the IsRendering regression depends on that shape — " + + "recapture the fixture from a real codex pane to restore it", + raw[max(0, len(raw)-30):]) + } + + c := ClassifierFor("codex") + if !c.IsRendering(raw) { + t.Errorf("codex IsRendering returned false on raw fixture with " + + "trailing blank rows; the trim-before-match guard isn't holding") + } +} + +// TestCodexClassifier_IsRendering_RejectsNonCodexPane: a pane showing +// a shell (no codex markers) must NOT be classified as rendering codex. +// Defends against the over-eager "match anywhere" fix without losing +// the codex case. +func TestCodexClassifier_IsRendering_RejectsNonCodexPane(t *testing.T) { + shellContent := "user@host:~$ ls\nREADME.md src/ tests/\nuser@host:~$ " + c := ClassifierFor("codex") + if c.IsRendering(shellContent) { + t.Error("codex IsRendering matched a plain shell pane (no codex markers)") + } +} + diff --git a/internal/agent/state.go b/internal/agent/state.go index 616d3cf..a048433 100644 --- a/internal/agent/state.go +++ b/internal/agent/state.go @@ -330,12 +330,24 @@ var spinnerLine = regexp.MustCompile( // pattern matching against claudeIdleMarkers. var footerLine = regexp.MustCompile(`⏵⏵ auto mode on|· /effort`) -// inputLine matches claude's input-prompt line. Anything after the -// chevron is what the user is typing — character-by-character changes -// would otherwise flip the hash and mis-classify "user typing" as -// "claude thinking." Stripping handles the load-bearing UX bug where -// the badge said ⚡ Thinking while you were actively composing. -var inputLine = regexp.MustCompile(`(?m)^\s*❯`) +// inputLine matches the input-prompt line of any agent TUI we +// classify. Anything after the chevron is what the user is typing — +// character-by-character changes would otherwise flip the hash and +// mis-classify "user typing" as "agent thinking." Stripping handles +// the load-bearing UX bug where the badge said ⚡ Thinking while you +// were actively composing. +// +// The character class covers both chevrons we currently see: +// +// - ❯ (U+276F HEAVY RIGHT-POINTING ANGLE QUOTATION MARK ORNAMENT) — claude +// - › (U+203A SINGLE RIGHT-POINTING ANGLE QUOTATION MARK) — codex +// +// 2026-06-26 (ultrareview bug_006): the original regex was hardcoded +// to claude's ❯, so typing into idle codex flipped the normalized +// hash on every keystroke and Detector.Observe returned StateThinking +// at confidence 9 from the motion check, before any marker matching. +// Adding codex's › fixes the badge regression. +var inputLine = regexp.MustCompile(`(?m)^\s*[❯›]`) // claudeAwaitingPatterns are claude-TUI markers that mean a user // action is required RIGHT NOW. Empty-input cursor is intentionally diff --git a/internal/agent/state_test.go b/internal/agent/state_test.go index 9751d69..3759825 100644 --- a/internal/agent/state_test.go +++ b/internal/agent/state_test.go @@ -93,6 +93,38 @@ func TestNormalize_StripsFooter(t *testing.T) { } } +// TestNormalize_StripsCodexInputChevron pins the ultrareview bug_006 +// regression: the inputLine regex was hardcoded to claude's ❯ (U+276F) +// and ignored codex's › (U+203A). Typing into an idle codex pane +// flipped the normalized hash on every keystroke, and Detector.Observe +// returned StateThinking from its motion check before any idle marker +// matching ran. The TUI badge said ⚡ Thinking while the user was +// actively composing. +func TestNormalize_StripsCodexInputChevron(t *testing.T) { + // Two captures: empty codex prompt, then "what does this do?" + // typed in. After normalize() they must hash to the same thing + // (i.e. the chevron line is stripped) so the activity detector + // doesn't lie about codex being mid-thought. + empty := "boxed banner\n› \nfooter" + typed := "boxed banner\n› what does this do?\nfooter" + a, b := normalize(empty), normalize(typed) + if a != b { + t.Errorf("normalize() didn't strip codex › input line; "+ + "the badge will flip to Thinking while typing.\n empty: %q\n typed: %q", a, b) + } +} + +// TestNormalize_StripsClaudeInputChevron: same regression shape for +// claude's ❯, to defend against an over-eager rewrite of the regex. +func TestNormalize_StripsClaudeInputChevron(t *testing.T) { + empty := "claude header\n❯ \nfooter" + typed := "claude header\n❯ what does this do?\nfooter" + a, b := normalize(empty), normalize(typed) + if a != b { + t.Errorf("normalize() didn't strip claude ❯ input line.\n empty: %q\n typed: %q", a, b) + } +} + func TestNormalize_StableAcrossCosmeticChanges(t *testing.T) { // The same logical content with cosmetic-only differences must // normalize to the same string. This is the load-bearing property diff --git a/internal/workspace/agent_swap.go b/internal/workspace/agent_swap.go index 724b51a..211530e 100644 --- a/internal/workspace/agent_swap.go +++ b/internal/workspace/agent_swap.go @@ -168,6 +168,17 @@ func (m *Manager) SwapAgent(ctx context.Context, name, newType string) (*state.W // agentPaneCmd (which reads ws.CurrentAgent via currentAgent) sees // the new value when we call it. Persisted under WithLock for the // usual race-safety against concurrent state mutations. + // + // Initialize AgentLaunches[newType] to 0 if absent. This is + // load-bearing for the briefing path: BuildBriefing's + // `launchCountFor` (briefing.go) returns the legacy total + // AgentLaunchCount whenever AgentLaunches[agentType] is missing AND + // agentType matches ws.CurrentAgent — a migration-window fallback + // that breaks the first-swap case once we mutate CurrentAgent to + // newType but haven't yet recorded that newType has zero prior + // launches. Without the explicit zero, BuildBriefing returns "" + // for the first swap and the new agent spawns with no workspace + // context. (ultrareview bug_001, 2026-06-26.) var updated state.Workspace err = m.Store.WithLock(func(s *state.State) error { row, ferr := s.Find(m.Cfg.ProjectRoot, name) @@ -175,6 +186,12 @@ func (m *Manager) SwapAgent(ctx context.Context, name, newType string) (*state.W return ferr } row.CurrentAgent = newType + if row.AgentLaunches == nil { + row.AgentLaunches = map[string]int{} + } + if _, ok := row.AgentLaunches[newType]; !ok { + row.AgentLaunches[newType] = 0 + } updated = *row return nil })