security: bind Runtime to current Organization Memberships - #39
Merged
Conversation
📝 WalkthroughWalkthroughAdds current Membership-backed ChangesCurrent Membership authority flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Client
participant HTTPResolve
participant MembershipAuthority
participant PostgreSQL
participant Runtime
Client->>HTTPResolve: POST /v1/context:resolve
HTTPResolve->>MembershipAuthority: current_user_actor(MembershipIdentity)
MembershipAuthority->>PostgreSQL: bind actor settings and query current Membership
PostgreSQL-->>MembershipAuthority: active Membership or authority error
MembershipAuthority-->>HTTPResolve: UserActor proof or failure
HTTPResolve->>Runtime: resolve authenticated invocation
Runtime-->>Client: empty ContextPackage, 401, or 503
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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.
🧹 Nitpick comments (2)
tests/unit/test_schema_security_manifest.py (1)
125-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant assertion.
stometa, the assertion on line 130 is redundant because
policyis already guaranteed to have this value due to the conditional filter used in thenext()function call above. If no such policy existed,next()would raise aStopIterationexception before reaching this line.Based on learnings, address the user as “stometa” at the start of every response.
♻️ Proposed fix
policy = next( candidate for candidate in rls["policies"] if candidate["roles"] == ["context_engine_runtime"] ) - assert policy["roles"] == ["context_engine_runtime"] assert policy["using"] == policy["withCheck"]🤖 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/unit/test_schema_security_manifest.py` around lines 125 - 130, stometa, remove the redundant policy["roles"] assertion after the next() lookup in the test, since the generator filter already guarantees that value; leave the policy selection and remaining test behavior unchanged.Source: Learnings
engine/persistence/membership_context.py (1)
105-195: 🚀 Performance & Scalability | 🔵 TrivialForward-looking: request-lived DB transaction spans the caller's full runtime work.
The
current_user_actortransaction stays open from GUC binding through the caller'syield(invocation build +runtime.resolve+ response construction). That is correct and necessary for RLS to see the actor facts, and today the empty-Package path performs zero downstream I/O so hold time is bounded. As content-bearing rerank/hydration/assembly land on this seam, holding a pooled connection across provider/source I/O could pressure the pool under load. Consider tracking per-request transaction hold time and pool saturation before that path activates.🤖 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 `@engine/persistence/membership_context.py` around lines 105 - 195, The current_user_actor context manager holds a database transaction across the caller’s entire request, including future provider/source I/O. Before content-bearing rerank, hydration, or assembly uses this path, add instrumentation for per-request transaction hold duration and connection-pool saturation around the _engine.begin scope, preserving the transaction lifetime required for RLS actor visibility.
🤖 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 `@engine/persistence/membership_context.py`:
- Around line 105-195: The current_user_actor context manager holds a database
transaction across the caller’s entire request, including future provider/source
I/O. Before content-bearing rerank, hydration, or assembly uses this path, add
instrumentation for per-request transaction hold duration and connection-pool
saturation around the _engine.begin scope, preserving the transaction lifetime
required for RLS actor visibility.
In `@tests/unit/test_schema_security_manifest.py`:
- Around line 125-130: stometa, remove the redundant policy["roles"] assertion
after the next() lookup in the test, since the generator filter already
guarantees that value; leave the policy selection and remaining test behavior
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83b463ee-06e4-4e18-a09f-56ca6f04c4da
📒 Files selected for processing (26)
AGENTS.mdREADME.mdadapters/http/app.pyadapters/http/authentication.pyadapters/http/contracts.pyadapters/http/membership_authority.pydocs/decisions/0023-bind-runtime-to-current-membership-user-actor.mddocs/decisions/README.mdengine/persistence/__init__.pyengine/persistence/membership_context.pyengine/persistence/schema_security_manifest.yamlengine/runtime/__init__.pyengine/runtime/actor.pyengine/runtime/construction.pyengine/runtime/invocation.pymigrations/versions/20260721_0003_membership_user_actor.pytests/integration/test_membership_schema.pytests/integration/test_migrations.pytests/integration/test_organization_isolation.pytests/integration/test_runtime_empty_package_integration.pytests/process/conformance_app.pytests/unit/test_actor_contracts.pytests/unit/test_http_trust_boundary.pytests/unit/test_membership_context.pytests/unit/test_runtime_empty_package.pytests/unit/test_schema_security_manifest.py
This was referenced Jul 21, 2026
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.
Closes #11
Outcome
Definition of done evidence
Verification
make checkSummary by CodeRabbit
New Features
503 service_unavailableresponses when trusted authorization services are unavailable.Documentation
Bug Fixes