From 57b93fcc1bbb4ecfed517de68f1554c1a7fe1c0e Mon Sep 17 00:00:00 2001 From: deveshctl Date: Thu, 6 Aug 2026 13:22:34 +0530 Subject: [PATCH] fix: show friendly error messages on more failure paths Several failure paths surfaced raw internal error strings to the user instead of the actionable messages the rest of the tool already produced. - Interactive `layerx `: engine/resolver selection failures (no engine reachable, Podman connection unconfigured or malformed) are produced before the TUI starts and were returned bare, so the user saw the raw Error() string with no recovery hint. Route them through the same friendly presenter the --json path already uses, and silence cobra's default printer so nothing double-prints. The error is returned unchanged so the process exit code is unaffected. - Interactive viewer: the archive "could not ..." message unconditionally told the user to free disk space / set TMPDIR, even for seek and I/O failures (e.g. on a network mount) that have nothing to do with a full disk. Gate the hint on the disk-full cause, matching the CLI. - JSON export: a full disk leaked the internal temporary spool file path into the user-facing error. Rewrite the disk-full case to name the output path the user supplied instead. Also document why unrecognised and usage errors exit 2 in main.go, so the distinction from rule/build failures is not accidentally collapsed later. --- CHANGELOG.md | 15 +++++++++++++++ cmd/json.go | 28 ++++++++++++++++++++++------ cmd/json_test.go | 20 ++++++++++++++++++++ cmd/root.go | 19 +++++++++++++++---- main.go | 4 ++++ tui/model.go | 10 +++++++++- tui/model_test.go | 20 ++++++++++++++++++++ 7 files changed, 105 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fb12da4..e0a0cd5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,21 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 identical structs on every redraw are eliminated. Reduces GC pressure noticeably on low-power machines and slow terminals. +### Fixed +- Running `layerx ` with no reachable container engine (or with a + Podman connection that is unconfigured, missing, or malformed) now prints the + same friendly, actionable message the other commands already showed — + previously the interactive command printed the raw internal error string with + no recovery hint. +- The archive "could not …" error in the interactive viewer no longer suggests + freeing disk space or setting `TMPDIR` for failures that are not disk-full + (for example a seek or I/O error on a network mount), matching the + command-line behaviour. The disk-space hint now appears only when the + underlying cause is a full disk. +- JSON export now reports a clean "no space left to write <path>" message on a + full disk instead of leaking the internal temporary spool file path into the + error shown to the user. + ## [v1.6.0] - 2026-07-28 Eight built-in colour themes, transparent-background mode, and TUI visual diff --git a/cmd/json.go b/cmd/json.go index 9099bc6..8461226 100644 --- a/cmd/json.go +++ b/cmd/json.go @@ -3,9 +3,11 @@ package cmd import ( "context" "encoding/json" + "errors" "fmt" "os" "path/filepath" + "syscall" "github.com/deveshctl/layerx/image" ) @@ -95,7 +97,10 @@ func runJSONExportFromAnalysis(analysis *image.Analysis, outputPath string) erro } if err := writeJSONAtomic(outputPath, data); err != nil { - return fmt.Errorf("writing %s: %w", outputPath, err) + // writeJSONAtomic already names outputPath in its disk-full message; + // wrap the generic path with %w so callers can still errors.Is/As it + // without prefixing a second, redundant "writing :". + return fmt.Errorf("could not write JSON: %w", err) } fmt.Fprintf(os.Stderr, "layerx: wrote analysis to %s\n", outputPath) @@ -174,26 +179,37 @@ func writeJSONAtomic(targetPath string, data []byte) error { dir := filepath.Dir(targetPath) f, err := os.CreateTemp(dir, ".layerx-json-*.tmp") if err != nil { - return err + return cleanDiskWriteErr(targetPath, err) } tmp := f.Name() if _, err := f.Write(data); err != nil { f.Close() os.Remove(tmp) - return err + return cleanDiskWriteErr(targetPath, err) } if err := f.Sync(); err != nil { f.Close() os.Remove(tmp) - return err + return cleanDiskWriteErr(targetPath, err) } if err := f.Close(); err != nil { os.Remove(tmp) - return err + return cleanDiskWriteErr(targetPath, err) } if err := os.Rename(tmp, targetPath); err != nil { os.Remove(tmp) - return err + return cleanDiskWriteErr(targetPath, err) } return nil } + +// cleanDiskWriteErr rewrites a disk-full failure so the user-facing message +// names the output path they supplied instead of the internal .tmp spool file +// (which the raw syscall error carries). Non-ENOSPC errors pass through +// unchanged — their paths are already the target or a directory the user chose. +func cleanDiskWriteErr(targetPath string, err error) error { + if errors.Is(err, syscall.ENOSPC) { + return fmt.Errorf("no space left to write %s (free up disk space and try again)", targetPath) + } + return err +} diff --git a/cmd/json_test.go b/cmd/json_test.go index 1279c5c..23d0719 100644 --- a/cmd/json_test.go +++ b/cmd/json_test.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "strings" + "syscall" "testing" "github.com/deveshctl/layerx/image" @@ -308,3 +309,22 @@ func TestRunJSONExport_ContextCancelled(t *testing.T) { _, statErr := os.Stat(outPath) assert.True(t, os.IsNotExist(statErr), "no output file must exist when resolve was cancelled (statErr = %v)", statErr) } + +func TestCleanDiskWriteErr_ENOSPCNamesTargetNotTmp(t *testing.T) { + // The raw write error carries the internal .tmp spool path; on ENOSPC the + // user-facing message must name the output path they supplied instead. + target := "/home/user/out.json" + raw := &os.PathError{Op: "write", Path: "/home/user/.layerx-json-123.tmp", Err: syscall.ENOSPC} + got := cleanDiskWriteErr(target, raw) + assert.Contains(t, got.Error(), target) + assert.NotContains(t, got.Error(), ".tmp") + assert.Contains(t, got.Error(), "no space left") +} + +func TestCleanDiskWriteErr_NonENOSPCPassesThrough(t *testing.T) { + // Non-disk-full errors are returned unchanged — their paths are the target + // or a directory the user chose, so there is nothing internal to hide. + raw := errors.New("permission denied") + got := cleanDiskWriteErr("/home/user/out.json", raw) + assert.Same(t, raw, got) +} diff --git a/cmd/root.go b/cmd/root.go index d3a0c62..6d28f95 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -256,24 +256,35 @@ func runInspect(cmd *cobra.Command, args []string) error { return runJSONExport(cmd.Context(), imageRef, flagJSON, noCache) } + // selectResolver produces the typed engine/resolver errors (no engine + // found, Podman connection not configured, malformed connection config) + // synchronously, before the TUI starts. Route them through presentCLIError + // so the user sees the friendly, actionable message rather than the raw + // Error() string cobra would otherwise print — and silence cobra's own + // printer so it does not double-print. Mirrors the --json branch above. + cmd.SilenceErrors = true + resolver, err := selectResolver(imageRef) if err != nil { - return err + return presentCLIError(os.Stderr, err) } theme, err := resolveTheme(cfg.Theme, flagTheme) if err != nil { - return err + return presentCLIError(os.Stderr, err) } - return tui.Run(tui.Config{ + if err := tui.Run(tui.Config{ ImageRef: imageRef, Resolver: resolver, NoCache: noCache, Platform: activePlatformDisplay(), Theme: theme, TransparentBg: cfg.TransparentBackground, - }) + }); err != nil { + return presentCLIError(os.Stderr, err) + } + return nil } // warnCIThresholdFlagsIgnored prints a warning to w when CI=true is active diff --git a/main.go b/main.go index 4ab68ee..fa3db94 100644 --- a/main.go +++ b/main.go @@ -47,6 +47,10 @@ func main() { // `layerx build` exactly like `docker build` / `podman build`. os.Exit(e.ExitCode) } + // Exit 2 covers ErrCIUsage, ErrCompareUsage, and any unrecognised error. + // Usage errors are deliberately distinct from rule failures (ErrCIFailed → + // 1) and build failures (ErrBuildFailed → engine code): do NOT remap + // ErrCIUsage/ErrCompareUsage to exit 1. os.Exit(2) } diff --git a/tui/model.go b/tui/model.go index 5ad0b2f..c2650f5 100644 --- a/tui/model.go +++ b/tui/model.go @@ -8,6 +8,7 @@ import ( "os" "path/filepath" "strings" + "syscall" "time" tea "charm.land/bubbletea/v2" @@ -2438,7 +2439,14 @@ func friendlyError(err error) string { return fmt.Sprintf("Not a valid image archive: %q. Expected a docker-save or OCI layout tarball.", invalidErr.Path) } if infraErr, ok := errors.AsType[*image.ErrArchiveInfra](err); ok { - return fmt.Sprintf("Could not %s: %v. Free up disk space or set TMPDIR to a writable location and try again.", infraErr.Op, infraErr.Cause) + // The disk-space hint is only trustworthy when the cause is actually a + // full disk. ErrArchiveInfra also covers seek/temp-file/I/O failures + // (e.g. on a network mount), where telling the user to free disk space + // misdirects them. Show the hint only on ENOSPC, matching the CLI. + if errors.Is(infraErr.Cause, syscall.ENOSPC) { + return fmt.Sprintf("Could not %s: %v. Free up disk space or set TMPDIR to a writable location and try again.", infraErr.Op, infraErr.Cause) + } + return fmt.Sprintf("Could not %s: %v.", infraErr.Op, infraErr.Cause) } // Platform errors are already user-readable; pass them through verbatim // so the multi-line "Available platforms:" list keeps its formatting. diff --git a/tui/model_test.go b/tui/model_test.go index 40e2a24..e29bafd 100644 --- a/tui/model_test.go +++ b/tui/model_test.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "strings" + "syscall" "testing" tea "charm.land/bubbletea/v2" @@ -758,6 +759,25 @@ func TestFriendlyErrorGenericReturnsMessage(t *testing.T) { assert.Equal(t, "unexpected failure", friendlyError(err)) } +func TestFriendlyErrorArchiveInfra_ENOSPCKeepsHint(t *testing.T) { + // A genuine disk-full cause should keep the free-up-space hint. + err := &image.ErrArchiveInfra{Op: "spool layer to temp file", Cause: syscall.ENOSPC} + msg := friendlyError(err) + assert.Contains(t, msg, "spool layer to temp file") + assert.Contains(t, msg, "Free up disk space") +} + +func TestFriendlyErrorArchiveInfra_NonENOSPCDropsHint(t *testing.T) { + // A seek/I/O failure that is not ENOSPC must NOT tell the user to free + // disk space — that misdirects on network-mount and permission errors. + err := &image.ErrArchiveInfra{Op: "seek in spooled archive", Cause: errors.New("input/output error")} + msg := friendlyError(err) + assert.Contains(t, msg, "seek in spooled archive") + assert.Contains(t, msg, "input/output error") + assert.NotContains(t, msg, "Free up disk space") + assert.NotContains(t, msg, "TMPDIR") +} + // --- renderLayers ------------------------------------------------------------ func TestRenderLayersDoesNotPanic(t *testing.T) {