diff --git a/.surface b/.surface index 68eba3d39..dc6d51651 100644 --- a/.surface +++ b/.surface @@ -677,6 +677,9 @@ CMD basecamp connect doctor CMD basecamp connect import CMD basecamp connect redispatch CMD basecamp connect release +CMD basecamp connect service +CMD basecamp connect service install +CMD basecamp connect service uninstall CMD basecamp connect setup CMD basecamp connect shadow CMD basecamp connect shadow promote @@ -5486,6 +5489,72 @@ FLAG basecamp connect release --stats type=bool FLAG basecamp connect release --styled type=bool FLAG basecamp connect release --todolist type=string FLAG basecamp connect release --verbose type=count +FLAG basecamp connect service --account type=string +FLAG basecamp connect service --agent type=bool +FLAG basecamp connect service --cache-dir type=string +FLAG basecamp connect service --count type=bool +FLAG basecamp connect service --help type=bool +FLAG basecamp connect service --hints type=bool +FLAG basecamp connect service --ids-only type=bool +FLAG basecamp connect service --in type=string +FLAG basecamp connect service --jq type=string +FLAG basecamp connect service --json type=bool +FLAG basecamp connect service --markdown type=bool +FLAG basecamp connect service --md type=bool +FLAG basecamp connect service --no-hints type=bool +FLAG basecamp connect service --no-stats type=bool +FLAG basecamp connect service --profile type=string +FLAG basecamp connect service --project type=string +FLAG basecamp connect service --quiet type=bool +FLAG basecamp connect service --stats type=bool +FLAG basecamp connect service --styled type=bool +FLAG basecamp connect service --todolist type=string +FLAG basecamp connect service --verbose type=count +FLAG basecamp connect service install --account type=string +FLAG basecamp connect service install --agent type=bool +FLAG basecamp connect service install --cache-dir type=string +FLAG basecamp connect service install --count type=bool +FLAG basecamp connect service install --help type=bool +FLAG basecamp connect service install --hints type=bool +FLAG basecamp connect service install --hold type=bool +FLAG basecamp connect service install --ids-only type=bool +FLAG basecamp connect service install --in type=string +FLAG basecamp connect service install --jq type=string +FLAG basecamp connect service install --json type=bool +FLAG basecamp connect service install --markdown type=bool +FLAG basecamp connect service install --md type=bool +FLAG basecamp connect service install --no-enable type=bool +FLAG basecamp connect service install --no-hints type=bool +FLAG basecamp connect service install --no-stats type=bool +FLAG basecamp connect service install --profile type=string +FLAG basecamp connect service install --project type=string +FLAG basecamp connect service install --quiet type=bool +FLAG basecamp connect service install --shadow type=bool +FLAG basecamp connect service install --stats type=bool +FLAG basecamp connect service install --styled type=bool +FLAG basecamp connect service install --todolist type=string +FLAG basecamp connect service install --verbose type=count +FLAG basecamp connect service uninstall --account type=string +FLAG basecamp connect service uninstall --agent type=bool +FLAG basecamp connect service uninstall --cache-dir type=string +FLAG basecamp connect service uninstall --count type=bool +FLAG basecamp connect service uninstall --help type=bool +FLAG basecamp connect service uninstall --hints type=bool +FLAG basecamp connect service uninstall --ids-only type=bool +FLAG basecamp connect service uninstall --in type=string +FLAG basecamp connect service uninstall --jq type=string +FLAG basecamp connect service uninstall --json type=bool +FLAG basecamp connect service uninstall --markdown type=bool +FLAG basecamp connect service uninstall --md type=bool +FLAG basecamp connect service uninstall --no-hints type=bool +FLAG basecamp connect service uninstall --no-stats type=bool +FLAG basecamp connect service uninstall --profile type=string +FLAG basecamp connect service uninstall --project type=string +FLAG basecamp connect service uninstall --quiet type=bool +FLAG basecamp connect service uninstall --stats type=bool +FLAG basecamp connect service uninstall --styled type=bool +FLAG basecamp connect service uninstall --todolist type=string +FLAG basecamp connect service uninstall --verbose type=count FLAG basecamp connect setup --account type=string FLAG basecamp connect setup --agent type=bool FLAG basecamp connect setup --allow type=stringArray @@ -18790,6 +18859,9 @@ SUB basecamp connect doctor SUB basecamp connect import SUB basecamp connect redispatch SUB basecamp connect release +SUB basecamp connect service +SUB basecamp connect service install +SUB basecamp connect service uninstall SUB basecamp connect setup SUB basecamp connect shadow SUB basecamp connect shadow promote diff --git a/e2e/smoke/smoke_lifecycle.bats b/e2e/smoke/smoke_lifecycle.bats index 2c052c281..b5c56559f 100644 --- a/e2e/smoke/smoke_lifecycle.bats +++ b/e2e/smoke/smoke_lifecycle.bats @@ -52,6 +52,14 @@ load smoke_helper mark_out_of_scope "Applies a reconciliation file to a local connector ledger — covered by Go tests in internal/commands and internal/connector" } +@test "connect service install is out of scope" { + mark_out_of_scope "Writes a systemd user unit and asks systemctl to start it, which the smoke runner has no session for — covered by Go tests in internal/commands" +} + +@test "connect service uninstall is out of scope" { + mark_out_of_scope "Stops a systemd user unit and removes it, which the smoke runner has no session for — covered by Go tests in internal/commands" +} + @test "auth refresh is out of scope" { mark_out_of_scope "Requires OAuth credentials" } diff --git a/internal/commands/commands.go b/internal/commands/commands.go index a742a76a3..30f473b32 100644 --- a/internal/commands/commands.go +++ b/internal/commands/commands.go @@ -146,7 +146,7 @@ func CommandCategories() []CommandCategory { {Name: "bonfire", Category: "additional", Description: "Multi-chat orchestration", Actions: []string{"split", "layout"}, Experimental: true, DevOnly: true}, {Name: "api", Category: "additional", Description: "Raw API access"}, {Name: "mcp", Category: "additional", Description: "Serve Basecamp to MCP clients over stdio"}, - {Name: "connect", Category: "additional", Description: "Run a local agent connector for a Basecamp agent, and see and decide what it runs", Actions: []string{"setup", "show", "status", "doctor", "redispatch", "discard", "release", "shadow", "import"}}, + {Name: "connect", Category: "additional", Description: "Run a local agent connector for a Basecamp agent, and see and decide what it runs", Actions: []string{"setup", "show", "status", "doctor", "redispatch", "discard", "release", "shadow", "import", "service"}}, {Name: "help", Category: "additional", Description: "Show help"}, {Name: "version", Category: "additional", Description: "Show version"}, }, diff --git a/internal/commands/connect.go b/internal/commands/connect.go index 2b48f911d..5f46a60b0 100644 --- a/internal/commands/connect.go +++ b/internal/commands/connect.go @@ -60,7 +60,8 @@ wait for review, until basecamp connect release. Linux only. basecamp connect discard close a record without running it basecamp connect release clear the hold basecamp connect shadow promote make the shadow ledger the connector's, held - basecamp connect import apply a cutover reconciliation file`, + basecamp connect import apply a cutover reconciliation file + basecamp connect service install have the OS keep it running`, Example: ` basecamp connect setup -P agent --operator-profile me --serve 12345 basecamp connect -P agent basecamp connect -P agent --project 12345 --shadow`, @@ -75,7 +76,8 @@ wait for review, until basecamp connect release. Linux only. } addConnectRunFlags(cmd, &run) cmd.AddCommand(newConnectSetupCmd(), newConnectWorkerMCPCmd(), newConnectShowCmd(), newConnectStatusCmd(), newConnectDoctorCmd(), - newConnectRedispatchCmd(), newConnectDiscardCmd(), newConnectReleaseCmd(), newConnectShadowCmd(), newConnectImportCmd()) + newConnectRedispatchCmd(), newConnectDiscardCmd(), newConnectReleaseCmd(), newConnectShadowCmd(), newConnectImportCmd(), + newConnectServiceCmd()) return cmd } diff --git a/internal/commands/connect_run.go b/internal/commands/connect_run.go index cc1bdabf5..6556c3ebd 100644 --- a/internal/commands/connect_run.go +++ b/internal/commands/connect_run.go @@ -560,10 +560,15 @@ func connectSupportedOS(goos string) bool { // then the other is not told two different stories about their machine. const connectLinuxOnlyReason = "the task token reaches a worker's MCP server over an inherited descriptor, and Linux is the only platform that seals the descriptors a process inherits" -// connectUnsupportedOSError is the run command's refusal on a platform the -// connector does not run on. +// connectUnsupportedOSError is the refusal on a platform the connector does +// not run on, given by the run command and by `service install`, which would +// otherwise write a unit that supervises a process that cannot start. +// +// It opens on the connector rather than on "basecamp connect ..." because a +// refusal is prose, not a command to run, and a hint that begins like a +// command is read as one — by a person, and by TestHintCommandsResolve. func connectUnsupportedOSError(goos string) error { - return output.ErrUsage(fmt.Sprintf("basecamp connect runs on Linux only, not %s: %s", goos, connectLinuxOnlyReason)) + return output.ErrUsage(fmt.Sprintf("The connector runs on Linux only, not %s: %s", goos, connectLinuxOnlyReason)) } // connectServed is connect.json's served projects as they are now, not as diff --git a/internal/commands/connect_service.go b/internal/commands/connect_service.go new file mode 100644 index 000000000..f216e5ffd --- /dev/null +++ b/internal/commands/connect_service.go @@ -0,0 +1,745 @@ +package commands + +import ( + "context" + "errors" + "fmt" + "os" + "os/exec" + "os/user" + "path/filepath" + "runtime" + "slices" + "sort" + "strconv" + "strings" + "time" + + "github.com/spf13/cobra" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/connector/driver" + "github.com/basecamp/basecamp-cli/internal/connector/setup" + "github.com/basecamp/basecamp-cli/internal/output" + "github.com/basecamp/basecamp-cli/internal/richtext" +) + +// The connector has no daemon of its own and is not going to grow one: it +// runs in the foreground, exits 130 and 143 on SIGINT and SIGTERM after +// canceling and settling its live tasks, and leaves being restarted to the +// OS. These commands write the unit that does the restarting, and take it +// away again. +// +// systemd only. The connector runs on Linux alone, for the reason the run +// command gives, so there is no second supervisor to write for: a launchd +// agent would supervise a process that refuses to start. + +// connectServiceUnitPrefix begins every unit this writes. The profile name +// completes it, so one machine can supervise several agents. +const connectServiceUnitPrefix = "basecamp-connect-" + +// connectServiceRestartSec is how long systemd waits before starting the +// connector again. Long enough that a profile which cannot start — a +// credential the service cannot reach, a policy file someone else can +// write — burns through systemd's default start-limit burst and lands in +// `failed` where a person can see it, rather than spinning. +const connectServiceRestartSec = 5 + +// connectServiceStartLimit bounds the restarting. Restart=always on its own +// will retry a connector that can never start — a credential the service +// cannot reach, a worker that is not installed — for as long as the machine +// is up, and `is-active` says `activating` the whole time. That is a service +// reporting health it does not have. Five starts inside five minutes, which +// at RestartSec=5 takes about twenty-five seconds, puts the unit in `failed` +// where a person and `systemctl --user is-active` both see it. +const ( + connectServiceStartLimitSec = 300 + connectServiceStartLimitN = 5 +) + +// connectServiceStopSec is how long systemd waits after SIGTERM before it +// resorts to SIGKILL. The connector spends that time canceling live +// workers and posting their `ended at shutdown` completions; killed early, +// those completions are never written and the records need `redispatch` by +// hand. Ninety seconds is systemd's own default, stated here rather than +// inherited so that changing it is a decision someone made. +const connectServiceStopSec = 90 + +// connectServiceGOOS is the platform these commands answer for. A variable +// so the refusal on a platform the connector does not run on is testable +// from the one platform it does. +var connectServiceGOOS = runtime.GOOS + +// runSystemctl runs systemctl for the calling user. A variable so tests +// drive install and uninstall without a session bus. +var runSystemctl = func(args ...string) ([]byte, error) { + path, err := exec.LookPath("systemctl") + if err != nil { + return nil, err + } + return exec.Command(path, append([]string{"--user"}, args...)...).CombinedOutput() //nolint:gosec // path from LookPath, args are literals and validated ids +} + +// connectServiceFlags are the parts of a run this unit records. +// +// Only the flags that describe a standing service are here. `--since` +// enters the feed at one id and is a one-shot: written into a unit it would +// re-enter there on every restart, which is the opposite of resuming, so it +// is not accepted. `--driver` and `--acp-adapters` are overrides of what +// connect.json already holds, and connect.json is where a service's +// settings belong — that also keeps every value in the unit either a +// validated profile name or a run of digits, with no free text to quote. +type connectServiceFlags struct { + projects []string + shadow bool + hold bool + noEnable bool +} + +// newConnectServiceCmd is the service group. +func newConnectServiceCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "service", + Short: "Install or remove the OS service that keeps the connector running", + Long: `Install or remove a systemd user unit that runs the connector and starts +it again when it stops. + +The connector itself stays in the foreground: the unit is the supervisor, +as it is for any other long-running command. One unit per profile, so a +machine can serve several agents.`, + Args: cobra.NoArgs, + } + cmd.AddCommand(newConnectServiceInstallCmd(), newConnectServiceUninstallCmd()) + return cmd +} + +func newConnectServiceInstallCmd() *cobra.Command { + var f connectServiceFlags + cmd := &cobra.Command{ + Use: "install", + Short: "Write and start the systemd user unit for a profile's connector", + Long: `Write a systemd user unit that runs this profile's connector, then enable +and start it. The unit restarts the connector whenever it stops, so a +crash or a kill brings it back. + +The unit records the run you asked for: the profile, and any --project, +--shadow or --hold. It does not take --since, which enters the feed at one +id and would re-enter there on every restart instead of resuming; nor +--driver or --acp-adapters, which override what connect.json holds, and a +standing service's settings belong in connect.json. + +Installing again over an existing unit rewrites it and restarts the +service with the new arguments. + +Examples: + basecamp connect service install -P agent + basecamp connect service install -P agent --project 12345 --shadow + basecamp connect service install -P agent --no-enable`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + return runConnectServiceInstall(cmd, &f) + }, + } + fl := cmd.Flags() + fl.Var((*repeatedString)(&f.projects), "project", "Only hear events in this project id (repeatable; default every project the agent can see)") + fl.BoolVar(&f.shadow, "shadow", false, "Run in shadow: admit and log in an isolated state directory, dispatch and post nothing") + fl.BoolVar(&f.hold, "hold", false, "Run with the durable hold set: intake and admission run, nothing is dispatched or posted") + fl.BoolVar(&f.noEnable, "no-enable", false, "Write the unit but do not enable or start it") + return cmd +} + +func newConnectServiceUninstallCmd() *cobra.Command { + cmd := &cobra.Command{ + Use: "uninstall", + Short: "Stop the profile's connector service and remove its unit", + Long: `Stop and disable the profile's connector service, then remove its unit +file. + +The connector is stopped with SIGTERM, so it cancels its live workers, +posts their completions and exits, the same as an interrupt at a terminal. +Nothing else is removed: the ledger, the checkpoint and connect.json stay +where they are, and installing again resumes from them. + +Removing a unit that is not there succeeds and says so. + +Examples: + basecamp connect service uninstall -P agent`, + Args: cobra.NoArgs, + RunE: func(cmd *cobra.Command, _ []string) error { + return runConnectServiceUninstall(cmd) + }, + } + return cmd +} + +// connectServiceProfile is the profile these commands act on, refused here +// rather than by the unit failing later. +func connectServiceProfile(app *appctx.App) (string, error) { + if app == nil { + return "", fmt.Errorf("app not initialized") + } + if !connectSupportedOS(connectServiceGOOS) { + return "", connectUnsupportedOSError(connectServiceGOOS) + } + name := app.Config.ActiveProfile + if name == "" { + return "", output.ErrUsageHint("The connector's service needs the agent's profile", "Pass -P/--profile , a profile set up with `basecamp connect setup`.") + } + if !isValidProfileName(name) { + return "", output.ErrUsage(fmt.Sprintf("Invalid profile name %q: use only letters, numbers, hyphens, and underscores", name)) + } + return name, nil +} + +// connectServiceUnitName is the unit file's name for a profile. +func connectServiceUnitName(profile string) string { + return connectServiceUnitPrefix + profile + ".service" +} + +// connectServiceUnitPath is where the unit goes: the user unit directory +// under the XDG config home, which is where `systemctl --user` looks. +func connectServiceUnitPath(profile string) (string, error) { + if !isValidProfileName(profile) { + return "", fmt.Errorf("invalid profile name %q", profile) + } + home := os.Getenv("XDG_CONFIG_HOME") + if home == "" { + h, err := os.UserHomeDir() + if err != nil { + return "", err + } + home = filepath.Join(h, ".config") + } + return filepath.Join(home, "systemd", "user", connectServiceUnitName(profile)), nil +} + +// connectServiceUnit renders the unit. +// +// The command line holds only the profile name, validated to letters, +// numbers, hyphens and underscores, and project ids parsed as numbers +// before they get here, so nothing a person typed reaches it as text. +// +// The Environment lines are the exception and are free text: they carry +// PATH and the XDG variables as install found them. They are quoted, and +// connectServiceEnv refuses any that holds a newline, which is the only +// character that could end the directive and begin another. +func connectServiceUnit(exe, profile string, projects []int64, shadow, hold bool, env []string, envFile string) string { + args := []string{"connect", "--profile", profile} + for _, id := range projects { + args = append(args, "--project", strconv.FormatInt(id, 10)) + } + if shadow { + args = append(args, "--shadow") + } + if hold { + args = append(args, "--hold") + } + + var b strings.Builder + fmt.Fprintf(&b, "# Written by `basecamp connect service install`. Edits are lost the next\n") + fmt.Fprintf(&b, "# time it runs; change the run instead and install again.\n") + fmt.Fprintf(&b, "[Unit]\n") + fmt.Fprintf(&b, "Description=Basecamp agent connector for profile %s\n", profile) + fmt.Fprintf(&b, "Documentation=https://github.com/basecamp/basecamp-cli\n") + fmt.Fprintf(&b, "After=network-online.target\n") + fmt.Fprintf(&b, "Wants=network-online.target\n") + // [Unit], not [Service]: systemd moved the start limit here in v229. + fmt.Fprintf(&b, "StartLimitIntervalSec=%d\n", connectServiceStartLimitSec) + fmt.Fprintf(&b, "StartLimitBurst=%d\n\n", connectServiceStartLimitN) + fmt.Fprintf(&b, "[Service]\n") + fmt.Fprintf(&b, "Type=simple\n") + // A systemd user manager starts from its own environment, not from the + // shell that installed this. Without these the connector would look for + // another connect.json, keep its ledger somewhere else, and run a worker + // it could not find — active, and failing every dispatch. + for _, e := range env { + fmt.Fprintf(&b, "Environment=%s\n", systemdQuote(e, false)) + } + // Where a credential goes, if the driver needs one. The leading dash + // makes it optional, so the ordinary case has no file at all; install + // never writes it, because install would be writing a secret. + if envFile != "" { + fmt.Fprintf(&b, "EnvironmentFile=-%s\n", systemdQuote(envFile, false)) + } + fmt.Fprintf(&b, "ExecStart=%s\n", systemdExecLine(exe, args)) + fmt.Fprintf(&b, "Restart=always\n") + fmt.Fprintf(&b, "RestartSec=%d\n", connectServiceRestartSec) + // The connector's own contract with a supervisor: SIGTERM cancels live + // workers, settles them and exits 143, and 143 is therefore a clean + // stop rather than a failure. + fmt.Fprintf(&b, "KillSignal=SIGTERM\n") + // mixed, not the default control-group: that signals the workers at the + // same instant as the connector, and a worker's session ending before + // the connector's context is canceled is read as StopLost rather than + // StopShutdown — a stranded attempt needing redispatch by hand, which is + // the opposite of what TimeoutStopSec above is for. Only the connector + // gets the SIGTERM; systemd still SIGKILLs whatever is left in the + // cgroup once the timeout passes. + fmt.Fprintf(&b, "KillMode=mixed\n") + fmt.Fprintf(&b, "TimeoutStopSec=%d\n", connectServiceStopSec) + fmt.Fprintf(&b, "SuccessExitStatus=143\n\n") + fmt.Fprintf(&b, "[Install]\n") + fmt.Fprintf(&b, "WantedBy=default.target\n") + return b.String() +} + +// systemdExecLine renders an ExecStart command line. +func systemdExecLine(exe string, args []string) string { + parts := make([]string, 0, len(args)+1) + parts = append(parts, systemdQuote(exe, true)) + for _, a := range args { + parts = append(parts, systemdQuote(a, true)) + } + return strings.Join(parts, " ") +} + +// systemdQuote quotes one value for a unit file. +// +// Quoting is not enough on its own. systemd expands its own % specifiers +// everywhere — %u is the user name, %% is a literal percent — and expands +// $VAR in a command line even inside double quotes. A home directory with a +// percent in it, or a path holding $HOME verbatim, would otherwise be +// rewritten into something else or refused. So % is always doubled, and $ +// is doubled only where it means anything: expandDollar is true for a +// command line and false for Environment=, which takes $ literally. +// +// Control characters are written as C escapes, which systemd reads inside +// double quotes, rather than being allowed to end the directive. +func systemdQuote(s string, expandDollar bool) string { + var b strings.Builder + b.WriteByte('"') + for _, r := range s { + switch { + case r == '\\' || r == '"': + b.WriteByte('\\') + b.WriteRune(r) + case r == '%': + b.WriteString("%%") + case r == '$' && expandDollar: + b.WriteString("$$") + case r == '\n': + b.WriteString(`\n`) + case r == '\r': + b.WriteString(`\r`) + case r == '\t': + b.WriteString(`\t`) + case r < 0x20 || r == 0x7f: + fmt.Fprintf(&b, `\x%02x`, r) + default: + b.WriteRune(r) + } + } + b.WriteByte('"') + return b.String() +} + +// connectServiceEnvKeys are the variables the unit pins. A systemd user +// manager is started at login with an environment of its own: it does not +// have the shell's PATH, and it computes XDG defaults itself. Left to it, +// the connector reads a different connect.json from the one install just +// checked, keeps its ledger somewhere else, and runs the worker by bare +// name — `claude`, `codex` — off a PATH that does not have it. The unit +// would be active and every dispatch would fail. +// +// The list is the connector's own, not a new one: driver.BaseEnv is what a +// worker may inherit, and connector.MCPServerEnv what the worker's MCP +// server needs on top. A fourth list here would drift from those two, and +// an allowlist that drifts is an allowlist that reads as configuration and +// does nothing. +func connectServiceEnvKeys() []string { + keys := append([]string{}, driver.BaseEnv...) + keys = append(keys, connector.MCPServerEnv...) + for name, secret := range connectServiceDriverEnv { + if !secret { + keys = append(keys, name) + } + } + slices.Sort(keys) + return slices.Compact(keys) +} + +// connectServiceDriverEnv is what the drivers read beyond driver.BaseEnv, +// and whether each one authenticates somebody. Half of them do, and a unit +// file is not where a credential goes: `systemctl cat` prints it, a backup +// copies it, and driver.BaseEnv already says the worker environment carries +// "nothing that authenticates anyone". +// +// So the settings are pinned and the credentials are not. A credential the +// service needs goes in the environment file the unit reads, which the +// person owns and install never writes. +// +// TestConnectServiceClassifiesEveryDriverVariable keeps this map level with +// the drivers' own lists, so a new one is a failing test rather than a +// variable that quietly stops reaching a worker. +var connectServiceDriverEnv = map[string]bool{ + "CLAUDE_CONFIG_DIR": false, + "ANTHROPIC_BASE_URL": false, + "ANTHROPIC_API_KEY": true, + "CODEX_HOME": false, + "CODEX_API_KEY": true, + "OPENAI_BASE_URL": false, + "OPENAI_API_KEY": true, +} + +// connectServiceMissingCredentials names the driver credentials this shell +// has and the service will not, so a person hears it at install rather than +// discovering it in a dispatch that failed. +func connectServiceMissingCredentials() []string { + var missing []string + for name, secret := range connectServiceDriverEnv { + if !secret { + continue + } + if v, ok := os.LookupEnv(name); ok && v != "" { + missing = append(missing, name) + } + } + slices.Sort(missing) + return missing +} + +// connectServiceEnvFile is where the unit reads the credentials install +// will not copy: beside the profile's own connector state, owner-only. +func connectServiceEnvFile(profile string) (string, error) { + path, err := setup.Path(config.GlobalConfigDir(), profile) + if err != nil { + return "", err + } + return filepath.Join(filepath.Dir(path), "service.env"), nil +} + +// connectServiceEnv is the environment to pin, in unit form. +func connectServiceEnv() ([]string, error) { + var env []string + for _, k := range connectServiceEnvKeys() { + v, ok := os.LookupEnv(k) + if !ok || v == "" { + continue + } + if strings.ContainsAny(v, "\n\r\x00") { + return nil, output.ErrUsageHint( + fmt.Sprintf("%s holds a newline or a null byte, which cannot go into a unit file", k), + "Fix "+k+" in this shell, then install again. No unit was written.") + } + env = append(env, k+"="+v) + } + return env, nil +} + +// connectServiceExecutable is the path the unit should run, which is the +// stable one rather than the resolved one. +// +// os.Executable reads /proc/self/exe, which the kernel has already followed +// to the real file. On a mise or Nix installation that is a versioned store +// path behind a shim: baking it into a unit means the service keeps running +// the version installed today after an upgrade, and fails to start at all +// once that version is collected. So the name the caller was invoked by is +// resolved through PATH and made absolute, without following the last +// symlink — the shim is the point. +// +// The resolved path is the fallback, for a binary invoked by a name that no +// longer finds it. +func connectServiceExecutable() (string, error) { + if len(os.Args) > 0 && os.Args[0] != "" { + if found, err := exec.LookPath(os.Args[0]); err == nil { + if abs, err := filepath.Abs(found); err == nil { + return abs, nil + } + } + } + exe, err := os.Executable() + if err != nil { + return "", fmt.Errorf("cannot find this program's own path, which the unit has to name: %w", err) + } + return exe, nil +} + +func runConnectServiceInstall(cmd *cobra.Command, f *connectServiceFlags) error { + app := appctx.FromContext(cmd.Context()) + profile, err := connectServiceProfile(app) + if err != nil { + return err + } + projects, err := parseProjectIDs(f.projects) + if err != nil { + return err + } + sort.Slice(projects, func(i, j int) bool { return projects[i] < projects[j] }) + + // A profile with no setup would give a unit that starts, fails, and is + // restarted until systemd gives up. Refuse it here, where the reason + // can be read, rather than in a journal five restarts later. + if err := connectServiceRequireSetup(profile); err != nil { + return err + } + + exe, err := connectServiceExecutable() + if err != nil { + return err + } + + path, err := connectServiceUnitPath(profile) + if err != nil { + return err + } + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { + return fmt.Errorf("cannot create %s: %w", richtext.SanitizeSingleLine(filepath.Dir(path)), err) + } + env, err := connectServiceEnv() + if err != nil { + return err + } + envFile, err := connectServiceEnvFile(profile) + if err != nil { + return err + } + unit := connectServiceUnit(exe, profile, projects, f.shadow, f.hold, env, envFile) + // Written whole or not at all. A direct write truncates the unit that is + // enabled and running now, so an install that ran out of disk would + // leave a half a unit: systemd keeps running from what it already loaded + // and then refuses to load it at the next boot, which is a failure a + // reboot away from whoever caused it. + if err := writeFileAtomic(path, []byte(unit), 0o600); err != nil { + return fmt.Errorf("cannot write %s: %w", richtext.SanitizeSingleLine(path), err) + } + + name := connectServiceUnitName(profile) + summary := fmt.Sprintf("Wrote %s", path) + enabled := false + if !f.noEnable { + if out, err := connectServiceEnable(name, path); err != nil { + // systemctl says nothing at all when it is missing, or when the + // failure is in running it rather than in what it was asked; the + // error is the only account of that, so it is not thrown away. + why := strings.TrimSpace(string(out)) + if why == "" { + why = err.Error() + } + return output.ErrUsageHint( + fmt.Sprintf("Wrote %s, but could not start it: %s", richtext.SanitizeSingleLine(path), richtext.SanitizeSingleLine(why)), + "Start it yourself: systemctl --user daemon-reload && systemctl --user enable "+name+" && systemctl --user restart "+name) + } + enabled = true + summary = fmt.Sprintf("Wrote %s and started %s", path, name) + if warning := connectServiceLingerWarning(cmd.Context()); warning != "" { + summary += ". " + warning + } + } + if missing := connectServiceMissingCredentials(); len(missing) > 0 { + summary += fmt.Sprintf(". %s %s set here and will not be in the service, which never carries a credential: put %s in %s (owner-only) and the unit will read it", + strings.Join(missing, ", "), + map[bool]string{true: "is", false: "are"}[len(missing) == 1], + map[bool]string{true: "it", false: "them"}[len(missing) == 1], + envFile) + } + return app.OK(map[string]any{"unit": name, "path": path, "enabled": enabled}, + output.WithSummary(summary)) +} + +// connectServiceRequireSetup refuses a profile the connector could not run. +func connectServiceRequireSetup(profile string) error { + path, err := setup.Path(config.GlobalConfigDir(), profile) + if err != nil { + return output.ErrUsage(err.Error()) + } + f, err := setup.Load(path) + switch { + case errors.Is(err, os.ErrNotExist): + return output.ErrNotFoundHint("connect.json for profile", profile, + "The profile has not been set up, so the service would restart a connector that cannot start. Set it up: basecamp connect setup -P "+richtext.ShellQuote(profile)+" --operator-profile '' --serve ") + case err != nil: + return output.ErrUsageHint("connect.json cannot be used: "+setup.ErrorText(err), + "No unit was written. Fix or remove "+richtext.SanitizeSingleLine(path)+", then run setup again.") + } + if f.Profile != profile { + return output.ErrUsageHint(fmt.Sprintf("%s names profile %q, not %q", richtext.SanitizeSingleLine(path), f.Profile, profile), + "No unit was written. Remove "+richtext.SanitizeSingleLine(path)+" and run setup again for this profile.") + } + return nil +} + +// connectServiceEnable reloads, enables and starts the unit. +// +// enable without --now, then restart. `enable --now` starts it, and a +// restart straight after would stop that process and start another — +// and the first one can have begun intake, or a dispatch, in between. +// restart starts an inactive unit and refreshes a running one, so it is +// both the first start and the way a reinstall picks up a new command +// line. +func connectServiceEnable(unit, wantPath string) ([]byte, error) { + if out, err := runSystemctl("daemon-reload"); err != nil { + return out, err + } + // Before anything is started: the unit the manager found has to be the + // unit that was just written. A user manager computes its own search + // path from its own environment, so a custom XDG_CONFIG_HOME set in + // this shell alone puts the file somewhere it will never look — and + // `enable` would go on to succeed against some older unit, or fail + // with a message about a unit that does exist. + if out, err := connectServiceCheckFragment(unit, wantPath); err != nil { + return out, err + } + if out, err := runSystemctl("enable", unit); err != nil { + return out, err + } + // A unit sitting in start-limit-hit refuses to restart until its window + // expires — "start of the service was attempted too often" — so a + // reinstall made right after fixing whatever stopped it would do + // nothing for five minutes. Clearing the counter is what makes + // installing again the way to recover, which is what install says it + // is. Best effort: a unit that was never failed has nothing to clear + // and systemctl says so. + _, _ = runSystemctl("reset-failed", unit) + return runSystemctl("restart", unit) +} + +// connectServiceCheckFragment asks systemd which file it reads for this +// unit, and refuses when that is not the file just written. +func connectServiceCheckFragment(unit, wantPath string) ([]byte, error) { + out, err := runSystemctl("show", "-p", "FragmentPath", "--value", unit) + if err != nil { + return out, err + } + got := strings.TrimSpace(string(out)) + if got == wantPath { + return nil, nil + } + where := "nowhere: the user manager does not see a unit by that name" + if got != "" { + where = got + } + return nil, fmt.Errorf("systemd reads %s for %s, not the file just written; "+ + "the user manager's unit search path is computed from its own environment, not from this shell's XDG_CONFIG_HOME", where, unit) +} + +// connectServiceLingerWarning reports when the user manager will not be +// running after a reboot, so the unit's own "Restart" promise stops at the +// next power cut. +func connectServiceLingerWarning(ctx context.Context) string { + // The process's own user, not $USER: exec runs no shell, so an unset + // USER would be passed to loginctl as the four characters "$USER" and + // the lookup would fail silently — no warning, on exactly the machines + // most likely to need one. USER can also name a different account than + // the one whose manager this unit is being installed into. + me, err := user.Current() + if err != nil { + return "" + } + name := me.Username + // Bounded: loginctl talks to logind, and an install must not hang on a + // warning it can do without. + ctx, cancel := context.WithTimeout(ctx, 5*time.Second) + defer cancel() + out, err := exec.CommandContext(ctx, "loginctl", "show-user", name, "-p", "Linger", "--value").Output() //nolint:gosec // fixed argv, the name is this process's own user + if err != nil { + return "" + } + if strings.TrimSpace(string(out)) == "yes" { + return "" + } + return "This user's systemd manager starts at login, so the service will not come back after a reboot until someone logs in. " + + "To have it start at boot: sudo loginctl enable-linger " + richtext.ShellQuote(name) +} + +func runConnectServiceUninstall(cmd *cobra.Command) error { + app := appctx.FromContext(cmd.Context()) + profile, err := connectServiceProfile(app) + if err != nil { + return err + } + path, err := connectServiceUnitPath(profile) + if err != nil { + return err + } + name := connectServiceUnitName(profile) + + // A missing file does not mean a stopped connector: systemd goes on + // running a unit it has already loaded after its file is removed by + // hand. Returning "nothing to remove" here would report the opposite of + // what uninstall promises, so the stop is attempted either way and only + // the wording changes. + _, statErr := os.Stat(path) + hadFile := statErr == nil + + // Stopping is best effort: the unit file must go even when there is no + // session bus to talk to, or an uninstall on a machine without a user + // session would leave the unit behind for the next login to start. + // Best effort is not the same as unreported, though — a stop that + // failed may have left the connector running, and saying "Stopped" then + // would be the summary telling someone the opposite of what happened. + stopOut, stopErr := runSystemctl("disable", "--now", name) + if err := os.Remove(path); err != nil && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("cannot remove %s: %w", richtext.SanitizeSingleLine(path), err) + } + _, _ = runSystemctl("daemon-reload") + + summary := fmt.Sprintf("Stopped %s and removed %s", name, path) + if !hadFile { + summary = fmt.Sprintf("No unit file at %s; stopped %s in case it was still loaded", path, name) + } + stopped := true + // A unit systemd has never heard of is the idempotent case, not a + // failure: uninstalling twice must succeed. + if stopErr != nil && !connectServiceUnknownUnit(stopOut) { + stopped = false + why := strings.TrimSpace(string(stopOut)) + if why == "" { + why = stopErr.Error() + } + summary = fmt.Sprintf("Removed %s, but could not stop %s, which may still be running: %s", + path, name, richtext.SanitizeSingleLine(why)) + } + return app.OK(map[string]any{"unit": name, "path": path, "removed": hadFile, "stopped": stopped}, + output.WithSummary(summary)) +} + +// connectServiceUnknownUnit reports whether systemctl refused because it has +// never heard of the unit, which is what uninstalling an uninstalled service +// looks like and is the one refusal that counts as success. +// +// "no such file or directory" is deliberately not among these. systemctl +// says it for a missing unit and also for "Failed to connect to bus: No +// such file or directory", which is a different thing entirely: the bus is +// gone, nothing was stopped, and the connector may still be running. +// Treating that as an uninstalled service would report the connector +// stopped when it is not. +func connectServiceUnknownUnit(out []byte) bool { + text := strings.ToLower(string(out)) + if strings.Contains(text, "failed to connect to bus") || strings.Contains(text, "connection refused") { + return false + } + return strings.Contains(text, "not loaded") || + strings.Contains(text, "does not exist") +} + +// writeFileAtomic writes data to path through a temporary file in the same +// directory, so a reader never sees a partial file and a failed write leaves +// whatever was there before. +func writeFileAtomic(path string, data []byte, perm os.FileMode) error { + dir := filepath.Dir(path) + tmp, err := os.CreateTemp(dir, "."+filepath.Base(path)+".*.tmp") + if err != nil { + return err + } + tmpPath := tmp.Name() + defer func() { _ = os.Remove(tmpPath) }() + + if err := tmp.Chmod(perm); err != nil { + _ = tmp.Close() + return err + } + if _, err := tmp.Write(data); err != nil { + _ = tmp.Close() + return err + } + if err := tmp.Sync(); err != nil { + _ = tmp.Close() + return err + } + if err := tmp.Close(); err != nil { + return err + } + return os.Rename(tmpPath, path) +} diff --git a/internal/commands/connect_service_test.go b/internal/commands/connect_service_test.go new file mode 100644 index 000000000..db504f0b7 --- /dev/null +++ b/internal/commands/connect_service_test.go @@ -0,0 +1,676 @@ +//go:build linux + +package commands + +import ( + "bytes" + "context" + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/basecamp/basecamp-cli/internal/appctx" + "github.com/basecamp/basecamp-cli/internal/config" + "github.com/basecamp/basecamp-cli/internal/connector" + "github.com/basecamp/basecamp-cli/internal/connector/admission" + "github.com/basecamp/basecamp-cli/internal/connector/driver" + "github.com/basecamp/basecamp-cli/internal/connector/driver/acp" + "github.com/basecamp/basecamp-cli/internal/connector/driver/claude" + "github.com/basecamp/basecamp-cli/internal/connector/driver/codex" + "github.com/basecamp/basecamp-cli/internal/connector/setup" + "github.com/basecamp/basecamp-cli/internal/output" +) + +// connectServiceHome points the config and unit directories at a temp home +// and records the systemctl calls instead of making them. +func connectServiceHome(t *testing.T) *[][]string { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(home, ".config")) + t.Setenv("XDG_STATE_HOME", filepath.Join(home, ".local", "state")) + + var calls [][]string + prev := runSystemctl + runSystemctl = func(args ...string) ([]byte, error) { + calls = append(calls, args) + // A working manager reads the file that was just written, which is + // what the fragment check asks it. + if len(args) > 0 && args[0] == "show" { + path, err := connectServiceUnitPath("agent") + if err != nil { + return nil, err + } + return []byte(path + "\n"), nil + } + return nil, nil + } + t.Cleanup(func() { runSystemctl = prev }) + return &calls +} + +// writeConnectSetup puts a connect.json where the service commands look for +// one, so the profile reads as set up. +func writeConnectSetup(t *testing.T, profile string) { + t.Helper() + path, err := setup.Path(config.GlobalConfigDir(), profile) + require.NoError(t, err) + require.NoError(t, os.MkdirAll(filepath.Dir(path), 0o700)) + f := setup.File{ + Version: 1, + Profile: profile, + AccountID: "2914079", + Agent: setup.Agent{PersonID: 52007412, Kind: "bot_user", IdentityID: 4242}, + Trust: admission.Trust{Mode: admission.TrustOperator, OperatorID: 26909558}, + Projects: map[int64]admission.Project{48699913: {}}, + Driver: "spawn", + Concurrency: 1, + Deadline: setup.Duration(30 * time.Minute), + } + b, err := json.Marshal(f) + require.NoError(t, err) + require.NoError(t, os.WriteFile(path, b, 0o600)) +} + +func connectServiceApp(t *testing.T, profile string) (*appctx.App, *bytes.Buffer) { + t.Helper() + cfg, err := config.Load(config.FlagOverrides{}) + require.NoError(t, err) + cfg.ActiveProfile = profile + said := &bytes.Buffer{} + return &appctx.App{ + Config: cfg, + Output: output.New(output.Options{Format: output.FormatStyled, Writer: said}), + }, said +} + +//nolint:contextcheck // the context is handed to the command, not to a call +func runConnectServiceCmd(t *testing.T, app *appctx.App, args ...string) (string, error) { + t.Helper() + cmd := NewConnectCmd() + cmd.SetArgs(append([]string{"service"}, args...)) + cmd.SetContext(appctx.WithApp(context.Background(), app)) + var out bytes.Buffer + cmd.SetOut(&out) + cmd.SetErr(&out) + cmd.SilenceErrors = true + cmd.SilenceUsage = true + err := cmd.Execute() + return out.String(), err +} + +// The unit's whole point: systemd starts the connector again when it stops. +// Without Restart the unit is a launcher, not a supervisor, and a killed +// connector stays dead. +func TestConnectServiceUnitRestartsTheConnector(t *testing.T) { + unit := connectServiceUnit("/usr/bin/basecamp", "agent", nil, false, false, nil, "") + + assert.Contains(t, unit, "\nRestart=always\n", "a unit that does not restart supervises nothing") + assert.Contains(t, unit, "\nRestartSec=5\n") + assert.Contains(t, unit, "\nWantedBy=default.target\n", "without an [Install] section the unit cannot be enabled") +} + +// SIGTERM is how the connector is asked to stop, and it spends its last +// seconds canceling live workers and posting their completions. A unit +// that killed it outright would leave those records needing redispatch by +// hand, and would count its own 143 as a crash. +func TestConnectServiceUnitLetsTheConnectorSettleItsWorkers(t *testing.T) { + unit := connectServiceUnit("/usr/bin/basecamp", "agent", nil, false, false, nil, "") + + assert.Contains(t, unit, "\nKillSignal=SIGTERM\n") + assert.Contains(t, unit, "\nTimeoutStopSec=90\n") + assert.Contains(t, unit, "\nSuccessExitStatus=143\n", "143 is the connector's clean exit on SIGTERM, not a failure") +} + +func TestConnectServiceUnitRecordsTheRun(t *testing.T) { + unit := connectServiceUnit("/usr/bin/basecamp", "agent", []int64{12345, 67890}, true, true, nil, "") + + exec := unitDirective(t, unit, "ExecStart") + assert.Equal(t, + `"/usr/bin/basecamp" "connect" "--profile" "agent" "--project" "12345" "--project" "67890" "--shadow" "--hold"`, + exec) +} + +// Every word in the command line is quoted, so a path with a space in it +// stays one argument rather than becoming two. +func TestConnectServiceUnitQuotesTheExecutablePath(t *testing.T) { + unit := connectServiceUnit(`/home/a b/go bin/basecamp`, "agent", nil, false, false, nil, "") + + assert.Contains(t, unitDirective(t, unit, "ExecStart"), `"/home/a b/go bin/basecamp" "connect"`) +} + +// --since enters the feed at one id. In a unit it would re-enter there on +// every restart instead of resuming from the ledger, so the flag is not +// offered at all; likewise the two overrides that belong in connect.json. +func TestConnectServiceInstallRefusesTheFlagsAUnitMustNotCarry(t *testing.T) { + connectServiceHome(t) + writeConnectSetup(t, "agent") + + for _, flag := range []string{"--since=5", "--driver=acp", "--acp-adapters=/tmp/a"} { + app, _ := connectServiceApp(t, "agent") + out, err := runConnectServiceCmd(t, app, "install", flag) + require.Error(t, err, flag) + assert.Contains(t, out+err.Error(), "unknown flag", flag) + } +} + +func TestConnectServiceInstallWritesAndStartsTheUnit(t *testing.T) { + calls := connectServiceHome(t) + writeConnectSetup(t, "agent") + app, _ := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "install", "--project", "48699913") + require.NoError(t, err) + + path, err := connectServiceUnitPath("agent") + require.NoError(t, err) + b, err := os.ReadFile(path) //nolint:gosec // path built from a temp home + require.NoError(t, err) + assert.Contains(t, string(b), `"--profile" "agent" "--project" "48699913"`) + + info, err := os.Stat(path) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o600), info.Mode().Perm()) + + assert.Equal(t, [][]string{ + {"daemon-reload"}, + {"show", "-p", "FragmentPath", "--value", "basecamp-connect-agent.service"}, + {"enable", "basecamp-connect-agent.service"}, + {"reset-failed", "basecamp-connect-agent.service"}, + {"restart", "basecamp-connect-agent.service"}, + }, *calls, "reloaded, checked to be the file systemd reads, enabled, cleared and restarted, or the new command line never runs") +} + +// A unit for a profile that was never set up would start a connector that +// refuses to run, and systemd would restart it until it gave up. The +// refusal belongs here, where its reason can be read. +func TestConnectServiceInstallRefusesAProfileWithNoSetup(t *testing.T) { + calls := connectServiceHome(t) + app, _ := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "install") + require.Error(t, err) + assert.Contains(t, err.Error(), "connect.json") + + path, err := connectServiceUnitPath("agent") + require.NoError(t, err) + assert.NoFileExists(t, path, "no unit is written for a profile that cannot run") + assert.Empty(t, *calls, "and systemd is not asked to start one") +} + +func TestConnectServiceInstallNoEnableOnlyWritesTheUnit(t *testing.T) { + calls := connectServiceHome(t) + writeConnectSetup(t, "agent") + app, _ := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "install", "--no-enable") + require.NoError(t, err) + + path, err := connectServiceUnitPath("agent") + require.NoError(t, err) + assert.FileExists(t, path) + assert.Empty(t, *calls) +} + +func TestConnectServiceUninstallStopsAndRemoves(t *testing.T) { + calls := connectServiceHome(t) + writeConnectSetup(t, "agent") + app, _ := connectServiceApp(t, "agent") + _, err := runConnectServiceCmd(t, app, "install", "--no-enable") + require.NoError(t, err) + *calls = nil + + _, err = runConnectServiceCmd(t, app, "uninstall") + require.NoError(t, err) + + path, err := connectServiceUnitPath("agent") + require.NoError(t, err) + assert.NoFileExists(t, path) + assert.Equal(t, [][]string{ + {"disable", "--now", "basecamp-connect-agent.service"}, + {"daemon-reload"}, + }, *calls) +} + +// One unit per profile, so a machine can supervise several agents without +// one install standing on another's. +func TestConnectServiceUnitIsNamedPerProfile(t *testing.T) { + assert.Equal(t, "basecamp-connect-agent.service", connectServiceUnitName("agent")) + assert.Equal(t, "basecamp-connect-other_bot.service", connectServiceUnitName("other_bot")) + + a, err := connectServiceUnitPath("agent") + require.NoError(t, err) + b, err := connectServiceUnitPath("other") + require.NoError(t, err) + assert.NotEqual(t, a, b) + assert.Equal(t, "user", filepath.Base(filepath.Dir(a)), "systemctl --user reads systemd/user") +} + +// A profile name is the only part of the unit's name a person chooses, and +// a name with a slash or a newline in it would name another file or add a +// directive. It never gets that far: the same validation the run command +// applies refuses it first. +func TestConnectServiceRefusesAProfileNameThatIsNotOne(t *testing.T) { + for _, name := range []string{"../evil", "a b", "a\nExecStart=/bin/sh", "a/b"} { + _, err := connectServiceUnitPath(name) + assert.Error(t, err, name) + } +} + +// A connector that can never start — a credential the service cannot +// reach, a worker that is not installed — must become visible rather than +// be retried forever. Restart=always on its own reports `activating` for as +// long as the machine is up, which is a service claiming health it has not +// got. +func TestConnectServiceUnitStopsRetryingAConnectorThatCannotStart(t *testing.T) { + unit := connectServiceUnit("/usr/bin/basecamp", "agent", nil, false, false, nil, "") + + assert.Contains(t, unit, "\nStartLimitIntervalSec=300\n") + assert.Contains(t, unit, "\nStartLimitBurst=5\n") +} + +// The user manager does not have the shell's PATH and computes the XDG +// defaults itself, so a unit that pins nothing runs a connector that reads +// another connect.json, keeps its ledger elsewhere, and looks for its +// worker on a PATH that has not got it — active, and failing every +// dispatch. +func TestConnectServiceUnitPinsTheEnvironmentInstallVerified(t *testing.T) { + unit := connectServiceUnit("/usr/bin/basecamp", "agent", nil, false, false, + []string{"PATH=/home/a/bin:/usr/bin", "XDG_CONFIG_HOME=/home/a/.config"}, "") + + assert.Contains(t, unit, `Environment="PATH=/home/a/bin:/usr/bin"`) + assert.Contains(t, unit, `Environment="XDG_CONFIG_HOME=/home/a/.config"`) +} + +func TestConnectServiceEnvRefusesAValueThatWouldSplitTheUnitFile(t *testing.T) { + t.Setenv("PATH", "/usr/bin\nExecStart=/bin/sh") + + _, err := connectServiceEnv() + require.Error(t, err) + assert.Contains(t, err.Error(), "PATH") +} + +// enable --now starts it, and a restart straight after would stop that +// process and start another — with intake, or a dispatch, possibly already +// begun in between. +func TestConnectServiceInstallStartsTheConnectorOnce(t *testing.T) { + calls := connectServiceHome(t) + writeConnectSetup(t, "agent") + app, _ := connectServiceApp(t, "agent") + + require.NoError(t, runConnectServiceInstallForTest(t, app)) + + for _, c := range *calls { + assert.NotContains(t, c, "--now", "enable --now starts a process that the restart then kills: %v", c) + } +} + +// systemd has to be reading the file that was just written. A custom +// XDG_CONFIG_HOME set in this shell alone puts the unit where the user +// manager will never look, and enable would go on to succeed against some +// older unit. +func TestConnectServiceInstallRefusesWhenSystemdReadsAnotherFile(t *testing.T) { + connectServiceHome(t) + writeConnectSetup(t, "agent") + prev := runSystemctl + runSystemctl = func(args ...string) ([]byte, error) { + if len(args) > 0 && args[0] == "show" { + return []byte("/etc/systemd/user/basecamp-connect-agent.service\n"), nil + } + return nil, nil + } + t.Cleanup(func() { runSystemctl = prev }) + app, _ := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "install") + require.Error(t, err) + assert.Contains(t, err.Error(), "/etc/systemd/user/basecamp-connect-agent.service") +} + +// The platform gate is the run command's, so a Mac is refused before a unit +// is written for a connector that would not start on it. +func TestConnectServiceRefusesAPlatformTheConnectorDoesNotRunOn(t *testing.T) { + connectServiceHome(t) + writeConnectSetup(t, "agent") + prev := connectServiceGOOS + connectServiceGOOS = "darwin" + t.Cleanup(func() { connectServiceGOOS = prev }) + app, _ := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "install") + require.Error(t, err) + assert.Contains(t, err.Error(), connectLinuxOnlyReason) + + path, err := connectServiceUnitPath("agent") + require.NoError(t, err) + assert.NoFileExists(t, path) +} + +// When systemctl is missing, or fails in the running rather than in what it +// was asked, its combined output is empty and the error is the only account +// there is. +func TestConnectServiceInstallKeepsTheReasonWhenSystemctlSaysNothing(t *testing.T) { + connectServiceHome(t) + writeConnectSetup(t, "agent") + prev := runSystemctl + runSystemctl = func(_ ...string) ([]byte, error) { + return nil, errors.New("exec: \"systemctl\": executable file not found in $PATH") + } + t.Cleanup(func() { runSystemctl = prev }) + app, _ := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "install") + require.Error(t, err) + assert.Contains(t, err.Error(), "executable file not found") +} + +// runConnectServiceInstallForTest runs install and returns its error. +func runConnectServiceInstallForTest(t *testing.T, app *appctx.App) error { + t.Helper() + _, err := runConnectServiceCmd(t, app, "install") + return err +} + +// systemd expands its own %% specifiers everywhere and $VAR in a command +// line even inside double quotes, so a home directory with a percent in it, +// or a path holding $HOME verbatim, would be rewritten into something else +// before the connector ever ran. +func TestConnectServiceUnitSurvivesAPathSystemdWouldRewrite(t *testing.T) { + unit := connectServiceUnit(`/opt/%u/$agent/base"camp`, "agent", nil, false, false, + []string{`PATH=/opt/%u/bin:/x$y`}, "") + + assert.Contains(t, unitDirective(t, unit, "ExecStart"), `"/opt/%%u/$$agent/base\"camp"`, + "a command line expands both %% and $") + assert.Contains(t, unit, `Environment="PATH=/opt/%%u/bin:/x$y"`, + "Environment expands %% but takes $ literally, so doubling it there would corrupt the value") +} + +// A control character in a path must not end the directive and begin +// another. +func TestConnectServiceUnitEscapesControlCharacters(t *testing.T) { + unit := connectServiceUnit("/opt/a\tb/basecamp", "agent", nil, false, false, nil, "") + + exec := unitDirective(t, unit, "ExecStart") + assert.Contains(t, exec, `\t`) + assert.NotContains(t, exec, "\t", "a real tab is written as an escape, not passed through") +} + +// The unit gives up after five failed starts, and systemd then refuses to +// restart it until the window passes — "start of the service was attempted +// too often". So the install that follows a fix has to clear the counter, +// or installing again would do nothing for five minutes, which is what the +// command tells people to do. +func TestConnectServiceInstallClearsAFailedUnitBeforeStartingIt(t *testing.T) { + calls := connectServiceHome(t) + writeConnectSetup(t, "agent") + app, _ := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "install") + require.NoError(t, err) + + order := make([]string, 0, len(*calls)) + for _, c := range *calls { + order = append(order, c[0]) + } + require.Contains(t, order, "reset-failed") + assert.Less(t, indexOfCall(order, "reset-failed"), indexOfCall(order, "restart"), + "clearing the counter after the restart would be too late") +} + +// A stop that failed may have left the connector running, and a summary +// that said "Stopped" would be telling someone the opposite of what +// happened. +func TestConnectServiceUninstallSaysWhenItCouldNotStopTheConnector(t *testing.T) { + connectServiceHome(t) + writeConnectSetup(t, "agent") + app, said := connectServiceApp(t, "agent") + _, err := runConnectServiceCmd(t, app, "install", "--no-enable") + require.NoError(t, err) + + prev := runSystemctl + runSystemctl = func(args ...string) ([]byte, error) { + if len(args) > 0 && args[0] == "disable" { + return []byte("Failed to disable unit: Connection reset by peer"), errors.New("exit status 1") + } + return nil, nil + } + t.Cleanup(func() { runSystemctl = prev }) + said.Reset() + + _, err = runConnectServiceCmd(t, app, "uninstall") + require.NoError(t, err, "the unit file still has to go") + assert.Contains(t, said.String(), "may still be running") + assert.NotContains(t, said.String(), "Stopped basecamp-connect-agent.service") + + path, err := connectServiceUnitPath("agent") + require.NoError(t, err) + assert.NoFileExists(t, path) +} + +func indexOfCall(order []string, want string) int { + for i, c := range order { + if c == want { + return i + } + } + return -1 +} + +// os.Executable reads /proc/self/exe, which the kernel has already followed +// to the real file — on a mise or Nix install, a versioned store path behind +// a shim. Baking that into a unit means the service keeps running today's +// version after an upgrade, and stops starting at all once it is collected. +func TestConnectServiceInstallNamesTheStablePathNotTheResolvedOne(t *testing.T) { + connectServiceHome(t) + writeConnectSetup(t, "agent") + + // A shim on PATH pointing at a versioned target, as mise lays it out. + bin := t.TempDir() + store := filepath.Join(bin, "basecamp-1.2.3") + require.NoError(t, os.WriteFile(store, []byte("#!/bin/sh\n"), 0o700)) + shim := filepath.Join(bin, "basecamp-shim") + require.NoError(t, os.Symlink(store, shim)) + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) + + prev := os.Args + os.Args = []string{"basecamp-shim"} + t.Cleanup(func() { os.Args = prev }) + + app, _ := connectServiceApp(t, "agent") + _, err := runConnectServiceCmd(t, app, "install", "--no-enable") + require.NoError(t, err) + + path, err := connectServiceUnitPath("agent") + require.NoError(t, err) + b, err := os.ReadFile(path) //nolint:gosec // path built from a temp home + require.NoError(t, err) + assert.Contains(t, string(b), shim, "the unit runs the stable name") + assert.NotContains(t, string(b), store, "not the version behind it, which an upgrade replaces") +} + +// A direct write truncates the unit that is enabled and running now, so an +// install that ran out of disk would leave half a unit: systemd keeps going +// from what it loaded, then refuses to load it at the next boot. +func TestConnectServiceInstallLeavesTheOldUnitWhenItCannotWriteTheNew(t *testing.T) { + connectServiceHome(t) + writeConnectSetup(t, "agent") + app, _ := connectServiceApp(t, "agent") + _, err := runConnectServiceCmd(t, app, "install", "--no-enable") + require.NoError(t, err) + + path, err := connectServiceUnitPath("agent") + require.NoError(t, err) + before, err := os.ReadFile(path) //nolint:gosec // path built from a temp home + require.NoError(t, err) + + // No new file can be created in the directory, so the write fails. + require.NoError(t, os.Chmod(filepath.Dir(path), 0o500)) + t.Cleanup(func() { _ = os.Chmod(filepath.Dir(path), 0o700) }) + + _, err = runConnectServiceCmd(t, app, "install", "--no-enable", "--project", "999") + require.Error(t, err) + + after, err := os.ReadFile(path) //nolint:gosec // path built from a temp home + require.NoError(t, err) + assert.Equal(t, string(before), string(after), "the unit that was there is still whole") +} + +// systemd keeps running a unit it has already loaded after the file is +// removed by hand, so "no file" is not "not running". Saying nothing to +// remove there would report the opposite of what uninstall promises. +func TestConnectServiceUninstallStopsALoadedUnitWithNoFile(t *testing.T) { + calls := connectServiceHome(t) + app, said := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "uninstall") + require.NoError(t, err) + + asked := make([]string, 0, len(*calls)) + for _, c := range *calls { + asked = append(asked, c[0]) + } + assert.Contains(t, asked, "disable", "the connector may still be loaded and running") + assert.Contains(t, said.String(), "in case it was still loaded") +} + +// Uninstalling twice succeeds: a unit systemd has never heard of is the +// idempotent case, not a failure. +func TestConnectServiceUninstallOfAnUnknownUnitSucceeds(t *testing.T) { + connectServiceHome(t) + prev := runSystemctl + runSystemctl = func(args ...string) ([]byte, error) { + if len(args) > 0 && args[0] == "disable" { + return []byte("Failed to disable unit: Unit file basecamp-connect-agent.service does not exist."), errors.New("exit status 1") + } + return nil, nil + } + t.Cleanup(func() { runSystemctl = prev }) + app, said := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "uninstall") + require.NoError(t, err) + assert.NotContains(t, said.String(), "may still be running") +} + +// The default KillMode signals the workers at the same instant as the +// connector. A worker's session ending before the connector's context is +// canceled is read as StopLost rather than StopShutdown — a stranded +// attempt needing redispatch by hand, which is the opposite of what the +// stop timeout is there to buy. +func TestConnectServiceUnitSignalsTheConnectorAndNotItsWorkers(t *testing.T) { + unit := connectServiceUnit("/usr/bin/basecamp", "agent", nil, false, false, nil, "") + + assert.Contains(t, unit, "\nKillMode=mixed\n", + "control-group would SIGTERM the workers alongside the connector and strand their attempts") +} + +// The unit's environment is the connector's own two lists, not a third one +// written here: a list that drifts from them is an allowlist that reads as +// configuration and does nothing. +func TestConnectServiceUnitPinsTheListsTheConnectorAlreadyHas(t *testing.T) { + keys := connectServiceEnvKeys() + + for _, name := range driver.BaseEnv { + assert.Contains(t, keys, name, "a worker may inherit %s, so the service has to have it", name) + } + for _, name := range connector.MCPServerEnv { + assert.Contains(t, keys, name, "the worker's MCP server needs %s", name) + } + assert.Contains(t, keys, "CLAUDE_CONFIG_DIR", "a driver setting the worker needs") + assert.NotContains(t, keys, "ANTHROPIC_API_KEY", "a credential never goes in the unit") + assert.NotContains(t, keys, "CODEX_API_KEY") + assert.NotContains(t, keys, "OPENAI_API_KEY") +} + +// The drivers name the variables their agents need. This map says which of +// those authenticate somebody, and a new one the map has not heard of would +// otherwise stop reaching workers without anyone noticing. +func TestConnectServiceClassifiesEveryDriverVariable(t *testing.T) { + named := map[string]bool{} + for _, name := range claude.Env { + named[name] = true + } + for _, name := range codex.Env { + named[name] = true + } + for _, adapter := range acp.Adapters() { + for _, name := range adapter.Env { + named[name] = true + } + } + + for name := range named { + _, ok := connectServiceDriverEnv[name] + assert.Truef(t, ok, "a driver reads %s and the service does not know whether it is a credential", name) + } + for name := range connectServiceDriverEnv { + assert.Truef(t, named[name], "%s is classified here and no driver reads it", name) + } +} + +// A credential the service will not have is said at install, not found out +// in a dispatch that failed. +func TestConnectServiceInstallSaysWhichCredentialsTheServiceWillNotHave(t *testing.T) { + connectServiceHome(t) + writeConnectSetup(t, "agent") + t.Setenv("ANTHROPIC_API_KEY", "not-a-real-key") + app, said := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "install", "--no-enable") + require.NoError(t, err) + + assert.Contains(t, said.String(), "ANTHROPIC_API_KEY") + assert.Contains(t, said.String(), "service.env") + + path, err := connectServiceUnitPath("agent") + require.NoError(t, err) + b, err := os.ReadFile(path) //nolint:gosec // path built from a temp home + require.NoError(t, err) + assert.NotContains(t, string(b), "not-a-real-key", "the unit never carries the secret itself") + assert.Contains(t, string(b), "EnvironmentFile=-", "it reads the file the person owns, if there is one") +} + +// systemctl says "no such file or directory" for a unit it has never heard +// of and also for "Failed to connect to bus: No such file or directory". +// The second leaves the connector running, so reading it as an uninstalled +// service would report the opposite of what happened. +func TestConnectServiceUninstallDoesNotReadALostBusAsAStoppedConnector(t *testing.T) { + connectServiceHome(t) + writeConnectSetup(t, "agent") + app, said := connectServiceApp(t, "agent") + _, err := runConnectServiceCmd(t, app, "install", "--no-enable") + require.NoError(t, err) + + prev := runSystemctl + runSystemctl = func(args ...string) ([]byte, error) { + if len(args) > 0 && args[0] == "disable" { + return []byte("Failed to connect to bus: No such file or directory"), errors.New("exit status 1") + } + return nil, nil + } + t.Cleanup(func() { runSystemctl = prev }) + said.Reset() + + _, err = runConnectServiceCmd(t, app, "uninstall") + require.NoError(t, err, "the unit file still goes") + assert.Contains(t, said.String(), "may still be running") +} + +// unitDirective returns the value of a unit file's directive. +func unitDirective(t *testing.T, unit, key string) string { + t.Helper() + for _, line := range strings.Split(unit, "\n") { + if after, ok := strings.CutPrefix(line, key+"="); ok { + return after + } + } + t.Fatalf("no %s in unit:\n%s", key, unit) + return "" +} diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 0864099ad..a274d8831 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -7,8 +7,9 @@ description: | and readiness (basecamp connect setup). Explains every setup result and failure. Also reads what the connector ran (status, doctor) and carries out a person's decisions on its records (redispatch, discard, - release, the cutover's shadow promote and import). Starting and supervising - the connector is not in this skill yet. + release, the cutover's shadow promote and import). Starts and supervises the + connector through its systemd service, and reads the pointer lines it + writes. Use when asked to connect an agent, set up or change the connector, add or remove a project, change who can drive the agent, find out why setup says the connector is not ready, or see, retry, close or release what the @@ -28,6 +29,10 @@ triggers: - redispatch an event - held records - release the hold + - start the connector + - stop the connector + - basecamp connect service install + - is the connector running --- # Basecamp connector: connect an agent and manage its setup @@ -452,22 +457,81 @@ that record or that step. person is doing a cutover and asks for them. Doctor exits `not_ready` (exit 7) when a check fails; explain each failed check. -Follow a hint from these commands only as the rules above allow: one that says -to reconnect the agent's profile rotates its secret and needs the person's -consent, and one that says to run or stop the connector is the person's to do, -since starting it is not part of this skill. +Follow a hint from these commands only as the rules above allow: a hint that +says to reconnect the agent's profile rotates its secret and needs the person's +consent. -## Not this skill's to do yet +## Running the connector -These come with card 24. Do not start, supervise or watch the connector from -here, and do not look for flags for it: +**Never run `basecamp connect` yourself.** It runs in the foreground until it +is interrupted, so a session that starts it never gets its turn back, and a +connector that lives only as long as your session dies with it. It is a +service. Install the service and let the OS run it. -- starting and supervising the connector, and reading the NDJSON pointer lines - it writes while it runs (the command writes them today; using them is not - this skill's yet); -- a `service install` subcommand that keeps it running under systemd or - launchd, which does not exist in the CLI; -- the Claude Code and Codex plugins that start it. + basecamp connect service install -P '' -When the person asks to start the connector, say plainly that setup is done (or -what is left), and that starting it is not available from this skill yet. +That writes a systemd user unit, enables it and starts it. The unit restarts +the connector whenever it stops, so a crash or a kill brings it back. + +**A reboot is the exception.** A user manager starts at login, so unless +lingering is on, the connector stays down after a restart until someone signs +in. Install says so when it finds lingering off, and gives the +`sudo loginctl enable-linger` command. Pass that on; do not run it, and do not +tell anyone an unattended machine will come back on its own until they have. +Pass the run's shape to install, not to the connector: `--project ` +(repeatable) to hear only some projects, `--shadow` to admit and log without +dispatching or posting anything, `--hold` to run with the durable hold set. +Installing again over an existing service rewrites the unit and restarts it, so +that is how a project is added or shadow is turned off. + +`basecamp connect service uninstall -P ''` stops it and removes the +unit. The stop is a SIGTERM, which is how the connector is meant to be asked: +it cancels its live workers, posts their completions and exits. Nothing else is +removed — the ledger, the checkpoint and connect.json stay, and installing +again resumes from them. + +Linux only, and the refusal says so: the connector runs nowhere else, so there +is no macOS service to install. If a person on a Mac asks, say that rather than +looking for a launchd flag. + +Quote the profile, as everywhere else in this skill. + +### Seeing what it is doing + +Two different questions, two different places. + +**What it has heard and run** is `basecamp connect status -P ''`, +which reads the ledger. That is the answer to "is it working", "what is it +holding", "did that mention get picked up". It works whether or not the service +is running. + +**Whether it is up** is the service: + + systemctl --user is-active basecamp-connect-''.service + systemctl --user status basecamp-connect-''.service + +A unit in `failed` after several restarts is a connector that cannot start: +the unit gives up after five tries rather than retrying forever and reporting +`activating` while nothing works. Run `basecamp connect doctor -P ''` +and explain the failed check. Once the cause is fixed, `service install` again +— it clears the failure counter, which a bare `systemctl --user restart` does +not, so restarting by hand from `failed` is refused until the window passes. + +### The pointer lines + +While it runs, the connector writes one JSON object per line on stdout — a +pointer line per event it saw and per decision it took — and its logs on +stderr. Under the service both land in the journal: + + journalctl --user -u basecamp-connect-''.service -n 50 -o cat + +**A pointer line carries no content, and you must not try to make it.** It +names ids, the trigger, the class, the route and the state, and nothing of what +anyone wrote. Read a line for which event, which state, which route. If a +person wants to know what was said, the answer is in Basecamp, not here — open +the recording the pointer names. Never quote a pointer line into a Basecamp +comment, and never reconstruct a message from one. + +Use the journal to answer "what happened at 14:32" or "why did that event not +dispatch". For anything about the current state of a record, prefer `status`: +it is the ledger's own answer, and the journal is a log of how it got there.