Skip to content

Commit 5831efe

Browse files
nixrajputclaude
andcommitted
fix(cli): defer premature capability gates to Phase F + address review
Remove the `backup --jobs`→CapParallel and `sync --stream`→CapNativeStream preflight gates. Both guarded features that the current verbs do not yet implement: app.Sync ignores opt.Stream (always pipes Backup→Restore via io.Pipe), and pg_dump parallelism (-j/-Fd) is unwired ("not yet effective; Phase F"). Gating them now would falsely reject valid operations the moment a driver declaring NativeStream:false / Parallel:false lands, while gating a no-op feature. The RequireCapability helper + Capabilities flags stay; the gates get wired in Phase F alongside the features they guard. Also from CodeRabbit review on PR #3: - _testing/suite.go: the Cancel_PropagatesToSubprocess subtest could flake on fast hosts when the prior subtest's tiny fixture lets pg_dump finish before the 150ms cancel. Seed ~5 MiB via fx.SQLOpener so the dump is reliably in-flight at cancel time; keep the strong non-nil assertion. (Integration-tag only; not run in CI.) - DRIVERS.md: add a `text` language tag to the architecture-diagram fence (markdownlint MD040), and correct the capability section + README/CHANGELOG to describe gating as helper-available-but-not-yet-wired, not CLI-wired. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent c2a8047 commit 5831efe

6 files changed

Lines changed: 67 additions & 22 deletions

File tree

CHANGELOG.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,6 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
1414
- Phase C interactive TUI dashboard: multi-panel Bubble Tea UI (profiles · dumps · jobs) with live job-progress subscription, backup/restore modal forms, an error overlay, and golden snapshot tests.
1515
- Phase D driver-layer hardening:
1616
- Shared cross-driver test harness `RunDriverSuite` under `internal/driver/_testing/`, giving every driver four contract tests for free (connect/inspect, backup→restore round-trip, cancel propagation, bad-credentials sentinel). The Postgres integration test now runs on it.
17-
- Capability gating: `app.RequireCapability` resolves a profile to its driver and rejects unsupported affordances with a `CodeUser` error. Wired into the CLI so `backup --jobs N>1` is gated by `Parallel` and `sync --stream` by `NativeStream`.
17+
- Capability gating helper: `app.RequireCapability` resolves a profile to its driver and rejects unsupported affordances with a `CodeUser` error. The helper and the `Capabilities` flags are in place; verbs are wired to it only where the gated feature is implemented. Streaming (`NativeStream`) and parallel backup (`Parallel`) are deferred to Phase F, so `sync --stream` / `backup --jobs` are not yet gated — the wiring lands with those features.
1818
- Connection-probe retry on Postgres `Connect` (3 attempts, exponential backoff) per spec §4.3; the generic `Retry` helper moved to `internal/jobs` to keep the driver layer free of an app-layer import.
1919
- `docs/DRIVERS.md` contributor guide documenting the driver contract, registration, the test harness, capability flags, and error mapping.

README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ A single binary that turns the painful, error-prone sprawl of `pg_dump` → `pg_
5252
| **A** — Skeleton | Go module, Cobra CLI, TUI placeholder, `Driver` interface + registry, `errs`/`config`/`secrets`/`profile` packages, golangci-lint + depguard, cross-platform CI | ✅ Complete |
5353
| **B** — Postgres walking skeleton | `backup`, `restore`, `sync`, `verify`, `inspect`, `dumps`, `config`, `profile` working end-to-end against PostgreSQL | ✅ Complete |
5454
| **C** — TUI dashboard | Multi-panel Bubble Tea dashboard (profiles · dumps · jobs) with live job progress, backup/restore modal forms, and snapshot tests | ✅ Complete |
55-
| **D** — Driver hardening | Shared cross-driver test harness (`RunDriverSuite`), capability gating (`RequireCapability`), connection-probe retry, and a `docs/DRIVERS.md` contributor guide | ✅ Complete |
55+
| **D** — Driver hardening | Shared cross-driver test harness (`RunDriverSuite`), capability-gating helper (`RequireCapability`), Postgres connection-probe retry, and a `docs/DRIVERS.md` contributor guide | ✅ Complete |
5656
| **E** — MySQL + MariaDB | Both drivers via a shared `_mysqlcommon` package | ⏳ Planned |
5757
| **F** — Advanced transfer | Incremental backups, bounded-buffer streaming, cross-engine sync, CDC | ⏳ Planned |
5858
| **G** — Ops features | Cloud storage, secret backends, profile groups + 2FA, team mode, audit log, retention, telemetry | ⏳ Planned |

docs/DRIVERS.md

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ that shells out to the engine's native tools for data movement (e.g.
2424
siphon is strictly layered; imports flow **downward only**, enforced at lint
2525
time by `golangci-lint`'s `depguard`:
2626

27-
```
27+
```text
2828
internal/cli internal/tui presentation (Cobra · Bubble Tea)
2929
└──────┬───────┘
3030
internal/app application verbs (backup, restore, sync, …)
@@ -120,13 +120,19 @@ type Capabilities struct {
120120
}
121121
```
122122

123-
**Declare these honestly.** The presentation layer gates affordances through
124-
`app.RequireCapability(deps, profileName, cap)` (in `internal/app/capgate.go`)
125-
before running a verb. If your driver declares `Parallel: false`, the CLI rejects
126-
`--jobs N` (with `N > 1`) up front with an `errs.ErrDriverUnsupported` error and
127-
an actionable hint — instead of crashing partway through. Over-declaring a
128-
capability you don't actually support turns that clean rejection into a runtime
129-
failure.
123+
**Declare these honestly.** A verb pre-flight can gate an affordance through
124+
`app.RequireCapability(deps, profileName, cap)` (in `internal/app/capgate.go`):
125+
if the resolved driver doesn't support `cap`, it returns an
126+
`errs.ErrDriverUnsupported` error with an actionable hint up front, instead of
127+
crashing partway through. Over-declaring a capability you don't actually support
128+
turns that clean rejection into a runtime failure.
129+
130+
> **Status:** the gate helper and the `Capabilities` flags exist today, but the
131+
> verbs are only gated where a feature is actually wired. Native streaming
132+
> (`NativeStream`) and parallel backup (`Parallel`) are deferred to Phase F, so
133+
> `sync --stream` / `backup --jobs` are not yet gated — the gate will be wired in
134+
> alongside those features. Declare your flags honestly now so the gating is
135+
> correct the moment a verb starts honoring them.
130136
131137
When you add support for a capability, flip its flag — that single change lights
132138
up the corresponding UI path.

internal/cli/backup.go

Lines changed: 0 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,6 @@ func newBackupCmd() *cobra.Command {
2929
if err != nil {
3030
return err
3131
}
32-
if parallel > 1 {
33-
if err := app.RequireCapability(deps, profileName, app.CapParallel); err != nil {
34-
return err
35-
}
36-
}
3732
ch, _, err := app.Backup(c.Context(), deps, app.BackupOpts{
3833
Profile: profileName,
3934
IncludeTables: includeTables,

internal/cli/sync.go

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -27,13 +27,6 @@ func newSyncCmd() *cobra.Command {
2727
if err != nil {
2828
return err
2929
}
30-
if stream {
31-
for _, name := range []string{fromName, toName} {
32-
if err := app.RequireCapability(deps, name, app.CapNativeStream); err != nil {
33-
return err
34-
}
35-
}
36-
}
3730
ch, _, err := app.Sync(c.Context(), deps, app.SyncOpts{
3831
From: fromName, To: toName, Stream: stream, Tables: tables,
3932
})

internal/driver/_testing/suite.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,57 @@ import (
44
"bytes"
55
"context"
66
"errors"
7+
"strconv"
8+
"strings"
79
"testing"
810
"time"
911

1012
"github.com/nixrajput/siphon/internal/driver"
1113
"github.com/nixrajput/siphon/internal/errs"
1214
)
1315

16+
// seedCancelLoad bulk-loads a table of wide rows via the fixture's raw SQL
17+
// connection so a subsequent Backup has enough data to stay in-flight past the
18+
// cancel delay. Uses only portable SQL (CREATE TABLE + parameterized INSERT in
19+
// one transaction), so it works for any engine the harness is pointed at.
20+
func seedCancelLoad(t *testing.T, fx Fixtures) {
21+
t.Helper()
22+
ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
23+
defer cancel()
24+
25+
db, err := fx.SQLOpener()
26+
if err != nil {
27+
t.Fatalf("seedCancelLoad: SQLOpener: %v", err)
28+
}
29+
defer func() { _ = db.Close() }()
30+
31+
if _, err := db.ExecContext(ctx,
32+
`CREATE TABLE cancel_load (id integer primary key, payload text)`); err != nil {
33+
t.Fatalf("seedCancelLoad: create table: %v", err)
34+
}
35+
36+
// ~5000 rows of ~1 KiB each ≈ 5 MiB — large enough that the dump can't
37+
// complete within the 150ms cancel window, small enough to load quickly.
38+
// The payload is a fixed safe literal (no user input), so it's inlined
39+
// rather than parameterized — this keeps the SQL placeholder-agnostic
40+
// ($1 vs ? differs by engine) so the harness stays portable across drivers.
41+
payload := strings.Repeat("x", 1024)
42+
tx, err := db.BeginTx(ctx, nil)
43+
if err != nil {
44+
t.Fatalf("seedCancelLoad: begin: %v", err)
45+
}
46+
for i := 0; i < 5000; i++ {
47+
if _, err := tx.ExecContext(ctx,
48+
"INSERT INTO cancel_load (id, payload) VALUES ("+strconv.Itoa(i)+", '"+payload+"')"); err != nil {
49+
_ = tx.Rollback()
50+
t.Fatalf("seedCancelLoad: insert %d: %v", i, err)
51+
}
52+
}
53+
if err := tx.Commit(); err != nil {
54+
t.Fatalf("seedCancelLoad: commit: %v", err)
55+
}
56+
}
57+
1458
// RunDriverSuite exercises the full Driver contract against a real
1559
// database. ctor returns the driver-under-test; fx provides the env.
1660
//
@@ -79,6 +123,13 @@ func RunDriverSuite(t *testing.T, ctor func() driver.Driver, fx Fixtures) {
79123
})
80124

81125
t.Run("Cancel_PropagatesToSubprocess", func(t *testing.T) {
126+
// Inflate the database so the dump takes long enough that cancel()
127+
// reliably lands while pg_dump (or the engine's equivalent) is still
128+
// streaming. With only the round-trip subtest's tiny fixture, the dump
129+
// can finish in well under the cancel delay on a fast host, making
130+
// Backup return a clean nil and the assertion below spuriously fail.
131+
seedCancelLoad(t, fx)
132+
82133
conn, err := d.Connect(context.Background(), fx.Profile)
83134
if err != nil {
84135
t.Fatalf("Connect: %v", err)

0 commit comments

Comments
 (0)