fix: Suppress non-PMG error from Critical UI Error (#311)

* fix: Suppress non-PMG error from Critical UI Error

* fix: Code review fixes

---------

Co-authored-by: Sahil Bansal <bansalsahil315@gmail.com>

internal/flows/proxy_flow.go

@@ -4,7 +4,6 @@ import (
 	"context"
 	"fmt"
 	"os"
-	"os/exec"
 	"path/filepath"
 	"time"
 
@@ -290,16 +289,16 @@ func (f *proxyFlow) Run(ctx context.Context, args []string, parsedCmd *packagema
 	return handleExecutionResultError(executionError)
 }
 
-// handleExecutionResultError handles the error from the execution result.
+// handleExecutionResultError returns the execution error so RunE can route it
+// through ui.ExitFromCommandError, the single exit point. A transparent
+// *runner.ChildExitError survives the %w wrap (errors.As unwraps it) and is
+// passed through with the child's exit code; everything else keeps the visible
+// PMG error framing.
 func handleExecutionResultError(err error) error {
 	if err == nil {
 		return nil
 	}
 
-	if exitErr, ok := err.(*exec.ExitError); ok {
-		os.Exit(exitErr.ExitCode())
-	}
-
 	return fmt.Errorf("failed to execute command: %w", err)
 }
 

internal/proc/signal_other.go

@@ -0,0 +1,10 @@
+//go:build windows
+
+package proc
+
+// SignalInfo on non-unix platforms cannot decode signal termination from the
+// process status, so it always reports not-signaled and the raw exit code is
+// used verbatim by callers.
+func SignalInfo(_ any) (signum int, signaled bool) {
+	return 0, false
+}

internal/proc/signal_unix.go

@@ -0,0 +1,19 @@
+//go:build !windows
+
+// Package proc decodes process termination status. It is a leaf utility shared
+// by the execution layer (runner) and the PTY session, so neither has to
+// reach through wrapper layers to learn how a child process ended.
+package proc
+
+import "syscall"
+
+// SignalInfo reports whether a process was terminated by a signal, given the
+// platform-specific status from (*exec.ExitError).Sys() / (*ptyx.ExitError).Sys().
+// signum is the signal number; callers form the conventional exit code as 128+signum.
+func SignalInfo(sys any) (signum int, signaled bool) {
+	ws, ok := sys.(syscall.WaitStatus)
+	if !ok || !ws.Signaled() {
+		return 0, false
+	}
+	return int(ws.Signal()), true
+}

internal/proc/signal_unix_test.go

@@ -0,0 +1,58 @@
+//go:build !windows
+
+package proc
+
+import (
+	"syscall"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestSignalInfo(t *testing.T) {
+	tests := []struct {
+		name         string
+		sys          any
+		wantSignum   int
+		wantSignaled bool
+	}{
+		{
+			name:         "SIGINT terminated process",
+			sys:          syscall.WaitStatus(int(syscall.SIGINT)),
+			wantSignum:   int(syscall.SIGINT),
+			wantSignaled: true,
+		},
+		{
+			name:         "SIGTERM terminated process",
+			sys:          syscall.WaitStatus(int(syscall.SIGTERM)),
+			wantSignum:   int(syscall.SIGTERM),
+			wantSignaled: true,
+		},
+		{
+			name:         "normal exit code 2 is not signaled",
+			sys:          syscall.WaitStatus(2 << 8),
+			wantSignum:   0,
+			wantSignaled: false,
+		},
+		{
+			name:         "nil status is not signaled",
+			sys:          nil,
+			wantSignum:   0,
+			wantSignaled: false,
+		},
+		{
+			name:         "non-WaitStatus value is not signaled",
+			sys:          "not a wait status",
+			wantSignum:   0,
+			wantSignaled: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			signum, signaled := SignalInfo(tt.sys)
+			assert.Equal(t, tt.wantSignaled, signaled)
+			assert.Equal(t, tt.wantSignum, signum)
+		})
+	}
+}

internal/pty/exiterror_test.go

@@ -0,0 +1,34 @@
+//go:build !windows
+
+package pty
+
+import (
+	"errors"
+	"syscall"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestNewExitError(t *testing.T) {
+	underlying := errors.New("boom")
+
+	t.Run("normal non-zero exit carries the raw code", func(t *testing.T) {
+		e := newExitError(2, syscall.WaitStatus(2<<8), underlying)
+		assert.Equal(t, 2, e.Code)
+		assert.False(t, e.Signaled)
+		assert.ErrorIs(t, e, underlying)
+	})
+
+	t.Run("signal termination resolves to 128+signum and marks signaled", func(t *testing.T) {
+		e := newExitError(-1, syscall.WaitStatus(int(syscall.SIGINT)), underlying)
+		assert.Equal(t, 128+int(syscall.SIGINT), e.Code)
+		assert.True(t, e.Signaled)
+	})
+
+	t.Run("no wait status falls back to the provided code", func(t *testing.T) {
+		e := newExitError(-1, nil, underlying)
+		assert.Equal(t, -1, e.Code)
+		assert.False(t, e.Signaled)
+	})
+}

internal/pty/session.go

@@ -8,6 +8,7 @@ import (
 	"strings"
 
 	"github.com/safedep/dry/log"
+	"github.com/safedep/pmg/internal/proc"
 	"github.com/safedep/ptyx"
 	"golang.org/x/term"
 )
@@ -152,9 +153,9 @@ func (s *session) Wait() error {
 	err := s.spawn.Wait()
 	if err != nil {
 		if exitErr, ok := err.(*ptyx.ExitError); ok {
-			return &ExitError{Code: exitErr.ExitCode, Err: err}
+			return newExitError(exitErr.ExitCode, exitErr.Sys(), err)
 		}
-		return &ExitError{Code: -1, Err: err}
+		return newExitError(-1, nil, err)
 	}
 	return nil
 }
@@ -178,8 +179,19 @@ func (s *session) Close() error {
 
 // ExitError is returned when the child process exits with non-zero code
 type ExitError struct {
-	Code int
-	Err  error // Underlying error from ptyx
+	Code     int
+	Signaled bool  // true if the child was terminated by a signal (Ctrl+C, SIGTERM, …)
+	Err      error // Underlying error from ptyx
+}
+
+// newExitError resolves the child's termination into an ExitError. When the
+// process was signal-terminated, Code is the conventional 128+signum and
+// Signaled is set; otherwise the raw exit code is preserved.
+func newExitError(code int, sys any, err error) *ExitError {
+	if signum, signaled := proc.SignalInfo(sys); signaled {
+		return &ExitError{Code: 128 + signum, Signaled: true, Err: err}
+	}
+	return &ExitError{Code: code, Err: err}
 }
 
 func (e *ExitError) Error() string {

internal/runner/childexit.go

@@ -0,0 +1,81 @@
+package runner
+
+import (
+	"errors"
+	"fmt"
+	"os/exec"
+
+	"github.com/safedep/dry/usefulerror"
+	"github.com/safedep/pmg/errcodes"
+	"github.com/safedep/pmg/internal/proc"
+	"github.com/safedep/pmg/internal/pty"
+)
+
+// ChildExitError marks a transparent passthrough: the wrapped package manager
+// exited on its own. PMG attributes nothing to itself and mirrors the exit code,
+// so the package manager's own output stands and PMG stays invisible.
+type ChildExitError struct {
+	Code     int    // child's exit code (or 128+signum for signal termination)
+	Signaled bool   // true if terminated by a signal (Ctrl+C, SIGTERM, …)
+	PMName   string // package manager name, for the dim one-liner
+}
+
+func (e *ChildExitError) Error() string {
+	return fmt.Sprintf("%s exited with code %d", e.PMName, e.Code)
+}
+func (e *ChildExitError) ExitCode() int     { return e.Code }
+func (e *ChildExitError) Transparent() bool { return true }
+func (e *ChildExitError) IsSignaled() bool  { return e.Signaled }
+
+// classify turns a package-manager execution error into either a transparent
+// child exit or a visible PMG error, and is the only place the fork lives. It is
+// pure: sandbox denials are persisted separately by the caller and never make a
+// child's own non-zero exit loud — a restrictive policy routinely denies benign
+// operations and causation cannot be inferred from a denial. Only a failure on
+// PMG's side of the boundary (the tool never produced an exit status) is loud.
+func classify(err error, pmName string) error {
+	if err == nil {
+		return nil
+	}
+
+	code, signaled, resolved := extractExit(err)
+	return decideExit(err, code, signaled, resolved, pmName)
+}
+
+// extractExit pulls the exit code and signal status from a process error.
+// resolved is false when the child never produced a real exit status (e.g. the
+// binary failed to launch), which must surface as a visible PMG error.
+func extractExit(err error) (code int, signaled bool, resolved bool) {
+	var exitErr *exec.ExitError
+	if errors.As(err, &exitErr) {
+		if signum, sig := proc.SignalInfo(exitErr.Sys()); sig {
+			return 128 + signum, true, true
+		}
+		code = exitErr.ExitCode()
+		return code, false, code >= 0
+	}
+
+	var ptyErr *pty.ExitError
+	if errors.As(err, &ptyErr) {
+		return ptyErr.Code, ptyErr.Signaled, ptyErr.Signaled || ptyErr.Code >= 0
+	}
+
+	return -1, false, false
+}
+
+func decideExit(err error, code int, signaled, resolved bool, pmName string) error {
+	if !resolved {
+		return visibleExecError(err)
+	}
+	return &ChildExitError{Code: code, Signaled: signaled, PMName: pmName}
+}
+
+// visibleExecError is the loud error for a genuine PMG-side failure: the package
+// manager never produced an exit status (e.g. the binary could not be launched).
+func visibleExecError(err error) error {
+	return usefulerror.NewUsefulError().
+		WithCode(errcodes.PackageManagerExecutionFailed).
+		WithHumanError("Failed to execute package manager command").
+		WithHelp("Check the package manager command and its arguments").
+		Wrap(err)
+}

internal/runner/childexit_test.go

@@ -0,0 +1,103 @@
+package runner
+
+import (
+	"errors"
+	"os/exec"
+	"testing"
+
+	"github.com/safedep/dry/usefulerror"
+	"github.com/safedep/pmg/errcodes"
+	"github.com/safedep/pmg/internal/pty"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestExtractExit(t *testing.T) {
+	t.Run("direct non-zero exit resolves the real code", func(t *testing.T) {
+		err := exec.Command("sh", "-c", "exit 2").Run()
+		require.Error(t, err)
+
+		code, signaled, resolved := extractExit(err)
+		assert.Equal(t, 2, code)
+		assert.False(t, signaled)
+		assert.True(t, resolved)
+	})
+
+	t.Run("direct signal termination resolves to 128+signum", func(t *testing.T) {
+		err := exec.Command("sh", "-c", "kill -INT $$").Run()
+		require.Error(t, err)
+
+		code, signaled, resolved := extractExit(err)
+		assert.Equal(t, 130, code) // 128 + SIGINT(2)
+		assert.True(t, signaled)
+		assert.True(t, resolved)
+	})
+
+	t.Run("pty exit error reads its fields directly", func(t *testing.T) {
+		code, signaled, resolved := extractExit(&pty.ExitError{Code: 1})
+		assert.Equal(t, 1, code)
+		assert.False(t, signaled)
+		assert.True(t, resolved)
+
+		code, signaled, resolved = extractExit(&pty.ExitError{Code: 143, Signaled: true})
+		assert.Equal(t, 143, code)
+		assert.True(t, signaled)
+		assert.True(t, resolved)
+	})
+
+	t.Run("non-exit error is unresolved", func(t *testing.T) {
+		_, _, resolved := extractExit(errors.New("failed to launch binary"))
+		assert.False(t, resolved)
+	})
+}
+
+func TestDecideExit(t *testing.T) {
+	runErr := errors.New("npm failed")
+
+	t.Run("plain child exit becomes a transparent ChildExitError", func(t *testing.T) {
+		err := decideExit(runErr, 1, false, true, "npm")
+
+		var ce *ChildExitError
+		require.True(t, errors.As(err, &ce))
+		assert.Equal(t, 1, ce.ExitCode())
+		assert.True(t, ce.Transparent())
+		assert.False(t, ce.IsSignaled())
+		assert.Equal(t, "npm", ce.PMName)
+	})
+
+	t.Run("signaled child exit is transparent and signaled", func(t *testing.T) {
+		err := decideExit(runErr, 130, true, true, "npm")
+
+		var ce *ChildExitError
+		require.True(t, errors.As(err, &ce))
+		assert.True(t, ce.IsSignaled())
+		assert.Equal(t, 130, ce.ExitCode())
+	})
+
+	t.Run("unresolved exit is a visible launch failure", func(t *testing.T) {
+		err := decideExit(runErr, -1, false, false, "npm")
+
+		usefulErr, ok := usefulerror.AsUsefulError(err)
+		require.True(t, ok)
+		assert.Equal(t, errcodes.PackageManagerExecutionFailed, usefulErr.Code())
+		assert.Equal(t, "Failed to execute package manager command", usefulErr.HumanError())
+
+		var ce *ChildExitError
+		assert.False(t, errors.As(err, &ce))
+	})
+}
+
+// classify is pure: a child that produced its own exit status is always a
+// transparent passthrough. Sandbox denials are persisted by the caller and
+// never reach classify, so they cannot make the exit loud (issue #309).
+func TestClassifyTransparentChildExit(t *testing.T) {
+	childErr := exec.Command("sh", "-c", "exit 1").Run()
+	require.Error(t, childErr)
+
+	err := classify(childErr, "npm")
+
+	var ce *ChildExitError
+	require.True(t, errors.As(err, &ce))
+	assert.Equal(t, 1, ce.ExitCode())
+	assert.Equal(t, "npm", ce.PMName)
+}

internal/runner/execute.go

@@ -115,13 +115,13 @@ func ExecuteWithOptions(ctx context.Context, pc *packagemanager.ParsedCommand, o
 
 	switch mode {
 	case ExecutionModePTY:
-		return runPTY(ctx, cmd, cmd.Env, result, opts.PreparePTYSession)
+		return runPTY(ctx, cmd, cmd.Env, result, opts.PackageManagerName, opts.PreparePTYSession)
 	default:
-		return runDirect(cmd, result)
+		return runDirect(cmd, result, opts.PackageManagerName)
 	}
 }
 
-func runDirect(cmd *exec.Cmd, result *sandbox.ExecutionResult) error {
+func runDirect(cmd *exec.Cmd, result *sandbox.ExecutionResult, pmName string) error {
 	if !result.ShouldRun() {
 		return nil
 	}
@@ -129,7 +129,8 @@ func runDirect(cmd *exec.Cmd, result *sandbox.ExecutionResult) error {
 	log.Debugf("Running command with args: %s: %v", cmd.Path, cmd.Args[1:])
 
 	if err := cmd.Run(); err != nil {
-		return wrapCommandExecutionError(err, result)
+		executor.ObserveViolations(result, err)
+		return classify(err, pmName)
 	}
 
 	log.Debugf("Command completed successfully")
@@ -141,6 +142,7 @@ func runPTY(
 	cmd *exec.Cmd,
 	env []string,
 	result *sandbox.ExecutionResult,
+	pmName string,
 	beforeWait func(*PTYRuntime) error,
 ) error {
 	if !result.ShouldRun() {
@@ -241,7 +243,8 @@ func runPTY(
 	}
 
 	if sessionError != nil {
-		return wrapCommandExecutionError(sessionError, result)
+		executor.ObserveViolations(result, sessionError)
+		return classify(sessionError, pmName)
 	}
 
 	return nil
@@ -309,15 +312,3 @@ func mergeEnv(base, overrides []string) []string {
 
 	return env
 }
-
-func wrapCommandExecutionError(err error, result *sandbox.ExecutionResult) error {
-	if exitErr, ok := err.(*exec.ExitError); ok {
-		return executor.WrapCommandExecutionError(err, result, exitErr.ExitCode())
-	}
-
-	if sessionError, ok := err.(*pty.ExitError); ok {
-		return executor.WrapCommandExecutionError(sessionError, result, sessionError.Code)
-	}
-
-	return executor.WrapCommandExecutionError(err, result, -1)
-}

internal/ui/error.go

@@ -37,29 +37,28 @@ func ErrorExitWithCode(err error, code int) {
 	os.Exit(code)
 }
 
-// printMinimalError prints error in minimal two-line format:
+// Error output goes to stderr so the wrapped package manager's stdout passes
+// through clean (e.g. `pmg npm view --json` stays parseable).
 func printMinimalError(code, message, hint string) {
-	fmt.Printf("%s  %s\n", Colors.ErrorCode(" %s ", code), Colors.Red(message))
+	fmt.Fprintf(os.Stderr, "%s  %s\n", Colors.ErrorCode(" %s ", code), Colors.Red(message))
 
 	if hint != "" && hint != "No additional help is available for this error." {
-		fmt.Printf(" %s %s\n", Colors.Dim("→"), Colors.Dim(hint))
+		fmt.Fprintf(os.Stderr, " %s %s\n", Colors.Dim("→"), Colors.Dim(hint))
 	}
 }
 
-// printVerboseError prints detailed error for debugging (--verbose mode)
-// Includes additional help and original error chain for troubleshooting
 func printVerboseError(code, message, hint, additionalHelp, originalError string) {
-	fmt.Printf("%s  %s\n", Colors.ErrorCode(" %s ", code), Colors.Red(message))
+	fmt.Fprintf(os.Stderr, "%s  %s\n", Colors.ErrorCode(" %s ", code), Colors.Red(message))
 
 	if hint != "" && hint != "No additional help is available for this error." {
-		fmt.Printf(" %s %s\n", Colors.Dim("→"), Colors.Dim(hint))
+		fmt.Fprintf(os.Stderr, " %s %s\n", Colors.Dim("→"), Colors.Dim(hint))
 	}
 
 	if additionalHelp != "" && additionalHelp != "No additional help is available for this error." {
-		fmt.Printf(" %s %s\n", Colors.Dim("→"), Colors.Dim(additionalHelp))
+		fmt.Fprintf(os.Stderr, " %s %s\n", Colors.Dim("→"), Colors.Dim(additionalHelp))
 	}
 
 	if originalError != "" && originalError != message {
-		fmt.Printf(" %s %s\n", Colors.Dim("┄"), Colors.Dim(originalError))
+		fmt.Fprintf(os.Stderr, " %s %s\n", Colors.Dim("┄"), Colors.Dim(originalError))
 	}
 }

internal/ui/exit.go

@@ -0,0 +1,58 @@
+package ui
+
+import (
+	"errors"
+	"fmt"
+	"os"
+)
+
+// transparentExit is satisfied by *runner.ChildExitError without importing it.
+type transparentExit interface {
+	error
+	Transparent() bool
+	ExitCode() int
+	IsSignaled() bool
+}
+
+type exitDecision struct {
+	transparent bool
+	notice      bool
+	code        int
+	message     string
+}
+
+// classifyExit is the pure decision behind ExitFromCommandError. The notice is
+// suppressed for signal exits (the user initiated the interrupt) and in silent
+// mode.
+func classifyExit(err error) exitDecision {
+	var te transparentExit
+	if !errors.As(err, &te) || !te.Transparent() {
+		return exitDecision{}
+	}
+
+	d := exitDecision{transparent: true, code: te.ExitCode()}
+	if !te.IsSignaled() && verbosityLevel != VerbosityLevelSilent {
+		d.notice = true
+		d.message = "↳ pmg: " + te.Error()
+	}
+	return d
+}
+
+// ExitFromCommandError is the single exit point for package-manager commands. A
+// child that exited on its own is mirrored transparently; everything else keeps
+// the visible PMG error framing.
+func ExitFromCommandError(err error) {
+	if err == nil {
+		return
+	}
+
+	if d := classifyExit(err); d.transparent {
+		ClearStatus()
+		if d.notice {
+			fmt.Fprintln(os.Stderr, Colors.Dim(d.message))
+		}
+		os.Exit(d.code)
+	}
+
+	ErrorExit(err)
+}

internal/ui/exit_test.go

@@ -0,0 +1,81 @@
+package ui
+
+import (
+	"errors"
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+// fakeChildExit satisfies the transparentExit interface, standing in for
+// *runner.ChildExitError without a cross-package import.
+type fakeChildExit struct {
+	code     int
+	signaled bool
+	pmName   string
+}
+
+func (e *fakeChildExit) Error() string    { return fmt.Sprintf("%s exited with code %d", e.pmName, e.code) }
+func (e *fakeChildExit) ExitCode() int    { return e.code }
+func (e *fakeChildExit) Transparent() bool { return true }
+func (e *fakeChildExit) IsSignaled() bool { return e.signaled }
+
+func withVerbosity(t *testing.T, level VerbosityLevel) {
+	t.Helper()
+	prev := verbosityLevel
+	verbosityLevel = level
+	t.Cleanup(func() { verbosityLevel = prev })
+}
+
+func TestClassifyExit(t *testing.T) {
+	t.Run("genuine non-zero child exit prints a dim notice and mirrors the code", func(t *testing.T) {
+		withVerbosity(t, VerbosityLevelNormal)
+
+		d := classifyExit(&fakeChildExit{code: 1, pmName: "npm"})
+
+		assert.True(t, d.transparent)
+		assert.Equal(t, 1, d.code)
+		assert.True(t, d.notice)
+		assert.Equal(t, "↳ pmg: npm exited with code 1", d.message)
+	})
+
+	t.Run("transparent exit is detected even when wrapped", func(t *testing.T) {
+		withVerbosity(t, VerbosityLevelNormal)
+
+		wrapped := fmt.Errorf("failed to execute command: %w", &fakeChildExit{code: 2, pmName: "pnpm"})
+		d := classifyExit(wrapped)
+
+		assert.True(t, d.transparent)
+		assert.Equal(t, 2, d.code)
+		assert.True(t, d.notice)
+	})
+
+	t.Run("signal termination is silent but still mirrors the code", func(t *testing.T) {
+		withVerbosity(t, VerbosityLevelNormal)
+
+		d := classifyExit(&fakeChildExit{code: 130, signaled: true, pmName: "npm"})
+
+		assert.True(t, d.transparent)
+		assert.Equal(t, 130, d.code)
+		assert.False(t, d.notice)
+		assert.Empty(t, d.message)
+	})
+
+	t.Run("silent mode suppresses the notice", func(t *testing.T) {
+		withVerbosity(t, VerbosityLevelSilent)
+
+		d := classifyExit(&fakeChildExit{code: 1, pmName: "npm"})
+
+		assert.True(t, d.transparent)
+		assert.Equal(t, 1, d.code)
+		assert.False(t, d.notice)
+	})
+
+	t.Run("non-transparent error falls through to the loud path", func(t *testing.T) {
+		d := classifyExit(errors.New("some PMG failure"))
+
+		assert.False(t, d.transparent)
+		assert.False(t, d.notice)
+	})
+}

internal/ui/ui.go

@@ -184,7 +184,7 @@ func PromptSecret(label string) (string, error) {
 func Fatalf(msg string, args ...interface{}) {
 	ClearStatus()
 
-	fmt.Println(Colors.Red(fmt.Sprintf(msg, args...)))
+	fmt.Fprintln(os.Stderr, Colors.Red(fmt.Sprintf(msg, args...)))
 	os.Exit(1)
 }