fix: address system-install review findings

- shim: make system executable resolution injectable so tests pass under
  umask 002; skip the root-owner test when running as root
- doctor: treat resolution into either the system or per-user shim dir as
  intercepted, and collapse the shim-in-PATH check to a single call site
- setup: make remove (both --system and per-user) best-effort with
  errors.Join so one failed step no longer strands the other artifact
- shim: allow a group-writable install parent dir (Debian/Ubuntu ship
  /usr/local/bin as root:staff 2775) while still rejecting world-writable
  and non-root-owned parents
- audit: attribute cloud events to SUDO_USER when running under sudo
- docs: drop the soft-fail event-logging claim (hard-fail is retained)

cmd/setup/doctor.go

@@ -103,7 +103,7 @@ func runCoreChecks(cfg *config.RuntimeConfig) []doctor.CheckResult {
 				info, err := os.Stat(cfg.EventLogDir())
 				if err != nil {
 					return doctor.CheckResult{
-						Status:  doctor.StatusWarn,
+						Status:  doctor.StatusFail,
 						Message: "Event log directory not found",
 					}
 				}
@@ -304,14 +304,18 @@ func pathIsUnderDir(path, dir string) bool {
 	return strings.HasPrefix(cleanPath, prefix)
 }
 
-func classifyPackageManagerResolutions(packageManagers []string, shimDir string, lookPath func(string) (string, error)) (underShim, shadowed []string) {
+// classifyPackageManagerResolutions splits package managers by where they
+// resolve on PATH: underShim means the command runs through a pmg shim (so it
+// is intercepted), shadowed means a real npm/pip sits ahead of the shims (so
+// interception is bypassed and the user should be warned).
+func classifyPackageManagerResolutions(packageManagers []string, shimDirs []string, lookPath func(string) (string, error)) (underShim, shadowed []string) {
 	for _, pm := range packageManagers {
 		resolved, err := lookPath(pm)
 		if err != nil {
 			continue
 		}
 
-		if pathIsUnderDir(resolved, shimDir) {
+		if resolvesUnderAny(resolved, shimDirs) {
 			underShim = append(underShim, pm)
 			continue
 		}
@@ -320,10 +324,30 @@ func classifyPackageManagerResolutions(packageManagers []string, shimDir string,
 	return underShim, shadowed
 }
 
+func resolvesUnderAny(path string, dirs []string) bool {
+	for _, dir := range dirs {
+		if pathIsUnderDir(path, dir) {
+			return true
+		}
+	}
+	return false
+}
+
+// shimDirs lists every directory a package manager may legitimately resolve
+// into. System and per-user installs are supported side by side, so resolution
+// into either shim directory counts as intercepted rather than shadowed.
+func shimDirs() []string {
+	dirs := []string{shim.SystemBinDir()}
+	if userDir, err := shim.UserBinDir(); err == nil {
+		dirs = append(dirs, userDir)
+	}
+	return dirs
+}
+
 func checkShimDirResolution(shimDir, pathLabel string, pathEntries []string) doctor.CheckResult {
 	underShim, shadowed := classifyPackageManagerResolutions(
 		alias.DefaultConfig().PackageManagers,
-		shimDir,
+		shimDirs(),
 		exec.LookPath,
 	)
 
@@ -362,18 +386,21 @@ func checkShimDirResolution(shimDir, pathLabel string, pathEntries []string) doc
 func checkShimInPathResult() doctor.CheckResult {
 	pathEntries := filepath.SplitList(os.Getenv("PATH"))
 
-	if shim.SystemShimsInstalled() {
-		return checkShimDirResolution(shim.SystemBinDir(), "System shim directory", pathEntries)
+	// The primary directory only picks the label and PATH-membership target;
+	// classification accepts resolution into either shim dir regardless.
+	shimDir, pathLabel := shim.SystemBinDir(), "System shim directory"
+	if !shim.SystemShimsInstalled() {
+		userDir, err := shim.UserBinDir()
+		if err != nil {
+			return doctor.CheckResult{
+				Status:  doctor.StatusWarn,
+				Message: fmt.Sprintf("Could not resolve shim directory: %v", err),
+			}
+		}
+		shimDir, pathLabel = userDir, "Shim directory"
 	}
 
-	userDir, err := shim.UserBinDir()
-	if err != nil {
-		return doctor.CheckResult{
-			Status:  doctor.StatusWarn,
-			Message: fmt.Sprintf("Could not resolve shim directory: %v", err),
-		}
-	}
-	return checkShimDirResolution(userDir, "Shim directory", pathEntries)
+	return checkShimDirResolution(shimDir, pathLabel, pathEntries)
 }
 
 func runProtectionChecks(coreResults []doctor.CheckResult) []doctor.CheckResult {

cmd/setup/doctor_system_test.go

@@ -73,9 +73,32 @@ func TestClassifyPackageManagerResolutions(t *testing.T) {
 
 	under, shadowed := classifyPackageManagerResolutions(
 		[]string{"npm", "pip", "uv"},
-		shimDir,
+		[]string{shimDir},
 		lookPath,
 	)
 	assert.Equal(t, []string{"npm"}, under)
 	assert.Equal(t, []string{"pip"}, shadowed)
 }
+
+func TestClassifyPackageManagerResolutionsAcceptsEitherShimDir(t *testing.T) {
+	systemDir := "/usr/local/lib/pmg/bin"
+	userDir := "/home/dev/.pmg/bin"
+	lookPath := func(name string) (string, error) {
+		switch name {
+		case "npm":
+			return systemDir + "/npm", nil
+		case "pip":
+			return userDir + "/pip", nil
+		default:
+			return "/usr/bin/" + name, nil
+		}
+	}
+
+	under, shadowed := classifyPackageManagerResolutions(
+		[]string{"npm", "pip", "yarn"},
+		[]string{systemDir, userDir},
+		lookPath,
+	)
+	assert.ElementsMatch(t, []string{"npm", "pip"}, under)
+	assert.Equal(t, []string{"yarn"}, shadowed)
+}

cmd/setup/setup.go

@@ -152,15 +152,21 @@ func remove(system, removeConfig bool) error {
 		return removeSystem(removeConfig)
 	}
 
+	// Best-effort: attempt every cleanup step so one failure does not strand the
+	// other artifacts and force a rerun.
+	var errs []error
 	if removeConfig {
 		// Only ever remove the per-user file; the globally managed
 		// config is not ours to delete from a per-user uninstall.
 		if err := config.RemoveUserConfigFile(); err != nil {
-			return err
+			errs = append(errs, err)
 		}
 	}
 
 	if runtime.GOOS == "windows" {
+		if len(errs) > 0 {
+			return errors.Join(errs...)
+		}
 		fmt.Printf("%s %s\n", ui.Colors.Green("✓"), "PMG config removed. No aliases or shims to clean up on Windows.")
 		return nil
 	}
@@ -168,21 +174,20 @@ func remove(system, removeConfig bool) error {
 	cfg := alias.DefaultConfig()
 	rcFileManager, err := alias.NewDefaultRcFileManager(cfg.RcFileName)
 	if err != nil {
-		return err
-	}
-
-	aliasManager := alias.New(cfg, rcFileManager)
-	if err := aliasManager.Remove(); err != nil {
-		return fmt.Errorf("failed to remove aliases: %w", err)
+		errs = append(errs, err)
+	} else if err := alias.New(cfg, rcFileManager).Remove(); err != nil {
+		errs = append(errs, fmt.Errorf("failed to remove aliases: %w", err))
 	}
 
 	shimMgr, err := shim.NewDefaultShimManager()
 	if err != nil {
-		return fmt.Errorf("failed to create shim manager: %w", err)
+		errs = append(errs, fmt.Errorf("failed to create shim manager: %w", err))
+	} else if err := shimMgr.Remove(); err != nil {
+		errs = append(errs, fmt.Errorf("failed to remove shims: %w", err))
 	}
 
-	if err := shimMgr.Remove(); err != nil {
-		return fmt.Errorf("failed to remove shims: %w", err)
+	if len(errs) > 0 {
+		return errors.Join(errs...)
 	}
 
 	fmt.Printf("%s %s\n", ui.Colors.Green("✓"), "PMG aliases and shims removed. Restart your terminal for changes to take effect")
@@ -199,14 +204,21 @@ func removeSystem(removeConfig bool) error {
 		return fmt.Errorf("failed to create system shim manager: %w", err)
 	}
 
+	// Best-effort: attempt config removal even if shim removal fails, so one
+	// failed step does not strand the other artifact and force a rerun. Shims
+	// are removed first so interception stops before the managed config goes.
+	var errs []error
 	if err := shimMgr.Remove(); err != nil {
-		return fmt.Errorf("failed to remove system shims: %w", err)
+		errs = append(errs, fmt.Errorf("failed to remove system shims: %w", err))
 	}
 	if removeConfig {
 		if err := config.RemoveSystemConfigFile(); err != nil {
-			return err
+			errs = append(errs, err)
 		}
 	}
+	if len(errs) > 0 {
+		return errors.Join(errs...)
+	}
 
 	fmt.Printf("%s %s\n", ui.Colors.Green("✓"), "PMG system install removed")
 	return nil

cmd/setup/setup_test.go

@@ -10,7 +10,7 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestErrIfSystemInstallAllowed(t *testing.T) {
+func TestRequireSystemInstallSupported(t *testing.T) {
 	orig := setupGeteuid
 	t.Cleanup(func() { setupGeteuid = orig })
 

docs/system-install.md

@@ -112,7 +112,7 @@ Shared policy lives under `/etc/safedep/pmg`. Runtime data stays per user:
 
 You can relocate these with `PMG_CONFIG_DIR` and `PMG_CACHE_DIR`.
 
-The invoking user must be able to write their config directory. If they cannot, PMG skips event logging for that run (and prints a warning) and continues the package-manager command. Cloud sync also needs that directory to store pending events.
+The invoking user must be able to write their config directory.
 
 In Docker images, avoid creating `/home/<user>/.config/safedep` as root during the build. Either fix ownership for the runtime user, or set `PMG_CONFIG_DIR` to a writable location.
 

internal/audit/cloud_sink.go

@@ -95,8 +95,7 @@ func (s *cloudSink) buildInvocationContext() *controltowerv1.EndpointInvocationC
 	ctx.SetCommand(s.command)
 	ctx.SetWorkingDirectory(s.workingDir)
 
-	u, err := user.Current()
-	if err == nil {
+	if u := invokingUser(); u != nil {
 		ctx.SetUsername(u.Username)
 		ctx.SetUsernameUid(u.Uid)
 	}
@@ -119,6 +118,21 @@ func (s *cloudSink) buildInvocationContext() *controltowerv1.EndpointInvocationC
 	return ctx
 }
 
+// invokingUser resolves the human behind the command, preferring SUDO_USER so a
+// `sudo npm ...` is attributed to the operator rather than root.
+func invokingUser() *user.User {
+	if name := os.Getenv("SUDO_USER"); name != "" {
+		if u, err := user.Lookup(name); err == nil {
+			return u
+		}
+	}
+	u, err := user.Current()
+	if err != nil {
+		return nil
+	}
+	return u
+}
+
 func buildCommand(packageManager string, args []string) string {
 	if packageManager == "" {
 		return ""

internal/shim/shim.go

@@ -1,6 +1,7 @@
 package shim
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -99,25 +100,26 @@ func (m *ShimManager) Install() error {
 }
 
 func (m *ShimManager) Remove() error {
+	// Best-effort: a failure removing the shim directory must not skip profile
+	// and rc cleanup, otherwise a rerun is needed to fully uninstall.
+	var errs []error
 	if err := os.RemoveAll(m.config.BinDir); err != nil {
-		return fmt.Errorf("failed to remove shim directory %s: %w", m.config.BinDir, err)
+		errs = append(errs, fmt.Errorf("failed to remove shim directory %s: %w", m.config.BinDir, err))
 	}
 
 	if m.config.ManageProfile {
 		if err := removeSystemProfile(); err != nil {
-			return fmt.Errorf("failed to remove system profile: %w", err)
+			errs = append(errs, fmt.Errorf("failed to remove system profile: %w", err))
 		}
 	}
 
-	if m.config.SkipShellRc {
-		return nil
+	if !m.config.SkipShellRc {
+		if err := m.removePathFromShells(); err != nil {
+			errs = append(errs, fmt.Errorf("failed to clean shell configs: %w", err))
+		}
 	}
 
-	if err := m.removePathFromShells(); err != nil {
-		return fmt.Errorf("failed to clean shell configs: %w", err)
-	}
-
-	return nil
+	return errors.Join(errs...)
 }
 
 func (m *ShimManager) IsInstalled() (bool, error) {

internal/shim/system.go

@@ -23,6 +23,10 @@ var (
 	// systemExecutableOwnershipCheck requires root ownership of the binary and
 	// its parent directory. Disabled in tests that cannot create root-owned files.
 	systemExecutableOwnershipCheck = true
+	// resolveExecutable resolves the running pmg binary for system install.
+	// Overridable in tests so validation does not run against the go-build test
+	// binary, which is group-writable under a 002 umask.
+	resolveExecutable = currentExecutable
 )
 
 // SystemBinDir returns the directory for system-wide PMG shims.
@@ -57,7 +61,7 @@ func NewSystemShimManagerForRemove() (*ShimManager, error) {
 
 func newSystemShimManager(validateExecutable bool) (*ShimManager, error) {
 	aliasCfg := alias.DefaultConfig()
-	pmgBin, err := currentExecutable()
+	pmgBin, err := resolveExecutable()
 	if err != nil {
 		return nil, err
 	}
@@ -81,8 +85,8 @@ func newSystemShimManager(validateExecutable bool) (*ShimManager, error) {
 
 // validateSystemExecutable rejects binaries unsafe for system-wide shims.
 // Shims hard-code this path, so the binary must be executable by all users,
-// not writable by group/others, and owned by root in a root-owned parent
-// directory that is not writable by group/others.
+// not writable by group/others, and owned by root in a root-owned, non-world-
+// writable parent.
 func validateSystemExecutable(path string) error {
 	info, err := os.Stat(path)
 	if err != nil {
@@ -126,16 +130,24 @@ func requireRootOwnedPath(path string, info os.FileInfo) error {
 	return nil
 }
 
+// requireSafeParentDir validates only the immediate parent of the executable,
+// not the full chain up to /. It requires a root-owned, non-world-writable
+// parent so an unprivileged account cannot swap the shared binary that every
+// user's shims exec; a maliciously writable grandparent is out of scope.
+//
+// Group-writable is allowed deliberately: Debian/Ubuntu ship /usr/local/bin as
+// root:staff mode 2775, so rejecting group-writable would refuse the documented
+// install location out of the box. The tradeoff is that a member of the parent
+// directory's group can replace the binary — harden the directory (chmod g-w)
+// on multi-user hosts where that group is not trusted.
 func requireSafeParentDir(dir string) error {
 	info, err := os.Stat(dir)
 	if err != nil {
 		return fmt.Errorf("failed to inspect directory %s: %w", dir, err)
 	}
 
-	groupOrOtherWrite := os.FileMode(0o022)
-
-	if info.Mode().Perm()&groupOrOtherWrite != 0 {
-		return fmt.Errorf("directory %s containing pmg executable is writable by group or others", dir)
+	if info.Mode().Perm()&os.FileMode(0o002) != 0 {
+		return fmt.Errorf("directory %s containing pmg executable is writable by others", dir)
 	}
 
 	uid, ok := fileOwnerUID(info)

internal/shim/system_test.go

@@ -14,10 +14,19 @@ func useSystemPaths(t *testing.T, dir string) {
 	systemBinDirOverride = filepath.Join(dir, "bin")
 	systemProfilePathOverride = filepath.Join(dir, "profile.d", "pmg.sh")
 	systemExecutableOwnershipCheck = false
+
+	// The go-build test binary is group-writable under a 002 umask, which the
+	// executable validation rightly rejects. Point resolution at a crafted
+	// 0755 binary so the manager validates a realistic path, not the harness.
+	exe := filepath.Join(dir, "pmg")
+	require.NoError(t, os.WriteFile(exe, []byte("#!/bin/sh\n"), 0o755))
+	resolveExecutable = func() (string, error) { return exe, nil }
+
 	t.Cleanup(func() {
 		systemBinDirOverride = ""
 		systemProfilePathOverride = ""
 		systemExecutableOwnershipCheck = true
+		resolveExecutable = currentExecutable
 	})
 }
 
@@ -142,6 +151,10 @@ func TestValidateSystemExecutableRejectsGroupWritable(t *testing.T) {
 }
 
 func TestValidateSystemExecutableRejectsNonRootOwner(t *testing.T) {
+	if os.Geteuid() == 0 {
+		t.Skip("running as root: temp file is root-owned, so the owner check passes")
+	}
+
 	dir := t.TempDir()
 	path := filepath.Join(dir, "pmg")
 	require.NoError(t, os.WriteFile(path, []byte("binary"), 0o755))