Conversation
…heck After schema migrations run, verify expected tables exist in tenant schema before transitioning to active state. This closes the partial-provisioning gap where the schema namespace exists but migrations failed silently. Each service can define a SentinelTable (the primary domain table created by its first migration). If set, that specific table is checked. If not set, the service is assumed to have no required tables (e.g., internal-account). Default service configs now include sentinel tables for all services with migrations.
Contributor
📝 WalkthroughWalkthroughAdds post-migration sentinel-table verification to tenant provisioning: a new sentinel error, a verification helper that checks for expected tables in the tenant schema, and integration of that check into the ProvisionSchemas flow with updated service config to specify sentinel tables. Changes
Sequence Diagram(s)sequenceDiagram
participant Provisioner as Provisioner
participant Migrator as Migration Engine
participant DB as Postgres
participant Store as Provisioning Store
participant Logger as Logger
Provisioner->>Migrator: createAndMigrateSchemas(schema)
Migrator->>DB: apply migrations in tenant schema
DB-->>Migrator: migration results
Migrator-->>Provisioner: migrations succeeded
Provisioner->>DB: SELECT EXISTS(...) for each SentinelTable
DB-->>Provisioner: table exists / not exists or error
alt any verification failure
Provisioner->>Logger: log verification error
Provisioner->>Store: markProvisioningFailed(schema, reason)
Store-->>Provisioner: state=failed
Provisioner-->>Logger: return error (ErrSchemaVerificationFailed)
else all verifications passed or skipped
Provisioner->>Store: markProvisioningActive(schema)
Store-->>Provisioner: state=active
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Claude Code ReviewCommit: SummaryClean, well-scoped PR that closes a real gap: detecting partial provisioning where the schema namespace exists but migrations failed silently. The sentinel table pattern is a good fit -- lightweight verification that each service's primary domain table landed. Tests cover the four key scenarios (present, missing, no-sentinel-with-tables, empty-schema-no-sentinel). The refactor from Second commit correctly aligns the Risk Assessment
Findings
Previously Flagged
Bot Review Notes
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@services/tenant/provisioner/provisioner_helpers.go`:
- Around line 164-178: The current "no sentinel" branch queries tableCount but
never fails when tableCount == 0; update the else branch that runs
serviceDB.WithContext(bypassCtx(ctx)).Raw(...).Scan(&tableCount).Error to treat
an empty schema as a verification failure when the service has no configured
sentinel: after the query (and after handling err), if tableCount == 0 and
svc.Config.SentinelTable == "" then logger.Error with context ("service",
svc.Name, "table_count", tableCount) and append failedServices with a
descriptive message (e.g. fmt.Sprintf("%s (no tables found, missing sentinel
config)", svc.Name)) so missed/empty SentinelTable no longer silently passes
verification.
In `@services/tenant/provisioner/provisioner.go`:
- Around line 301-305: The comment for SentinelTable says an empty value should
trigger a "at least one table exists" check but the post-migration verification
logic currently allows zero tables; reconcile them by either updating the
comment to reflect that zero tables are permitted or changing the verification
code to enforce tablesCount > 0 when SentinelTable == "". Locate the
post-migration verification routine (the code that inspects schema table count
after migrations) and: if you choose behavioral fix, add a conditional that when
SentinelTable == "" validates that the discovered table count is >= 1 and fails
otherwise; if you choose documentation fix, update the comment on SentinelTable
to accurately describe the current verification behavior. Ensure the change
references SentinelTable so future readers see the intended contract.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8505057e-d810-41ab-ba3f-a68ad4ec492d
📒 Files selected for processing (5)
services/tenant/provisioner/errors.goservices/tenant/provisioner/postgres_provisioner.goservices/tenant/provisioner/postgres_provisioner_test.goservices/tenant/provisioner/provisioner.goservices/tenant/provisioner/provisioner_helpers.go
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
…uery The doc comment promised "at least one table exists" verification when SentinelTable is empty, but the code intentionally allowed zero tables for services without migrations. Fix the doc to match the behavior and remove the unused COUNT query.
bjcoombs
dismissed
coderabbitai[bot]’s stale review
Stale bot review - findings addressed in 552cd38
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/tenant/provisioner/provisioner_helpers.go (1)
133-137: Treat missing service DB handles as verification failures instead of silently skipping.If
p.serviceDbsever drifts fromp.config.Services, this path can incorrectly pass verification and allow activation despite an unverified service.Proposed change
for _, svc := range p.config.Services { serviceDB, ok := p.serviceDbs[svc.Name] if !ok { - continue // Already caught by provisionSingleService + logger.Error("service database handle missing during schema verification", + "service", svc.Name, + "schema", schemaName) + failedServices = append(failedServices, fmt.Sprintf("%s (service DB unavailable)", svc.Name)) + continue }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@services/tenant/provisioner/provisioner_helpers.go` around lines 133 - 137, The loop over p.config.Services silently skips when a service DB handle is missing (serviceDB, ok := p.serviceDbs[svc.Name] ... continue), which can hide verification failures; change this to treat a missing entry as a verification failure by returning an error (or a failed verification result) instead of continuing. Update the verification path in the function containing that loop (referencing p.config.Services, p.serviceDbs and provisionSingleService) so that when ok is false you log/return a clear error indicating the missing service DB handle and fail the overall verification/activation flow. Ensure callers of this function handle the error consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@services/tenant/provisioner/provisioner_helpers.go`:
- Around line 133-137: The loop over p.config.Services silently skips when a
service DB handle is missing (serviceDB, ok := p.serviceDbs[svc.Name] ...
continue), which can hide verification failures; change this to treat a missing
entry as a verification failure by returning an error (or a failed verification
result) instead of continuing. Update the verification path in the function
containing that loop (referencing p.config.Services, p.serviceDbs and
provisionSingleService) so that when ok is false you log/return a clear error
indicating the missing service DB handle and fail the overall
verification/activation flow. Ensure callers of this function handle the error
consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 42d93a7c-91f2-4621-9d46-140f7103af72
📒 Files selected for processing (2)
services/tenant/provisioner/provisioner.goservices/tenant/provisioner/provisioner_helpers.go
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
activestateSentinelTablefield onServiceConfig- each service declares its primary domain table as a sentinelparty,account,financial_position_log,financial_booking_log,payment_order,data_source,instrument_definitioninternal-account,reconciliation) have no sentinel and pass verification automaticallyfailedstate with detailed error message listing which services/tables are missingTest plan
ErrSchemaVerificationFailed