WS-AUTH-001-12F2: govern manual submission policy drafts - #292
Conversation
|
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:
📝 WalkthroughWalkthroughThe PR activates AUTH-12F2 manual submission-policy creation and append-only updates. It adds scoped Project Manager authorization, lineage and warning-custody checks, replay handling, atomic successor supersession, route integration, tests, E2E checks, and updated documentation. ChangesManual submission-policy mutation
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant ProjectManager
participant ProjectRouter
participant SubmissionPolicyMutationService
participant PreparedAuthorizationService
participant SubmissionPolicyMutationReplayRepository
ProjectManager->>ProjectRouter: Submit policy create or successor update
ProjectRouter->>SubmissionPolicyMutationService: Invoke authorized mutation
SubmissionPolicyMutationService->>PreparedAuthorizationService: Validate scoped PM grant
SubmissionPolicyMutationService->>SubmissionPolicyMutationReplayRepository: Reserve or classify replay
SubmissionPolicyMutationService-->>ProjectRouter: Return policy response or conflict
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 |
…2-manual-submission-policy
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
backend/app/modules/projects/submission_policy_mutation_service.py (2)
405-405: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNarrow the
policy_bodyannotation todict.
_manual_mutationdeclarespolicy_body: dict | None, but line 428 passes the value straight intocanonical_manual_submission_policy_body, which callspolicy_body.get(...). ANonevalue raisesAttributeErrorinstead of a domain error. Both callers already resolve a non-null body:create_manualusespayload.policy_body.model_dump(...), andupdate_manualfalls back topredecessor.policy_body. Tighten the annotation so the type checker enforces the real contract.♻️ Proposed annotation fix
- policy_body: dict | None, + policy_body: dict,Also applies to: 428-430
🤖 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/submission_policy_mutation_service.py` at line 405, Change the policy_body parameter annotation in _manual_mutation from dict | None to dict, preserving the existing callers create_manual and update_manual that provide non-null bodies and the canonical_manual_submission_policy_body call.
621-623: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winValidate the stored resource context directly instead of re-serializing it.
replay.resource_context_jsonis already adictloaded from the JSON column. Line 622 serializes it to a string only somodel_validate_jsoncan parse it back. Usemodel_validateon the dict and remove the round trip. This also removes the only use of thejsonimport in this module.Note on the static analysis hint for this line: the
use-jsonifyrule is a false positive here. This value is never written to an HTTP response, so no output-encoding boundary applies.♻️ Proposed fix
- resource = ProjectSubmissionArtifactPolicyMutationResourceContext.model_validate_json( - json.dumps(replay.resource_context_json) - ) + resource = ProjectSubmissionArtifactPolicyMutationResourceContext.model_validate( + replay.resource_context_json + )Then remove the now-unused
import jsonat line 5.🤖 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/submission_policy_mutation_service.py` around lines 621 - 623, Update the resource validation in the replay handling flow to call ProjectSubmissionArtifactPolicyMutationResourceContext.model_validate directly with replay.resource_context_json instead of serializing and parsing it. Remove the now-unused json import from the module.Source: Linters/SAST tools
backend/tests/test_authorization.py (1)
3835-3854: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe wrong-project test does not model a wrong-project grant.
_GuideMutationAuthorityFactsreceivesgrant=Noneat Line 3844. That is the same fixture used bytest_submission_artifact_policy_create_update_missing_pm_grant_denies_boundedat Line 3810 through Line 3814. The comment at Line 3841 states that the repository query filters the unrelated grant, but no unrelated grant is ever supplied. The test therefore asserts the missing-grant path again and cannot detect a regression that makes the scope filter accept a grant scoped to another project.Supply a grant whose
scope_project_idis a different project, so the scope filter is the control under test.Note:
backend/scripts/api_contract_e2e.pydoes exercise a real wrong-project Project Manager grant at Line 1073 through Line 1082, so the property is covered end to end. This comment addresses the unit-level guard only.♻️ Proposed fix to model a grant scoped to another project
project_id = uuid4() - # The repository's exact-scope query filters the unrelated grant and returns none. + # The repository's exact-scope query must filter this other-project grant. facts = _GuideMutationAuthorityFacts( context, - grant=None, + grant=SimpleNamespace( + id=uuid4(), + status="active", + scope_project_id=uuid4(), + ), permission_id=PermissionId.PROJECT_EFFECTIVE_POLICY_MANAGE, )Confirm that
_GuideMutationAuthorityFactsfilters by the requested scope before returning the grant. If it returns the grant unconditionally, assert the denial comes from the scope check rather than from an absent grant.🤖 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 3835 - 3854, Update test_submission_artifact_policy_create_update_wrong_project_grant_denies to provide a non-None grant through _GuideMutationAuthorityFacts, using a scope_project_id different from the requested project_id. Verify _GuideMutationAuthorityFacts applies the requested scope filter before returning grants; if it does not, fix that filtering so the test exercises denial of an unrelated-project grant rather than the missing-grant path.
🤖 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/modules/projects/submission_policy_mutation_service.py`:
- Around line 739-747: Add a private _operation_identity helper that accepts
action, resolved actor, project_id, predecessor_id, and key, builds the shared
stable parts using predecessor_id or the "create" sentinel, and returns both
operation and policy UUIDs. Replace the duplicated derivation logic in
create_manual, update_manual, _manual_mutation, and _existing_manual_replay with
this helper; pass selected_policy_id as predecessor_id only for artifact-policy
updates in _existing_manual_replay, otherwise preserving the current sentinel
behavior.
- Around line 823-825: Update update_manual to validate that predecessor belongs
to the current project/owner immediately after get_submission_artifact_policy
and before reading source_snapshot_id or calling _manual_mutation; treat an
owner/status mismatch the same as predecessor being None by raising
SubmissionArtifactPolicyNotFound, so absent and foreign policy IDs both return
404.
In `@backend/tests/test_api_controls.py`:
- Around line 551-556: Update the route inventory assertions in the test module
to include the new submission-artifact-policy POST and PATCH routes and their
corresponding permissions in route_inventory and protected_inventory.
Recalculate and update both inventory counts and SHA256 digests so the
assertions match the expanded route definitions.
In `@backend/tests/test_projects.py`:
- Around line 12259-12271: Before setting guide_row.status to "active" in the
guide setup, populate the guide’s selected review and revision policy fields by
calling the existing policy mutation endpoints or reusing a fixture that
performs this setup. Ensure the guide activation remains valid under
active_policy_selection_required while preserving the existing
submission-artifact policy patch assertions.
In `@docs/operations_project_operating_manual.md`:
- Around line 41-44: The submission artifact policy sentence in the checklist
must distinguish the current rollout from the later lifecycle: state that
derivation and approval are planned/later actions while 12F2 currently activates
only manual Project Manager create/update, or otherwise mark those actions as
planned. Preserve the existing governed update behavior that appends a successor
without editing agent output or an existing draft in place.
---
Nitpick comments:
In `@backend/app/modules/projects/submission_policy_mutation_service.py`:
- Line 405: Change the policy_body parameter annotation in _manual_mutation from
dict | None to dict, preserving the existing callers create_manual and
update_manual that provide non-null bodies and the
canonical_manual_submission_policy_body call.
- Around line 621-623: Update the resource validation in the replay handling
flow to call
ProjectSubmissionArtifactPolicyMutationResourceContext.model_validate directly
with replay.resource_context_json instead of serializing and parsing it. Remove
the now-unused json import from the module.
In `@backend/tests/test_authorization.py`:
- Around line 3835-3854: Update
test_submission_artifact_policy_create_update_wrong_project_grant_denies to
provide a non-None grant through _GuideMutationAuthorityFacts, using a
scope_project_id different from the requested project_id. Verify
_GuideMutationAuthorityFacts applies the requested scope filter before returning
grants; if it does not, fix that filtering so the test exercises denial of an
unrelated-project grant rather than the missing-grant path.
🪄 Autofix
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: 3a9e6ba0-b344-4d44-8d2c-75ea821fb5bf
📒 Files selected for processing (23)
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/CHUNK_MAP.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/DECISIONS.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-12F2-submission-policy-manual-drafts.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12F2-internal-review-evidence.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/reviews/WS-AUTH-001-12F2-pr-trust-bundle.mdbackend/app/modules/authorization/catalogue.pybackend/app/modules/authorization/prepared.pybackend/app/modules/authorization/runtime.pybackend/app/modules/projects/router.pybackend/app/modules/projects/schemas.pybackend/app/modules/projects/service.pybackend/app/modules/projects/submission_policy_mutation_repository.pybackend/app/modules/projects/submission_policy_mutation_service.pybackend/scripts/api_contract_e2e.pybackend/tests/test_alembic.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.
Actionable comments posted: 1
🤖 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/tests/verified_guide_fixtures.py`:
- Around line 283-305: Update the verified report fixture flow around
existing_usages to load the authoritative existing verified report for
source_snapshot_id and return its ID when present, rather than creating another
report. Only call create_verified_material_fixture and create a new verified
report when no authoritative report exists, while preserving the existing
usage-copy behavior.
🪄 Autofix
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: b1cdd82f-018c-44e9-aa59-99a5412a42fd
📒 Files selected for processing (4)
backend/app/modules/projects/submission_policy_mutation_service.pybackend/tests/test_projects.pybackend/tests/test_tasks.pybackend/tests/verified_guide_fixtures.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/app/modules/projects/submission_policy_mutation_service.py
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/app/modules/projects/submission_policy_mutation_service.py (1)
808-827: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftReturn the committed replay for overlapping update retries.
At Line 808,
update_manualperforms the pre-mutation replay lookup. Two identical update requests can both observe no replay. The first request supersedes the predecessor at Line 584 and commits the replay. The second request then reaches the locked lineage check, observes that the predecessor is no longer"draft"at Line 279, and raisesPolicyEditBlockedbefore Line 539 can classify the committed replay.A retry with the same idempotency key can therefore fail even though the operation already committed. After this locked predecessor conflict, re-run
_existing_manual_replaywith the same request and resource facts. Return the stored response only when the digests match. KeepPolicyEditBlockedfor a different operation, and add an overlapping-request test.🤖 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/submission_policy_mutation_service.py` around lines 808 - 827, Update update_manual’s locked predecessor-conflict path to re-run _existing_manual_replay with the same request and resource facts after detecting the predecessor is no longer draft. Return the committed replay only when its digests match the current request; otherwise preserve PolicyEditBlocked for a different operation, and add a test covering overlapping retries with the same idempotency key.
🤖 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/app/modules/projects/submission_policy_mutation_service.py`:
- Around line 808-827: Update update_manual’s locked predecessor-conflict path
to re-run _existing_manual_replay with the same request and resource facts after
detecting the predecessor is no longer draft. Return the committed replay only
when its digests match the current request; otherwise preserve PolicyEditBlocked
for a different operation, and add a test covering overlapping retries with the
same idempotency key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 537f798d-dc55-4374-a5de-6b9caf67ee33
📒 Files selected for processing (6)
backend/app/modules/authorization/runtime.pybackend/app/modules/projects/repository.pybackend/app/modules/projects/submission_policy_mutation_service.pybackend/scripts/api_contract_e2e.pybackend/tests/test_authorization.pybackend/tests/test_projects.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/app/modules/authorization/runtime.py
- backend/scripts/api_contract_e2e.py
2 participants
Workstream PR Trust Bundle
Chunk
WS-AUTH-001-12F2- Manual Submission Policy DraftsGoal
Activate only human Project Manager manual create/update for submission-policy
drafts, with exact sufficiency lineage, non-bypassable Workstream defaults,
append-only replacement, PREP evidence, and replay custody.
What changed
lookups and exact locked PREP before mutation.
predecessor supersession.
without depending on later guide/setup lineage.
Scope and behavior
project and its current authoritative sufficiency lineage.
review, revision, payment, and reputation behavior are unchanged.
Local evidence
Database-backed project tests and the roughly four-hour full suite are not run
on the user's slow local machine. GitHub Actions must run the PostgreSQL-backed
focused coverage, API E2E, repository-wide 78 percent floor, and changed-
subsystem 90 percent floor on the exact pushed head.
Acceptance proof
service, and role-claim-only callers deny.
allowed evidence together.
pass on the exact pushed head.
Human review focus
Human merge ownership
Summary by CodeRabbit
New Features
Documentation