From acc1c1722d01ebaad4d4d22592ad6481b82e4cd4 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 19 Sep 2026 17:10:46 +0200 Subject: [PATCH 1/7] Let the OS keep the connector running MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The connector runs in the foreground and exits when it is stopped, which is the right shape — the CLI has no daemon pattern and should not grow one — but it left nobody to start it again after a crash, a kill or a reboot. `basecamp connect service install` writes the systemd user unit that does, enables it and starts it; `uninstall` stops it and takes the unit away. systemd only, and no launchd. The connector runs on Linux alone, because the task token reaches a worker's MCP server over an inherited descriptor and Linux is the only platform that seals those. A launchd agent would supervise a process that refuses to start, so `service install` gives the same one-sentence refusal on macOS that the run command gives, from the same constant. The unit carries the profile and any --project, --shadow or --hold. It does not take --since, which enters the feed at one id and in a unit would re-enter there on every restart instead of resuming; nor --driver or --acp-adapters, which override what connect.json already holds, and a standing service's settings belong in connect.json. That also leaves every value in the unit either a validated profile name or a run of digits, so no directive can arrive on a second line. Two directives say what the connector needs from a supervisor. KillSignal=SIGTERM with TimeoutStopSec=90 gives it the time it spends cancelling live workers and posting their `ended at shutdown` completions; killed outright, those records need redispatch by hand. SuccessExitStatus=143 makes its clean exit on SIGTERM a stop rather than a failure. Install refuses a profile with no connect.json rather than writing a unit that restarts a connector which cannot start, so the reason is read once here instead of five times in a journal. The refusal in connectUnsupportedOSError now opens on "The connector" rather than "basecamp connect". Reaching it from a second command brought it into TestHintCommandsResolve's scan, which read the prose after it as a subcommand — and it was right to: a refusal that begins like a command is followed like one. Evidence: the unit as generated, given to a real user systemd with a stand-in that behaves like the connector, restarts it after kill -9 (NRestarts=1) and stops clean on SIGTERM (Result=success, exit 143). The same unit with Restart=always removed leaves it failed with MainPID=0. Twelve Go tests cover the rendering and both commands; mutating the unit to Restart=no, to SIGKILL, dropping the setup check, and dropping the reload-and-start each turn one of them red. Still on this card, behind nothing now: the basecamp-connect skill's "start the connector and read pointer lines" half, and the Claude and Codex plugins. Co-Authored-By: Claude Opus 5 (1M context) --- .surface | 72 +++++ e2e/smoke/smoke_lifecycle.bats | 8 + internal/commands/connect.go | 6 +- internal/commands/connect_run.go | 11 +- internal/commands/connect_service.go | 372 ++++++++++++++++++++++ internal/commands/connect_service_test.go | 273 ++++++++++++++++ 6 files changed, 737 insertions(+), 5 deletions(-) create mode 100644 internal/commands/connect_service.go create mode 100644 internal/commands/connect_service_test.go 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/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..73fbe7d00 --- /dev/null +++ b/internal/commands/connect_service.go @@ -0,0 +1,372 @@ +package commands + +import ( + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "runtime" + "sort" + "strconv" + "strings" + + "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/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 + +// 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 + +// 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(runtime.GOOS) { + return "", connectUnsupportedOSError(runtime.GOOS) + } + 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. +// +// Every value in it is either the profile name, which is validated to +// letters, numbers, hyphens and underscores, or a project id, which is +// parsed as a number before it gets here. Nothing a person typed reaches +// the file as text, so no directive can be smuggled in on a second line. +func connectServiceUnit(exe, profile string, projects []int64, shadow, hold bool) 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\n") + fmt.Fprintf(&b, "[Service]\n") + fmt.Fprintf(&b, "Type=simple\n") + 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") + 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. systemd reads +// double-quoted arguments with C-style escapes, which is what the +// executable's own path may need; the arguments after it are literals and +// validated ids. +func systemdExecLine(exe string, args []string) string { + parts := make([]string, 0, len(args)+1) + parts = append(parts, systemdQuote(exe)) + for _, a := range args { + parts = append(parts, systemdQuote(a)) + } + return strings.Join(parts, " ") +} + +// systemdQuote quotes one word for a unit file's command line. +func systemdQuote(s string) string { + r := strings.NewReplacer(`\`, `\\`, `"`, `\"`) + return `"` + r.Replace(s) + `"` +} + +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 := os.Executable() + if err != nil { + return fmt.Errorf("cannot find this program's own path, which the unit has to name: %w", err) + } + if resolved, err := filepath.EvalSymlinks(exe); err == nil { + exe = resolved + } + + 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) + } + unit := connectServiceUnit(exe, profile, projects, f.shadow, f.hold) + if err := os.WriteFile(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); err != nil { + return output.ErrUsageHint( + fmt.Sprintf("Wrote %s, but could not start it: %s", richtext.SanitizeSingleLine(path), richtext.SanitizeSingleLine(strings.TrimSpace(string(out)))), + "Start it yourself: systemctl --user daemon-reload && systemctl --user enable --now "+name) + } + enabled = true + summary = fmt.Sprintf("Wrote %s and started %s", path, name) + } + 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 +} + +func connectServiceEnable(unit string) ([]byte, error) { + if out, err := runSystemctl("daemon-reload"); err != nil { + return out, err + } + // enable --now on an already-running unit leaves it running with the + // old command line, so the restart is asked for explicitly. + if out, err := runSystemctl("enable", "--now", unit); err != nil { + return out, err + } + return runSystemctl("restart", unit) +} + +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) + + if _, err := os.Stat(path); errors.Is(err, os.ErrNotExist) { + return app.OK(map[string]any{"unit": name, "path": path, "removed": false}, + output.WithSummary(fmt.Sprintf("No unit at %s; nothing to remove", path))) + } + + // 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. + _, _ = 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") + return app.OK(map[string]any{"unit": name, "path": path, "removed": true}, + output.WithSummary(fmt.Sprintf("Stopped %s and removed %s", name, path))) +} diff --git a/internal/commands/connect_service_test.go b/internal/commands/connect_service_test.go new file mode 100644 index 000000000..e669dccb7 --- /dev/null +++ b/internal/commands/connect_service_test.go @@ -0,0 +1,273 @@ +//go:build unix + +package commands + +import ( + "bytes" + "context" + "encoding/json" + "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/admission" + "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) + 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) + + 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) + + 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) + + 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) + + 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"}, + {"enable", "--now", "basecamp-connect-agent.service"}, + {"restart", "basecamp-connect-agent.service"}, + }, *calls, "the unit has to be reloaded, enabled 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) +} + +// Removing what is not there is not an error: an uninstall that failed +// because it had already run would be repeated by hand until someone +// checked why. +func TestConnectServiceUninstallOfNothingSucceeds(t *testing.T) { + calls := connectServiceHome(t) + app, said := connectServiceApp(t, "agent") + + _, err := runConnectServiceCmd(t, app, "uninstall") + require.NoError(t, err) + assert.Contains(t, said.String(), "nothing to remove") + assert.Empty(t, *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) + } +} + +// 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 "" +} From 41aa8c0259ffa921e0c0d97d2ed431aab81374a6 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 19 Sep 2026 17:15:42 +0200 Subject: [PATCH 2/7] Let the skill start the connector, and read what it writes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The basecamp-connect skill ended on a section saying starting and supervising the connector was not its to do, and that `service install` did not exist. Both stopped being true one commit ago, so the section is replaced by the thing it was deferring. The rule it leads with is the one an agent most needs: never run `basecamp connect` yourself. It runs in the foreground until interrupted, so a session that starts it never gets its turn back, and a connector that lives only as long as a session dies with it. Install the service and let the OS run it — including for changes, since installing again rewrites the unit and restarts it, which is how a project is added or shadow is turned off. Seeing what it does is split, because they are different questions. What it heard and ran is `status`, which reads the ledger and answers whether or not the service is up. Whether it is up is `systemctl --user`, and a unit that has landed in `failed` is a connector that cannot start: run `doctor` and explain the failed check rather than reinstalling and watching it fail again. The pointer lines get the emphasis the card asks for: they carry ids, the trigger, the class, the route and the state, and nothing anyone wrote. The skill says to read them for which event and which state, never to quote one into Basecamp or reconstruct a message from one, and to reach for the recording the pointer names when someone wants the content. Both plugins carry this same skills directory — Codex names it in its manifest, Claude Code discovers it — so this is the plugin half of the card too, and neither manifest needs a change. Co-Authored-By: Claude Opus 5 (1M context) --- skills/basecamp-connect/SKILL.md | 89 ++++++++++++++++++++++++++------ 1 file changed, 72 insertions(+), 17 deletions(-) diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index 0864099ad..dfda37176 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,72 @@ 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, a kill or a reboot brings it back. +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; +run `basecamp connect doctor -P ''` and explain the failed check, +rather than reinstalling the service and watching it fail again. + +### 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. From ca89eeec9add28f13ed4856e9bbc64469201153b Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 19 Sep 2026 17:28:57 +0200 Subject: [PATCH 3/7] A unit that is active while every dispatch fails MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven review findings, and the theme of the ones that matter is a service that reports health it has not got. **A connector that can never start reported `activating` forever.** Restart=always with RestartSec=5 outruns systemd's default start limit — five restarts take twenty-five seconds against a ten-second window — so the limit never trips and the unit retries for as long as the machine is up. StartLimitIntervalSec=300 with StartLimitBurst=5 puts it in `failed` after about thirty-five seconds instead. Measured both ways on a real user systemd with a stand-in that exits 1: as written it reaches `failed`/`start-limit-hit` at t+35s; with the two directives removed it was still `activating` at t+65s and 12 restarts. **The unit ran in an environment the install never checked.** A user manager starts at login with an environment of its own: it has not got the shell's PATH and computes the XDG defaults itself. So the connector would read a different connect.json from the one install validated, keep its ledger somewhere else, and look for its worker by bare name — `claude`, `codex` — on a PATH without it. Active, and failing every dispatch. The unit now pins PATH and the XDG variables install verified the setup in, and refuses to write one if any of them holds a newline. **systemd might not be reading the file that was written.** The unit directory comes from this shell's XDG_CONFIG_HOME, and the manager computes its search path from its own. Install now asks systemd which fragment it reads for the unit and refuses when that is not the file just written, rather than enabling something else. **enable --now then restart started the connector twice**, and the first process could begin intake or a dispatch before the second killed it. Now `enable` without `--now`, then one `restart`, which starts an inactive unit and refreshes a running one. **A reboot does not bring it back without lingering.** The user manager starts at login. Install says so, with the `loginctl enable-linger` command, rather than promising a guarantee it has not got; arranging it is a system-level change and stays the person's. **The success-path tests ran on every Unix**, and the command refuses darwin, so they would have failed on a Mac. `GOOS=darwin go test -c` confirms they compile there. The file is Linux-only now, and the refusal has a test of its own through an injectable GOOS — correct behavior that was badly asserted. Also: `service` is in the connect action catalog, which STYLE.md requires and TestCatalogMatchesRegisteredCommands enforces; and a systemctl that fails with no output — missing, or failing in the running rather than in what it was asked — keeps its error instead of reporting an empty reason. Co-Authored-By: Claude Opus 5 (1M context) --- internal/commands/commands.go | 2 +- internal/commands/connect_service.go | 151 ++++++++++++++++++++-- internal/commands/connect_service_test.go | 139 +++++++++++++++++++- 3 files changed, 273 insertions(+), 19 deletions(-) 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_service.go b/internal/commands/connect_service.go index 73fbe7d00..9986ae708 100644 --- a/internal/commands/connect_service.go +++ b/internal/commands/connect_service.go @@ -1,6 +1,7 @@ package commands import ( + "context" "errors" "fmt" "os" @@ -10,6 +11,7 @@ import ( "sort" "strconv" "strings" + "time" "github.com/spf13/cobra" @@ -41,6 +43,18 @@ const connectServiceUnitPrefix = "basecamp-connect-" // `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, @@ -49,6 +63,11 @@ const connectServiceRestartSec = 5 // 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) { @@ -157,8 +176,8 @@ func connectServiceProfile(app *appctx.App) (string, error) { if app == nil { return "", fmt.Errorf("app not initialized") } - if !connectSupportedOS(runtime.GOOS) { - return "", connectUnsupportedOSError(runtime.GOOS) + if !connectSupportedOS(connectServiceGOOS) { + return "", connectUnsupportedOSError(connectServiceGOOS) } name := app.Config.ActiveProfile if name == "" { @@ -198,7 +217,7 @@ func connectServiceUnitPath(profile string) (string, error) { // letters, numbers, hyphens and underscores, or a project id, which is // parsed as a number before it gets here. Nothing a person typed reaches // the file as text, so no directive can be smuggled in on a second line. -func connectServiceUnit(exe, profile string, projects []int64, shadow, hold bool) string { +func connectServiceUnit(exe, profile string, projects []int64, shadow, hold bool, env []string) string { args := []string{"connect", "--profile", profile} for _, id := range projects { args = append(args, "--project", strconv.FormatInt(id, 10)) @@ -218,8 +237,17 @@ func connectServiceUnit(exe, profile string, projects []int64, shadow, hold bool 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\n") + 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)) + } fmt.Fprintf(&b, "ExecStart=%s\n", systemdExecLine(exe, args)) fmt.Fprintf(&b, "Restart=always\n") fmt.Fprintf(&b, "RestartSec=%d\n", connectServiceRestartSec) @@ -253,6 +281,36 @@ func systemdQuote(s string) string { return `"` + r.Replace(s) + `"` } +// 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. +// +// So install records the environment it verified the setup in, and the +// service runs in that one. +var connectServiceEnvKeys = []string{"PATH", "XDG_CONFIG_HOME", "XDG_STATE_HOME", "XDG_RUNTIME_DIR", "XDG_DATA_HOME"} + +// 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 +} + func runConnectServiceInstall(cmd *cobra.Command, f *connectServiceFlags) error { app := appctx.FromContext(cmd.Context()) profile, err := connectServiceProfile(app) @@ -287,7 +345,11 @@ func runConnectServiceInstall(cmd *cobra.Command, f *connectServiceFlags) error if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { return fmt.Errorf("cannot create %s: %w", richtext.SanitizeSingleLine(filepath.Dir(path)), err) } - unit := connectServiceUnit(exe, profile, projects, f.shadow, f.hold) + env, err := connectServiceEnv() + if err != nil { + return err + } + unit := connectServiceUnit(exe, profile, projects, f.shadow, f.hold, env) if err := os.WriteFile(path, []byte(unit), 0o600); err != nil { return fmt.Errorf("cannot write %s: %w", richtext.SanitizeSingleLine(path), err) } @@ -296,13 +358,23 @@ func runConnectServiceInstall(cmd *cobra.Command, f *connectServiceFlags) error summary := fmt.Sprintf("Wrote %s", path) enabled := false if !f.noEnable { - if out, err := connectServiceEnable(name); err != nil { + 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(strings.TrimSpace(string(out)))), - "Start it yourself: systemctl --user daemon-reload && systemctl --user enable --now "+name) + 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 + } } return app.OK(map[string]any{"unit": name, "path": path, "enabled": enabled}, output.WithSummary(summary)) @@ -330,18 +402,75 @@ func connectServiceRequireSetup(profile string) error { return nil } -func connectServiceEnable(unit string) ([]byte, error) { +// 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 } - // enable --now on an already-running unit leaves it running with the - // old command line, so the restart is asked for explicitly. - if out, err := runSystemctl("enable", "--now", unit); err != nil { + // 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 } 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 { + user := os.Getenv("USER") + if user == "" { + user = "$USER" + } + // 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", user, "-p", "Linger", "--value").Output() //nolint:gosec // fixed argv, USER read from the environment + 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 " + user +} + func runConnectServiceUninstall(cmd *cobra.Command) error { app := appctx.FromContext(cmd.Context()) profile, err := connectServiceProfile(app) diff --git a/internal/commands/connect_service_test.go b/internal/commands/connect_service_test.go index e669dccb7..ce163dd01 100644 --- a/internal/commands/connect_service_test.go +++ b/internal/commands/connect_service_test.go @@ -1,4 +1,4 @@ -//go:build unix +//go:build linux package commands @@ -6,6 +6,7 @@ import ( "bytes" "context" "encoding/json" + "errors" "os" "path/filepath" "strings" @@ -35,6 +36,15 @@ func connectServiceHome(t *testing.T) *[][]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 }) @@ -95,7 +105,7 @@ func runConnectServiceCmd(t *testing.T, app *appctx.App, args ...string) (string // 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) + 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") @@ -107,7 +117,7 @@ func TestConnectServiceUnitRestartsTheConnector(t *testing.T) { // 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) + unit := connectServiceUnit("/usr/bin/basecamp", "agent", nil, false, false, nil) assert.Contains(t, unit, "\nKillSignal=SIGTERM\n") assert.Contains(t, unit, "\nTimeoutStopSec=90\n") @@ -115,7 +125,7 @@ func TestConnectServiceUnitLetsTheConnectorSettleItsWorkers(t *testing.T) { } func TestConnectServiceUnitRecordsTheRun(t *testing.T) { - unit := connectServiceUnit("/usr/bin/basecamp", "agent", []int64{12345, 67890}, true, true) + unit := connectServiceUnit("/usr/bin/basecamp", "agent", []int64{12345, 67890}, true, true, nil) exec := unitDirective(t, unit, "ExecStart") assert.Equal(t, @@ -126,7 +136,7 @@ func TestConnectServiceUnitRecordsTheRun(t *testing.T) { // 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) + 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"`) } @@ -166,9 +176,10 @@ func TestConnectServiceInstallWritesAndStartsTheUnit(t *testing.T) { assert.Equal(t, [][]string{ {"daemon-reload"}, - {"enable", "--now", "basecamp-connect-agent.service"}, + {"show", "-p", "FragmentPath", "--value", "basecamp-connect-agent.service"}, + {"enable", "basecamp-connect-agent.service"}, {"restart", "basecamp-connect-agent.service"}, - }, *calls, "the unit has to be reloaded, enabled and restarted or the new command line never runs") + }, *calls, "reloaded, checked to be the file systemd reads, enabled and restarted, or the new command line never runs") } // A unit for a profile that was never set up would start a connector that @@ -260,6 +271,120 @@ func TestConnectServiceRefusesAProfileNameThatIsNotOne(t *testing.T) { } } +// 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 +} + // unitDirective returns the value of a unit file's directive. func unitDirective(t *testing.T, unit, key string) string { t.Helper() From 585c4d225a83aa7a9e2d92c193e2ffd85b556f67 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 19 Sep 2026 17:33:46 +0200 Subject: [PATCH 4/7] Say where the start limit lives, and stop claiming the unit has no free text MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two things my own reading caught. The start-limit directives belong in [Unit] rather than [Service] — systemd moved them there in v229, and a blank line above them made the file read as though a section had ended — so the comment says which section and why, with the line closed up. And connectServiceUnit's comment still said nothing a person typed reaches the file as text. Pinning the environment made that untrue: PATH and the XDG variables are free text. The comment now names them as the exception and points at the check that makes them safe, rather than promising something the function stopped doing. Co-Authored-By: Claude Opus 5 (1M context) --- internal/commands/connect_service.go | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/internal/commands/connect_service.go b/internal/commands/connect_service.go index 9986ae708..92d98ceb4 100644 --- a/internal/commands/connect_service.go +++ b/internal/commands/connect_service.go @@ -213,10 +213,14 @@ func connectServiceUnitPath(profile string) (string, error) { // connectServiceUnit renders the unit. // -// Every value in it is either the profile name, which is validated to -// letters, numbers, hyphens and underscores, or a project id, which is -// parsed as a number before it gets here. Nothing a person typed reaches -// the file as text, so no directive can be smuggled in on a second line. +// 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) string { args := []string{"connect", "--profile", profile} for _, id := range projects { @@ -236,7 +240,8 @@ func connectServiceUnit(exe, profile string, projects []int64, shadow, hold bool 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\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") From e6119365bca0e40fa27fa8445d2cd19060363f00 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 19 Sep 2026 17:48:10 +0200 Subject: [PATCH 5/7] Six the review found in code it had already passed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit All six were real, and the header said "Findings: None" while the body carried them, which is the reason to read the body. **systemd rewrites the unit before the connector sees it.** It expands its own % specifiers everywhere and $VAR in a command line even inside double quotes, so `/opt/%u/$agent/basecamp` is not the path that runs. % is now doubled everywhere and $ only in ExecStart, since Environment= takes $ literally and doubling it there would corrupt PATH. Control characters are written as C escapes rather than being allowed to end the directive. **The start limit I added made reinstalling useless.** A unit in start-limit-hit refuses to restart until its window expires, and install tells people to install again to recover. Measured on a real user systemd: from `failed`/`start-limit-hit`, a plain restart is refused with "start of the service was attempted too often" and the unit stays failed; with `reset-failed` first it comes up active. Install now clears the counter before the restart. **The linger check answered for the wrong user, or none.** exec runs no shell, so an unset USER was passed to loginctl as the four characters "$USER", the lookup failed, and the warning was silently suppressed — on exactly the machines most likely to need it. It resolves the process's own user now, and shell-quotes it in the command it prints. **Uninstall said "Stopped" whether or not it had.** The stop stays best effort, because the unit file has to go even with no session bus, but a failure is now reported: the summary says the connector may still be running, and names why. **The skill promised reboot recovery it has not got.** Qualified: a crash or a kill brings the connector back, a reboot does not until lingering is on, and the skill passes the command on rather than running it. It also now says to reinstall rather than restart by hand after a failure, which is the counter-clearing above. Each has a test, and each test goes red under the matching mutation: leaving % alone, doubling $ in Environment, dropping the reset-failed, and making uninstall claim success regardless. Co-Authored-By: Claude Opus 5 (1M context) --- internal/commands/connect_service.go | 99 ++++++++++++++++++----- internal/commands/connect_service_test.go | 88 +++++++++++++++++++- skills/basecamp-connect/SKILL.md | 17 +++- 3 files changed, 180 insertions(+), 24 deletions(-) diff --git a/internal/commands/connect_service.go b/internal/commands/connect_service.go index 92d98ceb4..0dde71cab 100644 --- a/internal/commands/connect_service.go +++ b/internal/commands/connect_service.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "os/exec" + "os/user" "path/filepath" "runtime" "sort" @@ -251,7 +252,7 @@ func connectServiceUnit(exe, profile string, projects []int64, shadow, hold bool // 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)) + fmt.Fprintf(&b, "Environment=%s\n", systemdQuote(e, false)) } fmt.Fprintf(&b, "ExecStart=%s\n", systemdExecLine(exe, args)) fmt.Fprintf(&b, "Restart=always\n") @@ -267,23 +268,54 @@ func connectServiceUnit(exe, profile string, projects []int64, shadow, hold bool return b.String() } -// systemdExecLine renders an ExecStart command line. systemd reads -// double-quoted arguments with C-style escapes, which is what the -// executable's own path may need; the arguments after it are literals and -// validated ids. +// 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)) + parts = append(parts, systemdQuote(exe, true)) for _, a := range args { - parts = append(parts, systemdQuote(a)) + parts = append(parts, systemdQuote(a, true)) } return strings.Join(parts, " ") } -// systemdQuote quotes one word for a unit file's command line. -func systemdQuote(s string) string { - r := strings.NewReplacer(`\`, `\\`, `"`, `\"`) - return `"` + r.Replace(s) + `"` +// 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 @@ -431,6 +463,14 @@ func connectServiceEnable(unit, wantPath string) ([]byte, error) { 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) } @@ -457,15 +497,21 @@ func connectServiceCheckFragment(unit, wantPath string) ([]byte, error) { // running after a reboot, so the unit's own "Restart" promise stops at the // next power cut. func connectServiceLingerWarning(ctx context.Context) string { - user := os.Getenv("USER") - if user == "" { - user = "$USER" + // 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", user, "-p", "Linger", "--value").Output() //nolint:gosec // fixed argv, USER read from the environment + 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 "" } @@ -473,7 +519,7 @@ func connectServiceLingerWarning(ctx context.Context) string { 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 " + user + "To have it start at boot: sudo loginctl enable-linger " + richtext.ShellQuote(name) } func runConnectServiceUninstall(cmd *cobra.Command) error { @@ -496,11 +542,26 @@ func runConnectServiceUninstall(cmd *cobra.Command) error { // 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. - _, _ = runSystemctl("disable", "--now", name) + // 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") - return app.OK(map[string]any{"unit": name, "path": path, "removed": true}, - output.WithSummary(fmt.Sprintf("Stopped %s and removed %s", name, path))) + + summary := fmt.Sprintf("Stopped %s and removed %s", name, path) + stopped := true + if stopErr != nil { + 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": true, "stopped": stopped}, + output.WithSummary(summary)) } diff --git a/internal/commands/connect_service_test.go b/internal/commands/connect_service_test.go index ce163dd01..078f9f3b0 100644 --- a/internal/commands/connect_service_test.go +++ b/internal/commands/connect_service_test.go @@ -178,8 +178,9 @@ func TestConnectServiceInstallWritesAndStartsTheUnit(t *testing.T) { {"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 and restarted, or the new command line never runs") + }, *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 @@ -385,6 +386,91 @@ func runConnectServiceInstallForTest(t *testing.T, app *appctx.App) error { 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 +} + // unitDirective returns the value of a unit file's directive. func unitDirective(t *testing.T, unit, key string) string { t.Helper() diff --git a/skills/basecamp-connect/SKILL.md b/skills/basecamp-connect/SKILL.md index dfda37176..a274d8831 100644 --- a/skills/basecamp-connect/SKILL.md +++ b/skills/basecamp-connect/SKILL.md @@ -471,7 +471,13 @@ service. Install the service and let the OS run it. basecamp connect service install -P '' That writes a systemd user unit, enables it and starts it. The unit restarts -the connector whenever it stops, so a crash, a kill or a reboot brings it back. +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. @@ -504,9 +510,12 @@ is running. 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; -run `basecamp connect doctor -P ''` and explain the failed check, -rather than reinstalling the service and watching it fail again. +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 From 5cb4ded5be0e2ffa639d7fbe5b832a0f8a963032 Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 19 Sep 2026 18:07:04 +0200 Subject: [PATCH 6/7] Three more, and one of them is this machine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **The unit named a version, not a program.** 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 — this machine's own Go is at .local/share/mise/installs/go/1.27.1/bin — so the unit would have kept running the version installed today after an upgrade, and stopped starting at all once it was collected. The install now resolves the name it was invoked by through PATH and makes it absolute without following the last symlink, because the shim is the point. The resolved path stays as the fallback. **A failed write left half a unit.** Writing straight over the file truncates the unit that is enabled and running, so an install that ran out of disk would leave systemd running from what it had already loaded and refusing to load it at the next boot — a failure a reboot away from whoever caused it. Written through a temporary file and renamed now, so a failed reinstall leaves the unit that was there. **Uninstall walked away from a running connector.** It returned "nothing to remove" when the file was absent, but systemd goes on running a unit it has already loaded after its file is removed by hand, so that reported the opposite of what uninstall promises. The stop is attempted either way, and only the wording changes. A unit systemd has never heard of is recognised as the idempotent case, so uninstalling twice still succeeds. TestConnectServiceUninstallOfNothingSucceeds asserted the behaviour that was wrong — that no file means no systemctl call — and is replaced by the two tests above, which keep its real claim that removing nothing is not an error. Each has a test, and each is red under its mutation: going back to EvalSymlinks, writing without the rename, returning early with no file, and treating an unknown unit as a failure. Co-Authored-By: Claude Opus 5 (1M context) --- internal/commands/connect_service.go | 102 +++++++++++++++++--- internal/commands/connect_service_test.go | 109 +++++++++++++++++++--- 2 files changed, 186 insertions(+), 25 deletions(-) diff --git a/internal/commands/connect_service.go b/internal/commands/connect_service.go index 0dde71cab..3fc2501ec 100644 --- a/internal/commands/connect_service.go +++ b/internal/commands/connect_service.go @@ -348,6 +348,34 @@ func connectServiceEnv() ([]string, error) { 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) @@ -367,12 +395,9 @@ func runConnectServiceInstall(cmd *cobra.Command, f *connectServiceFlags) error return err } - exe, err := os.Executable() + exe, err := connectServiceExecutable() if err != nil { - return fmt.Errorf("cannot find this program's own path, which the unit has to name: %w", err) - } - if resolved, err := filepath.EvalSymlinks(exe); err == nil { - exe = resolved + return err } path, err := connectServiceUnitPath(profile) @@ -387,7 +412,12 @@ func runConnectServiceInstall(cmd *cobra.Command, f *connectServiceFlags) error return err } unit := connectServiceUnit(exe, profile, projects, f.shadow, f.hold, env) - if err := os.WriteFile(path, []byte(unit), 0o600); err != nil { + // 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) } @@ -534,10 +564,13 @@ func runConnectServiceUninstall(cmd *cobra.Command) error { } name := connectServiceUnitName(profile) - if _, err := os.Stat(path); errors.Is(err, os.ErrNotExist) { - return app.OK(map[string]any{"unit": name, "path": path, "removed": false}, - output.WithSummary(fmt.Sprintf("No unit at %s; nothing to remove", path))) - } + // 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 @@ -552,8 +585,13 @@ func runConnectServiceUninstall(cmd *cobra.Command) error { _, _ = 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 - if stopErr != nil { + // 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 == "" { @@ -562,6 +600,46 @@ func runConnectServiceUninstall(cmd *cobra.Command) 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": true, "stopped": stopped}, + 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. +func connectServiceUnknownUnit(out []byte) bool { + text := strings.ToLower(string(out)) + return strings.Contains(text, "not loaded") || + strings.Contains(text, "does not exist") || + strings.Contains(text, "no such file or directory") +} + +// 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 index 078f9f3b0..b500a5201 100644 --- a/internal/commands/connect_service_test.go +++ b/internal/commands/connect_service_test.go @@ -234,19 +234,6 @@ func TestConnectServiceUninstallStopsAndRemoves(t *testing.T) { }, *calls) } -// Removing what is not there is not an error: an uninstall that failed -// because it had already run would be repeated by hand until someone -// checked why. -func TestConnectServiceUninstallOfNothingSucceeds(t *testing.T) { - calls := connectServiceHome(t) - app, said := connectServiceApp(t, "agent") - - _, err := runConnectServiceCmd(t, app, "uninstall") - require.NoError(t, err) - assert.Contains(t, said.String(), "nothing to remove") - assert.Empty(t, *calls) -} - // One unit per profile, so a machine can supervise several agents without // one install standing on another's. func TestConnectServiceUnitIsNamedPerProfile(t *testing.T) { @@ -471,6 +458,102 @@ func indexOfCall(order []string, want string) int { 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") +} + // unitDirective returns the value of a unit file's directive. func unitDirective(t *testing.T, unit, key string) string { t.Helper() From 11a76bfbe55754789de2382ab045c4945d226f2b Mon Sep 17 00:00:00 2001 From: Jorge Manrubia Date: Sat, 19 Sep 2026 19:19:18 +0200 Subject: [PATCH 7/7] One seam: the unit describes an environment nobody checked MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five rounds of review on this file, and stepping back, they are mostly one thing seen from different sides. A unit file is an assertion about how the connector will run, and install wrote it without ever confirming the assertion held: the PATH, the XDG directories, the executable path, systemd's own specifier expansion, and now the kill mode and the drivers' variables. Each round found another way for the service to be active while the connector could not do its job. This round's three: **The default kill mode strands the work it was meant to settle.** KillMode=control-group SIGTERMs the workers at the same instant as the connector, and a worker session ending before the connector's context is canceled reaches goneStop, which returns StopLost rather than StopShutdown. That is an attempt needing redispatch by hand — the exact opposite of what TimeoutStopSec is there to buy. KillMode=mixed sends the signal to the connector alone and lets systemd SIGKILL the rest after the timeout. **The unit pinned my list rather than the connector's.** The connector already says what a worker may inherit, in driver.BaseEnv, and what its MCP server needs on top, in connector.MCPServerEnv — with a comment about this very hazard, that an allowlist which drifts reads as configuration and does nothing. The unit is built from those two now instead of the five names I had picked, plus the drivers' own settings. **And the credentials are not pinned, deliberately.** driver.BaseEnv carries "nothing that authenticates anyone", and a unit file is not where a credential goes: systemctl cat prints it and a backup copies it. So the settings go in the unit, the credentials do not, and the unit reads an optional owner-only environment file beside connect.json that the person writes. Install names which credentials this shell has that the service will not, rather than leaving it to a dispatch that failed. A map says which driver variables authenticate somebody, and a test keeps it level with the drivers' own lists so a new variable is a failing test rather than one that quietly stops reaching workers. **Uninstall could report a stop it had not made.** "no such file or directory" is systemctl's answer for a unit it has never heard of and also for "Failed to connect to bus: No such file or directory" — which leaves the connector running. The classifier no longer accepts it. Each has a test, and each test is red under its mutation: control-group for mixed, reclassifying ANTHROPIC_API_KEY as not a credential, and letting the bus failure count as an unknown unit again. The middle one looked green at first because the mutation had not applied; it was re-run with the substitution asserted. Co-Authored-By: Claude Opus 5 (1M context) --- internal/commands/connect_service.go | 118 ++++++++++++++++++-- internal/commands/connect_service_test.go | 125 ++++++++++++++++++++-- 2 files changed, 226 insertions(+), 17 deletions(-) diff --git a/internal/commands/connect_service.go b/internal/commands/connect_service.go index 3fc2501ec..f216e5ffd 100644 --- a/internal/commands/connect_service.go +++ b/internal/commands/connect_service.go @@ -9,6 +9,7 @@ import ( "os/user" "path/filepath" "runtime" + "slices" "sort" "strconv" "strings" @@ -18,6 +19,8 @@ import ( "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" @@ -222,7 +225,7 @@ func connectServiceUnitPath(profile string) (string, error) { // 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) string { +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)) @@ -254,6 +257,12 @@ func connectServiceUnit(exe, profile string, projects []int64, shadow, hold bool 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) @@ -261,6 +270,14 @@ func connectServiceUnit(exe, profile string, projects []int64, shadow, hold bool // 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") @@ -326,14 +343,77 @@ func systemdQuote(s string, expandDollar bool) string { // name — `claude`, `codex` — off a PATH that does not have it. The unit // would be active and every dispatch would fail. // -// So install records the environment it verified the setup in, and the -// service runs in that one. -var connectServiceEnvKeys = []string{"PATH", "XDG_CONFIG_HOME", "XDG_STATE_HOME", "XDG_RUNTIME_DIR", "XDG_DATA_HOME"} +// 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 { + for _, k := range connectServiceEnvKeys() { v, ok := os.LookupEnv(k) if !ok || v == "" { continue @@ -411,7 +491,11 @@ func runConnectServiceInstall(cmd *cobra.Command, f *connectServiceFlags) error if err != nil { return err } - unit := connectServiceUnit(exe, profile, projects, f.shadow, f.hold, env) + 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 @@ -443,6 +527,13 @@ func runConnectServiceInstall(cmd *cobra.Command, f *connectServiceFlags) error 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)) } @@ -606,12 +697,21 @@ func runConnectServiceUninstall(cmd *cobra.Command) error { // connectServiceUnknownUnit reports whether systemctl refused because it has // never heard of the unit, which is what uninstalling an uninstalled service -// looks like. +// 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") || - strings.Contains(text, "no such file or directory") + strings.Contains(text, "does not exist") } // writeFileAtomic writes data to path through a temporary file in the same diff --git a/internal/commands/connect_service_test.go b/internal/commands/connect_service_test.go index b500a5201..db504f0b7 100644 --- a/internal/commands/connect_service_test.go +++ b/internal/commands/connect_service_test.go @@ -18,7 +18,12 @@ import ( "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" ) @@ -105,7 +110,7 @@ func runConnectServiceCmd(t *testing.T, app *appctx.App, args ...string) (string // 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) + 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") @@ -117,7 +122,7 @@ func TestConnectServiceUnitRestartsTheConnector(t *testing.T) { // 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) + unit := connectServiceUnit("/usr/bin/basecamp", "agent", nil, false, false, nil, "") assert.Contains(t, unit, "\nKillSignal=SIGTERM\n") assert.Contains(t, unit, "\nTimeoutStopSec=90\n") @@ -125,7 +130,7 @@ func TestConnectServiceUnitLetsTheConnectorSettleItsWorkers(t *testing.T) { } func TestConnectServiceUnitRecordsTheRun(t *testing.T) { - unit := connectServiceUnit("/usr/bin/basecamp", "agent", []int64{12345, 67890}, true, true, nil) + unit := connectServiceUnit("/usr/bin/basecamp", "agent", []int64{12345, 67890}, true, true, nil, "") exec := unitDirective(t, unit, "ExecStart") assert.Equal(t, @@ -136,7 +141,7 @@ func TestConnectServiceUnitRecordsTheRun(t *testing.T) { // 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) + 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"`) } @@ -265,7 +270,7 @@ func TestConnectServiceRefusesAProfileNameThatIsNotOne(t *testing.T) { // 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) + unit := connectServiceUnit("/usr/bin/basecamp", "agent", nil, false, false, nil, "") assert.Contains(t, unit, "\nStartLimitIntervalSec=300\n") assert.Contains(t, unit, "\nStartLimitBurst=5\n") @@ -278,7 +283,7 @@ func TestConnectServiceUnitStopsRetryingAConnectorThatCannotStart(t *testing.T) // 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"}) + []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"`) @@ -379,7 +384,7 @@ func runConnectServiceInstallForTest(t *testing.T, app *appctx.App) error { // 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`}) + []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 $") @@ -390,7 +395,7 @@ func TestConnectServiceUnitSurvivesAPathSystemdWouldRewrite(t *testing.T) { // 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) + unit := connectServiceUnit("/opt/a\tb/basecamp", "agent", nil, false, false, nil, "") exec := unitDirective(t, unit, "ExecStart") assert.Contains(t, exec, `\t`) @@ -554,6 +559,110 @@ func TestConnectServiceUninstallOfAnUnknownUnitSucceeds(t *testing.T) { 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()