diff --git a/CHANGELOG.md b/CHANGELOG.md index 775deee..c2cb294 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,42 @@ 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.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). + ## [0.21.17.0] - 2026-06-25 — remote hosts stop splitting each project into two headers On the Workspaces tab, a remote host like TOWER could show a project twice: first a lone `(main)` row under one `canopy` header, then the project's real workspaces under a second `canopy` header further down. Every project on the host did it, so the list read as twice as many projects as you have. 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 ddeaeea..a194d17 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.21.17.0 +0.22.0.0 diff --git a/cmd/canopy/agent.go b/cmd/canopy/agent.go new file mode 100644 index 0000000..6f11ecf --- /dev/null +++ b/cmd/canopy/agent.go @@ -0,0 +1,205 @@ +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 + } + + // 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 + // 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", + oldType, ws.CurrentAgent, 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) +} + +// 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. +// +// 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/agent/briefing.go b/internal/agent/briefing.go index 3231b42..a5fb836 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,39 @@ func BuildBriefing(ws state.Workspace, cfg *config.Config, hints []state.Hint) s return buildDelta(hints) } +// 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 ws.AgentLaunches == nil && 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..5a0f361 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,85 @@ 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. +// +// 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() + // 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, "codex": 0} + + // Now BuildBriefing is called for the codex spawn. + 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_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). @@ -89,7 +167,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 +193,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 +213,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 +234,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 +251,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 +262,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 +295,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 +316,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 +332,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 +347,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 +368,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 +383,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 +401,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 +429,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 +450,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 +467,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 +486,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..fc96646 --- /dev/null +++ b/internal/agent/classifier_codex.go @@ -0,0 +1,122 @@ +package agent + +import ( + "regexp" + "strings" +) + +// 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 } + +// 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. +// +// 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 { + // Strip trailing blank rows the visible pane preserves. + trimmed := strings.TrimRight(content, "\n\t ") + for _, p := range codexIdleMarkers { + if p.MatchString(trimmed) { + 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..dc91408 --- /dev/null +++ b/internal/agent/classifier_test.go @@ -0,0 +1,280 @@ +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. + // + // 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: true, + }, + } + 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") +} + +// 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/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..a048433 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 @@ -306,12 +330,24 @@ var spinnerLine = regexp.MustCompile(`(?i)(churned|baked|cooking|simmering|brewi // 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 f2dde86..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 @@ -175,20 +207,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/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 54b3f5c..3491a3b 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 { diff --git a/internal/workspace/agent_swap.go b/internal/workspace/agent_swap.go new file mode 100644 index 0000000..211530e --- /dev/null +++ b/internal/workspace/agent_swap.go @@ -0,0 +1,287 @@ +// 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. + // + // 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) + if ferr != nil { + 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 + }) + 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..8f2807f --- /dev/null +++ b/internal/workspace/agent_swap_test.go @@ -0,0 +1,409 @@ +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" +) + +// 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. + 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