feat(orm): add BaseModel.sudo/withUser and remove Compute runAs#186
Merged
Conversation
- Add scoped sudo for RR/FR bypass with enter audit while keeping company scope. - Add withUser identity override stack so getUserId prefers the stack over identity. - Remove author-facing @compute({ runAs }) and engine wrapping in favor of method-body sudo/withUser. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds scoped ChangesAuthorization and compute execution
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Model
participant ContextFacade
participant RuntimeContext
participant AuthzRuntime
participant ComputeEngine
Model->>ContextFacade: withUser(userId, fn)
ContextFacade->>RuntimeContext: push user override
Model->>ContextFacade: sudo(fn)
ContextFacade->>AuthzRuntime: bypass record and field rules
ComputeEngine->>ComputeEngine: execute compute directly
AuthzRuntime-->>Model: restore bypass depths
RuntimeContext-->>Model: restore user override
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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! |
- Remove WebFieldMetadata.runAs and getFieldMetadataView passthrough. - Stop parsing, resolving, and codegen-emitting IrField/compute runAs. Co-authored-by: Cursor <cursoragent@cursor.com>
![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 (1)
modules/core/service/orm/repository/authz/tests/authz_runtime.test.ts (1)
166-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest name says "nested" but only exercises two sequential (non-nested) calls.
Unlike the sibling tests for
withRepositoryRecordRuleBypass/withRepositoryFieldRuleBypass(lines 116-122, 203-209) which nest a call inside another and assert depth reaches 2, this test for the combinedwithRepositoryAuthzRuleBypassnever nests — it just calls the wrapper twice back-to-back. The PR objectives explicitly flag nested composition as a pending spot-check; this test doesn't close that gap despite its name.✅ Suggested addition: actual nested case
await withRepositoryAuthzRuleBypass(async () => { expect(getRepositoryRecordRuleBypassDepth()).toBe(1); expect(getRepositoryFieldRuleBypassDepth()).toBe(1); + await withRepositoryAuthzRuleBypass(async () => { + expect(getRepositoryRecordRuleBypassDepth()).toBe(2); + expect(getRepositoryFieldRuleBypassDepth()).toBe(2); + }); + expect(getRepositoryRecordRuleBypassDepth()).toBe(1); + expect(getRepositoryFieldRuleBypassDepth()).toBe(1); }); expect(getRepositoryFieldRuleBypassDepth()).toBe(0); + expect(getRepositoryRecordRuleBypassDepth()).toBe(0);🤖 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 `@modules/core/service/orm/repository/authz/tests/authz_runtime.test.ts` around lines 166 - 191, Update the test withRepositoryAuthzRuleBypass scenario to include an actual nested wrapper invocation, asserting both record-rule and field-rule bypass depths reach 2 inside the inner callback and return to 1 after it completes. Preserve the existing sync and async coverage, and retain assertions that depths restore to 0 afterward.
🤖 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/core/service/orm/model/model_sudo.ts`:
- Around line 16-43: Update resolveSudoAuditBucket to reset the global
__choysumComputeAudit bucket when a new request context begins, while preserving
the current sudoHits initialization and recordSudoEnterAudit behavior within
that request. Reuse the existing request-context identifier or lifecycle hook
visible in the surrounding implementation to ensure audit entries cannot
accumulate or leak across requests.
In `@modules/core/service/runtime/context/user.ts`:
- Around line 80-106: The mutable stacks in withUser and the process-level
fallback must not allow overlapping asynchronous scopes to restore the wrong
user. Replace the shared-stack async handling with task-local isolation, or
explicitly reject overlapping async withUser calls, covering both the
request-scoped jsCtx path and processLevelUserIdStack path; add tests where
overlapping calls complete in reverse order and verify getUserId() remains
associated with each scope.
---
Nitpick comments:
In `@modules/core/service/orm/repository/authz/tests/authz_runtime.test.ts`:
- Around line 166-191: Update the test withRepositoryAuthzRuleBypass scenario to
include an actual nested wrapper invocation, asserting both record-rule and
field-rule bypass depths reach 2 inside the inner callback and return to 1 after
it completes. Preserve the existing sync and async coverage, and retain
assertions that depths restore to 0 afterward.
🪄 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: e54c9678-321a-4e65-bec9-56a1b0e2a402
📒 Files selected for processing (29)
modules/core/service/api/context.tsmodules/core/service/api/index.test.tsmodules/core/service/api/index.tsmodules/core/service/orm/decorator/compute.test.tsmodules/core/service/orm/decorator/compute.tsmodules/core/service/orm/metadata/compute.tsmodules/core/service/orm/metadata/field.tsmodules/core/service/orm/metadata/index.tsmodules/core/service/orm/metadata/model.tsmodules/core/service/orm/metadata/storage.test.tsmodules/core/service/orm/metadata/storage.tsmodules/core/service/orm/model/model.test.tsmodules/core/service/orm/model/model.tsmodules/core/service/orm/model/model_context_facade.tsmodules/core/service/orm/model/model_sudo.test.tsmodules/core/service/orm/model/model_sudo.tsmodules/core/service/orm/repository/authz/authz_runtime.tsmodules/core/service/orm/repository/authz/index.tsmodules/core/service/orm/repository/authz/tests/authz_runtime.test.tsmodules/core/service/orm/repository/query/tests/condition_compiler.test.tsmodules/core/service/runtime/compute/engine.test.tsmodules/core/service/runtime/compute/engine.tsmodules/core/service/runtime/compute/runas.test.tsmodules/core/service/runtime/compute/runas.tsmodules/core/service/runtime/compute/search_rewrite.tsmodules/core/service/runtime/context/context.test.tsmodules/core/service/runtime/context/index.tsmodules/core/service/runtime/context/source.tsmodules/core/service/runtime/context/user.ts
💤 Files with no reviewable changes (9)
- modules/core/service/runtime/compute/runas.test.ts
- modules/core/service/runtime/context/source.ts
- modules/core/service/orm/metadata/index.ts
- modules/core/service/runtime/compute/runas.ts
- modules/core/service/orm/metadata/model.ts
- modules/core/service/orm/metadata/field.ts
- modules/core/service/orm/metadata/storage.ts
- modules/core/service/orm/metadata/storage.test.ts
- modules/core/service/orm/metadata/compute.ts
- Remove withUser override entries by identity so sibling async scopes do not LIFO-pop each other. - Decrement shared RR/FR bypass depth on restore, matching withBypassDepths for out-of-order completion. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/parser/backendtsparser/parser_migration_test.go (1)
238-238: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPreserve negative coverage for the removed
runAscontract.The changed tests remove the old inputs and assertions but do not verify rejection or omission, allowing stale
runAspropagation to regress unnoticed.
internal/parser/backendtsparser/parser_migration_test.go#L238-L238: add a legacyrunAsfixture and assert decorate-time rejection.pkg/meta/meta_field_test.go#L168-L171: assert serialized resolved metadata omits"runAs".internal/module/artifact/generate/webapistore_test.go#L80-L81: assert generated store content omits therunAskey.🤖 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 `@internal/parser/backendtsparser/parser_migration_test.go` at line 238, Preserve negative coverage for the removed runAs contract: in internal/parser/backendtsparser/parser_migration_test.go lines 238-238, add a legacy runAs fixture and assert decorate-time rejection; in pkg/meta/meta_field_test.go lines 168-171, assert serialized resolved metadata omits "runAs"; and in internal/module/artifact/generate/webapistore_test.go lines 80-81, assert generated store content omits the runAs key.
🤖 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.
Nitpick comments:
In `@internal/parser/backendtsparser/parser_migration_test.go`:
- Line 238: Preserve negative coverage for the removed runAs contract: in
internal/parser/backendtsparser/parser_migration_test.go lines 238-238, add a
legacy runAs fixture and assert decorate-time rejection; in
pkg/meta/meta_field_test.go lines 168-171, assert serialized resolved metadata
omits "runAs"; and in internal/module/artifact/generate/webapistore_test.go
lines 80-81, assert generated store content omits the runAs key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d8ff6191-2ad5-4c56-9a5f-8f9ad2a15b86
📒 Files selected for processing (9)
internal/module/artifact/generate/webapistore.gointernal/module/artifact/generate/webapistore.ts.tplinternal/module/artifact/generate/webapistore_test.gointernal/parser/backendtsparser/field_resolved.gointernal/parser/backendtsparser/parser_migration_test.gomodules/web/web/stores/modelStore.test.tsmodules/web/web/stores/modelStore.tspkg/meta/meta_field.gopkg/meta/meta_field_test.go
💤 Files with no reviewable changes (5)
- internal/module/artifact/generate/webapistore.ts.tpl
- modules/web/web/stores/modelStore.test.ts
- internal/parser/backendtsparser/field_resolved.go
- modules/web/web/stores/modelStore.ts
- internal/module/artifact/generate/webapistore.go
- Store sudoHits on request service state so audit entries do not accumulate across requests. - Exercise nested withRepositoryAuthzRuleBypass depth restore in unit coverage. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/service/orm/model/model_sudo.test.ts`:
- Around line 21-25: Update readSudoHits to return only the current request
state's sudoHits, using an empty array when the request state or field is
absent; remove the globalThis.__choysumComputeAudit fallback from this helper.
If no-request tests still require legacy behavior, move that fallback into a
separate helper and use it only in those tests.
🪄 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: dfe9c4dd-f4df-4357-9285-7534ef9f6b97
📒 Files selected for processing (3)
modules/core/service/orm/model/model_sudo.test.tsmodules/core/service/orm/model/model_sudo.tsmodules/core/service/orm/repository/authz/tests/authz_runtime.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- modules/core/service/orm/model/model_sudo.ts
- modules/core/service/orm/repository/authz/tests/authz_runtime.test.ts
- Share a pendingAsync/syncDepth guard so sibling Promise.all scopes fail fast. - Keep sync nesting and sequential awaits; document yield-after-nest as unsupported without ALS. Co-authored-by: Cursor <cursoragent@cursor.com>
- Parse legacy @compute({ runAs }) fixtures and assert resolved JSON has no runAs. - Assert IrField and webapistore outputs omit the removed runAs key. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/core/service/runtime/context/scope.ts`:
- Around line 154-171: Update the cleanup logic in runWithAsyncScopeTracking to
track each context entry and remove the specific scope being settled rather than
restoring via LIFO branches. After removal, expose the latest remaining active
entry through CTX_OVERRIDE_KEY, preserving the correct context when detached
nested async withContext calls settle out of order. Add a regression test
covering this detached nested async scenario.
🪄 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: f9dc0367-039f-4b12-bad1-ee7adb93444a
📒 Files selected for processing (4)
modules/core/service/runtime/context/async_scope.tsmodules/core/service/runtime/context/context.test.tsmodules/core/service/runtime/context/scope.tsmodules/core/service/runtime/context/user.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- modules/core/service/runtime/context/user.ts
Contributor
Author
|
/improve |
PR Code Suggestions ✨No code suggestions found for the PR. |
- Drop the overlapping-async guard that also blocked legitimate nest-after-await used by repository update validation and bilingual write/read paths. - Keep documenting concurrent sibling scopes as unsupported without AsyncLocalStorage, and cover nest-after-await in unit tests. Co-authored-by: Cursor <cursoragent@cursor.com>
- Exercise BaseModel static/instance withUser and sudo wrappers end to end. - Cover sudo audit hint, process-bucket fallback, and audit disable env. - Cover withUser process async/throw, legacy stack entries, and sibling restore. - Cover authz bypass sync throw restore and no-req authz bypass; reject async @Search on sync rewrite. Co-authored-by: Cursor <cursoragent@cursor.com>
- Hit empty/null userId override stack tops so peekStackTop string branch is complete. - Corrupt bypass depth to NaN/Infinity/non-number so restoreBypassDepth uses the zero fallback. Co-authored-by: Cursor <cursoragent@cursor.com>
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.
User description
Summary
BaseModel.sudo(fn)for scoped RecordRule + FieldRule bypass (company scope retained) with enter audit, sync/async friendly for virtual compute reads.BaseModel.withUser(userId, fn)with a userId override stack;getUserId()prefers the stack overidentity.userId.withContextremains business-ctx only and does not switch authz user.@Compute({ runAs })(decorator rejects it), metadata, and engine/search wrapping so compute/search elevation is only via method-bodysudo/withUser(PR-P0-M1 / D1–D12).Test plan
./choysum test unit core --be./choysum test typecheck coresudo+withUsercomposition; syncsudoinside@Computebody@Compute({ runAs: 'sudo' })fails at decorate timeMade with Cursor
Summary by CodeRabbit
withUserfor temporary user identity overrides, with correct nesting and safe async scope behavior (including overlap detection).BaseModel.withUser(...)/ instancewithUser(...)andBaseModel.sudo(...)/ instancesudo(...)for authorization scoping and elevation.withUser.runAssupport end-to-end, including metadata, handler contracts, and related audit behavior.PR Type
Enhancement
Description
TypeScript modules (
modules/core):BaseModel.sudofor scoped rule bypass.BaseModel.withUseridentity override stack.@Compute({ runAs })decorator option.Go core: No changes outside
modules/.Added new source files with required SPDX headers.
Expanded TypeScript unit test coverage for sudo and user contexts.
File Walkthrough
9 files
Expose sudo and withUser methods on BaseModelImplement withModelSudo and enter audit recordingImplement user override stack and withUser handlerReject author-facing runAs option in Compute decoratorAdd sync-friendly combined authz rule bypass wrapperBridge withUser and sudo through model context facadeRe-export withUser from service api context entrypointRe-export withUser in service api root indexExport getUserId and withUser from context user module4 files
Add unit tests for withModelSudo executionAdd unit tests for withUser context behaviorUpdate Compute decorator tests for removed runAs optionAdd tests for combined authz rule bypass wrapper7 files
Remove runAs execution wrapper from compute engineRemove runAs wrapper from search rewrite logicRemove legacy compute runAs module fileRemove ComputeRunAs type and metadata propertiesRemove runAs property from ColumnComputeSpecRemove runAs from ComputeHandlerMeta interfaceRemove runAs field normalization from MetadataStorage9 files