Add canonical actor profile and identity resolution#124
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis change replaces the legacy actor registry with canonical actor profiles and issuer-subject identity links, adds transactional first-access provisioning and self-service APIs, preserves bounded legacy workflow eligibility, introduces migration guards, centralizes rate controls, and expands validation. ChangesCanonical actor authorization
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 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.
Actionable comments posted: 7
🧹 Nitpick comments (3)
backend/scripts/api_contract_e2e.py (1)
1077-1086: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptionally assert the absence of legacy token and identity fields.
The assertions successfully verify the expected values in the canonical actor response. For completeness in an API contract test, consider also verifying that the endpoint properly masks private internal fields (e.g.,
issuer,subject, androles), matching the strict behavior enforced in your test suite.💡 Proposed optional addition
canonical_actor = await request_json( client, "GET", "/api/v1/actors/me", worker_token, ) assert canonical_actor["actor_kind"] == "human" assert canonical_actor["domains"] == ["contributor"] assert canonical_actor["admin_roles"] == [] assert canonical_actor["project_role_grants"] == [] + assert "issuer" not in canonical_actor + assert "subject" not in canonical_actor + assert "roles" not in canonical_actor🤖 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/api_contract_e2e.py` around lines 1077 - 1086, Extend the canonical actor assertions near the existing actor_kind, domains, admin_roles, and project_role_grants checks to verify that legacy private fields issuer, subject, and roles are absent from canonical_actor. Preserve the current value assertions and use the test suite’s established absence assertion style.backend/tests/test_auth.py (1)
52-66: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse structural analysis for the compatibility allowlist.
Substring scanning and exact call-text counting can be bypassed by aliases or different arguments and can be tripped by comments. Parse imports and call expressions with
astso this test reliably constrains the authorization-adjacent compatibility bridge.🤖 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/tests/test_auth.py` around lines 52 - 66, Update test_legacy_submitter_eligibility_adapter_has_a_shrinking_static_allowlist to parse each Python file with ast and identify actual references to LegacyWorkflowEligibilityCompatibility through imports and call expressions, rather than scanning source text. Use AST call analysis to count invocations of _require_legacy_submitter_eligibility in modules/tasks/service.py, regardless of formatting or argument spelling, while excluding comments and unrelated text; preserve the existing allowed-consumer set and expected count of three.backend/tests/test_actors.py (1)
56-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the privileged database reset implementation. The repeated trigger-management SQL has already diverged and should have one test-owned implementation.
backend/tests/test_actors.py#L56-L89: move the canonical-actor reset extension behind explicit shared-helper options.backend/tests/test_auth.py#L78-L98: replace the local helper with the shared reset utility.backend/tests/test_checkers.py#L61-L81: replace the local helper with the shared reset utility.backend/tests/test_projects.py#L85-L105: replace the local helper with the shared reset utility.backend/tests/test_tasks.py#L68-L88: replace the local helper with the shared reset utility.🤖 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/tests/test_actors.py` around lines 56 - 89, Centralize the privileged reset logic in the shared test reset utility, adding explicit options for the canonical actor reset extension currently handled by clear_test_audit_events. Update backend/tests/test_actors.py lines 56-89 to use those options, and replace the local reset helpers in backend/tests/test_auth.py lines 78-98, backend/tests/test_checkers.py lines 61-81, backend/tests/test_projects.py lines 85-105, and backend/tests/test_tasks.py lines 68-88 with the shared utility; preserve each test suite’s required reset 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 `@backend/alembic/versions/0020_canonical_actor_profile.py`:
- Around line 351-359: The downgrade upsert in the canonical actor migration
must preserve current self-service fields: update display_name and contact_email
from actor_profiles on conflict, including propagating a cleared email instead
of retaining legacy values. Add a downgrade test covering both an updated
display name and a cleared contact_email.
- Line 297: Update the UUIDv5 seed used to generate actor_identity_links.id in
the migration so it cannot be ambiguous when external_issuer or external_subject
contains colons; use row.actor_id or an equivalent collision-free
length-prefixed representation while preserving deterministic UUID generation.
In `@backend/app/adapters/auth/dev.py`:
- Around line 38-46: Update the development auth validation around
dev_auth_subject and dev_auth_issuer to trim both values before checking
presence and MAX_VERIFIED_IDENTITY_ANCHOR_CHARACTERS length. Reject
whitespace-only anchors during this validation, while preserving the existing
error behavior for missing or oversized values.
In `@backend/app/modules/actors/service.py`:
- Around line 184-240: Serialize activate_legacy_workflow_eligibility per
external identity by acquiring the existing transaction-scoped identity lock, or
a dedicated eligibility lock, before upsert_legacy_identity and the eligibility
read/insert sequence. Ensure the lock makes concurrent first activations apply
the winning request deterministically and prevents auditing an insert when the
caller’s payload was not applied. Add a concurrency test using different
skill_tags payloads, asserting deterministic final state and exactly one audit
per actual transition.
In `@backend/app/modules/tasks/service.py`:
- Line 536: Update _worker_lifecycle_context to load the worker’s current
eligibility and suppress claim, start, and submit affordances whenever the
corresponding eligibility gates are inactive. Reuse the same eligibility logic
as _require_legacy_submitter_eligibility and retain existing assignment and
task-status checks, so advertised actions cannot lead to deterministic 403
responses.
In `@backend/tests/test_actors.py`:
- Around line 105-112: Make the database fixture teardown failure-safe by
wrapping the reset and cleanup sequence in nested try/finally blocks so
db_session.dispose_engine() and command.downgrade(config, "base") always
execute, even if clear_test_audit_events() fails. Apply the same teardown
structure in backend/tests/test_actors.py lines 105-112,
backend/tests/test_auth.py lines 119-126, backend/tests/test_checkers.py lines
101-108, backend/tests/test_projects.py lines 133-140, and
backend/tests/test_tasks.py lines 149-156, preserving the existing
migration_lock() and fixture yield flow.
In `@backend/tests/test_alembic.py`:
- Around line 1071-1110: Update _reset_canonical_actor_guard_state to track
whether actor_profile_history_guard was successfully disabled, then wrap the
reset updates in try/finally. In the finally block, when the trigger was
disabled, always re-enable it using a fresh transaction, including if the reset
or setup operations fail.
---
Nitpick comments:
In `@backend/scripts/api_contract_e2e.py`:
- Around line 1077-1086: Extend the canonical actor assertions near the existing
actor_kind, domains, admin_roles, and project_role_grants checks to verify that
legacy private fields issuer, subject, and roles are absent from
canonical_actor. Preserve the current value assertions and use the test suite’s
established absence assertion style.
In `@backend/tests/test_actors.py`:
- Around line 56-89: Centralize the privileged reset logic in the shared test
reset utility, adding explicit options for the canonical actor reset extension
currently handled by clear_test_audit_events. Update
backend/tests/test_actors.py lines 56-89 to use those options, and replace the
local reset helpers in backend/tests/test_auth.py lines 78-98,
backend/tests/test_checkers.py lines 61-81, backend/tests/test_projects.py lines
85-105, and backend/tests/test_tasks.py lines 68-88 with the shared utility;
preserve each test suite’s required reset behavior.
In `@backend/tests/test_auth.py`:
- Around line 52-66: Update
test_legacy_submitter_eligibility_adapter_has_a_shrinking_static_allowlist to
parse each Python file with ast and identify actual references to
LegacyWorkflowEligibilityCompatibility through imports and call expressions,
rather than scanning source text. Use AST call analysis to count invocations of
_require_legacy_submitter_eligibility in modules/tasks/service.py, regardless of
formatting or argument spelling, while excluding comments and unrelated text;
preserve the existing allowed-consumer set and expected count of three.
🪄 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: 099fb3fe-cd93-453a-9445-e0e483fe3b79
📒 Files selected for processing (29)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-06-canonical-actor-profile.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-06-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-06-pr-trust-bundle.md.agent-loop/merge-intents/WS-AUTH-001-06.jsonbackend/alembic/versions/0020_canonical_actor_profile.pybackend/app/adapters/auth/dev.pybackend/app/adapters/auth/flow.pybackend/app/api/deps/api_controls.pybackend/app/api/deps/auth.pybackend/app/api/deps/rate_controls.pybackend/app/api/router.pybackend/app/api/routes/auth.pybackend/app/db/models.pybackend/app/modules/actors/models.pybackend/app/modules/actors/repository.pybackend/app/modules/actors/schemas.pybackend/app/modules/actors/service.pybackend/app/modules/tasks/router.pybackend/app/modules/tasks/service.pybackend/app/schemas/auth.pybackend/scripts/api_contract_e2e.pybackend/tests/test_actors.pybackend/tests/test_alembic.pybackend/tests/test_api_rate_controls.pybackend/tests/test_auth.pybackend/tests/test_checkers.pybackend/tests/test_projects.pybackend/tests/test_tasks.pydocs/operations_authorization_service.md
💤 Files with no reviewable changes (1)
- backend/tests/test_api_rate_controls.py
…to codex/ws-auth-001-06-canonical-actor-profile
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/test_tasks.py (1)
126-144: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFlatten deep
try...finallynesting and release lock duringyield.Yielding inside
with migration_lock():holds the lock for the entire duration of the test. Ifpytest-xdistis used to run tests in parallel, this will effectively serialize test execution across workers and slow down the test suite.Consider releasing the lock during the test execution, re-acquiring it only for teardown migrations, and flattening the
try...finallyblocks for better readability.If holding the lock during the test is strictly required for this test suite (e.g., if workers share a single database and rely on the lock to prevent concurrency), you can still flatten the
try...finallystructure.♻️ Proposed refactor
config = alembic_config() with migration_lock(): command.downgrade(config, "base") command.upgrade(config, "head") try: yield postgres_database_url finally: try: asyncio.run(reset_test_database_state(postgres_database_url)) finally: try: asyncio.run(db_session.dispose_engine()) finally: with migration_lock(): command.downgrade(config, "base") try: asyncio.run(db_session.dispose_engine()) finally: get_settings.cache_clear()🤖 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/tests/test_tasks.py` around lines 126 - 144, Refactor the fixture teardown around migration_lock(), command.downgrade(), and command.upgrade() so the lock is released before yielding postgres_database_url and reacquired only for the final downgrade. Flatten the nested try/finally blocks while preserving reset_test_database_state(), engine disposal, migration cleanup, and get_settings.cache_clear() execution even when earlier teardown steps fail.
🤖 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/tests/conftest.py`:
- Around line 34-47: Update the database reset fixture around the
include_canonical_actors handling to always truncate actor_profiles,
actor_identity_links, legacy_actor_identities, and legacy_workflow_eligibility
before the downgrade. Remove the conditional trigger-disabling and partial
canonical-actor reset path so every suite starts with all actor state cleared.
---
Nitpick comments:
In `@backend/tests/test_tasks.py`:
- Around line 126-144: Refactor the fixture teardown around migration_lock(),
command.downgrade(), and command.upgrade() so the lock is released before
yielding postgres_database_url and reacquired only for the final downgrade.
Flatten the nested try/finally blocks while preserving
reset_test_database_state(), engine disposal, migration cleanup, and
get_settings.cache_clear() execution even when earlier teardown steps fail.
🪄 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: dfe40e38-7305-419f-abd7-49cd477b4ae7
📒 Files selected for processing (20)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-06-canonical-actor-profile.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-06-external-review-response.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-06-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-06-pr-trust-bundle.md.agent-loop/merge-intents/WS-AUTH-001-06.jsonbackend/alembic/versions/0020_canonical_actor_profile.pybackend/app/adapters/auth/dev.pybackend/app/adapters/auth/flow.pybackend/app/modules/actors/service.pybackend/app/modules/tasks/schemas.pybackend/app/modules/tasks/service.pybackend/scripts/api_contract_e2e.pybackend/tests/conftest.pybackend/tests/test_actors.pybackend/tests/test_alembic.pybackend/tests/test_auth.pybackend/tests/test_checkers.pybackend/tests/test_projects.pybackend/tests/test_tasks.pydocs/operations_authorization_service.md
🚧 Files skipped from review as they are similar to previous changes (10)
- .agent-loop/merge-intents/WS-AUTH-001-06.json
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-06-internal-review-evidence.md
- backend/scripts/api_contract_e2e.py
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-06-canonical-actor-profile.md
- backend/app/adapters/auth/dev.py
- backend/app/adapters/auth/flow.py
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-06-pr-trust-bundle.md
- backend/alembic/versions/0020_canonical_actor_profile.py
- docs/operations_authorization_service.md
- backend/tests/test_alembic.py
Comment on lines
+34
to
+47
| if include_canonical_actors: | ||
| await connection.execute( | ||
| "alter table actor_profiles disable trigger actor_profile_history_guard" | ||
| ) | ||
| await connection.execute( | ||
| "alter table actor_identity_links disable trigger " | ||
| "actor_identity_link_history_guard" | ||
| ) | ||
| await connection.execute("truncate table audit_events cascade") | ||
| await connection.execute("truncate table api_rate_control_counters") | ||
| if include_canonical_actors: | ||
| await connection.execute( | ||
| "truncate table actor_identity_links, actor_profiles cascade" | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reset both canonical and legacy actor state for every database-backed suite.
The default path leaves canonical actors intact, while include_canonical_actors=True still leaves legacy_actor_identities and legacy_workflow_eligibility. The subsequent downgrade preserves/renames these rows, making later suites inherit identities and eligibility from earlier suites.
Truncate all four actor tables before downgrade and remove the optional partial-reset 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 `@backend/tests/conftest.py` around lines 34 - 47, Update the database reset
fixture around the include_canonical_actors handling to always truncate
actor_profiles, actor_identity_links, legacy_actor_identities, and
legacy_workflow_eligibility before the downgrade. Remove the conditional
trigger-disabling and partial canonical-actor reset path so every suite starts
with all actor state cleared.
abiorh-claw
previously approved these changes
Jul 15, 2026
abiorh-claw
approved these changes
Jul 15, 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
2 participants
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.
WS-AUTH-001-06 PR Trust Bundle
Chunk
WS-AUTH-001-06- Canonical Actor Profile And Identity ResolutionGoal
Replace the legacy human registry as an authorization authority with one
canonical actor profile and issuer-subject identity link, while preserving a
strictly bounded compatibility bridge for existing submitter workflows.
Human-Approved Intent
Every verified human receives one canonical profile. Authorization is based on
explicit authority and project grants, not a default contributor role. Human
product language uses contributor, submitter, and reviewer; worker remains only
for internal service execution or explicitly inventoried legacy identifiers.
What Changed
0020classifies legacy identities, creates canonical actorprofiles and identity links, enforces identity and lifecycle invariants, and
provides guarded downgrade custody.
one profile/link plus authority evidence, and updates verification timestamps
using database time.
GET/PATCH /api/v1/actors/meexposes the canonical self-service contract;/api/v1/auth/meremains a compatibility view.infrastructure to a retryable, zero-write response.
claim, start, and submission boundary and cannot become authorization truth.
Why It Changed
Later grant and permission chunks require a stable actor reference that is not
coupled to email, mutable token display claims, or a legacy submitter record.
This chunk establishes that identity boundary before authorization policy is
activated.
Design Chosen
External identity is keyed only by exact, bounded issuer and subject. Human profile
state is separate from identity links and defaults to active without assigning
a role. First creation is serialized with a PostgreSQL advisory transaction
lock. Runtime repositories do not own commits or sessions, and authority events
are written in the same transaction as actor state.
Alternatives Rejected
identity issuer owns authentication.
explicit admin and project grants.
reviewer, and manager access must resolve through one canonical actor.
service-principal custody must be explicit.
Scope Control
This chunk does not implement admin-role grants, project-role grants, the
permission evaluator, artifact-storage authorization, cache consumers, frontend
identity flows, or service-principal provisioning. Those remain in later AUTH
chunks.
Product Behavior
A verified human is provisioned on first access and receives a stable actor ID.
They can read and update bounded display fields. Suspended identities remain
readable but cannot mutate; deactivated identities fail closed. Existing task
submitter flows continue through a compatibility projection while current
authorization remains unchanged until the grant and evaluator cutovers.
Acceptance Criteria Proof
link with advancing verification timestamps and no duplicate authority rows.
token parsing, verifier adapters, actor persistence, migration, and audit
provenance.
downgrade, re-upgrade, and terminal deactivation behavior have direct tests.
so pre-AUTH-06 code cannot resurrect stale private data.
compatibility role and active eligibility as their mutation gates.
Tests And Checks Run
166 branches; 83 actor/classification behavior tests passed in 706.29 seconds.
zero-side-effect scenarios passed on the integrated branch.
0020.suites passed; final coverage data includes the repaired static inventory and
added repeated-access, unavailable-control, and compatibility assertions.
integrity, all 71 engineering-loop agent-gate tests, loop-memory state, and
the schema-v2 AUTH-06 merge-intent validator passed.
the authoritative full-suite result and repository-wide 78 percent floor.
Test Delta
Tests replace obsolete registry-authority assumptions with canonical actor
behavior and add concurrency, rollback, privacy, lifecycle, migration, and
compatibility proof. No skip, xfail, coverage threshold, or meaningful
assertion was removed or weakened.
CI Integrity
No workflow, dependency, package script, coverage exclusion, or threshold
changed. The actor subsystem meets its 90 percent branch gate, and GitHub must
still enforce the repository-wide 78 percent baseline.
Reviewer Results
Exact reviewed SHA
abd76c995e51645b61d4d3ac07f1ff82ab6eb740passed seniorengineering, QA/test, security/auth, product/ops, architecture, CI integrity,
docs, reuse/dedup, and test-delta review with no blocking findings.
External Review
CodeRabbit's seven inline findings and three nitpicks were triaged. All valid
runtime, migration, lifecycle, privacy, test-harness, and test-contract findings
were repaired. Its generic diff-local docstring percentage is not the
repository's configured gate and required no narration-only churn. GitHub checks,
the next CodeRabbit head, and human review remain pending.
Remaining Risks
must use the documented non-owner runtime role.
0020requires a quiesced deployment because it classifies andswaps legacy identity storage.
the legacy route and storage.
Follow-Up Work
After merge, automated post-merge memory, stop, and a separate human start,
AUTH-07 may implement the next approved authority boundary. Later chunks own
admin grants, project grants, the evaluator, API cutovers, artifact-storage
authorization, and invalidation consumers.
Human Review Focus
Review migration classification and downgrade custody, first-access transaction
and rate-control behavior, issuer-subject bounds, non-human zero-write denial,
and the exact limits of the legacy compatibility bridge.
Human Merge Ownership
Only the human may approve and merge this PR. Internal or external checks do
not authorize merge.
Summary by CodeRabbit
GET/PATCH /api/v1/actors/me) for contributor-safe profile viewing and updates.503, and exhausted limits return retryable429withRetry-After.