feat: implement Phase A foundation and Phase B Postgres walking skeleton#1
Merged
Conversation
Phase A: Go module, Cobra CLI, Bubble Tea TUI placeholder, Driver interface + registry, and the errs/config/secrets/profile support packages, with golangci-lint + depguard layer enforcement and cross-platform CI. Phase B: backup, restore, sync, verify, inspect, dumps, config, and profile working end-to-end against PostgreSQL via pg_dump/pg_restore shell-out + pgx; checksummed dump catalog; job runner with streamed progress events. schedule/tunnel are Phase G stubs.
|
Warning Review limit reached
More reviews will be available in 45 minutes and 43 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request introduces siphon, a complete database backup/restore CLI tool written in Go with PostgreSQL support. It implements connection profiles, dump cataloging with checksums, job-based async operations, typed error handling, and both CLI and TUI interfaces, along with CI/build infrastructure. ChangesSiphon Database Backup/Restore Tool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
CI lint failed: action@v6 installed golangci-lint v1.64.8 (built with go1.24), which can't parse the repo's version:"2" config and refuses to run against the go 1.26.3 module directive. Bump to @v7 + pin v2.12.2 (the v2.x linter that passes locally). Tests were unaffected (all 3 OS pass); this is a CI-tooling fix only.
- actions/checkout v4→v6, actions/setup-go v5→v6, golangci-lint-action v7→v9 (clears the Node.js 20 deprecation — all now run on Node 24). - Reorder so Lint runs first; the 3-OS Test matrix now 'needs: lint', so tests only run once lint passes (fail-fast, saves CI minutes).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (6)
internal/jobs/event.go (1)
36-41: ⚡ Quick winClarify the
TablesDonefield type and purpose.The
Progressstruct hasTablesDone stringalongsideTablesTotal intandTablesPassed int. It's unclear whetherTablesDoneis intended to be:
- A counter (should be
intto matchTablesTotal/TablesPassed)- A formatted display string (should be renamed to clarify, e.g.,
TablesDoneFormatted)- A comma-separated list of completed table names
Consider renaming or documenting this field to clarify its purpose and expected format.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/jobs/event.go` around lines 36 - 41, The TablesDone field on the Progress struct is ambiguous (currently string); decide its intended meaning and update the type/name and all usages accordingly: if it is a numeric counter make TablesDone an int to match TablesTotal/TablesPassed; if it is a human-readable summary rename it to TablesDoneFormatted string; if it should hold multiple table names change it to []string (or a comma-separated string and rename to TablesDoneList/CommaSeparatedTables) and update any serialization/logging to match. Update the Progress definition and every place that constructs or reads Progress (search for Progress, TablesDone, TablesDoneFormatted, TablesDoneList) to reflect the chosen type and add a short comment on the field describing the expected format.internal/cli/config.go (1)
32-42: ⚡ Quick win
$EDITOR/$VISUALcontaining arguments will fail to launch.
exec.Command(editor, config.Path())treats the entireeditorstring as the executable name. Common values likecode --waitoremacsclient -twill fail because the binarycode --waitdoesn't exist. Consider splitting the editor string into fields.♻️ Proposed fix to split editor command and args
+import ( + "strings" + ... +) + RunE: func(c *cobra.Command, _ []string) error { editor := defaultEditor() if editor == "" { editor = "vi" } - ed := exec.Command(editor, config.Path()) + parts := strings.Fields(editor) + args := append(parts[1:], config.Path()) + ed := exec.Command(parts[0], args...) ed.Stdin = c.InOrStdin() ed.Stdout = c.OutOrStdout() ed.Stderr = c.ErrOrStderr() return ed.Run() },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/config.go` around lines 32 - 42, The RunE handler uses exec.Command(editor, config.Path()) which treats the whole defaultEditor() string as a single executable (so values like "code --wait" fail); change the logic in the RunE closure to split the editor string into the program and args (e.g., use strings.Fields(defaultEditor()) or a shell-aware splitter), ensure you fall back to "vi" when defaultEditor() is empty, then build the exec.Command by using the first token as the binary and appending config.Path() to the args slice before calling ed.Run(); update variables around editor and ed accordingly so exec.Command receives the correct binary and argument list.internal/driver/postgres/verify.go (1)
15-26: ⚡ Quick win
ctxis accepted but never observed during the checksum.
io.Copyover the entire dump stream can run for a long time on large backups, yet a cancelledctxwon't interrupt it. Wrap the reader so cancellation aborts the copy:♻️ Proposed fix
started := time.Now() h := sha256.New() - if _, err := io.Copy(h, r); err != nil { + if _, err := io.Copy(h, readerCtx(ctx, r)); err != nil { return nil, err }where
readerCtxreturns a reader whoseReadreturnsctx.Err()once the context is done (or checkctx.Err()between chunked reads).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/driver/postgres/verify.go` around lines 15 - 26, The Verify method (Conn.Verify) reads the entire stream with io.Copy but ignores ctx cancellation; wrap the incoming io.Reader with a context-aware reader (e.g., use or implement readerCtx(ctx, r) whose Read returns ctx.Err() once ctx is done) or perform chunked reads and check ctx.Err() between chunks before continuing the copy; then call io.Copy with that wrapped reader so a cancelled context aborts checksum computation and returns promptly.internal/driver/postgres/progress.go (1)
19-27: ⚡ Quick winPrefix match is locale-sensitive.
pg_dump --verbosediagnostics are localized viaLC_MESSAGES, so this hardcoded English prefix won't match under a non-C locale and progress would silently degrade to no table events. When this gets wired in Phase F, force a deterministic locale in the command environment (e.g. addLC_MESSAGES=CalongsidePGPASSWORDinbackup.go) so parsing stays reliable.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/driver/postgres/progress.go` around lines 19 - 27, The progress parsing uses an English prefix ("pg_dump: dumping contents of table") which is locale-sensitive; update the code that spawns pg_dump (in backup.go where you set PGPASSWORD) to also set LC_MESSAGES=C in the command environment so pg_dump emits deterministic English diagnostics and the extractAfter/emit logic in progress.go continues to match; no changes needed to progress.go parsing itself, just add LC_MESSAGES=C alongside PGPASSWORD when building the command env.internal/config/config.go (2)
101-108: ⚡ Quick winPrefer
filepath.Dirover a hand-rolledparentDir.
filepath.Diralready handles separators (platform-aware viaos.PathSeparatorsemantics) and edge cases. The current loop returns""for a root-level path like/config.yaml(wherefilepath.Dirreturns/), which would make the subsequentMkdirAll("")fail.♻️ Proposed refactor
import ( "errors" "fmt" "os" + "path/filepath" "gopkg.in/yaml.v3" )func Save(cfg *Config) error { path := Path() - if err := os.MkdirAll(parentDir(path), 0o700); err != nil { + if err := os.MkdirAll(filepath.Dir(path), 0o700); err != nil { return fmt.Errorf("mkdir config: %w", err) }-func parentDir(path string) string { - for i := len(path) - 1; i >= 0; i-- { - if path[i] == '/' || path[i] == '\\' { - return path[:i] - } - } - return "." -}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/config.go` around lines 101 - 108, Replace the custom parentDir function with the standard filepath.Dir: import "path/filepath", change parentDir to return filepath.Dir(path) (or remove the helper and call filepath.Dir where used), and ensure any subsequent call that does os.MkdirAll(dir, ...) never receives an empty string by falling back to "." only if filepath.Dir somehow returns "" (e.g., dir := filepath.Dir(path); if dir == "" { dir = "." }). This ensures platform-aware separators and correct handling of root-level paths; update usages of parentDir accordingly.
65-65: ⚖️ Poor tradeoff
os.ExpandEnvover the whole file is a footgun for$-bearing values.Interpolating the raw YAML before parsing means any literal
$in a value (e.g. a password likep$ssw0rd) is treated as a variable reference and silently rewritten — undefined names expand to empty strings. This is also broader than the documented${VAR}syntax since bare$VARis expanded too. Consider restricting interpolation to explicit${VAR}tokens (e.g.os.Expandwith a mapping that leaves unknown/unsupported tokens intact), or documenting the escaping requirement.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/config/config.go` at line 65, The code currently calls os.ExpandEnv on the entire raw YAML (variable expanded in internal/config/config.go), which silently removes $VAR tokens; replace this with os.Expand and supply a mapping function that returns os.Getenv(name) when the environment variable is set and otherwise returns the original token (e.g. "${"+name+"}") so unknown/unsupported tokens are preserved instead of being empty; this keeps literal $ values intact while still substituting explicit ${VAR} occurrences.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 20-41: Update the workflow steps that use marketplace actions to
(1) add persist-credentials: false to every actions/checkout step to avoid
leaving GITHUB_TOKEN in the runner, and (2) replace floating tags with pinned
commit SHAs for actions/checkout, actions/setup-go and
golangci/golangci-lint-action (i.e. change uses: entries for those actions to
use a specific commit SHA). Locate the steps referencing actions/checkout,
actions/setup-go and golangci/golangci-lint-action in the CI job definitions and
apply these two changes consistently.
In `@internal/app/backup.go`:
- Around line 109-131: After os.Rename(tmpPath, finalPath) succeeds, if
d.Dumps.WriteMeta(meta) returns an error we must remove the committed dump file
to avoid orphaned files; update the error path in the routine that builds meta
(using symbols tmpPath, finalPath, dumps.Meta and d.Dumps.WriteMeta) to attempt
os.Remove(finalPath) (and ignore or log any remove error) before returning
writeErr so the filesystem is cleaned up when metadata persistence fails.
In `@internal/app/sync.go`:
- Around line 19-22: The doc comment for Sync is out of sync with the
implementation: update behavior or code. Either (A) implement the described
gating/fallback by reading opt.Stream inside Sync, query each driver's
Capabilities().NativeStream, and only use the in-memory pipe when
opt.Stream==true and both drivers report NativeStream, otherwise call the
existing catalog-based Backup and Restore path (or add a catalogFallback
function) to perform Backup-then-Restore; or (B) if the current always-stream
behavior is intended, change the comment to state that Sync always uses the
in-memory pipe and remove references to opt.Stream and
Capabilities().NativeStream and the catalog fallback. Ensure references to Sync,
opt.Stream, Capabilities().NativeStream, and the catalog Backup/Restore are used
so reviewers can find and verify the change.
- Around line 59-66: The pipe writer is closed with pw.Close(), which signals a
clean EOF so dstConn.Restore can proceed and potentially commit destructive
changes before the backup error is observed; instead, catch the error returned
from srcConn.Backup and call pw.CloseWithError(err) when Backup returns a
non-nil error so the reader inside dstConn.Restore receives the failure and
aborts; specifically, modify the goroutine that runs srcConn.Backup to capture
its returned error, call pw.CloseWithError(err) on non-nil error (or pw.Close()
on nil), and ensure pr is still closed/unblocked as needed after restore to
avoid deadlocks (references: pw, pr, srcConn.Backup, dstConn.Restore,
CloseWithError).
In `@internal/cli/dumps.go`:
- Around line 72-79: The loop over rep.Would currently ignores errors from
deps.Dumps.Delete(m.ID) and always prints a "deleted" message; modify the block
in the loop that runs when apply is true to capture the error returned by
deps.Dumps.Delete(m.ID), and if deletion fails print an error message to
c.ErrOrStderr() (or use process logger) including m.ID and the error instead of
the success message; only print the "✗ deleted" confirmation to c.OutOrStdout()
when the delete call returns nil. Ensure you update the branch that handles
apply (the code referencing deps.Dumps.Delete, m.ID, and c.OutOrStdout()) so
failures are surfaced to the user.
In `@internal/cli/profile.go`:
- Around line 78-95: The profileShowCmd currently prints the profile password
verbatim (via fmt.Fprintf to c.OutOrStdout) which leaks plaintext when the
password is a literal; update the RunE in profileShowCmd to detect whether
p.Password is a SecretRef (e.g., starts with "env:" or use an existing helper
like IsSecretRef if available) and, if it is not a SecretRef, replace the
displayed value with a masked placeholder such as "<hidden>" before calling
fmt.Fprintf so secret refs continue to be shown but literal passwords are not
echoed.
In `@internal/cli/root.go`:
- Around line 69-73: The fallback for dumpDir currently builds a non-portable
path and ignores os.UserHomeDir errors; update the logic around dumpDir
(cfg.Defaults.DumpDir) to call os.UserHomeDir(), check and propagate/handle its
error instead of discarding it, and construct the path using filepath.Join(home,
".local", "share", "siphon", "dumps") to ensure platform-correct separators;
also search for other places that derive the default dump directory and align
their behavior to maintain consistency.
In `@internal/config/path.go`:
- Around line 18-27: Handle the ignored error from os.UserHomeDir(): capture the
returned error from os.UserHomeDir() and, if non-nil, deterministically fall
back instead of using an empty string — e.g., on Windows first check APPDATA
then USERPROFILE, on Unix check HOME, and if those are empty use os.TempDir()
(or otherwise return an error if the function signature allows). Update the
branch that currently calls os.UserHomeDir() and the runtime.GOOS switch so they
use the validated/fallback home directory (or propagate the error) when building
the filepath rather than concatenating with an empty home.
In `@internal/driver/postgres/backup.go`:
- Around line 23-37: The drain goroutine reading from cmd.StderrPipe races with
cmd.Wait in postgres.backup: remove the StderrPipe + goroutine and instead
assign cmd.Stderr = io.Discard before starting the process (so os/exec handles
stderr without a separate reader); if you later wire parseProgress to read
stderr, restore a reader but synchronize it with cmd.Wait using a sync.WaitGroup
(start the reader goroutine, Add(1)/Done() around io.Copy, and call
WaitGroup.Wait() before calling cmd.Wait()) so the reader always finishes before
Wait() is invoked.
In `@internal/driver/postgres/driver.go`:
- Around line 59-78: The buildDSN function emits raw key=value tokens which
break when values contain spaces or quotes; update buildDSN to quote and escape
libpq keyword/value values: create a helper (e.g., kv or escapeKV) used by
buildDSN to wrap values with single quotes if they contain space, backslash or
single-quote, and replace `\`→`\\` and `'`→`\'` before quoting, then append
key+"="+escapedValue for fields like Host, User, Password, Database and sslmode
as before; ensure behavior for empty values remains unchanged.
In `@internal/driver/postgres/inspect.go`:
- Around line 13-21: The inspectQuery uses a redundant correlated subquery to
fetch reltuples from pg_class and uses COALESCE which doesn't handle the
sentinel -1; replace the subquery with direct use of c.reltuples and normalize
negative sentinel values by wrapping c.reltuples::bigint with
GREATEST(c.reltuples::bigint, 0) so the rows column uses the joined pg_class
alias (c) directly and never returns -1. Update the inspectQuery constant to
reference c.reltuples and apply GREATEST(..., 0) for the rows expression.
In `@internal/driver/postgres/integration_test.go`:
- Around line 30-31: The calls to c.Host(ctx) and c.MappedPort(ctx, "5432/tcp")
drop errors; update the helper in integration_test.go to check the returned
errors for Host and MappedPort and call t.Fatalf with a clear message and the
error (e.g., "failed to get container host: %v" / "failed to get mapped port:
%v") so host/port aren’t left empty and failures surface as container-networking
errors instead of later connection errors; keep using ctx, c, host, port and
t.Fatalf to fail fast.
In `@internal/driver/postgres/restore.go`:
- Around line 26-48: The comment in backup.go is stale because RestoreOpts lacks
a Parallel field; to fix, add a Parallel int field to
internal/driver/driver.go's RestoreOpts and update
internal/driver/postgres/restore.go's buildRestoreArgs to append pg_restore
parallel flags when set (e.g., if opt.Parallel > 0 then append "-j" and
strconv.Itoa(opt.Parallel)); also validate positive values and document the new
field, or alternatively remove/update the misleading comment in backup.go if you
prefer not to implement parallel restores.
In `@internal/dumps/catalog.go`:
- Around line 27-31: Validate and reject unsafe dump IDs before using them to
build filesystem paths: implement a helper on Catalog (e.g., safeID(id string)
(string, error)) that returns an error for empty IDs, IDs containing path
separators or traversal (id != filepath.Base(id)), and use that helper from
Path, MetaPath, ReadMeta, Delete and any List callers that accept or load m.ID;
ensure callers propagate the error instead of joining unvalidated IDs so path
traversal and deletion outside c.root are prevented.
In `@internal/secrets/resolver.go`:
- Around line 61-71: parseScheme currently treats any prefix before ':' as a
scheme which misclassifies literals like "p@ss:w0rd"; update parseScheme so it
only returns a scheme when it is unambiguous — either ensure the colon is
followed by '//' (i.e. require "scheme://") or validate the candidate scheme
against the resolver's registered schemes (e.g. check the
backends/registeredSchemes map) and otherwise return "" to signal a literal;
additionally, ensure Resolve treats a "" scheme as passthrough (do not error
when no backend matches the parsed scheme) so unknown or unregistered prefixes
fall back to literal values instead of producing a "no backend matches scheme"
error.
---
Nitpick comments:
In `@internal/cli/config.go`:
- Around line 32-42: The RunE handler uses exec.Command(editor, config.Path())
which treats the whole defaultEditor() string as a single executable (so values
like "code --wait" fail); change the logic in the RunE closure to split the
editor string into the program and args (e.g., use
strings.Fields(defaultEditor()) or a shell-aware splitter), ensure you fall back
to "vi" when defaultEditor() is empty, then build the exec.Command by using the
first token as the binary and appending config.Path() to the args slice before
calling ed.Run(); update variables around editor and ed accordingly so
exec.Command receives the correct binary and argument list.
In `@internal/config/config.go`:
- Around line 101-108: Replace the custom parentDir function with the standard
filepath.Dir: import "path/filepath", change parentDir to return
filepath.Dir(path) (or remove the helper and call filepath.Dir where used), and
ensure any subsequent call that does os.MkdirAll(dir, ...) never receives an
empty string by falling back to "." only if filepath.Dir somehow returns ""
(e.g., dir := filepath.Dir(path); if dir == "" { dir = "." }). This ensures
platform-aware separators and correct handling of root-level paths; update
usages of parentDir accordingly.
- Line 65: The code currently calls os.ExpandEnv on the entire raw YAML
(variable expanded in internal/config/config.go), which silently removes $VAR
tokens; replace this with os.Expand and supply a mapping function that returns
os.Getenv(name) when the environment variable is set and otherwise returns the
original token (e.g. "${"+name+"}") so unknown/unsupported tokens are preserved
instead of being empty; this keeps literal $ values intact while still
substituting explicit ${VAR} occurrences.
In `@internal/driver/postgres/progress.go`:
- Around line 19-27: The progress parsing uses an English prefix ("pg_dump:
dumping contents of table") which is locale-sensitive; update the code that
spawns pg_dump (in backup.go where you set PGPASSWORD) to also set LC_MESSAGES=C
in the command environment so pg_dump emits deterministic English diagnostics
and the extractAfter/emit logic in progress.go continues to match; no changes
needed to progress.go parsing itself, just add LC_MESSAGES=C alongside
PGPASSWORD when building the command env.
In `@internal/driver/postgres/verify.go`:
- Around line 15-26: The Verify method (Conn.Verify) reads the entire stream
with io.Copy but ignores ctx cancellation; wrap the incoming io.Reader with a
context-aware reader (e.g., use or implement readerCtx(ctx, r) whose Read
returns ctx.Err() once ctx is done) or perform chunked reads and check ctx.Err()
between chunks before continuing the copy; then call io.Copy with that wrapped
reader so a cancelled context aborts checksum computation and returns promptly.
In `@internal/jobs/event.go`:
- Around line 36-41: The TablesDone field on the Progress struct is ambiguous
(currently string); decide its intended meaning and update the type/name and all
usages accordingly: if it is a numeric counter make TablesDone an int to match
TablesTotal/TablesPassed; if it is a human-readable summary rename it to
TablesDoneFormatted string; if it should hold multiple table names change it to
[]string (or a comma-separated string and rename to
TablesDoneList/CommaSeparatedTables) and update any serialization/logging to
match. Update the Progress definition and every place that constructs or reads
Progress (search for Progress, TablesDone, TablesDoneFormatted, TablesDoneList)
to reflect the chosen type and add a short comment on the field describing the
expected format.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 097b8563-9bcb-4a6f-b709-e1ada53df617
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (61)
.github/workflows/ci.yml.golangci.ymlMakefilecmd/siphon/main.gogo.modinternal/app/backup.gointernal/app/backup_test.gointernal/app/drivers.gointernal/app/inspect.gointernal/app/restore.gointernal/app/retry.gointernal/app/roundtrip_test.gointernal/app/sync.gointernal/app/sync_test.gointernal/app/verify.gointernal/cli/backup.gointernal/cli/config.gointernal/cli/dumps.gointernal/cli/heartbeat.gointernal/cli/heartbeat_test.gointernal/cli/inspect.gointernal/cli/profile.gointernal/cli/restore.gointernal/cli/root.gointernal/cli/root_test.gointernal/cli/sync.gointernal/cli/verify.gointernal/config/config.gointernal/config/config_test.gointernal/config/path.gointernal/driver/driver.gointernal/driver/postgres/backup.gointernal/driver/postgres/backup_test.gointernal/driver/postgres/driver.gointernal/driver/postgres/driver_test.gointernal/driver/postgres/inspect.gointernal/driver/postgres/integration_test.gointernal/driver/postgres/progress.gointernal/driver/postgres/progress_test.gointernal/driver/postgres/restore.gointernal/driver/postgres/restore_test.gointernal/driver/postgres/verify.gointernal/driver/registry.gointernal/driver/registry_test.gointernal/dumps/catalog.gointernal/dumps/catalog_test.gointernal/dumps/meta.gointernal/dumps/prune.gointernal/errs/errs.gointernal/errs/errs_test.gointernal/jobs/event.gointernal/jobs/runner.gointernal/jobs/runner_test.gointernal/profile/store.gointernal/profile/store_test.gointernal/secrets/env.gointernal/secrets/passthrough.gointernal/secrets/resolver.gointernal/secrets/resolver_test.gointernal/tui/dashboard.gointernal/tui/dashboard_test.go
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.
A literal value with a colon before any '/' or space (e.g. a Postgres password 'p@ss:w0rd') was parsed as the scheme 'p@ss', matched no backend, and errored instead of resolving. Now an unclaimed scheme falls through to the passthrough backend; the error only fires when no passthrough exists. This also aligns with 'profile show' masking, which treats the same value as a literal. Regression tests added. Also narrows the .claude/settings.json secrets deny-glob from ./secrets/** to /secrets/** so it protects a root ./secrets/ credentials dir without blocking the internal/secrets/ Go source package.
os.UserHomeDir()'s error was discarded; when it fails (home env var unset) home became "", yielding a cwd-relative config path silently read/written in an unexpected location. New homeDir() helper falls back to the platform home env var (USERPROFILE/HOME), then os.TempDir(), so Path() is always absolute. No signature change — callers untouched. Regression tests added.
A table that has never been ANALYZEd reports reltuples = -1; surfacing that as a negative row estimate in 'siphon inspect' reads as a bug. GREATEST(...,0) clamps it to 0 (the conventional 'unknown/0' shown by pg tooling) while real counts pass through unchanged. Query structure (and the s. alias fix for the relname ambiguity) is otherwise unchanged — the 'simplify' half of the review note is intentionally not taken, to avoid regressing the just-fixed SQL.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/dumps/catalog_test.go`:
- Around line 96-99: The test currently uses t.Skip when
ReadMeta("01HXKZ000000000000000000") returns nil, masking a real failure; change
this to assert and fail instead: replace t.Skip with a fatal assertion (e.g.,
t.Fatalf) so the test fails if err == nil, then explicitly check that the
returned error is the expected not-found error using errors.Is(err, ErrNotFound)
(or the package's equivalent sentinel, e.g., catalog.ErrNotFound) and call
t.Fatalf if it is some other error — target the ReadMeta call and the nil-error
branch for this change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8350e100-ff8e-4fc9-a0ac-22ea5b438ffb
📒 Files selected for processing (18)
.claude/settings.json.github/workflows/ci.ymlinternal/app/backup.gointernal/app/sync.gointernal/cli/dumps.gointernal/cli/profile.gointernal/cli/root.gointernal/config/path.gointernal/config/path_test.gointernal/driver/postgres/backup.gointernal/driver/postgres/driver.gointernal/driver/postgres/driver_test.gointernal/driver/postgres/inspect.gointernal/driver/postgres/integration_test.gointernal/dumps/catalog.gointernal/dumps/catalog_test.gointernal/secrets/resolver.gointernal/secrets/resolver_test.go
🚧 Files skipped from review as they are similar to previous changes (8)
- internal/secrets/resolver.go
- internal/cli/profile.go
- internal/config/path.go
- internal/app/sync.go
- internal/driver/postgres/inspect.go
- internal/driver/postgres/driver.go
- internal/cli/dumps.go
- internal/driver/postgres/integration_test.go
The valid-but-missing-id branch used t.Skip, which masks a regression: if ReadMeta wrongly returned nil for a valid id, the test would skip rather than fail. Now it fails (t.Fatal) on nil and asserts errors.Is(err, errs.ErrDumpCorrupt) — the sentinel ReadMeta actually wraps for a missing dump.
This was referenced Jun 21, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Phase A: Go module, Cobra CLI, Bubble Tea TUI placeholder, Driver interface
Phase B: backup, restore, sync, verify, inspect, dumps, config, and profile working end-to-end against PostgreSQL via pg_dump/pg_restore shell-out + pgx; checksummed dump catalog; job runner with streamed progress events. schedule/tunnel are Phase G stubs.
Summary by CodeRabbit
New Features
Chores