From 4c8677768da7503060eed377df643a5cea7a1846 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 10 Sep 2026 12:59:47 -0700 Subject: [PATCH 1/5] Drive agent setup from the harness registry MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Setup knew its agents by name in five places: the BASECAMP_SETUP_AGENT selector arm (`case "claude", "codex":`) and its unknown-value message, the `setup agents` help text, agentBinaryPresent's switch on ids, and the wizard's escape hint. Each was a list to extend by hand, and the registry that already held every agent could have answered for all of them. AgentInfo gains FindBinary, so `setup agents` reads each agent's own lookup rather than a switch that assumes unknown ids have a binary. The selector's default arm asks harness.FindAgent and lists the accepted values from the registry; the help text and the escape hint read the same list. The shared skill's health predicate moves to harness as harness.BaselineSkillInstalled (with AgentSkillPath naming the file) so an agent's health check can be the very same stat that setup and doctor use, rather than a second reading of "installed". Two verdicts tighten in `setup agents`. Connected now means the handler succeeded and the checks pass, the verdict `setup ` already reached on its own: a check that passes despite a setup error is a conflict to report, not a connection. And the synthesized "binary not found" remediation applies only when the absence kept the agent from connecting — Claude's plugin is read from installed_plugins.json, so a machine with the plugin installed and no `claude` on PATH is connected, and was warned about anyway. The per-agent command and its summary stop saying "plugin" — `setup ` is "Connect to Basecamp" and answers "connected" / "not connected" — because the next agent to arrive has no plugin, and the Codex picker row's CODEX_HOME lookup becomes agentHomeSkillPath, a home-env-plus-default helper the next row can share. --- internal/commands/setup_agents_test.go | 33 ++++++++++ internal/commands/skill.go | 16 +++-- internal/commands/skill_test.go | 29 ++++++++- internal/commands/wizard.go | 7 ++- internal/commands/wizard_agents.go | 86 +++++++++++++++----------- internal/commands/wizard_test.go | 19 ++++-- internal/harness/agent.go | 5 ++ internal/harness/agent_test.go | 10 +-- internal/harness/claude.go | 9 +-- internal/harness/codex.go | 7 ++- internal/harness/harness.go | 39 ++++++++++++ 11 files changed, 195 insertions(+), 65 deletions(-) diff --git a/internal/commands/setup_agents_test.go b/internal/commands/setup_agents_test.go index 167e48195..44b6a042b 100644 --- a/internal/commands/setup_agents_test.go +++ b/internal/commands/setup_agents_test.go @@ -307,3 +307,36 @@ func TestSetupAgentsInvalidSelector(t *testing.T) { require.NotEmpty(t, env.Data.Warnings) assert.Contains(t, env.Data.Warnings[0], "frobnicate") } + +// The unknown-value warning names every accepted selector, read from the +// registry, so a new agent shows up in the message without anyone editing it. +func TestSetupAgentsInvalidSelectorListsEveryAgent(t *testing.T) { + emptyHome(t) + t.Setenv("BASECAMP_SETUP_AGENT", "frobnicate") + + env := runSetupAgentsJSON(t) + + require.NotEmpty(t, env.Data.Warnings) + assert.Contains(t, env.Data.Warnings[0], "expected claude, codex, all, or none") + assert.Equal(t, "claude, codex, all, or none", agentSelectorProse()) +} + +// A missing binary is remediation only when it kept the agent from +// connecting: Claude's plugin is read from installed_plugins.json, so a +// machine with the plugin already installed and no `claude` on PATH is +// connected, and `setup agents` says so without a "binary not found" warning. +func TestSetupAgentsNoBinaryWarningWhenAlreadyConnected(t *testing.T) { + home := emptyHome(t) + t.Setenv("BASECAMP_SETUP_AGENT", "claude") + pluginsDir := filepath.Join(home, ".claude", "plugins") + require.NoError(t, os.MkdirAll(pluginsDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(pluginsDir, "installed_plugins.json"), []byte(`{"version":2,"plugins":{"basecamp@37signals":[{"version":"1.0.0","scope":"user"}]}}`), 0o644)) + + env := runSetupAgentsJSON(t) + + require.Len(t, env.Data.Agents, 1) + assert.True(t, env.Data.Agents[0].PluginInstalled) + assert.Empty(t, env.Data.Warnings) + assert.Empty(t, env.Data.ManualCommands) + assert.Equal(t, "Installed baseline skill; connected Claude Code", env.Summary) +} diff --git a/internal/commands/skill.go b/internal/commands/skill.go index 027f3c415..56e779913 100644 --- a/internal/commands/skill.go +++ b/internal/commands/skill.go @@ -34,7 +34,7 @@ var skillLocations = []skillLocation{ {Name: "Claude Code (Project)", Path: ".claude/skills/basecamp/SKILL.md"}, {Name: "OpenCode (Global)", Path: "~/.config/opencode/skills/basecamp/SKILL.md"}, {Name: "OpenCode (Project)", Path: ".opencode/skills/basecamp/SKILL.md"}, - {Name: "Codex (Global)", Path: codexGlobalSkillPath()}, + {Name: "Codex (Global)", Path: agentHomeSkillPath("CODEX_HOME", "~/.codex")}, } // legacySkillLocations are paths an agent still reads but that we no longer @@ -314,12 +314,16 @@ func expandSkillPath(path string) string { return path } -func codexGlobalSkillPath() string { - codexHome := strings.TrimSpace(os.Getenv("CODEX_HOME")) - if codexHome == "" { - return "~/.codex/skills/basecamp/SKILL.md" +// agentHomeSkillPath is the skill's path under an agent's own home: $homeEnv +// when set, else defaultHome (tilde form, expanded at install time). Any +// agent that reads its home's skills directory and relocates that home with +// an environment variable is a picker row that differs only in these two. +func agentHomeSkillPath(homeEnv, defaultHome string) string { + agentHome := strings.TrimSpace(os.Getenv(homeEnv)) + if agentHome == "" { + return defaultHome + "/skills/basecamp/" + skillFilename } - return filepath.Join(codexHome, "skills", "basecamp", skillFilename) + return filepath.Join(agentHome, "skills", "basecamp", skillFilename) } // linkSkillToClaude creates a symlink at ~/.claude/skills/basecamp pointing to diff --git a/internal/commands/skill_test.go b/internal/commands/skill_test.go index 57c8a7771..65edcc923 100644 --- a/internal/commands/skill_test.go +++ b/internal/commands/skill_test.go @@ -227,8 +227,8 @@ func TestCopySkillFilesRejectsSubdirs(t *testing.T) { } // Pin the literals rather than deriving them, so a test can't mirror a typo the -// code has. Codex's entry is computed by codexGlobalSkillPath and covered -// separately. +// code has. Codex's entry is computed by agentHomeSkillPath and covered by +// TestAgentHomeSkillPath. // // These are install targets, not the full set of paths an agent reads. opencode // takes an optional plural throughout — its own table reads @@ -253,6 +253,31 @@ func TestSkillLocationsMatchAgentSearchPaths(t *testing.T) { } } +// Codex reads skills from its own home, which it relocates with CODEX_HOME; +// the picker row follows it. +func TestAgentHomeSkillPath(t *testing.T) { + for _, tc := range []struct{ name, env, home string }{ + {"Codex (Global)", "CODEX_HOME", "~/.codex"}, + } { + t.Run(tc.name, func(t *testing.T) { + t.Setenv(tc.env, "") + assert.Equal(t, tc.home+"/skills/basecamp/SKILL.md", agentHomeSkillPath(tc.env, tc.home)) + + override := t.TempDir() + t.Setenv(tc.env, " "+override+" ") + assert.Equal(t, filepath.Join(override, "skills", "basecamp", "SKILL.md"), agentHomeSkillPath(tc.env, tc.home)) + }) + } + + // The row the picker offers is at the default home. + t.Setenv("CODEX_HOME", "") + got := map[string]string{} + for _, loc := range skillLocations { + got[loc.Name] = loc.Path + } + assert.Equal(t, "~/.codex/skills/basecamp/SKILL.md", got["Codex (Global)"]) +} + // A wizard install written before #624 sits at opencode's singular path. // opencode still loads it, so dropping it from the refresh set does not break // the skill — it freezes it at the version that wrote it, which is worse than diff --git a/internal/commands/wizard.go b/internal/commands/wizard.go index 64c861169..d7bdba1a8 100644 --- a/internal/commands/wizard.go +++ b/internal/commands/wizard.go @@ -17,6 +17,7 @@ import ( "github.com/basecamp/basecamp-cli/internal/appctx" "github.com/basecamp/basecamp-cli/internal/auth" "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/harness" "github.com/basecamp/basecamp-cli/internal/output" "github.com/basecamp/basecamp-cli/internal/stdinarg" "github.com/basecamp/basecamp-cli/internal/tui" @@ -158,8 +159,8 @@ func runWizard(cmd *cobra.Command, app *appctx.App) error { // for the wizard by name here, so say so; isFirstRun asks the same question // and answers it differently — see wizardCanRun. // - // The gate belongs to this RunE alone: `setup claude`, `setup codex` and - // `setup agents` are the supported non-interactive paths and must keep + // The gate belongs to this RunE alone: `setup agents` and every per-agent + // `setup ` are the supported non-interactive paths and must keep // working, which a persistent hook here would have broken. if !setupCanRun(app) { return output.ErrUsageHint("basecamp setup needs an interactive terminal", wizardEscapeHint()) @@ -246,7 +247,7 @@ func runWizard(cmd *cobra.Command, app *appctx.App) error { // rather than restating that a terminal is missing. Modeled on stdinEscapeHint: // point at the real alternatives. func wizardEscapeHint() string { - return "Agent setup runs without a terminal: basecamp setup agents (or basecamp setup claude / basecamp setup codex). " + + return "Agent setup runs without a terminal: basecamp setup agents (or " + strings.Join(agentChoiceCommands(harness.AllAgents()), " / ") + "). " + "Set defaults directly with basecamp config set account_id (or basecamp accounts use ) and basecamp config set project_id . " + "Check authentication with basecamp auth status." } diff --git a/internal/commands/wizard_agents.go b/internal/commands/wizard_agents.go index fb5c3fb16..1eaf7c1e9 100644 --- a/internal/commands/wizard_agents.go +++ b/internal/commands/wizard_agents.go @@ -7,7 +7,6 @@ import ( "io" "os" "os/exec" - "path/filepath" "sort" "strings" "time" @@ -286,7 +285,7 @@ func wizardAgents(cmd *cobra.Command, styles *tui.Styles) (agentSetupOutcome, er preChecks := snapshotAgentChecks(agents) if detectedAgentsReady(preChecks) { for _, agent := range agents { - fmt.Fprintln(w, styles.RenderStatus(true, agent.Name+" plugin installed")) + fmt.Fprintln(w, styles.RenderStatus(true, agent.Name+" connected")) } fmt.Fprintln(w) return agentSetupOutcome{Detected: len(agents), Checks: preChecks}, nil @@ -574,8 +573,8 @@ func newSetupAgentCmds() []*cobra.Command { h := handler // capture cmds = append(cmds, &cobra.Command{ Use: agent.ID, - Short: fmt.Sprintf("Install the Basecamp plugin for %s", agent.Name), - Long: fmt.Sprintf("Set up the %s integration so %s can access Basecamp.", agent.Name, agent.Name), + Short: fmt.Sprintf("Connect %s to Basecamp", agent.Name), + Long: fmt.Sprintf("Install the Basecamp agent skill and set up the %s integration so %s can access Basecamp.", agent.Name, agent.Name), RunE: func(cmd *cobra.Command, args []string) error { app := appctx.FromContext(cmd.Context()) if app == nil { @@ -632,11 +631,11 @@ func newSetupAgentCmds() []*cobra.Command { } } - summary := agent.Name + " plugin installed" + summary := agent.Name + " connected" if !detected { summary = agent.Name + " not detected" } else if !installed { - summary = agent.Name + " plugin not installed" + summary = agent.Name + " not connected" } result := map[string]any{ @@ -648,7 +647,7 @@ func newSetupAgentCmds() []*cobra.Command { // If setup had errors, don't claim installed even if checks pass if installed { result["plugin_installed"] = false - summary = agent.Name + " plugin not installed" + summary = agent.Name + " not connected" } } if len(manualCommands) > 0 { @@ -677,9 +676,26 @@ func newSetupAgentCmds() []*cobra.Command { } // agentSetupEnv selects which coding agents `setup agents` targets. -// Values: claude | codex | all | none. Empty (unset) means auto-detect. +// Values: an agent id (claude | codex) | all | none. Empty (unset) means +// auto-detect. const agentSetupEnv = "BASECAMP_SETUP_AGENT" +// agentSelectorValues lists what agentSetupEnv accepts, for help and +// diagnostics: every registered agent id, then all and none. +func agentSelectorValues() []string { + var values []string + for _, agent := range harness.AllAgents() { + values = append(values, agent.ID) + } + return append(values, "all", "none") +} + +// agentSelectorProse renders agentSelectorValues as "claude, codex, all, or none". +func agentSelectorProse() string { + values := agentSelectorValues() + return strings.Join(values[:len(values)-1], ", ") + ", or " + values[len(values)-1] +} + // newSetupAgentsCmd builds `setup agents`. It always runs non-interactively: // it installs the baseline skill, connects agents per the BASECAMP_SETUP_AGENT // selector (or auto-detection), and emits a structured envelope. It never @@ -689,7 +705,7 @@ func newSetupAgentsCmd() *cobra.Command { Use: "agents", Short: "Install the Basecamp skill and connect detected coding agents", Long: "Install the baseline Basecamp agent skill and attempt to connect coding agents.\n\n" + - "Selection is controlled by " + agentSetupEnv + ": claude, codex, all, or none. When\n" + + "Selection is controlled by " + agentSetupEnv + ": " + agentSelectorProse() + ". When\n" + "unset, a single detected agent is connected; when several are detected none is\n" + "guessed — the per-agent `basecamp setup ` commands are surfaced instead.", // Selection is env-driven; positional args are always a mistake (typo, @@ -756,13 +772,13 @@ func runNonInteractiveAgentSetup(cmd *cobra.Command, app *appctx.App) error { targets = harness.AllAgents() case "none": // baseline skill only - case "claude", "codex": + default: if a := harness.FindAgent(selector); a != nil { targets = []harness.AgentInfo{*a} + } else { + selector = "invalid" + warnings = append(warnings, fmt.Sprintf("Unknown %s value %q; installed the baseline skill only (expected %s)", agentSetupEnv, selectorRaw, agentSelectorProse())) } - default: - selector = "invalid" - warnings = append(warnings, fmt.Sprintf("Unknown %s value %q; installed the baseline skill only (expected claude, codex, all, or none)", agentSetupEnv, selectorRaw)) } // Run handlers in id order so aggregation is deterministic. @@ -789,7 +805,7 @@ func runNonInteractiveAgentSetup(cmd *cobra.Command, app *appctx.App) error { } } - // manual_commands: ambiguous → both `setup `; else union of each + // manual_commands: ambiguous → every `setup `; else union of each // handler's own ordered sequence plus a synthesized hint for absent // binaries. Stable first-seen dedup preserves each handler's order. manualUnion := newOrderedStringSet() @@ -802,7 +818,7 @@ func runNonInteractiveAgentSetup(cmd *cobra.Command, app *appctx.App) error { for _, m := range r.manualCommands { manualUnion.add(m) } - if r.binaryAbsent { + if r.binaryAbsent && !r.pluginInstalled { manualUnion.add("basecamp setup " + r.id) } } @@ -810,9 +826,11 @@ func runNonInteractiveAgentSetup(cmd *cobra.Command, app *appctx.App) error { // warnings: synthesized missing-binary remediation, sorted-agent order. // The Claude handler treats a missing binary as no-op success while Codex - // returns an error, so synthesizing here keeps remediation symmetric. + // returns an error, so synthesizing here keeps remediation symmetric. Only + // when the absence actually prevented the connection — an agent whose + // integration is already healthy has nothing for the binary to fix. for _, r := range records { - if r.binaryAbsent { + if r.binaryAbsent && !r.pluginInstalled { warnings = append(warnings, fmt.Sprintf("%s: %s binary not found; install %s, then run: basecamp setup %s", r.id, r.name, r.name, r.id)) } } @@ -866,7 +884,7 @@ func runAgentSetupHandler(cmd *cobra.Command, agent harness.AgentInfo) agentSetu id: agent.ID, name: agent.Name, detectedBefore: agent.Detect != nil && agent.Detect(), - binaryAbsent: !agentBinaryPresent(agent.ID), + binaryAbsent: !agentBinaryPresent(agent), } if handler, ok := agentSetupHandlers[agent.ID]; ok && handler.RunNonInteractive != nil { @@ -880,21 +898,18 @@ func runAgentSetupHandler(cmd *cobra.Command, agent harness.AgentInfo) agentSetu } rec.detectedAfter = agent.Detect != nil && agent.Detect() - rec.pluginInstalled = agentChecksPass(agent) + // Connected means the handler succeeded AND health checks pass, the same + // verdict `setup ` reaches: a check that passes despite a setup error + // is a conflict to report, not a connection. + rec.pluginInstalled = len(rec.errors) == 0 && agentChecksPass(agent) return rec } -// agentBinaryPresent reports whether the agent's executable is on disk. -// Unknown agents are assumed present so no bogus remediation is synthesized. -func agentBinaryPresent(id string) bool { - switch id { - case "claude": - return harness.FindClaudeBinary() != "" - case "codex": - return harness.FindCodexBinary() != "" - default: - return true - } +// agentBinaryPresent reports whether the agent's executable is on disk. An +// agent with no executable to look for is assumed present so no bogus +// remediation is synthesized. +func agentBinaryPresent(agent harness.AgentInfo) bool { + return agent.FindBinary == nil || agent.FindBinary() != "" } // agentChecksPass reports whether every health check for the agent passes. @@ -993,14 +1008,11 @@ func orEmptyStrings(ss []string) []string { return ss } -// baselineSkillInstalled returns true if ~/.agents/skills/basecamp/SKILL.md exists. +// baselineSkillInstalled returns true if ~/.agents/skills/basecamp/SKILL.md +// exists. The predicate lives in harness so an agent's health check can be +// the same one. func baselineSkillInstalled() bool { - home, err := os.UserHomeDir() - if err != nil { - return false - } - _, err = os.Stat(filepath.Join(home, ".agents", "skills", "basecamp", "SKILL.md")) - return err == nil + return harness.BaselineSkillInstalled() } // joinNames joins names with commas and "and". diff --git a/internal/commands/wizard_test.go b/internal/commands/wizard_test.go index 7b5cecdda..78c6b2a73 100644 --- a/internal/commands/wizard_test.go +++ b/internal/commands/wizard_test.go @@ -666,9 +666,9 @@ func TestSetupClaudeSummaryStates(t *testing.T) { } else { installed, _ := envelope.Data["plugin_installed"].(bool) if installed { - assert.Equal(t, "Claude Code plugin installed", envelope.Summary) + assert.Equal(t, "Claude Code connected", envelope.Summary) } else { - assert.Equal(t, "Claude Code plugin not installed", envelope.Summary) + assert.Equal(t, "Claude Code not connected", envelope.Summary) } } } @@ -1198,11 +1198,18 @@ func TestSetupRefusesMachineOutputOnATerminal(t *testing.T) { } // TestSetupSubcommandsSurviveTheGate is the other half of the gate: it belongs -// to the parent's RunE only. `setup agents`, `setup claude` and `setup codex` -// are the supported non-interactive paths and must keep working off a terminal -// — a persistent hook here would have broken all three. +// to the parent's RunE only. `setup agents` and every `setup ` are the +// supported non-interactive paths and must keep working off a terminal — a +// persistent hook here would have broken all of them. func TestSetupSubcommandsSurviveTheGate(t *testing.T) { - for _, sub := range []string{"agents", "claude", "codex"} { + agents := harness.AllAgents() + subs := make([]string, 0, 1+len(agents)) + subs = append(subs, "agents") + for _, agent := range agents { + subs = append(subs, agent.ID) + } + require.Equal(t, []string{"agents", "claude", "codex"}, subs) + for _, sub := range subs { t.Run(sub, func(t *testing.T) { t.Setenv("BASECAMP_NO_KEYRING", "1") t.Setenv("HOME", t.TempDir()) diff --git a/internal/harness/agent.go b/internal/harness/agent.go index 1a3f686a3..17092246d 100644 --- a/internal/harness/agent.go +++ b/internal/harness/agent.go @@ -12,6 +12,11 @@ type AgentInfo struct { Detect func() bool // returns true if the agent is installed Checks func() []*StatusCheck // cheap health checks gating setup wizard behavior + // FindBinary returns the agent's executable path, or "" when it is not on + // disk. Setup reads it to decide whether a "binary not found" remediation + // applies; nil means the agent has no executable to look for. + FindBinary func() string + // Diagnostics returns the full doctor check suite, including checks that // are too slow or noisy for the wizard (e.g. version comparisons). // When nil, doctor falls back to Checks. diff --git a/internal/harness/agent_test.go b/internal/harness/agent_test.go index cd9d26155..8ba082203 100644 --- a/internal/harness/agent_test.go +++ b/internal/harness/agent_test.go @@ -88,15 +88,17 @@ func TestClaudeAgentInfoWiring(t *testing.T) { defer resetRegistry() RegisterAgent(AgentInfo{ - Name: "Claude Code", - ID: "claude", - Detect: DetectClaude, - Checks: func() []*StatusCheck { return []*StatusCheck{CheckClaudePlugin()} }, + Name: "Claude Code", + ID: "claude", + Detect: DetectClaude, + FindBinary: FindClaudeBinary, + Checks: func() []*StatusCheck { return []*StatusCheck{CheckClaudePlugin()} }, }) found := FindAgent("claude") require.NotNil(t, found) assert.Equal(t, "Claude Code", found.Name) assert.NotNil(t, found.Detect) + assert.NotNil(t, found.FindBinary) assert.NotNil(t, found.Checks) } diff --git a/internal/harness/claude.go b/internal/harness/claude.go index d94e900e9..baf61ccd2 100644 --- a/internal/harness/claude.go +++ b/internal/harness/claude.go @@ -13,10 +13,11 @@ import ( func init() { RegisterAgent(AgentInfo{ - Name: "Claude Code", - ID: "claude", - Detect: DetectClaude, - Checks: claudeChecks, + Name: "Claude Code", + ID: "claude", + Detect: DetectClaude, + FindBinary: FindClaudeBinary, + Checks: claudeChecks, Diagnostics: func(_ context.Context) []*StatusCheck { return append(claudeChecks(), CheckClaudePluginVersion()) }, diff --git a/internal/harness/codex.go b/internal/harness/codex.go index e1e298606..61d5a330b 100644 --- a/internal/harness/codex.go +++ b/internal/harness/codex.go @@ -62,9 +62,10 @@ type codexPluginState struct { func init() { RegisterAgent(AgentInfo{ - Name: "Codex", - ID: "codex", - Detect: DetectCodex, + Name: "Codex", + ID: "codex", + Detect: DetectCodex, + FindBinary: FindCodexBinary, Checks: func() []*StatusCheck { return []*StatusCheck{CheckCodexPlugin()} }, diff --git a/internal/harness/harness.go b/internal/harness/harness.go index 432a0215c..f60f1d976 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -1,6 +1,12 @@ // Package harness detects and checks AI agent integration health. package harness +import ( + "errors" + "os" + "path/filepath" +) + // StatusCheck represents a single agent integration health check result. type StatusCheck struct { Name string `json:"name"` @@ -8,3 +14,36 @@ type StatusCheck struct { Message string `json:"message"` Hint string `json:"hint,omitempty"` } + +// errNoHomeDir is the shared skill's state when its path cannot be built. +var errNoHomeDir = errors.New("cannot determine home directory") + +// AgentSkillPath returns the shared skill's path, +// ~/.agents/skills/basecamp/SKILL.md, or "" when the home directory cannot +// be determined. Every agent that reads the cross-agent skills directory +// finds the skill here. +func AgentSkillPath() string { + home, err := os.UserHomeDir() + if err != nil || home == "" { + return "" + } + return filepath.Join(filepath.Clean(home), ".agents", "skills", "basecamp", "SKILL.md") +} + +// BaselineSkillInstalled reports whether the shared skill is on disk. It is +// the one health predicate for the shared skill: setup and doctor answer +// from it, and so does any agent whose integration is the skill alone. +func BaselineSkillInstalled() bool { + return statAgentSkill() == nil +} + +// statAgentSkill is BaselineSkillInstalled with the reason: errNoHomeDir when +// the path cannot be built, the stat error when it can. +func statAgentSkill() error { + path := AgentSkillPath() + if path == "" { + return errNoHomeDir + } + _, err := os.Stat(path) + return err +} From 004ea1e47abacd4f910edd164f68283723f5d77b Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 10 Sep 2026 13:00:11 -0700 Subject: [PATCH 2/5] Connect Grok Build as a first-class coding agent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Register xAI's Grok Build in the agent harness, add `basecamp setup grok`, and include Grok in the skill picker, doctor, `setup agents`, the installers and the docs — a peer of Codex everywhere Codex appears. Grok has no Basecamp plugin. It reads user skills from ~/.grok/skills and from the cross-agent ~/.agents/skills, so the shared ~/.agents/skills/basecamp skill is the whole integration: `setup grok` installs it and confirms it is healthy, and doctor's "Grok Skill" check is skill presence — the same harness.BaselineSkillInstalled predicate setup and doctor already answer from. Nothing is copied into $GROK_HOME/skills; the picker's "Grok (Global)" row is there for someone who chooses that copy, as Codex's is, and refresh keeps it current like every other row. Detection is $GROK_HOME or ~/.grok, or a `grok` binary on PATH, in ~/.local/bin, or in $GROK_HOME/bin, where Grok Build's installers put it (the npm package honors $GROK_HOME/bin). Setup never fabricates the home directory: a machine without Grok gets "Grok not detected" and the shared skill, not a ~/.grok that would make every later detection lie. Rather than a second copy of codex.go, Grok is the first row of harness.SkillAgent — name, id, home env var, home directory, binary — whose methods (Detect, FindBinary, Home, CheckSkill) and registration are written once. Codex is deliberately not a row: it has a native plugin here, so its detection, setup and health stay its own. The command layer builds the setup handler for every row, the selector and doctor's remediation read the table, and the tests run once per row, so the next skill-only agent is a new row plus the prose lists in the installers and docs. --- .surface | 23 +++ AGENTS.md | 17 +- README.md | 10 +- e2e/installer.bats | 22 ++- e2e/setup.bats | 12 +- install.md | 14 +- internal/commands/doctor.go | 19 ++ internal/commands/doctor_test.go | 15 ++ internal/commands/setup_agents_test.go | 138 ++++++++++---- internal/commands/skill.go | 7 +- internal/commands/skill_test.go | 13 +- internal/commands/wizard_agents.go | 113 +++++++++--- internal/commands/wizard_skill_agent_test.go | 166 +++++++++++++++++ internal/commands/wizard_test.go | 2 +- internal/harness/harness.go | 4 +- internal/harness/skill_agent.go | 138 ++++++++++++++ internal/harness/skill_agent_test.go | 184 +++++++++++++++++++ scripts/install.ps1 | 8 +- scripts/install.sh | 10 +- skills/basecamp-doctor/SKILL.md | 1 + skills/basecamp/SKILL.md | 2 +- 21 files changed, 824 insertions(+), 94 deletions(-) create mode 100644 internal/commands/wizard_skill_agent_test.go create mode 100644 internal/harness/skill_agent.go create mode 100644 internal/harness/skill_agent_test.go diff --git a/.surface b/.surface index c7ff82bee..f2d12a980 100644 --- a/.surface +++ b/.surface @@ -991,6 +991,7 @@ CMD basecamp setup CMD basecamp setup agents CMD basecamp setup claude CMD basecamp setup codex +CMD basecamp setup grok CMD basecamp show CMD basecamp skill CMD basecamp skill install @@ -13205,6 +13206,27 @@ FLAG basecamp setup codex --stats type=bool FLAG basecamp setup codex --styled type=bool FLAG basecamp setup codex --todolist type=string FLAG basecamp setup codex --verbose type=count +FLAG basecamp setup grok --account type=string +FLAG basecamp setup grok --agent type=bool +FLAG basecamp setup grok --cache-dir type=string +FLAG basecamp setup grok --count type=bool +FLAG basecamp setup grok --help type=bool +FLAG basecamp setup grok --hints type=bool +FLAG basecamp setup grok --ids-only type=bool +FLAG basecamp setup grok --in type=string +FLAG basecamp setup grok --jq type=string +FLAG basecamp setup grok --json type=bool +FLAG basecamp setup grok --markdown type=bool +FLAG basecamp setup grok --md type=bool +FLAG basecamp setup grok --no-hints type=bool +FLAG basecamp setup grok --no-stats type=bool +FLAG basecamp setup grok --profile type=string +FLAG basecamp setup grok --project type=string +FLAG basecamp setup grok --quiet type=bool +FLAG basecamp setup grok --stats type=bool +FLAG basecamp setup grok --styled type=bool +FLAG basecamp setup grok --todolist type=string +FLAG basecamp setup grok --verbose type=count FLAG basecamp show --account type=string FLAG basecamp show --agent type=bool FLAG basecamp show --all-comments type=bool @@ -18658,6 +18680,7 @@ SUB basecamp setup SUB basecamp setup agents SUB basecamp setup claude SUB basecamp setup codex +SUB basecamp setup grok SUB basecamp show SUB basecamp skill SUB basecamp skill install diff --git a/AGENTS.md b/AGENTS.md index 9022ddc68..9b83a7b68 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -33,11 +33,26 @@ basecamp-cli/ │ └── version/ # Version info ├── e2e/ # BATS integration tests ├── skills/ # Agent skills -├── hooks/ # Agent lifecycle hooks (both agents) +├── hooks/ # Agent lifecycle hooks (both plugin agents) ├── .claude-plugin/ # Claude Code integration └── .codex-plugin/ # Codex plugin manifest ``` +## Coding-agent integrations + +Coding-agent integration lives in `internal/harness` (agent registry, detection, plugin and +skill health checks) and `internal/commands/wizard_agents.go` (`basecamp setup +claude|codex|grok|agents`). Claude Code and Codex each get a native plugin from the +`basecamp/claude-plugins` marketplace and have registrations of their own (`claude.go`, +`codex.go`). Grok Build has no plugin: it reads the shared `~/.agents/skills/basecamp` skill +directly, so it is a row of `harness.SkillAgent` (name, id, home env var, home directory, +binary) in `skill_agent.go`, and everything in `internal/commands` that touches a shared-skill +agent — the setup handler, the `BASECAMP_SETUP_AGENT` values, doctor's remediation — loops over +`harness.SkillAgents()` rather than naming it. A new shared-skill agent is a new row; the skill +picker's `(Global)` row, the prose lists in the installers and the docs are the places to update +by hand. `setup ` never fabricates an agent's home directory: a skill-only agent that is not +detected is reported missing, not created. + ## Basecamp API Reference API documentation: https://github.com/basecamp/bc3-api diff --git a/README.md b/README.md index 6437ac5f2..96b547f1f 100644 --- a/README.md +++ b/README.md @@ -2,10 +2,10 @@ `basecamp` is the official command-line interface for Basecamp. Manage projects, todos, messages, and more from your terminal or through AI agents. -- Works standalone or with any AI agent (Claude, Codex, Copilot, Gemini) +- Works standalone or with any AI agent (Claude, Codex, Grok, Copilot, Gemini) - JSON output with breadcrumbs for easy navigation - OAuth authentication with automatic token refresh -- Includes agent skills plus native Claude Code and Codex plugins +- Includes agent skills plus native Claude Code and Codex plugins; Grok Build reads the shared skill directly ## Quick Start @@ -107,7 +107,7 @@ The first interactive `basecamp` run applies the recommended setup automatically - Account granted by OAuth, otherwise the existing configured account or first available account, saved globally - No global default project; directory-specific and environment project settings continue to apply -- Every detected Claude Code or Codex integration +- Every detected Claude Code, Codex or Grok integration Run the same setup directly with `basecamp setup`. To choose the account, default project, config scope, and agent integrations, run: @@ -278,6 +278,8 @@ codex plugin add basecamp@37signals To pick up a newer plugin version later, refresh the marketplace with `codex plugin marketplace upgrade 37signals` (or re-run `basecamp setup codex`). +**Grok Build:** `basecamp setup grok` — installs the shared skill and confirms it is healthy. There is no Grok plugin: Grok reads user skills from `~/.grok/skills/` and from the cross-agent `~/.agents/skills/`, so the shared `~/.agents/skills/basecamp` skill is the whole integration. Grok is detected by `$GROK_HOME` (default `~/.grok`) or a `grok` binary on `PATH`, in `~/.local/bin`, or in `$GROK_HOME/bin` where its installers put it. Start a new Grok session after setup to load the skill. + **Other agents:** Point your agent at [`skills/basecamp/SKILL.md`](skills/basecamp/SKILL.md) for Basecamp workflow coverage. **Agent discovery:** Every command supports `--help --agent` for structured JSON output (flags, gotchas, subcommands). Use `basecamp commands --json` for the full catalog. @@ -335,7 +337,7 @@ client-registration flow) is obsolete and safe to delete. ```bash basecamp doctor # Check CLI health and diagnose issues basecamp doctor --verbose # Verbose output with details -basecamp doctor --json # Structured checks, including Claude and Codex +basecamp doctor --json # Structured checks, including Claude, Codex and Grok ``` ### Windows: Smart App Control and SmartScreen diff --git a/e2e/installer.bats b/e2e/installer.bats index 71042eeef..b47d5baec 100644 --- a/e2e/installer.bats +++ b/e2e/installer.bats @@ -199,6 +199,7 @@ EOF [[ "$status" -eq 0 ]] [[ "$output" == *"setup claude"* ]] [[ "$output" != *"setup codex"* ]] # codex unadvertised → never invoked + [[ "$output" != *"setup grok"* ]] # grok likewise } # Explicit `codex` on an old binary that lacks `setup codex` must NOT run the @@ -212,6 +213,25 @@ EOF [[ "$output" != *"setup codex"* ]] } +# Grok is the same explicit-selector shape as codex: an old binary that does +# not advertise `setup grok` degrades to the shared skill. +@test "old binary + BASECAMP_SETUP_AGENT=grok degrades to 'skill install', never 'setup grok'" { + write_stub old + run_post_install_setup "export BASECAMP_SETUP_AGENT=grok" + [[ "$status" -eq 0 ]] + [[ "$output" == *"skill install"* ]] + [[ "$output" != *"setup grok"* ]] +} + +# A new binary owns the selector: the installer hands every value, grok +# included, to `setup agents` rather than dispatching per agent itself. +@test "new binary + BASECAMP_SETUP_AGENT=grok dispatches to 'setup agents'" { + run_post_install_setup "export BASECAMP_SETUP_AGENT=grok" + [[ "$status" -eq 0 ]] + [[ "$output" == *"setup agents"* ]] + [[ "$output" != *"setup grok"* ]] +} + @test "install.sh has no residual 'setup claude' dispatch" { # `setup claude` may appear only inside the explicit-selector fallback case. run grep -n 'setup claude' "$INSTALL_SH" @@ -235,7 +255,7 @@ EOF grep -q 'setup agents' "$INSTALL_PS1" grep -q 'skill install' "$INSTALL_PS1" grep -q 'catch {' "$INSTALL_PS1" - # Explicit claude|codex selectors must be capability-checked before dispatch, + # Explicit claude|codex|grok selectors must be capability-checked before dispatch, # so an old binary never gets an unadvertised subcommand as a stray arg. grep -qF 'match "(?m)^\s+$selector\s"' "$INSTALL_PS1" # The keyring escape hatch belt (see the BASECAMP_NO_KEYRING tests below). diff --git a/e2e/setup.bats b/e2e/setup.bats index c455b13be..535dd70a1 100644 --- a/e2e/setup.bats +++ b/e2e/setup.bats @@ -188,7 +188,7 @@ run_in_pty() { # non-interactive paths and have to keep working — a persistent hook would have # taken all of them out, which is the easiest thing to get wrong here. -# hide_agent_binaries drops the developer's real claude/codex from PATH. Without +# hide_agent_binaries drops the developer's real claude/codex/grok from PATH. Without # it these tests shell out to whichever agent CLI happens to be installed, and # those have prompts of their own — a hang in somebody else's tool, unrelated to # the gate under test. What we are asserting is that the parent's gate does not @@ -227,3 +227,13 @@ hide_agent_binaries() { assert_not_timed_out assert_success } + +@test "setup grok still runs without a terminal" { + create_credentials + create_global_config '{"account_id": 99999}' + hide_agent_binaries + + run_guarded "basecamp setup grok --json < /dev/null" + assert_not_timed_out + assert_success +} diff --git a/install.md b/install.md index 6ab458d68..ae9e0e2ca 100644 --- a/install.md +++ b/install.md @@ -32,7 +32,7 @@ irm https://raw.githubusercontent.com/basecamp/basecamp-cli/main/scripts/install > **Note:** The install scripts run `basecamp setup` whenever they can attach it to a usable interactive terminal, including the standard `curl | bash` command. When no usable controlling terminal is available, output is redirected, or `BASECAMP_NONINTERACTIVE=1`/`true` is set, they skip authentication and run `basecamp setup agents`. That command installs the baseline agent skill and **attempts to connect** a single detected coding agent (best effort). If several agents are detected, or none is, only the baseline skill is installed and the per-agent commands are surfaced. Explicitly skipping first-time setup with `BASECAMP_SKIP_SETUP=1` still runs `setup agents`. If optional first-time setup is cancelled or does not finish, the installed CLI remains ready and the installer prints the command to resume setup. > -> Choose which agent to connect with `BASECAMP_SETUP_AGENT` (`claude`, `codex`, `all`, or `none`). Set it for the interpreter, not the fetch: +> Choose which agent to connect with `BASECAMP_SETUP_AGENT` (`claude`, `codex`, `grok`, `all`, or `none`). Set it for the interpreter, not the fetch: > - Bash: `curl -fsSL https://basecamp.com/install-cli | BASECAMP_SETUP_AGENT=codex bash` > - PowerShell: `$env:BASECAMP_SETUP_AGENT='codex'; irm https://raw.githubusercontent.com/basecamp/basecamp-cli/main/scripts/install.ps1 | iex` > @@ -131,7 +131,7 @@ EXECUTE NOW: Start with Step 1. Mark TODO items complete as you go. Stop when `b **Do not execute this section unless explicitly requested.** The core installation is complete when DONE WHEN passes. -Interactive setup in Step 1 connects every detected agent. Without a controlling terminal, the installer still installs the baseline skill and attempts to connect one detected agent. Run the commands here to force a specific agent, repair a connection, or connect agents that were not available during installation. Both agent plugins require the `basecamp` CLI installed above — the plugin invokes it for every Basecamp operation. +Interactive setup in Step 1 connects every detected agent. Without a controlling terminal, the installer still installs the baseline skill and attempts to connect one detected agent. Run the commands here to force a specific agent, repair a connection, or connect agents that were not available during installation. Every integration requires the `basecamp` CLI installed above — the plugins and the skill invoke it for every Basecamp operation. ### Claude Code @@ -163,7 +163,15 @@ codex plugin add basecamp@37signals To pick up a newer plugin version later, refresh with `codex plugin marketplace upgrade 37signals` (or re-run `basecamp setup codex`). -Verify either agent integration with structured diagnostics: +### Grok Build + +```bash +basecamp setup grok +``` + +This installs the shared Basecamp skill and confirms it is healthy. There is no Grok plugin: Grok reads user skills from `~/.grok/skills/` and from the cross-agent `~/.agents/skills/`, so the shared skill at `~/.agents/skills/basecamp/SKILL.md` is the whole integration. Setup requires Grok to be present — `$GROK_HOME` (default `~/.grok`) or a `grok` binary on `PATH`, in `~/.local/bin`, or in `$GROK_HOME/bin` — and never creates the Grok home itself. Start a new Grok session afterwards to load the skill. + +Verify any agent integration with structured diagnostics: ```bash basecamp doctor --json diff --git a/internal/commands/doctor.go b/internal/commands/doctor.go index 0d8f2ad10..f3f6c31e4 100644 --- a/internal/commands/doctor.go +++ b/internal/commands/doctor.go @@ -1136,6 +1136,14 @@ func buildDoctorBreadcrumbs(checks []Check) []output.Breadcrumb { Cmd: "basecamp setup codex", Description: "Install or update the Codex plugin", }) + default: + if agent, ok := skillAgentForCheck(c.Name); ok { + breadcrumbs = append(breadcrumbs, output.Breadcrumb{ + Action: "setup_" + agent.ID, + Cmd: "basecamp setup " + agent.ID, + Description: "Install the shared Basecamp skill for " + agent.Name, + }) + } } } @@ -1152,6 +1160,17 @@ func buildDoctorBreadcrumbs(checks []Check) []output.Breadcrumb { return unique } +// skillAgentForCheck returns the shared-skill agent whose check is named, so +// its remediation reads the harness table rather than a case per agent. +func skillAgentForCheck(name string) (harness.SkillAgent, bool) { + for _, agent := range harness.SkillAgents() { + if name == agent.Name+" Skill" { + return agent, true + } + } + return harness.SkillAgent{}, false +} + // pluralize returns singular or plural form based on count. func pluralize(n int, singular, plural string) string { if n == 1 { diff --git a/internal/commands/doctor_test.go b/internal/commands/doctor_test.go index 54f7d15fc..b413113b9 100644 --- a/internal/commands/doctor_test.go +++ b/internal/commands/doctor_test.go @@ -834,6 +834,21 @@ func TestBuildDoctorBreadcrumbs_Codex(t *testing.T) { assert.Equal(t, "basecamp setup codex", breadcrumbs[0].Cmd) } +// A shared-skill agent's failing skill check remediates with its own +// `setup `, read from the harness table rather than a case per agent. +func TestBuildDoctorBreadcrumbs_SkillAgents(t *testing.T) { + for _, agent := range harness.SkillAgents() { + t.Run(agent.ID, func(t *testing.T) { + breadcrumbs := buildDoctorBreadcrumbs([]Check{{Name: agent.Name + " Skill", Status: "fail"}}) + + require.Len(t, breadcrumbs, 1) + assert.Equal(t, "setup_"+agent.ID, breadcrumbs[0].Action) + assert.Equal(t, "basecamp setup "+agent.ID, breadcrumbs[0].Cmd) + }) + } + assert.Empty(t, buildDoctorBreadcrumbs([]Check{{Name: "Grok Skill", Status: "pass"}}), "a passing check needs no remediation") +} + func TestCheckLegacyInstall_SkipsKeyringWhenNoKeyring(t *testing.T) { t.Setenv("BASECAMP_NO_KEYRING", "1") t.Setenv("XDG_CACHE_HOME", t.TempDir()) diff --git a/internal/commands/setup_agents_test.go b/internal/commands/setup_agents_test.go index 44b6a042b..a7d3a8314 100644 --- a/internal/commands/setup_agents_test.go +++ b/internal/commands/setup_agents_test.go @@ -12,6 +12,7 @@ import ( "github.com/stretchr/testify/require" "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/harness" "github.com/basecamp/basecamp-cli/internal/output" ) @@ -191,11 +192,12 @@ func TestSetupAgentsAllForcesEveryHandler(t *testing.T) { env := runSetupAgentsJSON(t) assert.Equal(t, "all", env.Data.Selector) - assert.Equal(t, []string{"claude", "codex"}, env.Data.AttemptedAgents) - // Both binaries absent → symmetric synthesized remediation. + assert.Equal(t, []string{"claude", "codex", "grok"}, env.Data.AttemptedAgents) + // Every binary absent → symmetric synthesized remediation. assert.Contains(t, env.Data.ManualCommands, "basecamp setup claude") assert.Contains(t, env.Data.ManualCommands, "basecamp setup codex") - require.GreaterOrEqual(t, len(env.Data.Warnings), 2) + assert.Contains(t, env.Data.ManualCommands, "basecamp setup grok") + require.GreaterOrEqual(t, len(env.Data.Warnings), 3) }) t.Run("one detected", func(t *testing.T) { @@ -204,7 +206,7 @@ func TestSetupAgentsAllForcesEveryHandler(t *testing.T) { env := runSetupAgentsJSON(t) - assert.Equal(t, []string{"claude", "codex"}, env.Data.AttemptedAgents) + assert.Equal(t, []string{"claude", "codex", "grok"}, env.Data.AttemptedAgents) // Claude binary absent → synthesized; codex present and healthy → not. assert.Contains(t, env.Data.ManualCommands, "basecamp setup claude") assert.NotContains(t, env.Data.ManualCommands, "basecamp setup codex") @@ -219,12 +221,105 @@ func TestSetupAgentsAllForcesEveryHandler(t *testing.T) { env := runSetupAgentsJSON(t) assert.False(t, env.Data.Ambiguous, "explicit selector is never ambiguous") - assert.Equal(t, []string{"claude", "codex"}, env.Data.AttemptedAgents) + assert.Equal(t, []string{"claude", "codex", "grok"}, env.Data.AttemptedAgents) assert.Contains(t, env.Data.ManualCommands, "basecamp setup claude") assert.Contains(t, env.Data.ManualCommands, "basecamp setup codex") }) } +// forEachSkillAgent runs a test once per shared-skill agent: their setup is +// one code path, so their coverage is one test. +func forEachSkillAgent(t *testing.T, test func(t *testing.T, agent harness.SkillAgent)) { + t.Helper() + for _, agent := range harness.SkillAgents() { + t.Run(agent.ID, func(t *testing.T) { test(t, agent) }) + } +} + +// A shared-skill agent detected by its home directory alone connects: its +// whole integration is the skill `setup agents` just installed, so no binary +// is needed and no missing-binary remediation is synthesized. +func TestSetupAgentsSingleDetectedSkillAgent(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent harness.SkillAgent) { + home := emptyHome(t) + t.Setenv(agent.HomeEnv, "") + require.NoError(t, os.MkdirAll(filepath.Join(home, agent.HomeDir), 0o755)) + t.Setenv("BASECAMP_SETUP_AGENT", "") + + env := runSetupAgentsJSON(t) + + assert.Equal(t, "auto", env.Data.Selector) + assert.False(t, env.Data.Ambiguous) + assert.Equal(t, []string{agent.ID}, env.Data.AttemptedAgents) + require.Len(t, env.Data.Agents, 1) + assert.Equal(t, agent.ID, env.Data.Agents[0].ID) + assert.True(t, env.Data.Agents[0].DetectedBefore) + assert.True(t, env.Data.Agents[0].DetectedAfter) + assert.True(t, env.Data.Agents[0].PluginInstalled) + assert.Empty(t, env.Data.Errors) + assert.Empty(t, env.Data.Warnings) + assert.Empty(t, env.Data.ManualCommands) + assert.Equal(t, "Installed baseline skill; connected "+agent.Name, env.Summary) + }) +} + +// The selector accepts every registered id, a shared-skill agent's included. +func TestSetupAgentsSkillAgentSelector(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent harness.SkillAgent) { + home := emptyHome(t) + t.Setenv(agent.HomeEnv, "") + require.NoError(t, os.MkdirAll(filepath.Join(home, agent.HomeDir), 0o755)) + require.NoError(t, os.MkdirAll(filepath.Join(home, ".codex"), 0o755)) + t.Setenv("BASECAMP_SETUP_AGENT", agent.ID) + + env := runSetupAgentsJSON(t) + + assert.Equal(t, agent.ID, env.Data.Selector) + assert.False(t, env.Data.Ambiguous, "an explicit selector is never ambiguous") + assert.Equal(t, []string{agent.ID}, env.Data.AttemptedAgents) + assert.Equal(t, []string{"codex", agent.ID}, env.Data.DetectedBefore) + assert.Empty(t, env.Data.Errors) + }) +} + +// An explicitly selected shared-skill agent that is not on the machine is a +// failed connection with the agent's own remediation — and its home directory +// is never fabricated to make the next detection lie. +func TestSetupAgentsSkillAgentNotDetected(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent harness.SkillAgent) { + home := emptyHome(t) + t.Setenv(agent.HomeEnv, "") + t.Setenv("BASECAMP_SETUP_AGENT", agent.ID) + + env := runSetupAgentsJSON(t) + + assert.Equal(t, agent.ID, env.Data.Selector) + assert.True(t, env.Data.SkillInstalled, "the shared skill is installed regardless") + require.Len(t, env.Data.Agents, 1) + assert.False(t, env.Data.Agents[0].DetectedBefore) + assert.False(t, env.Data.Agents[0].DetectedAfter) + assert.False(t, env.Data.Agents[0].PluginInstalled, "a passing skill check is not a connection when the handler refused") + require.NotEmpty(t, env.Data.Errors) + assert.Contains(t, env.Data.Errors[0], agent.ID+": "+agent.Name+" not detected") + assert.Equal(t, []string{"basecamp setup " + agent.ID}, env.Data.ManualCommands) + assert.Equal(t, "Installed baseline skill; attempted "+agent.Name, env.Summary) + assert.NoFileExists(t, filepath.Join(home, agent.HomeDir)) + }) +} + +// The unknown-value warning names every accepted selector, so a new agent row +// shows up in the message without anyone editing it. +func TestSetupAgentsInvalidSelectorListsEveryAgent(t *testing.T) { + emptyHome(t) + t.Setenv("BASECAMP_SETUP_AGENT", "frobnicate") + + env := runSetupAgentsJSON(t) + + require.NotEmpty(t, env.Data.Warnings) + assert.Contains(t, env.Data.Warnings[0], "expected claude, codex, grok, all, or none") + assert.Equal(t, "claude, codex, grok, all, or none", agentSelectorProse()) +} + // TestSetupAgentsCodexMissingBinary asserts the real missing-binary contract: // the agentSetupError is unioned into top-level errors + a warning, and the // deduped remediation is the single `basecamp setup codex` (not the 3-command seq). @@ -307,36 +402,3 @@ func TestSetupAgentsInvalidSelector(t *testing.T) { require.NotEmpty(t, env.Data.Warnings) assert.Contains(t, env.Data.Warnings[0], "frobnicate") } - -// The unknown-value warning names every accepted selector, read from the -// registry, so a new agent shows up in the message without anyone editing it. -func TestSetupAgentsInvalidSelectorListsEveryAgent(t *testing.T) { - emptyHome(t) - t.Setenv("BASECAMP_SETUP_AGENT", "frobnicate") - - env := runSetupAgentsJSON(t) - - require.NotEmpty(t, env.Data.Warnings) - assert.Contains(t, env.Data.Warnings[0], "expected claude, codex, all, or none") - assert.Equal(t, "claude, codex, all, or none", agentSelectorProse()) -} - -// A missing binary is remediation only when it kept the agent from -// connecting: Claude's plugin is read from installed_plugins.json, so a -// machine with the plugin already installed and no `claude` on PATH is -// connected, and `setup agents` says so without a "binary not found" warning. -func TestSetupAgentsNoBinaryWarningWhenAlreadyConnected(t *testing.T) { - home := emptyHome(t) - t.Setenv("BASECAMP_SETUP_AGENT", "claude") - pluginsDir := filepath.Join(home, ".claude", "plugins") - require.NoError(t, os.MkdirAll(pluginsDir, 0o755)) - require.NoError(t, os.WriteFile(filepath.Join(pluginsDir, "installed_plugins.json"), []byte(`{"version":2,"plugins":{"basecamp@37signals":[{"version":"1.0.0","scope":"user"}]}}`), 0o644)) - - env := runSetupAgentsJSON(t) - - require.Len(t, env.Data.Agents, 1) - assert.True(t, env.Data.Agents[0].PluginInstalled) - assert.Empty(t, env.Data.Warnings) - assert.Empty(t, env.Data.ManualCommands) - assert.Equal(t, "Installed baseline skill; connected Claude Code", env.Summary) -} diff --git a/internal/commands/skill.go b/internal/commands/skill.go index 56e779913..6c90e9217 100644 --- a/internal/commands/skill.go +++ b/internal/commands/skill.go @@ -35,6 +35,7 @@ var skillLocations = []skillLocation{ {Name: "OpenCode (Global)", Path: "~/.config/opencode/skills/basecamp/SKILL.md"}, {Name: "OpenCode (Project)", Path: ".opencode/skills/basecamp/SKILL.md"}, {Name: "Codex (Global)", Path: agentHomeSkillPath("CODEX_HOME", "~/.codex")}, + {Name: "Grok (Global)", Path: agentHomeSkillPath("GROK_HOME", "~/.grok")}, } // legacySkillLocations are paths an agent still reads but that we no longer @@ -315,9 +316,9 @@ func expandSkillPath(path string) string { } // agentHomeSkillPath is the skill's path under an agent's own home: $homeEnv -// when set, else defaultHome (tilde form, expanded at install time). Any -// agent that reads its home's skills directory and relocates that home with -// an environment variable is a picker row that differs only in these two. +// when set, else defaultHome (tilde form, expanded at install time). Codex and +// Grok both read their home's skills directory and both relocate it with an +// environment variable, so their picker rows differ only in these two values. func agentHomeSkillPath(homeEnv, defaultHome string) string { agentHome := strings.TrimSpace(os.Getenv(homeEnv)) if agentHome == "" { diff --git a/internal/commands/skill_test.go b/internal/commands/skill_test.go index 65edcc923..5cc3aeead 100644 --- a/internal/commands/skill_test.go +++ b/internal/commands/skill_test.go @@ -227,8 +227,8 @@ func TestCopySkillFilesRejectsSubdirs(t *testing.T) { } // Pin the literals rather than deriving them, so a test can't mirror a typo the -// code has. Codex's entry is computed by agentHomeSkillPath and covered by -// TestAgentHomeSkillPath. +// code has. The Codex and Grok entries are computed by agentHomeSkillPath and +// covered by TestAgentHomeSkillPath. // // These are install targets, not the full set of paths an agent reads. opencode // takes an optional plural throughout — its own table reads @@ -253,11 +253,12 @@ func TestSkillLocationsMatchAgentSearchPaths(t *testing.T) { } } -// Codex reads skills from its own home, which it relocates with CODEX_HOME; -// the picker row follows it. +// Codex and Grok read skills from their own home, which each relocates with +// an environment variable; the picker row follows it. func TestAgentHomeSkillPath(t *testing.T) { for _, tc := range []struct{ name, env, home string }{ {"Codex (Global)", "CODEX_HOME", "~/.codex"}, + {"Grok (Global)", "GROK_HOME", "~/.grok"}, } { t.Run(tc.name, func(t *testing.T) { t.Setenv(tc.env, "") @@ -269,13 +270,15 @@ func TestAgentHomeSkillPath(t *testing.T) { }) } - // The row the picker offers is at the default home. + // The rows the picker offers are those two, at their default homes. t.Setenv("CODEX_HOME", "") + t.Setenv("GROK_HOME", "") got := map[string]string{} for _, loc := range skillLocations { got[loc.Name] = loc.Path } assert.Equal(t, "~/.codex/skills/basecamp/SKILL.md", got["Codex (Global)"]) + assert.Equal(t, "~/.grok/skills/basecamp/SKILL.md", got["Grok (Global)"]) } // A wizard install written before #624 sits at opencode's singular path. diff --git a/internal/commands/wizard_agents.go b/internal/commands/wizard_agents.go index 1eaf7c1e9..e4d2571a4 100644 --- a/internal/commands/wizard_agents.go +++ b/internal/commands/wizard_agents.go @@ -120,24 +120,85 @@ func refreshClaudeMarketplace(parent context.Context, claudePath string, stdout, _ = cmd.Run() } -// agentSetupHandlers maps agent ID → setup handler. -var agentSetupHandlers = map[string]agentSetupHandler{ - "claude": { - Labels: []string{ - "Add basecamp/claude-plugins marketplace to Claude Code", - "Install the basecamp plugin for Claude Code", +// agentSetupHandlers maps agent ID → setup handler. The two plugin agents are +// written out; every shared-skill agent's comes from the harness table. +var agentSetupHandlers = agentSetupHandlersFor(harness.SkillAgents()) + +func agentSetupHandlersFor(skillAgents []harness.SkillAgent) map[string]agentSetupHandler { + handlers := map[string]agentSetupHandler{ + "claude": { + Labels: []string{ + "Add basecamp/claude-plugins marketplace to Claude Code", + "Install the basecamp plugin for Claude Code", + }, + Run: runClaudeSetup, + RunNonInteractive: runClaudeSetupNonInteractive, }, - Run: runClaudeSetup, - RunNonInteractive: runClaudeSetupNonInteractive, - }, - "codex": { + "codex": { + Labels: []string{ + "Add the 37signals marketplace to Codex", + "Install the basecamp plugin for Codex", + }, + Run: runCodexSetup, + RunNonInteractive: runCodexSetupNonInteractive, + }, + } + for _, agent := range skillAgents { + handlers[agent.ID] = skillAgentSetupHandler(agent) + } + return handlers +} + +// skillAgentSetupHandler builds the handler for an agent that reads the +// shared skill directly: basecamp-cli has no plugin for it, so the one step +// is confirming the shared skill is in place. +func skillAgentSetupHandler(agent harness.SkillAgent) agentSetupHandler { + return agentSetupHandler{ Labels: []string{ - "Add the 37signals marketplace to Codex", - "Install the basecamp plugin for Codex", + "Install the shared Basecamp skill for " + agent.Name, + }, + // Interactive: print progress, warn and continue — like Codex, a + // failure here never aborts `basecamp setup`. + Run: func(cmd *cobra.Command, styles *tui.Styles) error { + w := cmd.OutOrStdout() + path, err := installSkillAgentSkill(agent) + if err != nil { + fmt.Fprintln(w, styles.Warning.Render(" "+agent.Name+" skill setup failed: "+err.Error())) + fmt.Fprintln(w, styles.Muted.Render(" Then verify with: basecamp doctor")) + return nil //nolint:nilerr // warn and continue; the post-setup snapshot reports the failure + } + fmt.Fprintln(w, styles.RenderStatus(true, agent.Name+" skill installed ("+path+")")) + fmt.Fprintln(w, styles.Muted.Render(" Start a new "+agent.Name+" session to load the Basecamp skill.")) + return nil + }, + RunNonInteractive: func(*cobra.Command) error { + _, err := installSkillAgentSkill(agent) + return err }, - Run: runCodexSetup, - RunNonInteractive: runCodexSetupNonInteractive, - }, + } +} + +// installSkillAgentSkill is a shared-skill agent's one step. The caller has +// installed the shared skill already; this confirms it is healthy for an +// agent that is actually present. Like Claude and Codex, it never fabricates +// the agent: creating its home on a machine without it would make every +// later detection — and this command's own verdict — report it installed. +func installSkillAgentSkill(agent harness.SkillAgent) (string, error) { + if !agent.Detect() { + setup := "basecamp setup " + agent.ID + return "", &agentSetupError{ + Summary: agent.Name + " not detected — install " + agent.Name + ", then run: " + setup, + Manual: []string{setup}, + } + } + path := harness.AgentSkillPath() + if path == "" { + return "", fmt.Errorf("cannot determine shared Agent Skills directory") + } + if !baselineSkillInstalled() { + return "", fmt.Errorf("shared Basecamp skill is not installed at %s", path) + } + return path, nil } // runClaudeSetup performs the Claude Code-specific setup steps @@ -676,8 +737,8 @@ func newSetupAgentCmds() []*cobra.Command { } // agentSetupEnv selects which coding agents `setup agents` targets. -// Values: an agent id (claude | codex) | all | none. Empty (unset) means -// auto-detect. +// Values: an agent id (claude | codex | grok) | all | none. Empty (unset) +// means auto-detect. const agentSetupEnv = "BASECAMP_SETUP_AGENT" // agentSelectorValues lists what agentSetupEnv accepts, for help and @@ -690,7 +751,7 @@ func agentSelectorValues() []string { return append(values, "all", "none") } -// agentSelectorProse renders agentSelectorValues as "claude, codex, all, or none". +// agentSelectorProse renders agentSelectorValues as "claude, codex, grok, all, or none". func agentSelectorProse() string { values := agentSelectorValues() return strings.Join(values[:len(values)-1], ", ") + ", or " + values[len(values)-1] @@ -827,8 +888,9 @@ func runNonInteractiveAgentSetup(cmd *cobra.Command, app *appctx.App) error { // warnings: synthesized missing-binary remediation, sorted-agent order. // The Claude handler treats a missing binary as no-op success while Codex // returns an error, so synthesizing here keeps remediation symmetric. Only - // when the absence actually prevented the connection — an agent whose - // integration is already healthy has nothing for the binary to fix. + // when the absence actually prevented the connection — a shared-skill + // agent is routinely detected by its home directory alone, and its + // skill-only setup succeeds without a binary. for _, r := range records { if r.binaryAbsent && !r.pluginInstalled { warnings = append(warnings, fmt.Sprintf("%s: %s binary not found; install %s, then run: basecamp setup %s", r.id, r.name, r.name, r.id)) @@ -898,9 +960,10 @@ func runAgentSetupHandler(cmd *cobra.Command, agent harness.AgentInfo) agentSetu } rec.detectedAfter = agent.Detect != nil && agent.Detect() - // Connected means the handler succeeded AND health checks pass, the same - // verdict `setup ` reaches: a check that passes despite a setup error - // is a conflict to report, not a connection. + // Connected means the handler succeeded AND health checks pass. Checks + // alone are not enough: the shared skill this command just installed + // passes a shared-skill agent's only check whether or not that agent is + // on the machine — the handler's refusal is what says it is not. rec.pluginInstalled = len(rec.errors) == 0 && agentChecksPass(agent) return rec } @@ -1009,8 +1072,8 @@ func orEmptyStrings(ss []string) []string { } // baselineSkillInstalled returns true if ~/.agents/skills/basecamp/SKILL.md -// exists. The predicate lives in harness so an agent's health check can be -// the same one. +// exists. The predicate lives in harness so the shared-skill agents' health +// check is the same one. func baselineSkillInstalled() bool { return harness.BaselineSkillInstalled() } diff --git a/internal/commands/wizard_skill_agent_test.go b/internal/commands/wizard_skill_agent_test.go new file mode 100644 index 000000000..5ec9e1854 --- /dev/null +++ b/internal/commands/wizard_skill_agent_test.go @@ -0,0 +1,166 @@ +package commands + +import ( + "bytes" + "context" + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/spf13/cobra" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/harness" + "github.com/basecamp/basecamp-cli/internal/tui" +) + +// runSetupSkillAgentJSON executes `setup ` in machine mode for a +// shared-skill agent and parses the envelope `setup codex` also answers. +func runSetupSkillAgentJSON(t *testing.T, agent harness.SkillAgent) setupCodexEnvelope { + t.Helper() + app, output := setupQuickstartTestApp(t, "", "") + app.Flags.JSON = true + app.Flags.Hints = true + t.Cleanup(app.Close) + + cmd := NewSetupCmd() + cmd.SetArgs([]string{agent.ID}) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + cmd.SetOut(&bytes.Buffer{}) + cmd.SetErr(&bytes.Buffer{}) + require.NoError(t, cmd.Execute()) + + var envelope setupCodexEnvelope + require.NoError(t, json.Unmarshal(output.Bytes(), &envelope), output.String()) + return envelope +} + +func TestNewSetupCmdHasSkillAgentSubcommands(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent harness.SkillAgent) { + sub := findSubcommand(NewSetupCmd(), agent.ID) + require.NotNil(t, sub) + assert.Equal(t, "Connect "+agent.Name+" to Basecamp", sub.Short) + }) +} + +func TestSetupSkillAgentHandlerIsInTheTable(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent harness.SkillAgent) { + handler, ok := agentSetupHandlers[agent.ID] + require.True(t, ok) + assert.Equal(t, []string{"Install the shared Basecamp skill for " + agent.Name}, handler.Labels) + assert.NotNil(t, handler.Run) + assert.NotNil(t, handler.RunNonInteractive) + }) + // The plugin agents keep their own handlers alongside. + assert.Contains(t, agentSetupHandlers, "claude") + assert.Contains(t, agentSetupHandlers, "codex") +} + +// `setup ` on a machine without the agent installs the shared skill, +// reports the agent missing with its own remediation, and leaves no trace +// of the agent behind: fabricating its home would make every later +// detection — and this command's own verdict — report it installed. +func TestSetupSkillAgentNotDetectedDoesNotFabricateHome(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent harness.SkillAgent) { + home := emptyHome(t) + t.Setenv(agent.HomeEnv, "") + + envelope := runSetupSkillAgentJSON(t, agent) + + assert.False(t, envelope.Data.AgentDetected) + assert.False(t, envelope.Data.PluginInstalled) + assert.Equal(t, agent.Name+" not detected", envelope.Summary) + require.Len(t, envelope.Data.Errors, 1) + assert.Contains(t, envelope.Data.Errors[0], agent.Name+" not detected") + assert.Equal(t, []string{"basecamp setup " + agent.ID}, envelope.Data.ManualCommands) + assert.FileExists(t, filepath.Join(home, ".agents", "skills", "basecamp", "SKILL.md"), "the shared skill is installed regardless") + assert.NoFileExists(t, filepath.Join(home, agent.HomeDir)) + }) +} + +func TestSetupSkillAgentDetectedByHomeConnects(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent harness.SkillAgent) { + home := emptyHome(t) + t.Setenv(agent.HomeEnv, "") + require.NoError(t, os.MkdirAll(filepath.Join(home, agent.HomeDir), 0o755)) + + envelope := runSetupSkillAgentJSON(t, agent) + + assert.True(t, envelope.Data.AgentDetected) + assert.True(t, envelope.Data.PluginInstalled) + assert.Empty(t, envelope.Data.Errors) + assert.Empty(t, envelope.Data.ManualCommands) + assert.Equal(t, agent.Name+" connected", envelope.Summary) + require.NotEmpty(t, envelope.Breadcrumbs) + assert.Equal(t, "basecamp doctor", envelope.Breadcrumbs[0].Cmd) + assert.NoFileExists(t, filepath.Join(home, agent.HomeDir, "skills"), "the skill is not copied into the agent's home; it reads ~/.agents directly") + }) +} + +// A relocated home ($GROK_HOME) detects the agent the same way, and a binary +// alone — no home directory yet — is enough too. +func TestSetupSkillAgentDetectedByOverrideOrBinary(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent harness.SkillAgent) { + t.Run("home override", func(t *testing.T) { + emptyHome(t) + t.Setenv(agent.HomeEnv, t.TempDir()) + + envelope := runSetupSkillAgentJSON(t, agent) + + assert.True(t, envelope.Data.AgentDetected) + assert.True(t, envelope.Data.PluginInstalled) + }) + t.Run("binary on PATH", func(t *testing.T) { + emptyHome(t) + t.Setenv(agent.HomeEnv, "") + bin := t.TempDir() + require.NoError(t, os.WriteFile(filepath.Join(bin, agent.Binary), []byte("#!/bin/sh\n"), 0o755)) //nolint:gosec // G306: test stub must be executable + t.Setenv("PATH", bin) + + envelope := runSetupSkillAgentJSON(t, agent) + + assert.True(t, envelope.Data.AgentDetected) + assert.True(t, envelope.Data.PluginInstalled) + }) + }) +} + +// The interactive handler warns and continues: `basecamp setup` must never +// abort on one agent, and the post-setup snapshot is what reports the miss. +func TestSkillAgentSetupHandlerWarnsAndContinues(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent harness.SkillAgent) { + home := emptyHome(t) + t.Setenv(agent.HomeEnv, "") + styles := tui.NewStylesWithTheme(tui.ResolveTheme(false)) + handler := agentSetupHandlers[agent.ID] + + run := func() string { + var out bytes.Buffer + cmd := &cobra.Command{} + cmd.SetOut(&out) + require.NoError(t, handler.Run(cmd, styles)) + return out.String() + } + + out := run() + assert.Contains(t, out, agent.Name+" skill setup failed") + assert.Contains(t, out, agent.Name+" not detected") + assert.Contains(t, out, "basecamp doctor") + assert.NoFileExists(t, filepath.Join(home, agent.HomeDir)) + + // Detected but the shared skill missing is its own failure; the caller + // installs the skill first, and this step only confirms it. + require.NoError(t, os.MkdirAll(filepath.Join(home, agent.HomeDir), 0o755)) + out = run() + assert.Contains(t, out, "shared Basecamp skill is not installed") + + _, err := installSkillFiles() + require.NoError(t, err) + out = run() + assert.Contains(t, out, agent.Name+" skill installed ("+harness.AgentSkillPath()+")") + assert.Contains(t, out, "Start a new "+agent.Name+" session") + }) +} diff --git a/internal/commands/wizard_test.go b/internal/commands/wizard_test.go index 78c6b2a73..c2ad8ca83 100644 --- a/internal/commands/wizard_test.go +++ b/internal/commands/wizard_test.go @@ -1208,7 +1208,7 @@ func TestSetupSubcommandsSurviveTheGate(t *testing.T) { for _, agent := range agents { subs = append(subs, agent.ID) } - require.Equal(t, []string{"agents", "claude", "codex"}, subs) + require.Equal(t, []string{"agents", "claude", "codex", "grok"}, subs) for _, sub := range subs { t.Run(sub, func(t *testing.T) { t.Setenv("BASECAMP_NO_KEYRING", "1") diff --git a/internal/harness/harness.go b/internal/harness/harness.go index f60f1d976..fa5a28082 100644 --- a/internal/harness/harness.go +++ b/internal/harness/harness.go @@ -31,8 +31,8 @@ func AgentSkillPath() string { } // BaselineSkillInstalled reports whether the shared skill is on disk. It is -// the one health predicate for the shared skill: setup and doctor answer -// from it, and so does any agent whose integration is the skill alone. +// the one health predicate for the shared skill: setup, doctor and the +// shared-skill agents' checks all answer from it. func BaselineSkillInstalled() bool { return statAgentSkill() == nil } diff --git a/internal/harness/skill_agent.go b/internal/harness/skill_agent.go new file mode 100644 index 000000000..37e186924 --- /dev/null +++ b/internal/harness/skill_agent.go @@ -0,0 +1,138 @@ +package harness + +import ( + "context" + "errors" + "os" + "os/exec" + "path/filepath" + "strings" +) + +// SkillAgent describes a coding agent whose whole Basecamp integration is the +// shared ~/.agents skill: basecamp-cli ships no plugin for it, so setup is +// confirming the shared skill and health is skill presence only. Each such +// agent differs only in the five fields below, which is why they are rows in +// a table rather than a file apiece. +// +// Codex is not a row. It has a native plugin here (.codex-plugin, installed +// through the 37signals marketplace), so its detection, setup and health are +// its own — see codex.go. An agent leaves this table the day it grows one. +type SkillAgent struct { + Name string // "Grok" + ID string // "grok"; the `basecamp setup ` subcommand and BASECAMP_SETUP_AGENT value + HomeEnv string // "GROK_HOME"; overrides HomeDir when set + HomeDir string // ".grok"; under the user's home directory + Binary string // "grok"; the executable's name +} + +// Grok is xAI's Grok Build CLI. It reads user skills from ~/.grok/skills and +// from the cross-agent ~/.agents/skills, so the shared skill is all it needs. +var Grok = SkillAgent{Name: "Grok", ID: "grok", HomeEnv: "GROK_HOME", HomeDir: ".grok", Binary: "grok"} + +// skillAgents is the registration table: every agent that reads the shared +// skill, in the order they register. +var skillAgents = []SkillAgent{Grok} + +func init() { + for _, agent := range skillAgents { + RegisterAgent(agent.agentInfo()) + } +} + +// SkillAgents returns every shared-skill agent, in registration order. +func SkillAgents() []SkillAgent { + return append([]SkillAgent(nil), skillAgents...) +} + +func (a SkillAgent) agentInfo() AgentInfo { + checks := func() []*StatusCheck { return []*StatusCheck{a.CheckSkill()} } + return AgentInfo{ + Name: a.Name, + ID: a.ID, + Detect: a.Detect, + FindBinary: a.FindBinary, + Checks: checks, + Diagnostics: func(context.Context) []*StatusCheck { return checks() }, + } +} + +// Detect reports whether the agent has a home directory or an executable. +func (a SkillAgent) Detect() bool { + if info, err := os.Stat(a.Home()); err == nil && info.IsDir() { + return true + } + return a.FindBinary() != "" +} + +// FindBinary returns the agent's executable path, or an empty string. It +// looks on PATH first, then where an installer puts the binary when the +// shell has not picked up the PATH change yet: ~/.local/bin, and the agent's +// own home's bin (Grok Build's installers write ~/.grok/bin/grok, or +// $GROK_HOME/bin/grok for the npm package). +func (a SkillAgent) FindBinary() string { + if path, err := exec.LookPath(a.Binary); err == nil { + return path + } + var candidates []string + if home, err := os.UserHomeDir(); err == nil && home != "" { + candidates = append(candidates, filepath.Join(filepath.Clean(home), ".local", "bin", a.Binary)) + } + if agentHome := a.Home(); agentHome != "" { + candidates = append(candidates, filepath.Join(agentHome, "bin", a.Binary)) + } + for _, candidate := range candidates { + if _, err := os.Stat(candidate); err == nil { + return candidate + } + } + return "" +} + +// Home returns the agent's home directory: $HomeEnv, or HomeDir under the +// user's home. Empty when neither can be determined. +func (a SkillAgent) Home() string { + if home := strings.TrimSpace(os.Getenv(a.HomeEnv)); home != "" { + return home + } + home, err := os.UserHomeDir() + if err != nil || home == "" { + return "" + } + return filepath.Join(filepath.Clean(home), a.HomeDir) +} + +// CheckSkill checks whether the shared Basecamp skill is installed for the +// agent. It answers from the same predicate as BaselineSkillInstalled; the +// extra states only say why it is false. +func (a SkillAgent) CheckSkill() *StatusCheck { + name := a.Name + " Skill" + err := statAgentSkill() + switch { + case errors.Is(err, errNoHomeDir): + return &StatusCheck{ + Name: name, + Status: "warn", + Message: "Cannot determine shared Agent Skills directory", + } + case os.IsNotExist(err): + return &StatusCheck{ + Name: name, + Status: "fail", + Message: "Skill not installed", + Hint: "Run: basecamp setup " + a.ID, + } + case err != nil: + return &StatusCheck{ + Name: name, + Status: "warn", + Message: "Cannot check " + a.Name + " skill", + Hint: "Unable to stat " + AgentSkillPath(), + } + } + return &StatusCheck{ + Name: name, + Status: "pass", + Message: "Installed", + } +} diff --git a/internal/harness/skill_agent_test.go b/internal/harness/skill_agent_test.go new file mode 100644 index 000000000..ff3c50286 --- /dev/null +++ b/internal/harness/skill_agent_test.go @@ -0,0 +1,184 @@ +package harness + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Every shared-skill agent is one row of the same table, so every test here +// runs once per row: a behavior one row has and another lacks is a bug in +// the table, not a difference between agents. +func forEachSkillAgent(t *testing.T, test func(t *testing.T, agent SkillAgent)) { + t.Helper() + for _, agent := range SkillAgents() { + t.Run(agent.ID, func(t *testing.T) { test(t, agent) }) + } +} + +// isolatedHome points HOME at an empty directory and PATH at another, with the +// agent's home override cleared, so nothing on the developer's machine leaks in. +func isolatedHome(t *testing.T, agent SkillAgent) string { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("PATH", t.TempDir()) + t.Setenv(agent.HomeEnv, "") + return home +} + +func writeStubBinary(t *testing.T, path string) { + t.Helper() + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) + require.NoError(t, os.WriteFile(path, []byte("#!/bin/sh\n"), 0o755)) //nolint:gosec // G306: test stub must be executable +} + +func TestSkillAgentTableHoldsGrok(t *testing.T) { + assert.Equal(t, []SkillAgent{Grok}, SkillAgents()) + assert.Equal(t, SkillAgent{Name: "Grok", ID: "grok", HomeEnv: "GROK_HOME", HomeDir: ".grok", Binary: "grok"}, Grok) +} + +// Sibling tests reset the global registry, so init()'s registration cannot be +// observed here; what can be is the AgentInfo a row registers, the same way +// TestClaudeAgentInfoWiring covers Claude. +func TestSkillAgentInfoWiring(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent SkillAgent) { + resetRegistry() + defer resetRegistry() + + RegisterAgent(agent.agentInfo()) + + info := FindAgent(agent.ID) + require.NotNil(t, info, "%s agent not registered", agent.ID) + assert.Equal(t, agent.Name, info.Name) + assert.NotNil(t, info.Detect) + assert.NotNil(t, info.FindBinary) + assert.NotNil(t, info.Checks) + assert.NotNil(t, info.Diagnostics) + + // Checks and Diagnostics are the same one check: skill presence. + isolatedHome(t, agent) + checks := info.Checks() + require.Len(t, checks, 1) + assert.Equal(t, agent.Name+" Skill", checks[0].Name) + assert.Equal(t, checks, info.Diagnostics(t.Context())) + }) +} + +func TestSkillAgentDetectByHomeDirectory(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent SkillAgent) { + home := isolatedHome(t, agent) + + assert.False(t, agent.Detect(), "no ~/%s and no binary", agent.HomeDir) + require.NoError(t, os.MkdirAll(filepath.Join(home, agent.HomeDir), 0o755)) + assert.True(t, agent.Detect(), "~/%s directory", agent.HomeDir) + }) +} + +func TestSkillAgentDetectByBinaryOnPath(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent SkillAgent) { + isolatedHome(t, agent) + bin := t.TempDir() + stub := filepath.Join(bin, agent.Binary) + writeStubBinary(t, stub) + t.Setenv("PATH", bin) + + assert.True(t, agent.Detect(), "%s on PATH detects %s without a home directory", agent.Binary, agent.Name) + assert.Equal(t, stub, agent.FindBinary()) + }) +} + +// Off PATH, the binary is found where an installer leaves it: ~/.local/bin, +// or the bin directory of the agent's own home — a relocated one included. +func TestSkillAgentFindBinaryOffPath(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent SkillAgent) { + cases := map[string]func(t *testing.T, home string) string{ + "local bin": func(_ *testing.T, home string) string { return filepath.Join(home, ".local", "bin") }, + "home bin": func(_ *testing.T, home string) string { return filepath.Join(home, agent.HomeDir, "bin") }, + "env home bin": func(t *testing.T, _ string) string { + override := t.TempDir() + t.Setenv(agent.HomeEnv, override) + return filepath.Join(override, "bin") + }, + } + for name, binDir := range cases { + t.Run(name, func(t *testing.T) { + home := isolatedHome(t, agent) + require.Empty(t, agent.FindBinary(), "before any install") + + stub := filepath.Join(binDir(t, home), agent.Binary) + writeStubBinary(t, stub) + assert.Equal(t, stub, agent.FindBinary()) + assert.True(t, agent.Detect()) + }) + } + }) +} + +func TestSkillAgentHomeHonorsEnvOverride(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent SkillAgent) { + home := isolatedHome(t, agent) + assert.Equal(t, filepath.Join(home, agent.HomeDir), agent.Home()) + + override := t.TempDir() + t.Setenv(agent.HomeEnv, override) + assert.Equal(t, override, agent.Home()) + assert.True(t, agent.Detect(), "an overridden home directory detects the agent") + }) +} + +func TestSkillAgentCheckSkill(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent SkillAgent) { + home := isolatedHome(t, agent) + + check := agent.CheckSkill() + assert.Equal(t, agent.Name+" Skill", check.Name) + assert.Equal(t, "fail", check.Status) + assert.Equal(t, "Run: basecamp setup "+agent.ID, check.Hint) + assert.False(t, BaselineSkillInstalled()) + + skillDir := filepath.Join(home, ".agents", "skills", "basecamp") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte("# basecamp"), 0o644)) + + check = agent.CheckSkill() + assert.Equal(t, "pass", check.Status, "%+v", check) + assert.True(t, BaselineSkillInstalled()) + }) +} + +// The check and the predicate answer from the same stat: a skill the agent +// cannot read is unhealthy for both, in the same direction. +func TestSkillAgentCheckSkillUnreadable(t *testing.T) { + if os.Getuid() == 0 { + t.Skip("root bypasses directory permissions") + } + forEachSkillAgent(t, func(t *testing.T, agent SkillAgent) { + home := isolatedHome(t, agent) + skillsDir := filepath.Join(home, ".agents", "skills") + require.NoError(t, os.MkdirAll(filepath.Join(skillsDir, "basecamp"), 0o755)) + require.NoError(t, os.Chmod(skillsDir, 0o000)) + t.Cleanup(func() { _ = os.Chmod(skillsDir, 0o755) }) + + check := agent.CheckSkill() + assert.Equal(t, "warn", check.Status, "%+v", check) + assert.Contains(t, check.Hint, AgentSkillPath()) + assert.False(t, BaselineSkillInstalled()) + }) +} + +func TestSkillAgentCheckSkillReportsMissingHome(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent SkillAgent) { + t.Setenv("HOME", "") + t.Setenv("USERPROFILE", "") + + check := agent.CheckSkill() + assert.Equal(t, "warn", check.Status) + assert.Equal(t, "Cannot determine shared Agent Skills directory", check.Message) + assert.Empty(t, AgentSkillPath()) + assert.False(t, BaselineSkillInstalled()) + }) +} diff --git a/scripts/install.ps1 b/scripts/install.ps1 index 2bec8692c..6a876cdf7 100644 --- a/scripts/install.ps1 +++ b/scripts/install.ps1 @@ -14,7 +14,7 @@ try { # BASECAMP_NONINTERACTIVE # Set to 1 or true to use non-interactive setup # BASECAMP_SETUP_AGENT Which coding agent(s) `setup agents` connects: -# claude | codex | all | none. Unset = auto-detect. +# claude | codex | grok | all | none. Unset = auto-detect. # Piped install sets it for the interpreter, not the fetch: # $env:BASECAMP_SETUP_AGENT='codex'; irm https://raw.githubusercontent.com/basecamp/basecamp-cli/main/scripts/install.ps1 | iex $Repo = 'basecamp/basecamp-cli' @@ -321,7 +321,7 @@ function Invoke-FirstTimeSetup([string]$Binary) { } # Invoke-PostInstallSetup installs the baseline skill and connects coding agents -# without prompting, honoring BASECAMP_SETUP_AGENT (claude|codex|all|none; +# without prompting, honoring BASECAMP_SETUP_AGENT (claude|codex|grok|all|none; # unset = auto-detect). It is strictly best-effort: agent setup must never fail # an otherwise-successful install, so every native call is wrapped so a nonzero # exit (amplified by $ErrorActionPreference='Stop' + @@ -349,7 +349,7 @@ function Invoke-PostInstallSetup([string]$Binary) { } $selector = $env:BASECAMP_SETUP_AGENT - if ($selector -in @('claude', 'codex')) { + if ($selector -in @('claude', 'codex', 'grok')) { # Capability-check first: an old `setup` parent accepts an unadvertised agent # id as a stray arg and launches the INTERACTIVE wizard. Degrade to the skill. if ($help -match "(?m)^\s+$selector\s") { @@ -359,7 +359,7 @@ function Invoke-PostInstallSetup([string]$Binary) { } } elseif ($selector -eq 'all') { $ranAgent = $false - foreach ($agent in @('claude', 'codex')) { + foreach ($agent in @('claude', 'codex', 'grok')) { if ($help -match "(?m)^\s+$agent\s") { # Mark attempted (not succeeded) -- matches install.sh's `ran_agent=1`, # which is set regardless of the setup call's exit status. diff --git a/scripts/install.sh b/scripts/install.sh index 3334ce227..c0aa28607 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -18,7 +18,7 @@ # and connect coding agents) # BASECAMP_SETUP_AGENT # Which coding agent(s) `setup agents` connects: -# claude | codex | all | none. Unset = auto-detect (connect +# claude | codex | grok | all | none. Unset = auto-detect (connect # a single detected agent; if several, install the skill # only and surface the per-agent commands). # Piped install sets it for the interpreter, not the fetch: @@ -577,13 +577,13 @@ binary_supports_setup_agents() { } # binary_supports_setup_agent reports whether the binary exposes a per-agent -# `setup ` subcommand for the given agent id (claude or codex). +# `setup ` subcommand for the given agent id (claude, codex or grok). binary_supports_setup_agent() { "$1" setup --help 2>/dev/null | grep -qE "^[[:space:]]+$2[[:space:]]" } # post_install_setup installs the baseline skill and connects coding agents -# without prompting. It honors BASECAMP_SETUP_AGENT (claude|codex|all|none; +# without prompting. It honors BASECAMP_SETUP_AGENT (claude|codex|grok|all|none; # unset = auto-detect). Never runs the interactive wizard. # # Cross-version: newer binaries get the intent-neutral `setup agents`. Older @@ -607,7 +607,7 @@ post_install_setup() { fi case "${BASECAMP_SETUP_AGENT:-}" in - claude|codex) + claude|codex|grok) # Capability-check first: an old `setup` parent accepts an unadvertised # agent id as a stray positional arg and launches the INTERACTIVE wizard, # violating the non-interactive contract. Degrade to the shared skill. @@ -621,7 +621,7 @@ post_install_setup() { # Explicit "every agent": dispatch each per-agent setup the binary knows, # falling back to the shared skill if it supports none of them. local ran_agent=0 agent - for agent in claude codex; do + for agent in claude codex grok; do if binary_supports_setup_agent "$bin" "$agent"; then BASECAMP_NO_KEYRING=1 "$bin" setup "$agent" || true ran_agent=1 diff --git a/skills/basecamp-doctor/SKILL.md b/skills/basecamp-doctor/SKILL.md index eafda447b..5643d419f 100644 --- a/skills/basecamp-doctor/SKILL.md +++ b/skills/basecamp-doctor/SKILL.md @@ -28,6 +28,7 @@ Report failures and warnings with their `hint` fields. Also inspect the top-leve - Agent plugin installation or version: `basecamp setup agents` (honors `BASECAMP_SETUP_AGENT`) - Codex plugin specifically: `basecamp setup codex` - Claude Code plugin specifically: `basecamp setup claude` +- Grok skill specifically: `basecamp setup grok` (skill-only; Grok reads the shared `~/.agents/skills/basecamp` skill) Every remediation above runs without a terminal. Bare `basecamp setup` is the human first-time flow and is **not** one of them: it opens browser OAuth and diff --git a/skills/basecamp/SKILL.md b/skills/basecamp/SKILL.md index 70c4bd6b3..9a004a317 100644 --- a/skills/basecamp/SKILL.md +++ b/skills/basecamp/SKILL.md @@ -1377,7 +1377,7 @@ basecamp setup agents # Install skill + connect dete basecamp setup agents --json # Structured result envelope ``` `setup agents` installs the baseline skill and connects coding agents without -prompting. Selection is driven by `BASECAMP_SETUP_AGENT` (`claude`, `codex`, +prompting. Selection is driven by `BASECAMP_SETUP_AGENT` (`claude`, `codex`, `grok`, `all`, or `none`); unset auto-detects — one detected agent is connected, several leave the skill only and surface the per-agent `basecamp setup ` commands. From bd3f5c82635e22b02f760395e327631735fda773 Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 10 Sep 2026 14:43:26 -0700 Subject: [PATCH 3/5] Hold fallback binary candidates to exec.LookPath's standard A file or directory merely named grok in ~/.local/bin or the agent's bin was reported as the binary, and made Detect say Grok was present with no home directory. Each candidate now goes through exec.LookPath, which requires an executable regular file on Unix and resolves the PATHEXT extension on Windows, where the official binary is grok.exe. The test stubs take that name on Windows too, so the PATH-lookup test exercises the implementation there instead of failing on the extension. --- internal/harness/skill_agent.go | 10 +++++-- internal/harness/skill_agent_test.go | 42 ++++++++++++++++++++++++++-- 2 files changed, 47 insertions(+), 5 deletions(-) diff --git a/internal/harness/skill_agent.go b/internal/harness/skill_agent.go index 37e186924..c44c856cb 100644 --- a/internal/harness/skill_agent.go +++ b/internal/harness/skill_agent.go @@ -69,7 +69,11 @@ func (a SkillAgent) Detect() bool { // looks on PATH first, then where an installer puts the binary when the // shell has not picked up the PATH change yet: ~/.local/bin, and the agent's // own home's bin (Grok Build's installers write ~/.grok/bin/grok, or -// $GROK_HOME/bin/grok for the npm package). +// $GROK_HOME/bin/grok for the npm package). Each fallback goes through +// exec.LookPath too, so it is held to the PATH lookup's standard — an +// executable regular file on Unix, a PATHEXT extension such as .exe on +// Windows — and a stale directory or non-executable file of that name is +// not reported as the binary. func (a SkillAgent) FindBinary() string { if path, err := exec.LookPath(a.Binary); err == nil { return path @@ -82,8 +86,8 @@ func (a SkillAgent) FindBinary() string { candidates = append(candidates, filepath.Join(agentHome, "bin", a.Binary)) } for _, candidate := range candidates { - if _, err := os.Stat(candidate); err == nil { - return candidate + if path, err := exec.LookPath(candidate); err == nil { + return path } } return "" diff --git a/internal/harness/skill_agent_test.go b/internal/harness/skill_agent_test.go index ff3c50286..1852f74b7 100644 --- a/internal/harness/skill_agent_test.go +++ b/internal/harness/skill_agent_test.go @@ -3,6 +3,7 @@ package harness import ( "os" "path/filepath" + "runtime" "testing" "github.com/stretchr/testify/assert" @@ -30,6 +31,16 @@ func isolatedHome(t *testing.T, agent SkillAgent) string { return home } +// stubBinaryPath is where a stub of the agent's executable goes in dir: the +// bare name on Unix, name.exe on Windows, where exec.LookPath resolves the +// bare name through PATHEXT and the official binary is grok.exe. +func stubBinaryPath(dir string, agent SkillAgent) string { + if runtime.GOOS == "windows" { + return filepath.Join(dir, agent.Binary+".exe") + } + return filepath.Join(dir, agent.Binary) +} + func writeStubBinary(t *testing.T, path string) { t.Helper() require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o755)) @@ -82,7 +93,7 @@ func TestSkillAgentDetectByBinaryOnPath(t *testing.T) { forEachSkillAgent(t, func(t *testing.T, agent SkillAgent) { isolatedHome(t, agent) bin := t.TempDir() - stub := filepath.Join(bin, agent.Binary) + stub := stubBinaryPath(bin, agent) writeStubBinary(t, stub) t.Setenv("PATH", bin) @@ -109,7 +120,7 @@ func TestSkillAgentFindBinaryOffPath(t *testing.T) { home := isolatedHome(t, agent) require.Empty(t, agent.FindBinary(), "before any install") - stub := filepath.Join(binDir(t, home), agent.Binary) + stub := stubBinaryPath(binDir(t, home), agent) writeStubBinary(t, stub) assert.Equal(t, stub, agent.FindBinary()) assert.True(t, agent.Detect()) @@ -118,6 +129,33 @@ func TestSkillAgentFindBinaryOffPath(t *testing.T) { }) } +// A fallback candidate is held to the PATH lookup's standard: something that +// merely has the binary's name is not the binary, and must not make Detect +// report the agent present. +func TestSkillAgentFindBinaryOffPathRequiresExecutable(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent SkillAgent) { + t.Run("directory", func(t *testing.T) { + home := isolatedHome(t, agent) + require.NoError(t, os.MkdirAll(filepath.Join(home, ".local", "bin", agent.Binary), 0o755)) + + assert.Empty(t, agent.FindBinary()) + assert.False(t, agent.Detect()) + }) + t.Run("non-executable file", func(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Windows has no execute bit; the extension decides") + } + home := isolatedHome(t, agent) + stub := filepath.Join(home, ".local", "bin", agent.Binary) + require.NoError(t, os.MkdirAll(filepath.Dir(stub), 0o755)) + require.NoError(t, os.WriteFile(stub, []byte("not a program"), 0o644)) + + assert.Empty(t, agent.FindBinary()) + assert.False(t, agent.Detect(), "~/.local/bin/%s without an execute bit", agent.Binary) + }) + }) +} + func TestSkillAgentHomeHonorsEnvOverride(t *testing.T) { forEachSkillAgent(t, func(t *testing.T, agent SkillAgent) { home := isolatedHome(t, agent) From 0be24bc4169d3778081810e4a82b1597db06cb3d Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 10 Sep 2026 14:49:06 -0700 Subject: [PATCH 4/5] Name the setup test's Grok stub for the platform too The harness tests took platform-named stubs (grok.exe on Windows) in the previous commit; the `setup grok` test that puts a binary on PATH still wrote the bare name, so it would have failed on a Windows checkout for the same reason. Same naming rule here. --- internal/commands/wizard_skill_agent_test.go | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/internal/commands/wizard_skill_agent_test.go b/internal/commands/wizard_skill_agent_test.go index 5ec9e1854..a0ecbbefd 100644 --- a/internal/commands/wizard_skill_agent_test.go +++ b/internal/commands/wizard_skill_agent_test.go @@ -6,6 +6,7 @@ import ( "encoding/json" "os" "path/filepath" + "runtime" "testing" "github.com/spf13/cobra" @@ -117,7 +118,13 @@ func TestSetupSkillAgentDetectedByOverrideOrBinary(t *testing.T) { emptyHome(t) t.Setenv(agent.HomeEnv, "") bin := t.TempDir() - require.NoError(t, os.WriteFile(filepath.Join(bin, agent.Binary), []byte("#!/bin/sh\n"), 0o755)) //nolint:gosec // G306: test stub must be executable + // Named for the platform, as the harness stubs are: grok.exe on + // Windows, where LookPath resolves the bare name through PATHEXT. + stub := filepath.Join(bin, agent.Binary) + if runtime.GOOS == "windows" { + stub += ".exe" + } + require.NoError(t, os.WriteFile(stub, []byte("#!/bin/sh\n"), 0o755)) //nolint:gosec // G306: test stub must be executable t.Setenv("PATH", bin) envelope := runSetupSkillAgentJSON(t, agent) From 37b946c19c3a9eacd1a8dc20c167767c2b318c0b Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 10 Sep 2026 15:00:10 -0700 Subject: [PATCH 5/5] Blame the binary only when its absence is what blocked the agent A shared-skill agent detected by its home directory never runs its binary during setup, so when the shared skill failed to install the synthesized "binary not found" remediation named the wrong cause next to the error that named the right one. The remediation now applies to a plugin agent that is not connected, or to a shared-skill agent absent altogether. emptyHome also isolates what the new tests reach: USERPROFILE, which os.UserHomeDir reads on Windows, and each shared-skill agent's home override such as GROK_HOME, so a developer's own Grok install cannot leak into the setup-agents tests. --- internal/commands/setup_agents_test.go | 28 +++++++++++++++++++++++ internal/commands/wizard_agents.go | 31 +++++++++++++++++++++----- 2 files changed, 53 insertions(+), 6 deletions(-) diff --git a/internal/commands/setup_agents_test.go b/internal/commands/setup_agents_test.go index a7d3a8314..d79ed7480 100644 --- a/internal/commands/setup_agents_test.go +++ b/internal/commands/setup_agents_test.go @@ -78,11 +78,18 @@ func runSetupAgentsStyled(t *testing.T) string { } // emptyHome points HOME and PATH at empty temp dirs so no agent is detected. +// emptyHome isolates a test from the developer's machine: an empty home +// (HOME, and USERPROFILE for os.UserHomeDir on Windows), an empty PATH, and +// no shared-skill agent home override such as GROK_HOME. func emptyHome(t *testing.T) string { t.Helper() home := t.TempDir() t.Setenv("HOME", home) + t.Setenv("USERPROFILE", home) t.Setenv("PATH", filepath.Join(home, "empty-bin")) + for _, agent := range harness.SkillAgents() { + t.Setenv(agent.HomeEnv, "") + } return home } @@ -147,6 +154,27 @@ func TestSetupAgentsBaselineSkillFailure(t *testing.T) { assert.Contains(t, env.Data.Errors[0], "skill:") } +// A shared-skill agent detected by its home directory never needed a binary +// to connect, so when the shared skill is what failed, the missing-binary +// remediation would name the wrong cause; the skill error already names it. +func TestSetupAgentsSkillAgentSkillFailureIsNotABinaryProblem(t *testing.T) { + forEachSkillAgent(t, func(t *testing.T, agent harness.SkillAgent) { + home := emptyHome(t) + require.NoError(t, os.MkdirAll(filepath.Join(home, agent.HomeDir), 0o755)) + require.NoError(t, os.WriteFile(filepath.Join(home, ".agents"), []byte("blocker"), 0o644)) + t.Setenv("BASECAMP_SETUP_AGENT", agent.ID) + + env := runSetupAgentsJSON(t) + + assert.False(t, env.Data.SkillInstalled) + assert.Contains(t, env.Data.Errors[0], "skill:") + for _, warning := range env.Data.Warnings { + assert.NotContains(t, warning, "binary not found", "the binary did not block %s", agent.Name) + } + assert.NotContains(t, env.Data.ManualCommands, "basecamp setup "+agent.ID) + }) +} + func TestSetupAgentsSingleDetectedCodex(t *testing.T) { installCodexStub(t, codexStubOptions{}) t.Setenv("BASECAMP_SETUP_AGENT", "") diff --git a/internal/commands/wizard_agents.go b/internal/commands/wizard_agents.go index e4d2571a4..70bc41030 100644 --- a/internal/commands/wizard_agents.go +++ b/internal/commands/wizard_agents.go @@ -788,6 +788,7 @@ func newSetupAgentsCmd() *cobra.Command { type agentSetupRecord struct { id, name string detectedBefore bool + skillOnly bool // a shared-skill agent: its setup never runs the binary detectedAfter bool pluginInstalled bool binaryAbsent bool @@ -879,7 +880,7 @@ func runNonInteractiveAgentSetup(cmd *cobra.Command, app *appctx.App) error { for _, m := range r.manualCommands { manualUnion.add(m) } - if r.binaryAbsent && !r.pluginInstalled { + if r.binaryBlocked() { manualUnion.add("basecamp setup " + r.id) } } @@ -887,12 +888,9 @@ func runNonInteractiveAgentSetup(cmd *cobra.Command, app *appctx.App) error { // warnings: synthesized missing-binary remediation, sorted-agent order. // The Claude handler treats a missing binary as no-op success while Codex - // returns an error, so synthesizing here keeps remediation symmetric. Only - // when the absence actually prevented the connection — a shared-skill - // agent is routinely detected by its home directory alone, and its - // skill-only setup succeeds without a binary. + // returns an error, so synthesizing here keeps remediation symmetric. for _, r := range records { - if r.binaryAbsent && !r.pluginInstalled { + if r.binaryBlocked() { warnings = append(warnings, fmt.Sprintf("%s: %s binary not found; install %s, then run: basecamp setup %s", r.id, r.name, r.name, r.id)) } } @@ -947,6 +945,7 @@ func runAgentSetupHandler(cmd *cobra.Command, agent harness.AgentInfo) agentSetu name: agent.Name, detectedBefore: agent.Detect != nil && agent.Detect(), binaryAbsent: !agentBinaryPresent(agent), + skillOnly: isSkillAgent(agent.ID), } if handler, ok := agentSetupHandlers[agent.ID]; ok && handler.RunNonInteractive != nil { @@ -968,6 +967,26 @@ func runAgentSetupHandler(cmd *cobra.Command, agent harness.AgentInfo) agentSetu return rec } +// binaryBlocked reports whether the binary's absence is what kept the agent +// from connecting, which is when the missing-binary remediation applies. A +// plugin agent needs its binary to install the plugin, so absent and not +// connected is that. A shared-skill agent is routinely detected by its home +// directory alone and its skill-only setup never runs the binary: detected +// but not connected means the shared skill failed, which is already in its +// errors, and only an agent absent altogether is missing its binary. +func (r agentSetupRecord) binaryBlocked() bool { + return r.binaryAbsent && !r.pluginInstalled && (!r.skillOnly || !r.detectedBefore) +} + +func isSkillAgent(id string) bool { + for _, agent := range harness.SkillAgents() { + if agent.ID == id { + return true + } + } + return false +} + // agentBinaryPresent reports whether the agent's executable is on disk. An // agent with no executable to look for is assumed present so no bogus // remediation is synthesized.