Implement actor identity profile registry#74
Merged
abiorh-claw merged 25 commits intoJul 7, 2026
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 (18)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (8)
📝 WalkthroughWalkthroughThis PR introduces a shared actor identity/profile registry with new models, repository, service, and schemas; migrates legacy worker/reviewer profile storage; rewires auth/task/project/checker flows to use registry-backed actor resolution; and updates scripts, docs, tests, and review-tracking artifacts to the canonical worker profile endpoint. ChangesProcess/Tracking Documentation
Actor Identity/Profile Registry Implementation
Estimated code review effort: 5 (Critical) | ~120 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 4
🧹 Nitpick comments (6)
backend/tests/test_auth.py (1)
41-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame migration-fixture duplication as
test_actors.py/test_tasks.py.This
auth_database_envfixture repeats the same base→head→base Alembic dance already flagged intest_actors.py(actor_database_env) and present intest_tasks.py(task_database_env). Worth extracting once into a shared conftest helper.🤖 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 41 - 63, The auth_database_env fixture duplicates the same Alembic base→head→base setup already used by actor_database_env and task_database_env. Extract the shared migration setup/teardown into a common conftest helper or shared fixture, then have auth_database_env delegate to it while keeping the existing db_session.dispose_engine, get_settings.cache_clear, and migration_lock behavior intact.backend/tests/test_actors.py (1)
25-118: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the migration fixture/helper boilerplate shared across test files.
actor_database_env/actor_client(25-55) and thealembic_config/set_dev_actor/actor_idhelpers (58-118) are near-identical to the equivalents added intest_auth.py::auth_database_envandtest_tasks.py::task_database_env/actor_id/set_dev_actor. Consider extracting a shared, parametrized fixture/helper module (e.g. inconftest.py) for the downgrade→upgrade→yield→downgrade pattern and the dev-actor env setup, since all three now do the same base↔head migration dance with only role/subject differences.🤖 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 25 - 118, The migration fixtures and dev-actor helpers are duplicated across test modules, especially the downgrade→upgrade→yield→downgrade flow in actor_database_env/actor_client and the env setup in set_dev_actor/alembic_config/actor_id. Move the shared Postgres migration and dev-auth setup into a common helper or conftest fixture, then have actor_database_env, auth_database_env, and task_database_env reuse it with parameters for roles, subject, and any test-specific actor identity values.backend/app/api/deps/auth.py (1)
59-76: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDB write on every authenticated request, including hot paths.
get_registered_actorperforms an identity/profile upsert plus acommit()on every call, and is wired intoclaim_task— a hot, frequently-polled endpoint for workers — as well as/auth/me. Consider only refreshing registry metadata periodically (e.g., skip the write if the identity was refreshed recently) rather than on every request, to avoid adding write latency/DB load to these paths.🤖 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/app/api/deps/auth.py` around lines 59 - 76, get_registered_actor is writing registry data on every authenticated request via ActorService.register_actor, which hits hot paths like claim_task and /auth/me. Update this dependency to avoid unconditional upserts/commit on every call by adding a freshness check or periodic refresh guard inside get_registered_actor or register_actor, so recently refreshed actors skip the DB write while still returning the same ActorContext.backend/app/modules/actors/models.py (1)
13-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck-constraint literals duplicate the Python tuples.
ACTOR_PROFILE_TYPES/ACTOR_PROFILE_STATUSESare declared as module constants, but theCheckConstraintSQL strings (lines 60-61, 64-65) hardcode the same values separately. If either tuple changes without updating the constraint string, the DB will silently diverge from application-level validation (e.g. an app-accepted new profile type gets rejected by Postgres, or vice versa).♻️ Build the constraint from the constants
+_PROFILE_TYPE_LIST = ", ".join(f"'{t}'" for t in ACTOR_PROFILE_TYPES) +_PROFILE_STATUS_LIST = ", ".join(f"'{s}'" for s in ACTOR_PROFILE_STATUSES) + __table_args__ = ( CheckConstraint( - "profile_type in ('worker', 'reviewer', 'admin', 'project_manager', 'project_owner')", + f"profile_type in ({_PROFILE_TYPE_LIST})", name="ck_actor_profiles_profile_type", ), CheckConstraint( - "status in ('observed', 'active', 'disabled')", + f"status in ({_PROFILE_STATUS_LIST})", name="ck_actor_profiles_status", ),Also applies to: 59-66
🤖 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/app/modules/actors/models.py` around lines 13 - 14, The check-constraint SQL in the model duplicates the values already defined by ACTOR_PROFILE_TYPES and ACTOR_PROFILE_STATUSES, so update the constraint definitions in the actors model to be derived from those constants instead of hardcoding the literals. Adjust the CheckConstraint setup where the profile type/status validation is declared so it reuses the module-level tuples, keeping the database rules aligned with the application-level constants. Use the existing symbols ACTOR_PROFILE_TYPES, ACTOR_PROFILE_STATUSES, and the model’s constraint declarations to locate and refactor the relevant code.backend/app/modules/actors/service.py (2)
178-179: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRedundant identity upsert on the worker profile activation route.
activate_worker_profilere-runsupsert_identityeven though the route'sget_registered_actordependency already calledregister_actor(which upserts identity) earlier in the same request. This causes two upsert+commit round trips for identity per activation call.🤖 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/app/modules/actors/service.py` around lines 178 - 179, The activate_worker_profile flow is redundantly upserting the same identity twice in one request because get_registered_actor already registers the actor before this method runs. Update activate_worker_profile in the Actor service to reuse the existing registered actor/identity from the request context instead of calling _repo.upsert_identity again, and keep the activation logic focused on profile activation only. Use the existing register_actor and _identity_from_actor symbols to locate the duplicate persistence path and remove the extra upsert+commit round trip.
143-147: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
orfallback silently ignores an explicit empty-dict override.
next_metadata = profile_metadata or existing.profile_metadatatreatsprofile_metadata={}the same asNone, so a caller intending to clear metadata to{}would have that silently dropped. No current caller passes{}(all pass non-empty dicts), so this is latent, but thedict | Nonesignature invites it.♻️ Suggested fix
- next_metadata = profile_metadata or existing.profile_metadata + next_metadata = existing.profile_metadata if profile_metadata is None else profile_metadata🤖 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/app/modules/actors/service.py` around lines 143 - 147, In the metadata update path, the `or` fallback in the actor service is treating an explicit empty dict the same as no value, so `profile_metadata={}` gets ignored instead of clearing metadata. Update the logic around `next_metadata` to distinguish `None` from an intentional empty mapping, and preserve the existing comparison/update flow in the `existing.profile_metadata` assignment so callers can explicitly set metadata to `{}`.
🤖 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/0012_actor_identity_profile_registry.py`:
- Around line 168-244: The backfill logic in _backfill_legacy_profiles is
overwriting actor_identities.last_seen_roles and last_claim_snapshot on each
legacy table pass, which loses previously collected role provenance for the same
actor_id. Update the upsert in _backfill_legacy_profiles so it merges the new
profile_type into the existing role list instead of replacing it, and preserve
prior claim snapshot provenance rather than resetting it to only the current
table. Use the actor_identities_table insert/on_conflict_do_update block and its
last_seen_roles/last_claim_snapshot fields as the place to adjust the merge
behavior.
In `@backend/app/api/deps/auth.py`:
- Around line 59-76: Handle registry failures in get_registered_actor by
catching exceptions from ActorService.register_actor before they escape the
dependency. Map ActorRegistryError (and any expected persistence/DB failure from
get_db_session) to a structured HTTP error here, or add a global exception
handler if that’s the preferred central fix. Keep the current actor return path
unchanged for success, and make sure the dependency remains the single place
where get_current_actor plus registry refresh is resolved.
In `@backend/app/modules/actors/repository.py`:
- Around line 25-78: The upsert_identity method in the ActorsRepository can
return a stale ActorIdentity instance because get_identity may reuse an existing
row from the session identity map. Update the fetch after the flush to load a
fresh row using self._session.get(ActorIdentity, identity.actor_id,
populate_existing=True), or explicitly refresh the persisted ActorIdentity
before returning it, so the returned object reflects the latest values written
by the insert/on_conflict_do_update path.
In `@backend/app/modules/actors/service.py`:
- Around line 63-71: The `register_actor()` flow is causing
`ActorProfile.updated_at` to change on every observation because
`ensure_observed_profile()` writes the profile row even when only
`ActorIdentity.last_seen_at` should move. Update `ensure_observed_profile()` in
`service.py` so the `observed` and non-`observed` branches only persist the
`ActorProfile` when status or metadata actually changes, and avoid any save path
that touches `updated_at` for no-op observations. Keep the existing behavior for
real profile changes, but ensure `register_actor()` can refresh identity state
without rewriting unchanged profiles.
---
Nitpick comments:
In `@backend/app/api/deps/auth.py`:
- Around line 59-76: get_registered_actor is writing registry data on every
authenticated request via ActorService.register_actor, which hits hot paths like
claim_task and /auth/me. Update this dependency to avoid unconditional
upserts/commit on every call by adding a freshness check or periodic refresh
guard inside get_registered_actor or register_actor, so recently refreshed
actors skip the DB write while still returning the same ActorContext.
In `@backend/app/modules/actors/models.py`:
- Around line 13-14: The check-constraint SQL in the model duplicates the values
already defined by ACTOR_PROFILE_TYPES and ACTOR_PROFILE_STATUSES, so update the
constraint definitions in the actors model to be derived from those constants
instead of hardcoding the literals. Adjust the CheckConstraint setup where the
profile type/status validation is declared so it reuses the module-level tuples,
keeping the database rules aligned with the application-level constants. Use the
existing symbols ACTOR_PROFILE_TYPES, ACTOR_PROFILE_STATUSES, and the model’s
constraint declarations to locate and refactor the relevant code.
In `@backend/app/modules/actors/service.py`:
- Around line 178-179: The activate_worker_profile flow is redundantly upserting
the same identity twice in one request because get_registered_actor already
registers the actor before this method runs. Update activate_worker_profile in
the Actor service to reuse the existing registered actor/identity from the
request context instead of calling _repo.upsert_identity again, and keep the
activation logic focused on profile activation only. Use the existing
register_actor and _identity_from_actor symbols to locate the duplicate
persistence path and remove the extra upsert+commit round trip.
- Around line 143-147: In the metadata update path, the `or` fallback in the
actor service is treating an explicit empty dict the same as no value, so
`profile_metadata={}` gets ignored instead of clearing metadata. Update the
logic around `next_metadata` to distinguish `None` from an intentional empty
mapping, and preserve the existing comparison/update flow in the
`existing.profile_metadata` assignment so callers can explicitly set metadata to
`{}`.
In `@backend/tests/test_actors.py`:
- Around line 25-118: The migration fixtures and dev-actor helpers are
duplicated across test modules, especially the downgrade→upgrade→yield→downgrade
flow in actor_database_env/actor_client and the env setup in
set_dev_actor/alembic_config/actor_id. Move the shared Postgres migration and
dev-auth setup into a common helper or conftest fixture, then have
actor_database_env, auth_database_env, and task_database_env reuse it with
parameters for roles, subject, and any test-specific actor identity values.
In `@backend/tests/test_auth.py`:
- Around line 41-63: The auth_database_env fixture duplicates the same Alembic
base→head→base setup already used by actor_database_env and task_database_env.
Extract the shared migration setup/teardown into a common conftest helper or
shared fixture, then have auth_database_env delegate to it while keeping the
existing db_session.dispose_engine, get_settings.cache_clear, and migration_lock
behavior intact.
🪄 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: 6640e446-5ab9-4aed-a385-7fb50ac7aff1
📒 Files selected for processing (39)
.agent-loop/LOOP_STATE.md.agent-loop/REVIEW_LOG.md.agent-loop/WORK_QUEUE.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/STATUS.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/chunks/WS-POL-001-11-actor-identity-profile-registry.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/reviews/WS-POL-001-11-internal-review-evidence.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/reviews/WS-POL-001-11-pr-trust-bundle.mdREADME.mdbackend/alembic/versions/0012_actor_identity_profile_registry.pybackend/app/api/deps/auth.pybackend/app/api/routes/auth.pybackend/app/api/routes/demo.pybackend/app/db/models.pybackend/app/modules/actors/__init__.pybackend/app/modules/actors/models.pybackend/app/modules/actors/repository.pybackend/app/modules/actors/schemas.pybackend/app/modules/actors/service.pybackend/app/modules/tasks/models.pybackend/app/modules/tasks/repository.pybackend/app/modules/tasks/router.pybackend/app/modules/tasks/schemas.pybackend/app/modules/tasks/service.pybackend/scripts/week1_api_e2e.pybackend/scripts/week1_dry_run.pybackend/scripts/week2_api_e2e.pybackend/tests/test_actors.pybackend/tests/test_alembic.pybackend/tests/test_auth.pybackend/tests/test_tasks.pydemos/week1_api_demo_ui/src/App.tsxdocs/architecture_data_model.mddocs/architecture_system_architecture.mddocs/glossary.mddocs/operations_roles_permissions.mddocs/spec_chunk_2_auth_actor_boundary.mddocs/spec_chunk_4_task_queue_assignment.mdexamples/terminal_benchmark/LOCAL_VALIDATION_NOTES.mdexamples/terminal_benchmark/terminal_benchmark_api_e2e.py
💤 Files with no reviewable changes (1)
- backend/app/modules/tasks/schemas.py
Comment on lines
+168
to
+244
| def _backfill_legacy_profiles(connection, table_name: str, profile_type: str) -> None: | ||
| """Backfill one legacy profile table into actor identities and profiles.""" | ||
| rows = ( | ||
| connection.execute( | ||
| sa.text( | ||
| f""" | ||
| select | ||
| id, | ||
| actor_id, | ||
| external_subject, | ||
| external_issuer, | ||
| display_name, | ||
| email, | ||
| skill_tags, | ||
| status, | ||
| created_at, | ||
| updated_at | ||
| from {table_name} | ||
| """ | ||
| ) | ||
| ) | ||
| .mappings() | ||
| .all() | ||
| ) | ||
| for row in rows: | ||
| roles = [profile_type] | ||
| connection.execute( | ||
| postgresql.insert(actor_identities_table) | ||
| .values( | ||
| actor_id=row["actor_id"], | ||
| external_subject=row["external_subject"], | ||
| external_issuer=row["external_issuer"], | ||
| display_name=row["display_name"], | ||
| email=row["email"], | ||
| last_seen_roles=roles, | ||
| last_claim_snapshot={"legacy_profile_backfill": table_name}, | ||
| auth_source="legacy_profile_backfill", | ||
| is_dev_auth=False, | ||
| ) | ||
| .on_conflict_do_update( | ||
| index_elements=["actor_id"], | ||
| set_={ | ||
| "external_subject": row["external_subject"], | ||
| "external_issuer": row["external_issuer"], | ||
| "display_name": row["display_name"], | ||
| "email": row["email"], | ||
| "last_seen_roles": roles, | ||
| "last_claim_snapshot": {"legacy_profile_backfill": table_name}, | ||
| "auth_source": "legacy_profile_backfill", | ||
| "is_dev_auth": False, | ||
| "last_seen_at": sa.func.now(), | ||
| "updated_at": sa.func.now(), | ||
| }, | ||
| ) | ||
| ) | ||
| connection.execute( | ||
| postgresql.insert(actor_profiles_table) | ||
| .values( | ||
| id=row["id"], | ||
| actor_id=row["actor_id"], | ||
| profile_type=profile_type, | ||
| status=row["status"], | ||
| skill_tags=row["skill_tags"] or [], | ||
| scope_type="global", | ||
| scope_id="global", | ||
| profile_metadata={"legacy_profile_backfill": table_name}, | ||
| ) | ||
| .on_conflict_do_update( | ||
| index_elements=["actor_id", "profile_type", "scope_type", "scope_id"], | ||
| set_={ | ||
| "status": row["status"], | ||
| "skill_tags": row["skill_tags"] or [], | ||
| "profile_metadata": {"legacy_profile_backfill": table_name}, | ||
| "updated_at": sa.func.now(), | ||
| }, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## files\n'
git ls-files 'backend/alembic/versions/0012_actor_identity_profile_registry.py' \
'backend/**/*.py' | sed -n '1,200p'
printf '\n## search for legacy profile tables and metadata fields\n'
rg -n --hidden --glob 'backend/**/*.py' \
'worker_profiles|reviewer_profiles|last_seen_roles|last_claim_snapshot|actor_identities_table|actor_profiles_table|profile_type' backend
printf '\n## read migration file around the backfill call and function\n'
FILE='backend/alembic/versions/0012_actor_identity_profile_registry.py'
wc -l "$FILE"
sed -n '1,280p' "$FILE"Repository: Flow-Research/workstream
Length of output: 32056
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## actor model and service semantics\n'
sed -n '1,180p' backend/app/modules/actors/models.py
printf '\n---\n'
sed -n '240,380p' backend/app/modules/actors/service.py
printf '\n## alembic migration tests around legacy tables and backfill\n'
sed -n '250,620p' backend/tests/test_alembic.py
printf '\n## actor tests that exercise multi-role identity state\n'
sed -n '120,220p' backend/tests/test_actors.pyRepository: Flow-Research/workstream
Length of output: 25886
Merge legacy role metadata during backfill
worker_profiles and reviewer_profiles can both exist for the same actor_id, so the second pass overwrites actor_identities.last_seen_roles/last_claim_snapshot with a single-role snapshot. That leaves multi-role actors with misleading audit metadata until their next login refreshes it. Merge the role list and preserve prior claim provenance instead of replacing 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 `@backend/alembic/versions/0012_actor_identity_profile_registry.py` around
lines 168 - 244, The backfill logic in _backfill_legacy_profiles is overwriting
actor_identities.last_seen_roles and last_claim_snapshot on each legacy table
pass, which loses previously collected role provenance for the same actor_id.
Update the upsert in _backfill_legacy_profiles so it merges the new profile_type
into the existing role list instead of replacing it, and preserve prior claim
snapshot provenance rather than resetting it to only the current table. Use the
actor_identities_table insert/on_conflict_do_update block and its
last_seen_roles/last_claim_snapshot fields as the place to adjust the merge
behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/alembic/versions/0012_actor_identity_profile_registry.py (1)
19-83: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRestore the legacy-profile backfill before dropping these tables.
upgrade()deletesworker_profilesandreviewer_profilesoutright, but the new flow only registers the token-derived actor and requires an existing activeActorProfileto claim tasks. That permanently loses priorskill_tags, status, and timestamps, and existing workers will stop qualifying until they re-activate in the new API.🤖 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/alembic/versions/0012_actor_identity_profile_registry.py` around lines 19 - 83, The `upgrade()` migration drops `worker_profiles` and `reviewer_profiles` too early, causing existing profile data to be lost before it is copied into the new actor registry. Add a backfill step in `upgrade()` for the `actor_identities` / `actor_profiles` transition so legacy rows from `worker_profiles` and `reviewer_profiles` are migrated into `ActorProfile` records (preserving `skill_tags`, status, and timestamps) before the `drop_table` calls. Use the existing migration symbols `upgrade`, `actor_profiles`, `worker_profiles`, and `reviewer_profiles` to place the copy logic immediately before the destructive cleanup.backend/app/modules/actors/service.py (1)
55-92: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winN+1 profile queries on every
get_registered_actorcall.
_can_skip_registry_refreshissues oneget_profilequery per required profile type/relationship in addition to the identity fetch. Sinceget_registered_actoris now the dependency for nearly all authenticated routes (projects, tasks, checkers), this adds several sequential DB round trips to the hot path of most API requests — even when the freshness check ultimately decides to skip all writes.list_profiles(actor_id)already fetches all of an actor's profiles in a single query and can replace the per-profile loop.⚡ Proposed fix using a single `list_profiles` query
required_profiles.extend(self._trusted_relationship_profiles(actor)) - for required_profile in required_profiles: - profile = await self._repo.get_profile( - actor.actor_id, - required_profile["profile_type"], - required_profile["scope_type"], - required_profile["scope_id"], - ) - if profile is None: + existing_profiles = { + (p.profile_type, p.scope_type, p.scope_id): p + for p in await self._repo.list_profiles(actor.actor_id) + } + for required_profile in required_profiles: + profile = existing_profiles.get( + ( + required_profile["profile_type"], + required_profile["scope_type"], + required_profile["scope_id"], + ) + ) + if profile is None: return False if ( profile.status == "observed" and profile.profile_metadata != required_profile.get("profile_metadata") ): return False return TrueAlso applies to: 282-322
🤖 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/app/modules/actors/service.py` around lines 55 - 92, The freshness check in register_actor/_can_skip_registry_refresh is doing N+1 profile lookups by calling get_profile repeatedly on the hot path. Update _can_skip_registry_refresh to use a single list_profiles(actor_id) query, build the needed profile set from that result, and keep the existing identity fetch/skip logic intact so get_registered_actor avoids sequential DB round trips when no refresh is needed.
🧹 Nitpick comments (2)
backend/app/modules/actors/service.py (2)
414-446: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
project_ownerclaim validation logic.This loop repeats the same dict/type/trim/empty-string validation for
workstream_relationship_profilesentries already implemented insanitized_claim_snapshot(backend/app/schemas/auth.py). Consider extracting a shared helper that yields validated(profile_type, scope_type, scope_id)tuples, with each caller attaching its own metadata, to avoid the two implementations drifting on a security-relevant claim 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/app/modules/actors/service.py` around lines 414 - 446, The `_trusted_relationship_profiles` validation for `workstream_relationship_profiles` duplicates the same `project_owner` sanitization already handled in `sanitized_claim_snapshot`, so the two paths can drift. Extract or reuse a shared helper for validating and normalizing the claim entries into `(profile_type, scope_type, scope_id)` values, then have both `ActorService._trusted_relationship_profiles` and the auth schema path attach their own metadata as needed.
452-488: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueActor audit writes go through
TaskRepository.
self._audit_repo = TaskRepository(session)(line 53) is reused here purely foradd_audit_event, even though these are actor-profile audit events unrelated to tasks. Consider extracting a genericAuditRepository(or renaming/relocating the method) soActorServicedoesn't depend on the tasks module for an unrelated concern.🤖 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/app/modules/actors/service.py` around lines 452 - 488, ActorService is using TaskRepository for actor-profile audit writes, which couples unrelated task storage to audit handling. Refactor the audit path in _write_profile_audit to use a dedicated AuditRepository (or move the audit persistence behind a generic audit-specific dependency) and update the ActorService constructor/field setup so add_audit_event is no longer sourced from the tasks module.
🤖 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 `@docs/internal_reviews/2026-06-13_week1_week2_deterministic_hardening.md`:
- Around line 117-124: The quoted CLI result in this doc does not match the
actual output from api_contract_e2e.py. Update the text in the deterministic
hardening section to use the exact string printed by scripts/api_contract_e2e.py
(the message containing “real API”), and keep the second quoted result aligned
with the script’s actual PASS output so the documentation reflects
evidence-backed output rather than paraphrased wording.
---
Outside diff comments:
In `@backend/alembic/versions/0012_actor_identity_profile_registry.py`:
- Around line 19-83: The `upgrade()` migration drops `worker_profiles` and
`reviewer_profiles` too early, causing existing profile data to be lost before
it is copied into the new actor registry. Add a backfill step in `upgrade()` for
the `actor_identities` / `actor_profiles` transition so legacy rows from
`worker_profiles` and `reviewer_profiles` are migrated into `ActorProfile`
records (preserving `skill_tags`, status, and timestamps) before the
`drop_table` calls. Use the existing migration symbols `upgrade`,
`actor_profiles`, `worker_profiles`, and `reviewer_profiles` to place the copy
logic immediately before the destructive cleanup.
In `@backend/app/modules/actors/service.py`:
- Around line 55-92: The freshness check in
register_actor/_can_skip_registry_refresh is doing N+1 profile lookups by
calling get_profile repeatedly on the hot path. Update
_can_skip_registry_refresh to use a single list_profiles(actor_id) query, build
the needed profile set from that result, and keep the existing identity
fetch/skip logic intact so get_registered_actor avoids sequential DB round trips
when no refresh is needed.
---
Nitpick comments:
In `@backend/app/modules/actors/service.py`:
- Around line 414-446: The `_trusted_relationship_profiles` validation for
`workstream_relationship_profiles` duplicates the same `project_owner`
sanitization already handled in `sanitized_claim_snapshot`, so the two paths can
drift. Extract or reuse a shared helper for validating and normalizing the claim
entries into `(profile_type, scope_type, scope_id)` values, then have both
`ActorService._trusted_relationship_profiles` and the auth schema path attach
their own metadata as needed.
- Around line 452-488: ActorService is using TaskRepository for actor-profile
audit writes, which couples unrelated task storage to audit handling. Refactor
the audit path in _write_profile_audit to use a dedicated AuditRepository (or
move the audit persistence behind a generic audit-specific dependency) and
update the ActorService constructor/field setup so add_audit_event is no longer
sourced from the tasks module.
🪄 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: 442248ac-79d0-4f4d-b1f6-e45c671a498f
⛔ Files ignored due to path filters (1)
demos/week1_api_demo_ui/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (56)
.agent-loop/REVIEW_LOG.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/CHUNK_MAP.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/chunks/WS-POL-001-11-actor-identity-profile-registry.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/reviews/WS-POL-001-11-internal-review-evidence.md.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/reviews/WS-POL-001-11-pr-trust-bundle.md.github/workflows/backend.yml.github/workflows/week1-api-demo-ui.ymlREADME.mdbackend/alembic/versions/0012_actor_identity_profile_registry.pybackend/app/adapters/auth/flow.pybackend/app/api/deps/auth.pybackend/app/api/router.pybackend/app/api/routes/demo.pybackend/app/core/config.pybackend/app/modules/actors/__init__.pybackend/app/modules/actors/models.pybackend/app/modules/actors/repository.pybackend/app/modules/actors/schemas.pybackend/app/modules/actors/service.pybackend/app/modules/checkers/router.pybackend/app/modules/projects/router.pybackend/app/modules/projects/service.pybackend/app/modules/tasks/router.pybackend/app/schemas/auth.pybackend/scripts/api_contract_e2e.pybackend/scripts/week1_dry_run.pybackend/scripts/week2_api_e2e.pybackend/tests/test_actors.pybackend/tests/test_alembic.pybackend/tests/test_auth.pybackend/tests/test_projects.pybackend/tests/test_tasks.pydemos/week1_api_demo_ui/index.htmldemos/week1_api_demo_ui/package.jsondemos/week1_api_demo_ui/src/App.tsxdemos/week1_api_demo_ui/src/api.tsdemos/week1_api_demo_ui/src/main.tsxdemos/week1_api_demo_ui/src/styles.cssdemos/week1_api_demo_ui/tsconfig.app.jsondemos/week1_api_demo_ui/tsconfig.jsondemos/week1_api_demo_ui/vite.config.tsdocs/architecture_brief/workstream_architecture_brief.mddocs/architecture_data_model.mddocs/diagrams/workstream_v01_container.mddocs/glossary.mddocs/internal_reviews/2026-06-12_week2_closeout_real_api_drill.mddocs/internal_reviews/2026-06-13_week1_week2_deterministic_hardening.mddocs/operations_roles_permissions.mddocs/roadmap_30_day_master_plan.mddocs/roadmap_day_by_day_execution_plan.mddocs/roadmap_status.mddocs/spec_chunk_2_auth_actor_boundary.mddocs/spec_week2_checker_framework.mdexamples/terminal_benchmark/terminal_benchmark_api_e2e.pyscripts/check_internal_review_evidence.pyscripts/test_agent_gates.py
💤 Files with no reviewable changes (17)
- .github/workflows/week1-api-demo-ui.yml
- demos/week1_api_demo_ui/package.json
- demos/week1_api_demo_ui/src/main.tsx
- demos/week1_api_demo_ui/tsconfig.json
- backend/app/modules/actors/init.py
- demos/week1_api_demo_ui/vite.config.ts
- demos/week1_api_demo_ui/index.html
- demos/week1_api_demo_ui/src/styles.css
- demos/week1_api_demo_ui/tsconfig.app.json
- backend/app/api/router.py
- backend/app/modules/projects/service.py
- demos/week1_api_demo_ui/src/api.ts
- backend/app/api/routes/demo.py
- backend/app/modules/actors/schemas.py
- demos/week1_api_demo_ui/src/App.tsx
- backend/scripts/week1_dry_run.py
- README.md
✅ Files skipped from review due to trivial changes (10)
- docs/diagrams/workstream_v01_container.md
- docs/architecture_brief/workstream_architecture_brief.md
- docs/spec_week2_checker_framework.md
- docs/roadmap_30_day_master_plan.md
- docs/internal_reviews/2026-06-12_week2_closeout_real_api_drill.md
- docs/roadmap_day_by_day_execution_plan.md
- docs/glossary.md
- docs/roadmap_status.md
- .agent-loop/REVIEW_LOG.md
- .agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/CHUNK_MAP.md
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/app/api/deps/auth.py
- backend/app/modules/actors/models.py
- docs/architecture_data_model.md
- docs/spec_chunk_2_auth_actor_boundary.md
- .agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/reviews/WS-POL-001-11-pr-trust-bundle.md
- backend/tests/test_tasks.py
abiorh-claw
approved these changes
Jul 7, 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.
PR Trust Bundle: WS-POL-001-11
Chunk
WS-POL-001-11- Actor Identity And Profile RegistryGoal
Implement Workstream's local actor identity and shared actor profile registry for verified Flow actors without turning Workstream into the auth provider or letting persisted profiles become route permissions.
Human-Approved Intent
The user asked for one shared actor/profile model instead of separate worker, reviewer, admin, project-manager, and project-owner implementations. The verified Flow token remains the authority for route access. Workstream stores local actor/profile rows for workflow eligibility, audit, display, future routing, and later reputation linkage.
Chunk contract:
.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/chunks/WS-POL-001-11-actor-identity-profile-registry.mdWhat Changed
actor_identitiesandactor_profileswith async SQLAlchemy models, repository, schemas, service, and Alembic migration.worker_profilesandreviewer_profilesinto the shared actor registry and removed the old profile tables as profile authority.get_current_actorpure and addedget_registered_actorfor explicit registry side effects./auth/me,/workers/me/profile, and task claim paths to register actor identity/profile metadata where needed.workertoken role plus activeActorProfile(profile_type="worker").POST /api/v1/workers/me/profile.workstream_relationship_profilesclaim shape for scoped project-owner metadata.Why It Changed
The live Terminal Benchmark drill proved worker profile setup must be real. Doing only a worker-specific profile path would create the same problem again for reviewers, project managers, admins, project owners, audit, and later reputation. This chunk creates the shared actor/profile base before the next live API drill.
Design Chosen
ActorIdentitystores local durable identity rows for verified Flow actors.ActorProfilestores profile type, status, skill tags, scope, and non-authoritative metadata.observedrecords token-observed metadata for audit/display only.activeis explicit workflow eligibility and still requires the matching current token role.disabledblocks workflow eligibility and is preserved by observation refresh.ActorContext.roles, not database profile rows.audit_eventsledger throughTaskRepository.add_audit_event.Alternatives Rejected
Scope Control
Implemented only actor identity/profile registry, migration/backfill/removal, touched-route registration, worker profile activation, worker claim eligibility, tests, demo/script cleanup required by the migration, and documentation alignment.
No Workstream-owned login/signup/session/password behavior was added. No post-submit, review, revision, payment, reputation, blockchain, object storage, agent runtime, or frontend product implementation was added.
Product Behavior
accept,needs_revision, andreject.Acceptance Criteria Proof
/auth/meregisters identity/profile metadata without Workstream auth sessions./api/v1/demo/worker-profile.Tests/Checks Run
Result summary:
Test Delta
backend/tests/test_actors.py.CI Integrity
Reviewer Results
Internal review evidence:
.agent-loop/initiatives/WS-POL-001-submission-artifact-policy-foundation/reviews/WS-POL-001-11-internal-review-evidence.mdReviewed code SHA:
912a1bef3ce7065e9563a03a440b24efe6af3f89Reviewer run IDs: see
WS-POL-001-11-internal-review-evidence.md.uuid4and does not weaken CI or tests.External Review
External review has not run yet. CodeRabbit and GitHub checks should run after the PR is opened.
Remaining Risks
get_current_actoruntil deliberately migrated.Follow-Up Work
Human Review Focus
Please inspect:
ActorProfilestatus semantics and preservation of explicit profile metadata.Human Merge Ownership
Only the user can approve and merge this PR. Codex must not merge it without explicit user approval for that specific PR.
Summary by CodeRabbit
POST /api/v1/workers/me/profileflow.