Repository navigation
Write the connect service's EnvironmentFile= as a bare path - #783
Conversation
EnvironmentFile= takes the rest of the line as one path. systemd does not
remove quotes there or decode C escapes, so the quoted path install wrote
was read as a relative path beginning with a quote and ignored on every
start ("EnvironmentFile= path is not absolute, ignoring"): a credential
put in service.env never reached the connector. Write it bare, doubling
% for specifiers; a space needs nothing. Refuse a config directory holding
a line break, which the line has no way to carry.
Also say at install when the unit runs a binary inside a version
manager's install directory (mise or asdf installs, the Nix store), which
the next upgrade deletes and the unit then fails to start.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
github-actions
Bot
added
commands
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Relative XDG_CONFIG_HOME values can still generate an invalid environment-file path.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
What changed in this PR
Fixes systemd EnvironmentFile= rendering and warns about unstable version-managed executable paths.
Changes:
- Emits bare, percent-escaped environment-file paths.
- Rejects line-breaking path characters.
- Adds version-manager warnings and tests.
[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or rungh pr ready --undo.
Click "Ready for review" or rungh pr readyto reengage.
| File | Description |
|---|---|
internal/commands/connect_service.go |
Updates unit rendering, validation, and executable warnings. |
internal/commands/connect_service_test.go |
Tests path rendering, validation, and warnings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61ac86716c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A relative XDG_CONFIG_HOME is kept as it is, and systemd ignores a relative EnvironmentFile=, so install would report success while the file never loaded.
systemd reads EnvironmentFile= as a glob, so * ? [ or a backslash in the config directory would name other files or none.
basecamp connect service installwroteEnvironmentFile=is not a word-split setting likeExecStart=orEnvironment=: systemd takes the rest of the line as one path, expands specifiers, and does nothing else. It does not remove quotes and does not decode C escapes. The quoted value is read as a relative path beginning with", so systemd logsEnvironmentFile= path is not absolute, ignoringand the file never loads. A credential put inservice.env, which is what install tells people to do, never reached the connector.Change
EnvironmentFile=is written as a bare path with%doubled. A space needs no escaping here, and there is no escape to use:\x20stays four literal characters.connectServiceEnvFilerefuses a path holding a newline, carriage return or NUL (only reachable through an oddXDG_CONFIG_HOME), because this line has no way to carry one and it would otherwise split into two directives. Same refusalconnectServiceEnvalready makes for theEnvironment=values. It also refuses a relative path.GlobalConfigDirkeeps a relativeXDG_CONFIG_HOMEas it is, and systemd ignores a relativeEnvironmentFile=. It also refuses*,?,[and\, because systemd reads the path as a glob./mise/installs/,/.asdf/installs/or/nix/store/.connectServiceExecutablealready prefers the PATH name over/proc/self/exe, so a mise shim stays a shim. But a shell with a versioned bin directory on PATH itself (mise activate, or a Go binary installed with mise's Go) pins a path that the next upgrade deletes, and then the unit fails to start at its next restart. The warning is a substring match on those three layouts, so it can miss other version managers. It does not block install.The other directives the same function writes were checked and are correct as they were.
ExecStart=andEnvironment=are both unquoted by systemd, so the quoting stays.Description=carries only the validated profile name. NoWorkingDirectory=is written.Verification
Tested on systemd 261 (Arch, thelio) with a throwaway user unit and a directory named
a b/50%off:EnvironmentFile=line-"/…/a b/50%%off/s.env"(old form)EnvironmentFiles=empty-/…/a b/50%%off/s.env(new form)-/…/a\x20b/50%%off/s.env-/…/a b/50%off/s.env%oexpanded as a specifier, wrong pathNext I loaded the
Environment=andEnvironmentFile=lines thatconnectServiceUnitrendered with this change, usingPATH=/opt/50%off/bin:/usr/bin,XDG_CONFIG_HOME=/tmp/x yand an env file undera b/50%off. They went into a oneshot unit running/usr/bin/env, and all three values reached the process exactly.TestConnectServiceUnitWritesTheEnvironmentFileAsABarePathchecks the exact rendered line for a normal path and for one with a space and a%. It fails against the old quoting. These tests are//go:build linux, like the rest of the file.make checkpassed on Linux.Not done
systemd-analyze verifyon the generated unit, which the card suggested. CI and macOS dev machines have no systemd manager to run it against, andsystemd-analyze --user verifyalready fails to start on a plain SSH session on thelio (Failed to initialize manager). A test that skips wherever it cannot run would add nothing. The exact-line assertion holds the output, and the systemd behavior it depends on is recorded in the table above.