feat(auth): authorize guide draft source mutations - #232
Conversation
…-guide-draft-source
…-guide-draft-source
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThe PR activates authorized project-guide creation, guide updates, and source-snapshot creation. It adds PREP bindings, idempotency custody, provenance metadata, migration safeguards, post-commit setup dispatch, API routes, tests, CI coverage, and documentation. ChangesGuide mutation authority
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant GuideMutationRouter
participant GuideMutationService
participant GuideMutationRepository
participant SetupQueue
Client->>GuideMutationRouter: Send mutation and Idempotency-Key
GuideMutationRouter->>GuideMutationService: Resolve authorization and execute
GuideMutationService->>GuideMutationRepository: Reserve or replay mutation
GuideMutationRepository-->>GuideMutationService: Return reservation state
GuideMutationService->>GuideMutationRepository: Commit response and custody
GuideMutationRouter->>SetupQueue: Dispatch setup after commit
GuideMutationRouter-->>Client: Return mutation response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/test_projects.py (1)
2360-2364: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the docstring to name the source-snapshot boundary.
The test now posts to the source-snapshot route, but the docstring still describes a "durable guide create". Guide creation no longer dispatches setup work, so the wording contradicts the test.
📝 Proposed fix
- """A late broker failure cannot turn a durable guide create into a false 503.""" + """A late broker failure cannot turn a durable source snapshot create into a false 503."""🤖 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 2360 - 2364, Update the docstring of test_create_source_snapshot_marks_setup_run_when_post_commit_enqueue_fails to describe the source-snapshot creation route and its post-commit broker failure behavior, replacing the outdated “durable guide create” wording without changing the test logic.
🧹 Nitpick comments (2)
backend/tests/test_authorization.py (1)
3579-3646: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for an exact-project Project Manager grant.
The stub grant has no
scope_project_id, so line 3646 only proves the system-scope path wherematched_scope_project_idisNone. The exact-project path is untested. That path is what makeskernel.py:554-558capture the grant scope andkernel.py:974propagate it, and it is what driveslast_mutation_scope_type="project"pluslast_mutation_scope_project_idinguide_mutation_service.py. The chunk contract proof matrix requires success for both system and exact-project Project Manager on all three actions.Add a parameter for a grant carrying
scope_project_id=project_idand assertdecision.matched_scope_project_id == project_id.🧪 Proposed parameterization sketch
- facts = _GuideMutationAuthorityFacts( - context, grant=SimpleNamespace(id=grant_id, status="active") - ) + facts = _GuideMutationAuthorityFacts( + context, + grant=SimpleNamespace( + id=grant_id, + status="active", + scope_project_id=None if grant_scope == "system" else project_id, + ), + )- assert decision.matched_scope_project_id is None + assert decision.matched_scope_project_id == ( + None if grant_scope == "system" else project_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/tests/test_authorization.py` around lines 3579 - 3646, Add parameterization to the authorization test around _GuideMutationAuthorityFacts and the prepared.consume assertions so it covers both a system-scope grant and an exact-project grant with scope_project_id=project_id. Preserve the existing successful decision checks, and assert matched_scope_project_id is None for the system case and project_id for the exact-project case across all three actions.backend/app/modules/projects/service.py (1)
1801-1832: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the now-unreachable ProjectService helpers.
These three methods only exist in
backend/app/modules/projects/service.pyand have no other references. Since the guide mutation flow creates snapshots and setup runs throughguide_mutation_service.py/setup_queue.dispatch_pre_submit_setup_pipeline_after_commitinstead, remove the helper chain to avoid maintaining dead snapshot/setup-run creation paths.Applies to lines 1801-1886.
🤖 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/service.py` around lines 1801 - 1832, Remove the unreachable helper chain beginning with ProjectService._create_guide_source_snapshot_model, including the related snapshot and setup-run creation helpers through the indicated section. Keep the active guide mutation flow in guide_mutation_service.py and setup_queue.dispatch_pre_submit_setup_pipeline_after_commit unchanged.
🤖 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/app/api/deps/authorization.py`:
- Line 227: Update the logger.exception call in get_authorization_service to
identify the non-prepared authorization transaction failure, while leaving the
prepared_authorization_service message unchanged so the two lifecycle paths
produce distinct log records.
In `@backend/app/modules/projects/guide_mutation_service.py`:
- Around line 94-98: Update the digest payload construction in the idempotency
flow to serialize the validated body with exclude_unset=True instead of
exclude_none=True. Preserve explicit null values while omitting fields that were
not provided, keeping the digest aligned with the changes applied by
ProjectGuideUpdate.
In `@backend/tests/project_create_fixtures.py`:
- Around line 62-75: Add await session.rollback() as the first statement in the
finally blocks restoring triggers in backend/tests/project_create_fixtures.py
lines 62-75 and backend/scripts/api_contract_e2e.py lines 83-93. Ensure both
helpers roll back before executing the enable trigger statements, preserving the
original exception and allowing trigger restoration to succeed.
- Around line 40-41: Update the fixture docstring to state that it suspends both
guide_mutation_product_custody and guide_lineage_lifecycle_guard, replacing the
inaccurate wording that mentions only the new custody trigger.
---
Outside diff comments:
In `@backend/tests/test_projects.py`:
- Around line 2360-2364: Update the docstring of
test_create_source_snapshot_marks_setup_run_when_post_commit_enqueue_fails to
describe the source-snapshot creation route and its post-commit broker failure
behavior, replacing the outdated “durable guide create” wording without changing
the test logic.
---
Nitpick comments:
In `@backend/app/modules/projects/service.py`:
- Around line 1801-1832: Remove the unreachable helper chain beginning with
ProjectService._create_guide_source_snapshot_model, including the related
snapshot and setup-run creation helpers through the indicated section. Keep the
active guide mutation flow in guide_mutation_service.py and
setup_queue.dispatch_pre_submit_setup_pipeline_after_commit unchanged.
In `@backend/tests/test_authorization.py`:
- Around line 3579-3646: Add parameterization to the authorization test around
_GuideMutationAuthorityFacts and the prepared.consume assertions so it covers
both a system-scope grant and an exact-project grant with
scope_project_id=project_id. Preserve the existing successful decision checks,
and assert matched_scope_project_id is None for the system case and project_id
for the exact-project case across all three actions.
🪄 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: bd7a9947-4fdf-4525-b46a-1895dd54668f
📒 Files selected for processing (33)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12D-guide-draft-source-mutations.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12D-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12D-pr-trust-bundle.md.github/workflows/backend.ymlREADME.mdbackend/alembic/versions/0045_guide_source_metadata_authority.pybackend/app/api/deps/authorization.pybackend/app/api/router.pybackend/app/modules/authorization/catalogue.pybackend/app/modules/authorization/kernel.pybackend/app/modules/authorization/prepared.pybackend/app/modules/authorization/runtime.pybackend/app/modules/projects/guide_mutation_repository.pybackend/app/modules/projects/guide_mutation_router.pybackend/app/modules/projects/guide_mutation_service.pybackend/app/modules/projects/models.pybackend/app/modules/projects/router.pybackend/app/modules/projects/schemas.pybackend/app/modules/projects/service.pybackend/app/modules/projects/setup_queue.pybackend/scripts/api_contract_e2e.pybackend/scripts/week2_api_e2e.pybackend/tests/conftest.pybackend/tests/project_create_fixtures.pybackend/tests/test_alembic.pybackend/tests/test_api_controls.pybackend/tests/test_authorization.pybackend/tests/test_checkers.pybackend/tests/test_projects.pybackend/tests/test_tasks.pydocs/operations_authorization_service.mddocs/spec_authorization_service.mddocs/spec_chunk_3_project_guide_foundation.md
💤 Files with no reviewable changes (1)
- backend/app/modules/projects/router.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
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/tests/project_create_fixtures.py (1)
96-138: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUse the same isolation guard as
suspend_historical_product_custodybefore disabling custody triggers.Line 98 checks only
str(database_name).startswith("workstream_test_").suspend_historical_product_custody, defined earlier in this same file, requiresdatabase_nameto fullmatchworkstream_test_([a-f0-9]{12})anddatabase_roleto fullmatchworkstream_role_([a-f0-9]{12})with a matching suffix. The equivalent function in backend/scripts/api_contract_e2e.py also uses a full regex match. A database name that merely starts with "workstream_test_" without matching the strict pattern passes this weaker check, letting the fixture disableguide_mutation_product_custodyandguide_lineage_lifecycle_guardoutside a genuinely isolated test database.This function also reimplements the disable/enable/rollback sequence that
suspend_historical_product_custodyalready provides, instead of reusing it.Use the stricter check, and consider reusing
suspend_historical_product_custodyto remove the duplicated trigger-management logic.🔒️ Proposed fix for the isolation check
async with session_factory() as session: database_name = await session.scalar(text("select current_database()")) - if not str(database_name).startswith("workstream_test_"): + if _ISOLATED_DATABASE_RE.fullmatch(str(database_name)) is None: raise RuntimeError("downstream activation fixture requires an isolated test database")🤖 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/project_create_fixtures.py` around lines 96 - 138, Update the downstream activation fixture to use the same strict database and role isolation validation as suspend_historical_product_custody, requiring matching workstream_test_ and workstream_role_ names with the expected hexadecimal suffix. Reuse suspend_historical_product_custody for trigger disabling, restoration, and transaction cleanup instead of duplicating that sequence around ProjectService.activate_guide.
🧹 Nitpick comments (1)
backend/app/modules/projects/service.py (1)
133-149: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTighten the secret-name alternatives to avoid over-blocking legitimate filenames.
secret[^/]*,token[^/]*, andcredential[^/]*only require a path component to start with the keyword. There is no boundary after the keyword. Filenames such as "secretary.txt", "tokenizer.py", or "credentialing.py" match this pattern and get rejected as secret artifacts, even though they contain no secret material.Add a boundary after each keyword so the match requires a separator, extension, or end of component.
♻️ Proposed refinement (validate against real fixtures before merging)
- r"secret[^/]*|" - r"credential[^/]*|" - r"token[^/]*|" + r"secrets?(?=$|[_.\-])[^/]*|" + r"credentials?(?=$|[_.\-])[^/]*|" + r"tokens?(?=$|[_.\-])[^/]*|"🤖 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/service.py` around lines 133 - 149, Update the SECRET_ARTIFACT_NAME_PATTERN alternatives for secret, token, and credential so each keyword is followed by a component boundary, separator, extension, or end of component rather than matching arbitrary suffixes. Preserve detection of genuine names such as secret-key, token.json, and credential files while allowing names like secretary.txt, tokenizer.py, and credentialing.py.
🤖 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/tests/project_create_fixtures.py`:
- Around line 96-138: Update the downstream activation fixture to use the same
strict database and role isolation validation as
suspend_historical_product_custody, requiring matching workstream_test_ and
workstream_role_ names with the expected hexadecimal suffix. Reuse
suspend_historical_product_custody for trigger disabling, restoration, and
transaction cleanup instead of duplicating that sequence around
ProjectService.activate_guide.
---
Nitpick comments:
In `@backend/app/modules/projects/service.py`:
- Around line 133-149: Update the SECRET_ARTIFACT_NAME_PATTERN alternatives for
secret, token, and credential so each keyword is followed by a component
boundary, separator, extension, or end of component rather than matching
arbitrary suffixes. Preserve detection of genuine names such as secret-key,
token.json, and credential files while allowing names like secretary.txt,
tokenizer.py, and credentialing.py.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 65b4aebc-53b4-4dc9-aa7a-9a5b171a62a7
📒 Files selected for processing (18)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12D-guide-draft-source-mutations.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12D-external-review-response.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12D-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12D-pr-trust-bundle.mdbackend/app/api/deps/authorization.pybackend/app/modules/projects/guide_mutation_service.pybackend/app/modules/projects/service.pybackend/scripts/api_contract_e2e.pybackend/scripts/week2_api_e2e.pybackend/tests/project_create_fixtures.pybackend/tests/test_artifact_admission.pybackend/tests/test_artifact_recovery.pybackend/tests/test_audit.pybackend/tests/test_auth.pybackend/tests/test_authorization.pybackend/tests/test_guide_bindings.pybackend/tests/test_projects.pybackend/tests/test_tasks.py
🚧 Files skipped from review as they are similar to previous changes (7)
- backend/app/api/deps/authorization.py
- backend/tests/test_authorization.py
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/chunks/WS-AUTH-001-12D-guide-draft-source-mutations.md
- backend/scripts/api_contract_e2e.py
- backend/tests/test_projects.py
- backend/tests/test_tasks.py
- backend/app/modules/projects/guide_mutation_service.py
There was a problem hiding this comment.
🧹 Nitpick comments (3)
backend/tests/test_projects.py (3)
3097-3107: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Replay.completedis a mutable class attribute.
completed: list[tuple] = []is bound to the class, so everyReplayinstance shares the same list. The current test creates one instance, so the assertionlen(replay.completed) == 3holds. If a future test reuses this helper, counts will accumulate across tests.Initialize the list per instance.
♻️ Suggested fix
class Replay: - completed: list[tuple] = [] + def __init__(self) -> None: + self.completed: list[tuple] = [] async def find(self, *_args): return NoneAlso applies to: 3164-3169
🤖 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 3097 - 3107, Update the Replay helper’s completed storage in both occurrences so each instance initializes its own empty list, rather than using the shared class-level mutable list; preserve complete’s existing append behavior and assertions.
2794-2852: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo parametrize cases share identical inputs and differ only through the fake.
Case 3
(True, True, "pending", "pending")and case 5(True, True, "pending", "claimed")pass the same inputs. The fakeSession.scalarbranches onexpectedto produce the difference. This couples the expectation to the stub instead of to a distinct database response, so the intent of each case is not obvious.Add an explicit input that models the database response, for example an
insert_winsflag, and branch the fake on that flag.♻️ Suggested clarification
`@pytest.mark.parametrize`( - ("identity_matches", "digest_matches", "status", "expected"), + ("identity_matches", "digest_matches", "status", "insert_wins", "expected"), [ - (False, True, "pending", "mismatch"), - (True, False, "pending", "mismatch"), - (True, True, "pending", "pending"), - (True, True, "committed", "replayed"), - (True, True, "pending", "claimed"), + (False, True, "pending", False, "mismatch"), + (True, False, "pending", False, "mismatch"), + (True, True, "pending", False, "pending"), + (True, True, "committed", False, "replayed"), + (True, True, "pending", True, "claimed"), ], ) async def test_guide_mutation_repository_classifies_existing_reservations( identity_matches: bool, digest_matches: bool, status: str, + insert_wins: bool, expected: str, ) -> None:Then use
insert_winsin place ofexpected == "claimed"insideSession.scalarandSession.get.🤖 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 2794 - 2852, Update test_guide_mutation_repository_classifies_existing_reservations to add an explicit insert_wins parameter to the parametrized inputs, set it distinctly for the pending and claimed cases, and use it in Session.scalar and Session.get instead of branching on expected. Keep expected solely as the asserted classification result.
3172-3332: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThis test asserts many unrelated invariants in one function.
test_guide_mutation_service_fails_closed_for_replay_and_authority_edgescovers replay mismatch, replay pending, replay commit, cached short-circuit for three operations,_reservation_outcomeclassification,_proveauthority failure, and_preparedenial composition. A failure in an early block hides the later blocks.Split it into focused tests per invariant. This keeps the fail-closed coverage and improves failure attribution.
🤖 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 3172 - 3332, Split test_guide_mutation_service_fails_closed_for_replay_and_authority_edges into focused tests, separating replay mismatch/pending/committed behavior, cached short-circuiting for create_guide/update_guide/create_snapshot, _reservation_outcome classification, _prove authority failure, and _prepare denial composition. Preserve each existing assertion and setup relevant to its invariant so failures identify the specific behavior that regressed.
🤖 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.
Nitpick comments:
In `@backend/tests/test_projects.py`:
- Around line 3097-3107: Update the Replay helper’s completed storage in both
occurrences so each instance initializes its own empty list, rather than using
the shared class-level mutable list; preserve complete’s existing append
behavior and assertions.
- Around line 2794-2852: Update
test_guide_mutation_repository_classifies_existing_reservations to add an
explicit insert_wins parameter to the parametrized inputs, set it distinctly for
the pending and claimed cases, and use it in Session.scalar and Session.get
instead of branching on expected. Keep expected solely as the asserted
classification result.
- Around line 3172-3332: Split
test_guide_mutation_service_fails_closed_for_replay_and_authority_edges into
focused tests, separating replay mismatch/pending/committed behavior, cached
short-circuiting for create_guide/update_guide/create_snapshot,
_reservation_outcome classification, _prove authority failure, and _prepare
denial composition. Preserve each existing assertion and setup relevant to its
invariant so failures identify the specific behavior that regressed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b6382386-7416-4a0e-885c-6986699a6204
📒 Files selected for processing (5)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12D-external-review-response.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12D-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12D-pr-trust-bundle.mdbackend/tests/test_authorization.pybackend/tests/test_projects.py
🚧 Files skipped from review as they are similar to previous changes (2)
- .agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12D-pr-trust-bundle.md
- backend/tests/test_authorization.py
…-guide-draft-source
…-guide-draft-source # Conflicts: # backend/tests/test_guide_bindings.py
2 participants
WS-AUTH-001-12D PR Trust Bundle
Intent
Activate exactly
project.guide.create,project.guide.update, andproject.guide_source_snapshot.createfor a system-scoped or exact-projectProject Manager. Keep ART byte operations, policy mutation, and guide activation
outside this chunk.
Design and scope
opaque, transaction-bound PREP protocol.
action, project, guide, target resource, operation, transaction, generation,
and current source lineage.
guide identity/lifecycle plus the complete snapshot manifest/item set.
historical active state only inside derived isolated databases.
Proof
retired guide request shape. The corrected isolated PostgreSQL regression lane
passed 9 tests across downstream setup, replay, and grant-scope paths.
validation regression. The repair restores secret-reference rejection, uses
canonical actor IDs in isolated activation seeds, and proves immutable
snapshots reject legacy rewrites. Its exact local selection completed 36 tests
without a failure before local execution was stopped for machine cost.
test-delta tracks: passed; low risks are documented in the review evidence.
90 percent gates remain required on the final pushed SHA.
Remaining risk and human review focus
shortened Alembic revision id documented beside the exact migration filename.
External review response
exclude_unset=Trueand a replay-mismatch regression.actions.
clean-cut guide contract and seed independent lifecycle policy prerequisites.
isolated activation seed to the exact admitted Flow subject and issuer.
WS-AUTH-001-12D-external-review-response.md.Chunk complete locally. Await corrected-head hosted CI, CodeRabbit, and human merge.
Summary by CodeRabbit
New Features
Documentation
Changes