Skip to content

fix(daemon): bake install-time SHELL into the managed service unit - #770

Open
mattyg wants to merge 3 commits into
kunchenguid:mainfrom
mattyg:fm/no-mistakes-nixos-path-fix
Open

fix(daemon): bake install-time SHELL into the managed service unit#770
mattyg wants to merge 3 commits into
kunchenguid:mainfrom
mattyg:fm/no-mistakes-nixos-path-fix

Conversation

@mattyg

@mattyg mattyg commented Aug 18, 2026

Copy link
Copy Markdown

Hey thanks for the agentic workflow projects. I appreciate the approach you're working towards. I ran into an issue where no-mistakes couldn't find any of the packages it needed. I'm on nixos where packages aren't in the standard linux locations like /usr/bin or /bin. This was claude's attempt to solve it which seems plausible. I'd understand if you didn't want to support non-standard distros, I can work around it.

Thanks.

Claude's Summary

On NixOS (and likely other non-FHS Linux distros), the no-mistakes daemon ends up running with a badly wrong PATH, causing daemon subprocess calls like git and ps to fail with executable file not found in $PATH even though those tools are genuinely installed and on the invoking user's normal PATH.

Root cause: the generated systemd/launchd service unit exports only HOME, a curated PATH, and proxy vars - never SHELL. Inside the restricted daemon process, internal/shellenv.LoginShell() falls through to shelling out to getent passwd $USER using that same minimal starting PATH. On NixOS, /bin and /usr/bin are nearly empty (real binaries live under /run/current-system/sw/bin, ~/.nix-profile/bin, /nix/store/...), so getent itself can't be found either. LoginShell() falls all the way through to its hardcoded "bash" default, and the login-shell probe then also can't find bash on that same minimal PATH. The probe errors out and falls back to WellKnownBinDirs(), which is FHS-only and resolves nothing useful on NixOS - leaving the daemon with a PATH that finds almost nothing.

Fix:

  1. Root cause - resolve the login shell at service-definition render time (daemon install/daemon start, which runs in the user's normal environment, not the daemon's restricted one) and bake it into the generated systemd unit / launchd plist as SHELL, mirroring how proxy vars are already forwarded (internal/daemon/service_systemd.go, service_launchd.go, service.go's new resolveInstallShell). This gives the running daemon's LoginShell() fast path (a plain env lookup) a real value immediately, without ever needing to shell out from inside the restricted process. This generalizes beyond NixOS to any minimal-PATH service-managed environment.
  2. Defense in depth - added the NixOS system profile (/run/current-system/sw/bin) and user profile (~/.nix-profile/bin) directories to shellenv.WellKnownBinDirsForHome, so even a still-degraded fallback has a chance of finding real binaries.

Existing installs pick up the fix automatically: reinstallManagedServiceIfChanged detects the new SHELL= line as drift on the next daemon start and reinstalls.

Also added a minimal flake.nix (devShells.default with go_1_25, matching go.mod's go 1.25.0) so contributors get a working Go toolchain via nix develop rather than relying on a machine-level install - this was needed to validate this very fix locally, since the daemon's own gate can't run while this bug is live on a NixOS host.

Test plan

  • gofmt -w . - clean
  • make lint (skill drift check + go vet ./...) - clean
  • go test -race ./... (all packages, e2e excluded via build tag) - all pass
  • New regression tests:
    • internal/shellenv: TestLoginShell_SHELLSetSkipsGetentProbe, TestResolve_NixOSDegradedFallbackIncludesNixSystemProfile
    • internal/daemon: TestRenderSystemdUnitBakesInInstallTimeShell, TestRenderLaunchAgentBakesInInstallTimeShell, plus updated TestStartInstallsSystemdUnitAndStartsManagedDaemon / TestStartInstallsLaunchAgentAndBootstrapsManagedDaemon asserting the baked SHELL line
  • nix flake check and nix develop --command go version verified on a live NixOS host (this exact bug's original repro environment)
  • go build -o ./bin/no-mistakes ./cmd/no-mistakes - clean

🤖 Generated with Claude Code

On NixOS the daemon's minimal service-unit environment (HOME, PATH, proxy
vars only - never SHELL) breaks internal/shellenv's login-shell PATH probe
at its root: LoginShell() falls through to shelling out to `getent passwd`
using the daemon's own restricted starting PATH, which on a non-FHS distro
can't find getent either (or bash, for the probe itself), so the probe
degrades all the way to the FHS-only WellKnownBinDirs fallback. Daemon
subprocess calls like `git`/`ps` then fail with "executable file not found
in $PATH" even though those tools are on the invoking user's normal PATH.

Resolve the login shell at service-definition render time (daemon
install/start, which runs with a normal environment) and bake it into the
generated systemd unit / launchd plist as SHELL, mirroring how proxy vars
are already forwarded. This gives the running daemon's LoginShell() fast
path a real value to return immediately, without ever needing to shell out
from inside the restricted process. As defense in depth, also add the NixOS
system/user profile directories to WellKnownBinDirsForHome so a still-
degraded fallback has a chance of finding real binaries.

Add a minimal flake.nix so contributors get a matching Go 1.25 toolchain via
`nix develop` instead of relying on a machine-level install.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@greptile-apps

greptile-apps Bot commented Aug 18, 2026

Copy link
Copy Markdown

Confidence Score: 4/5

The PR is not yet safe to merge because a drift-triggered reinstall can still replace the preserved shell with the degraded bash fallback.

The comparison path preserves the installed absolute shell, but the actual systemd or launchd installer resolves SHELL again without that guard, leaving the previously reported daemon PATH regression reachable whenever another definition change triggers refresh in a restricted environment.

Files Needing Attention: internal/daemon/selfexec.go, internal/daemon/service.go, internal/daemon/service_systemd.go, internal/daemon/service_launchd.go

Comments Outside Diff (1)

  1. internal/daemon/selfexec.go, line 239 (link)

    P1 Refresh rewrites preserved shell

    If a restricted daemon start resolves the shell to bash while unrelated service-definition drift requires a reinstall, this call does not pass the shell preserved during drift rendering. The platform installer resolves the degraded value again, replaces the valid absolute SHELL, and restarts the daemon with a login-shell probe that cannot locate installed tools on non-FHS systems.

Reviews (3): Last reviewed commit: "fix(daemon): make installShellIsDegraded..." | Re-trigger Greptile

Comment thread internal/daemon/selfexec.go
mattyg and others added 2 commits August 18, 2026 15:06
reinstallManagedServiceIfChanged called resolveInstallShell() fresh on
every daemon start, unconditionally. If a later call happened to run in a
restricted environment again (no SHELL, no reachable getent/bash), it would
re-resolve the degraded literal "bash" fallback, treat the currently good
absolute SHELL already baked into the unit/plist as drift, and
reinstall+restart the service - reintroducing the exact PATH bug the SHELL
baking fix targets.

Mirror the existing proxyEnv drift-inheritance pattern: extract the
already-installed SHELL from the on-disk unit/plist (systemdUnitShell /
launchAgentShell) and prefer it over a freshly degraded resolution
(installShellIsDegraded, keyed on the fallback's literal non-absolute
"bash" vs a real absolute path). A genuine SHELL change (a different
absolute path) is still detected as drift and applied.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
filepath.IsAbs is platform-semantic: a POSIX path like /bin/bash has no
Windows drive letter or UNC prefix, so it reports false (not absolute)
under GOOS=windows. That misclassified every valid Unix shell path as
degraded and broke the Windows CI leg's internal/daemon tests, even though
Windows never actually renders these Unix-only systemd/launchd service
definitions.

The degradation signal is the resolved value itself - shellenv.LoginShell's
literal "bash" sentinel, or empty - not whether it looks like an absolute
path on the current host. Add a regression test asserting the check's
answer is identical across linux/darwin/windows runtimeGOOS.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: I reviewed internal/daemon/service.go, selfexec.go (reinstallManagedServiceIfChanged), and service_systemd.go installSystemdUserService (not the title). Baking install-time SHELL into the unit is the right fail-closed fix for the NixOS PATH chicken-and-egg, and drift comparison does inherit a non-degraded SHELL.

I will not auto-merge:

  • installSystemdUserService (and the launchd installer) still call resolveInstallShell() independently. If unrelated definition drift triggers a reinstall while this daemon start is in a restricted environment, the installer overwrites the preserved absolute SHELL with literal bash and restarts onto the broken PATH. Please pass the inherited shell into the installer.
  • Required check PR must be raised via no-mistakes is red (UNSTABLE). Please re-raise through the gate if you can.
  • flake.nix / flake.lock are extra contributor tooling, not needed for the daemon fix — drop or split them.

Tests/e2e/windows-git are green. Not a security stop.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants