Feat/implement phase d#3
Merged
Merged
Conversation
Add RequireCapability(d Deps, profileName string, requiredCap Capability) in the app layer. It resolves the profile to its driver internally and checks the driver's Capabilities, returning a CodeUser errs.Error wrapping ErrDriverUnsupported when the capability is missing. Taking a profile name (not a driver.Driver) keeps the CLI/TUI from having to import internal/driver, respecting the depguard boundary.
Probe the connection via jobs.Retry (spec §4.3: 3 attempts with exponential backoff) so a briefly-unavailable server is not a hard fail. sql.Open stays a single lazy call; only PingContext is retried. On final failure the error still maps through wrapConnErr to errs.ErrConnectionFailed. Move Retry from internal/app to internal/jobs: the driver may not import internal/app (depguard driver-may-not-import-presentation), and driver already imports internal/jobs, so this avoids the import cycle. Retry had no callers; add a unit test for it in the jobs package.
|
Warning Review limit reached
More reviews will be available in 21 minutes and 58 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 (11)
📝 WalkthroughWalkthroughThis PR implements Phase D driver-layer hardening by introducing capability gating to control driver feature availability, a shared driver contract-test harness for validating driver implementations, relocating the Retry utility to a jobs package, adding Postgres connection-retry behavior, and integrating capability checks into CLI commands (backup/sync). New docs/DRIVERS.md guide provides comprehensive driver implementation requirements. ChangesDriver Hardening and Testing Infrastructure
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@docs/DRIVERS.md`:
- Around line 27-35: The fenced code block containing the ASCII architecture
diagram that starts with "internal/cli internal/tui presentation (Cobra ·
Bubble Tea)" is missing a language tag and triggers markdownlint MD040; update
that triple-backtick fence to include a language identifier (e.g., change ``` to
```text) so the block becomes a text code block and the MD040 warning is
resolved.
In `@internal/cli/sync.go`:
- Around line 30-36: The CLI currently defaults --stream to true and
preflight-checks app.CapNativeStream (in the block referencing deps, fromName,
toName), which mismatches internal/app/sync.go where opt.Stream is ignored and
Sync always uses io.Pipe; to fix, change the CLI so it does not require
NativeStream by default: set the --stream flag default to false (or remove the
preflight check), and only call app.RequireCapability(deps, name,
app.CapNativeStream) when you actually intend to use native driver streaming
(i.e., when opt.Stream is true AND the code path will invoke the driver's native
stream API); reference opt.Stream, Sync in internal/app/sync.go, and
app.CapNativeStream/app.RequireCapability when making the conditional.
In `@internal/driver/_testing/suite.go`:
- Around line 81-104: The Cancel_PropagatesToSubprocess test
(Cancel_PropagatesToSubprocess) assumes conn.Backup spawns a long-running
pg_dump and cancels after 150ms, but a small dataset from
BackupRestore_Roundtrip can let pg_dump finish early; to fix, make the subtest
create a larger dump workload before starting conn.Backup (e.g., insert/seed
many more rows or otherwise inflate the DB in this subtest) so pg_dump is almost
certainly in-flight when cancel() is called, or alternatively relax the
assertion after the cancel to accept a nil error if the subprocess already
finished; update the Cancel_PropagatesToSubprocess test code around the
conn.Backup call to perform the seeding (or to change the post-cancel error
check) so it reliably exercises cancellation.
🪄 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: bda4beb6-f53d-4b6c-8395-5e0ddd19845e
📒 Files selected for processing (13)
CHANGELOG.mdREADME.mddocs/DRIVERS.mdinternal/app/capgate.gointernal/app/capgate_test.gointernal/cli/backup.gointernal/cli/sync.gointernal/driver/_testing/fixtures.gointernal/driver/_testing/suite.gointernal/driver/postgres/driver.gointernal/driver/postgres/integration_test.gointernal/jobs/retry.gointernal/jobs/retry_test.go
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.
nixrajput
added a commit
that referenced
this pull request
May 31, 2026
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>
nixrajput
force-pushed
the
feat/implement-phase-d
branch
from
5831efe to
15e24dd
Compare
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.
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests