WS-ART-001-03C: cut over verified guide sources - #249
Conversation
|
Warning Review limit reached
Next review available in: 6 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (5)
📝 WalkthroughWalkthroughThe PR moves guide-source snapshots to server-owned schema-v2 identity and verified ART content. Setup generation waits for same-generation verification, then resumes through durable continuation and recovery workflows. Legacy sufficiency execution and caller-owned content identity are removed. ChangesGuide source cutover
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant ArtifactVerification
participant GuideContinuation
participant GuideSetupPreparation
participant ProjectSetupQueue
participant CeleryBeat
ArtifactVerification->>GuideContinuation: publish verified job continuation
GuideContinuation->>GuideSetupPreparation: prepare same-generation setup
GuideSetupPreparation->>ProjectSetupQueue: dispatch prepared setup run
CeleryBeat->>GuideContinuation: scan stranded continuations
GuideContinuation->>ProjectSetupQueue: republish retryable setup work
🚥 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/check_stale_artifact_contracts.py (1)
358-360: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject all removed guide-source authority fields.
This rule matches only
content_cid. It does not reject the removeddurable_refandcontent_hashfields. A future change can restore caller-owned guide identity while this gate passes.Add path-scoped stale rules for the removed v1 fields. Add one regression test per field.
🤖 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 `@scripts/check_stale_artifact_contracts.py` around lines 358 - 360, Extend the stale-artifact contract rules near LEGACY_GUIDE_CONTENT_CID in the guide_source_cutover scope to reject both removed v1 authority fields, durable_ref and content_hash, using path-scoped patterns. Add a separate regression test for each field and preserve the existing content_cid rejection.backend/scripts/api_contract_e2e.py (1)
1019-1043: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winReuse the setup run created with the source snapshot.
Creating a v2 source snapshot records its queued setup run. Both helpers add another run with
setup_generation=1. The unique guide-generation constraint rejects the commit and breaks the E2E or task fixture.
backend/scripts/api_contract_e2e.py#L1019-L1043: Load the existing setup run forsource_snapshot["id"]and update its post-submit output fields instead of insertingProjectSetupRun.backend/tests/test_tasks.py#L1051-L1066: Remove the manual generation-1 insert. Load the snapshot-created run when the fixture needs to set later setup outputs.🤖 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/scripts/api_contract_e2e.py` around lines 1019 - 1043, The setup run created with the source snapshot must be reused instead of inserting a duplicate generation-1 run. In backend/scripts/api_contract_e2e.py lines 1019-1043, load the existing ProjectSetupRun for source_snapshot["id"], update its post-submit output fields and derivation summary, and stop adding a new setup_run. In backend/tests/test_tasks.py lines 1051-1066, remove the manual generation-1 ProjectSetupRun insertion and load the snapshot-created run when setting later setup outputs.
🧹 Nitpick comments (12)
backend/alembic/versions/0048_guide_source_v2_cutover.py (1)
143-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDowngrade re-adds
content_hashas NOT NULL without a default.This is safe only because
_refuse_populatedruns first and both tables must be empty. The dependency is implicit. Add a short comment that documents the ordering requirement so a later edit does not move or remove the refusal.🤖 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/0048_guide_source_v2_cutover.py` around lines 143 - 246, The downgrade’s content_hash re-addition depends on _refuse_populated running first while both guide-source tables are empty. Add a concise comment immediately before _refuse_populated in downgrade() documenting that this refusal must remain before the NOT NULL legacy columns are restored.backend/app/modules/projects/service.py (2)
2738-2765: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
_verified_agent_iteminrun_verified_guide_sufficiency_agent.The local
agent_itemclosure at lines 701-726 buildsGuideSourceItemMaterialwith the same field-for-field mapping as this new static method. Two copies of the same projection can drift when the material contract changes. Callself._verified_agent_itemfromrun_verified_guide_sufficiency_agentand delete the closure.♻️ Proposed change at lines 701-726
- def agent_item(item) -> GuideSourceItemMaterial: - return GuideSourceItemMaterial( - source_kind=item.source_kind, - ... - untrusted_data_label="UNTRUSTED_GUIDE_SOURCE_DATA", - ) + agent_item = self._verified_agent_item🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/modules/projects/service.py` around lines 2738 - 2765, Update run_verified_guide_sufficiency_agent to call the existing _verified_agent_item method for GuideSourceItemMaterial projection, passing each canonical extraction row directly to it. Remove the local agent_item closure and preserve the current agent input behavior through the shared mapping.
3300-3348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the usage query into
ProjectRepository.
ProjectServicedocuments that repositories own database access. This method issuesselect()andfunc.count()directly againstself._session. Add a repository method, for examplelist_report_source_usages(report_id, setup_run_id, setup_generation, snapshot_id)andcount_snapshot_items(snapshot_id), then keep the count and ordering assertions in the service.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/modules/projects/service.py` around lines 3300 - 3348, Move the database queries from ProjectService._verified_report_usages into ProjectRepository methods such as list_report_source_usages and count_snapshot_items, passing the report, setup run, generation, and snapshot identifiers as arguments. Update _verified_report_usages to call those repository methods, while retaining the existing completeness, uniqueness, ordering validations and PolicySetupBlocked behavior in the service.backend/app/modules/artifacts/repository.py (1)
103-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the new method below
__init__.
get_verified_guide_content_candidateis defined between the class docstring and__init__at line 154. Every other method inArtifactRepositoryfollows the constructor. Move the method after__init__to keep the class layout consistent.🤖 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/repository.py` around lines 103 - 153, Move the get_verified_guide_content_candidate method to immediately after ArtifactRepository.__init__, keeping its implementation unchanged and preserving the existing ordering of all other methods.backend/app/modules/artifacts/guide_materialization.py (2)
559-559: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the
requestparameter.
materialize_with_fresh_authorityaccepts an untypedrequest. The coordinator passes aGuideExtractionRequest. Add the annotation underTYPE_CHECKINGto avoid an import cycle withguide_extraction_service.♻️ Proposed annotation
- async def materialize_with_fresh_authority(self, request) -> PreparedArtifact: + async def materialize_with_fresh_authority( + self, request: GuideExtractionRequest + ) -> PreparedArtifact:Add near the top of the module:
from typing import TYPE_CHECKING if TYPE_CHECKING: from app.modules.artifacts.guide_extraction_service import GuideExtractionRequest🤖 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/guide_materialization.py` at line 559, Annotate the request parameter of materialize_with_fresh_authority with GuideExtractionRequest, and add the corresponding import under a TYPE_CHECKING guard so the runtime import cycle with guide_extraction_service is avoided.
258-293: 🚀 Performance & Scalability | 🔵 TrivialRow locks are held across the provider read.
_load_read_factsuseswith_for_updateon the guide, snapshot, setup run, binding, content, and replica rows._authorize_and_preparethen streams the whole provider object into scratch inside the same transaction. The locks therefore stay held for the full object read.This path is now executed once per classification and up to twice per extraction attempt, so lock hold time scales with artifact size and provider latency. Consider recording the facts, releasing the transaction, performing the read, and then revalidating under a short second transaction. The post-read
_load_read_factscomparison at line 273 already provides the generation-drift check that such a design needs.🤖 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/guide_materialization.py` around lines 258 - 293, Refactor prepare_authorized_guide_source so _load_read_facts and authorization occur in a short transaction, then commit before _authorize_and_prepare performs the provider read; open a second transaction afterward to call _load_read_facts and compare against the original facts. Preserve incident recording and cleanup behavior, while ensuring row locks are not held during the provider stream.backend/app/modules/artifacts/guide_extraction_service.py (1)
522-524: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
exhaustedto match the returned value.
claim_materialization_slotreturns a replayed result, a terminal attempt result, or a budget-exhausted result. The nameexhausteddescribes only one of these cases.♻️ Proposed rename
- exhausted = await self._service.claim_materialization_slot(request) - if exhausted is not None: - return exhausted + settled = await self._service.claim_materialization_slot(request) + if settled is not None: + return settled🤖 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/guide_extraction_service.py` around lines 522 - 524, Rename the local variable exhausted in the materialization flow to a neutral name describing the value returned by claim_materialization_slot, such as slot_result, and update the subsequent None check and return to use the renamed variable.backend/app/modules/artifacts/guide_setup.py (1)
130-143: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueThe
FOR UPDATElock is released before the candidate is used.
get_verified_guide_content_candidateissuesSELECT ... FOR UPDATE OF artifact_replicas. This session has no explicit transaction block, so the implicit transaction ends when theasync withexits and the row lock is released at line 134. The candidate is then used at lines 155-166 to build authority facts outside any lock.The result is safe, because
bind_guide_sourcere-selects the same candidate under the binding transaction and fails closed on a mismatch. The lock here therefore adds contention without adding a guarantee. Consider adding a non-locking read variant for this pre-check, or document that this call is only an eligibility probe.🤖 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/guide_setup.py` around lines 130 - 143, The _verified_item method currently obtains a FOR UPDATE lock through get_verified_guide_content_candidate even though the session closes before the candidate is used. Add or reuse a non-locking read variant for this eligibility pre-check, and update _verified_item to call it while preserving the existing candidate validation and _VerifiedItem construction.backend/app/modules/projects/guide_setup_continuation.py (1)
20-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a docstring and return type to
_retryable_dispatch_predicate.The other functions in this module have docstrings. This helper returns a SQLAlchemy boolean clause that encodes the recovery contract shared with
dispatch_pre_submit_setup_pipeline_after_commit. State that contract here so the two definitions stay aligned.♻️ Proposed change
-def _retryable_dispatch_predicate(): +def _retryable_dispatch_predicate() -> ColumnElement[bool]: + """Match runs that dispatch may claim: stale pending or unclaimed queued work.""" return or_(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/modules/projects/guide_setup_continuation.py` around lines 20 - 30, Add a docstring and explicit SQLAlchemy boolean-clause return type to `_retryable_dispatch_predicate`, documenting the recovery contract it shares with `dispatch_pre_submit_setup_pipeline_after_commit`; keep the existing predicate conditions unchanged and ensure the annotation matches the clause returned by `or_`.backend/tests/test_projects.py (1)
6304-6320: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused
expected_detailparameter or assert it.Line 6319 deletes
expected_detailand line 6320 asserts one generic message for every case. The parametrizedexpected_detailvalues now carry no coverage. Either assert the per-case detail or drop the parameter from the parametrize tuple.🤖 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_projects.py` around lines 6304 - 6320, Update test_source_snapshot_rejects_credential_and_local_refs to use the parametrized expected_detail value in its assertion, verifying each case’s specific response detail; alternatively remove expected_detail from the test signature and parametrization if only the shared message is intended.backend/app/workers/celery_app.py (1)
84-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a dedicated interval setting for the continuation scan.
The guide-setup continuation scan reuses
artifact_pending_work_scan_interval_seconds. The two scans query different tables and have different cost. A separate setting lets operators tune the continuation scan without changing artifact pending-work behavior. This is optional for v0.1.🤖 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/workers/celery_app.py` around lines 84 - 87, Update the schedule entry identified by GUIDE_SETUP_CONTINUATION_SCAN_SCHEDULE to use a dedicated continuation-scan interval setting instead of settings.artifact_pending_work_scan_interval_seconds, and add or reference the corresponding configuration key through the existing settings mechanism while preserving the current task registration.backend/app/workers/project_setup.py (1)
116-130: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
_run_pre_submit_setup_pipelineis now a pure pass-through.The function forwards every argument to
_run_verified_pre_submit_sufficiency_continuationand adds no behavior. Keep it only if the Celery task and existing tests must retain this name. Otherwise call the continuation directly and delete the wrapper.🤖 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/workers/project_setup.py` around lines 116 - 130, Remove the redundant _run_pre_submit_setup_pipeline pass-through and update its callers to invoke _run_verified_pre_submit_sufficiency_continuation directly, unless the Celery task or existing tests require the wrapper’s public name; preserve all argument forwarding and 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
@.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03C-internal-review-evidence.md:
- Around line 8-21: Update the “Final reviewer results” evidence in the reviewed
artifact to include the reviewed commit, executed commands or test identifiers,
and hosted CI run details. After HOSTED Backend/Agent Gates completes, change
the CI integrity status from execution-only pass to the completed result and
record Backend run 30767889658 with its outcome.
In `@backend/alembic/versions/0048_guide_source_v2_cutover.py`:
- Around line 95-108: Update the constraint operations in the migration’s status
cutover to use the physical PostgreSQL name
ck_project_setup_runs_ck_project_setup_runs_status when dropping the existing
check. Ensure the replacement created by create_check_constraint is marked with
the physical name, using the established Alembic naming convention mechanism
such as op.f, so it is not prefixed again.
In `@backend/app/adapters/artifacts/internal_workers.py`:
- Around line 222-269: Update scan_guide_setup_continuations to route eligible
terminal job IDs directly to continue_guide_setup instead of
publish_verification_job, bypassing verification replay and duplicate receipts.
Keep the retryable snapshot page size independent from the terminal-continuation
job limit by using a dedicated configuration value or pagination strategy for
job IDs. Preserve the existing verified-terminal filtering and ordering.
In `@docs/decision_0011_submission_artifact_policy_drives_pre_submit.md`:
- Around line 49-50: Update the duplicate-rejection statement in the decision
record to describe the implemented mechanism:
build_guide_source_snapshot_manifest rejects duplicate (source_kind,
source_label) pairs before hashing, while item_id and item_order are assigned
there and duplicate ids/orders are enforced later by
validate_source_snapshot_integrity and the database constraints. Remove the
inaccurate claim that server-owned ids or orders are rejected before hashing.
In `@docs/spec_artifact_storage_service.md`:
- Around line 1551-1555: Move the 0048_guide_source_v2 migration bullet to
immediately after the 0042_guide_extraction bullet, preserving its existing
content and the list’s ascending revision order.
---
Outside diff comments:
In `@backend/scripts/api_contract_e2e.py`:
- Around line 1019-1043: The setup run created with the source snapshot must be
reused instead of inserting a duplicate generation-1 run. In
backend/scripts/api_contract_e2e.py lines 1019-1043, load the existing
ProjectSetupRun for source_snapshot["id"], update its post-submit output fields
and derivation summary, and stop adding a new setup_run. In
backend/tests/test_tasks.py lines 1051-1066, remove the manual generation-1
ProjectSetupRun insertion and load the snapshot-created run when setting later
setup outputs.
In `@scripts/check_stale_artifact_contracts.py`:
- Around line 358-360: Extend the stale-artifact contract rules near
LEGACY_GUIDE_CONTENT_CID in the guide_source_cutover scope to reject both
removed v1 authority fields, durable_ref and content_hash, using path-scoped
patterns. Add a separate regression test for each field and preserve the
existing content_cid rejection.
---
Nitpick comments:
In `@backend/alembic/versions/0048_guide_source_v2_cutover.py`:
- Around line 143-246: The downgrade’s content_hash re-addition depends on
_refuse_populated running first while both guide-source tables are empty. Add a
concise comment immediately before _refuse_populated in downgrade() documenting
that this refusal must remain before the NOT NULL legacy columns are restored.
In `@backend/app/modules/artifacts/guide_extraction_service.py`:
- Around line 522-524: Rename the local variable exhausted in the
materialization flow to a neutral name describing the value returned by
claim_materialization_slot, such as slot_result, and update the subsequent None
check and return to use the renamed variable.
In `@backend/app/modules/artifacts/guide_materialization.py`:
- Line 559: Annotate the request parameter of materialize_with_fresh_authority
with GuideExtractionRequest, and add the corresponding import under a
TYPE_CHECKING guard so the runtime import cycle with guide_extraction_service is
avoided.
- Around line 258-293: Refactor prepare_authorized_guide_source so
_load_read_facts and authorization occur in a short transaction, then commit
before _authorize_and_prepare performs the provider read; open a second
transaction afterward to call _load_read_facts and compare against the original
facts. Preserve incident recording and cleanup behavior, while ensuring row
locks are not held during the provider stream.
In `@backend/app/modules/artifacts/guide_setup.py`:
- Around line 130-143: The _verified_item method currently obtains a FOR UPDATE
lock through get_verified_guide_content_candidate even though the session closes
before the candidate is used. Add or reuse a non-locking read variant for this
eligibility pre-check, and update _verified_item to call it while preserving the
existing candidate validation and _VerifiedItem construction.
In `@backend/app/modules/artifacts/repository.py`:
- Around line 103-153: Move the get_verified_guide_content_candidate method to
immediately after ArtifactRepository.__init__, keeping its implementation
unchanged and preserving the existing ordering of all other methods.
In `@backend/app/modules/projects/guide_setup_continuation.py`:
- Around line 20-30: Add a docstring and explicit SQLAlchemy boolean-clause
return type to `_retryable_dispatch_predicate`, documenting the recovery
contract it shares with `dispatch_pre_submit_setup_pipeline_after_commit`; keep
the existing predicate conditions unchanged and ensure the annotation matches
the clause returned by `or_`.
In `@backend/app/modules/projects/service.py`:
- Around line 2738-2765: Update run_verified_guide_sufficiency_agent to call the
existing _verified_agent_item method for GuideSourceItemMaterial projection,
passing each canonical extraction row directly to it. Remove the local
agent_item closure and preserve the current agent input behavior through the
shared mapping.
- Around line 3300-3348: Move the database queries from
ProjectService._verified_report_usages into ProjectRepository methods such as
list_report_source_usages and count_snapshot_items, passing the report, setup
run, generation, and snapshot identifiers as arguments. Update
_verified_report_usages to call those repository methods, while retaining the
existing completeness, uniqueness, ordering validations and PolicySetupBlocked
behavior in the service.
In `@backend/app/workers/celery_app.py`:
- Around line 84-87: Update the schedule entry identified by
GUIDE_SETUP_CONTINUATION_SCAN_SCHEDULE to use a dedicated continuation-scan
interval setting instead of
settings.artifact_pending_work_scan_interval_seconds, and add or reference the
corresponding configuration key through the existing settings mechanism while
preserving the current task registration.
In `@backend/app/workers/project_setup.py`:
- Around line 116-130: Remove the redundant _run_pre_submit_setup_pipeline
pass-through and update its callers to invoke
_run_verified_pre_submit_sufficiency_continuation directly, unless the Celery
task or existing tests require the wrapper’s public name; preserve all argument
forwarding and behavior.
In `@backend/tests/test_projects.py`:
- Around line 6304-6320: Update
test_source_snapshot_rejects_credential_and_local_refs to use the parametrized
expected_detail value in its assertion, verifying each case’s specific response
detail; alternatively remove expected_detail from the test signature and
parametrization if only the shared message is intended.
🪄 Autofix (Beta)
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: 0a2b5440-12c3-43ca-ae2d-77cb9436cddf
📒 Files selected for processing (42)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03C-guide-source-cutover-continuation.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03C-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03C-pr-trust-bundle.md.github/workflows/backend.ymlREADME.mdbackend/alembic/versions/0048_guide_source_v2_cutover.pybackend/app/adapters/artifacts/internal_workers.pybackend/app/interfaces/project_agents.pybackend/app/modules/artifacts/guide_bindings.pybackend/app/modules/artifacts/guide_extraction_service.pybackend/app/modules/artifacts/guide_materialization.pybackend/app/modules/artifacts/guide_setup.pybackend/app/modules/artifacts/repository.pybackend/app/modules/projects/guide_mutation_router.pybackend/app/modules/projects/guide_mutation_service.pybackend/app/modules/projects/guide_setup_continuation.pybackend/app/modules/projects/models.pybackend/app/modules/projects/repository.pybackend/app/modules/projects/router.pybackend/app/modules/projects/schemas.pybackend/app/modules/projects/service.pybackend/app/modules/projects/setup_queue.pybackend/app/workers/artifacts.pybackend/app/workers/celery_app.pybackend/app/workers/project_setup.pybackend/scripts/api_contract_e2e.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_artifact_admission.pybackend/tests/test_artifact_recovery.pybackend/tests/test_artifact_verification.pybackend/tests/test_guide_bindings.pybackend/tests/test_guide_extraction.pybackend/tests/test_projects.pybackend/tests/test_tasks.pydocs/architecture_data_model.mddocs/decision_0011_submission_artifact_policy_drives_pre_submit.mddocs/spec_artifact_storage_service.mddocs/spec_chunk_3_project_guide_foundation.mddocs/template_submission_artifact_policy.mdscripts/check_stale_artifact_contracts.pyscripts/test_lightweight_agent_gates.py
💤 Files with no reviewable changes (2)
- backend/app/modules/projects/guide_mutation_router.py
- backend/app/interfaces/project_agents.py
…guide-source-cutover
…guide-source-cutover # Conflicts: # backend/tests/conftest.py # backend/tests/test_alembic.py
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/tests/test_projects.py (1)
4017-4231: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the verified-report helper out of a test model.
backend/tests/test_tasks.pyimportscreate_verified_report_fixturefromtest_projects.py, but the fixture also belongs tobackend/tests/test_projects.py. This creates a module import from product test fixtures into another product test module; keep the helper in its logical module and import its artifact setup helpers separately, or move that helper to a shared test utility if both modules need the full verified lineage 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_projects.py` around lines 4017 - 4231, Move create_verified_report_fixture out of test_projects.py into a shared test utility module used by both test_projects.py and test_tasks.py. Update both callers to import the helper from that shared location, while keeping its artifact setup dependencies imported separately and preserving the complete verified-lineage 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.
Nitpick comments:
In `@backend/tests/test_projects.py`:
- Around line 4017-4231: Move create_verified_report_fixture out of
test_projects.py into a shared test utility module used by both test_projects.py
and test_tasks.py. Update both callers to import the helper from that shared
location, while keeping its artifact setup dependencies imported separately and
preserving the complete verified-lineage behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ea6ea872-babe-4a26-a98e-3f4d3a48fcc5
📒 Files selected for processing (20)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03C-external-review-response.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03C-pr-trust-bundle.mdREADME.mdbackend/alembic/versions/0049_guide_source_v2_cutover.pybackend/app/adapters/artifacts/internal_workers.pybackend/app/core/config.pybackend/app/modules/projects/models.pybackend/app/modules/projects/repository.pybackend/app/modules/projects/router.pybackend/app/modules/projects/schemas.pybackend/app/workers/project_setup.pybackend/scripts/api_contract_e2e.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_artifact_admission.pybackend/tests/test_artifact_verification.pybackend/tests/test_projects.pybackend/tests/test_tasks.pydocs/decision_0011_submission_artifact_policy_drives_pre_submit.mddocs/spec_artifact_storage_service.md
🚧 Files skipped from review as they are similar to previous changes (13)
- backend/tests/conftest.py
- backend/app/modules/projects/repository.py
- backend/tests/test_artifact_admission.py
- backend/app/workers/project_setup.py
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03C-pr-trust-bundle.md
- backend/tests/test_artifact_verification.py
- docs/decision_0011_submission_artifact_policy_drives_pre_submit.md
- backend/app/modules/projects/schemas.py
- README.md
- docs/spec_artifact_storage_service.md
- backend/tests/test_tasks.py
- backend/scripts/api_contract_e2e.py
- backend/app/adapters/artifacts/internal_workers.py
…e-cutover' into codex/ws-art-001-03c-guide-source-cutover
…guide-source-cutover # Conflicts: # scripts/test_lightweight_agent_gates.py
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/app/modules/projects/service.py (4)
3253-3347: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winBind each provenance order to the snapshot row.
The query verifies that usage orders are consecutive and that source item IDs are unique. It does not verify that
GuideSufficiencyReportSourceUsage.item_orderequals the referencedGuideSourceSnapshotItem.item_order.Two source IDs can be swapped while all current checks pass. The resulting provenance references can have the wrong server-owned order, and activation can still accept them.
Add the order equality to the join or compare the ordered source-item IDs with the ordered snapshot rows.
Proposed validation
.where( GuideSufficiencyReportSourceUsage.report_id == sufficiency_report.id, GuideSufficiencyReportSourceUsage.project_setup_run_id == sufficiency_report.project_setup_run_id, GuideSufficiencyReportSourceUsage.setup_generation == sufficiency_report.setup_generation, GuideSourceSnapshotItem.source_snapshot_id == sufficiency_report.source_snapshot_id, + GuideSourceSnapshotItem.item_order + == GuideSufficiencyReportSourceUsage.item_order, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/modules/projects/service.py` around lines 3253 - 3347, Update _verified_report_usages to bind each GuideSufficiencyReportSourceUsage.item_order to the referenced GuideSourceSnapshotItem.item_order, either by adding that equality to the join or by comparing ordered IDs against the snapshot rows. Preserve the existing completeness, uniqueness, and consecutive-order checks while rejecting swapped source IDs.
2769-2776: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winReject booleans in integer manifest fields.
boolis a subclass ofintin Python. Therefore,generation=Truecan equal1, anditem_order=Falsecan equal0. The current checks accept these malformed JSON values.Use exact integer checks before comparison.
Proposed validation
- if manifest.get("generation") != snapshot.creation_generation: + generation = manifest.get("generation") + if type(generation) is not int or generation != snapshot.creation_generation: fail() - if not isinstance(manifest_item["item_order"], int): + if type(manifest_item["item_order"]) is not int: fail()Also applies to: 2815-2823
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/modules/projects/service.py` around lines 2769 - 2776, Update the manifest validation around the schema_version, snapshot_id, generation, and corresponding item_order checks to require exact integers before comparing values, explicitly rejecting booleans despite Python’s bool/int relationship. Apply the same validation in the additional manifest-validation block noted around the item-level checks, while preserving the existing value comparisons and failure behavior.
2835-2848: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winEnforce a closed ART allowlist for source kinds and adapters.
_guide_source_token,build_guide_source_snapshot_manifest, and the runtime snapshot integrity check only normalize/format-checksource_kindandingestion_adapter; they do not reject non-ART source kinds or unregistered adapters. Accept anySAFE_TOKEN_PATTERN-valid value and allow manifests to include arbitrary externalsource_kind/adapter declarations that are not part of the ART source bundle. Add a closed allowlist and require an exact ingestion adapter registry check so these durable snapshot records do not admit unimplemented source items.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/modules/projects/service.py` around lines 2835 - 2848, Enforce a closed ART allowlist in _guide_source_token, build_guide_source_snapshot_manifest, and the runtime snapshot integrity check: accept only supported source_kind values and require ingestion_adapter to exactly match a registered ART adapter. Reject unknown or merely SAFE_TOKEN_PATTERN-valid declarations before they are normalized or persisted, while preserving existing validation for allowed entries.
957-972: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winVerify material immutability during external policy derivation.
run_submission_artifact_policy_derivation_agentandrun_post_submit_checker_policy_derivation_agentcall_verified_guide_source_material, await the external agent, then validate context before persistence. That helper computessource_itemsfrom a singleGuideSufficiencyMaterialPort.load, and_verified_source_material_refspersists artifact extraction rows without comparing prompt provenance. Ifloadcan return different content under the same setup generation, the derived policy can reference changed material. Makeloadimmutable for the full canonical derivation payload, or reload the material before savingsource_material_refs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/modules/projects/service.py` around lines 957 - 972, Ensure material provenance remains immutable across the full derivation flow in run_submission_artifact_policy_derivation_agent and run_post_submit_checker_policy_derivation_agent: either make GuideSufficiencyMaterialPort.load provide a stable canonical payload for the entire operation, or reload it immediately before _verified_source_material_refs persists source_material_ref rows and compare it with the originally verified material. Reject or avoid persistence when the material changes.
🧹 Nitpick comments (1)
.github/workflows/backend.yml (1)
149-149: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winPass the step output through the environment.
job_start_epochcurrently comes fromdate +%s, so the current value is numeric and not attacker-controlled. Line 149 still inserts a step output directly into generated shell source. Pass it through a step-level environment variable and print"$JOB_START_EPOCH"instead. This removes thezizmortemplate-injection finding and keeps the sink safe if the producer changes.Proposed hardening
- name: Bind lane bundle timing if: ${{ always() }} working-directory: backend shell: bash + env: + JOB_START_EPOCH: ${{ steps.identity.outputs.job_start_epoch }} run: >- - printf '%s\n' '${{ steps.identity.outputs.job_start_epoch }}' + printf '%s\n' "$JOB_START_EPOCH" > '.ci/lane-bundle/${{ matrix.lane }}/job-start-epoch.txt'🤖 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 @.github/workflows/backend.yml at line 149, Update the workflow step that prints job_start_epoch to pass steps.identity.outputs.job_start_epoch through that step’s environment, then print the quoted JOB_START_EPOCH variable instead of interpolating the output directly in shell source.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/app/modules/projects/service.py`:
- Around line 3253-3347: Update _verified_report_usages to bind each
GuideSufficiencyReportSourceUsage.item_order to the referenced
GuideSourceSnapshotItem.item_order, either by adding that equality to the join
or by comparing ordered IDs against the snapshot rows. Preserve the existing
completeness, uniqueness, and consecutive-order checks while rejecting swapped
source IDs.
- Around line 2769-2776: Update the manifest validation around the
schema_version, snapshot_id, generation, and corresponding item_order checks to
require exact integers before comparing values, explicitly rejecting booleans
despite Python’s bool/int relationship. Apply the same validation in the
additional manifest-validation block noted around the item-level checks, while
preserving the existing value comparisons and failure behavior.
- Around line 2835-2848: Enforce a closed ART allowlist in _guide_source_token,
build_guide_source_snapshot_manifest, and the runtime snapshot integrity check:
accept only supported source_kind values and require ingestion_adapter to
exactly match a registered ART adapter. Reject unknown or merely
SAFE_TOKEN_PATTERN-valid declarations before they are normalized or persisted,
while preserving existing validation for allowed entries.
- Around line 957-972: Ensure material provenance remains immutable across the
full derivation flow in run_submission_artifact_policy_derivation_agent and
run_post_submit_checker_policy_derivation_agent: either make
GuideSufficiencyMaterialPort.load provide a stable canonical payload for the
entire operation, or reload it immediately before _verified_source_material_refs
persists source_material_ref rows and compare it with the originally verified
material. Reject or avoid persistence when the material changes.
---
Nitpick comments:
In @.github/workflows/backend.yml:
- Line 149: Update the workflow step that prints job_start_epoch to pass
steps.identity.outputs.job_start_epoch through that step’s environment, then
print the quoted JOB_START_EPOCH variable instead of interpolating the output
directly in shell source.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7ab1e5b-3958-4223-948c-39e7eb725683
📒 Files selected for processing (19)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03C-external-review-response.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03C-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03C-pr-trust-bundle.md.github/workflows/backend.ymlbackend/alembic/versions/0049_guide_source_v2_cutover.pybackend/app/modules/artifacts/guide_extraction_service.pybackend/app/modules/artifacts/guide_materialization.pybackend/app/modules/artifacts/guide_setup.pybackend/app/modules/artifacts/repository.pybackend/app/modules/projects/guide_setup_continuation.pybackend/app/modules/projects/service.pybackend/tests/test_api_controls.pybackend/tests/test_artifact_admission.pybackend/tests/test_guide_bindings.pybackend/tests/test_projects.pybackend/tests/test_tasks.pybackend/tests/verified_guide_fixtures.pyscripts/check_stale_artifact_contracts.pyscripts/test_lightweight_agent_gates.py
🚧 Files skipped from review as they are similar to previous changes (9)
- backend/app/modules/artifacts/guide_extraction_service.py
- backend/app/modules/artifacts/repository.py
- backend/app/modules/projects/guide_setup_continuation.py
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03C-pr-trust-bundle.md
- backend/tests/test_tasks.py
- backend/app/modules/artifacts/guide_setup.py
- backend/alembic/versions/0049_guide_source_v2_cutover.py
- backend/app/modules/artifacts/guide_materialization.py
- backend/tests/test_projects.py
2 participants
WS-ART-001-03C PR Trust Bundle
Chunk
WS-ART-001-03C— Guide Source Cutover And Continuation (L1).Goal
Make verified ART bindings and canonical extraction usages the only
authoritative guide content, remove legacy caller byte identity, and continue
the same setup generation automatically after verification.
Human-approved intent
Project Manager guide uploads may be PDF, DOCX, PPTX, CSV, XLSX, Markdown,
plain text, JSON, or supported images in v0.1. They are distinct from the
submitter's required outer ZIP. ART preserves original bytes, and setup agents
consume only verified, bounded extracted content. No audio/video support,
retention worker, manual resume action, or submission work belongs here.
What changed
sanitized non-authoritative labels; removed hash/CID/ref/excerpt authority.
verified report uniqueness.
activation.
project-owned Celery continuation using identifiers/generation facts only.
Why it changed
Caller metadata cannot prove which bytes Workstream checked. Policy derivation
and activation must instead follow exact verified content and extraction
lineage while preserving the existing project setup generation fence.
Design chosen
GuideSourceSnapshotdeclares ordered items. ART owns content identity throughArtifactContent, verified replicas, exact bindings, classifications,extractions, and usages. The project continuation receives only a closed
prepare_generationcapability. AUTH-04B prepared handles are fresh,transaction-bound, never serialized, and consumed before provider reads or
binding mutations.
Alternatives rejected
Scope control
No task/submission/checker/review cutover, generic download permission,
provider/factory change, AUTH catalogue activation, or Project Manager resume
command is included.
Product behavior
Creating a snapshot records queued setup but produces no agent output until
every declared item has verified same-generation ART material. Verification
continues setup automatically. Missing, changed, stale, cross-context, or
incomplete content fails closed as artifact/setup failure, not guide
insufficiency.
Acceptance criteria proof
late-worker status backstep guard.
replay, queued-before-material behavior, and fresh/stale dispatch behavior.
Tests/checks run
git diff --check: passed.the user requested hosted sharded CI rather than a full local suite.
Test delta
Legacy route/automatic-output tests were replaced by verified-source waiting,
real constrained ART provenance, live derivation route replay, visibility, and
dispatch recovery tests. No skip or xfail was introduced.
CI integrity
The repository-wide 78% floor remains unchanged. The backend workflow adds 90%
coverage reports for the project subsystem and project-agent boundary without
weakening lint, E2E, semantic-lane, skip/deselect, or existing subsystem gates.
Reviewer results
Architecture, security, product/ops, senior engineering, CI integrity, docs,
reuse/dedup, test-delta, and QA passed after findings were resolved.
External review
GitHub Backend/Agent Gates and CodeRabbit remain required after the PR is pushed.
Remaining risks
The broad PostgreSQL integration and migration matrix is delegated to hosted
CI because no local test database is configured. The implementation depends on
the already-merged AUTH-04B exact fixed-service actions and fails closed if
their live grants or identities are unavailable.
Follow-up work
After merge and hosted evidence, the next planned ART sequence is submission
bundle work beginning with ART-04A. It starts only with human direction and its
own bounded chunk contract.
Human review focus
Review migration refusal semantics, exact report-usage completeness, the
transaction-held AUTH/read boundary, dispatch claim recovery, and the removal
of all legacy guide-content authority.
Human merge ownership
The human owner decides whether and when to merge this PR. This bundle does not
authorize merge.
Summary by CodeRabbit