feat(artifacts): persist effective pre-submit evidence - #291
Conversation
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe PR expands pre-submit processing to execute locked project policies, validate canonical artifact and evidence paths, reload task lineage, and persist immutable combined evidence with deterministic replay and storage custody facts. ChangesEffective pre-submit evidence
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant PreparedBundle
participant EffectiveProcessor
participant LockedContext
participant EvidenceService
participant PostgreSQL
PreparedBundle->>EffectiveProcessor: Execute locked platform and project policy
EffectiveProcessor-->>PreparedBundle: Return validated combined results
PreparedBundle->>LockedContext: Reload and lock task and policy lineage
LockedContext-->>PreparedBundle: Return validated custody context
PreparedBundle->>EvidenceService: Persist execution and custody facts
EvidenceService->>PostgreSQL: Insert or replay immutable evidence
PostgreSQL-->>EvidenceService: Return evidence identity and pass capability
Possibly related PRs
Suggested reviewers: 🚥 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: 5
🧹 Nitpick comments (10)
backend/app/modules/artifacts/submission_materialization.py (2)
91-99: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShare the allowed storage-scheme set instead of repeating the literal.
The set
{"local", "s3"}now appears here and invalidate_pre_submission_execution_resultinbackend/app/modules/checkers/pre_submit_execution.py. The migrationck_pre_submit_evidence_storage_schemecheck constraint repeats the same values. Export one module-level constant frompre_submit_executionand use it in both Python call sites. This prevents the two runtime validators from drifting apart when a provider is added.♻️ Proposed change
- if storage_scheme not in {"local", "s3"}: + if storage_scheme not in ALLOWED_STORAGE_SCHEMES: raise ValueError("pre-submit materializer storage scheme is invalid")Add the constant in
backend/app/modules/checkers/pre_submit_execution.py:ALLOWED_STORAGE_SCHEMES = frozenset({"local", "s3"})🤖 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 91 - 99, Define the shared module-level ALLOWED_STORAGE_SCHEMES constant in pre_submit_execution.py as the single allowed-value set, then import and use it in validate_pre_submission_execution_result and the SubmissionMaterializer initializer instead of duplicating {"local", "s3"}. Keep the existing validation behavior unchanged.
186-196: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCapture
generation_idbefore materialization runs.
generation_idis read at line 196, aftermaterialize_prepared_bundlereturned.ArtifactPreparedHandle.generation_idraisesRuntimeError("prepared artifact is closed")once the handle closes. The current flow works because the caller closes the handle afterexecutereturns. The ordering is an implicit dependency on preparation internals.Read
generation_idnext tocommitmentat line 187. This makes the persistence request independent of the handle lifetime.🛡️ Proposed change
- execution = await self._materialization.materialize_prepared_bundle(request) commitment = request.prepared_artifact.commitment + prepared_generation_id = request.prepared_artifact.generation_id + execution = await self._materialization.materialize_prepared_bundle(request) async with self._session.begin(): return await PreSubmitEvidenceService(self._session).persist( PreSubmitEvidencePersistenceRequest( @@ - prepared_generation_id=request.prepared_artifact.generation_id, + prepared_generation_id=prepared_generation_id,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/modules/artifacts/submission_materialization.py` around lines 186 - 196, Capture request.prepared_artifact.generation_id before awaiting self._materialization.materialize_prepared_bundle(request), alongside commitment in the execution flow. Use the captured value when constructing PreSubmitEvidencePersistenceRequest so persistence no longer reads generation_id after materialization may close the prepared artifact handle.backend/alembic/versions/0058_pre_submit_evidence.py (1)
303-323: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider binding membership to the insert transaction without depending on the
created_atdefault.The guard proves same-transaction membership by comparing
parent_created_atwithtransaction_timestamp(). This holds only while every writer leavescreated_atto thenow()server default._set_valuesinbackend/app/modules/artifacts/pre_submit_evidence.pycurrently omitscreated_at, so the invariant holds today. A later caller that supplies an explicitcreated_atwould silently close membership and make every result insert fail.Add a matching check constraint or a
before insertguard onpre_submit_evidence_setsthat rejects a caller-suppliedcreated_atdifferent fromtransaction_timestamp(). This keeps the membership rule self-enforcing.🤖 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/0058_pre_submit_evidence.py` around lines 303 - 323, Update the migration’s pre_submit_evidence_sets setup to enforce that inserted created_at values equal transaction_timestamp(), using a check constraint or BEFORE INSERT trigger. Anchor the change near guard_pre_submit_evidence_result_membership and ensure explicit timestamps from callers are rejected while the existing server-default path remains valid.backend/tests/test_effective_pre_submit_execution.py (1)
89-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the mismatch branch of
consume.The test proves single use. It does not prove fact binding.
PreSubmitPassCapability.consumerejects a call when any ofprepared_generation_id,predecessor_submission_id,effective_plan_sha256,archive_sha256,semantic_manifest_sha256, orstorage_schemediffers. The test name states the capability is "generation-bound", but no assertion exercises a mismatched generation.Add one negative case on a fresh capability so the mismatch branch is proven independently of the consumed flag.
💚 Proposed additional negative case
with pytest.raises(PreSubmitEvidenceConflict, match="pre_submit_pass_capability_invalid"): capability.consume( prepared_generation_id=generation_id, predecessor_submission_id=None, effective_plan_sha256=_sha("7"), archive_sha256=_sha("1"), semantic_manifest_sha256=_sha("2"), storage_scheme="s3", ) + + unconsumed = PreSubmitPassCapability( + evidence_set_id=uuid4(), + prepared_generation_id=generation_id, + predecessor_submission_id=None, + effective_plan_sha256=_sha("7"), + archive_sha256=_sha("1"), + semantic_manifest_sha256=_sha("2"), + storage_scheme="s3", + ) + with pytest.raises(PreSubmitEvidenceConflict, match="pre_submit_pass_capability_invalid"): + unconsumed.consume( + prepared_generation_id=uuid4(), + predecessor_submission_id=None, + effective_plan_sha256=_sha("7"), + archive_sha256=_sha("1"), + semantic_manifest_sha256=_sha("2"), + storage_scheme="s3", + )🤖 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 89 - 121, Extend test_pass_capability_is_generation_bound_and_single_use with a fresh PreSubmitPassCapability and assert consume raises PreSubmitEvidenceConflict when prepared_generation_id differs, while keeping the other facts matching; retain the existing successful consumption and second-use assertions to continue covering single-use behavior independently.backend/app/modules/artifacts/models.py (1)
239-242: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winRequire
failure_codefor non-passing statuses.
statusacceptsfailedwhilefailure_codestays nullable and unconstrained. Afailedresult can therefore persist with no stable code. The chunk contract requires thepre_submission_check_failedaudit projection to carry stable codes, so this row shape produces an incomplete audit record that immutability then makes permanent.Add a shaped check constraint so the database enforces the pairing.
🛡️ Proposed constraint
CheckConstraint( "status in ('passed','warning','advisory_disabled','dependency_not_run','failed')", name="ck_pre_submit_result_status", ), + CheckConstraint( + "(status = 'failed' and failure_code is not null) or " + "(status <> 'failed' and failure_code is null)", + name="ck_pre_submit_result_failure_code_shape", + ),Also applies to: 292-293
🤖 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 239 - 242, Update the constraints around the pre-submit result status in the model definition, including the corresponding constraint at the other referenced location, to enforce that non-passing statuses require a non-null failure_code while passing status permits the existing nullable behavior. Add a shaped database check constraint covering the status/failure_code pairing alongside the existing status constraint.backend/app/modules/checkers/pre_submit_execution.py (3)
439-468: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an explicit fail-closed branch for an unmapped policy primitive.
The
if/elifchain covers all twelve currentPreSubmissionPolicyPrimitivemembers, so the dispatch is correct today. The chain has no finalelse.If a maintainer adds an enum member to
PreSubmissionPolicyPrimitivewithout a branch here, control reaches Line 468 withfailure_count = 0andmessage_code = "passed"._blocking_or_passthen returnsPASSED, and the submission gate reports the new policy check as satisfied. The default direction is fail-open on a gating decision.Add a terminal
elsethat raises, so an unmapped primitive fails closed.🛡️ Proposed fail-closed default
elif primitive is PreSubmissionPolicyPrimitive.WARN_LOW_QUALITY_GENERATED_ARTIFACT: matches = matched_low_quality_patterns( (self._input.packet.summary, self._input.packet.contributor_attestation, *paths) ) return self._result( entry, PreSubmissionResultStatus.WARNING if matches else PreSubmissionResultStatus.PASSED, message_code="quality_signal_warning" if matches else "passed", metadata=(("matched_category_count", len(matches)),) if matches else (), ) + else: + raise PreSubmissionInfrastructureUnavailable( + "pre_submission_policy_primitive_unknown" + ) return self._blocking_or_pass(entry, failure_count, message_code)🤖 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/checkers/pre_submit_execution.py` around lines 439 - 468, Add a terminal else branch to the primitive dispatch chain in the policy evaluation method, after the WARN_LOW_QUALITY_GENERATED_ARTIFACT branch and before _blocking_or_pass, that raises the existing infrastructure/configuration exception for an unmapped PreSubmissionPolicyPrimitive. Ensure unsupported future enum members cannot fall through with the default passed result.
143-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNarrow the
metadatavalue type toint.The annotation permits
int | bool | str.validate_pre_submission_execution_result(Line 609) requirestype(value) is not intto fail, so it rejects everystrvalue and everyboolvalue.type(True) is intisFalse.The annotation therefore advertises value types that always fail validation.
_result(Line 529) repeats the same wide annotation. Narrow both totuple[tuple[str, int], ...]so the declared type matches the enforced contract.♻️ Proposed annotation change
- metadata: tuple[tuple[str, int | bool | str], ...] = () + metadata: tuple[tuple[str, int], ...] = ()Apply the same change to the
metadataparameter of_resultat Line 529.🤖 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/checkers/pre_submit_execution.py` at line 143, Update the metadata annotation at the shown declaration and the metadata parameter in _result to tuple[tuple[str, int], ...]. Keep validate_pre_submission_execution_result unchanged so the declared types match its existing strict integer-only validation.
532-533: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBind
schema_versionto the plan entry, not to the module constant.
_resultalways writesPRE_SUBMISSION_RESULT_SCHEMA_VERSION.validate_pre_submission_execution_result(Line 585) compares the producedschema_versionagainst the per-entryplan_entry.result_schema.The two agree only while every catalogue definition declares the same
result_schemaas the module constant. If one definition ever versions its result schema, every result in that plan fails validation withpre_submission_result_context_invalid, and the whole pre-submit execution fails closed.Read the value from the plan entry so the producer honors the per-entry field the validator enforces.
♻️ Proposed change
return PreSubmissionEntryResult( - schema_version=PRE_SUBMISSION_RESULT_SCHEMA_VERSION, + schema_version=entry.result_schema,🤖 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/checkers/pre_submit_execution.py` around lines 532 - 533, Update the result construction in _result to set schema_version from the current plan entry’s result_schema instead of PRE_SUBMISSION_RESULT_SCHEMA_VERSION, matching the per-entry value enforced by validate_pre_submission_execution_result.backend/app/modules/checkers/compiler.py (1)
605-613: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOne canonical relative-path rule is expressed twice. The compiler writes the policy paths into the compiled bundle, and the executor revalidates the same paths before comparing them to sealed-tree entries. Both sides state the canonical-path rule separately, so the two copies must stay in agreement. If one copy changes, a bundle can pass compilation and then fail execution with
pre_submission_policy_path_unmappable.
backend/app/modules/checkers/compiler.py#L605-L613: extract_is_canonical_relative_pathinto one shared helper and call it here.backend/app/modules/checkers/pre_submit_execution.py#L481-L492: replace the inline canonical checks in_canonical_policy_pathswith a call to that shared helper, and keep the duplicate-path check local.🤖 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/checkers/compiler.py` around lines 605 - 613, The canonical relative-path validation is duplicated between compiler and executor; extract one shared helper and reuse it in both locations. In backend/app/modules/checkers/compiler.py lines 605-613, move _is_canonical_relative_path to the shared checker utility and call it from compilation. In backend/app/modules/checkers/pre_submit_execution.py lines 481-492, replace the inline checks in _canonical_policy_paths with that helper while keeping duplicate-path detection local.backend/app/modules/artifacts/pre_submit_evidence.py (1)
311-326: 🗄️ Data Integrity & Integration | 🔵 TrivialExplicitly keep the replay path on PostgreSQL READ COMMITTED.
If this PostgreSQL server is configured with
default_transaction_isolation = REPEATABLE READorSERIALIZABLE, the follow-upSELECTafteron_conflict_do_nothingdoes not see the committed row and raisespre_submit_evidence_operation_conflictfor an exact replay. Set the per-session isolation explicitly if the server default cannot be guaranteed.🤖 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 311 - 326, Update the replay handling around the `inserted_id` check in the pre-submit evidence method to execute on a PostgreSQL session explicitly configured for READ COMMITTED. Ensure the follow-up `select(PreSubmitEvidenceSet)` sees the committed conflicting row even when the server default is REPEATABLE READ or SERIALIZABLE, while preserving the existing conflict validation and exception behavior.
🤖 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/models.py`:
- Around line 198-206: Update the artifact model’s guide binding around
guide_id, guide_version, and source_snapshot_id so the database enforces that
guide_id belongs to the same project_id and guide_version lineage. Add a
composite foreign-key relationship or reuse an appropriate composite unique
target from the existing guide lineage constraints, ensuring mismatched
guide/version combinations cannot be persisted.
In `@backend/app/modules/artifacts/submission_materialization.py`:
- Around line 177-189: Clarify the transaction contract of
PreparedBundlePreSubmitEvidenceService.execute: callers must invoke it with no
active transaction so self._session.begin() can establish the persistence
transaction after materialization. Document this precondition in the service or
move the persistence work into the caller’s existing transactional boundary,
ensuring the implementation does not attempt nested begin calls.
In `@backend/app/modules/checkers/compiler.py`:
- Around line 616-630: Update _required_evidence_path to reject the special
evidence keys "." and ".." before returning the projected path, while preserving
the existing validation for empty or invalid characters. Raise
PreSubmitCheckerCompilerError for these non-canonical keys so invalid evidence
paths fail during compilation.
In `@backend/app/modules/tasks/pre_submit_context.py`:
- Around line 110-117: Update the guide query in the pre-submit context flow to
filter ProjectGuide.id by task.locked_guide_id in addition to the existing
project and version predicates. Also extend the earlier required-lineage
null-check to fail closed when task.locked_guide_id is None, keeping the
effective-policy and checker-policy lineage checks unchanged.
In `@docs/spec_artifact_storage_service.md`:
- Around line 1227-1230: Clarify the passing evidence-set contract in the
documented section: an exact replay returns the existing durable evidence set
without a pass capability, so ART-04C must not consume one from the replay and
must require re-preparation before continuing. Preserve the existing capability
binding and immediate-consumption requirements for non-replayed eligible
evidence.
---
Nitpick comments:
In `@backend/alembic/versions/0058_pre_submit_evidence.py`:
- Around line 303-323: Update the migration’s pre_submit_evidence_sets setup to
enforce that inserted created_at values equal transaction_timestamp(), using a
check constraint or BEFORE INSERT trigger. Anchor the change near
guard_pre_submit_evidence_result_membership and ensure explicit timestamps from
callers are rejected while the existing server-default path remains valid.
In `@backend/app/modules/artifacts/models.py`:
- Around line 239-242: Update the constraints around the pre-submit result
status in the model definition, including the corresponding constraint at the
other referenced location, to enforce that non-passing statuses require a
non-null failure_code while passing status permits the existing nullable
behavior. Add a shaped database check constraint covering the
status/failure_code pairing alongside the existing status constraint.
In `@backend/app/modules/artifacts/pre_submit_evidence.py`:
- Around line 311-326: Update the replay handling around the `inserted_id` check
in the pre-submit evidence method to execute on a PostgreSQL session explicitly
configured for READ COMMITTED. Ensure the follow-up
`select(PreSubmitEvidenceSet)` sees the committed conflicting row even when the
server default is REPEATABLE READ or SERIALIZABLE, while preserving the existing
conflict validation and exception behavior.
In `@backend/app/modules/artifacts/submission_materialization.py`:
- Around line 91-99: Define the shared module-level ALLOWED_STORAGE_SCHEMES
constant in pre_submit_execution.py as the single allowed-value set, then import
and use it in validate_pre_submission_execution_result and the
SubmissionMaterializer initializer instead of duplicating {"local", "s3"}. Keep
the existing validation behavior unchanged.
- Around line 186-196: Capture request.prepared_artifact.generation_id before
awaiting self._materialization.materialize_prepared_bundle(request), alongside
commitment in the execution flow. Use the captured value when constructing
PreSubmitEvidencePersistenceRequest so persistence no longer reads generation_id
after materialization may close the prepared artifact handle.
In `@backend/app/modules/checkers/compiler.py`:
- Around line 605-613: The canonical relative-path validation is duplicated
between compiler and executor; extract one shared helper and reuse it in both
locations. In backend/app/modules/checkers/compiler.py lines 605-613, move
_is_canonical_relative_path to the shared checker utility and call it from
compilation. In backend/app/modules/checkers/pre_submit_execution.py lines
481-492, replace the inline checks in _canonical_policy_paths with that helper
while keeping duplicate-path detection local.
In `@backend/app/modules/checkers/pre_submit_execution.py`:
- Around line 439-468: Add a terminal else branch to the primitive dispatch
chain in the policy evaluation method, after the
WARN_LOW_QUALITY_GENERATED_ARTIFACT branch and before _blocking_or_pass, that
raises the existing infrastructure/configuration exception for an unmapped
PreSubmissionPolicyPrimitive. Ensure unsupported future enum members cannot fall
through with the default passed result.
- Line 143: Update the metadata annotation at the shown declaration and the
metadata parameter in _result to tuple[tuple[str, int], ...]. Keep
validate_pre_submission_execution_result unchanged so the declared types match
its existing strict integer-only validation.
- Around line 532-533: Update the result construction in _result to set
schema_version from the current plan entry’s result_schema instead of
PRE_SUBMISSION_RESULT_SCHEMA_VERSION, matching the per-entry value enforced by
validate_pre_submission_execution_result.
In `@backend/tests/test_effective_pre_submit_execution.py`:
- Around line 89-121: Extend
test_pass_capability_is_generation_bound_and_single_use with a fresh
PreSubmitPassCapability and assert consume raises PreSubmitEvidenceConflict when
prepared_generation_id differs, while keeping the other facts matching; retain
the existing successful consumption and second-use assertions to continue
covering single-use behavior independently.
🪄 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: 1d7f1f9b-b284-40df-af3b-57582504052d
📒 Files selected for processing (30)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/AUTH_HANDOFF.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/CHUNK_MAP.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.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-04B3-effective-pre-submit-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B3-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B3-pr-trust-bundle.md.agent-loop/initiatives/WS-XINT-002-art-auth-end-to-end/chunks/WS-XINT-002-06A-pre-submit-materialization-activation.mdbackend/alembic/versions/0058_pre_submit_evidence.pybackend/app/db/models.pybackend/app/interfaces/artifact_operations.pybackend/app/modules/actors/models.pybackend/app/modules/artifacts/models.pybackend/app/modules/artifacts/pre_submit_evidence.pybackend/app/modules/artifacts/submission_materialization.pybackend/app/modules/checkers/compiler.pybackend/app/modules/checkers/pre_submit_execution.pybackend/app/modules/tasks/models.pybackend/app/modules/tasks/pre_submit_context.pybackend/scripts/run_test_lanes.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_checker_catalogue.pybackend/tests/test_ci_test_lanes.pybackend/tests/test_default_pre_submit_execution.pybackend/tests/test_effective_pre_submit_execution.pydocs/architecture_data_model.mddocs/architecture_lockdown.mddocs/glossary.mddocs/spec_artifact_storage_service.md
| guide = await session.scalar( | ||
| select(ProjectGuide) | ||
| .where( | ||
| ProjectGuide.project_id == task.project_id, | ||
| ProjectGuide.version == task.locked_guide_version, | ||
| ) | ||
| .with_for_update() | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Bind the guide query to task.locked_guide_id.
The guide query filters on project_id and locked_guide_version only. The task row stores locked_guide_id, which names the exact locked guide row. The effective-policy query (Lines 118-130) and the checker-policy query (Lines 131-146) both bind their id column explicitly. The guide query is the only lineage read that does not.
The current form depends on (project_id, version) being unique on project_guides. If two rows ever share that pair, scalar() returns an arbitrary row or raises on multiple results. PreSubmitEvidenceService.persist in backend/app/modules/artifacts/pre_submit_evidence.py (Line 467) then compares locked.guide_id against the plan lineage and fails closed, so wrong lineage is not persisted. The failure surfaces as a late conflict instead of a precise lookup.
Add the locked_guide_id predicate so this read matches the locked contract directly.
🛡️ Proposed fix to bind the exact locked guide row
guide = await session.scalar(
select(ProjectGuide)
.where(
+ ProjectGuide.id == task.locked_guide_id,
ProjectGuide.project_id == task.project_id,
ProjectGuide.version == task.locked_guide_version,
)
.with_for_update()
)Add or task.locked_guide_id is None to the null-check block at Lines 89-95 so a missing locked guide ID also fails closed.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| guide = await session.scalar( | |
| select(ProjectGuide) | |
| .where( | |
| ProjectGuide.project_id == task.project_id, | |
| ProjectGuide.version == task.locked_guide_version, | |
| ) | |
| .with_for_update() | |
| ) | |
| guide = await session.scalar( | |
| select(ProjectGuide) | |
| .where( | |
| ProjectGuide.id == task.locked_guide_id, | |
| ProjectGuide.project_id == task.project_id, | |
| ProjectGuide.version == task.locked_guide_version, | |
| ) | |
| .with_for_update() | |
| ) |
🤖 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/tasks/pre_submit_context.py` around lines 110 - 117,
Update the guide query in the pre-submit context flow to filter ProjectGuide.id
by task.locked_guide_id in addition to the existing project and version
predicates. Also extend the earlier required-lineage null-check to fail closed
when task.locked_guide_id is None, keeping the effective-policy and
checker-policy lineage checks unchanged.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/test_default_pre_submit_execution.py (1)
525-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest the evidence-set guards without a foreign-key failure.
Lines 526-528 target an evidence set that already has result rows. PostgreSQL can reject
DELETEandTRUNCATEthroughpre_submit_evidence_results.evidence_set_idbefore the evidence-set immutability guard runs.Create an otherwise valid evidence set with no result rows for the delete case. Use
TRUNCATE pre_submit_evidence_sets CASCADEfor the truncate case. Assert the immutable-trigger error in both cases.🤖 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 525 - 543, Update the immutable_statements test around the evidence-set mutations so each guard is reached without foreign-key interference: create a valid evidence set with no result rows for the DELETE case, and use TRUNCATE pre_submit_evidence_sets CASCADE for the truncate case. Assert the immutable-trigger DBAPIError for both cases while preserving the existing checks for result-table mutations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/tests/test_default_pre_submit_execution.py`:
- Around line 525-543: Update the immutable_statements test around the
evidence-set mutations so each guard is reached without foreign-key
interference: create a valid evidence set with no result rows for the DELETE
case, and use TRUNCATE pre_submit_evidence_sets CASCADE for the truncate case.
Assert the immutable-trigger DBAPIError for both cases while preserving the
existing checks for result-table mutations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bb79f7e-67c2-4f7e-8e03-d370ac5fad5f
📒 Files selected for processing (17)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B3-external-review-response.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B3-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B3-pr-trust-bundle.mdbackend/alembic/versions/0058_pre_submit_evidence.pybackend/app/modules/artifacts/models.pybackend/app/modules/artifacts/pre_submit_evidence.pybackend/app/modules/artifacts/submission_materialization.pybackend/app/modules/checkers/compiler.pybackend/app/modules/checkers/pre_submit_defaults.pybackend/app/modules/checkers/pre_submit_execution.pybackend/app/modules/projects/models.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_default_pre_submit_execution.pybackend/tests/test_effective_pre_submit_execution.pydocs/architecture_data_model.mddocs/spec_artifact_storage_service.md
🚧 Files skipped from review as they are similar to previous changes (11)
- docs/spec_artifact_storage_service.md
- backend/tests/test_effective_pre_submit_execution.py
- backend/tests/conftest.py
- backend/app/modules/checkers/compiler.py
- backend/app/modules/artifacts/models.py
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-04B3-pr-trust-bundle.md
- docs/architecture_data_model.md
- backend/app/modules/artifacts/pre_submit_evidence.py
- backend/app/modules/artifacts/submission_materialization.py
- backend/tests/test_alembic.py
- backend/app/modules/checkers/pre_submit_execution.py
2 participants
WS-ART-001-04B3 PR Trust Bundle
Chunk
WS-ART-001-04B3— effective pre-submit execution evidence.Goal
Execute the exact task-locked project-policy entries through the same sealed
04B1/04B2 plan and persist one bounded immutable evidence set for the complete
platform-plus-project result.
What changed
primitives through the existing catalogue and effective plan.
service.
failure audit, and a non-replayable process-local pass capability.
0058_pre_submit_evidencewith composite lineage constraints.Scope control
No public route, provider I/O, ArtifactContent, admission, Submission, review,
contribution, compensation, reputation, AUTH activation, generic download, or
legacy removal is introduced.
Acceptance proof
Tests prove real locked actor/identity/task/assignment/guide/policy context,
passing persistence, exact replay, blocked persistence, bounded path-free audit,
single-use capability behavior, immutable aggregate membership, forged-envelope
rejection, scratch cleanup, and unchanged artifact/admission/Submission/checker/
review-queue side-effect tables.
CI integrity
No workflow or coverage floor was weakened. The new effective-execution module
is owned by the existing task-lifecycle semantic lane. Repository coverage stays
at 78 percent and the existing 90 percent subsystem gates remain intact.
Reviewer results
Architecture, security, QA, product/ops, senior engineering, docs, reuse, and
test-delta passed. CI integrity passed with only the expected requirement that
the committed branch be evaluated by hosted checks.
Remaining risks and follow-up
The returned pass capability is intentionally process-local and cannot be
recreated by replay. ART-04C must consume it while freshly revalidating the
complete crossed-state matrix before any durable put, provider write, admission,
or Submission effect. AUTH activation and legacy cutover remain later ordered
chunks.
Human review focus
Human merge ownership
Only the repository owner may approve and merge this PR.
Summary by CodeRabbit
New Features
Documentation
Tests