feat(auth): activate guide binding and read - #245
Conversation
|
Warning Review limit reached
Next review available in: 52 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 (18)
📝 WalkthroughWalkthroughGuide-source read and binding operations now use fixed-service authorization with typed resource contexts, transaction-bound single-use handles, expanded lineage locking, updated catalogue ownership, and comprehensive tests and documentation. ChangesGuide-source authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant GuideMaterialization
participant FixedServiceAuthorization
participant Database
participant Provider
GuideMaterialization->>Database: Lock guide-source lineage facts
GuideMaterialization->>FixedServiceAuthorization: Prepare and consume read capability
FixedServiceAuthorization-->>GuideMaterialization: Return single-use authorization
GuideMaterialization->>Provider: Inspect locked source replica
GuideMaterialization->>Database: Persist classification in the transaction
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
backend/tests/test_guide_bindings.py (1)
1688-1694: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
authorityargument.
_materialization_requestignores itsauthorityparameter; it only builds field values. This test supplies the real authority throughauthority_factory, so the_AllowReadAuthority()instance created here is discarded. Removing the parameter from the helper and its call sites makes the authority source unambiguous.🤖 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_guide_bindings.py` around lines 1688 - 1694, Remove the unused authority parameter from _materialization_request and update every call site, including the materialize_guide_source test, to stop passing _AllowReadAuthority(). Preserve the existing authority_factory-based authority setup.backend/tests/test_authorization.py (1)
5455-5476: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the inert
human_authorityparameter.
del human_authorityat Line 5476 discards the parameter. Both parameter sets then run identical logic, and the valuesproject_managerandaccess_administratornever influence the context or the repository stub. A reader can conclude that two distinct human roles are covered, which is not the case.Either remove the third parametrize value, or configure
_runtime_contextwith the named role so each case exercises a distinct human authority.♻️ Proposed cleanup
`@pytest.mark.parametrize`( - ("action_id", "resource_type", "human_authority"), + ("action_id", "resource_type"), [ - ( - ActionId.ARTIFACT_GUIDE_SOURCE_BINDING_CREATE, - "guide_source_binding", - "project_manager", - ), - ( - ActionId.ARTIFACT_GUIDE_SOURCE_READ, - "guide_source_read", - "access_administrator", - ), + (ActionId.ARTIFACT_GUIDE_SOURCE_BINDING_CREATE, "guide_source_binding"), + (ActionId.ARTIFACT_GUIDE_SOURCE_READ, "guide_source_read"), ], ) `@pytest.mark.asyncio` async def test_human_admin_authority_cannot_substitute_for_fixed_guide_services( action_id: ActionId, resource_type: str, - human_authority: str, ) -> None: - del human_authority context = _runtime_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_authorization.py` around lines 5455 - 5476, Remove the inert human_authority parameter from test_human_admin_authority_cannot_substitute_for_fixed_guide_services and its `@pytest.mark.parametrize` entries, unless the test is updated to pass each role into _runtime_context. Ensure the parametrized cases only represent values that affect the test logic.backend/app/modules/authorization/prepared.py (1)
357-367: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider gating the new branch on
action_idand renaming the local variable.Two consistency points, neither of which changes current behavior:
- Every other branch in
_scope_from_resourcepairs a resource type with its owningaction_id. This branch maps bytype(resource)only. A caller that pairs a guide-source context with an unrelatedaction_idstill gets a valid scope here; the denial happens later in_require_prelocked. Gating onaction_idkeeps the fail-closed decision local.artifact_resource_typeholds astrhere and is reassigned to a class at Line 417. Use a distinct name for the new local.Also consider hoisting the mapping to a module-level constant, since it is rebuilt on every call.
♻️ Proposed refactor
- artifact_internal_types = { - GuideSourceBindingResourceContext: "guide_source_binding", - GuideSourceReadResourceContext: "guide_source_read", - } - artifact_resource_type = artifact_internal_types.get(type(resource)) - if artifact_resource_type is not None: + guide_resource_type = _GUIDE_INTERNAL_RESOURCE_TYPE_BY_ACTION.get(action_id) + if guide_resource_type is not None and isinstance( + resource, (GuideSourceBindingResourceContext, GuideSourceReadResourceContext) + ) and resource.resource_type == guide_resource_type: return PreparedAuthorityScope( kind=PreparedAuthorityScopeKind.ARTIFACT_INTERNAL, - artifact_resource_type=artifact_resource_type, + artifact_resource_type=guide_resource_type, artifact_resource_id=resource.resource_id, )Module-level constant:
_GUIDE_INTERNAL_RESOURCE_TYPE_BY_ACTION = { ActionId.ARTIFACT_GUIDE_SOURCE_BINDING_CREATE: "guide_source_binding", ActionId.ARTIFACT_GUIDE_SOURCE_READ: "guide_source_read", }🤖 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/prepared.py` around lines 357 - 367, Update _scope_from_resource to gate the guide-source context branch by the matching action_id, using the proposed module-level _GUIDE_INTERNAL_RESOURCE_TYPE_BY_ACTION mapping instead of rebuilding a type-only mapping per call. Rename the local string result to avoid colliding with the later artifact_resource_type class assignment, while preserving the existing PreparedAuthorityScope construction.backend/app/modules/artifacts/guide_materialization.py (1)
181-214: 🩺 Stability & Availability | 🔵 TrivialConsider a bounded
lock_timeout/statement_timeoutfor this transaction.The transaction now holds
FOR UPDATElocks on the full guide lineage across provider I/O inself._preparation.prepare(self._store.open(...)). That is intentional and covered bytest_authorized_read_locks_lineage_through_provider_access. The lock hold time is therefore bounded by the preparation deadline, not by database work, and concurrentproject_setup_runswriters block for that whole period.Set an explicit
SET LOCAL lock_timeoutandstatement_timeouton this session so a slow provider cannot pin lineage rows for the full deadline, and add a metric for the lock hold duration.🤖 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/artifacts/guide_materialization.py` around lines 181 - 214, Within the transaction in the guide materialization flow, configure session-local lock_timeout and statement_timeout before loading and locking the guide lineage, using bounded values compatible with the preparation deadline. Measure the duration that the transaction holds the lineage locks through self._preparation.prepare and record a metric for that lock-hold duration, while preserving the existing provider-access locking behavior.backend/app/modules/artifacts/authorization.py (1)
526-547: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert that the loaded profile matches the requested service identity.
_fixed_service_contextselects the actor withget_service_actor(service_identity.value)but then builds the context fromServiceIdentity(profile.service_identity). The requested identity and the stored identity are never compared. Every downstream identity check (SERVICE_ACTIONS_BY_IDENTITY[context.service_identity]in_prepare_prelocked, and_locked_service_context) then validates the stored value against itself. If the repository lookup ever widens or the row is edited, an action reserved for one fixed service can be prepared under another identity. Add the explicit equality check to close the gap locally.🔒️ Proposed check
link = await actors.get_identity_link_for_actor(profile.id) if ( link is None or link.actor_profile_id != profile.id or link.subject_kind != ActorKind.SERVICE.value + or profile.service_identity != service_identity.value ): raise ArtifactAuthorityDeniedError("artifact service principal is unavailable") try: return ServiceAuthorizationContext( actor_profile_id=UUID(profile.id), actor_kind=ActorKind.SERVICE, actor_status=ActorStatus(profile.status), identity_link_id=UUID(link.id), identity_link_status=IdentityLinkStatus(link.status), - service_identity=ServiceIdentity(profile.service_identity), + service_identity=service_identity, request_id=request_id, correlation_id=correlation_id, )🤖 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/artifacts/authorization.py` around lines 526 - 547, Update _fixed_service_context to explicitly compare profile.service_identity with the requested service_identity.value after loading the profile and before constructing ServiceAuthorizationContext; raise ArtifactAuthorityDeniedError with the existing unavailable message when they differ, while preserving the current link validation and context construction for matching identities.
🤖 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/ACTIVATION_CUSTODY.md:
- Around line 41-46: Update the custody table’s availability annotations to
match its header: mark WS-AUTH-001-ART-02D-INTERNAL as Active and mark every
remaining unannotated action-chunk row as Planned. Keep the existing ActionIds
unchanged, and ensure each row explicitly states its availability so the table
is self-contained.
In
@.agent-loop/initiatives/WS-XINT-002-art-auth-end-to-end/reviews/WS-XINT-002-04B-internal-review.md:
- Around line 40-47: Update the “Verification evidence” section to record the
exact reviewed commit SHA, full commands, and test selectors for every listed
check. After the required rebase, rerun the verification against the final PR
head, refresh the evidence with those results, and confirm hosted checks passed
for that same commit.
- Around line 3-5: Update the Result section to state that local review passed
provisionally, while merge readiness remains pending hosted exact-head checks.
Incorporate the required hosted full-coverage and database-backed guide tests
into the status, and ensure the related sections at the referenced review areas
consistently describe these gates as outstanding rather than implying approval.
In `@backend/app/modules/artifacts/guide_materialization.py`:
- Around line 371-384: Remove ArtifactStorageNamespace from the
with_for_update(of=...) exclusive lock list in the guide materialization query.
If provider I/O still requires protecting the namespace row from deletion,
acquire it separately with a shared lock while keeping the existing per-lineage
exclusive locks unchanged.
- Around line 261-269: Update the _GuideReadIncident handler to call
_record_incident on a best-effort basis, catching and suppressing any
incident-write exception. Always re-raise GuideSourceMaterializationError for
the original incident, preserving its bounded error contract regardless of
database failures during recording.
In `@backend/app/modules/authorization/runtime.py`:
- Around line 1228-1240: Update GuideSourceBindingAuthorityFacts to include the
required logical_role field matching the binding resource context, and update
GuideSourceReadAuthorityFacts to use binding_id instead of resource_id. Ensure
both models’ field names and required values align with
_guide_source_resource_context and its **asdict(facts) construction so binding
writes and guide reads validate successfully.
In `@backend/tests/test_guide_bindings.py`:
- Around line 1858-1885: The lock-contention test currently treats all
DBAPIError failures as blocked and inserts an invalid snapshot hash. In the
test’s exception handler, assert that the PostgreSQL error SQLSTATE is 55P03
before setting blocked, and update the ProjectSetupRun construction to use the
seeded GuideSourceSnapshot.bundle_hash instead of recomputing the hash.
---
Nitpick comments:
In `@backend/app/modules/artifacts/authorization.py`:
- Around line 526-547: Update _fixed_service_context to explicitly compare
profile.service_identity with the requested service_identity.value after loading
the profile and before constructing ServiceAuthorizationContext; raise
ArtifactAuthorityDeniedError with the existing unavailable message when they
differ, while preserving the current link validation and context construction
for matching identities.
In `@backend/app/modules/artifacts/guide_materialization.py`:
- Around line 181-214: Within the transaction in the guide materialization flow,
configure session-local lock_timeout and statement_timeout before loading and
locking the guide lineage, using bounded values compatible with the preparation
deadline. Measure the duration that the transaction holds the lineage locks
through self._preparation.prepare and record a metric for that lock-hold
duration, while preserving the existing provider-access locking behavior.
In `@backend/app/modules/authorization/prepared.py`:
- Around line 357-367: Update _scope_from_resource to gate the guide-source
context branch by the matching action_id, using the proposed module-level
_GUIDE_INTERNAL_RESOURCE_TYPE_BY_ACTION mapping instead of rebuilding a
type-only mapping per call. Rename the local string result to avoid colliding
with the later artifact_resource_type class assignment, while preserving the
existing PreparedAuthorityScope construction.
In `@backend/tests/test_authorization.py`:
- Around line 5455-5476: Remove the inert human_authority parameter from
test_human_admin_authority_cannot_substitute_for_fixed_guide_services and its
`@pytest.mark.parametrize` entries, unless the test is updated to pass each role
into _runtime_context. Ensure the parametrized cases only represent values that
affect the test logic.
In `@backend/tests/test_guide_bindings.py`:
- Around line 1688-1694: Remove the unused authority parameter from
_materialization_request and update every call site, including the
materialize_guide_source test, to stop passing _AllowReadAuthority(). Preserve
the existing authority_factory-based authority setup.
🪄 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: 87ebd5e0-7eb3-41bc-abaa-2d0ab4e942ff
📒 Files selected for processing (16)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/ACTIVATION_CUSTODY.md.agent-loop/initiatives/WS-XINT-002-art-auth-end-to-end/reviews/WS-XINT-002-04B-internal-review.md.agent-loop/initiatives/WS-XINT-002-art-auth-end-to-end/reviews/WS-XINT-002-04B-pr-trust-bundle.mdbackend/app/interfaces/artifact_operations.pybackend/app/modules/artifacts/authorization.pybackend/app/modules/artifacts/guide_materialization.pybackend/app/modules/authorization/catalogue.pybackend/app/modules/authorization/kernel.pybackend/app/modules/authorization/prepared.pybackend/app/modules/authorization/runtime.pybackend/tests/test_artifact_architecture.pybackend/tests/test_authorization.pybackend/tests/test_guide_bindings.pydocs/operations_authorization_service.mddocs/spec_artifact_storage_service.mddocs/spec_authorization_service.md
229af17 to
71a1b17
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
CodeRabbit review disposition on current head
Local Ruff, focused AUTH/audit tests, architecture tests, stale-doc scans, link checks, and diff checks pass. Agent Gates passes on the exact head; hosted Backend full coverage is still running. A fresh CodeRabbit invocation was requested but was rate-limited, so this disposition is tied to the existing review threads and the checked diff. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Final exact-head readiness evidence for
The PR is ready for human merge. No merge was performed. |
2 participants
PR Trust Bundle: WS-XINT-002-04B
Chunk
WS-XINT-002-04B— guide binding and guide read authorization activation.Goal and human-approved intent
Activate exactly
artifact.guide_source.binding.createforworkstream.artifact.bindingandartifact.guide_source.readforworkstream.artifact.guide_reader, preserving exact transaction, identity,lineage, verified-content, and no-provider-I/O-on-denial guarantees.
What changed and why
WS-XINT-002-04Bcustody.PreparedAuthorizationHandleprotocol.fresh authority in its owned session and holds exact lineage locks through the
protected provider read and atomic classification write.
Design chosen
Reuse centralized PREP with two closed contexts and fixed service identities.
Binding retains the caller-owned transaction. Reading prepares and consumes
inside the materializer-owned transaction because handles cannot cross sessions.
The protected read holds canonical lineage locks through provider access.
Alternatives rejected
leaves a stale-lineage race.
Scope control and product behavior
No new action/permission identifiers, migration, route, Celery payload,
submission/checker/review authority, generic download, parser behavior, or
ART-03C legacy cutover. Project Managers retain ingest only; neither human nor
Admin authority implies binding/read service authority.
Acceptance proof and test delta
copied/wrong handle, replay, wrong service, human substitution, every adapter
fact mismatch, cross-resource selectors, stale generation, wrong content, and
wrong logical role are covered.
evidence where applicable.
idempotency key, never a prepared handle.
expectation was replaced by the stronger lock-through-provider invariant.
Tests/checks run
ruff check app tests scripts: passed.pytest tests/test_artifact_architecture.py -q: 20 passed.tests/test_authorization.pyguide/custody/service cases: passed.exact PR head.
CI integrity
No workflow, dependency, package script, test config, skip/xfail, coverage
threshold, or fail-open changes.
Reviewer results
Security, architecture, QA, senior engineering, product/ops, CI integrity,
docs, reuse/dedup, and test-delta tracks pass after all blocking findings were
resolved. Details are in
WS-XINT-002-04B-internal-review.md.External review
Planning PR #244 has Agent Gates passing; hosted Backend and external review run
on its amended exact head. Runtime external review begins after this commit is
pushed and the stacked PR is opened.
Remaining risks and follow-up work
operationally heavier; ART-03C worker tuning must preserve deadlines.
prepared/kernel mapping registries.
Human review focus and merge ownership
Review exact fixed identities, full fact manifests, lock-through-read ordering,
atomic decision evidence, no human inheritance, and the absence of ART-03C scope.
Planning PR #244 must merge first. Human approval owns every merge.
Summary by CodeRabbit
New Features
Documentation
Tests