feat(auth): hard-cut project diagnostic reads - #216
Conversation
|
Warning Review limit reached
Next review available in: 42 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 (1)
📝 WalkthroughWalkthroughSix project diagnostic GET routes now use centralized typed authorization, concealed denials, snapshot-bound locking, persisted context digests, updated action activation, route metadata, tests, coverage enforcement, and documentation. ChangesProject diagnostic read authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Router
participant ProjectRepository
participant AuthorizationService
Client->>Router: request diagnostic GET
Router->>ProjectRepository: lock snapshot-bound project data
ProjectRepository-->>Router: return canonical diagnostic facts
Router->>AuthorizationService: authorize diagnostic resource
AuthorizationService-->>Router: allow or concealed denial
Router-->>Client: diagnostic response
Possibly related PRs
🚥 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: 3
🧹 Nitpick comments (6)
backend/app/modules/projects/authorization_reads.py (2)
166-167: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
RuntimeErrorhere surfaces as an unconcealed 500 if the invariant ever breaks.The guard is correct today (missing targets always deny), but if any future path allows with
target is None, the caller gets a 500 rather than the concealed 404 this PR is built around. Consider raising the same concealed not-found error instead of a bareRuntimeError, keeping the invariant assertion in logs.🤖 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/projects/authorization_reads.py` around lines 166 - 167, Update the target-missing guard in the authorization read flow to raise the same concealed not-found error used for denied or missing targets instead of a bare RuntimeError, while preserving the invariant violation in logs for diagnostics.
50-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the setup-run value instead of reusing the wide
targetunion.
targetis typedDiagnosticResult | None(record, sequence, or tuple), but Line 63 and Lines 68-77 accessoutput_post_submit_checker_policy_idand per-record fields on it. That only holds because this branch always assigns aProjectSetupRun; a type checker cannot see it and a future branch reordering would break silently.♻️ Suggested narrowing
- target = await repository.lock_latest_project_setup_run( + setup_run = await repository.lock_latest_project_setup_run( project_id, guide_id, guide.version ) + target = setup_run if ( action_id is ActionId.PROJECT_POST_SUBMIT_CHECKER_POLICY_SETUP_READ - and target is not None - and target.output_post_submit_checker_policy_id is not None + and setup_run is not None + and setup_run.output_post_submit_checker_policy_id is not None ):🤖 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/projects/authorization_reads.py` around lines 50 - 79, Introduce a separately typed setup-run variable for the project setup actions in the guide branch, assigning the result of lock_latest_project_setup_run to it and using it for output_post_submit_checker_policy_id and the field comparisons. Keep target for the broader DiagnosticResult result, and assign the narrowed setup-run value to target only where the surrounding flow requires it.backend/app/modules/authorization/runtime.py (2)
258-273: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueValidator allows
target_exists=Truewith no snapshot binding.For single-target kinds the composer always sets
source_snapshot_id/hashwhen a record exists, but the model itself does not require it, so a future caller could assert an existing single target with no snapshot facts and still pass. If collections are the only intended exception, encode that (e.g. require snapshot facts unlesstarget_kindends in_collection).🤖 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/runtime.py` around lines 258 - 273, Strengthen require_canonical_shape so an existing non-collection target requires both source_snapshot_id and source_snapshot_hash; allow missing snapshot facts only when target_kind ends in "_collection". Preserve the existing paired-binding validation and reject target_exists=True without snapshot facts for single-target kinds.
254-256: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueConsider constraining
source_snapshot_hashshape liketarget_binding_digest.
target_binding_digestis pattern-validated but the co-bound snapshot hash is a free-formstr | None, so a malformed hash still produces a canonical-looking context and digest.♻️ Proposed tightening
- source_snapshot_hash: str | None = None + source_snapshot_hash: str | None = Field(default=None, pattern=r"^sha256:[0-9a-f]{64}$")🤖 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/runtime.py` around lines 254 - 256, Update the source_snapshot_hash field in the relevant model to validate the same sha256-prefixed 64-hex-character format as target_binding_digest, while retaining its optional None behavior.backend/app/modules/authorization/kernel.py (1)
112-121: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSerialized reads put
FOR UPDATElocks on every diagnostic GET.Adding these six reads to
_SERIALIZED_ADMIN_READSmakes each request lock the caller's actor/link rows and the matched grant (for_update=serialized), on top of the row locks taken by the new repository lock helpers. That is defensible for anti-stale-authority guarantees, but it serializes concurrent GETs from the same actor and can create contention with grant mutations. Worth confirming that the read latency and lock-wait profile is acceptable under the hosted E2E load before rollout.🤖 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/kernel.py` around lines 112 - 121, Validate the lock-wait and read-latency impact of the diagnostic actions added to _SERIALIZED_ADMIN_READS under hosted E2E concurrency; if contention is unacceptable, remove those diagnostic read ActionId entries so GET requests do not use for_update=serialized, while preserving serialization for reads that require anti-stale-authority protection.backend/tests/test_projects.py (1)
332-375: 📐 Maintainability & Code Quality | 🔵 Trivial
add_project_manager_admin_grantsilently grants an unrelated "submitter" project role.Line 344 unconditionally calls
add_project_role_for_default_actor(project_id, "submitter")as a side effect purely to reach theaccess_administratorbootstrap grantor inside that helper — but this also creates a realsubmitterProjectRoleGrantfor the default actor that the docstring doesn't mention. Any future test that assertsproject_roles/admin_rolescontents after callingcreate_guide(which now always calls this function, line 1115) or this helper directly will unexpectedly see an extrasubmittergrant. Consider extracting the bootstrap-grantor logic into its own explicit helper (e.g.,ensure_access_administrator_bootstrap()) instead of piggybacking on a role-grant helper for its side effect.♻️ Suggested direction
-async def add_project_manager_admin_grant(project_id: str) -> UUID: - """Grant the default registered human exact project diagnostic authority.""" - async with db_session.get_session_factory()() as session: - existing = await session.scalar(...) - if existing is not None: - return existing.id - await add_project_role_for_default_actor(project_id, "submitter") +async def add_project_manager_admin_grant(project_id: str) -> UUID: + """Grant the default registered human exact project diagnostic authority. + + Note: bootstraps the access_administrator grantor if it does not exist yet. + """ + async with db_session.get_session_factory()() as session: + existing = await session.scalar(...) + if existing is not None: + return existing.id + await _ensure_access_administrator_bootstrap() async with db_session.get_session_factory()() as session: ...🤖 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_projects.py` around lines 332 - 375, Remove the unrelated submitter grant side effect from add_project_manager_admin_grant. Extract or reuse an explicit ensure_access_administrator_bootstrap helper that establishes and returns the required access_administrator grantor, then use it before creating the project_manager AdminRoleGrant while preserving existing idempotency.
🤖 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-11C1-project-setup-diagnostic-reads.md:
- Around line 117-121: Update the admission-order sentence to replace the
grammatically incomplete “verifier outage its canonical 503” wording with a
complete phrase such as “a verifier outage preserves its canonical 503,” without
changing the stated behavior or other clauses.
In `@backend/app/modules/projects/repository.py`:
- Around line 409-428: Bound the rows locked by lock_guide_sufficiency_reports
and lock_submission_artifact_policies instead of applying with_for_update to
every matching record. Add the same established cap used by
list_superseded_post_submit_checker_policies, or paginate the diagnostic list
while preserving the existing ordering and locking behavior.
- Line 378: Wrap the overlong .join(...) calls in the repository query chains,
including the joins near GuideSufficiencyReport.source_snapshot_id and the
corresponding occurrences around lines 415 and 491, using the nearby multi-line
join style shown around 551-554; preserve the existing join arguments and query
behavior.
---
Nitpick comments:
In `@backend/app/modules/authorization/kernel.py`:
- Around line 112-121: Validate the lock-wait and read-latency impact of the
diagnostic actions added to _SERIALIZED_ADMIN_READS under hosted E2E
concurrency; if contention is unacceptable, remove those diagnostic read
ActionId entries so GET requests do not use for_update=serialized, while
preserving serialization for reads that require anti-stale-authority protection.
In `@backend/app/modules/authorization/runtime.py`:
- Around line 258-273: Strengthen require_canonical_shape so an existing
non-collection target requires both source_snapshot_id and source_snapshot_hash;
allow missing snapshot facts only when target_kind ends in "_collection".
Preserve the existing paired-binding validation and reject target_exists=True
without snapshot facts for single-target kinds.
- Around line 254-256: Update the source_snapshot_hash field in the relevant
model to validate the same sha256-prefixed 64-hex-character format as
target_binding_digest, while retaining its optional None behavior.
In `@backend/app/modules/projects/authorization_reads.py`:
- Around line 166-167: Update the target-missing guard in the authorization read
flow to raise the same concealed not-found error used for denied or missing
targets instead of a bare RuntimeError, while preserving the invariant violation
in logs for diagnostics.
- Around line 50-79: Introduce a separately typed setup-run variable for the
project setup actions in the guide branch, assigning the result of
lock_latest_project_setup_run to it and using it for
output_post_submit_checker_policy_id and the field comparisons. Keep target for
the broader DiagnosticResult result, and assign the narrowed setup-run value to
target only where the surrounding flow requires it.
In `@backend/tests/test_projects.py`:
- Around line 332-375: Remove the unrelated submitter grant side effect from
add_project_manager_admin_grant. Extract or reuse an explicit
ensure_access_administrator_bootstrap helper that establishes and returns the
required access_administrator grantor, then use it before creating the
project_manager AdminRoleGrant while preserving existing idempotency.
🪄 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: 13b61ab1-2658-491f-a79a-1aa0493db099
📒 Files selected for processing (20)
.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-11C1-project-setup-diagnostic-reads.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-11C1-pr-trust-bundle.md.github/workflows/backend.ymlbackend/app/api/deps/authorization.pybackend/app/modules/authorization/catalogue.pybackend/app/modules/authorization/kernel.pybackend/app/modules/authorization/runtime.pybackend/app/modules/projects/authorization_reads.pybackend/app/modules/projects/repository.pybackend/app/modules/projects/router.pybackend/app/modules/projects/service.pybackend/tests/test_api_controls.pybackend/tests/test_audit.pybackend/tests/test_authorization.pybackend/tests/test_projects.pydocs/operations_authorization_service.mddocs/operations_project_operating_manual.mddocs/spec_authorization_service.md
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/scripts/api_contract_e2e.py (1)
652-670: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the stale
create_policy_bundle_for_guidecallers.The helper is redefined locally in
backend/tests/test_tasks.pyand missing inbackend/tests/test_checkers.py, so the calls there are not passing the updated signature frombackend/scripts/api_contract_e2e.pyand will fail before the E2E/test flow reaches the 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/scripts/api_contract_e2e.py` around lines 652 - 670, The callers of create_policy_bundle_for_guide in backend/tests/test_tasks.py and backend/tests/test_checkers.py must match the helper’s updated signature from backend/scripts/api_contract_e2e.py. Update the local test_tasks definition and its callers, and add or provide the corresponding helper in test_checkers, including the required tokens, manager_subject, project_id, guide_id, run_id, and optional checker/severity arguments so the flows reach the API.
🤖 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.
Outside diff comments:
In `@backend/scripts/api_contract_e2e.py`:
- Around line 652-670: The callers of create_policy_bundle_for_guide in
backend/tests/test_tasks.py and backend/tests/test_checkers.py must match the
helper’s updated signature from backend/scripts/api_contract_e2e.py. Update the
local test_tasks definition and its callers, and add or provide the
corresponding helper in test_checkers, including the required tokens,
manager_subject, project_id, guide_id, run_id, and optional checker/severity
arguments so the flows reach the API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 304c3667-f638-4631-b5e8-0518fa30025c
📒 Files selected for processing (11)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-11C1-project-setup-diagnostic-reads.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-11C1-external-review-response.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-11C1-pr-trust-bundle.mdbackend/app/modules/authorization/runtime.pybackend/app/modules/projects/authorization_reads.pybackend/app/modules/projects/repository.pybackend/scripts/api_contract_e2e.pybackend/tests/test_authorization.pybackend/tests/test_projects.pydocs/operations_project_operating_manual.mddocs/spec_authorization_service.md
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/operations_project_operating_manual.md
- backend/app/modules/authorization/runtime.py
- docs/spec_authorization_service.md
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-11C1-pr-trust-bundle.md
- backend/tests/test_authorization.py
- backend/app/modules/projects/repository.py
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-11C1-project-setup-diagnostic-reads.md
- backend/tests/test_projects.py
…1-setup-diagnostic-reads
|
Final exact-head evidence for 8e2d249:
The PR remains blocked only on required human review/approval. No merge was performed. |
2 participants
PR Trust Bundle: WS-AUTH-001-11C1
Chunk
WS-AUTH-001-11C1- Project Setup Diagnostic Read Cutover (L1/P1).Goal And Human-Approved Intent
Hard-cut exactly six project-guide diagnostic GET routes from issuer role
claims to scoped local administrative grants, without a migration, mutation
change, compatibility path, or 11C2 activation. The user started this chunk on
2026-07-28 after AUTH-11B merged.
What Changed And Why
actions as planned.
source-snapshot, and binding-digest authorization facts.
policy, current actor/identity link, and matched grant through projection and
commit.
ActorContextand performed legacy role checks.
decision-context digest evidence, docs, tests, and an additive hosted 90%
branch-coverage gate for the new composer.
Design Chosen
ProjectRepositoryremains the only project persistence owner. A narrowapplication composer loads and locks feature facts, AUTH evaluates one strict
resource context, and authorization-neutral response projection follows in the
same transaction. Historical diagnostic rows remain readable only when their
complete guide-version and source-snapshot binding is canonical.
Rejected alternatives: retaining token-role fallback, duplicating six route
flows, adding an AUTH project repository, using an unbound project-only
context, or weakening the broad legacy project coverage boundary.
Scope And Product Behavior
Allowed: covered Project Manager, scoped Audit Authority, and system Operator.
Denied with concealed 404: Finance Authority, Access Administrator,
contributors, wrong project scope, revoked grant/link, inactive actor,
non-human caller, missing/cross-project/cross-guide/stale child, and target-kind
mismatch. Authentication and rate controls retain canonical 401/503 and
429/503 responses before private lookup. Read permission grants no mutation
authority.
Acceptance Evidence And Test Delta
post-submit policy binding.
concealment, Project Manager/Operator/Audit allow, Finance/Access
Administrator/contributor/wrong-scope/revoked denial.
project lookup.
the non-11C1 mutation routes.
Checks Run
app,tests, andscripts: pass.git diff --cached --check: pass./openapi.jsonafter both health probes passed. Hosted API E2E and the fullsuite remain mandatory on the exact PR head.
CI Integrity
No gate was weakened. Hosted Backend retains semantic lanes, API E2E, global
78%, authorization 90%, and all existing subsystem gates. The new composer
uses a separate coverage data file and a branch-aware 90% focused gate, so it
does not erase or replace combined full-suite coverage.
Reviewer Results
Preimplementation architecture, security, product/ops, QA, CI-integrity, and
senior-engineering reviews passed after contract repair. Exact implementation
architecture, security, product/ops, senior-engineering, CI-integrity, docs,
reuse/dedup, and test-delta reviews pass. QA passes with the condition that
hosted Backend/API E2E evidence be recorded before completion.
The CodeRabbit correction received focused architecture and security passes;
QA and test-delta passed with only the low residual risk that the 100-row cap is
proved at compiled-SQL rather than a 101-row live route fixture. Exact SQL shape,
ordering, lock target, and cap are asserted, and hosted PostgreSQL lanes remain
mandatory.
External Review And Remaining Risk
The first hosted Backend run found two stale explicit active-action test
expectations; both were corrected without weakening exact equality. CodeRabbit's
valid findings were addressed with canonical snapshot validation, bounded
newest-first collection locks, setup-run type narrowing, an explicit fixture
bootstrap helper, repository formatting, and corrected contract wording. The
invariant-failure and authority-serialization suggestions were rejected because
they would hide a kernel defect or weaken the approved concurrent-revocation
boundary. Full rationale is recorded in the 11C1 external-review response.
Final-head Backend, Agent Gates, and CodeRabbit evidence remains mandatory.
Follow-Up And Human Review Focus
AUTH-11C2 remains separate and unstarted. Review the canonical snapshot joins,
post-submit policy binding, concealed response boundary, role matrix, and the
additive coverage step. The human owns merge approval; the agent must not merge
this PR without explicit approval for that PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests