Add idempotent authority mutation foundation#119
Conversation
📝 WalkthroughWalkthroughThis PR adds authority mutation idempotency for ten operations, including strict request schemas, canonical digests, PostgreSQL-backed reservation and completion, linked audit evidence, invalidation events, mismatch denial handling, migration safeguards, integration tests, and updated workstream documentation. ChangesAuthority mutation idempotency
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant AuthorityMutationService
participant AuthorityIdempotencyRepository
participant PostgreSQL
participant audit_events
Caller->>AuthorityMutationService: Submit canonical authority mutation
AuthorityMutationService->>AuthorityIdempotencyRepository: Reserve namespace and digest
AuthorityIdempotencyRepository->>PostgreSQL: Insert or lock idempotency record
PostgreSQL-->>AuthorityIdempotencyRepository: Claimed, replay, or mismatch
AuthorityMutationService->>audit_events: Write success and invalidation evidence
AuthorityMutationService->>AuthorityIdempotencyRepository: Complete pending claim
AuthorityIdempotencyRepository->>PostgreSQL: Commit immutable response reference
Possibly related PRs
Suggested reviewers: 🚥 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 |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.agent-loop/LOOP_STATE.md:
- Around line 11-17: Synchronize the lifecycle artifacts with published PR `#119`
by removing the stale “PR publication pending” state and retaining only checks,
external review, human approval, merge, memory-update, and stop gates. Update
.agent-loop/LOOP_STATE.md lines 11-17, .agent-loop/WORK_QUEUE.md lines 7 and
64-67, CHUNK_MAP.md lines 24 and 112-114, and STATUS.md lines 62-65, 74-75, 96,
and 127-129; ensure each status, progress summary, chunk entry, proposed-next
narrative, active gate, blocker narrative, and stop condition reflects the
post-publication state.
In `@backend/app/modules/authorization/models.py`:
- Around line 19-79: Update the constraint names in
AuthorityIdempotencyRecord.__table_args__ to match the migration: rename the
replay namespace UniqueConstraint to
uq_authority_idempotency_records_replay_namespace and the id/actor reference
UniqueConstraint to uq_authority_idempotency_records_actor_reference. Also
rename the separate actor-reference CheckConstraint so it no longer conflicts
with the unique constraint, preserving the migration’s corresponding
check-constraint name.
In `@backend/tests/test_authorization.py`:
- Around line 1014-1041: Update the test cleanup around the loser tasks in the
authorization scenario to always cancel and await whichever loser task is active
before disposing winner_engine and loser_engine. Ensure this cleanup runs when
lock observation, _complete, or commit raises, while preserving the existing
ReplayedReservation and MismatchedReservation assertions on successful
execution.
🪄 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: 94a9771f-80d2-4175-a231-ad06d05cd43b
📒 Files selected for processing (20)
.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/STATUS.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-05B-idempotency-invalidation.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-05B-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-05B-pr-trust-bundle.mdbackend/alembic/versions/0019_authority_idempotency.pybackend/app/db/models.pybackend/app/modules/authorization/__init__.pybackend/app/modules/authorization/models.pybackend/app/modules/authorization/repository.pybackend/app/modules/authorization/schemas.pybackend/app/modules/authorization/service.pybackend/tests/test_alembic.pybackend/tests/test_audit.pybackend/tests/test_authorization.pydocs/architecture_data_model.mddocs/operations_authorization_service.md
| - Status: CAT post-merge memory merged through PR #118 as `eba7e2b` on | ||
| 2026-07-14. `WS-AUTH-001-05B` implementation and repair are complete at | ||
| reviewed runtime SHA `e083890`; every required internal review track passed. | ||
| - Prior `WS-AUTH-001-01` reviewed implementation SHA: `be0b836` | ||
| - Prior `WS-AUTH-001-01` final merged branch head: `b5217e1` | ||
| - Latest integrated `main` merge commit: `4c5d4fc` | ||
| - Current gate: CAT post-merge memory and stop; no runtime implementation chunk | ||
| is active. | ||
| - Latest integrated `main` merge commit: `eba7e2b` | ||
| - Current gate: PR publication, GitHub checks, CodeRabbit, and human review. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Synchronize all current lifecycle artifacts with published PR #119.
The same stale “PR publication pending” state appears across the active loop documents. Since the supplied PR objective identifies this review as PR #119, publication is complete; retain only the remaining checks, external review, human approval, merge, memory-update, and stop gates.
.agent-loop/LOOP_STATE.md#L11-L17: update the current gate and status..agent-loop/WORK_QUEUE.md#L7-L7: change the AUTH-05B status..agent-loop/WORK_QUEUE.md#L64-L67: update the proposed-next narrative..agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md#L24-L24: update the chunk status..agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md#L112-L114: update the stop-condition wording..agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md#L62-L65: update the progress summary..agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md#L74-L75: update the active implementation gate..agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md#L96-L96: update the chunk table..agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md#L127-L129: update the blocker/gate narrative.
📍 Affects 4 files
.agent-loop/LOOP_STATE.md#L11-L17(this comment).agent-loop/WORK_QUEUE.md#L7-L7.agent-loop/WORK_QUEUE.md#L64-L67.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md#L24-L24.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md#L112-L114.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md#L62-L65.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md#L74-L75.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md#L96-L96.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/STATUS.md#L127-L129
🤖 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 @.agent-loop/LOOP_STATE.md around lines 11 - 17, Synchronize the lifecycle
artifacts with published PR `#119` by removing the stale “PR publication pending”
state and retaining only checks, external review, human approval, merge,
memory-update, and stop gates. Update .agent-loop/LOOP_STATE.md lines 11-17,
.agent-loop/WORK_QUEUE.md lines 7 and 64-67, CHUNK_MAP.md lines 24 and 112-114,
and STATUS.md lines 62-65, 74-75, 96, and 127-129; ensure each status, progress
summary, chunk entry, proposed-next narrative, active gate, blocker narrative,
and stop condition reflects the post-publication state.
| __table_args__ = ( | ||
| UniqueConstraint( | ||
| "actor_ref_kind", "actor_ref", "operation", "idempotency_key", | ||
| name="replay_namespace", | ||
| ), | ||
| UniqueConstraint("id", "actor_ref_kind", "actor_ref", name="actor_reference"), | ||
| CheckConstraint( | ||
| "actor_ref_kind in ('legacy_actor', 'actor_profile', 'system_principal')", | ||
| name="actor_kind", | ||
| ), | ||
| CheckConstraint( | ||
| "((actor_ref_kind = 'system_principal' and actor_ref = " | ||
| "'workstream:system:bootstrap') or (actor_ref_kind <> 'system_principal' and " | ||
| "actor_ref ~ '^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$'))", | ||
| name="actor_reference", | ||
| ), | ||
| CheckConstraint( | ||
| "operation in ('service_actor.create', 'admin_role_grant.issue', " | ||
| "'admin_role_grant.revoke', 'project_role_grant.issue', " | ||
| "'project_role_grant.revoke', 'actor_profile.suspend', " | ||
| "'actor_profile.reactivate', 'actor_profile.deactivate', " | ||
| "'actor_identity_link.revoke', 'actor_identity_link.reactivate')", | ||
| name="operation", | ||
| ), | ||
| CheckConstraint("request_digest ~ '^sha256:[0-9a-f]{64}$'", name="request_digest"), | ||
| CheckConstraint("status in ('pending', 'committed')", name="status"), | ||
| CheckConstraint( | ||
| "response_resource_version is null or response_resource_version > 0", | ||
| name="response_version", | ||
| ), | ||
| CheckConstraint( | ||
| "(status = 'pending' and response_resource_type is null and " | ||
| "response_resource_id is null and response_resource_version is null and " | ||
| "response_http_status is null and committed_at is null) or " | ||
| "(status = 'committed' and response_resource_type is not null and " | ||
| "response_resource_id is not null and response_http_status is not null and " | ||
| "committed_at is not null)", | ||
| name="state_shape", | ||
| ), | ||
| CheckConstraint( | ||
| "response_http_status is null or ((operation in ('service_actor.create', " | ||
| "'admin_role_grant.issue', 'project_role_grant.issue') and " | ||
| "response_http_status = 201) or (operation not in ('service_actor.create', " | ||
| "'admin_role_grant.issue', 'project_role_grant.issue') and " | ||
| "response_http_status = 200))", | ||
| name="response_status", | ||
| ), | ||
| CheckConstraint( | ||
| "(operation = 'service_actor.create' and (response_resource_type is null or " | ||
| "response_resource_type = 'actor_profile')) or " | ||
| "(operation like 'admin_role_grant.%' and (response_resource_type is null or " | ||
| "response_resource_type = 'admin_role_grant')) or " | ||
| "(operation like 'project_role_grant.%' and (response_resource_type is null or " | ||
| "response_resource_type = 'project_role_grant')) or " | ||
| "(operation like 'actor_profile.%' and (response_resource_type is null or " | ||
| "response_resource_type = 'actor_profile')) or " | ||
| "(operation like 'actor_identity_link.%' and (response_resource_type is null or " | ||
| "response_resource_type = 'actor_identity_link'))", | ||
| name="response_type", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any backend test fixture creates schema via ORM metadata rather than Alembic.
rg -n 'create_all|metadata\.create_all' backend/tests backend/app 2>/dev/nullRepository: Flow-Research/workstream
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== model excerpt =="
nl -ba backend/app/modules/authorization/models.py | sed -n '1,140p'
echo
echo "== migration excerpt =="
nl -ba backend/alembic/versions/0019_authority_idempotency.py | sed -n '1,220p'
echo
echo "== search for the constraint names =="
rg -n 'name="actor_reference"|name="replay_namespace"|uq_authority_idempotency_records_actor_reference|uq_authority_idempotency_records_replay_namespace' backend/app backend/alembic/versionsRepository: Flow-Research/workstream
Length of output: 222
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== model excerpt =="
sed -n '1,140p' backend/app/modules/authorization/models.py | cat -n
echo
echo "== migration excerpt =="
sed -n '1,220p' backend/alembic/versions/0019_authority_idempotency.py | cat -n
echo
echo "== search for the constraint names =="
rg -n 'name="actor_reference"|name="replay_namespace"|uq_authority_idempotency_records_actor_reference|uq_authority_idempotency_records_replay_namespace' backend/app backend/alembic/versionsRepository: Flow-Research/workstream
Length of output: 18845
Rename the model constraint names to match the migration
AuthorityIdempotencyRecord defines both a UniqueConstraint and a CheckConstraint as actor_reference, and the unique-constraint name on replay_namespace also differs from backend/alembic/versions/0019_authority_idempotency.py. Make the model use the same unique names as the migration (uq_authority_idempotency_records_replay_namespace and uq_authority_idempotency_records_actor_reference) to avoid metadata collisions and Alembic drift.
🤖 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/authorization/models.py` around lines 19 - 79, Update the
constraint names in AuthorityIdempotencyRecord.__table_args__ to match the
migration: rename the replay namespace UniqueConstraint to
uq_authority_idempotency_records_replay_namespace and the id/actor reference
UniqueConstraint to uq_authority_idempotency_records_actor_reference. Also
rename the separate actor-reference CheckConstraint so it no longer conflicts
with the unique constraint, preserving the migration’s corresponding
check-constraint name.
| try: | ||
| async with winner_factory() as winner: | ||
| service = AuthorityMutationService(winner) | ||
| claim = await _claim(service, actor, key, request) | ||
| loser = asyncio.create_task(lose(request)) | ||
| await asyncio.wait_for( | ||
| _wait_for_database_lock(authorization_database_env, "auth05b-loser"), | ||
| timeout=5, | ||
| ) | ||
| await _complete(service, claim, request) | ||
| await winner.commit() | ||
| assert isinstance(await asyncio.wait_for(loser, timeout=5), ReplayedReservation) | ||
|
|
||
| key = uuid4() | ||
| async with winner_factory() as winner: | ||
| service = AuthorityMutationService(winner) | ||
| claim = await _claim(service, actor, key, request) | ||
| loser = asyncio.create_task(lose(_request())) | ||
| await asyncio.wait_for( | ||
| _wait_for_database_lock(authorization_database_env, "auth05b-loser"), | ||
| timeout=5, | ||
| ) | ||
| await _complete(service, claim, request) | ||
| await winner.commit() | ||
| assert isinstance(await asyncio.wait_for(loser, timeout=5), MismatchedReservation) | ||
| finally: | ||
| await winner_engine.dispose() | ||
| await loser_engine.dispose() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Always cancel and await the active loser task during cleanup.
If lock observation, completion, or commit fails before the explicit await, the task can retain its session and connection past engine disposal.
Proposed cleanup
+ loser_task = None
try:
async with winner_factory() as winner:
...
- loser = asyncio.create_task(lose(request))
+ loser_task = asyncio.create_task(lose(request))
...
- assert isinstance(await asyncio.wait_for(loser, timeout=5), ReplayedReservation)
+ assert isinstance(
+ await asyncio.wait_for(loser_task, timeout=5),
+ ReplayedReservation,
+ )
...
- loser = asyncio.create_task(lose(_request()))
+ loser_task = asyncio.create_task(lose(_request()))
...
- assert isinstance(await asyncio.wait_for(loser, timeout=5), MismatchedReservation)
+ assert isinstance(
+ await asyncio.wait_for(loser_task, timeout=5),
+ MismatchedReservation,
+ )
finally:
+ if loser_task is not None:
+ if not loser_task.done():
+ loser_task.cancel()
+ await asyncio.gather(loser_task, return_exceptions=True)
await winner_engine.dispose()
await loser_engine.dispose()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| async with winner_factory() as winner: | |
| service = AuthorityMutationService(winner) | |
| claim = await _claim(service, actor, key, request) | |
| loser = asyncio.create_task(lose(request)) | |
| await asyncio.wait_for( | |
| _wait_for_database_lock(authorization_database_env, "auth05b-loser"), | |
| timeout=5, | |
| ) | |
| await _complete(service, claim, request) | |
| await winner.commit() | |
| assert isinstance(await asyncio.wait_for(loser, timeout=5), ReplayedReservation) | |
| key = uuid4() | |
| async with winner_factory() as winner: | |
| service = AuthorityMutationService(winner) | |
| claim = await _claim(service, actor, key, request) | |
| loser = asyncio.create_task(lose(_request())) | |
| await asyncio.wait_for( | |
| _wait_for_database_lock(authorization_database_env, "auth05b-loser"), | |
| timeout=5, | |
| ) | |
| await _complete(service, claim, request) | |
| await winner.commit() | |
| assert isinstance(await asyncio.wait_for(loser, timeout=5), MismatchedReservation) | |
| finally: | |
| await winner_engine.dispose() | |
| await loser_engine.dispose() | |
| loser_task = None | |
| try: | |
| async with winner_factory() as winner: | |
| service = AuthorityMutationService(winner) | |
| claim = await _claim(service, actor, key, request) | |
| loser_task = asyncio.create_task(lose(request)) | |
| await asyncio.wait_for( | |
| _wait_for_database_lock(authorization_database_env, "auth05b-loser"), | |
| timeout=5, | |
| ) | |
| await _complete(service, claim, request) | |
| await winner.commit() | |
| assert isinstance( | |
| await asyncio.wait_for(loser_task, timeout=5), | |
| ReplayedReservation, | |
| ) | |
| key = uuid4() | |
| async with winner_factory() as winner: | |
| service = AuthorityMutationService(winner) | |
| claim = await _claim(service, actor, key, request) | |
| loser_task = asyncio.create_task(lose(_request())) | |
| await asyncio.wait_for( | |
| _wait_for_database_lock(authorization_database_env, "auth05b-loser"), | |
| timeout=5, | |
| ) | |
| await _complete(service, claim, request) | |
| await winner.commit() | |
| assert isinstance( | |
| await asyncio.wait_for(loser_task, timeout=5), | |
| MismatchedReservation, | |
| ) | |
| finally: | |
| if loser_task is not None: | |
| if not loser_task.done(): | |
| loser_task.cancel() | |
| await asyncio.gather(loser_task, return_exceptions=True) | |
| await winner_engine.dispose() | |
| await loser_engine.dispose() |
🤖 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_authorization.py` around lines 1014 - 1041, Update the
test cleanup around the loser tasks in the authorization scenario to always
cancel and await whichever loser task is active before disposing winner_engine
and loser_engine. Ensure this cleanup runs when lock observation, _complete, or
commit raises, while preserving the existing ReplayedReservation and
MismatchedReservation assertions on successful execution.
WS-AUTH-001-05B PR Trust Bundle
Chunk
WS-AUTH-001-05B- Authority Idempotency And Invalidation FoundationGoal
Make the ten sensitive authority mutations concurrency-safe and retry-safe,
with typed replay results and exactly linked success/invalidation evidence.
Human-Approved Intent
The human approved the repaired L1 contract and its semantic chunk boundary.
The work remains backend foundation only; AUTH-06 must not start automatically.
What Changed
0019adds immutable actor-scoped idempotency reservations, apending-to-committed transition, typed replay references, database timestamps,
a deferred pending guard, and guarded downgrade custody.
only canonical, bounded, server-derived facts.
mismatches, validates concrete success evidence, constructs invalidations,
and completes the claim in one caller-owned transaction.
cause, request, correlation, and project context and closes committed records
to later evidence.
exact and mismatched concurrency, rollback, privacy, direct bypasses,
immutable state, and upgrade/downgrade races.
Why It Changed
Later actor, grant, and permission chunks need a trustworthy retry boundary.
Without it, a lost response or concurrent retry could duplicate authority
state, disclose another request's result, or leave misleading audit evidence.
Design Chosen
The unique namespace is actor kind, actor reference, operation, and client key.
Only the canonical request digest and typed response reference persist. The
existing shared audit service remains the sole application writer, while
PostgreSQL independently enforces linkage and immutability. Repositories never
commit, roll back, retry, or create sessions.
Alternatives Rejected
stability.
direct SQL must fail closed.
shared authority evidence envelope.
later owning chunks.
Scope Control
No API route, permission evaluator, actor/grant table, token verifier, cache,
queue, invalidation consumer, product authorization decision, dependency,
workflow, or CI threshold changed.
Product Behavior
There is no new externally reachable product behavior. Internal callers can
claim a mutation, receive an exact committed replay, or receive a private
mismatch signal. Completion requires one concrete success plus one linked
invalidation event.
Acceptance Criteria Proof
committed result and one evidence pair.
database-linked proof.
cross-actor, wrong-entity, wrong-target, and mismatched-cause paths fail.
delete, and truncate.
diagnostics, public state, cause, and context.
non-empty custody, preserves compatible pre-0019 rows, and restores head.
Tests And Checks Run
links, internal review evidence structure, and diff integrity passed or are
rerun after this bundle is finalized.
no failure before intentional interruption after two hours on an overloaded
four-core host. GitHub Backend owns the authoritative full-suite and global
coverage result.
Test Delta
Tests are additive or strengthened. Existing audit assertions were adapted for
mandatory idempotency linkage, and positive operation coverage replaced weaker
assertions. No raises guard, skip, xfail, coverage threshold, or test selection
was removed or weakened.
CI Integrity
No workflow, dependency, package script, exclusion, or coverage setting
changed. GitHub Backend must still enforce the repository-wide 78 percent floor
and the existing artifact-foundation 90 percent floor.
Reviewer Results
Exact runtime SHA
e0838908dfa19227fab2f73e32c9b0c94dd39bb4passed seniorengineering, architecture, product/ops, docs, reuse/dedup, QA/test, test delta,
CI integrity, and security/auth/privacy review with no remaining findings.
External Review
Pending GitHub checks, CodeRabbit, and human review.
Remaining Risks
must use the documented non-owner runtime role.
current resource before disclosure.
Follow-Up Work
After merge, post-merge memory, stop, and a separate human start, AUTH-06 may
implement canonical actor profiles and identity links. Later chunks own the
authorization kernel, grants, resource cutovers, and invalidation consumers.
Human Review Focus
Review the migration triggers and downgrade lock ordering, canonical digest
boundary, request-to-evidence binding, mismatch privacy, and proof that no
route or product authorization behavior entered scope.
Human Merge Ownership
Only the human may approve and merge this PR. Internal or external checks do
not authorize merge.
Summary by CodeRabbit
New Features
Documentation
Tests