diff --git a/flake.lock b/flake.lock new file mode 100644 index 000000000..304b2357e --- /dev/null +++ b/flake.lock @@ -0,0 +1,61 @@ +{ + "nodes": { + "flake-utils": { + "inputs": { + "systems": "systems" + }, + "locked": { + "lastModified": 1731533236, + "narHash": "sha256-l0KFg5HjrsfsO/JpG+r7fRrqm12kzFHyUHqHCVpMMbI=", + "owner": "numtide", + "repo": "flake-utils", + "rev": "11707dc2f618dd54ca8739b309ec4fc024de578b", + "type": "github" + }, + "original": { + "owner": "numtide", + "repo": "flake-utils", + "type": "github" + } + }, + "nixpkgs": { + "locked": { + "lastModified": 1787001381, + "narHash": "sha256-Ue1Yo8gfHdD4TMtNewhA4tkSYeFqXThju0nCyJc3ALo=", + "owner": "NixOS", + "repo": "nixpkgs", + "rev": "ec2d622de0773551768cf98f3fc50cbcc003b9c5", + "type": "github" + }, + "original": { + "owner": "NixOS", + "ref": "nixos-unstable", + "repo": "nixpkgs", + "type": "github" + } + }, + "root": { + "inputs": { + "flake-utils": "flake-utils", + "nixpkgs": "nixpkgs" + } + }, + "systems": { + "locked": { + "lastModified": 1681028828, + "narHash": "sha256-Vy1rq5AaRuLzOxct8nz4T6wlgyUR7zLU309k9mBC768=", + "owner": "nix-systems", + "repo": "default", + "rev": "da67096a3b9bf56a91d16901293e51ba5b49a27e", + "type": "github" + }, + "original": { + "owner": "nix-systems", + "repo": "default", + "type": "github" + } + } + }, + "root": "root", + "version": 7 +} diff --git a/flake.nix b/flake.nix new file mode 100644 index 000000000..786f4c778 --- /dev/null +++ b/flake.nix @@ -0,0 +1,22 @@ +{ + description = "no-mistakes development environment"; + + inputs = { + nixpkgs.url = "github:NixOS/nixpkgs/nixos-unstable"; + flake-utils.url = "github:numtide/flake-utils"; + }; + + outputs = { self, nixpkgs, flake-utils }: + flake-utils.lib.eachDefaultSystem (system: + let + pkgs = import nixpkgs { inherit system; }; + in + { + # go.mod requires `go 1.25.0`; go_1_25 keeps this dev shell in sync + # with that toolchain instead of relying on the machine's own PATH. + devShells.default = pkgs.mkShell { + buildInputs = [ pkgs.go_1_25 ]; + }; + } + ); +} diff --git a/internal/daemon/selfexec.go b/internal/daemon/selfexec.go index 2b895d12b..e160c4bb2 100644 --- a/internal/daemon/selfexec.go +++ b/internal/daemon/selfexec.go @@ -167,6 +167,7 @@ func reinstallManagedServiceIfChanged(p *paths.Paths) (bool, error) { // with "403 Request not allowed". This mirrors the executable inheritance // below: prefer the current environment, fall back to what is on disk. var inheritedProxyEnv [][2]string + var inheritedShell string if readErr == nil { switch runtimeGOOS { case "darwin": @@ -174,22 +175,38 @@ func reinstallManagedServiceIfChanged(p *paths.Paths) (bool, error) { renderedExecutable = existingExe } inheritedProxyEnv = launchAgentProxyEnv(existing) + if existingShell, ok := launchAgentShell(existing); ok { + inheritedShell = existingShell + } case "linux": if existingExe, ok := systemdUnitExecutable(existing); ok { renderedExecutable = existingExe } inheritedProxyEnv = systemdUnitProxyEnv(existing) + if existingShell, ok := systemdUnitShell(existing); ok { + inheritedShell = existingShell + } } } proxyEnv := serviceProxyEnv() if len(proxyEnv) == 0 { proxyEnv = inheritedProxyEnv } + // Prefer the freshly resolved shell, but never let a degraded resolution + // (this call itself running in a restricted environment - see + // installShellIsDegraded) overwrite an already-installed, previously + // working absolute SHELL: that would falsely detect drift and + // reinstall+restart the daemon back onto a broken PATH. See + // resolveInstallShell. + shell := resolveInstallShell() + if installShellIsDegraded(shell) && inheritedShell != "" { + shell = inheritedShell + } switch runtimeGOOS { case "darwin": - wanted = renderLaunchAgentWithProxyEnv(renderedExecutable, p, home, proxyEnv) + wanted = renderLaunchAgentWithProxyEnv(renderedExecutable, p, home, shell, proxyEnv) case "linux": - wanted = renderSystemdUnitWithProxyEnv(renderedExecutable, p, home, proxyEnv) + wanted = renderSystemdUnitWithProxyEnv(renderedExecutable, p, home, shell, proxyEnv) } switch { case readErr == nil && string(existing) == wanted: diff --git a/internal/daemon/service.go b/internal/daemon/service.go index 0a7aff515..85c51a747 100644 --- a/internal/daemon/service.go +++ b/internal/daemon/service.go @@ -12,6 +12,7 @@ import ( "testing" "github.com/kunchenguid/no-mistakes/internal/paths" + "github.com/kunchenguid/no-mistakes/internal/shellenv" ) // Base identifiers for the managed-service artifacts. The live identifiers @@ -42,6 +43,41 @@ var serviceManagerBypassed = defaultServiceManagerBypassed var prepareManagedDaemonLaunch = managedDaemonLaunch var inspectManagedDaemonService = managedDaemonServiceState +// resolveInstallShell resolves the login shell to bake into a generated +// service unit's environment as SHELL, at install/render time - never inside +// the running managed daemon itself. A daemon started by systemd/launchd +// inherits only a minimal environment (HOME, a curated PATH, proxy vars; see +// proxyEnvKeys above) with no SHELL, so internal/shellenv's LoginShell() falls +// through to shelling out to getent/dscl using that same minimal PATH - which +// on a non-FHS distro like NixOS can't even find getent, and the whole probe +// degrades to WellKnownBinDirs. The process rendering the service definition +// (`daemon install`/`daemon start`, run interactively or from the user's own +// shell) has a normal, complete environment, so resolving SHELL here and +// exporting it fixes the chicken-and-egg problem at its root: the daemon's own +// LoginShell() fast path (a plain env lookup) then succeeds immediately. +var resolveInstallShell = shellenv.LoginShell + +// installShellIsDegraded reports whether a resolveInstallShell() result is +// shellenv.LoginShell's last-resort literal "bash" sentinel (or empty) +// rather than a real, resolved shell. This must key off the value itself, +// not path shape: filepath.IsAbs is platform-semantic (a POSIX path like +// /bin/bash is not "absolute" under Windows' drive-letter/UNC rules), so it +// misclassified every valid Unix shell path as degraded on the Windows CI +// leg, which never actually renders these Unix-only service definitions but +// does compile and run this package's tests. +// +// Drift detection (reinstallManagedServiceIfChanged) uses this to avoid +// overwriting an already-installed, previously-resolved SHELL with a +// degraded one: without this check, a later `daemon start` that happens to +// run in a restricted environment (no SHELL, no working getent/bash on PATH +// - the exact NixOS chicken-and-egg problem this baked-in SHELL fixes) would +// re-resolve the degraded fallback, treat the currently good SHELL as drift, +// and reinstall+restart the service onto a broken one. +func installShellIsDegraded(shell string) bool { + shell = strings.TrimSpace(shell) + return shell == "" || shell == "bash" +} + type managedServiceState int type managedServiceLaunch struct { diff --git a/internal/daemon/service_launchd.go b/internal/daemon/service_launchd.go index 139782115..9135e5cbd 100644 --- a/internal/daemon/service_launchd.go +++ b/internal/daemon/service_launchd.go @@ -33,11 +33,12 @@ func installLaunchAgent(p *paths.Paths, exe string) error { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return fmt.Errorf("create launch agents directory: %w", err) } + shell := resolveInstallShell() // writeServiceFile resolves the proxy environment once and feeds it to the // renderer, so the plist content and its permission mode stay in sync // (see serviceProxyEnv / writeServiceFile). render := func(proxyEnv [][2]string) string { - return renderLaunchAgentWithProxyEnv(exe, p, home, proxyEnv) + return renderLaunchAgentWithProxyEnv(exe, p, home, shell, proxyEnv) } if err := writeServiceFile(path, launchAgentProxyEnv, render); err != nil { return fmt.Errorf("write launch agent: %w", err) @@ -186,12 +187,13 @@ func launchdDomainTarget() (string, error) { // (preferring the on-disk definition when the live environment has none) and // pass it in. func renderLaunchAgent(exe string, p *paths.Paths, home string) string { - return renderLaunchAgentWithProxyEnv(exe, p, home, serviceProxyEnv()) + return renderLaunchAgentWithProxyEnv(exe, p, home, resolveInstallShell(), serviceProxyEnv()) } // renderLaunchAgentWithProxyEnv renders the launchd plist using a proxy -// environment supplied by the caller (see serviceProxyEnv). -func renderLaunchAgentWithProxyEnv(exe string, p *paths.Paths, home string, proxyEnv [][2]string) string { +// environment supplied by the caller (see serviceProxyEnv) and a login shell +// resolved by the caller (see resolveInstallShell). +func renderLaunchAgentWithProxyEnv(exe string, p *paths.Paths, home string, shell string, proxyEnv [][2]string) string { values := []string{exe, "daemon", "run", "--root", p.Root()} var args strings.Builder for _, value := range values { @@ -214,6 +216,9 @@ func renderLaunchAgentWithProxyEnv(exe string, p *paths.Paths, home string, prox envDict.WriteString(" PATH\n ") envDict.WriteString(xmlEscaped(managedServicePath(home))) envDict.WriteString("\n") + envDict.WriteString(" SHELL\n ") + envDict.WriteString(xmlEscaped(shell)) + envDict.WriteString("\n") for _, kv := range proxyEnv { envDict.WriteString(" ") envDict.WriteString(xmlEscaped(kv[0])) diff --git a/internal/daemon/service_launchd_test.go b/internal/daemon/service_launchd_test.go index c29d9b201..12656a09c 100644 --- a/internal/daemon/service_launchd_test.go +++ b/internal/daemon/service_launchd_test.go @@ -56,6 +56,7 @@ func TestStartInstallsLaunchAgentAndBootstrapsManagedDaemon(t *testing.T) { "EnvironmentVariables", "HOME", "" + home + "", + "SHELL\n /bin/bash", "RunAtLoad", "KeepAlive", } { @@ -302,7 +303,7 @@ func TestRenderLaunchAgentForwardsEveryProxyEnvKey(t *testing.T) { proxyEnv = append(proxyEnv, [2]string{key, "val-" + key}) } - plist := renderLaunchAgentWithProxyEnv("/opt/no-mistakes/bin/no-mistakes", paths.WithRoot(t.TempDir()), "/home/u", proxyEnv) + plist := renderLaunchAgentWithProxyEnv("/opt/no-mistakes/bin/no-mistakes", paths.WithRoot(t.TempDir()), "/home/u", "/bin/bash", proxyEnv) for _, key := range proxyEnvKeys { fragment := "" + key + "\n val-" + key + "" if !strings.Contains(plist, fragment) { diff --git a/internal/daemon/service_render.go b/internal/daemon/service_render.go index a5c1198ad..8d694b884 100644 --- a/internal/daemon/service_render.go +++ b/internal/daemon/service_render.go @@ -151,6 +151,35 @@ func systemdUnitProxyEnv(data []byte) [][2]string { return out } +// systemdUnitShell extracts the baked-in SHELL Environment= entry from a +// rendered systemd unit, mirroring systemdUnitProxyEnv. Drift detection uses +// this to preserve an already-installed SHELL when the current render +// resolution has degraded (see resolveInstallShell / shellenv.LoginShell's +// literal "bash" fallback) instead of overwriting a previously-resolved, +// working absolute path with an unresolvable one - which would falsely +// detect drift on every subsequent `daemon start` in a restricted +// environment and reinstall+restart the daemon back into the NixOS PATH bug +// this mechanism exists to fix. +func systemdUnitShell(data []byte) (string, bool) { + for _, line := range strings.Split(string(data), "\n") { + line = strings.TrimSpace(line) + if !strings.HasPrefix(line, "Environment=") { + continue + } + assignment, err := strconv.Unquote(strings.TrimPrefix(line, "Environment=")) + if err != nil { + continue + } + assignment = strings.ReplaceAll(assignment, "%%", "%") + key, value, ok := strings.Cut(assignment, "=") + if !ok || key != "SHELL" { + continue + } + return value, true + } + return "", false +} + // launchAgentProxyEnv extracts the forwarded proxy entries from the // EnvironmentVariables of a rendered launchd plist, mirroring // launchAgentExecutable. Pairs are returned in file order with XML escaping @@ -210,6 +239,62 @@ func launchAgentProxyEnv(data []byte) [][2]string { } } +// launchAgentShell extracts the baked-in SHELL value from the +// EnvironmentVariables of a rendered launchd plist, mirroring +// launchAgentProxyEnv. See systemdUnitShell for why drift detection needs +// this. +func launchAgentShell(data []byte) (string, bool) { + decoder := xml.NewDecoder(bytes.NewReader(data)) + var sawEnvVarsKey bool + var inEnvDict bool + var pendingKey string + var havePendingKey bool + for { + token, err := decoder.Token() + if err != nil { + return "", false + } + switch t := token.(type) { + case xml.StartElement: + switch t.Name.Local { + case "key": + var key string + if err := decoder.DecodeElement(&key, &t); err != nil { + return "", false + } + key = strings.TrimSpace(key) + if inEnvDict { + pendingKey = key + havePendingKey = true + } else { + sawEnvVarsKey = key == "EnvironmentVariables" + } + case "dict": + if sawEnvVarsKey { + inEnvDict = true + sawEnvVarsKey = false + } + case "string": + if !inEnvDict || !havePendingKey { + continue + } + var value string + if err := decoder.DecodeElement(&value, &t); err != nil { + return "", false + } + if pendingKey == "SHELL" { + return value, true + } + havePendingKey = false + } + case xml.EndElement: + if inEnvDict && t.Name.Local == "dict" { + return "", false + } + } + } +} + func firstCommandArg(command string) (string, bool) { if command == "" { return "", false diff --git a/internal/daemon/service_render_proxy_test.go b/internal/daemon/service_render_proxy_test.go index a5f560f68..2f6dbbc78 100644 --- a/internal/daemon/service_render_proxy_test.go +++ b/internal/daemon/service_render_proxy_test.go @@ -21,13 +21,13 @@ func TestSystemdUnitProxyEnvRoundTripsThroughRenderer(t *testing.T) { {"http_proxy", "http://lower:1/"}, } - unit := renderSystemdUnitWithProxyEnv("/usr/local/bin/no-mistakes", p, "/home/u", in) + unit := renderSystemdUnitWithProxyEnv("/usr/local/bin/no-mistakes", p, "/home/u", "/bin/bash", in) got := systemdUnitProxyEnv([]byte(unit)) if !reflect.DeepEqual(got, in) { t.Fatalf("systemdUnitProxyEnv() = %v, want %v (HOME/PATH must be skipped, %% undone)", got, in) } - if reRendered := renderSystemdUnitWithProxyEnv("/usr/local/bin/no-mistakes", p, "/home/u", got); reRendered != unit { + if reRendered := renderSystemdUnitWithProxyEnv("/usr/local/bin/no-mistakes", p, "/home/u", "/bin/bash", got); reRendered != unit { t.Fatalf("re-rendering parsed proxy env did not reproduce the unit:\nwant:\n%s\ngot:\n%s", unit, reRendered) } } @@ -43,13 +43,13 @@ func TestLaunchAgentProxyEnvRoundTripsThroughRenderer(t *testing.T) { {"http_proxy", "http://lower:1/"}, } - plist := renderLaunchAgentWithProxyEnv("/usr/local/bin/no-mistakes", p, "/home/u", in) + plist := renderLaunchAgentWithProxyEnv("/usr/local/bin/no-mistakes", p, "/home/u", "/bin/bash", in) got := launchAgentProxyEnv([]byte(plist)) if !reflect.DeepEqual(got, in) { t.Fatalf("launchAgentProxyEnv() = %v, want %v (HOME/PATH must be skipped)", got, in) } - if reRendered := renderLaunchAgentWithProxyEnv("/usr/local/bin/no-mistakes", p, "/home/u", got); reRendered != plist { + if reRendered := renderLaunchAgentWithProxyEnv("/usr/local/bin/no-mistakes", p, "/home/u", "/bin/bash", got); reRendered != plist { t.Fatalf("re-rendering parsed proxy env did not reproduce the plist:\nwant:\n%s\ngot:\n%s", plist, reRendered) } } @@ -59,11 +59,11 @@ func TestLaunchAgentProxyEnvRoundTripsThroughRenderer(t *testing.T) { // a no-op when there was never a baked-in proxy. func TestServiceProxyEnvParsersReturnNilForNoProxyDefinition(t *testing.T) { p := paths.WithRoot(t.TempDir()) - unit := renderSystemdUnitWithProxyEnv("/usr/local/bin/no-mistakes", p, "/home/u", nil) + unit := renderSystemdUnitWithProxyEnv("/usr/local/bin/no-mistakes", p, "/home/u", "/bin/bash", nil) if got := systemdUnitProxyEnv([]byte(unit)); len(got) != 0 { t.Fatalf("systemdUnitProxyEnv(no-proxy unit) = %v, want empty", got) } - plist := renderLaunchAgentWithProxyEnv("/usr/local/bin/no-mistakes", p, "/home/u", nil) + plist := renderLaunchAgentWithProxyEnv("/usr/local/bin/no-mistakes", p, "/home/u", "/bin/bash", nil) if got := launchAgentProxyEnv([]byte(plist)); len(got) != 0 { t.Fatalf("launchAgentProxyEnv(no-proxy plist) = %v, want empty", got) } diff --git a/internal/daemon/service_render_test.go b/internal/daemon/service_render_test.go index 52e95631f..7e9bcbc0c 100644 --- a/internal/daemon/service_render_test.go +++ b/internal/daemon/service_render_test.go @@ -131,6 +131,86 @@ func TestManagedServicePathUsesSharedWellKnownDirs(t *testing.T) { } } +// TestInstallShellIsDegraded locks in the signal reinstallManagedServiceIfChanged +// uses to tell a real resolveInstallShell() result (a set $SHELL, or a +// getent/dscl lookup - always an absolute path) apart from +// shellenv.LoginShell's last-resort literal "bash" fallback, so drift +// detection knows when it must not overwrite an already-installed shell (see +// service_shell_inherit_test.go). +func TestInstallShellIsDegraded(t *testing.T) { + t.Parallel() + for _, tc := range []struct { + shell string + want bool + }{ + {"/bin/bash", false}, + {"/run/current-system/sw/bin/bash", false}, + {"/usr/bin/zsh", false}, + {"bash", true}, + {"", true}, + } { + if got := installShellIsDegraded(tc.shell); got != tc.want { + t.Errorf("installShellIsDegraded(%q) = %v, want %v", tc.shell, got, tc.want) + } + } +} + +// TestInstallShellIsDegraded_PlatformIndependent is the Windows CI +// regression guard: installShellIsDegraded used to delegate to +// filepath.IsAbs, which is platform-semantic (a POSIX path like /bin/bash +// has no Windows drive letter or UNC prefix, so filepath.IsAbs returns false +// for it under GOOS=windows). That misclassified every valid Unix shell path +// as degraded and broke the Windows CI leg, even though this package's +// Windows build never actually renders the Unix-only systemd/launchd +// definitions installShellIsDegraded protects. The check must depend only on +// the resolved value, not the host's path-parsing rules, so it must return +// the same answer regardless of runtimeGOOS. +func TestInstallShellIsDegraded_PlatformIndependent(t *testing.T) { + oldGOOS := runtimeGOOS + defer func() { runtimeGOOS = oldGOOS }() + + for _, shell := range []string{"/bin/bash", "/run/current-system/sw/bin/bash", "/usr/bin/zsh", "bash", ""} { + var results []bool + for _, goos := range []string{"linux", "darwin", "windows"} { + runtimeGOOS = goos + results = append(results, installShellIsDegraded(shell)) + } + for i := 1; i < len(results); i++ { + if results[i] != results[0] { + t.Fatalf("installShellIsDegraded(%q) depends on runtimeGOOS: %v", shell, results) + } + } + } +} + +// TestRenderSystemdUnitBakesInInstallTimeShell and +// TestRenderLaunchAgentBakesInInstallTimeShell are the NixOS root-cause +// regression: a daemon started by systemd/launchd inherits only HOME, a +// curated PATH, and proxy vars - never SHELL - so +// internal/shellenv.LoginShell() inside the running daemon falls back to +// shelling out to getent/dscl using that same minimal PATH, which on a +// non-FHS distro like NixOS can't even find getent. Baking the value the +// installing process resolved (via resolveInstallShell, which has a normal +// environment) directly into the generated unit/plist as SHELL gives +// LoginShell()'s fast path a real value to return immediately, without ever +// shelling out from inside the restricted daemon process. +func TestRenderSystemdUnitBakesInInstallTimeShell(t *testing.T) { + t.Parallel() + unit := renderSystemdUnitWithProxyEnv("/usr/local/bin/no-mistakes", paths.WithRoot(t.TempDir()), "/home/u", "/run/current-system/sw/bin/bash", nil) + want := `Environment="SHELL=/run/current-system/sw/bin/bash"` + if !strings.Contains(unit, want) { + t.Fatalf("systemd unit should bake in the install-time shell, want %q, got:\n%s", want, unit) + } +} + +func TestRenderLaunchAgentBakesInInstallTimeShell(t *testing.T) { + t.Parallel() + plist := renderLaunchAgentWithProxyEnv("/usr/local/bin/no-mistakes", paths.WithRoot(t.TempDir()), "/home/u", "/run/current-system/sw/bin/bash", nil) + if got := extractPlistValue(t, plist, "SHELL"); got != "/run/current-system/sw/bin/bash" { + t.Fatalf("launchd plist SHELL = %q, want %q", got, "/run/current-system/sw/bin/bash") + } +} + // extractPlistValue pulls the value that follows a given in // an Apple plist. Keeps the rendering assertions readable and independent // of byte-for-byte formatting. diff --git a/internal/daemon/service_shell_inherit_test.go b/internal/daemon/service_shell_inherit_test.go new file mode 100644 index 000000000..8fa59282e --- /dev/null +++ b/internal/daemon/service_shell_inherit_test.go @@ -0,0 +1,198 @@ +package daemon + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/kunchenguid/no-mistakes/internal/paths" +) + +// TestReinstallSystemdServiceKeepsInstalledShellWhenResolutionDegrades is the +// regression guard for a PR #770 review finding: drift detection used to call +// resolveInstallShell() unconditionally on every `daemon start`. A daemon +// that installed successfully with a real SHELL (because the installing +// shell had $SHELL set, or getent/dscl resolved one) could later be +// restarted from a restricted environment where resolveInstallShell() +// degrades to the literal "bash" fallback (see shellenv.LoginShell) - the +// exact NixOS chicken-and-egg situation the baked-in SHELL fixes. Without +// this guard, that later degraded re-resolution would be treated as drift, +// overwrite the already-working absolute SHELL with the unusable "bash" +// literal, and reinstall+restart the daemon straight back into the PATH bug +// this mechanism exists to fix. +func TestReinstallSystemdServiceKeepsInstalledShellWhenResolutionDegrades(t *testing.T) { + p := paths.WithRoot(filepath.Join(t.TempDir(), "nm-home")) + if err := p.EnsureDirs(); err != nil { + t.Fatal(err) + } + home := t.TempDir() + + cleanup := stubServiceRuntime(t) + defer cleanup() + runtimeGOOS = "linux" + serviceUserHomeDir = func() (string, error) { return home, nil } + serviceExecutablePath = func() (string, error) { return "/usr/local/bin/no-mistakes", nil } + + unitPath := filepath.Join(home, ".config", "systemd", "user", systemdServiceName(p)) + if err := os.MkdirAll(filepath.Dir(unitPath), 0o755); err != nil { + t.Fatal(err) + } + + // Install-time resolution succeeded with a real absolute shell. + resolveInstallShell = func() string { return "/run/current-system/sw/bin/bash" } + unit := renderSystemdUnit("/usr/local/bin/no-mistakes", p, home) + if err := os.WriteFile(unitPath, []byte(unit), 0o644); err != nil { + t.Fatal(err) + } + + // A later `daemon start` runs in a restricted environment (no SHELL, no + // reachable getent/bash) where the probe degrades to the literal "bash". + resolveInstallShell = func() string { return "bash" } + + var commands []string + serviceCommandRunner = func(name string, args ...string) ([]byte, error) { + commands = append(commands, name+" "+strings.Join(args, " ")) + return nil, nil + } + daemonHealthCheck = func(*paths.Paths) (bool, error) { return true, nil } + + changed, err := reinstallManagedServiceIfChanged(p) + if err != nil { + t.Fatalf("reinstallManagedServiceIfChanged: %v", err) + } + if changed { + t.Fatal("degraded shell re-resolution re-detected drift and reinstalled, reintroducing the NixOS PATH bug") + } + if len(commands) != 0 { + t.Fatalf("no systemctl command should run when there is no real drift; ran %v", commands) + } + data, err := os.ReadFile(unitPath) + if err != nil { + t.Fatal(err) + } + if string(data) != unit { + t.Fatalf("unit changed after degraded-shell restart:\n%s", data) + } + if !strings.Contains(string(data), `Environment="SHELL=/run/current-system/sw/bin/bash"`) { + t.Fatal("the previously-installed working SHELL was overwritten with the degraded fallback") + } +} + +// TestReinstallLaunchAgentKeepsInstalledShellWhenResolutionDegrades is the +// launchd counterpart. +func TestReinstallLaunchAgentKeepsInstalledShellWhenResolutionDegrades(t *testing.T) { + p := paths.WithRoot(filepath.Join(t.TempDir(), "nm-home")) + if err := p.EnsureDirs(); err != nil { + t.Fatal(err) + } + home := t.TempDir() + + cleanup := stubServiceRuntime(t) + defer cleanup() + runtimeGOOS = "darwin" + serviceUserHomeDir = func() (string, error) { return home, nil } + serviceExecutablePath = func() (string, error) { return "/usr/local/bin/no-mistakes", nil } + + plistPath := launchAgentPath(p) + if err := os.MkdirAll(filepath.Dir(plistPath), 0o755); err != nil { + t.Fatal(err) + } + + resolveInstallShell = func() string { return "/opt/homebrew/bin/bash" } + plist := renderLaunchAgent("/usr/local/bin/no-mistakes", p, home) + if err := os.WriteFile(plistPath, []byte(plist), 0o644); err != nil { + t.Fatal(err) + } + + resolveInstallShell = func() string { return "bash" } + + var commands []string + serviceCommandRunner = func(name string, args ...string) ([]byte, error) { + commands = append(commands, name+" "+strings.Join(args, " ")) + return nil, nil + } + daemonHealthCheck = func(*paths.Paths) (bool, error) { return true, nil } + + changed, err := reinstallManagedServiceIfChanged(p) + if err != nil { + t.Fatalf("reinstallManagedServiceIfChanged: %v", err) + } + if changed { + t.Fatal("degraded shell re-resolution re-detected drift and reinstalled, reintroducing the NixOS PATH bug") + } + if len(commands) != 0 { + t.Fatalf("no launchctl command should run when there is no real drift; ran %v", commands) + } + data, err := os.ReadFile(plistPath) + if err != nil { + t.Fatal(err) + } + if string(data) != plist { + t.Fatalf("plist changed after degraded-shell restart:\n%s", data) + } + if !strings.Contains(string(data), "SHELL\n /opt/homebrew/bin/bash") { + t.Fatal("the previously-installed working SHELL was overwritten with the degraded fallback") + } +} + +// TestReinstallSystemdServiceAppliesAGenuineShellChange guards the other +// direction: the degraded-resolution guard above must not make drift +// detection blind to a real, intentional SHELL change (e.g. the installing +// user's default shell changed from bash to zsh). When resolveInstallShell +// returns a different absolute path, that is real drift and must still be +// applied. +func TestReinstallSystemdServiceAppliesAGenuineShellChange(t *testing.T) { + p := paths.WithRoot(filepath.Join(t.TempDir(), "nm-home")) + if err := p.EnsureDirs(); err != nil { + t.Fatal(err) + } + home := t.TempDir() + + cleanup := stubServiceRuntime(t) + defer cleanup() + runtimeGOOS = "linux" + serviceUserHomeDir = func() (string, error) { return home, nil } + serviceExecutablePath = func() (string, error) { return "/usr/local/bin/no-mistakes", nil } + + unitPath := filepath.Join(home, ".config", "systemd", "user", systemdServiceName(p)) + if err := os.MkdirAll(filepath.Dir(unitPath), 0o755); err != nil { + t.Fatal(err) + } + + resolveInstallShell = func() string { return "/bin/bash" } + unit := renderSystemdUnit("/usr/local/bin/no-mistakes", p, home) + if err := os.WriteFile(unitPath, []byte(unit), 0o644); err != nil { + t.Fatal(err) + } + + resolveInstallShell = func() string { return "/usr/bin/zsh" } + + running := true + serviceCommandRunner = func(name string, args ...string) ([]byte, error) { + command := name + " " + strings.Join(args, " ") + if strings.Contains(command, "systemctl --user stop ") { + running = false + } + if strings.Contains(command, "systemctl --user restart ") || strings.Contains(command, "systemctl --user start ") { + running = true + } + return nil, nil + } + daemonHealthCheck = func(*paths.Paths) (bool, error) { return running, nil } + + changed, err := reinstallManagedServiceIfChanged(p) + if err != nil { + t.Fatalf("reinstallManagedServiceIfChanged: %v", err) + } + if !changed { + t.Fatal("a genuine SHELL change should still be detected as drift and applied") + } + data, err := os.ReadFile(unitPath) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(data), `Environment="SHELL=/usr/bin/zsh"`) { + t.Fatalf("expected the new SHELL to be applied, got:\n%s", data) + } +} diff --git a/internal/daemon/service_systemd.go b/internal/daemon/service_systemd.go index 5f991c736..2da50d521 100644 --- a/internal/daemon/service_systemd.go +++ b/internal/daemon/service_systemd.go @@ -19,11 +19,12 @@ func installSystemdUserService(p *paths.Paths, exe string) error { if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { return fmt.Errorf("create systemd user directory: %w", err) } + shell := resolveInstallShell() // writeServiceFile resolves the proxy environment once and feeds it to the // renderer, so the unit content and its permission mode stay in sync // (see serviceProxyEnv / writeServiceFile). render := func(proxyEnv [][2]string) string { - return renderSystemdUnitWithProxyEnv(exe, p, home, proxyEnv) + return renderSystemdUnitWithProxyEnv(exe, p, home, shell, proxyEnv) } if err := writeServiceFile(path, systemdUnitProxyEnv, render); err != nil { return fmt.Errorf("write systemd unit: %w", err) @@ -93,12 +94,13 @@ func legacySystemdUserServicePath() string { // (preferring the on-disk definition when the live environment has none) and // pass it in. func renderSystemdUnit(exe string, p *paths.Paths, home string) string { - return renderSystemdUnitWithProxyEnv(exe, p, home, serviceProxyEnv()) + return renderSystemdUnitWithProxyEnv(exe, p, home, resolveInstallShell(), serviceProxyEnv()) } // renderSystemdUnitWithProxyEnv renders the systemd unit using a proxy -// environment supplied by the caller (see serviceProxyEnv). -func renderSystemdUnitWithProxyEnv(exe string, p *paths.Paths, home string, proxyEnv [][2]string) string { +// environment supplied by the caller (see serviceProxyEnv) and a login shell +// resolved by the caller (see resolveInstallShell). +func renderSystemdUnitWithProxyEnv(exe string, p *paths.Paths, home string, shell string, proxyEnv [][2]string) string { command := strings.Join([]string{ systemdEscapeArg(exe), systemdEscapeArg("daemon"), @@ -109,6 +111,7 @@ func renderSystemdUnitWithProxyEnv(exe string, p *paths.Paths, home string, prox envLines := []string{ systemdEnvironmentLine("HOME", home), systemdEnvironmentLine("PATH", managedServicePath(home)), + systemdEnvironmentLine("SHELL", shell), } // Forward proxy variables so the daemon (and the agents it spawns) can // reach the network through the user's proxy. See serviceProxyEnv. diff --git a/internal/daemon/service_systemd_test.go b/internal/daemon/service_systemd_test.go index aa58c2ab4..4e73223f6 100644 --- a/internal/daemon/service_systemd_test.go +++ b/internal/daemon/service_systemd_test.go @@ -54,6 +54,7 @@ func TestStartInstallsSystemdUnitAndStartsManagedDaemon(t *testing.T) { "ExecStart=/usr/local/bin/no-mistakes daemon run --root " + p.Root(), "WorkingDirectory=" + p.Root(), "Environment=\"HOME=" + home + "\"", + "Environment=\"SHELL=/bin/bash\"", "Restart=always", "WantedBy=default.target", } { @@ -274,7 +275,7 @@ func TestRenderSystemdUnitForwardsEveryProxyEnvKey(t *testing.T) { proxyEnv = append(proxyEnv, [2]string{key, "val-" + key}) } - unit := renderSystemdUnitWithProxyEnv("/usr/local/bin/no-mistakes", paths.WithRoot(t.TempDir()), "/home/u", proxyEnv) + unit := renderSystemdUnitWithProxyEnv("/usr/local/bin/no-mistakes", paths.WithRoot(t.TempDir()), "/home/u", "/bin/bash", proxyEnv) for _, key := range proxyEnvKeys { want := `Environment="` + key + "=val-" + key + `"` if !strings.Contains(unit, want) { @@ -297,7 +298,7 @@ func TestRenderSystemdUnitEscapesPercentInProxyEnv(t *testing.T) { {"HTTPS_PROXY", "http://user:p%40ss%3Aw0rd@proxy:8080"}, } - unit := renderSystemdUnitWithProxyEnv("/usr/local/bin/no-mistakes", paths.WithRoot(t.TempDir()), "/home/u", proxyEnv) + unit := renderSystemdUnitWithProxyEnv("/usr/local/bin/no-mistakes", paths.WithRoot(t.TempDir()), "/home/u", "/bin/bash", proxyEnv) want := `Environment="HTTPS_PROXY=http://user:p%%40ss%%3Aw0rd@proxy:8080"` if !strings.Contains(unit, want) { t.Fatalf("systemd unit should double %% in proxy env so it survives specifier expansion, want %q, got:\n%s", want, unit) diff --git a/internal/daemon/service_test.go b/internal/daemon/service_test.go index 142eb73b2..b2e2b61d4 100644 --- a/internal/daemon/service_test.go +++ b/internal/daemon/service_test.go @@ -1705,6 +1705,7 @@ func stubServiceRuntime(t *testing.T) func() { oldInspectManagedDaemonService := inspectManagedDaemonService oldHealthCheck := daemonHealthCheck oldServiceBypass := serviceManagerBypassed + oldResolveInstallShell := resolveInstallShell serviceManagerBypassed = func() bool { return false } prepareManagedDaemonLaunch = func(*paths.Paths) (managedServiceLaunch, error) { return managedServiceLaunch{}, nil @@ -1712,6 +1713,9 @@ func stubServiceRuntime(t *testing.T) func() { inspectManagedDaemonService = func(*paths.Paths, managedServiceLaunch) (managedServiceState, error) { return managedServiceUnknown, nil } + // Stubbed to a fixed value so render assertions are deterministic and + // independent of the SHELL env var of the process running `go test`. + resolveInstallShell = func() string { return "/bin/bash" } return func() { runtimeGOOS = oldGOOS serviceUserHomeDir = oldUserHomeDir @@ -1722,5 +1726,6 @@ func stubServiceRuntime(t *testing.T) func() { inspectManagedDaemonService = oldInspectManagedDaemonService daemonHealthCheck = oldHealthCheck serviceManagerBypassed = oldServiceBypass + resolveInstallShell = oldResolveInstallShell } } diff --git a/internal/shellenv/shellenv.go b/internal/shellenv/shellenv.go index acc7a3022..d8f80a03b 100644 --- a/internal/shellenv/shellenv.go +++ b/internal/shellenv/shellenv.go @@ -144,6 +144,7 @@ func WellKnownBinDirsForHome(home string) []string { filepath.Join(home, ".local", "bin"), filepath.Join(home, "go", "bin"), filepath.Join(home, ".cargo", "bin"), + filepath.Join(home, ".nix-profile", "bin"), filepath.Join(home, "bin"), ) } @@ -156,6 +157,13 @@ func WellKnownBinDirsForHome(home string) []string { "/bin", "/usr/sbin", "/sbin", + // NixOS keeps real binaries out of the FHS locations above; the system + // profile symlink farm is the non-FHS equivalent of /usr/bin. This is + // defense in depth for the degraded fallback only - the root-cause fix + // is baking SHELL into the generated service unit (see + // internal/daemon/service_systemd.go / service_launchd.go) so the login + // shell probe above succeeds instead of falling back to this list. + "/run/current-system/sw/bin", ) return dirs } diff --git a/internal/shellenv/shellenv_test.go b/internal/shellenv/shellenv_test.go index 46f1179dc..e1a979520 100644 --- a/internal/shellenv/shellenv_test.go +++ b/internal/shellenv/shellenv_test.go @@ -487,3 +487,88 @@ func TestShellCommandTimeout_IsRelaxed(t *testing.T) { t.Fatalf("shell resolution timeout %v is too aggressive; interactive shells under load need headroom (#143)", shellCommandTimeout) } } + +// TestLoginShell_SHELLSetSkipsGetentProbe is the NixOS root-cause regression. +// A daemon started by systemd/launchd only exports HOME, PATH, and proxy vars +// - SHELL is not among them. On NixOS, LoginShell() unset-SHELL fallback +// shells out to `getent passwd $USER` using the daemon's own minimal starting +// PATH, but NixOS keeps /bin and /usr/bin nearly empty (real binaries live +// under /run/current-system/sw/bin and /nix/store/...), so getent itself +// cannot be found either: LoginShell() falls all the way through to its +// hardcoded "bash" default, and the shell probe in resolveUncached then also +// cannot find bash on that same minimal PATH. Baking the install-time SHELL +// value into the generated service unit (internal/daemon/service_systemd.go / +// service_launchd.go) fixes this by giving LoginShell()'s fast path a real, +// already-resolved absolute path, so it never needs to shell out to getent at +// all. This test locks in that no subprocess is invoked once SHELL is set. +func TestLoginShell_SHELLSetSkipsGetentProbe(t *testing.T) { + resetForTests() + t.Setenv("SHELL", "/run/current-system/sw/bin/bash") + + oldOutput := shellCommandOutput + defer func() { + shellCommandOutput = oldOutput + resetForTests() + }() + shellCommandOutput = func(name string, args ...string) ([]byte, error) { + t.Fatalf("LoginShell should not shell out when $SHELL is already set, got %s %v", name, args) + return nil, nil + } + + if got := LoginShell(); got != "/run/current-system/sw/bin/bash" { + t.Fatalf("LoginShell() = %q, want the SHELL env var value", got) + } +} + +// TestResolve_NixOSDegradedFallbackIncludesNixSystemProfile reproduces the +// diagnosed NixOS bug end-to-end at the shellenv layer, for a daemon that +// somehow still starts without a usable SHELL (e.g. an older installed +// service unit before the install-time SHELL fix takes effect on next +// `daemon start`): SHELL is unset, and the getent probe LoginShell() would +// use on Linux also fails (simulating the daemon's minimal starting PATH +// being unable to find getent). LoginShell() ends up at its final "bash" +// default, the login-shell probe itself then fails to find bash too, and +// resolveUncached falls back to WellKnownBinDirsForHome. That fallback list +// must include the NixOS system and user profile directories so daemon +// subprocess calls like `git`/`ps` can still resolve real binaries instead of +// failing with "executable file not found in $PATH". +func TestResolve_NixOSDegradedFallbackIncludesNixSystemProfile(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("Resolve short-circuits to os.Environ() on Windows") + } + resetForTests() + oldGOOS := runtimeGOOS + runtimeGOOS = "linux" + t.Setenv("SHELL", "") + t.Setenv("HOME", "/home/nixuser") + t.Setenv("USER", "nixuser") + + oldOutput := shellCommandOutput + defer func() { + shellCommandOutput = oldOutput + runtimeGOOS = oldGOOS + resetForTests() + }() + shellCommandOutput = func(string, ...string) ([]byte, error) { + // Simulates both `getent` (LoginShell's Linux fallback) and `bash` (the + // probe shell itself) being unreachable on the daemon's minimal PATH. + return nil, &noSuchFileError{} + } + + env, err := Resolve() + if err != nil { + t.Fatal(err) + } + path, ok := envValue(env, "PATH") + if !ok { + t.Fatalf("expected PATH in resolved env, got %v", env) + } + for _, want := range []string{ + "/run/current-system/sw/bin", + "/home/nixuser/.nix-profile/bin", + } { + if !strings.Contains(path, want) { + t.Fatalf("expected degraded fallback PATH to include %q, got %q", want, path) + } + } +}