WS-ART-001-04C1: persist submission bundle durable put intent - #296
WS-ART-001-04C1: persist submission bundle durable put intent#296Abiorh001 wants to merge 9 commits into
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)
📝 WalkthroughWalkthroughAdds durable submission-bundle admission with sealed pre-submit capabilities, prepared custody, authorization, immutable evidence-to-put intents, replay validation, and post-commit publication. Database constraints, migrations, integration tests, and architecture specifications cover the new flow. ChangesSubmission bundle durable admission
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant SubmissionBundleDurablePutService
participant ArtifactAdmissionService
participant ArtifactRepository
participant StorageProvider
Caller->>SubmissionBundleDurablePutService: admit_in_transaction(request)
SubmissionBundleDurablePutService->>ArtifactAdmissionService: admit submission bundle
ArtifactAdmissionService->>ArtifactRepository: lock evidence and persist durable intent
SubmissionBundleDurablePutService-->>Caller: committed admission
Caller->>SubmissionBundleDurablePutService: publish_after_commit(...)
SubmissionBundleDurablePutService->>StorageProvider: publish prepared artifact
StorageProvider-->>Caller: bounded put result
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: 2
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/artifacts/service.py (1)
1896-1907: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDigest
request_digestfor pre-existing artifact put attempts before addingpre_submit_evidence_set_id.
_existing_attempt()rejects any mismatch between the recalculatedrequest_digestand the stored value. The new digest input changesnullguideandchecker_outputattempts to includepre_submit_evidence_set_id, so replay fails closed unless persisted rows are backfilled or nulls are excluded from the hash.🤖 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 1896 - 1907, Update the request_digest construction in _existing_attempt() to preserve replay compatibility for pre-existing guide and checker_output attempts: exclude pre_submit_evidence_set_id from the hashed payload when its value is null, or otherwise use the established persisted-digest/backfill path. Ensure recalculated digests still match stored rows created before pre_submit_evidence_set_id was introduced.
🧹 Nitpick comments (13)
backend/app/modules/artifacts/pre_submit_evidence.py (1)
480-482: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the defensive
getattrfallback.
__init__always assigns_live_pass_bindings. Thegetattrdefault hides a future initialization bug and makes_claims_pass_bindingsilently returnFalseinstead of failing. Read the attribute directly.♻️ Proposed change
def _claims_pass_binding(self, binding: object) -> bool: """Recognize only an issuance registered by this live service instance.""" - return binding in getattr(self, "_live_pass_bindings", set()) + return binding in self._live_pass_bindings🤖 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/pre_submit_evidence.py` around lines 480 - 482, Update _claims_pass_binding to read self._live_pass_bindings directly instead of using getattr with an empty-set fallback, preserving the existing membership check and allowing missing initialization to fail visibly.backend/app/modules/artifacts/models.py (1)
1051-1061: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRemove the redundant single-column indexes.
uq_submission_bundle_intent_evidenceanduq_submission_bundle_intent_put_attemptalready create unique btree indexes onpre_submit_evidence_set_idandput_attempt_id. The extraindex=Trueadds two duplicate indexes on the same columns. Each insert then maintains four indexes instead of two.If you drop
index=True, also dropix_submission_bundle_durable_intents_pre_submit_evidence_set_idandix_submission_bundle_durable_intents_put_attempt_idfrombackend/alembic/versions/0059_submission_bundle_durable_intent.py(upgrade lines 150-159 and the matchingdrop_indexcalls indowngrade), and update the expected index set inbackend/tests/test_alembic.py.♻️ Proposed change
pre_submit_evidence_set_id: Mapped[str] = mapped_column( ForeignKey("pre_submit_evidence_sets.id", ondelete="RESTRICT"), nullable=False, - index=True, ) put_attempt_id: Mapped[str] = mapped_column( ForeignKey("artifact_put_attempts.id", ondelete="RESTRICT"), nullable=False, - index=True, )🤖 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/models.py` around lines 1051 - 1061, Remove index=True from the pre_submit_evidence_set_id and put_attempt_id mapped columns in the relevant model. Remove the corresponding ix_submission_bundle_durable_intents_pre_submit_evidence_set_id and ix_submission_bundle_durable_intents_put_attempt_id creation and downgrade calls in the migration 0059_submission_bundle_durable_intent, then update the expected index set in test_alembic.py to exclude both redundant indexes.backend/alembic/versions/0059_submission_bundle_durable_intent.py (1)
87-119: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueConsider pinning
search_pathon the new trigger function.
guard_artifact_receipt_producer_referenceresolvesartifact_put_attemptsthrough the callersearch_path. Addset search_path = pg_catalog, publicto the function definition to make resolution deterministic for every session.♻️ Proposed hardening
create function guard_artifact_receipt_producer_reference() - returns trigger language plpgsql as $$ + returns trigger language plpgsql + set search_path = pg_catalog, public as $$🤖 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/alembic/versions/0059_submission_bundle_durable_intent.py` around lines 87 - 119, Update the SQL definition of guard_artifact_receipt_producer_reference to pin its search path to pg_catalog, public, ensuring artifact_put_attempts resolves deterministically regardless of the caller session’s search_path.backend/app/modules/artifacts/schemas.py (2)
41-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a typed
custodyannotation behindTYPE_CHECKING.
custody: objectremoves static type information from the request contract. The runtime guard insourcestill enforces the exact type. ATYPE_CHECKINGforward reference keeps the cycle broken and restores type checker support.♻️ Proposed annotation change
+if TYPE_CHECKING: + from app.modules.artifacts.submission_custody import SubmissionBundlePreparedCustody + `@final` `@dataclass`(frozen=True, slots=True) class SubmissionBundleArtifactAdmissionRequest: """One passing prepared bundle selected for durable provider intent.""" pre_submit_evidence_set_id: UUID - custody: object + custody: SubmissionBundlePreparedCustody | object replay_durable_intent_id: UUID | None🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/modules/artifacts/schemas.py` around lines 41 - 57, Update SubmissionBundleArtifactAdmissionRequest.custody to use a TYPE_CHECKING-only forward-reference annotation for SubmissionBundlePreparedCustody, while retaining the runtime-local import and exact type guard in source. Preserve the existing runtime behavior and avoid introducing an import cycle.
155-163: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep the
locked_guide_sha256binding explicit for the submission authorizer.
locked_guide_sha256is validated during submission bundle admission, appears in pre-submit evidence digesting, and is present in the locked context.SubmissionBundleDurableIntentAuthorityFactsdoes not include it, so an authorizer cannot bind that locked guide content hash directly. Add it when the real submission authority is implemented.🤖 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/schemas.py` around lines 155 - 163, Add a locked_guide_sha256: str field to SubmissionBundleDurableIntentAuthorityFacts alongside the existing guide and policy hash bindings. Ensure the field is populated from the locked context and carried through submission bundle admission and pre-submit evidence digesting so the submission authorizer can bind the locked guide content hash explicitly.backend/tests/test_submission_bundle_admission.py (2)
208-220: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover every field that participates in the replay lineage.
The loop drifts 7 of the 26 fields. It proves those 7 are included. It does not detect a field that is accidentally dropped from
_submission_replay_lineageduring a later refactor.Assert the full membership instead. Iterate over the participating field names, drift each one, and assert the lineage changes. Keep an explicit exclusion set for fields that must not participate, so the exclusions are also documented and enforced.
💚 Proposed stronger membership check
- for field, changed in { - "semantic_manifest_id": str(uuid4()), - "locked_guide_sha256": _sha("a"), - "effective_policy_id": str(uuid4()), - "pre_submit_policy_id": str(uuid4()), - "catalogue_id": "other.catalogue", - "catalogue_version": "2", - "catalogue_manifest_sha256": _sha("b"), - }.items(): - drifted = SimpleNamespace(**{**values, field: changed}) - assert ArtifactAdmissionService._submission_replay_lineage( - original - ) != ArtifactAdmissionService._submission_replay_lineage(drifted) + excluded = {"result_count", "result_manifest_sha256"} + baseline = ArtifactAdmissionService._submission_replay_lineage(original) + for field, value in values.items(): + if isinstance(value, bool): + changed: object = not value + elif isinstance(value, int): + changed = value + 1 + elif value is None: + changed = str(uuid4()) + else: + changed = f"{value}.drift" + drifted = SimpleNamespace(**{**values, field: changed}) + lineage = ArtifactAdmissionService._submission_replay_lineage(drifted) + if field in excluded: + assert lineage == baseline, field + else: + assert lineage != baseline, fieldAdjust
excludedto match the intended contract.🤖 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 208 - 220, Strengthen the test around ArtifactAdmissionService._submission_replay_lineage by enumerating every field intended to participate, drifting each field and asserting the lineage changes. Define an explicit excluded set for fields that must not participate, and assert those exclusions remain unchanged; align the excluded fields with the intended contract.
223-250: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a barrier so the two consumers actually race.
asyncio.to_threadstarts the two calls without synchronization. The second thread often starts after the first has already finished. The assertionsorted(outcomes) == [False, True]then passes on a purely sequential execution, so the test does not prove thatconsumeis atomic under contention.Release both threads at the same point with a
threading.Barrier(2).💚 Proposed barrier
+import threading + manager, prepared = await _prepared(tmp_path) capability = _capability(prepared, uuid4()) + barrier = threading.Barrier(2) def consume() -> bool: + barrier.wait(timeout=5) try: capability.consume(🤖 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 223 - 250, Update test_concurrent_pass_capability_consumption_has_one_winner to create a threading.Barrier(2), have each consume invocation wait on the barrier immediately before capability.consume, and then run both consumers through asyncio.to_thread. Preserve the existing outcome assertion and cleanup while ensuring both calls are released together to exercise concurrent consumption.backend/tests/test_default_pre_submit_execution.py (2)
81-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert on the recorded authority facts.
_AllowSubmissionPreparedAuthorizationstoresfacts, but no assertion readsfinal_authority.factsin the workflow test. The test therefore does not verify the content ofSubmissionBundleDurableIntentAuthorityFacts, which is the central new contract of this PR.Add assertions after the first successful admission. Check at least
actor_profile_id,task_id,pre_submit_evidence_set_id, andarchive_sha256.💚 Proposed assertion after the first admission
provider.execute_committed_put.assert_not_awaited() provider.resume_committed_put.assert_not_awaited() + assert final_authority.facts is not None + assert final_authority.facts.actor_profile_id == actor_id + assert final_authority.facts.task_id == request.task_id + assert final_authority.facts.pre_submit_evidence_set_id == ( + first.evidence.evidence_set_id + ) + assert ( + final_authority.facts.archive_sha256 + == request.prepared_artifact.commitment.sha256 + )🤖 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_default_pre_submit_execution.py` around lines 81 - 89, Extend the workflow test after the first successful admission to assert the recorded facts on final_authority.facts. Verify actor_profile_id, task_id, pre_submit_evidence_set_id, and archive_sha256 against the expected admission values, covering the SubmissionBundleDurableIntentAuthorityFacts contract.
752-795: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClose
drift_preparedanddenied_preparedexplicitly in the test.Both artifacts are never closed by the test body. They are closed only because
admit_in_transactionrunsawait prepared.close()in itsexcept BaseExceptionhandler. The test therefore depends on an implementation detail of the code under test for its own cleanup.
manager.close()in thefinallyblock is a backstop, but an explicit close documents the expected ownership. It also keeps the test correct if the service later stops closing on failure.♻️ Proposed explicit cleanup
pass_capability=drift.pass_capability, ) ) + await drift_prepared.close() await session.execute(pass_capability=denied.pass_capability, ) ) + await denied_prepared.close() durable_counts = {🤖 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_default_pre_submit_execution.py` around lines 752 - 795, Explicitly close both drift_prepared and denied_prepared in the test after their respective admission attempts, using cleanup that runs even when the expected exception is raised. Keep manager.close() as the existing backstop, and ensure cleanup does not rely on admit_in_transaction internally closing the artifacts.backend/app/modules/artifacts/submission_admission.py (1)
90-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider making the two-phase handoff a context manager.
admit_in_transactionreturns an openPreparedArtifact. Ownership transfers to the caller. If the caller does not callpublish_after_commit, the scratch file stays open until theArtifactScratchManagercloses. The error paths inside this method are correct, but the success path depends on caller discipline.An async context manager, or a single method that accepts the caller transaction boundary, would make the close guaranteed. This is a design suggestion, not a defect in the current tests.
🤖 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/submission_admission.py` around lines 90 - 103, The two-phase handoff in admit_in_transaction should guarantee PreparedArtifact cleanup instead of relying on callers to invoke publish_after_commit. Refactor this handoff to use an async context manager or accept the caller’s transaction boundary, ensuring the prepared artifact is closed whenever the post-commit publication path is not completed while preserving successful publication behavior.backend/tests/test_effective_pre_submit_execution.py (1)
92-100: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared test helper for minting a pass capability.
backend/tests/test_submission_bundle_admission.pydefines an equivalent_capabilityhelper at Lines 52-62 that also callsPreSubmitEvidenceService(SimpleNamespace())._mint_pass_capability(...). Both call sites reach into the same private method.Move the mint call into a shared test helper module. A single helper keeps both tests aligned if the guarded-construction signature changes.
🤖 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_effective_pre_submit_execution.py` around lines 92 - 100, Extract the repeated PreSubmitEvidenceService._mint_pass_capability invocation into a shared test helper module, then update test_effective_pre_submit_execution.py and test_submission_bundle_admission.py to use that helper. Preserve the existing arguments and return value so both tests remain aligned with the guarded-construction signature.backend/app/modules/artifacts/service.py (1)
2151-2208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the paired positional tuples with a field-keyed comparison.
Lines 2162-2203 compare an 18-element
priortuple against an 18-elementcurrenttuple. The two literals must stay positionally aligned by hand. A future insertion or reordering in one tuple silently shifts the comparison and weakens the replay fence without a syntax or type error.
test_replay_lineage_includes_policy_catalogue_and_manifest_identitycovers_submission_replay_lineageonly. It does not cover this inline pair, so a misalignment can pass CI.Extract two small helpers that build
dict[str, object]frompriorand fromcurrent, then compare the dictionaries. Field names then bind the operands.🤖 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 2151 - 2208, Replace the positional tuple comparison in the replay validation condition with two small field-keyed helper mappings for the relevant prior and current fields, returning dict[str, object]. Compare those dictionaries directly while preserving the existing field values and conversions, and use the helpers at the comparison site so field additions or reordering cannot silently misalign operands.backend/tests/test_alembic.py (1)
207-246: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCapture every constraint that migration 0059 replaces.
upgrade()replaces four check constraints:producer_request_type,producer_identity, andproducer_referenceonartifact_put_attempts, pluscontract_producer_referenceonartifact_operation_receipts.downgrade()restores all four.This helper reads only
producer_request_type. Theprior == restoredassertion at Line 276 therefore cannot detect a downgrade defect in the other three constraints.Read all four definitions so the round trip proves full reversibility.
♻️ Proposed helper extension
- request_type = await connection.scalar( - text( - "select pg_get_constraintdef(oid) from pg_constraint " - "where conrelid='artifact_put_attempts'::regclass " - "and conname='producer_request_type'" - ) - ) + put_constraints = dict( + ( + await connection.execute( + text( + "select conname, pg_get_constraintdef(oid) from pg_constraint " + "where conrelid='artifact_put_attempts'::regclass " + "and conname in " + "('producer_request_type','producer_identity','producer_reference')" + ) + ) + ).all() + ) + request_type = put_constraints.get("producer_request_type") + receipt_reference = await connection.scalar( + text( + "select pg_get_constraintdef(oid) from pg_constraint " + "where conrelid='artifact_operation_receipts'::regclass " + "and conname='contract_producer_reference'" + ) + )return { "table_exists": table_exists, "request_type": request_type, + "put_constraints": put_constraints, + "receipt_reference": receipt_reference, "constraints": constraints, "triggers": triggers, "receipt_triggers": receipt_triggers, }🤖 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_alembic.py` around lines 207 - 246, Extend the migration test’s pre-migration capture alongside request_type to read and store the definitions of producer_identity and producer_reference on artifact_put_attempts, plus contract_producer_reference on artifact_operation_receipts. Include all four captured definitions in the existing prior/restored comparison so the downgrade round-trip validates every constraint replaced by migration 0059.
🤖 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/service.py`:
- Around line 2317-2332: Move the replay-lineage validation in
_submission_bundle_replay_attempt ahead of pass_capability.consume in
_submission_bundle_facts, so failed replay validation cannot consume the
original process-local capability. Preserve immediate consumption only for the
fresh preparation capability and retain the existing consumed-evidence identity
check after validation.
In `@docs/spec_artifact_storage_service.md`:
- Around line 1242-1243: Update the documentation sentence describing the typed
joins around PreSubmitEvidenceSet and ArtifactPutAttempt to state that the
listed actor, identity, project, task, assignment, predecessor, and version
facts are reachable through those joined records, not stored on the
SubmissionBundleDurableIntent join row; keep its four stored fields and
non-duplicated lineage clear.
---
Outside diff comments:
In `@backend/app/modules/artifacts/service.py`:
- Around line 1896-1907: Update the request_digest construction in
_existing_attempt() to preserve replay compatibility for pre-existing guide and
checker_output attempts: exclude pre_submit_evidence_set_id from the hashed
payload when its value is null, or otherwise use the established
persisted-digest/backfill path. Ensure recalculated digests still match stored
rows created before pre_submit_evidence_set_id was introduced.
---
Nitpick comments:
In `@backend/alembic/versions/0059_submission_bundle_durable_intent.py`:
- Around line 87-119: Update the SQL definition of
guard_artifact_receipt_producer_reference to pin its search path to pg_catalog,
public, ensuring artifact_put_attempts resolves deterministically regardless of
the caller session’s search_path.
In `@backend/app/modules/artifacts/models.py`:
- Around line 1051-1061: Remove index=True from the pre_submit_evidence_set_id
and put_attempt_id mapped columns in the relevant model. Remove the
corresponding ix_submission_bundle_durable_intents_pre_submit_evidence_set_id
and ix_submission_bundle_durable_intents_put_attempt_id creation and downgrade
calls in the migration 0059_submission_bundle_durable_intent, then update the
expected index set in test_alembic.py to exclude both redundant indexes.
In `@backend/app/modules/artifacts/pre_submit_evidence.py`:
- Around line 480-482: Update _claims_pass_binding to read
self._live_pass_bindings directly instead of using getattr with an empty-set
fallback, preserving the existing membership check and allowing missing
initialization to fail visibly.
In `@backend/app/modules/artifacts/schemas.py`:
- Around line 41-57: Update SubmissionBundleArtifactAdmissionRequest.custody to
use a TYPE_CHECKING-only forward-reference annotation for
SubmissionBundlePreparedCustody, while retaining the runtime-local import and
exact type guard in source. Preserve the existing runtime behavior and avoid
introducing an import cycle.
- Around line 155-163: Add a locked_guide_sha256: str field to
SubmissionBundleDurableIntentAuthorityFacts alongside the existing guide and
policy hash bindings. Ensure the field is populated from the locked context and
carried through submission bundle admission and pre-submit evidence digesting so
the submission authorizer can bind the locked guide content hash explicitly.
In `@backend/app/modules/artifacts/service.py`:
- Around line 2151-2208: Replace the positional tuple comparison in the replay
validation condition with two small field-keyed helper mappings for the relevant
prior and current fields, returning dict[str, object]. Compare those
dictionaries directly while preserving the existing field values and
conversions, and use the helpers at the comparison site so field additions or
reordering cannot silently misalign operands.
In `@backend/app/modules/artifacts/submission_admission.py`:
- Around line 90-103: The two-phase handoff in admit_in_transaction should
guarantee PreparedArtifact cleanup instead of relying on callers to invoke
publish_after_commit. Refactor this handoff to use an async context manager or
accept the caller’s transaction boundary, ensuring the prepared artifact is
closed whenever the post-commit publication path is not completed while
preserving successful publication behavior.
In `@backend/tests/test_alembic.py`:
- Around line 207-246: Extend the migration test’s pre-migration capture
alongside request_type to read and store the definitions of producer_identity
and producer_reference on artifact_put_attempts, plus
contract_producer_reference on artifact_operation_receipts. Include all four
captured definitions in the existing prior/restored comparison so the downgrade
round-trip validates every constraint replaced by migration 0059.
In `@backend/tests/test_default_pre_submit_execution.py`:
- Around line 81-89: Extend the workflow test after the first successful
admission to assert the recorded facts on final_authority.facts. Verify
actor_profile_id, task_id, pre_submit_evidence_set_id, and archive_sha256
against the expected admission values, covering the
SubmissionBundleDurableIntentAuthorityFacts contract.
- Around line 752-795: Explicitly close both drift_prepared and denied_prepared
in the test after their respective admission attempts, using cleanup that runs
even when the expected exception is raised. Keep manager.close() as the existing
backstop, and ensure cleanup does not rely on admit_in_transaction internally
closing the artifacts.
In `@backend/tests/test_effective_pre_submit_execution.py`:
- Around line 92-100: Extract the repeated
PreSubmitEvidenceService._mint_pass_capability invocation into a shared test
helper module, then update test_effective_pre_submit_execution.py and
test_submission_bundle_admission.py to use that helper. Preserve the existing
arguments and return value so both tests remain aligned with the
guarded-construction signature.
In `@backend/tests/test_submission_bundle_admission.py`:
- Around line 208-220: Strengthen the test around
ArtifactAdmissionService._submission_replay_lineage by enumerating every field
intended to participate, drifting each field and asserting the lineage changes.
Define an explicit excluded set for fields that must not participate, and assert
those exclusions remain unchanged; align the excluded fields with the intended
contract.
- Around line 223-250: Update
test_concurrent_pass_capability_consumption_has_one_winner to create a
threading.Barrier(2), have each consume invocation wait on the barrier
immediately before capability.consume, and then run both consumers through
asyncio.to_thread. Preserve the existing outcome assertion and cleanup while
ensuring both calls are released together to exercise concurrent consumption.
🪄 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: 5d32d02e-49b5-4556-96ce-b3f7f0ac685a
📒 Files selected for processing (20)
backend/alembic/versions/0059_submission_bundle_durable_intent.pybackend/app/db/models.pybackend/app/modules/artifacts/models.pybackend/app/modules/artifacts/pre_submit_evidence.pybackend/app/modules/artifacts/repository.pybackend/app/modules/artifacts/schemas.pybackend/app/modules/artifacts/service.pybackend/app/modules/artifacts/submission_admission.pybackend/app/modules/artifacts/submission_authorization.pybackend/app/modules/artifacts/submission_custody.pybackend/app/modules/tasks/pre_submit_context.pybackend/scripts/run_test_lanes.pybackend/tests/test_alembic.pybackend/tests/test_artifact_architecture.pybackend/tests/test_ci_test_lanes.pybackend/tests/test_default_pre_submit_execution.pybackend/tests/test_effective_pre_submit_execution.pybackend/tests/test_submission_bundle_admission.pydocs/architecture_data_model.mddocs/spec_artifact_storage_service.md
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_default_pre_submit_execution.py (1)
74-91: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftUse issuance-observed capabilities here.
_fresh_checked_bundle()returns a newpass_capabilityeach call but stores it only inresult, while the workflow’s originalresponse.pass_capabilityis used later. This makes replay test the original pre-submission pass capability instead of the bundle produced by that fresh execution. Updatefresh_checked_bundle()to return the capability, or usefresh.pass_capabilityat the admission call sites.🤖 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_default_pre_submit_execution.py` around lines 74 - 91, Update _fresh_checked_bundle() and its admission call sites so replay uses the pass_capability issued by the fresh execution rather than the original response.pass_capability. Return the capability from _fresh_checked_bundle() or consistently reference fresh.pass_capability when invoking admission, ensuring the issuance-observed capability is used.
🤖 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_default_pre_submit_execution.py`:
- Around line 933-939: Update the cleanup block around manager.close(),
custody_triggers restoration, and engine.dispose() so engine.dispose() executes
from an inner finally block even when the restoration transaction or enable
trigger statement fails. Preserve the existing reversed trigger restoration
behavior while guaranteeing engine disposal on every cleanup path.
---
Outside diff comments:
In `@backend/tests/test_default_pre_submit_execution.py`:
- Around line 74-91: Update _fresh_checked_bundle() and its admission call sites
so replay uses the pass_capability issued by the fresh execution rather than the
original response.pass_capability. Return the capability from
_fresh_checked_bundle() or consistently reference fresh.pass_capability when
invoking admission, ensuring the issuance-observed capability is used.
🪄 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: 410e000b-9aaa-45d7-bb44-7a50b289213a
📒 Files selected for processing (5)
backend/alembic/versions/0059_submission_bundle_durable_intent.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_default_pre_submit_execution.pydocs/spec_artifact_storage_service.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/spec_artifact_storage_service.md
- backend/tests/test_alembic.py
4950417 to
1b557f4
Compare
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/alembic/versions/0060_submission_bundle_durable_intent.py`:
- Around line 139-148: Update the migration creating the durable-intent foreign
key to add a PostgreSQL insert trigger that checks the referenced
artifact_put_attempts row has producer_request_type equal to submission_bundle,
rejecting other producer types. Define the trigger function and trigger during
upgrade, and drop both in downgrade alongside the existing constraints.
🪄 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: 69235f38-9d11-4549-a9a9-41b4e6d1a0eb
📒 Files selected for processing (7)
backend/alembic/versions/0060_submission_bundle_durable_intent.pybackend/app/modules/artifacts/service.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_artifact_authorization.pybackend/tests/test_default_pre_submit_execution.pydocs/spec_artifact_storage_service.md
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/spec_artifact_storage_service.md
- backend/tests/conftest.py
- backend/tests/test_default_pre_submit_execution.py
- backend/app/modules/artifacts/service.py
1 participant
What changed
Why
04C1 must prove that durable evidence alone cannot authorize storage and that caller loss after intent commit does not strand or duplicate the artifact operation.
Validation
The PostgreSQL migration and DB-backed workflow tests are intentionally delegated to hosted Backend/Agent Gates because WORKSTREAM_TEST_DATABASE_URL is not configured locally.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation