Add fixed service identity foundation#132
Conversation
📝 WalkthroughWalkthroughAUTH-09A adds a fixed service identity model, eight planned authorization actions, a strict seven-identity action matrix, guarded migration ChangesAuthorization service identity foundation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Operator
participant MappingCLI
participant PostgreSQL
participant Alembic0023
Operator->>MappingCLI: validate or bind private mapping
MappingCLI->>PostgreSQL: snapshot existing service rows
MappingCLI-->>Operator: emit bounded counts and digests
MappingCLI->>Alembic0023: supply verified envelope
Alembic0023->>PostgreSQL: assign service identities and persist migration evidence
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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.
Actionable comments posted: 2
🤖 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 `@backend/alembic/versions/0023_service_actor_identity.py`:
- Around line 14-21: Freeze the historical migration inputs by moving the
contracts used by revision 0023—load_migration_mapping,
database_binding_identifier, and SERVICE_IDENTITY_VALUES—into the revision
itself or a versioned migration-only module. Update the imports and migration
references to use those immutable definitions, while retaining
ExistingServiceActorRow, ServiceIdentityMappingError, and source_row_set_sha256
only if they are not part of the mutable contracts.
In `@backend/scripts/service_actor_identity_mapping.py`:
- Around line 87-119: The main workflow and engine cleanup currently run on
separate event loops. Add an async entrypoint that executes _execute and
dispose_engine within the same asyncio.run call, while preserving the existing
success or error result when cleanup fails; only report database_cleanup_failed
when the workflow itself succeeded and cleanup raises.
🪄 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 Plus
Run ID: e7a54101-af43-438f-97bb-4bc2d00219bd
📒 Files selected for processing (36)
.agent-loop/LOOP_STATE.md.agent-loop/REVIEW_LOG.md.agent-loop/WORK_QUEUE.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DISCOVERY.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/PLAN.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/RISKS.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-09-actor-state-service-actors.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-09A-service-identity-foundation.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-09B-controlled-service-provisioning.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-09C-actor-identity-admin-reads.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-09D-actor-identity-lifecycle.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-10-project-role-grants.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12-project-mutation-cutover.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-13-task-assignment-cutover.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-14-submission-checker-cutover.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-09-initial-plan-review.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-09A-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-09A-pr-trust-bundle.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-09A-preimplementation-plan-review.md.agent-loop/merge-intents/WS-AUTH-001-09A.jsonbackend/alembic/versions/0023_service_actor_identity.pybackend/app/modules/actors/models.pybackend/app/modules/actors/service_identities.pybackend/app/modules/actors/service_identity_migration.pybackend/app/modules/authorization/catalogue.pybackend/scripts/service_actor_identity_mapping.pybackend/tests/test_actor_migration_tools.pybackend/tests/test_actors.pybackend/tests/test_alembic.pybackend/tests/test_auth.pybackend/tests/test_authorization.pydocs/operations_authorization_service.mddocs/spec_authorization_service.md
| def main(argv: list[str] | None = None) -> int: | ||
| """Run the validate, bind, or verify workflow with bounded output.""" | ||
| result = 0 | ||
| stdout_message: str | None = None | ||
| stderr_message: str | None = None | ||
| try: | ||
| args = _parser().parse_args(argv) | ||
| report = asyncio.run(_execute(args)) | ||
| stdout_message = json.dumps( | ||
| report, | ||
| allow_nan=False, | ||
| separators=(",", ":"), | ||
| sort_keys=True, | ||
| ) | ||
| except ServiceIdentityMappingError as exc: | ||
| error = {"error": exc.code, "status": "error"} | ||
| if exc.count is not None: | ||
| error["count"] = exc.count | ||
| stderr_message = json.dumps(error, separators=(",", ":"), sort_keys=True) | ||
| result = 2 | ||
| except KeyboardInterrupt: | ||
| stderr_message = '{"error":"interrupted","status":"error"}' | ||
| result = 130 | ||
| except Exception: | ||
| stderr_message = '{"error":"database_operation_failed","status":"error"}' | ||
| result = 2 | ||
| try: | ||
| asyncio.run(dispose_engine()) | ||
| except Exception: | ||
| if result == 0: | ||
| stdout_message = None | ||
| stderr_message = '{"error":"database_cleanup_failed","status":"error"}' | ||
| result = 2 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
session_file="$(fd -a '^session\.py$' backend/app | head -n1)"
ast-grep outline "$session_file" --items all \
--match 'get_engine|dispose_engine' --view expanded
rg -nP -C5 '\b(get_engine|dispose_engine)\s*\(' \
"$session_file" backend/scripts/service_actor_identity_mapping.py
rg -n -C3 'NullPool|poolclass|create_async_engine' "$session_file"Repository: Flow-Research/workstream
Length of output: 5096
🌐 Web query:
SQLAlchemy async engine asyncio.run dispose engine different event loop asyncpg pool bound to loop docs
💡 Result:
When using SQLAlchemy's AsyncEngine, connection pools (such as those used by asyncpg) are bound to the specific asyncio event loop active when the pool is first initialized [1][2]. Because asyncio.run creates a new event loop on every invocation, sharing an AsyncEngine across multiple calls to asyncio.run will cause connection errors, such as RuntimeError: Task got Future attached to a different loop [3][4]. To avoid this, you should follow these practices: 1. Use a Single Event Loop: Design your application to manage a single event loop for its entire lifecycle rather than repeatedly calling asyncio.run [3][2]. For example, in a web application, the framework manages the event loop for you [3]. 2. Explicitly Dispose Before Reuse: If you must reuse an AsyncEngine across different event loops, you must call await engine.dispose before the loop shuts down or before moving the engine to a new loop [1][5]. If you are disposing an engine after its original event loop has already closed, you may need to call await engine.dispose(close=False) to prevent errors when the underlying driver tries to access the closed loop [3]. 3. Use NullPool for Transient Scopes: If your architecture requires frequently spinning up and shutting down event loops (e.g., short-lived scripts), configure the engine to use NullPool [1][3]. This disables persistent connection pooling, ensuring each connection is fresh and not tied to a potentially dead event loop [1][3]: from sqlalchemy.ext.asyncio import create_async_engine from sqlalchemy.pool import NullPool engine = create_async_engine("postgresql+asyncpg://...", poolclass=NullPool) Always await engine.dispose before your application or scope finishes to ensure connections are cleaned up properly and to avoid warnings about unclosed resources [1][6]. Attempting to rely on garbage collection to clean up these connections in an async environment is unreliable because finalizers cannot perform the necessary awaitable cleanup [1][7].
Citations:
- 1: https://docs.sqlalchemy.org/en/21/orm/extensions/asyncio.html
- 2: https://async-workflows.com/async-engines-dialects-and-connection-pooling/configuring-async-engines-and-connection-pools/
- 3: [asyncio, asyncpg] RuntimeError: Task got Future attached to a different loop when calling asyncio.run() multiple times sqlalchemy/sqlalchemy#12211
- 4: Event loop closing when reusing an AsyncEngine sqlalchemy/sqlalchemy#11507
- 5: https://www.sqlalchemy.org/docs/orm/extensions/asyncio.html
- 6: When to dispose async engine? sqlalchemy/sqlalchemy#7531
- 7: https://docs.sqlalchemy.org/en/14/orm/extensions/asyncio.html?highlight=async
Dispose the async engine on the same event loop.
asyncio.run(_execute(args)) creates the pooled asyncpg engine on one loop, then asyncio.run(dispose_engine()) cleans it up on a new loop. That can trip loop-bound pool state and turn successful runs into database_cleanup_failed. Wrap both calls in one async entrypoint and keep the original result if cleanup also fails.
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 94-99: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
report,
allow_nan=False,
separators=(",", ":"),
sort_keys=True,
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
[info] 104-104: use jsonify instead of json.dumps for JSON output
Context: json.dumps(error, separators=(",", ":"), sort_keys=True)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 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 `@backend/scripts/service_actor_identity_mapping.py` around lines 87 - 119, The
main workflow and engine cleanup currently run on separate event loops. Add an
async entrypoint that executes _execute and dispose_engine within the same
asyncio.run call, while preserving the existing success or error result when
cleanup fails; only report database_cleanup_failed when the workflow itself
succeeded and cleanup raises.
2 participants
WS-AUTH-001-09A PR Trust Bundle
Chunk
WS-AUTH-001-09A- Fixed Service Identity FoundationGoal
Give each internal service ActorProfile one stable ServiceAccount-style local
identity, retain the Identity Issuer subject as a separate credential link, and
define a closed service-action candidate matrix without provisioning or
activating service authority.
Changes
ActorProfile.service_identitysemantics for service profiles only.historical owner metadata, and planned availability.
0023, a confidential exact-set operator mapping tool, andimmutable bounded migration evidence.
Boundary
ActorProfile is the stable local principal.
(issuer, subject)is only thecredential link. Authority candidates come from reviewed typed code, never
email, token roles, display fields, grants, dynamic assignment rows, or request
input.
This chunk adds no route, provisioning, service admission, evaluator, resource
composer, feature behavior, grant, action activation, compatibility alias,
fallback, or second identity model. Internal services are not human
Contributors.
Proof
0023proof: three passed.percent catalogue, 93.41 percent combined.
percent docstring coverage, and diff integrity pass.
Exact implementation SHA
fe61df64fbf82a1f6871c380e6fc1986a4f12205passed senior engineering,QA/test, security/auth, product/ops, architecture, CI integrity, docs,
reuse/dedup, and test-delta review after valid findings were repaired.
External Review
Earlier CodeRabbit findings on mutable historical migration imports and
cross-event-loop cleanup are fixed. The migration consumes a packaged frozen
contract and the operator lifecycle uses one async loop with explicit error
precedence. Replacement external checks are pending on the converged head.
Follow-up
PR #140 resolved activation custody: feature owners supply hidden facts, guards,
behavior, and typed manifests; only AUTH integrates evaluators and changes
availability. Implementation remains deliberately separate. The binding order
is
09B -> 09C -> 09D -> 09E -> ART/REV custody -> PREP, with a human startrequired at each chunk boundary.
Remaining Risk
A host crash during private-envelope publication may require regenerating the
file. Missing, partial, or unverifiable evidence remains fail-closed.
Human Review Focus
Review the fixed identity model, strict private mapping custody, exact matrix
metadata, inert actions, immutable evidence, and absence of dynamic assignment
or compatibility authority. Only the human may approve and merge this PR.