Add shared append-only authority audit evidence#115
Conversation
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAUTH-05 is split into active 05A and inactive 05B. The change implements a typed, privacy-bounded, append-only authority evidence envelope in the existing audit ledger, adds migration and repository/service support, and expands migration, audit, compatibility, and custody tests. ChangesAuthorization evidence foundation
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Caller
participant AuthorityAuditEventInput
participant AuditService
participant AuditRepository
participant PostgreSQL
Caller->>AuthorityAuditEventInput: submit authority evidence
AuthorityAuditEventInput->>AuditService: validated normalized input
AuditService->>AuditRepository: create authority AuditEvent
AuditRepository->>PostgreSQL: insert and flush audit row
PostgreSQL-->>AuditRepository: enforce constraints and append-only triggers
AuditRepository-->>Caller: persisted AuditEvent
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
🧹 Nitpick comments (2)
backend/app/modules/audit/schemas.py (1)
102-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile positional enum slicing for shared reasons.
tuple(AuthorityEventType)[2:5]picksACTOR_IDENTITY_LINKED/REVOKED/REACTIVATEDby declaration position rather than by name. It currently matches the migration's explicitREASONSdict, but silently breaks (assigns the wrong reason set with no error) if a future PR inserts/reorders an enum member before index 5.♻️ Proposed fix: use explicit keys instead of positional slicing
- **dict.fromkeys(tuple(AuthorityEventType)[2:5], {"identity_lifecycle_change"}), + **dict.fromkeys( + ( + AuthorityEventType.ACTOR_IDENTITY_LINKED, + AuthorityEventType.ACTOR_IDENTITY_LINK_REVOKED, + AuthorityEventType.ACTOR_IDENTITY_LINK_REACTIVATED, + ), + {"identity_lifecycle_change"}, + ),🤖 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/audit/schemas.py` around lines 102 - 121, Replace the positional tuple slice in _REASONS with explicit AuthorityEventType keys for ACTOR_IDENTITY_LINKED, ACTOR_IDENTITY_REVOKED, and ACTOR_IDENTITY_REACTIVATED, assigning each the existing identity_lifecycle_change reason set. Preserve all other reason mappings unchanged.backend/tests/test_tasks.py (1)
80-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSuggest typing
sessionfor developer ergonomics.Consider adding an
AsyncSessiontype hint to thesessionparameter to improve IDE support and code clarity.♻️ Proposed refactor
-async def delete_audit_fixture_as_owner(session, event_id: str) -> None: +async def delete_audit_fixture_as_owner(session: AsyncSession, event_id: str) -> None:(Assuming
AsyncSessionis or will be imported in the module context)🤖 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 80 - 101, Update the delete_audit_fixture_as_owner helper to annotate its session parameter with the project’s AsyncSession type, adding or reusing the appropriate import if needed while preserving the existing 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
@.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-05B-idempotency-invalidation.md:
- Around line 149-165: The verification block contains prose and placeholders
instead of runnable commands. Replace the migration, AUTH-05B coverage, and
full-suite entries with concrete shell commands, running the exact migration
node first against the same isolated PostgreSQL database, then
audit/authorization tests, focused AUTH-05B coverage with its 90% threshold, and
the full suite with its 78% threshold; preserve all other CI integrity commands
unchanged.
In
@.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-05A-internal-review-evidence.md:
- Around line 3-5: Update the “Reviewer run IDs” entry in the AUTH-05A evidence
record to replace the stale /root/auth04b_final_* references with the actual
AUTH-05A reviewer run IDs for reviewed SHA
44901286b5c867a414cc39a9ccff5307bd23ad52, preserving the existing audit-record
format.
---
Nitpick comments:
In `@backend/app/modules/audit/schemas.py`:
- Around line 102-121: Replace the positional tuple slice in _REASONS with
explicit AuthorityEventType keys for ACTOR_IDENTITY_LINKED,
ACTOR_IDENTITY_REVOKED, and ACTOR_IDENTITY_REACTIVATED, assigning each the
existing identity_lifecycle_change reason set. Preserve all other reason
mappings unchanged.
In `@backend/tests/test_tasks.py`:
- Around line 80-101: Update the delete_audit_fixture_as_owner helper to
annotate its session parameter with the project’s AsyncSession type, adding or
reusing the appropriate import if needed while preserving the existing behavior.
🪄 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: 30676bdc-7525-4deb-8f9e-a87d42c64f92
📒 Files selected for processing (23)
.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/DISCOVERY.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/PLAN.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-05-authority-evidence-foundation.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-05A-shared-audit-authority-evidence.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-05A-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-05A-pr-trust-bundle.mdbackend/alembic/versions/0018_authority_audit_evidence.pybackend/app/modules/audit/repository.pybackend/app/modules/audit/schemas.pybackend/app/modules/audit/service.pybackend/app/modules/tasks/models.pybackend/app/modules/tasks/repository.pybackend/tests/test_alembic.pybackend/tests/test_audit.pybackend/tests/test_tasks.pydocs/architecture_data_model.mddocs/operations_authorization_service.md
2 participants
WS-AUTH-001-05A PR Trust Bundle
Chunk
WS-AUTH-001-05A- Shared Audit Ownership And Append-Only Authority EvidenceGoal
Add one typed, privacy-bounded, append-only authority evidence envelope to the
existing shared
audit_eventspath without changing legacy task/checkerbehavior or activating authorization decisions.
Human-Approved Intent
The human approved AUTH-05A as one semantic chunk bounded by its allowed files,
non-goals, acceptance criteria, behavior evidence, and required reviews rather
than an arbitrary production-line ceiling. AUTH-05B remains separate.
What Changed
0018extends the shared audit table with versioned authorityfields, closed registries, cross-field constraints, database-owned time,
authority-cause integrity, and append-only normal-DML triggers.
reason, fact, reference, permission, denial, role, and project matrices.
AuditRepositorywhilepreserving caller session and transaction ownership.
positive and negative paths, legacy compatibility, rollback, mutation
custody, non-retention, and migration downgrade guards.
Why It Changed
Later authorization chunks require trustworthy evidence custody before actor,
grant, permission evaluation, route cutover, idempotency, or invalidation
consumers can be implemented.
Design Chosen
The existing audit table remains the sole store. Application writes use one
typed service and shared repository. PostgreSQL independently enforces the
closed envelope so direct SQL cannot bypass privacy or integrity rules.
Rejected mappings and JSON are inspected from one stable snapshot, and service
re-admission never serializes untrusted mutated values before validation.
Alternatives Rejected
custody and duplicate writer behavior.
same boundary.
because either half alone is an incomplete security boundary.
Scope Control
No route, dependency, middleware, permission evaluator, actor/grant lifecycle,
authorization cutover, idempotency record/replay path, invalidation consumer,
token verifier, workflow, dependency, or CI threshold was added or changed.
Product Behavior
Legacy audit behavior and task/checker projections remain unchanged. New
authority evidence is internal foundation behavior only; it does not grant,
revoke, authorize, route, or invalidate product actions.
Acceptance Criteria Proof
registered tokens, denial codes, cause domain, self-cause, and append-only
custody have direct behavior proof.
representation, and post-validation mutation cases retain no rejected value
and emit no serializer warning.
Tests And Checks Run
stale wording/auth/artifact, Markdown links, and diff integrity passed.
CI owns the full suite and repository-wide 78 percent coverage floor.
Test Delta
Tests were added or strengthened only. No assertion, raises guard, skip, xfail,
coverage threshold, or exclusion was removed or weakened.
CI Integrity
No workflow, package, dependency, coverage configuration, or test selection
file changed. GitHub checks must pass before merge.
Reviewer Results
Exact SHA
44901286b5c867a414cc39a9ccff5307bd23ad52passed senior engineering,architecture, reuse/dedup, docs, product/ops, QA/test, CI integrity, test delta,
and security/auth review with no remaining findings.
External Review
GitHub Actions and CodeRabbit are pending PR publication.
Remaining Risks
credentials; production must use the documented non-owner runtime role.
perform authorization.
Follow-Up Work
After merge, post-merge memory, and a separate human start, AUTH-05B may add the
idempotency and invalidation foundation. Later chunks own actor profiles,
grants, permission evaluation, and route cutover.
Human Review Focus
Review the migration constraints and downgrade ordering, typed/SQL matrix
parity, single-snapshot privacy boundary, shared writer transaction ownership,
and proof that no authorization behavior was activated.
Human Merge Ownership
Only the human may approve and merge this PR. Completion of internal or
external review does not authorize merge.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation