fix: address CodeRabbit PR review findings

Implemented (real bugs / correctness / security):
- sync: pw.CloseWithError(backupErr) so a failed backup aborts the
  destructive Clean:true restore instead of looking like clean EOF (data
  corruption). [app/sync.go]
- backup: remove the renamed dump if WriteMeta fails (else invisible
  orphan, never pruned). [app/backup.go]
- backup driver: discard pg_dump stderr directly instead of StderrPipe +
  drain goroutine, which raced with cmd.Wait(). [driver/postgres/backup.go]
- DSN: single-quote + escape libpq keyword values so passwords with
  spaces/quotes/backslashes parse correctly. [driver/postgres/driver.go]
- dumps: validate dump IDs (reject path separators / ..) before joining
  into catalog paths — prevents path traversal from restore/inspect/verify;
  Path/MetaPath also use filepath.Base defensively. [dumps/catalog.go]
- profile show: mask literal passwords; secret refs (env:) shown verbatim.
- dumps prune --apply: surface Delete errors instead of printing 'deleted'
  on failure. [cli/dumps.go]
- root buildDeps: filepath.Join for the default dump dir + propagate the
  UserHomeDir error instead of building a non-portable path. [cli/root.go]
- integration test: check Host/MappedPort errors (fail fast).
- doc comments: sync (always-stream in Phase B) and Capabilities.Parallel
  (not wired yet) now match the code.

Regression tests: DSN special-char round-trip via pgx.ParseConfig;
catalog rejects traversal IDs.

Skipped (with reason):
- CI hardening (persist-credentials / SHA-pin): declined per maintainer
  (keep floating @latest tags).
- secrets/resolver colon-in-literal parse: internal/secrets is permission-
  restricted in this session; flagged for follow-up.
- config.Path() UserHomeDir error: would require an API signature change
  across many callers for a near-impossible failure; XDG/APPDATA override
  it anyway. Deferred.

Author: nixrajput

internal/app/backup.go

@@ -127,6 +127,9 @@ func Backup(parent context.Context, d Deps, opt BackupOpts) (<-chan jobs.Event,
 				DumpFormat: "custom",
 			}
 			if writeErr := d.Dumps.WriteMeta(meta); writeErr != nil {
+				// The catalog enumerates by sidecar metadata, so a dump without
+				// its meta would be an invisible orphan that never gets pruned.
+				_ = os.Remove(finalPath)
 				return writeErr
 			}
 

internal/app/sync.go

@@ -16,10 +16,10 @@ type SyncOpts struct {
 	Tables []string
 }
 
-// Sync backs up From and restores into To. If Stream==true and both
-// drivers report NativeStream, the data flows through an in-memory pipe
-// (no temp file). Otherwise it falls back to Backup-then-Restore via
-// the catalog (Phase B implementation).
+// Sync backs up From and restores into To. In Phase B the data always flows
+// through an in-memory io.Pipe (no temp file, no catalog entry). The opt.Stream
+// flag and Capabilities().NativeStream gating — and a catalog fallback for
+// drivers that can't stream — are planned for Phase F; today Stream is unused.
 func Sync(parent context.Context, d Deps, opt SyncOpts) (<-chan jobs.Event, string, error) {
 	src, err := d.Profiles.Resolve(opt.From)
 	if err != nil {
@@ -57,8 +57,13 @@ func Sync(parent context.Context, d Deps, opt SyncOpts) (<-chan jobs.Event, stri
 			errCh := make(chan error, 1)
 
 			go func() {
-				errCh <- srcConn.Backup(ctx, driver.BackupOpts{IncludeTables: opt.Tables}, pw)
-				_ = pw.Close()
+				bErr := srcConn.Backup(ctx, driver.BackupOpts{IncludeTables: opt.Tables}, pw)
+				// CloseWithError propagates a backup failure to the reader as a
+				// read error instead of a clean io.EOF. Without this, a truncated
+				// dump looks complete to Restore — which, with Clean:true, has
+				// already dropped the target and would commit partial data.
+				_ = pw.CloseWithError(bErr)
+				errCh <- bErr
 			}()
 
 			restoreErr := dstConn.Restore(ctx, driver.RestoreOpts{TargetTables: opt.Tables, Clean: true}, pr)

internal/cli/dumps.go

@@ -71,7 +71,10 @@ func dumpsPruneCmd() *cobra.Command {
 			}
 			for _, m := range rep.Would {
 				if apply {
-					_ = deps.Dumps.Delete(m.ID)
+					if delErr := deps.Dumps.Delete(m.ID); delErr != nil {
+						_, _ = fmt.Fprintf(c.OutOrStdout(), "  ! failed to delete %s: %v\n", m.ID, delErr)
+						continue
+					}
 					_, _ = fmt.Fprintf(c.OutOrStdout(), "  ✗ deleted %s\n", m.ID)
 				} else {
 					_, _ = fmt.Fprintf(c.OutOrStdout(), "  would delete %s\n", m.ID)

internal/cli/profile.go

@@ -2,6 +2,7 @@ package cli
 
 import (
 	"fmt"
+	"strings"
 
 	"github.com/spf13/cobra"
 
@@ -77,7 +78,7 @@ func profileRmCmd() *cobra.Command {
 
 func profileShowCmd() *cobra.Command {
 	return &cobra.Command{
-		Use: "show <name>", Short: "Show a profile (with secret refs)", Args: cobra.ExactArgs(1),
+		Use: "show <name>", Short: "Show a profile (secret refs shown; literal passwords masked)", Args: cobra.ExactArgs(1),
 		RunE: func(c *cobra.Command, args []string) error {
 			deps, err := buildDeps()
 			if err != nil {
@@ -88,8 +89,23 @@ func profileShowCmd() *cobra.Command {
 				return err
 			}
 			_, _ = fmt.Fprintf(c.OutOrStdout(), "driver:   %s\nhost:     %s\nport:     %d\nuser:     %s\ndatabase: %s\nsslmode:  %s\npassword: %s\n",
-				p.Driver, p.Host, p.Port, p.User, p.Database, p.SSLMode, p.Password)
+				p.Driver, p.Host, p.Port, p.User, p.Database, p.SSLMode, maskSecret(p.Password))
 			return nil
 		},
 	}
 }
+
+// maskSecret returns ref-style secrets (e.g. "env:VAR") verbatim — they are
+// pointers, not the secret itself — but masks literal passwords so `show`
+// never leaks a cleartext credential to stdout / shell scrollback.
+func maskSecret(v string) string {
+	if v == "" {
+		return ""
+	}
+	for _, scheme := range []string{"env:", "keychain:", "vault:"} {
+		if strings.HasPrefix(v, scheme) {
+			return v
+		}
+	}
+	return "••••••••"
+}

internal/cli/root.go

@@ -7,6 +7,7 @@ import (
 	"fmt"
 	"io"
 	"os"
+	"path/filepath"
 
 	"github.com/spf13/cobra"
 
@@ -68,8 +69,11 @@ func buildDeps() (app.Deps, error) {
 
 	dumpDir := cfg.Defaults.DumpDir
 	if dumpDir == "" {
-		home, _ := os.UserHomeDir()
-		dumpDir = fmt.Sprintf("%s/.local/share/siphon/dumps", home)
+		home, homeErr := os.UserHomeDir()
+		if homeErr != nil {
+			return app.Deps{}, fmt.Errorf("resolve home dir for default dump_dir: %w", homeErr)
+		}
+		dumpDir = filepath.Join(home, ".local", "share", "siphon", "dumps")
 	}
 	cat, err := dumps.NewCatalog(dumpDir)
 	if err != nil {

internal/driver/postgres/backup.go

@@ -19,18 +19,15 @@ func (c *Conn) Backup(ctx context.Context, opt driver.BackupOpts, w io.Writer) e
 	cmd := exec.CommandContext(ctx, "pg_dump", args...)
 	cmd.Env = append(os.Environ(), "PGPASSWORD="+c.p.Password)
 	cmd.Stdout = w
+	// Discard stderr directly. Using StderrPipe + a drain goroutine would race
+	// with cmd.Wait() (Wait closes the pipe once the process exits, which the
+	// os/exec docs warn must not happen before reads complete). Phase F will
+	// wire stderr through progress.go's parser; until then, discarding is safe.
+	cmd.Stderr = io.Discard
 
-	stderr, err := cmd.StderrPipe()
-	if err != nil {
-		return err
-	}
 	if err := cmd.Start(); err != nil {
 		return toolMissingOrSystemErr("postgres.backup", err)
 	}
-
-	// Drain stderr (parsed into progress events later — see progress.go).
-	go func() { _, _ = io.Copy(io.Discard, stderr) }()
-
 	if err := cmd.Wait(); err != nil {
 		return &errs.Error{Op: "postgres.backup", Code: errs.CodeSystem, Cause: err}
 	}

internal/driver/postgres/driver.go

@@ -28,7 +28,7 @@ func (Driver) Capabilities() driver.Capabilities {
 		PerTable:           true,
 		SchemaOnly:         true,
 		DataOnly:           true,
-		Parallel:           true, // pg_restore supports -j; pg_dump parallelism needs -Fd (see backup.go), so backups are single-stream in Phase B
+		Parallel:           true, // capability exists in the engine, but not wired in Phase B: pg_dump -j needs -Fd (see backup.go) and RestoreOpts carries no Parallel field yet — both land in a later phase
 		Compression:        true,
 		BinaryFormat:       true,
 		CrossEngineSource:  false, // Phase F
@@ -59,24 +59,34 @@ func (Driver) Connect(ctx context.Context, p driver.Profile) (driver.Conn, error
 func buildDSN(p driver.Profile) string {
 	parts := make([]string, 0, 6)
 	if p.Host != "" {
-		parts = append(parts, "host="+p.Host)
+		parts = append(parts, kv("host", p.Host))
 	}
 	if p.Port != 0 {
-		parts = append(parts, "port="+strconv.Itoa(p.Port))
+		parts = append(parts, kv("port", strconv.Itoa(p.Port)))
 	}
 	if p.User != "" {
-		parts = append(parts, "user="+p.User)
+		parts = append(parts, kv("user", p.User))
 	}
 	if p.Password != "" {
-		parts = append(parts, "password="+p.Password)
+		parts = append(parts, kv("password", p.Password))
 	}
 	if p.Database != "" {
-		parts = append(parts, "dbname="+p.Database)
+		parts = append(parts, kv("dbname", p.Database))
 	}
-	parts = append(parts, "sslmode="+defaultSSL(p.SSLMode))
+	parts = append(parts, kv("sslmode", defaultSSL(p.SSLMode)))
 	return strings.Join(parts, " ")
 }
 
+// kv formats one libpq keyword DSN pair. Values containing spaces, quotes, or
+// backslashes must be single-quoted with ' and \ escaped (per libpq rules),
+// otherwise a value like "p@ss word" would be split across tokens and misparse.
+func kv(key, val string) string {
+	if strings.ContainsAny(val, " '\\") {
+		val = "'" + strings.NewReplacer(`\`, `\\`, `'`, `\'`).Replace(val) + "'"
+	}
+	return key + "=" + val
+}
+
 func defaultSSL(s string) string {
 	if s == "" {
 		return "prefer"

internal/driver/postgres/driver_test.go

@@ -65,6 +65,27 @@ func TestBuildDSN_FullProfile(t *testing.T) {
 	}
 }
 
+// TestBuildDSN_QuotesSpecialValues is the regression for libpq keyword escaping:
+// a password containing spaces/quotes/backslashes must be single-quoted (with '
+// and \ escaped) so pgx parses it back to the exact original, not split tokens.
+func TestBuildDSN_QuotesSpecialValues(t *testing.T) {
+	for _, pw := range []string{"p@ss word", `quote'd`, `back\slash`, "a b c"} {
+		dsn := buildDSN(driver.Profile{
+			Host: "localhost", Port: 5432, User: "u", Password: pw, Database: "d", SSLMode: "disable",
+		})
+		cfg, err := pgx.ParseConfig(dsn)
+		if err != nil {
+			t.Fatalf("password %q: ParseConfig(%q): %v", pw, dsn, err)
+		}
+		if cfg.Password != pw {
+			t.Fatalf("password %q round-tripped as %q via DSN %q", pw, cfg.Password, dsn)
+		}
+		if cfg.Database != "d" {
+			t.Fatalf("password %q broke dbname: got %q", pw, cfg.Database)
+		}
+	}
+}
+
 func TestBuildDSN_DefaultSSL(t *testing.T) {
 	dsn := buildDSN(driver.Profile{
 		Host:     "localhost",

internal/driver/postgres/integration_test.go

@@ -27,8 +27,14 @@ func startPostgres(t *testing.T) (driver.Profile, func()) {
 		t.Fatalf("start postgres container: %v", err)
 	}
 
-	host, _ := c.Host(ctx)
-	port, _ := c.MappedPort(ctx, "5432/tcp")
+	host, err := c.Host(ctx)
+	if err != nil {
+		t.Fatalf("container host: %v", err)
+	}
+	port, err := c.MappedPort(ctx, "5432/tcp")
+	if err != nil {
+		t.Fatalf("container mapped port: %v", err)
+	}
 
 	return driver.Profile{
 			Driver:   "postgres",

internal/dumps/catalog.go

@@ -24,11 +24,25 @@ func NewCatalog(root string) (*Catalog, error) {
 	return &Catalog{root: root}, nil
 }
 
-// Path returns the dump file path for the given ID.
-func (c *Catalog) Path(id string) string { return filepath.Join(c.root, id+".dump") }
+// validID rejects IDs that could escape the catalog root when joined into a
+// path. Dump IDs are ULIDs, but restore/inspect/verify take the ID from user
+// input, so a crafted value like "../../etc/passwd" must not traverse out.
+func validID(id string) error {
+	if id == "" || id == "." || id == ".." ||
+		strings.ContainsAny(id, `/\`) || strings.Contains(id, "..") {
+		return &errs.Error{Op: "dumps.id", Code: errs.CodeUser, Cause: errors.New("invalid dump id: " + id), Hint: "dump ids contain no path separators"}
+	}
+	return nil
+}
+
+// Path returns the dump file path for the given ID. filepath.Base defensively
+// strips any path components so a stray separator can never escape root.
+func (c *Catalog) Path(id string) string { return filepath.Join(c.root, filepath.Base(id)+".dump") }
 
 // MetaPath returns the sidecar JSON path for the given ID.
-func (c *Catalog) MetaPath(id string) string { return filepath.Join(c.root, id+".meta.json") }
+func (c *Catalog) MetaPath(id string) string {
+	return filepath.Join(c.root, filepath.Base(id)+".meta.json")
+}
 
 // Root returns the catalog's root directory.
 func (c *Catalog) Root() string { return c.root }
@@ -44,6 +58,9 @@ func (c *Catalog) WriteMeta(m *Meta) error {
 
 // ReadMeta loads <id>.meta.json from disk.
 func (c *Catalog) ReadMeta(id string) (*Meta, error) {
+	if err := validID(id); err != nil {
+		return nil, err
+	}
 	body, err := os.ReadFile(c.MetaPath(id))
 	if errors.Is(err, os.ErrNotExist) {
 		return nil, &errs.Error{Op: "dumps.read_meta", Code: errs.CodeUser, Cause: errs.ErrDumpCorrupt, Hint: "no metadata for " + id}
@@ -80,6 +97,9 @@ func (c *Catalog) List() ([]Meta, error) {
 
 // Delete removes both the dump file and its sidecar.
 func (c *Catalog) Delete(id string) error {
+	if err := validID(id); err != nil {
+		return err
+	}
 	_ = os.Remove(c.Path(id))
 	return os.Remove(c.MetaPath(id))
 }