Implement ART-04C2 ready admission publication - #300
Conversation
|
Warning Review limit reached
Next review available in: 27 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 (12)
📝 WalkthroughWalkthroughAdds hidden ZIP submission preparation with deny-by-default authorization, durable evidence, verified immutable ChangesSubmission bundle admission
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TaskRouter
participant PreparedSubmissionBundlePreparationCommand
participant SubmissionBundlePreparationAuthorization
participant PreparedBundlePreSubmitEvidenceService
participant SubmissionBundleDurablePut
participant VerificationService
participant SubmissionBundleAdmissionPublisher
TaskRouter->>PreparedSubmissionBundlePreparationCommand: forward authenticated ZIP request
PreparedSubmissionBundlePreparationCommand->>SubmissionBundlePreparationAuthorization: preflight and prepare final authorization
PreparedSubmissionBundlePreparationCommand->>PreparedBundlePreSubmitEvidenceService: materialize and persist evidence
PreparedSubmissionBundlePreparationCommand->>SubmissionBundleDurablePut: create or replay durable put
VerificationService->>SubmissionBundleAdmissionPublisher: publish verified result
SubmissionBundleAdmissionPublisher-->>TaskRouter: return bounded ready or replay state
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[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/app/modules/artifacts/service.py (1)
1267-1285: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winTerminalize verified verification jobs when publication fails.
_complete_verificationwrites the receipt, thenpublish_verifiedmay raiseSubmissionBundleAdmissionPublicationErrorfor missing durable intent or lineage mismatch. That aborts the terminal transaction beforejob.status,terminal_at, and_clear_job_fence()run, so the job remainsrunningwithlease_expires_atset. Catch the publication error, call_terminalize_verification_conflict(job, now), and returnconflictinstead of letting the exception roll back verification completion.🤖 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 1267 - 1285, The _complete_verification flow must terminalize verified jobs when publish_verified raises SubmissionBundleAdmissionPublicationError. Catch that specific error around SubmissionBundleAdmissionPublisher.publish_verified, call _terminalize_verification_conflict(job, now), and return "conflict" so job status, terminal_at, and _clear_job_fence() are still applied instead of propagating the exception.
🧹 Nitpick comments (10)
backend/alembic/versions/0061_submission_bundle_admission.py (2)
61-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePass the logical constraint name, not a
ck_-prefixed name.
op.create_check_constraintapplies the naming conventionck_%(table_name)s_%(constraint_name)s. The nameck_pre_submit_evidence_policy_context_sha256therefore produces the physical constraintck_pre_submit_evidence_sets_ck_pre_submit_evidence_policy_context_sha256. The doubled prefix is inconsistent with the in-table constraints in this same migration, which use bare logical names such aspolicy_context_hashandarchive_sha256, and with0060_submission_bundle_durable_intent.py, which usescontract_producer_reference.Upgrade and downgrade stay symmetric, so no round trip breaks. Rename the logical name for consistency and to keep the physical name readable.
Based on learnings: pass
op.create_check_constraintandop.drop_constraintthe logical constraint-name suffix, not the physical PostgreSQL constraint name.♻️ Proposed rename
op.create_check_constraint( - "ck_pre_submit_evidence_policy_context_sha256", + "policy_context_sha256", "pre_submit_evidence_sets", "locked_policy_context_hash ~ '^sha256:[0-9a-f]{64}$'", )Apply the matching change in
downgrade:op.drop_constraint( - "ck_pre_submit_evidence_policy_context_sha256", + "policy_context_sha256", "pre_submit_evidence_sets", type_="check", )🤖 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/0061_submission_bundle_admission.py` around lines 61 - 65, Rename the check constraint identifier used by the create and matching drop operations in the migration from the ck_-prefixed value to the bare logical name policy_context_hash, keeping the existing table and validation expression unchanged and ensuring upgrade and downgrade remain symmetric.Source: Learnings
44-56: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winKeep the policy-context hash encoding consistent.
The migration filters out only
id, so its backfil values for the ten policy-context columns should match the same keys thatbackend/app/modules/artifacts/pre_submit_evidence.pywrites. Use the existingcanonical_json_hashhelper in the backfill, then the stored identity remains deterministic across backfilled and application-written rows.🤖 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/0061_submission_bundle_admission.py` around lines 44 - 56, The migration’s backfill currently reimplements policy-context hashing and may include keys differently from application writes. Update the backfill loop in the migration to use the existing canonical_json_hash helper from pre_submit_evidence.py with the ten policy-context columns, preserving the sha256 digest format and database update behavior.backend/tests/test_alembic.py (1)
295-360: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCover the
pre_submit_evidence_setschanges and the backfill path.The probe and the round trip only inspect
submission_bundle_admissions. Migration0061also addslocked_policy_context_hashtopre_submit_evidence_sets, backfills it, sets itNOT NULL, adds a check constraint, and disables and re-enablespre_submit_evidence_sets_immutablearound the backfill. None of that is asserted.Two gaps result:
- The round trip does not prove the column and the check constraint are removed on downgrade, or that the immutability trigger is left enabled after upgrade.
- The backfill loop at migration lines 44-56 runs only when evidence rows exist. This empty-database test never executes it, so the trigger disable/enable sequence and the
NOT NULLalter are untested against data.Extend
_submission_bundle_admission_schemato report the evidence column, its constraint, andtgenabledforpre_submit_evidence_sets_immutable. Add a second test that inserts onepre_submit_evidence_setsrow before the upgrade and asserts the backfilled digest.As per coding guidelines: "New or materially changed backend subsystems must maintain at least 90% test coverage".
♻️ Proposed probe extension
triggers = set( ( await connection.scalars( text( "select tgname from pg_trigger where not tgisinternal and " "tgrelid=to_regclass('submission_bundle_admissions')" ) ) ).all() ) - return {"table_exists": table_exists, "constraints": constraints, "triggers": triggers} + evidence_column = bool( + await connection.scalar( + text( + "select count(*) from information_schema.columns " + "where table_name='pre_submit_evidence_sets' " + "and column_name='locked_policy_context_hash'" + ) + ) + ) + evidence_immutable_enabled = await connection.scalar( + text( + "select tgenabled from pg_trigger " + "where tgname='pre_submit_evidence_sets_immutable'" + ) + ) + return { + "table_exists": table_exists, + "constraints": constraints, + "triggers": triggers, + "evidence_column": evidence_column, + "evidence_immutable_enabled": evidence_immutable_enabled, + }🤖 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 295 - 360, Extend _submission_bundle_admission_schema to inspect pre_submit_evidence_sets for locked_policy_context_hash existence, its check constraint, and the pre_submit_evidence_sets_immutable trigger’s tgenabled state. Update test_0061_submission_bundle_admission_empty_round_trip to assert these artifacts are absent after downgrade and present with the trigger enabled after upgrade. Add a populated round-trip test that inserts one pre_submit_evidence_sets row before upgrading, then verifies migration 0061 backfills locked_policy_context_hash with the expected digest and preserves the enabled trigger state.Source: Coding guidelines
backend/app/modules/artifacts/submission_materialization.py (1)
313-339: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueNo change needed for
execute’s transaction context.No production caller invokes
PreparedBundlePreSubmitEvidenceService.execute; only the test covers its transaction-free requirement. Remove it if a public service path is intended, or add a production caller under the same required 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/app/modules/artifacts/submission_materialization.py` around lines 313 - 339, The PreparedBundlePreSubmitEvidenceService.execute method has no production caller, so either remove execute and its test if this service is not intended as a public entry point, or add a production caller that invokes it with the required transaction-free session contract. Keep materialize and persist behavior unchanged.backend/tests/test_default_pre_submit_execution.py (2)
960-983: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for more than one admission in a scope.
The scenario publishes exactly one admission, so
unbound_ready_bytesandstale_bytesalways equal one charge. That value stays correct even if the aggregate inbackend/app/modules/artifacts/operator.pyjoins one charge row to several admissions. Add a case with two admissions in the same project scope. This case proves that the reported bytes stay consistent withcounted_bytes.🤖 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 960 - 983, Extend the test scenario around admission_usage to publish two admissions within the same project scope, then assert the aggregate unbound_ready_bytes and stale_bytes remain consistent with counted_bytes rather than being multiplied by the admission join. Update the existing assertions in the test to validate both admissions’ counts and byte totals.
984-1000: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the
commitcall out of thepytest.raisesblock.
await session.commit()on Line 992 and Line 999 is unreachable whensession.executeraises. If the database reports the violation only at commit time, the intent still holds, but the current shape hides which statement fails. Put theexecuteandcommitcalls in one explicit sequence outside the assertion, or drop the unreachablecommitcall.🤖 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 984 - 1000, Update the two DBAPIError assertions around the session.execute calls so the commit is not placed after an operation that is expected to raise; remove the unreachable await session.commit() calls or restructure each assertion to explicitly cover the execute-and-commit sequence, preserving rollback cleanup afterward.backend/app/modules/artifacts/operator.py (1)
572-583: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead each aggregate tuple once.
Each field calls
admission_usage.get(...)again and rebuilds the default tuple. Bind the two tuples once for readability.🤖 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/operator.py` around lines 572 - 583, In the code producing the aggregate fields around admission_usage, bind the “ready” and “stale” lookup results once using the existing scope key, then read each tuple’s count and bytes values for unbound_ready_count, unbound_ready_bytes, stale_count, and stale_bytes. Remove the repeated admission_usage.get calls while preserving the current defaults and output values.backend/app/modules/artifacts/submission_admission_publication.py (1)
136-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd type annotations to
_matches_verified_lineage.The parameters
evidence,attempt,job,replica,content, andreceipthave no annotations. The rest of this module is fully typed. Annotate them with the model types and| Nonewhere the caller can passNone.♻️ Proposed annotation
`@staticmethod` - def _matches_verified_lineage(evidence, attempt, job, replica, content, receipt) -> bool: + def _matches_verified_lineage( + evidence: PreSubmitEvidenceSet | None, + attempt: ArtifactPutAttempt, + job: ArtifactVerificationJob, + replica: ArtifactReplica | None, + content: ArtifactContent | None, + receipt: ArtifactVerificationReceipt | None, + ) -> bool:🤖 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_publication.py` around lines 136 - 167, Update the `_matches_verified_lineage` signature to annotate `evidence`, `attempt`, `job`, `replica`, `content`, and `receipt` with their corresponding model types, adding `| None` for parameters that callers may pass as `None`; preserve the existing boolean return annotation and method behavior.backend/app/adapters/artifacts/__init__.py (1)
288-304: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBuild one
SubmissionArchiveInspectorand reuse it.The composition creates two inspectors from the same
submission_archive_limits(settings)value. One goes toPreparedBundleMaterializationService, the other toSubmissionBundlePreparationRuntime.inspector. The duplication doubles the per-request construction and lets the two limit sources drift if only one call site is later changed.♻️ Proposed refactor
preparation = ArtifactPreparationService(manager) catalogue = request.app.state.pre_submission_checker_catalogue + inspector = SubmissionArchiveInspector(submission_archive_limits(settings)) storage_schemes = {"local": "local", "s3_compatible": "s3"} @@ materialization = PreparedBundleMaterializationService( authorization=materialization_authority, preparation=preparation, - archive_inspector=SubmissionArchiveInspector( - submission_archive_limits(settings) - ), + archive_inspector=inspector, catalogue=catalogue, storage_scheme=storage_scheme, ) @@ yield SubmissionBundlePreparationRuntime( preparation=preparation, - inspector=SubmissionArchiveInspector(submission_archive_limits(settings)), + inspector=inspector, catalogue=catalogue,🤖 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/adapters/artifacts/__init__.py` around lines 288 - 304, Construct a single SubmissionArchiveInspector from submission_archive_limits(settings) in the surrounding runtime composition, then pass that same instance to both PreparedBundleMaterializationService and SubmissionBundlePreparationRuntime.inspector. Remove the second inline construction while preserving the existing service wiring.backend/tests/test_artifact_architecture.py (1)
569-584: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRestore the forbidden-field assertion for the excluded request.
Line 61 removes
SubmissionBundlePreparationRequestfromPREPARED_MUTATION_REQUESTS. That exclusion also drops the check at line 495, which rejectsaction_id,resource_context, andfactsfields. This new test replaces only theprepared_authorizationcheck. Add the disjoint-field assertion so the request keeps the same architecture guarantee.♻️ Proposed addition
assert "prepared_authorization" not in fields assert fields["authorization_context"] == {"AuthorizationContext"} assert fields["idempotency_key"] == {"UUID"} + assert {"action_id", "resource_context", "facts"}.isdisjoint(fields)🤖 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_artifact_architecture.py` around lines 569 - 584, Update test_submission_preparation_http_request_never_carries_prepared_authority to also assert that SubmissionBundlePreparationRequest excludes action_id, resource_context, and facts, preserving the forbidden-field architecture guarantee alongside the existing prepared_authorization check.
🤖 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_admission_publication.py`:
- Around line 95-98: Update the lineage-mismatch handling in
_complete_verification so an ineligible or permanently inadmissible
terminal_status of "passed" produces a terminal "conflict" outcome instead of
raising SubmissionBundleAdmissionPublicationError. Preserve the verification
receipt and terminal job state, preventing retries from reverting the verified
job to pending, running, or provider_unavailable.
In `@backend/app/modules/artifacts/submission_preparation.py`:
- Around line 323-328: Add a partial unique index in migration
0061_submission_bundle_admission.py enforcing uniqueness of
consumed_by_submission_id for non-null values, so each submission admission
reference can have at most one consumer. Keep existing constraints unchanged and
ensure the migration includes the corresponding downgrade removal.
In `@backend/app/modules/tasks/router.py`:
- Around line 410-413: Validate the summary and contributor_attestation header
values at the task route boundary before constructing SubmissionPacketView,
rejecting any non-ASCII content. Ensure only validated ASCII values enter
immutable pre-submit evidence and hashing, while preserving existing handling
for absent headers.
- Around line 432-449: Update the route boundary around
PreparedSubmissionBundlePreparationCommand.prepare() to catch
PreSubmitLockedContextInvalid and map it to an appropriate handled client HTTP
error, preserving the existing handling for ArtifactAuthorityDeniedError and
SubmissionBundlePreparationRejected.
---
Outside diff comments:
In `@backend/app/modules/artifacts/service.py`:
- Around line 1267-1285: The _complete_verification flow must terminalize
verified jobs when publish_verified raises
SubmissionBundleAdmissionPublicationError. Catch that specific error around
SubmissionBundleAdmissionPublisher.publish_verified, call
_terminalize_verification_conflict(job, now), and return "conflict" so job
status, terminal_at, and _clear_job_fence() are still applied instead of
propagating the exception.
---
Nitpick comments:
In `@backend/alembic/versions/0061_submission_bundle_admission.py`:
- Around line 61-65: Rename the check constraint identifier used by the create
and matching drop operations in the migration from the ck_-prefixed value to the
bare logical name policy_context_hash, keeping the existing table and validation
expression unchanged and ensuring upgrade and downgrade remain symmetric.
- Around line 44-56: The migration’s backfill currently reimplements
policy-context hashing and may include keys differently from application writes.
Update the backfill loop in the migration to use the existing
canonical_json_hash helper from pre_submit_evidence.py with the ten
policy-context columns, preserving the sha256 digest format and database update
behavior.
In `@backend/app/adapters/artifacts/__init__.py`:
- Around line 288-304: Construct a single SubmissionArchiveInspector from
submission_archive_limits(settings) in the surrounding runtime composition, then
pass that same instance to both PreparedBundleMaterializationService and
SubmissionBundlePreparationRuntime.inspector. Remove the second inline
construction while preserving the existing service wiring.
In `@backend/app/modules/artifacts/operator.py`:
- Around line 572-583: In the code producing the aggregate fields around
admission_usage, bind the “ready” and “stale” lookup results once using the
existing scope key, then read each tuple’s count and bytes values for
unbound_ready_count, unbound_ready_bytes, stale_count, and stale_bytes. Remove
the repeated admission_usage.get calls while preserving the current defaults and
output values.
In `@backend/app/modules/artifacts/submission_admission_publication.py`:
- Around line 136-167: Update the `_matches_verified_lineage` signature to
annotate `evidence`, `attempt`, `job`, `replica`, `content`, and `receipt` with
their corresponding model types, adding `| None` for parameters that callers may
pass as `None`; preserve the existing boolean return annotation and method
behavior.
In `@backend/app/modules/artifacts/submission_materialization.py`:
- Around line 313-339: The PreparedBundlePreSubmitEvidenceService.execute method
has no production caller, so either remove execute and its test if this service
is not intended as a public entry point, or add a production caller that invokes
it with the required transaction-free session contract. Keep materialize and
persist behavior unchanged.
In `@backend/tests/test_alembic.py`:
- Around line 295-360: Extend _submission_bundle_admission_schema to inspect
pre_submit_evidence_sets for locked_policy_context_hash existence, its check
constraint, and the pre_submit_evidence_sets_immutable trigger’s tgenabled
state. Update test_0061_submission_bundle_admission_empty_round_trip to assert
these artifacts are absent after downgrade and present with the trigger enabled
after upgrade. Add a populated round-trip test that inserts one
pre_submit_evidence_sets row before upgrading, then verifies migration 0061
backfills locked_policy_context_hash with the expected digest and preserves the
enabled trigger state.
In `@backend/tests/test_artifact_architecture.py`:
- Around line 569-584: Update
test_submission_preparation_http_request_never_carries_prepared_authority to
also assert that SubmissionBundlePreparationRequest excludes action_id,
resource_context, and facts, preserving the forbidden-field architecture
guarantee alongside the existing prepared_authorization check.
In `@backend/tests/test_default_pre_submit_execution.py`:
- Around line 960-983: Extend the test scenario around admission_usage to
publish two admissions within the same project scope, then assert the aggregate
unbound_ready_bytes and stale_bytes remain consistent with counted_bytes rather
than being multiplied by the admission join. Update the existing assertions in
the test to validate both admissions’ counts and byte totals.
- Around line 984-1000: Update the two DBAPIError assertions around the
session.execute calls so the commit is not placed after an operation that is
expected to raise; remove the unreachable await session.commit() calls or
restructure each assertion to explicitly cover the execute-and-commit sequence,
preserving rollback cleanup afterward.
🪄 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: 23f362b9-ca92-47c6-b2c8-83ee48a8fe3e
📒 Files selected for processing (27)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/AUTH_HANDOFF.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/STATUS.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-04C2-ready-admission-publication.mdbackend/alembic/versions/0061_submission_bundle_admission.pybackend/app/adapters/artifacts/__init__.pybackend/app/interfaces/artifact_operations.pybackend/app/modules/artifacts/models.pybackend/app/modules/artifacts/operator.pybackend/app/modules/artifacts/pre_submit_evidence.pybackend/app/modules/artifacts/router.pybackend/app/modules/artifacts/service.pybackend/app/modules/artifacts/submission_admission.pybackend/app/modules/artifacts/submission_admission_publication.pybackend/app/modules/artifacts/submission_authorization.pybackend/app/modules/artifacts/submission_materialization.pybackend/app/modules/artifacts/submission_preparation.pybackend/app/modules/tasks/pre_submit_context.pybackend/app/modules/tasks/router.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_artifact_architecture.pybackend/tests/test_artifact_operator_api.pybackend/tests/test_default_pre_submit_execution.pybackend/tests/test_submission_bundle_admission.pydocs/architecture_data_model.mddocs/operations_artifact_storage.mddocs/spec_artifact_storage_service.md
| admission = await self._session.scalar( | ||
| select(SubmissionBundleAdmission).where( | ||
| SubmissionBundleAdmission.consumed_by_submission_id == str(submission_id), | ||
| SubmissionBundleAdmission.status == "consumed", | ||
| ) | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
consumed_by_submission_id has no uniqueness constraint behind this lookup.
_load_predecessor selects one admission by consumed_by_submission_id and status == "consumed". scalar() returns the first row and does not detect duplicates. The predecessor archive_sha256 and semantic_manifest_sha256 returned at lines 339-340 then feed evaluate_submission_change, so a wrong row would produce a wrong unchanged-work decision.
docs/spec_artifact_storage_service.md states that a unique Submission admission reference permits at most one consumer. Migration 0061_submission_bundle_admission.py does not implement it: the only unique constraints are uq_submission_bundle_admission_intent, uq_submission_bundle_admission_evidence, and uq_submission_bundle_admission_verification.
Nothing sets consumed_by_submission_id in this chunk, because consumption is out of scope for 04C2. The gap is therefore not reachable today. The 05 consumption chunk will depend on it. Add a partial unique index now, so the invariant exists before the first writer.
🛡️ Proposed constraint in migration 0061
for column in (
"pre_submit_evidence_set_id",
"artifact_content_id",
"actor_profile_id",
"project_id",
"task_id",
"status",
):
op.create_index(
f"ix_submission_bundle_admissions_{column}", "submission_bundle_admissions", [column]
)
+ op.create_index(
+ "uq_submission_bundle_admission_consumer",
+ "submission_bundle_admissions",
+ ["consumed_by_submission_id"],
+ unique=True,
+ postgresql_where=sa.text("consumed_by_submission_id is not null"),
+ )🤖 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_preparation.py` around lines 323 -
328, Add a partial unique index in migration 0061_submission_bundle_admission.py
enforcing uniqueness of consumed_by_submission_id for non-null values, so each
submission admission reference can have at most one consumer. Keep existing
constraints unchanged and ensure the migration includes the corresponding
downgrade removal.
dc5cf34 to
121f86c
Compare
1 participant
Chunk
WS-ART-001-04C2 — Ready Admission Publication
Goal
Publish one immutable, capacity-charged
readysubmission-bundle admission only after exact provider read-back verification, and compose the still-hidden continuous contributor preparation route.Human-approved intent
Contributor submissions remain one outer ZIP. ART owns safe bounded preparation, exact archive and semantic-manifest identity, effective pre-submit execution, immutable evidence, provider-neutral storage, read-back verification, and ready-admission publication. This chunk does not activate contributor authority or create a Submission.
What changed
0061_submission_admissionand immutableSubmissionBundleAdmissionlineage.POST /api/v1/tasks/{task_id}/submission-bundle-preparationscomposition.Why it changed
04C1 records checked durable storage intent, but a later consumer still needs a trusted fact proving that the exact intended ZIP was stored and independently verified. The ready admission is that fact and remains separate from later Submission creation.
Design chosen
The generic verifier invokes one narrow publisher only for a successful full-read verification receipt. Publication locks the durable intent with
FOR UPDATE, reloads the complete evidence/attempt/content/replica/receipt lineage, and inserts one immutable ready admission. The database repeats the critical lineage checks. Exact replay returns the existing operation/admission without another provider write or capacity charge.Alternatives rejected
Scope control
The route remains hidden and
artifact.submission_bundle.prepareremains unavailable. No AUTH catalogue/grant activation, public Submission creation, binding, review, contribution, compensation, retention, or legacy cutover is included.Product behavior
Verified matching content produces one
readyadmission. Client abandonment may leave a quota-charged ready admission. Later 05A alone may transitionready -> consumed|stale. Operators can see bounded unbound-ready and stale pressure without receiving artifact coordinates or deletion authority.Acceptance criteria proof
Tests/checks run
0059 -> 0060 -> 0061upgrade: passed against PostgreSQL.f22478cb0230f14e7ffd7c4494e519b54daff046e19ca6dfde9fcf53dc99a144).git diff --check: passed.Test delta
Added route, deny-before-read, authority cleanup, direct/observed publication, negative lineage, real concurrent publication, ready/stale projection, immutable update/delete, and Alembic schema coverage. No tests were removed or skipped.
CI integrity
No workflow, test runner, threshold, skip, or coverage weakening. Repository 78% and changed artifact/task subsystem 90% gates remain intact.
Reviewer results
External review
GitHub Actions and CodeRabbit are pending on this final head.
Remaining risks
Hosted PostgreSQL and coverage lanes remain authoritative. Before 05A introduces consumed/stale replay behavior, replay helpers should return the persisted admission status rather than assuming ready.
Follow-up work
After this merges, AUTH may implement the separately reviewed activation contract for
artifact.submission_bundle.prepare; ART then proceeds only in the approved cross-initiative order.Human review focus
Review the
0061immutable lineage/backfill guards, verifier transaction publication, concurrent idempotency, hidden route authorization ordering, and the absence of Submission/binding side effects.Human merge ownership
Only the human owner may approve and merge this PR.