feat: prepare safe-migrate v0.4.4 - #8
Conversation
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (7)
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
🧹 Nitpick comments (6)
tests/live_auto_sync.rs (1)
99-106: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueQuery
session_useras its own oracle.The test uses
current_useras the expected value for bothsource_roleandsource_session_role. The two differ when the connection sets a role, for example throughoptions=-c role=.... Read both values to keep the assertion exact.♻️ Proposed change
- let expected_role: String = client - .query_one("SELECT current_user", &[]) - .expect("query current_user") - .get(0); - run_auto_sync_case(&database_url, &expected_role, "lint"); - run_auto_sync_case(&database_url, &expected_role, "lint-chain"); + let row = client + .query_one("SELECT current_user, session_user", &[]) + .expect("query role oracle"); + let expected_role: String = row.get(0); + let expected_session_role: String = row.get(1); + run_auto_sync_case(&database_url, &expected_role, &expected_session_role, "lint"); + run_auto_sync_case( + &database_url, + &expected_role, + &expected_session_role, + "lint-chain", + );
run_auto_sync_casethen takesexpected_session_roleand asserts it againstcache.metadata.source_session_role.🤖 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 `@tests/live_auto_sync.rs` around lines 99 - 106, Update the test setup around run_auto_sync_case to query session_user separately from current_user and retain both expected values. Extend run_auto_sync_case to accept expected_session_role, and assert cache.metadata.source_session_role against it while continuing to use expected_role for source_role in both lint cases.tests/live_differential_harness.rs (1)
1055-1107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the role expectations into the manifest and classify missing roles correctly.
Two points on this check:
- The function hardcodes
rule_26_chain-conflictand four role names inside the generic harness. The manifest already carries declarative per-rule expectations such asrequired_relationsandfixture_scopes. Arequired_role_edgesmanifest field would keep fixture data indifferential_manifest.jsonand keep the harness rule-agnostic.- Every failure is reported as
RootCauseClassification::SimulatorBug. A missing role meansdifferential_baseline.sqldid not create it, or the sync role cannot readpg_roles. That is an environment or baseline problem. Classify an absent role separately from an incorrectcan_set_role_toedge so the mismatch report points at the real cause.♻️ Proposed change for point 2
- match role("sm_set_member") { - Some(member) if edge(&member.can_set_role_to, "sm_set_bridge") => {} - _ => errors.push("sm_set_member is missing its SET edge to sm_set_bridge"), - } + let mut absent = Vec::new(); + for name in ["sm_set_member", "sm_set_bridge"] { + if role(name).is_none() { + absent.push(format!("{name} is absent from the synced role catalog")); + } + }Map entries in
absenttoMismatchCategory::BaselineObjectAbsentwithRootCauseClassification::EnvironmentIssue, and keepRoleMembershipMismatch/SimulatorBugfor present roles with wrong edges.🤖 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 `@tests/live_differential_harness.rs` around lines 1055 - 1107, Update check_role_membership_cache to load declarative required_role_edges expectations from differential_manifest.json instead of hardcoding rule_26_chain-conflict and role names, keeping the harness rule-agnostic. Track roles absent from cache separately from roles with incorrect edges; report absent roles as BaselineObjectAbsent with EnvironmentIssue, while retaining RoleMembershipMismatch and SimulatorBug for present roles whose edges are wrong.src/sync.rs (1)
944-965: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAlign
member_ofwith its current meaning.
member_ofcurrently records allpg_auth_membersrows, while its doc comment says it is only inherited roles. On PostgreSQL 16+,pg_auth_membersis independent forset_optionandinherit_option, so a membership without the SET option (for exampleWITH SET FALSE) still populatesmember_of. Either split this into “membership edges” and “inherited privileges” or update the doc comment to “roles this role is a member of”.🤖 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 `@src/sync.rs` around lines 944 - 965, Update the documentation for the role field member_of to describe it as the roles this role is a member of, rather than inherited roles. Keep the existing membership_query processing and can_set_role_to behavior unchanged.src/ast/visitor.rs (1)
2924-2939: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
resolve_string_literalfor the session-authorization literal.This block strips single quotes manually. It handles only plain
STRINGtokens.SET SESSION AUTHORIZATION E'app\x5fuser';and other prefixed string forms returnNone, so the statement produces no fact. The newresolve_string_literalhelper already decodes every supported literal kind and removes theexpectcall.♻️ Proposed refactor
- let role_from_literal = node.literal().and_then(|lit| { - let text = lit.syntax().text().to_string(); - let unescaped = text - .strip_prefix('\'') - .and_then(|inner| inner.strip_suffix('\'')) - .map(|inner| inner.replace("''", "'")); - if unescaped.as_deref().is_none_or(str::is_empty) { - None - } else { - Some(crate::analysis::facts::RoleFact::Named { - name: unescaped.expect("checked above"), - via_legacy_group_syntax: false, - }) - } - }); + let role_from_literal = node + .literal() + .and_then(|lit| Self::resolve_string_literal(&lit)) + .filter(|name| !name.is_empty()) + .map(|name| crate::analysis::facts::RoleFact::Named { + name, + via_legacy_group_syntax: false, + });🤖 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 `@src/ast/visitor.rs` around lines 2924 - 2939, Update the session-authorization literal handling in the visitor to call the existing resolve_string_literal helper instead of manually stripping quotes and unescaping text. Preserve the current empty-string filtering and construct RoleFact::Named with the resolved value, removing the expect-based extraction so prefixed literals such as E strings are supported.src/analysis/state.rs (1)
1571-1591: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract one relation-owner helper. Both sites resolve the
RoleFactidentity, taint confidence for an unknown identity, snapshot the relation, assignObjectId::new("", owner), and report the same conflict text. The duplicated logic can diverge when the owner rules change.
src/analysis/state.rs#L1571-L1591: replace the inlineAlterTableActionMutation::OwnerTobody with a call to the shared helper.src/analysis/state.rs#L2283-L2302: replace the inlineMutation::ChangeRelationOwnerbody with a call to the same helper.♻️ Proposed helper
fn change_relation_owner( &mut self, id: &ObjectId, new_owner: &crate::analysis::facts::RoleFact, ) -> MutationResult { let Some((owner, known)) = self.role_fact_identity(new_owner) else { self.snapshot_confidence(); self.local.confidence = Confidence::Tainted; return MutationResult::Skipped; }; if !known { self.snapshot_confidence(); self.local.confidence = Confidence::Tainted; } self.snapshot_relation(id); match self.local.relations.get_mut(id) { Some(RelationOverlay::Present(relation)) => { relation.owner = ObjectId::new("", owner); MutationResult::Applied } _ => MutationResult::Conflict { reason: format!("relation '{}' does not exist", id), }, } }🤖 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 `@src/analysis/state.rs` around lines 1571 - 1591, Extract a shared change_relation_owner helper in src/analysis/state.rs that performs role identity resolution, confidence tainting, relation snapshotting, owner assignment, and the existing conflict result. Replace the inline OwnerTo logic at src/analysis/state.rs lines 1571-1591 and the ChangeRelationOwner logic at lines 2283-2302 with calls to this helper.tests/state_mutation.rs (1)
706-734: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the test name with the condition under test.
The test never sets a cache version. It only omits
cache.metadata.source_role. A V4 cache with absent role provenance reaches the same state. Rename the test to describe missing role provenance, so the name does not imply version gating that the test does not cover.Consider also asserting that
public.moodkeepsoldafter the tainted resolution. The rename statement currently has no assertion.♻️ Proposed rename
- fn v3_without_role_provenance_taints_explicit_user_search_path() { + fn cache_without_role_provenance_taints_explicit_user_search_path() {🤖 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 `@tests/state_mutation.rs` around lines 706 - 734, Rename v3_without_role_provenance_taints_explicit_user_search_path to describe missing role provenance rather than cache-version behavior. In the same test, add an assertion after analyze confirming that public.mood still contains the old enum value, while preserving the existing tainted-resolution assertions.
🤖 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 `@src/ast/visitor.rs`:
- Around line 1740-1750: Update the AddValue handling in the AST visitor to
derive before from the parsed AddValue placement accessor instead of searching
add_value.syntax().text(). Preserve the resulting AlterTypeActionFact::AddValue
fields while ensuring enum labels containing “before” do not override an AFTER
placement.
In `@tests/cli_tests.rs`:
- Around line 30-33: Ensure the published crate contains the
live_tests/.safe-migrate.cache fixture required by the write_fresh_cache test.
Update the root manifest’s exclude/include configuration to retain this file in
the package, or modify the test setup to use a fallback that does not depend on
the frozen fixture while preserving existing test behavior.
In `@tests/state_mutation.rs`:
- Around line 1941-1948: Update the engine.analyze call in the affected role
test to unwrap its result instead of binding it to _. Match the existing pattern
used by the other role tests, while leaving the SQL and subsequent assertions
unchanged.
---
Nitpick comments:
In `@src/analysis/state.rs`:
- Around line 1571-1591: Extract a shared change_relation_owner helper in
src/analysis/state.rs that performs role identity resolution, confidence
tainting, relation snapshotting, owner assignment, and the existing conflict
result. Replace the inline OwnerTo logic at src/analysis/state.rs lines
1571-1591 and the ChangeRelationOwner logic at lines 2283-2302 with calls to
this helper.
In `@src/ast/visitor.rs`:
- Around line 2924-2939: Update the session-authorization literal handling in
the visitor to call the existing resolve_string_literal helper instead of
manually stripping quotes and unescaping text. Preserve the current empty-string
filtering and construct RoleFact::Named with the resolved value, removing the
expect-based extraction so prefixed literals such as E strings are supported.
In `@src/sync.rs`:
- Around line 944-965: Update the documentation for the role field member_of to
describe it as the roles this role is a member of, rather than inherited roles.
Keep the existing membership_query processing and can_set_role_to behavior
unchanged.
In `@tests/live_auto_sync.rs`:
- Around line 99-106: Update the test setup around run_auto_sync_case to query
session_user separately from current_user and retain both expected values.
Extend run_auto_sync_case to accept expected_session_role, and assert
cache.metadata.source_session_role against it while continuing to use
expected_role for source_role in both lint cases.
In `@tests/live_differential_harness.rs`:
- Around line 1055-1107: Update check_role_membership_cache to load declarative
required_role_edges expectations from differential_manifest.json instead of
hardcoding rule_26_chain-conflict and role names, keeping the harness
rule-agnostic. Track roles absent from cache separately from roles with
incorrect edges; report absent roles as BaselineObjectAbsent with
EnvironmentIssue, while retaining RoleMembershipMismatch and SimulatorBug for
present roles whose edges are wrong.
In `@tests/state_mutation.rs`:
- Around line 706-734: Rename
v3_without_role_provenance_taints_explicit_user_search_path to describe missing
role provenance rather than cache-version behavior. In the same test, add an
assertion after analyze confirming that public.mood still contains the old enum
value, while preserving the existing tainted-resolution assertions.
🪄 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 Plus
Run ID: 03d368f9-9202-427f-a15b-9c9d6d7def12
⛔ Files ignored due to path filters (22)
Cargo.lockis excluded by!**/*.locklive_tests/.safe-migrate.cacheis excluded by!live_tests/**live_tests/README.mdis excluded by!live_tests/**live_tests/differential_baseline.sqlis excluded by!live_tests/**live_tests/differential_manifest.jsonis excluded by!live_tests/**live_tests/rule_20_alter-type-add-value-txn/safe_010_rename_value_search_path.sqlis excluded by!live_tests/**live_tests/rule_20_alter-type-add-value-txn/safe_011_rename_value_quoted.sqlis excluded by!live_tests/**live_tests/rule_20_alter-type-add-value-txn/safe_012_create_then_rename_value.sqlis excluded by!live_tests/**live_tests/rule_22_opaque-dynamic-sql/013_invalid_set_role_current_user.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/012_rename_enum_missing_label.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/013_rename_enum_duplicate_label.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/014_rename_value_non_enum.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/015_rename_value_missing_type.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/016_missing_set_role.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/017_unauthorized_set_role.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/safe_010_role_transaction_semantics.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/safe_011_session_authorization.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/safe_012_role_search_path_and_owner.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/safe_013_quoted_session_authorization.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/safe_014_session_authorization_rollback.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/safe_015_session_authorization_default.sqlis excluded by!live_tests/**live_tests/rule_26_chain-conflict/safe_016_transitive_set_role_membership.sqlis excluded by!live_tests/**
📒 Files selected for processing (26)
.gitignoreCHANGELOG.mdCargo.tomlREADME.mddocs/CONTRACT.mddocs/internal/ARCHITECTURE.mddocs/internal/CACHE.mddocs/internal/TESTING.mdsrc/analysis/facts.rssrc/analysis/mutations.rssrc/analysis/resolver.rssrc/analysis/state.rssrc/analysis/transaction.rssrc/ast/visitor.rssrc/ast/visitor_tests.rssrc/db/cache.rssrc/main.rssrc/model/role.rssrc/sync.rssrc/sync_tests.rstests/cli_tests.rstests/live_auto_sync.rstests/live_cache_encryption.rstests/live_differential_harness.rstests/rule_catalog.rstests/state_mutation.rs
1 participant
Summary
SET ROLE, session authorization, ownership, and PostgreSQL 16+SET OPTIONCompatibility
safe-migrate syncUser impact
cache inspectreports a redacted role count without exposing role names or membership edgessync --out, global--no-color, and the GitHub Action's explicitconfigbehaviorValidation
cargo test --lockedcargo clippy --all-targets --locked -- -D warningscargo fmt -- --checkcargo package --locked --allow-dirtysafe-migrate 0.4.4bash scripts/test-install-dry-runscripts/fuzz: 413 inputs, zero crashes, timeouts, operational errors, or invalid reportsscripts/live-auto-syncagainst PostgreSQL 18.2scripts/live-cache-encryptionagainst PostgreSQL 18.2live_tests/run.sh -v: 517/517 passedscripts/live-differential -v: 301/301 matched PostgreSQL 18.2 in 416.45 secondsSummary by CodeRabbit
New Features
SET ROLE, session authorization, ownership changes, and role-sensitive permissions.Bug Fixes
Documentation
Chores