refactor(meta): migrate Auth/loader/codegen readers to effective ModelId - #247
Conversation
- Resolve meta_model via LookupEffectiveModel (no tip Order) in loader, bootstrap, and dual-store ACL remap after migrate/recompute. - Point Auth FieldRule/RecordRule/MethodAccess at a single effective ModelId and add MetaModelRaw TS facades for declaration reads. - Load codegen/webBuilder from effective-only rows (empty module_id) without in-memory IMD merge. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughThe PR centralizes effective-model lookup, updates metadata and authorization consumers, excludes declaration shells from artifact generation, adds raw metadata entities, and remaps ACL references after effective-model migrations. ChangesEffective model resolution
ACL reference remapping
Raw metadata model entities
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DualStoreMigration
participant RemapACLToEffectiveModelIDs
participant MetadataDatabase
participant AuthTables
DualStoreMigration->>RemapACLToEffectiveModelIDs: invoke after effective unique index
RemapACLToEffectiveModelIDs->>MetadataDatabase: resolve effective models, fields, and services
RemapACLToEffectiveModelIDs->>AuthTables: update model, field, and service references
AuthTables-->>DualStoreMigration: return remapping result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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
🧹 Nitpick comments (3)
pkg/meta/acl_remap.go (2)
98-98: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
oldToEffectiveparameter.
remapFieldRuleFieldIDsnever readsoldToEffective. Line 144 discards it inside the loop on every iteration. This is a dead artifact. Remove the parameter and update the call site at line 92.♻️ Proposed cleanup
-func remapFieldRuleFieldIDs(db *gorm.DB, oldToEffective map[string]string) error { +func remapFieldRuleFieldIDs(db *gorm.DB) error {} - _ = oldToEffective // retained for call-site clarity }Call site at line 92:
- if err := remapFieldRuleFieldIDs(db, oldToEffective); err != nil { + if err := remapFieldRuleFieldIDs(db); err != nil {Also applies to: 144-144
🤖 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 `@pkg/meta/acl_remap.go` at line 98, Remove the unused oldToEffective parameter from remapFieldRuleFieldIDs and update its call site accordingly. Delete the loop statement that discards oldToEffective, while preserving the function’s existing remapping behavior.
73-95: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrap the ACL rewrites in one transaction.
The three model-ID updates, the field-rule remap, and the service remap run as separate statements outside a transaction. If a later statement fails, the ACL tables keep a partial mapping. The function is idempotent, so a re-run repairs the state, but only if an operator notices the error.
Wrap the mutation phase in
db.Transactionso the ACL rewrite commits atomically.🤖 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 `@pkg/meta/acl_remap.go` around lines 73 - 95, Wrap the entire mutation phase in pkg/meta/acl_remap.go’s visible remap flow, including the table updates, remapFieldRuleFieldIDs, and remapOrphanServices, in a single db.Transaction callback. Return each error from the callback so any failure rolls back all ACL changes, and only return the transaction result after it commits successfully.pkg/meta/acl_remap_test.go (1)
126-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the idempotency check and cover the service remap path.
Two gaps:
- The second
RemapACLToEffectiveModelIDscall only asserts that no error occurs. Re-readmeta_model_idandmeta_field_idafter the second call to prove the values stay ateff-idandnew-field.remapOrphanServicesis never exercised. The seededauth_role_method_accessrow has nometa_service_id, so the whole function returns after the first row filter. Seed aServiceundershell-idand a matchingServiceundereff-id, setmeta_service_idonma1, then assert the remap to the effective service ID.💚 Proposed idempotency assertion
// Idempotent. if err := RemapACLToEffectiveModelIDs(db); err != nil { t.Fatalf("second RemapACL: %v", err) } + var rr2, fr2Model, fr2Field string + if err := db.Raw(`SELECT meta_model_id FROM auth_role_record_rule WHERE id = ?`, "rr1").Scan(&rr2).Error; err != nil { + t.Fatalf("re-read rr: %v", err) + } + if err := db.Raw(`SELECT meta_model_id, meta_field_id FROM auth_role_field_rule WHERE id = ?`, "fr1").Row().Scan(&fr2Model, &fr2Field); err != nil { + t.Fatalf("re-read fr: %v", err) + } + if rr2 != "eff-id" || fr2Model != "eff-id" || fr2Field != "new-field" { + t.Fatalf("second run changed values: rr=%q frModel=%q frField=%q", rr2, fr2Model, fr2Field) + } if err := RemapACLToEffectiveModelIDs(nil); err == nil {🤖 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 `@pkg/meta/acl_remap_test.go` around lines 126 - 132, Strengthen the test around the second RemapACLToEffectiveModelIDs call by re-reading the affected ACL records and asserting meta_model_id remains eff-id and meta_field_id remains new-field. Extend the test fixtures to seed a Service under shell-id and its matching effective Service under eff-id, assign ma1’s meta_service_id to the shell service, and assert the remap updates it to the effective service ID, thereby exercising remapOrphanServices.
🤖 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 `@modules/auth/service/models/_resolve_effective_model.ts`:
- Around line 36-47: Ensure the effective-model lookup always includes ModuleId
in the fields passed to MetaModel.Search, even when the caller-provided fields
array omits it. Update the surrounding resolver logic for the effective-row
selection, preserving the caller’s requested fields while guaranteeing ModuleId
is available to moduleIdEmpty().
In `@pkg/meta/acl_remap.go`:
- Around line 169-198: The ACL remap helpers currently suppress all GORM Take
errors; import errors and in pkg/meta/acl_remap.go lines 169-198 update the
service lookups at lines 170, 188, and 196 to continue only for errors.Is(err,
gorm.ErrRecordNotFound), returning other errors wrapped with the service ID.
Apply the same handling in lines 121-134 at lookups 123 and 132, wrapping
non-not-found errors with the field-rule ID; no other sites require changes.
---
Nitpick comments:
In `@pkg/meta/acl_remap_test.go`:
- Around line 126-132: Strengthen the test around the second
RemapACLToEffectiveModelIDs call by re-reading the affected ACL records and
asserting meta_model_id remains eff-id and meta_field_id remains new-field.
Extend the test fixtures to seed a Service under shell-id and its matching
effective Service under eff-id, assign ma1’s meta_service_id to the shell
service, and assert the remap updates it to the effective service ID, thereby
exercising remapOrphanServices.
In `@pkg/meta/acl_remap.go`:
- Line 98: Remove the unused oldToEffective parameter from
remapFieldRuleFieldIDs and update its call site accordingly. Delete the loop
statement that discards oldToEffective, while preserving the function’s existing
remapping behavior.
- Around line 73-95: Wrap the entire mutation phase in pkg/meta/acl_remap.go’s
visible remap flow, including the table updates, remapFieldRuleFieldIDs, and
remapOrphanServices, in a single db.Transaction callback. Return each error from
the callback so any failure rolls back all ACL changes, and only return the
transaction result after it commits successfully.
🪄 Autofix
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: 5cd7c8d5-7d48-4008-91bc-f1c79ec8d93f
📒 Files selected for processing (35)
internal/bootstrap/service/coordinator.gointernal/module/artifact/build/web/webBuilder.gointernal/module/artifact/generate/generator.gointernal/module/artifact/generate/generator_test.gointernal/module/evolution/data/loader.gointernal/module/evolution/data/translated_seed.gomodules/auth/service/models/_resolve_effective_model.tsmodules/auth/service/models/_user_field_rule_eval.tsmodules/auth/service/models/_user_method_access.tsmodules/auth/service/models/_user_permission_state_acl.tsmodules/auth/service/models/_user_record_rule_eval.tsmodules/auth/service/tests/authz_context_memoization.test.tsmodules/auth/service/tests/authz_mutation_crud_coverage.test.tsmodules/auth/service/tests/bootstrap_gift_pack.test.tsmodules/auth/service/tests/check_method_access_company_scope.test.tsmodules/auth/service/tests/check_method_access_diagnostics.test.tsmodules/auth/service/tests/field_rule.test.tsmodules/auth/service/tests/permission_state.test.tsmodules/auth/service/tests/record_rule.test.tsmodules/auth/service/tests/record_rule_eval_edges.test.tsmodules/auth/service/tests/role_ui_resource_sync.test.tsmodules/document/service/tests/_owner_auth_test_fixtures.tsmodules/meta/service/models/argument_raw.tsmodules/meta/service/models/decorator_raw.tsmodules/meta/service/models/field_raw.tsmodules/meta/service/models/index.tsmodules/meta/service/models/model_raw.tsmodules/meta/service/models/parameter_raw.tsmodules/meta/service/models/service_raw.tsmodules/meta/service/models/type_parameter_raw.tspkg/meta/acl_remap.gopkg/meta/acl_remap_test.gopkg/meta/dual_store_migrate.gopkg/meta/lookup_effective.gopkg/meta/lookup_effective_test.go
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
- Always fetch ModuleId/UpdatedAt in the Auth effective resolver and mirror Go Id/UpdatedAt tie-breaks when shells coexist. - Remap field ACL FKs even when model ids are already effective; treat only ErrRecordNotFound as skip; wrap ACL rewrites in one transaction. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f5eb589. Configure here.
- Add ACL remap hooks and branch/error coverage for remap, migrate, and lookup. - Cover Auth effective resolve, field-rule empty appId, ACL dedupe, and bootstrap admin lookup paths. Co-authored-by: Cursor <cursoragent@cursor.com>
- Split compound branches that Codecov marked partial and drop dead optional chaining. - Extend resolve/ACL unit tests for ModuleId/UpdatedAt edge cases and null Search rows. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
pkg/meta/acl_remap_coverage_test.go (5)
92-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis subtest verifies no error, not the documented behavior.
The name is
skip_invalid_live_and_deleted_rows, and the fixtures set up a soft-deletedshellfor the same(auth, User)key aseff, plus a duplicate liveeff2. The only assertion is thatRemapACLToEffectiveModelIDsreturns nil. The test passes if the function skips every row, and it passes if the function picks the wrong effective model betweeneffandeff2.
seedACLTablescreated the ACL tables, but no ACL rows were inserted, so nothing observable is produced. Insert a rule that points atshelland assert itsmeta_model_idafterwards.pickEffectiveAmongmust chooseeffovereff2here, becauseeffhas an emptyModuleId; asserting the resulting id locks that tie-break in.🤖 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 `@pkg/meta/acl_remap_coverage_test.go` around lines 92 - 116, Strengthen the skip_invalid_live_and_deleted_rows subtest by inserting an ACL rule referencing shell, then assert after RemapACLToEffectiveModelIDs that its meta_model_id points to eff. Keep the invalid-id fixture and duplicate live models, ensuring the assertion verifies pickEffectiveAmong selects eff over eff2 because eff has an empty ModuleId.
283-299: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert the rollback that this subtest is positioned to prove.
exec_field_id_errorlets themeta_model_idUPDATEs succeed throughprev(db, sql, values...)and then fails themeta_field_idUPDATE. That is the exact shape needed to prove the transaction added inRemapACLToEffectiveModelIDsreverts partial work.The subtest only checks the error string. Read one of the model-id columns after the call and assert it holds the pre-call value. Without that assertion, a regression that drops
db.Transactionand calls the helpers ondbstill passes every test in this file.🤖 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 `@pkg/meta/acl_remap_coverage_test.go` around lines 283 - 299, Extend the exec_field_id_error subtest after RemapACLToEffectiveModelIDs returns to query a model-id column affected by the earlier successful update, such as meta_model_id, and assert it still has its pre-call value. Keep the existing error assertion and use the test’s established database helpers, ensuring the check proves the transaction rolled back partial updates.
183-194: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the debugging note and seed
fr-no-repldirectly.The trailing comment on line 187 records a train of thought rather than intent: "of under shell; no Login on wrong model? Wait of name Login exists on eff as nf ... Use field only on shell with unique name." A future reader cannot tell what the line asserts.
Lines 187, 189-193 also insert
fr-no-replwithof, create thelonelyfield, then UPDATE the row tolonely. Createlonelybefore the insert and seed the final value once.♻️ Proposed cleanup
mustExecACL(`INSERT INTO auth_role_field_rule (id, meta_model_id, meta_field_id) VALUES (?,?,?)`, "fr-blank-name", "eff-h", "bnf") - mustExecACL(`INSERT INTO auth_role_field_rule (id, meta_model_id, meta_field_id) VALUES (?,?,?)`, "fr-no-repl", "eff-h", "of") // of under shell; no Login on wrong model? Wait of name Login exists on eff as nf - Take by model+name finds nf. Use field only on shell with unique name. - // Replace fr-no-repl: field with name only on shell. + // "Lonely" exists only under the shell model, so no replacement field + // resolves under the effective model and the rule is skipped. lonely := &Field{BaseModel: BaseModel{Id: sql.NullString{String: "lonely", Valid: true}}, Name: "Lonely", ModelId: shell.Id} if err := db.Create(lonely).Error; err != nil { t.Fatalf("lonely: %v", err) } - mustExecACL(`UPDATE auth_role_field_rule SET meta_field_id=? WHERE id=?`, "lonely", "fr-no-repl") + mustExecACL(`INSERT INTO auth_role_field_rule (id, meta_model_id, meta_field_id) VALUES (?,?,?)`, "fr-no-repl", "eff-h", "lonely") mustExecACL(`INSERT INTO auth_role_field_rule (id, meta_model_id, meta_field_id) VALUES (?,?,?)`, "fr-same", "eff-h", "nf")🤖 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 `@pkg/meta/acl_remap_coverage_test.go` around lines 183 - 194, Remove the trailing debugging comment from the fr-no-repl seed in the ACL coverage test. In the surrounding setup, create the lonely Field before inserting fr-no-repl, then insert that rule once with lonely as meta_field_id and remove the subsequent UPDATE, preserving the existing fr-same setup.
424-431: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the errors from these fixture writes.
Lines 424, 425, 430, 431, 446, and 449 discard the result of
db.Createanddb.Exec. If a fixture write fails, the subtest still reports success, because the only assertion is thatRemapACLToEffectiveModelIDsreturns nil, and it returns nil when there is no data.Every other subtest in this file fails fast with
t.Fatalf. Use the same pattern here, or add a smallmustCreate(t, db, v)helper and use it across the file.Also applies to: 446-449
🤖 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 `@pkg/meta/acl_remap_coverage_test.go` around lines 424 - 431, Check and fail fast on every fixture write in the affected subtest: replace ignored results from db.Create and db.Exec, including the writes around shell, eff, svc, and auth_role_method_access, with the file’s existing t.Fatalf pattern or a shared mustCreate(t, db, v) helper. Apply the same handling to the additional writes at the referenced later locations so fixture setup errors cannot be masked.
372-409: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
invalid_eff_id_in_mapdoes not reach the branch it names, and it contains a dead assignment.Line 379 assigns
aclLoadLiveModels. Line 394 reassigns it before any call toRemapACLToEffectiveModelIDs. The first stub is dead.The surviving stubs are also mismatched.
aclLoadLiveModels(line 394) returns the keysauth/Zandauth/BadEff.aclLoadAllModels(line 385) returns rows forauth/Userandauth/MissingEff. No key is present in both, sooldToEffectivestays empty and the function performs no rewrite. The only assertion is that the error is nil, which a no-op satisfies.The comments at lines 389, 393, 398, and 405 confirm this. Line 405 states "Replace BadEff's entry: can't easily."
The named branch is
!eff.Id.Validatpkg/meta/acl_remap.goline 112. Reaching it needseffectiveByKeyto hold an entry with an invalidId, which the builder at line 82 prevents. The branch is therefore unreachable through the public function.Two options. Delete the subtest and remove the now-unreachable
!eff.Id.Validguard inacl_remap.goline 112. Or keep the guard as defensive code and replace this subtest with a comment that records why it is not covered. Either way, remove the abandoned attempts and the dead assignment.🤖 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 `@pkg/meta/acl_remap_coverage_test.go` around lines 372 - 409, The invalid_eff_id_in_map subtest is dead code and cannot reach the !eff.Id.Valid branch because effectiveByKey only stores models with valid IDs. Remove the abandoned stubs, conflicting reassignment, and ineffective assertions; either delete the subtest and the unreachable guard in RemapACLToEffectiveModelIDs, or retain the guard while replacing the test with a concise comment documenting that the branch is unreachable through the public function.pkg/meta/acl_remap.go (3)
170-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUnresolvable ACL references are skipped with no signal to the operator. Both remap helpers use
continuefor every reference they cannot resolve. The row keeps its stale foreign key, the transaction commits, andMigrateIMDCatalogToDualStoreandRecomputeAllEffectiveFromRawreport success. A stalemeta_field_idormeta_service_idis an ACL rule that no longer matches, so permissions change silently and no output shows it happened.
pkg/meta/acl_remap.go#L170-L191: count the three skip reasons inremapFieldRuleFieldIDs— old field row missing (line 174), old field name blank (line 180), no replacement field under(modelID, name)(line 185). Return or log the totals with the rule IDs.pkg/meta/acl_remap.go#L254-L266: count the skip reasons inremapOrphanServicesthe same way — effective model not found (line 250), effective id invalid (line 255), no replacement service under(eff.Id, name)(line 260), replacement id invalid or unchanged (line 265). Report the totals with the method-access IDs.Aggregate both counts into one summary that the migration emits. The skip behavior itself is the right default; the missing part is the record of it.
🤖 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 `@pkg/meta/acl_remap.go` around lines 170 - 191, Add structured counters and rule/access IDs to the skip paths in remapFieldRuleFieldIDs (missing old field, blank name, missing replacement) and remapOrphanServices (missing model, invalid IDs, missing or unchanged replacement). Preserve the continue behavior, aggregate both helpers’ counts into one summary, and have MigrateIMDCatalogToDualStore and RecomputeAllEffectiveFromRaw emit that summary.
127-139: 🚀 Performance & Scalability | 🔵 Trivial | ⚖️ Poor tradeoffThe model remap issues one UPDATE per table per mapping.
oldToEffectiveholds one entry for every historical shell that resolves to a different effective model. The loop runs3 × len(oldToEffective)UPDATE statements inside a single transaction. On a large catalog after a full recompute, that map can hold one entry per replaced model per module generation.A single statement per table removes the round-trip cost:
♻️ Batch the rewrite with a temporary mapping table
+ if len(oldToEffective) > 0 { + if err := aclExec(tx, `CREATE TEMP TABLE acl_model_remap (old_id TEXT PRIMARY KEY, new_id TEXT)`); err != nil { + return fmt.Errorf("create remap temp table: %w", err) + } + for oldID, newID := range oldToEffective { + if err := aclExec(tx, `INSERT INTO acl_model_remap (old_id, new_id) VALUES (?, ?)`, oldID, newID); err != nil { + return fmt.Errorf("seed remap temp table: %w", err) + } + } + }Then each table needs one statement:
UPDATE <table> SET meta_model_id = (SELECT new_id FROM acl_model_remap WHERE old_id = meta_model_id) WHERE meta_model_id IN (SELECT old_id FROM acl_model_remap)The current form is correct. Apply this only if catalog size makes the migration slow.
🤖 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 `@pkg/meta/acl_remap.go` around lines 127 - 139, Only optimize the remap path if migration performance requires it: in the transaction surrounding the table loop, create and populate a temporary acl_model_remap table from oldToEffective, then update each table once using that mapping table to replace matching meta_model_id values. Preserve aclHasTable checks, transaction/error handling, and the existing behavior when no mappings exist.
14-63: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider moving the injection seam out of package-level mutable globals.
The hooks ship in the production binary as package-level variables. Any test that reassigns them mutates shared state for the whole package. The current tests restore each hook with
t.Cleanup, so the behavior is correct today, but the pattern blockst.Parallel()inpkg/metatests and it lets a missedCleanupleak a stub into unrelated tests.A struct of function fields with a package default, or an unexported interface passed into the remap functions, gives the same error-path coverage with no shared mutable state.
🤖 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 `@pkg/meta/acl_remap.go` around lines 14 - 63, Replace the package-level mutable hook variables in the ACL remap injection seam with per-invocation dependencies: define a hook struct or unexported interface containing the existing operations, provide production defaults, and pass it through the relevant remap functions. Update tests to construct overrides rather than reassign shared globals, preserving existing error-path coverage and enabling parallel execution.pkg/meta/lookup_effective_test.go (1)
85-130: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd an order-independence case for
pickEffectiveAmong.The four cases cover each precedence level: single row,
UpdatedAt,Id, and emptyModuleIdoutrankingUpdatedAt. Each passes one fixed slice order.
RemapACLToEffectiveModelIDsdepends on more than the precedence order. Atpkg/meta/acl_remap.goline 86 it folds pairwise over map iteration:picked := pickEffectiveAmong([]Model{existing, row})Map iteration order is random in Go. The fold produces the same result as
LookupEffectiveModel, which passes all rows at once, only if the comparison is a total order. Assert that directly by running each existing case with the slice reversed and expecting the same id, and by adding one three-element case compared against both a forward fold and a reverse fold.🤖 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 `@pkg/meta/lookup_effective_test.go` around lines 85 - 130, Extend the “single_row_and_tie_breaks” tests for order independence by rerunning each existing two-row precedence case with its slice reversed and asserting the same selected ID. Add a three-element case and verify that pickEffectiveAmong returns the same ID as both forward and reverse pairwise folds, matching the all-at-once result and confirming total-order behavior.
🤖 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 `@modules/auth/service/models/_resolve_effective_model.ts`:
- Around line 92-94: Remove the fixed limit from the candidate query used before
pickEffectiveAmong, and fetch all matching declaration shells through the
existing pagination mechanism instead. Ensure older effective rows with empty
ModuleId values remain eligible, and add a regression case covering more than 50
shells with one older effective row.
In `@pkg/meta/acl_remap.go`:
- Around line 236-253: Before calling aclLookupEffective in the service remap
loop, skip the historical model when hist.Application or hist.Name is blank,
matching the existing blank-key handling in the main loop. Update the logic
after aclTakeModelUnscoped and before aclLookupEffective so invalid historical
lookup keys continue without aborting the remap; preserve existing error
handling for valid keys.
---
Nitpick comments:
In `@pkg/meta/acl_remap_coverage_test.go`:
- Around line 92-116: Strengthen the skip_invalid_live_and_deleted_rows subtest
by inserting an ACL rule referencing shell, then assert after
RemapACLToEffectiveModelIDs that its meta_model_id points to eff. Keep the
invalid-id fixture and duplicate live models, ensuring the assertion verifies
pickEffectiveAmong selects eff over eff2 because eff has an empty ModuleId.
- Around line 283-299: Extend the exec_field_id_error subtest after
RemapACLToEffectiveModelIDs returns to query a model-id column affected by the
earlier successful update, such as meta_model_id, and assert it still has its
pre-call value. Keep the existing error assertion and use the test’s established
database helpers, ensuring the check proves the transaction rolled back partial
updates.
- Around line 183-194: Remove the trailing debugging comment from the fr-no-repl
seed in the ACL coverage test. In the surrounding setup, create the lonely Field
before inserting fr-no-repl, then insert that rule once with lonely as
meta_field_id and remove the subsequent UPDATE, preserving the existing fr-same
setup.
- Around line 424-431: Check and fail fast on every fixture write in the
affected subtest: replace ignored results from db.Create and db.Exec, including
the writes around shell, eff, svc, and auth_role_method_access, with the file’s
existing t.Fatalf pattern or a shared mustCreate(t, db, v) helper. Apply the
same handling to the additional writes at the referenced later locations so
fixture setup errors cannot be masked.
- Around line 372-409: The invalid_eff_id_in_map subtest is dead code and cannot
reach the !eff.Id.Valid branch because effectiveByKey only stores models with
valid IDs. Remove the abandoned stubs, conflicting reassignment, and ineffective
assertions; either delete the subtest and the unreachable guard in
RemapACLToEffectiveModelIDs, or retain the guard while replacing the test with a
concise comment documenting that the branch is unreachable through the public
function.
In `@pkg/meta/acl_remap.go`:
- Around line 170-191: Add structured counters and rule/access IDs to the skip
paths in remapFieldRuleFieldIDs (missing old field, blank name, missing
replacement) and remapOrphanServices (missing model, invalid IDs, missing or
unchanged replacement). Preserve the continue behavior, aggregate both helpers’
counts into one summary, and have MigrateIMDCatalogToDualStore and
RecomputeAllEffectiveFromRaw emit that summary.
- Around line 127-139: Only optimize the remap path if migration performance
requires it: in the transaction surrounding the table loop, create and populate
a temporary acl_model_remap table from oldToEffective, then update each table
once using that mapping table to replace matching meta_model_id values. Preserve
aclHasTable checks, transaction/error handling, and the existing behavior when
no mappings exist.
- Around line 14-63: Replace the package-level mutable hook variables in the ACL
remap injection seam with per-invocation dependencies: define a hook struct or
unexported interface containing the existing operations, provide production
defaults, and pass it through the relevant remap functions. Update tests to
construct overrides rather than reassign shared globals, preserving existing
error-path coverage and enabling parallel execution.
In `@pkg/meta/lookup_effective_test.go`:
- Around line 85-130: Extend the “single_row_and_tie_breaks” tests for order
independence by rerunning each existing two-row precedence case with its slice
reversed and asserting the same selected ID. Add a three-element case and verify
that pickEffectiveAmong returns the same ID as both forward and reverse pairwise
folds, matching the all-at-once result and confirming total-order behavior.
🪄 Autofix
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: 8e45ff8b-7411-453b-845d-a88654889cb2
📒 Files selected for processing (11)
internal/bootstrap/service/coordinator_admin_coverage_test.gomodules/auth/service/models/_resolve_effective_model.tsmodules/auth/service/models/_user_permission_state_acl.tsmodules/auth/service/tests/field_rule_effective_resolve.test.tsmodules/auth/service/tests/permission_state_acl_source.test.tsmodules/auth/service/tests/resolve_effective_model.test.tspkg/meta/acl_remap.gopkg/meta/acl_remap_coverage_test.gopkg/meta/acl_remap_test.gopkg/meta/dual_store_migrate.gopkg/meta/lookup_effective_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- pkg/meta/dual_store_migrate.go
- modules/auth/service/models/_user_permission_state_acl.ts
- Rewrite rowUpdatedAt null/updated_at fallthrough so the null-ts path is exercised. - Normalize ACL label trimming via an explicit null check and cover null Application/Name rows. Co-authored-by: Cursor <cursoragent@cursor.com>
- Skip blank historical model keys in ACL service remap and paginate Auth effective lookups. - Strengthen remap/lookup tests for tie-break, rollback, and fixture error checking. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
User description
Summary
LookupEffectiveModeland wire loader / translated_seed / bootstrap to resolve(application, name)without tipOrder.INworkaround.RemapACLToEffectiveModelIDsremaps auth ACLmeta_model_id/ field / orphan service FKs; runs after dual-store migrate and full recompute.module_idempty); drop production in-memory IMD merge.MetaModelRaw(+ raw field/service/decorator/argument/parameter/type_parameter) facades.Test plan
go test ./pkg/meta/ ./internal/module/evolution/data/ ./internal/module/artifact/generate/ ./internal/bootstrap/service/ ./internal/module/artifact/build/web/ -count=1./choysum test unit auth --be(176 ok)Notes
sqlModelIdtip / SavedFilter remain EDS-4.Made with Cursor
PR Type
Enhancement, Tests
Description
Migrate Go loader and codegen to effective models
Add ACL remapping and effective lookups in Go
Refactor Auth module evaluation to effective model IDs
Export TS raw metadata facades and update tests
File Walkthrough
6 files
Use LookupEffectiveModel for auth User bootstrap resolutionLoad effective-only model rows in gRPC generatorMigrate data loader queries to LookupEffectiveModelRefactor FieldRule evaluation to single effective model IDUse effective model resolver in method access evaluationUse resolveEffectiveModelRow in record rule evaluation11 files
Filter web builder model dependencies to effective projectionsAdd LookupEffectiveModel helper for single live projectionsAdd RemapACLToEffectiveModelIDs for auth rule foreign keysTrigger RemapACLToEffectiveModelIDs after dual-store migration andrecomputeAdd TypeScript helper to resolve effective model and app IDsDeduplicate ACL model search results by application and nameAdd MetaModelRaw declaration facade modelAdd MetaFieldRaw declaration facade modelAdd MetaServiceRaw declaration facade modelAdd MetaDecoratorRaw declaration facade modelExport raw metadata facade models3 files
Add unit tests for LookupEffectiveModel functionAdd unit tests for RemapACLToEffectiveModelIDsUpdate Auth field rule unit tests to resolve effective model ID15 files
Summary by CodeRabbit
Bug Fixes
New Features