feat(auth): activate contributor bundle preparation - #327
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. 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 (3)
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 (7)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe change activates ChangesSubmission preparation authorization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ArtifactRoute
participant PreparationAuthorization
participant AuthorizationKernel
participant PostgreSQL
participant SubmissionAdmission
ArtifactRoute->>PreparationAuthorization: construct with session and authorization context
PreparationAuthorization->>AuthorizationKernel: preflight and revalidate submission authority
AuthorizationKernel->>PostgreSQL: lock actor and active submitter role
PreparationAuthorization->>AuthorizationKernel: prepare final authorization from durable-intent facts
SubmissionAdmission->>PreparationAuthorization: consume single-use authorization
SubmissionAdmission-->>ArtifactRoute: continue durable admission
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 (6)
backend/tests/test_submission_bundle_admission.py (3)
128-140: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider closing the composed authority in this test.
The factory constructs
AuthorizationServiceandPreparedAuthorizationService, and the latter registers a prepared consumer on the former. The test asserts the wiring but never callsauthority.close(). Adding the call would also cover the cleanup path used by the productionfinallyblock.🤖 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_submission_bundle_admission.py` around lines 128 - 140, Update test_artifact_adapter_composes_active_preparation_authority to call authority.close() after asserting the composed authority wiring, covering cleanup of the internally registered prepared consumer and matching the production cleanup path.
123-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCall
prepare_finalwith the contract keyword.The port declares
prepare_final(self, *, facts: SubmissionBundleDurableIntentAuthorityFacts). This test passesrequest=request.DenySubmissionBundlePreparationAuthorization.prepare_finalaccepts**values, so the call succeeds, but the test no longer exercises the real keyword. Usefacts=so the test keeps tracking the port signature.💚 Proposed keyword alignment
with pytest.raises(ArtifactAuthorityDeniedError): - await authority.prepare_final(request=request) + await authority.prepare_final(facts=object())🤖 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_submission_bundle_admission.py` around lines 123 - 124, Update the prepare_final invocation in the test to pass the request object using the facts keyword, matching the SubmissionBundleDurableIntentAuthorityFacts port signature while preserving the existing exception assertion.
933-941: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the new non-
Nonehandle rejection.
ArtifactAdmissionService.admitnow denies aSubmissionBundleArtifactAdmissionRequestwhenprepared_authorization is not None(backend/app/modules/artifacts/service.pyLine 1888). This test asserts the positive case only. A negative case would lock the new guard, and the guideline requires materially changed backend subsystems to stay at or above 90 percent coverage.Do you want me to draft that 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/tests/test_submission_bundle_admission.py` around lines 933 - 941, Add a negative admission test alongside the existing admit callback test, covering a SubmissionBundleArtifactAdmissionRequest with a non-None prepared_authorization and asserting ArtifactAdmissionService.admit rejects it. Keep the existing positive-case assertions unchanged and use the established test fixtures and rejection mechanism.Source: Coding guidelines
backend/tests/test_authorization.py (1)
3826-3858: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrepared authorization capabilities are created in tests without the cleanup call. Both tests construct a prepared authorization capability and end without invoking
close(), so the outstanding issuances and the registered prepared consumer are never released and the cleanup path stays uncovered.
backend/tests/test_authorization.py#L3826-L3858: callprepared.close()after the replay rejection assertion, matching the pattern at Lines 5420 and 13531.backend/tests/test_submission_bundle_admission.py#L128-L140: callauthority.close()after the wiring assertions on the composedPreparedSubmissionBundlePreparationAuthorization.🤖 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 3826 - 3858, Call prepared.close() in backend/tests/test_authorization.py at lines 3826-3858 after the replay rejection assertion. Also call authority.close() in backend/tests/test_submission_bundle_admission.py at lines 128-140 after the wiring assertions for the composed PreparedSubmissionBundlePreparationAuthorization.backend/app/modules/artifacts/authorization.py (1)
90-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type annotations to the adapter parameters.
requestandfactshave no annotations in_request_input,_final_input,_final_values,preflight,revalidate,prepare_final, andconsume. The port atbackend/app/modules/artifacts/submission_authorization.pydeclaresSubmissionBundlePreparationRequestandSubmissionBundleDurableIntentAuthorityFacts. Annotating the implementation keeps the adapter checkable against the port.Also applies to: 108-108, 127-127, 156-156, 190-190, 222-222
🤖 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 90 - 91, Annotate the request and facts parameters in the adapter methods _request_input, _final_input, _final_values, preflight, revalidate, prepare_final, and consume with the corresponding SubmissionBundlePreparationRequest and SubmissionBundleDurableIntentAuthorityFacts types declared by the submission_authorization port, preserving existing behavior while keeping the implementation type-checkable against that interface.backend/app/modules/artifacts/service.py (1)
1915-1917: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a separate local for the minted handle.
prepared_authorizationis a parameter annotatedPreparedAuthorizationHandle | None, and it is reassigned here fromprepare_final, whose port declares a return type ofobject. A dedicated local, for examplesubmission_handle, keeps the parameter type honest and makes the guard at Line 1888 easier to 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/artifacts/service.py` around lines 1915 - 1917, In the surrounding submission flow, keep the `prepared_authorization` parameter unchanged and assign the result of `submission_prepared_authorization.prepare_final` to a separate local such as `submission_handle`. Use that local for subsequent minted-handle processing while preserving the existing guard and behavior.
🔇 Additional comments (26)
.agent-loop/CURRENT_STATE.md (1)
31-31: LGTM!.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/CHUNK_MAP.md (1)
14-15: LGTM!.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/STATUS.md (1)
30-34: LGTM!.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/chunks/WS-ARCH-001-02G-auth-preparation-activation.md (2)
43-80: LGTM!Also applies to: 84-114, 144-149
118-125: 📐 Maintainability & Code QualityExtend the verification scope to the changed ART packages.
Line 119 does not include
backend/app/modules/artifacts/service.py. Line 120 measures coverage only forapp.modules.authorization, although this chunk covers ART authorization, admission, and service code.Confirm that hosted CI enforces the 90 percent changed-subsystem gate. Otherwise, extend the commands to lint and measure the ART packages.
Proposed verification update
-(cd backend && .venv/bin/python -m ruff check app/modules/authorization app/adapters/auth app/adapters/artifacts app/modules/artifacts/submission_authorization.py app/modules/artifacts/submission_admission.py app/modules/artifacts/authorization.py tests/test_authorization.py tests/test_submission_bundle_admission.py) +(cd backend && .venv/bin/python -m ruff check app/modules/authorization app/adapters/auth app/adapters/artifacts app/modules/artifacts/service.py app/modules/artifacts/submission_authorization.py app/modules/artifacts/submission_admission.py app/modules/artifacts/authorization.py tests/test_authorization.py tests/test_submission_bundle_admission.py) -(cd backend && export WORKSTREAM_TEST_DATABASE_URL="${WORKSTREAM_TEST_DATABASE_URL:?set WORKSTREAM_TEST_DATABASE_URL}" && .venv/bin/python -m pytest -q tests/test_authorization.py tests/test_submission_bundle_admission.py tests/test_default_pre_submit_execution.py tests/architecture/test_authorization_boundary.py --cov=app.modules.authorization --cov-fail-under=90) +(cd backend && export WORKSTREAM_TEST_DATABASE_URL="${WORKSTREAM_TEST_DATABASE_URL:?set WORKSTREAM_TEST_DATABASE_URL}" && .venv/bin/python -m pytest -q tests/test_authorization.py tests/test_submission_bundle_admission.py tests/test_default_pre_submit_execution.py tests/architecture/test_authorization_boundary.py --cov=app.modules.authorization --cov=app.modules.artifacts --cov=app.adapters.artifacts --cov-fail-under=90)As per coding guidelines, new or materially changed backend subsystems must remain at or above 90 percent test coverage, and CI must preserve the 78 percent repository-wide baseline.
Source: Coding guidelines
.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/ACTIVATION_CUSTODY.md (1)
47-47: LGTM!Also applies to: 63-72
.agent-loop/initiatives/WS-XINT-002-art-auth-end-to-end/STATUS.md (1)
34-36: LGTM!docs/architecture_data_model.md (1)
840-840: LGTM!docs/spec_authorization_service.md (1)
258-260: LGTM!Also applies to: 484-484, 504-506
.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/evidence/WS-ARCH-001-02G-preparation-activation.md (1)
13-15: 📐 Maintainability & Code QualityReconcile the focused-test count.
State the exact command and suite that produced the final count. The evidence reports 100 focused tests, while the PR objectives report 101 focused admission/AUTH boundary tests.
backend/app/modules/authorization/runtime.py (1)
1477-1537: LGTM!Also applies to: 1582-1583, 1675-1676
backend/app/modules/artifacts/submission_authorization.py (1)
7-8: LGTM!Also applies to: 36-43
backend/app/modules/authorization/prepared.py (1)
71-73: LGTM!Also applies to: 171-174, 310-332, 389-410, 473-478, 582-585, 611-660, 899-902, 995-1005
backend/app/modules/authorization/catalogue.py (1)
707-711: LGTM!Also applies to: 878-878
backend/app/modules/authorization/kernel.py (1)
92-92: LGTM!Also applies to: 577-597, 1126-1148
backend/app/modules/authorization/repository.py (1)
139-154: LGTM!backend/app/adapters/artifacts/__init__.py (1)
228-240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Sandbox verification was unavailable.Tighten the dependency annotations.
contextis annotated asobject, and the function has no return annotation. The providerget_artifact_authorization_contextreturnsAuthorizationContext, and the consumer at Line 250 expectsSubmissionBundlePreparationAuthorization. Precise annotations keep the port contract checkable.♻️ Proposed annotation tightening
def get_submission_bundle_preparation_authorization( session: Annotated[AsyncSession, Depends(get_db_session)], context: Annotated[ - object, + AuthorizationContext, Depends(get_artifact_authorization_context), ], -): +) -> SubmissionBundlePreparationAuthorization:Confirm that
AuthorizationContextis importable at module scope without creating an import cycle; the adapter imports the concrete class lazily inside the function body for that reason.backend/app/modules/artifacts/authorization.py (2)
115-125: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the field-set coupling between the facts dataclass and the strict resource model.
consumesplats_final_values(facts, json=False)intoSubmissionBundlePreparationResourceContext, which uses_STRICT_FROZEN. The construction succeeds only while the field set ofSubmissionBundleDurableIntentAuthorityFactsmatches the model field set exactly, afterproject_idis renamed toscope_project_id. Any future field added to either side raises a validation error at consumption time, inside the root transaction.The error is caught and converted to a denial, so a mismatch surfaces as a silent authorization failure rather than a clear contract break. Add a construction-time assertion or a dedicated test that asserts the two field sets stay aligned.
Also applies to: 222-243
68-88: LGTM!Also applies to: 185-188, 245-246
backend/app/modules/artifacts/service.py (1)
1886-1892: LGTM!backend/app/modules/artifacts/submission_admission.py (2)
484-487: LGTM!
101-101: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
⚠️ Unverified finding
Sandbox verification was unavailable.Consider removing the vestigial
prepared_authorizationfield.The sole production caller now passes
None, andArtifactAdmissionService.admitdenies any non-Nonevalue for aSubmissionBundleArtifactAdmissionRequestatbackend/app/modules/artifacts/service.pyLine 1888. The field onSubmissionBundleDurablePutRequestis therefore carried only to be rejected. Removing it, and the forwarding at Lines 167, would shrink the surface and remove a misleading option from the request contract.Confirm first that no other caller supplies a handle here.
Also applies to: 553-553
backend/tests/test_authorization.py (1)
1748-1752: LGTM!Also applies to: 2154-2156, 2237-2250
backend/tests/test_default_pre_submit_execution.py (2)
765-780: 🩺 Stability & Availability | ⚡ Quick win
⚠️ Unverified finding
Sandbox verification was unavailable.Verify the transaction state required by
revalidateat this point.
PreparedSubmissionBundlePreparationAuthorization.revalidatecallsPreparedAuthorizationService.preflight, which calls_root_transaction()and thenAuthorizationService._prepare_prelocked._prepare_prelockedraisesTypeError("prelocked authority requires one active root transaction")whensync_session.get_transaction()returnsNoneor an inactive transaction.Line 777 awaits
revalidatewith no enclosingasync with session.begin(). The precedingworkflow.executecalls require a transaction-free session, so the session may hold no active transaction here.revalidatecatches onlyPreparedAuthorizationHandleInvalidandPreparedAuthorizationUnsupported, so aTypeErrorwould escape as a raw failure rather than a denial.If the call depends on SQLAlchemy autobegin from an earlier statement, wrap it in an explicit
async with session.begin():block to make the requirement visible. Also consider callingfinal_authority.close()after the durable assertions, matching the productionfinallyblock inPreparedSubmissionBundlePreparationCommand.prepare.
652-689: LGTM!Also applies to: 798-798, 825-825, 857-857, 883-883
backend/tests/test_submission_bundle_admission.py (1)
536-546: LGTM!Also applies to: 576-579, 588-600, 617-622, 955-958
🤖 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/artifacts/submission_authorization.py`:
- Around line 45-49: Update the test call to prepare_final to pass a
SubmissionBundleDurableIntentAuthorityFacts value through the facts keyword
instead of request. Keep the existing deny implementation unchanged, since it
already accepts arbitrary keyword values.
In `@docs/roadmap_status.md`:
- Around line 64-66: Update the WS-ARCH-001-02G entries in the roadmap status
document so they are not presented as implemented on main while PR `#327` remains
unmerged. Move the capability to “Integration In Progress” or explicitly qualify
it as activating after merge, and apply the same merge-status wording to the
authority statement near the active-capability section; keep “Implemented On
main” limited to merged behavior.
---
Nitpick comments:
In `@backend/app/modules/artifacts/authorization.py`:
- Around line 90-91: Annotate the request and facts parameters in the adapter
methods _request_input, _final_input, _final_values, preflight, revalidate,
prepare_final, and consume with the corresponding
SubmissionBundlePreparationRequest and
SubmissionBundleDurableIntentAuthorityFacts types declared by the
submission_authorization port, preserving existing behavior while keeping the
implementation type-checkable against that interface.
In `@backend/app/modules/artifacts/service.py`:
- Around line 1915-1917: In the surrounding submission flow, keep the
`prepared_authorization` parameter unchanged and assign the result of
`submission_prepared_authorization.prepare_final` to a separate local such as
`submission_handle`. Use that local for subsequent minted-handle processing
while preserving the existing guard and behavior.
In `@backend/tests/test_authorization.py`:
- Around line 3826-3858: Call prepared.close() in
backend/tests/test_authorization.py at lines 3826-3858 after the replay
rejection assertion. Also call authority.close() in
backend/tests/test_submission_bundle_admission.py at lines 128-140 after the
wiring assertions for the composed
PreparedSubmissionBundlePreparationAuthorization.
In `@backend/tests/test_submission_bundle_admission.py`:
- Around line 128-140: Update
test_artifact_adapter_composes_active_preparation_authority to call
authority.close() after asserting the composed authority wiring, covering
cleanup of the internally registered prepared consumer and matching the
production cleanup path.
- Around line 123-124: Update the prepare_final invocation in the test to pass
the request object using the facts keyword, matching the
SubmissionBundleDurableIntentAuthorityFacts port signature while preserving the
existing exception assertion.
- Around line 933-941: Add a negative admission test alongside the existing
admit callback test, covering a SubmissionBundleArtifactAdmissionRequest with a
non-None prepared_authorization and asserting ArtifactAdmissionService.admit
rejects it. Keep the existing positive-case assertions unchanged and use the
established test fixtures and rejection mechanism.
🪄 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: b24d7de4-21ac-4fa9-aed5-f0e2de12c40d
📒 Files selected for processing (23)
.agent-loop/CURRENT_STATE.md.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/CHUNK_MAP.md.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/STATUS.md.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/chunks/WS-ARCH-001-02G-auth-preparation-activation.md.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/evidence/WS-ARCH-001-02G-preparation-activation.md.agent-loop/initiatives/WS-AUTH-001-workstream-authorization-service/ACTIVATION_CUSTODY.md.agent-loop/initiatives/WS-XINT-002-art-auth-end-to-end/STATUS.mdbackend/app/adapters/artifacts/__init__.pybackend/app/modules/artifacts/authorization.pybackend/app/modules/artifacts/service.pybackend/app/modules/artifacts/submission_admission.pybackend/app/modules/artifacts/submission_authorization.pybackend/app/modules/authorization/catalogue.pybackend/app/modules/authorization/kernel.pybackend/app/modules/authorization/prepared.pybackend/app/modules/authorization/repository.pybackend/app/modules/authorization/runtime.pybackend/tests/test_authorization.pybackend/tests/test_default_pre_submit_execution.pybackend/tests/test_submission_bundle_admission.pydocs/architecture_data_model.mddocs/roadmap_status.mddocs/spec_authorization_service.md
| Contributor bundle preparation now uses the exact assigned-contributor AUTH | ||
| PREP boundary from WS-ARCH-001-02G; Submission consumption and binding remain | ||
| separately gated by 02H. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not mark 02G as implemented on main before merge.
PR #327 is still open. Lines 64-66 therefore place unmerged behavior under “Implemented On main”. Line 109 also says that the authority is active without a merge qualifier.
Move the capability to “Integration In Progress” or state that PR #327 activates it after merge. Keep the implemented-on-main list limited to merged behavior.
Proposed wording
- Contributor bundle preparation now uses the exact assigned-contributor AUTH
- PREP boundary from WS-ARCH-001-02G; Submission consumption and binding remain
- separately gated by 02H.
+ PR `#327` activates the exact assigned-contributor AUTH PREP boundary from
+ WS-ARCH-001-02G after merge; Submission consumption and binding remain
+ separately gated by 02H.
- preparation authority is active through WS-ARCH-001-02G; admission
+ preparation authority is pending merge of WS-ARCH-001-02G; admissionAlso applies to: 109-111
🤖 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 `@docs/roadmap_status.md` around lines 64 - 66, Update the WS-ARCH-001-02G
entries in the roadmap status document so they are not presented as implemented
on main while PR `#327` remains unmerged. Move the capability to “Integration In
Progress” or explicitly qualify it as activating after merge, and apply the same
merge-status wording to the authority statement near the active-capability
section; keep “Implemented On main” limited to merged behavior.
…-auth-preparation-activation
There was a problem hiding this comment.
Actionable comments posted: 1
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_authorization.py (1)
2101-2158: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winAdd
ActionId.ARTIFACT_SUBMISSION_BUNDLE_PREPAREto the explicit ACTIVE set.The assertion at Lines 2101-2158 requires the literal set to equal every definition whose availability is
ACTIVE. The literal set lists 54 members. The count assertion at Lines 2234-2240 requires 55 ACTIVE definitions. The two assertions cannot both pass. The missing member is the action this PR activates.Line 2153 also packs two enum members on one line, which breaks the one-per-line style used by the rest of the set.
🐛 Proposed fix
- ActionId.ARTIFACT_GUIDE_SOURCE_BINDING_CREATE, ActionId.ARTIFACT_GUIDE_SOURCE_READ, + ActionId.ARTIFACT_GUIDE_SOURCE_BINDING_CREATE, + ActionId.ARTIFACT_GUIDE_SOURCE_READ, + ActionId.ARTIFACT_SUBMISSION_BUNDLE_PREPARE, ActionId.ARTIFACT_VERIFICATION_EXECUTE,Run the following script to confirm the member count and the catalogue availability:
#!/bin/bash # Count members of the literal ACTIVE set in the changed assertion. python - <<'PY' import re, pathlib src = pathlib.Path("backend/tests/test_authorization.py").read_text().splitlines() block = "\n".join(src[2100:2158]) members = re.findall(r"ActionId\.[A-Z0-9_]+", block) print("literal ACTIVE members:", len(members)) print("has prepare:", "ActionId.ARTIFACT_SUBMISSION_BUNDLE_PREPARE" in members) PY # Count ACTIVE and PLANNED definitions declared in the catalogue. rg -nP -C 3 'ARTIFACT_SUBMISSION_BUNDLE_PREPARE' backend/app/modules/authorization/catalogue.pyAlso applies to: 2234-2240
🤖 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 2101 - 2158, Add ActionId.ARTIFACT_SUBMISSION_BUNDLE_PREPARE to the explicit ACTIVE action set in the authorization test, and format the packed enum entries so each member appears on its own line. Keep the set aligned with the ACTIVE definitions and the existing count assertion.
🧹 Nitpick comments (1)
backend/tests/test_submission_preparation_authorization.py (1)
95-116: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd cases for the remaining denial branches of
evaluate_submitter_authority.The
Factsdouble always returns a locked actor pair and an active grant. The test therefore proves the allow path and the fact-mismatch path only. Three denial branches inbackend/app/modules/authorization/artifact_project_authority.pystay unexercised:
find_active_project_rolereturnsNone, which must raisePreparedAuthorizationUnsupported(PERMISSION_NOT_GRANTED).- The grant status is not
"active", which must deny withPERMISSION_NOT_GRANTED.- The final resource carries an
actor_profile_idoridentity_link_idthat differs from the locked context, which must deny withRESOURCE_GUARD_DENIED. This branch is the actor-substitution guard, so it deserves direct coverage.New backend subsystems must stay at or above 90 percent coverage, as per coding guidelines: "New or materially changed backend subsystems must remain at or above 90 percent test coverage."
🤖 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_submission_preparation_authorization.py` around lines 95 - 116, Add tests covering the remaining denial branches of evaluate_submitter_authority using the Facts double: return None from find_active_project_role and assert PreparedAuthorizationUnsupported with PERMISSION_NOT_GRANTED; return an inactive grant and assert the same denial; and provide final resources whose actor_profile_id or identity_link_id differs from the locked context, asserting RESOURCE_GUARD_DENIED. Keep the existing allow and fact-mismatch coverage and ensure the authorization subsystem remains at least 90% covered.Source: Coding guidelines
🤖 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/test_authorization.py`:
- Around line 2241-2246: Correct the inline comment on the planned-count
assertion in the ACTION_DEFINITIONS availability check so it accurately
describes the expected 47 planned definitions; do not change the assertion or
surrounding logic.
---
Outside diff comments:
In `@backend/tests/test_authorization.py`:
- Around line 2101-2158: Add ActionId.ARTIFACT_SUBMISSION_BUNDLE_PREPARE to the
explicit ACTIVE action set in the authorization test, and format the packed enum
entries so each member appears on its own line. Keep the set aligned with the
ACTIVE definitions and the existing count assertion.
---
Nitpick comments:
In `@backend/tests/test_submission_preparation_authorization.py`:
- Around line 95-116: Add tests covering the remaining denial branches of
evaluate_submitter_authority using the Facts double: return None from
find_active_project_role and assert PreparedAuthorizationUnsupported with
PERMISSION_NOT_GRANTED; return an inactive grant and assert the same denial; and
provide final resources whose actor_profile_id or identity_link_id differs from
the locked context, asserting RESOURCE_GUARD_DENIED. Keep the existing allow and
fact-mismatch coverage and ensure the authorization subsystem remains at least
90% covered.
🪄 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: 26424d29-b793-4964-8000-10e36d886d95
📒 Files selected for processing (15)
.agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/chunks/WS-ARCH-001-02G-auth-preparation-activation.md.agent-loop/initiatives/WS-AUTH-003-module-boundary-recovery/TEST_STRUCTURE_DEBT.jsonbackend/app/modules/artifacts/authorization.pybackend/app/modules/authorization/artifact_project_authority.pybackend/app/modules/authorization/kernel.pybackend/app/modules/authorization/pre_submit_materialization.pybackend/app/modules/authorization/prepared.pybackend/app/modules/authorization/runtime.pybackend/app/modules/authorization/submission_preparation.pybackend/tests/submission_preparation_auth_helpers.pybackend/tests/test_authorization.pybackend/tests/test_default_pre_submit_execution.pybackend/tests/test_submission_bundle_admission.pybackend/tests/test_submission_preparation_adapter.pybackend/tests/test_submission_preparation_authorization.py
💤 Files with no reviewable changes (1)
- backend/tests/test_submission_bundle_admission.py
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/tests/test_default_pre_submit_execution.py
- .agent-loop/initiatives/WS-ARCH-001-modular-monolith-boundaries/chunks/WS-ARCH-001-02G-auth-preparation-activation.md
- backend/app/modules/authorization/kernel.py
- backend/app/modules/authorization/prepared.py
- backend/app/modules/artifacts/authorization.py
- backend/app/modules/authorization/runtime.py
2 participants
Outcome
Activates only
artifact.submission_bundle.preparethrough the existing opaque AUTH PREP protocol after WS-ARCH-001-02A-02F.Safety boundary
submission.create, artifact binding, public route, or adjacent action activationEvidence
Summary by CodeRabbit
New Features
Documentation
Tests