Skip to content

Nothing started the connector again after a crash, and no skill could start it at all - #775

Merged
jorgemanrubia merged 7 commits into
mainfrom
connect-service-install
Sep 19, 2026
Merged

jorgemanrubia merged 7 commits into
mainfrom
connect-service-install

Conversation

@jorgemanrubia

@jorgemanrubia jorgemanrubia commented Sep 19, 2026

Copy link
Copy Markdown
Member

The connector runs in the foreground and exits when it is stopped. That is the right shape — it follows the grain of basecamp mcp, and 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. Someone running an agent had to notice it was gone.

Originally tracked in 24 service install, the skill and the plugins.

basecamp connect service install writes the systemd user unit that supervises it, enables it and starts it. uninstall stops it and takes the unit away. One unit per profile, so a machine can serve several agents.

systemd only, and no launchd

The card asks for a launchd agent too. There is nothing for it to supervise: the connector runs on Linux alone, because a 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. A launchd agent would restart 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, rather than inventing a second story about the machine.

What the unit says, and why

The unit records the run: the profile, and any --project, --shadow or --hold. It does not accept --since, which enters the feed at one id — in a unit that means re-entering there on every restart instead of resuming from the ledger. Nor --driver or --acp-adapters, which override what connect.json already holds, and a standing service's settings belong in connect.json. That has a second effect worth naming: every value in the unit is then either a profile name validated to letters, digits, hyphens and underscores, or a project id parsed as a number, so nothing a person typed reaches the file as text and no directive can arrive on a second line.

Two directives are the connector's contract with a supervisor. KillSignal=SIGTERM with TimeoutStopSec=90 gives it the seconds 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. The reason is then read once, here, instead of five times in a journal.

One message moved: 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, handed to a real user systemd with a stand-in that behaves like the connector (runs until signalled, exits 143 on SIGTERM):

  • kill -9 the main process → systemd starts it again, NRestarts=1, active
  • systemctl --user stopinactive, Result=success — the 143 counted as a clean stop
  • the same unit with Restart=always removed → after kill -9, failed, MainPID=0, NRestarts=0

Twelve Go tests cover the rendering and both commands. None of them passes over an empty set: mutating the unit to Restart=no reds the restart test, to KillSignal=SIGKILL reds the settle test, dropping the setup check reds the refusal test, and dropping the reload-and-start reds the install test.

The skill, and the plugins

The basecamp-connect skill ended on a section saying starting the connector was not its to do and that service install did not exist. Both stopped being true, so that section is replaced by the thing it was deferring. Its setup half landed in Let an agent connect an agent and manage connector setup: the basecamp-connect skill.

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 turned off.

Seeing what it does is split, because they are two 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; a unit that has landed in failed is a connector that cannot start, so the skill runs doctor and explains 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 open 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 too, and neither manifest needs a change.

Copilot AI balanced review requested due to automatic review settings September 19, 2026 15:11
@github-actions github-actions Bot added commands CLI command implementations tests Tests (unit and e2e) labels Sep 19, 2026
@jorgemanrubia jorgemanrubia changed the title Nothing started the connector again after a crash Nothing started the connector again after a crash, and no skill could start it at all Sep 19, 2026
@github-actions github-actions Bot added the skills Agent skills label Sep 19, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The service currently has cross-platform test failures and several systemd lifecycle and environment gaps that can leave connectors offline or misreported.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity · 4 Medium severity · 2 Low severity

Open (7)
What changed in this PR

Adds systemd user-service management for the Linux connector.

Changes:

  • Adds per-profile service install/uninstall commands.
  • Generates supervised systemd units with connector options.
  • Adds unit tests, CLI surface entries, and smoke-test exclusions.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File Description
internal/​commands/​connect.go Registers the service command group.
internal/​commands/​connect_service.go Implements systemd unit lifecycle.
internal/​commands/​connect_service_test.go Tests rendering and lifecycle commands.
internal/​commands/​connect_run.go Generalizes unsupported-OS messaging.
e2e/​smoke/​smoke_lifecycle.bats Marks service operations out of scope.
.surface Records the new CLI surface.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread internal/commands/connect_service_test.go Outdated
Comment thread internal/commands/connect_service.go
Comment thread internal/commands/connect_service.go
Comment thread internal/commands/connect_service.go
Comment thread internal/commands/connect_service.go Outdated
Comment thread internal/commands/connect.go
Comment thread internal/commands/connect_service.go Outdated
Copilot AI review requested due to automatic review settings September 19, 2026 15:20

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Service lifecycle, environment, restart limiting, cross-platform tests, and command catalog issues remain unresolved.

Review effort: Balanced
Findings: 1 High severity · 4 Medium severity · 2 Low severity

Open (7)
Previously missed (3)

In code that hasn't changed since last review

Medium severity Restart policy bypasses intended systemd start-rate limit

internal/​commands/​connect_service.go:225

RestartSec=5 does not “burn through” systemd's usual 5-start/10-second rate limit: at most a few starts occur in that window, so a permanently broken connector can restart every five seconds indefinitely instead of landing in failed as the command and skill expect. Define an explicit StartLimitIntervalSec/StartLimitBurst pair whose window actually contains the intended attempts.

Medium severity Percent signs in executable paths are not escaped for systemd

internal/​commands/​connect_service.go:254

systemd expands % specifiers in ExecStart even inside quotes. A valid executable path containing % is therefore changed (or rejected for an unknown specifier), so the generated service cannot launch the same binary. Escape literal percent signs as %% in addition to the existing C-style escaping.

Medium severity Stop failure is ignored and command falsely reports service stopped

internal/​commands/​connect_service.go:365

Ignoring this failure can leave the connector running: removing and reloading a loaded unit does not terminate its active process. The command then returns success and says “Stopped” even when disable --now could not reach the user bus or failed to stop the unit. Continue the file cleanup if desired, but preserve/report the stop error and do not claim the process stopped.

Copilot AI review requested due to automatic review settings September 19, 2026 15:29
@jorgemanrubia

Copy link
Copy Markdown
Member Author

🤖 All seven findings addressed and answered on their threads. The two that mattered most were both a service reporting health it had not got: a connector that could never start reported activating forever (RestartSec=5 outruns systemd's default ten-second start-limit window — measured at 12 restarts and still activating at t+65s), and the unit ran in the user manager's environment rather than the one install verified, so it would have been active while every dispatch failed on a PATH without claude or codex. Re-requesting a review on the new head.

Copilot AI previously approved these changes Sep 19, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approved

The implementation is cohesive, well-tested, and the previously identified correctness issues are resolved.

Review effort: Balanced
Findings: None

Resolved since last review (7)

Copilot AI review requested due to automatic review settings September 19, 2026 15:33
Copilot AI dismissed their stale review, a newer Copilot review was requested September 19, 2026 15:33

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unit escaping, restart recovery, and uninstall handling still contain correctness gaps.

Review effort: Balanced
Findings: None

Previously missed (6)

In code that hasn't changed since last review

Medium severity Escape systemd specifiers, variables, and control characters

internal/​commands/​connect_service.go:287

systemdQuote only handles backslashes and quotes, but systemd still expands % specifiers in both Environment= and ExecStart=, and $VAR in ExecStart= even inside quotes. Paths such as /opt/%u/$agent/basecamp are therefore changed (or rejected), and control characters in an executable path can break the directive. Use systemd's literal %%/$$ escapes and C-style escaping for control characters; keep $ escaping specific to ExecStart= because Environment= treats it literally.

Medium severity Reset failed state before restarting the service

internal/​commands/​connect_service.go:434

This unit deliberately enters start-limit-hit after repeated failures, but systemctl restart does not clear that rate-limit counter. Reinstalling immediately after fixing connect.json or the worker will still fail until the interval expires. Run reset-failed before the restart so reinstall can recover the service as documented.

This issue also appears on line 491 of the same file.

Medium severity Resolve the installing user directly instead of using $USER

internal/​commands/​connect_service.go:463

exec.CommandContext does not invoke a shell, so when USER is unset the literal string $USER is passed to loginctl; the lookup fails and the code silently suppresses the reboot warning. USER can also name a different account from the process UID. Resolve the current OS user/UID directly so the linger check describes the user manager being installed into.

Medium severity Report disable failures instead of claiming successful cleanup

internal/​commands/​connect_service.go:499

Discarding the disable --now error makes the final summary claim the connector was stopped even when the session-bus call failed and the process may still be running. Continue removing the unit file if cleanup must be best-effort, but retain and report the stop failure as a partial uninstall instead of returning unconditional success.

Low severity Shell-quote the interpolated username

internal/​commands/​connect_service.go:476

This copy-paste command interpolates the environment-derived username without the repository's canonical shell quoting. Use richtext.ShellQuote(user), as generated commands do in internal/commands/templates.go:299 and internal/commands/files.go:1923, so directory-service or otherwise unusual usernames cannot change the command.

Low severity Qualify reboot recovery instructions when lingering is disabled

skills/​basecamp-connect/​SKILL.md:474

This still promises recovery after a reboot unconditionally, but the service starts only after login unless lingering is enabled—the install command itself now warns about exactly that case. Qualify this instruction so the skill does not tell agents that an unattended connector has reboot recovery when it does not.

Copilot AI review requested due to automatic review settings September 19, 2026 15:48
@jorgemanrubia

Copy link
Copy Markdown
Member Author

🤖 All six addressed. They arrived in the review body with Findings: None in the header and no threads attached, which is worth naming: the thread count said nothing.

  • systemd specifiers and variables. % is doubled everywhere, $ only in ExecStartEnvironment= takes $ literally and doubling it there would corrupt PATH. Control characters are written as C escapes.
  • reset-failed before the restart. This was a trap I introduced with the start limit, and it measured exactly as you said 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 clears the counter now, which is what makes "install again" the recovery it claims to be.
  • $USER. Resolved from the process's own user instead. exec runs no shell, so an unset USER was being passed as the four characters $USER, the lookup failed, and the warning vanished — on exactly the machines most likely to need it. The printed command is richtext.ShellQuoted.
  • Uninstall claiming a stop it did not make. Still best effort on the file, but a failed stop is now reported: the summary says the connector may still be running and names why.
  • SKILL.md's reboot promise. Qualified — a crash or a kill brings it back, a reboot does not until lingering is on, and the skill passes the enable-linger command on rather than running it.

Each has a test, and each test is red under the matching mutation: leaving % alone, doubling $ in Environment, dropping the reset-failed, and making uninstall claim success regardless.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

Unit persistence and uninstall edge cases can leave the connector outdated, corrupted, or still running.

Review effort: Balanced
Findings: None

Previously missed (3)

In code that hasn't changed since last review

Medium severity Resolved executable paths become stale after upgrades

internal/​commands/​connect_service.go:375

Resolving the executable through symlinks bakes a version-specific target into this long-lived unit. On supported Nix and mise installations, the stable command is a profile symlink/shim while the resolved store/version path changes; after an upgrade the service keeps running the old binary, and garbage collection/removal makes its next restart fail. Capture the stable invocation path (for example, resolve os.Args[0] through PATH and make it absolute without dereferencing the final symlink) and cover a symlinked installation.

Medium severity Direct unit writes can leave a truncated file after failure

internal/​commands/​connect_service.go:390

This direct write truncates an existing enabled unit before the replacement is complete. A disk-full error or interruption can therefore leave a partial unit behind, so the currently cached service may run until the next reboot and then fail to load. Use the package's atomic temp-and-rename writer so a failed reinstall preserves the previous valid unit.

Medium severity Uninstall skips stopping loaded units when the file is absent

internal/​commands/​connect_service.go:540

The absence of the fragment file does not mean the connector is stopped: systemd can retain and run an already-loaded unit after its file is manually removed. This early return then reports “nothing to remove” while leaving that connector alive, contrary to uninstall's stop contract. Attempt to stop/disable the named unit and reload even when the path is absent, while treating an unknown unit as the idempotent success case.

Copilot AI review requested due to automatic review settings September 19, 2026 16:07
@jorgemanrubia

Copy link
Copy Markdown
Member Author

🤖 All three, again from the review body with Findings: None in the header.

  • The resolved executable path. This one is true of the machine I am on: os.Executable reads /proc/self/exe, which the kernel has already followed, and a mise install is a versioned store path behind a shim. The unit would have kept running today's version after an upgrade and stopped starting once it was collected. It now resolves the invoked name through PATH and makes it absolute without following the last symlink, with the resolved path as the fallback.
  • The non-atomic write. Written to a temporary file in the same directory and renamed. A failed reinstall now leaves the unit that was there whole, rather than half of the one it was replacing — which systemd would have gone on running until the next boot and then refused to load.
  • Uninstall with no file. You are right that absence of the fragment is not absence of a running connector. The stop is attempted either way now 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 — no file, no systemctl call — so it is replaced by two tests that keep its real claim.

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.

Copilot AI previously approved these changes Sep 19, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approved

The service lifecycle implementation is comprehensive, securely handles unit inputs, and is covered by focused tests.

Review effort: Balanced
Findings: None

@jorgemanrubia

Copy link
Copy Markdown
Member Author

🤖 A test asserted five of fifteen waited, and CI got one has landed as 77993043. That was the failing Tests check here — a latent flake I introduced in #774, not anything in this branch. Rebasing onto it.

jorgemanrubia and others added 2 commits September 19, 2026 19:01
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
jorgemanrubia and others added 4 commits September 19, 2026 19:01
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) <noreply@anthropic.com>
…ee text

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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
**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) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 19, 2026 17:05
@jorgemanrubia
jorgemanrubia force-pushed the connect-service-install branch from 5807763 to 5cb4ded Compare September 19, 2026 17:05
Copilot AI dismissed their stale review, a newer Copilot review was requested September 19, 2026 17:05

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟡 Changes recommended

The unit’s shutdown and environment handling can cause incorrect task settlement or failed dispatches, and uninstall can falsely report success.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 3 Medium severity

Open (3)

Comment thread internal/commands/connect_service.go
Comment thread internal/commands/connect_service.go Outdated
Comment thread internal/commands/connect_service.go
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) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings September 19, 2026 17:19

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approved

The implementation is cohesive, extensively tested, and the previously identified lifecycle, environment, platform, and reporting issues are resolved.

Review effort: Balanced
Findings: None

Resolved since last review (3)

@jorgemanrubia
jorgemanrubia merged commit f91189c into main Sep 19, 2026
27 checks passed
@jorgemanrubia
jorgemanrubia deleted the connect-service-install branch September 19, 2026 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commands CLI command implementations skills Agent skills tests Tests (unit and e2e)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants