Add optional Postgres result sink for obacloud integration#4
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 40 minutes and 30 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: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThis PR implements an optional Postgres "result sink" feature for the validator. It adds a new ChangesResult Sink Implementation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 2
🤖 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 `@sink/sink_integration_test.go`:
- Line 41: Defer currently discards the result of admin.Close(ctx); wrap it in a
deferred anonymous function that captures the error from admin.Close(ctx) and
fails or logs the test appropriately (e.g., t.Fatalf or t.Errorf) so teardown
failures are surfaced; locate the defer using the admin.Close and ctx
identifiers in the test and replace it with a defer func() { if err :=
admin.Close(ctx); err != nil { t.Fatalf("admin.Close error: %v", err) } }() (or
t.Errorf) to ensure the Close error is handled.
In `@sink/sink.go`:
- Around line 154-166: The Write method currently accepts any status string; add
a guard at the start of Config.Write to enforce the allowed status vocabulary
(only "completed" and "error") and return a descriptive error for any other
value before performing DB I/O or calling Validate/allowedTables; update
Config.Write to check the status parameter (e.g., compare against the allowed
set) and fail fast, referencing the Config.Write function, the status parameter,
and ResultTable/allowedTables so callers cannot persist arbitrary statuses.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 458d1f5c-686a-4abc-bb93-55ff1b864669
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
CLAUDE.mdcmd/oba-validator/main.gocmd/oba-validator/main_test.goconfig/config.goconfig/config_test.godocs/superpowers/plans/2026-05-25-result-sink-implementation.mddocs/superpowers/specs/2026-05-25-result-sink-design.mdgo.modreport/report.gosink/dsn.gosink/sink.gosink/sink_integration_test.gosink/sink_test.go
CodeRabbit findings on PR #4: - sink_integration_test.go was discarding admin.Close(ctx)'s error via a bare defer; wrap it in an anonymous func that t.Errorf's on error so teardown failures surface in CI. - sink.Write accepted any status string and would happily persist a row with status='whatever'. Add a guard at the top of Write that rejects anything other than 'completed' or 'error' before opening a connection, with a new TestWriteRejectsUnsupportedStatus exercising the guard.
- S1192: extract sinkWriteFailedMsg const for the 4 'result sink write failed:' stderr prefixes in cmd/oba-validator/main.go. - S3776: lower TestNormalizeDSN cognitive complexity by extracting the per-case assertions into a checkDSNResult helper. - Duplication: replace the 5 near-identical httptest stub-server setups in cmd/oba-validator/main_test.go with newStubOBA(t), newStubFeed(t), and sinkCfgJSON(...) helpers. Cuts ~60 duplicated lines, dropping the diff's duplicated-line density below SonarCloud's 3% threshold.
|
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
Adds an optional Postgres "result sink" so a caller (specifically obacloud's
ServerValidationJob) can read a validator run's report back from a database after a Render one-off job finishes, instead of scraping stdout. The validator still prints to stdout and exits with its existing codes — the DB write is purely additive and only activates when the caller supplies the new five sink fields in the input config.sink/package:Config+Configured/Validate/Write,normalizeDSNforjdbc:URL handling,pgx/v5for the actual write.config.Configparses five flat snake_case JSON fields (db_url,db_user,db_pass,correlation_id,result_table) and rejects partial sink configs at config-load time.reportexposesRenderJSON/RenderErrorJSONbyte-returning helpers so the same JSON goes to stdout and to the sink.main.gowrites one row per run after stdout:status="completed"for both PASS and FAIL verdicts (the verdict lives atsummary.verdictinsideresult_data);status="error"is reserved for theerrorDocumentvariant. Sink failures are logged to stderr and never alter the validator's exit code.Contract is locked — it's an upstream dependency of obacloud PR #747.
Design context:
docs/superpowers/specs/2026-05-25-result-sink-design.mddocs/superpowers/plans/2026-05-25-result-sink-implementation.mdBackward compatibility
Stdout output, exit codes, the published report schema, and CLI flags are all unchanged. Pre-sink invocations (no
db_urlin the payload) behave byte-identically to today;TestRunSkipsSinkWhenNotConfiguredpins this.Security
db_passis scrubbed in three layers:sink.redactErr(raw + URL-encoded for pgx error echoes),main.redactionSecrets/scrubfor inline raw-arg errors, andcfg.DBPassis threaded into the validator-error scrub.result_tableis enforced against a closed allow-list ({"oba_validator_results"}) both at config-load time and again insidesink.Write(defense in depth).normalizeDSNdefaultssslmode=requireandconnect_timeout=5.Test plan
make test— all unit tests pass (sink integration test skips withoutOBA_VALIDATOR_DB_DSN).go vet ./...andgofmt -l .clean.make buildsucceeds.TestRunJSONConfigErrorRedactsInlineAPIKey,TestRunTextConfigErrorRedactsInlineAPIKey) still pass.TestRunInvokesSinkOnCompleted,TestRunSkipsSinkWhenNotConfigured,TestRunSinkErrorDoesNotAlterExitCode,TestRunRenderJSONFailureWritesSinkErrorRow.OBA_VALIDATOR_DB_DSN="postgres://postgres@127.0.0.1:5432/postgres?sslmode=disable" go test ./sink/ -run TestWriteIntegration -v.Suggested follow-ups (non-blocking)
Surfaced by the final whole-branch review; safe to defer to a follow-up PR:
WriteText → RenderJSON-for-sinkbranch.validatorRunpackage-level var so the validator-error path can be exercised in tests (it's currently dead code becausevalidator.Runnever returns a non-nil error today).OBA_VALIDATOR_DB_DSNinCLAUDE.md's Commands section and add amake test-sink-integrationtarget.README.mddescribing the optional sink activation.Deployment
The new image must be deployed to the existing Render service
crn-d8a9p20jo6nc73e9h9jgbefore obacloud PR #747 is usable end-to-end. The image tag,render.yaml, and Render service id are unchanged. During the deploy gap, obacloud times out cleanly with "No result found for correlation_id" — no data loss.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes