WS-ART-001-03A: hidden guide source byte ingest - #215
Conversation
|
Warning Review limit reached
Next review available in: 35 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 (8)
📝 WalkthroughWalkthroughGuide-source byte ingest is added behind a fail-closed authorization boundary. The change introduces durable ingest staging and migration safeguards, transactional admission and replay-aware publishing, a hidden route, focused tests, and updated CI and review evidence. ChangesGuide artifact ingest
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
b37793d to
5d03900
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
backend/app/modules/artifacts/authorization.py (1)
80-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider explicit keyword-only params instead of
**values: UUID.This digest is security-relevant (it must match AUTH's prepared-request binding). With
**values, a mistyped or omitted keyword at a call site fails only at runtime insideguide_ingest_prepared_request_value, and type checkers can't verify arity. Mirroring the five explicit keyword-only parameters keeps the binding statically checked.♻️ Explicit signature
-def guide_ingest_prepared_request_digest(**values: UUID) -> str: +def guide_ingest_prepared_request_digest( + *, + project_id: UUID, + guide_id: UUID, + guide_source_snapshot_id: UUID, + guide_source_item_id: UUID, + idempotency_key: UUID, +) -> str: """Match PreparedAuthorizationService's canonical request binding.""" return canonical_json_hash( { "domain": "workstream.prepared_authorization.request.v1", - "request": guide_ingest_prepared_request_value(**values), + "request": guide_ingest_prepared_request_value( + project_id=project_id, + guide_id=guide_id, + guide_source_snapshot_id=guide_source_snapshot_id, + guide_source_item_id=guide_source_item_id, + idempotency_key=idempotency_key, + ), } )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/app/modules/artifacts/authorization.py` around lines 80 - 87, Update guide_ingest_prepared_request_digest to declare the same five explicit keyword-only parameters as guide_ingest_prepared_request_value, preserving their UUID types and forwarding them to the existing request-value helper. Remove the **values signature so callers and type checkers enforce the required names and arity while keeping the canonical hashing behavior unchanged.backend/tests/test_artifact_recovery.py (1)
77-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated test double across three suites.
The same
_AllowGuidePreparedAuthorization(handle viaobject.__new__,consumereturning the actor profile id) also appears inbackend/tests/test_artifact_admission.pyandbackend/tests/test_artifact_internal_authorization.py. Moving it toconftest.pykeeps the three copies from drifting as the prepared-authorization contract evolves.🤖 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_recovery.py` around lines 77 - 85, Move the shared _AllowGuidePreparedAuthorization test double into the applicable conftest.py, preserving its object.__new__(PreparedAuthorizationHandle) setup and consume behavior. Remove the duplicate class definitions from test_artifact_recovery.py, test_artifact_admission.py, and test_artifact_internal_authorization.py so all three suites reuse the centralized fixture/helper.backend/tests/test_artifact_architecture.py (1)
37-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptionally extend the structural checks to results.
CANONICAL_REQUESTSis verified against AST class names and the forbidden-field set, butCANONICAL_RESULTSis only checked for__all__membership. A future result dataclass could exposeprovider_object_ref/storage_namespacewithout failing this guard.♻️ Suggested addition
+ result_names = { + node.name + for node in tree.body + if isinstance(node, ast.ClassDef) and node.name.endswith("Result") + } + assert result_names == CANONICAL_RESULTSThen include
CANONICAL_RESULTSin theclass_node.name in CANONICAL_REQUESTSfilter used forforbidden_fields.Also applies to: 287-333
🤖 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` at line 37, Extend the structural forbidden-field validation to cover result classes as well as request classes. In the AST class-name filter around CANONICAL_REQUESTS and forbidden_fields, include CANONICAL_RESULTS so canonical result dataclasses are rejected when exposing provider_object_ref or storage_namespace, while preserving the existing __all__ checks.backend/app/modules/artifacts/service.py (1)
306-347: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftByte upload happens while the PREP transaction is open.
prepare_and_admitstreams and hashes the entire client body inside the authority transaction (_artifact_admission_transaction(existing=True)reuses the issuer's root transaction). With the deny adapter this is inert, but once 04A activates a real AUTH-backed adapter, a slow or stalled upload will hold an open DB transaction — and any rows locked duringprepare— for the full transfer duration, risking connection-pool exhaustion and lock contention.Worth confirming the activation plan either bounds body read time or moves preparation ahead of the transaction open.
🤖 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 306 - 347, Move the body-consuming `prepare_and_admit` call in the artifact ingestion flow so byte upload and hashing complete before `authority_transaction` is opened, or otherwise enforce a strict bounded read timeout before entering the transaction. Preserve the existing authorization preparation and admission behavior, and update the surrounding `transaction_open` lifecycle in the containing method to match the new ordering.backend/app/modules/artifacts/repository.py (1)
165-213: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
stage_guide_source_ingestre-fetches lineage that the caller already locked.
admit()inservice.pycallsself._repo.get_guide_lineage(...)to build the authority facts, then immediately callsstage_guide_source_ingest, which callsget_guide_lineageagain internally — the same FOR UPDATE query onGuideSourceSnapshotItem/GuideSourceSnapshotruns twice per admission. It's not incorrect (Postgres re-locks the already-held row for the same transaction without blocking), just a redundant round trip on the ingest hot path.Consider accepting the previously-fetched
GuideLineageFactsas a parameter sostage_guide_source_ingestdoesn't need to re-query.🤖 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 165 - 213, Update stage_guide_source_ingest to accept the previously fetched GuideLineageFacts from admit instead of calling get_guide_lineage again; validate the supplied lineage against guide_source_snapshot_id, guide_id, project_id, and guide_source_item_id, while preserving the existing ingest conflict and persistence 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/adapters/artifacts/__init__.py`:
- Around line 156-180: Move scratch-manager construction into the existing
cleanup-protected flow around the bootstrap setup so any
ArtifactConfigurationError from create_artifact_scratch_manager still reaches
cleanup. Ensure bootstrap.close() runs whenever bootstrap was created, while
manager.close() runs only after the manager is successfully constructed;
preserve the existing yielded services and initialization behavior.
In `@backend/app/modules/projects/router.py`:
- Around line 232-247: Parse project_id, guide_id, source_snapshot_id,
source_item_id, and the optional idempotency_key before the try block,
preserving the concealed 404 response for parsing failures. Pass the parsed UUID
values into ingest.ingest, and remove ValueError from the exception tuple so
only ArtifactAdmissionRelationshipError and ArtifactAuthorityDeniedError are
mapped to 404; unrelated ValueError instances must propagate distinctly.
In `@scripts/test_lightweight_agent_gates.py`:
- Around line 54-61: Update
test_backend_workflow_has_one_project_subsystem_coverage_gate so it extracts the
named “Project subsystem coverage” workflow step, asserts that step contains the
expected coverage command, and verifies the step appears after “Backend
full-suite coverage”; do not count the step name and command independently
across the entire workflow.
---
Nitpick comments:
In `@backend/app/modules/artifacts/authorization.py`:
- Around line 80-87: Update guide_ingest_prepared_request_digest to declare the
same five explicit keyword-only parameters as
guide_ingest_prepared_request_value, preserving their UUID types and forwarding
them to the existing request-value helper. Remove the **values signature so
callers and type checkers enforce the required names and arity while keeping the
canonical hashing behavior unchanged.
In `@backend/app/modules/artifacts/repository.py`:
- Around line 165-213: Update stage_guide_source_ingest to accept the previously
fetched GuideLineageFacts from admit instead of calling get_guide_lineage again;
validate the supplied lineage against guide_source_snapshot_id, guide_id,
project_id, and guide_source_item_id, while preserving the existing ingest
conflict and persistence behavior.
In `@backend/app/modules/artifacts/service.py`:
- Around line 306-347: Move the body-consuming `prepare_and_admit` call in the
artifact ingestion flow so byte upload and hashing complete before
`authority_transaction` is opened, or otherwise enforce a strict bounded read
timeout before entering the transaction. Preserve the existing authorization
preparation and admission behavior, and update the surrounding
`transaction_open` lifecycle in the containing method to match the new ordering.
In `@backend/tests/test_artifact_architecture.py`:
- Line 37: Extend the structural forbidden-field validation to cover result
classes as well as request classes. In the AST class-name filter around
CANONICAL_REQUESTS and forbidden_fields, include CANONICAL_RESULTS so canonical
result dataclasses are rejected when exposing provider_object_ref or
storage_namespace, while preserving the existing __all__ checks.
In `@backend/tests/test_artifact_recovery.py`:
- Around line 77-85: Move the shared _AllowGuidePreparedAuthorization test
double into the applicable conftest.py, preserving its
object.__new__(PreparedAuthorizationHandle) setup and consume behavior. Remove
the duplicate class definitions from test_artifact_recovery.py,
test_artifact_admission.py, and test_artifact_internal_authorization.py so all
three suites reuse the centralized fixture/helper.
🪄 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: a0ff7be3-7eff-4e14-af07-4ddba4232ea2
📒 Files selected for processing (30)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/PLAN.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03A-guide-source-byte-ingest.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03A-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03A-pr-trust-bundle.md.agent-loop/merge-intents/WS-ART-001-03A.json.github/workflows/backend.ymlbackend/alembic/versions/0038_guide_source_artifact_ingests.pybackend/app/adapters/artifacts/__init__.pybackend/app/interfaces/artifact_operations.pybackend/app/modules/artifacts/authorization.pybackend/app/modules/artifacts/repository.pybackend/app/modules/artifacts/router.pybackend/app/modules/artifacts/schemas.pybackend/app/modules/artifacts/service.pybackend/app/modules/projects/models.pybackend/app/modules/projects/router.pybackend/app/modules/projects/schemas.pybackend/scripts/run_test_lanes.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_artifact_admission.pybackend/tests/test_artifact_architecture.pybackend/tests/test_artifact_authorization.pybackend/tests/test_artifact_internal_authorization.pybackend/tests/test_artifact_recovery.pybackend/tests/test_ci_test_lanes.pybackend/tests/test_guide_artifacts.pydocs/spec_artifact_storage_service.mdscripts/test_lightweight_agent_gates.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/tests/test_guide_artifacts.py`:
- Around line 382-415: Update
test_guide_ingest_rejects_invalid_logical_role_before_preparation to expect
ArtifactAdmissionRelationshipError instead of the broad Exception type, while
preserving the existing message match and assertions that the source is not read
and no cleanup is pending.
🪄 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: 91babf2e-c939-4f48-a2ad-e3df51def937
📒 Files selected for processing (7)
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/PLAN.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03A-guide-source-byte-ingest.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03A-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03A-pr-trust-bundle.mdbackend/tests/test_guide_artifacts.pyscripts/test_lightweight_agent_gates.py
💤 Files with no reviewable changes (1)
- scripts/test_lightweight_agent_gates.py
🚧 Files skipped from review as they are similar to previous changes (5)
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03A-internal-review-evidence.md
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03A-guide-source-byte-ingest.md
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03A-pr-trust-bundle.md
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/PLAN.md
2 participants
PR Trust Bundle: WS-ART-001-03A
Chunk
WS-ART-001-03A— Guide Source Byte Ingest (L1)Goal And Human-Approved Intent
Accept exact guide-source bytes through bounded private scratch and the existing
immutable ArtifactStore path, without binding or activating guide reads yet.
What Changed And Why
0038.and recovery paths instead of adding candidate storage.
exact locked lineage plus server-computed digest, size, and media type.
exact projects subsystem coverage gate.
Design And Scope Control
Preflight occurs before runtime construction or byte reads. Final PREP consume,
staging, capacity reservation, and put intent commit together. Provider I/O
runs afterward through the already-activated fixed-service put resolver.
Binding, materialization, setup continuation, legacy removal, and guide action
activation remain outside 03A. No provider/factory or public API expansion was
introduced.
Acceptance Proof
capacity before another conditional write.
Tests And CI Integrity
Focused route/architecture tests, isolated PostgreSQL migration/admission/replay
tests, changed-file Ruff, compilation, stale-contract/wording/auth scans,
Markdown links, diff checks, and agent gates pass. GitHub owns the full sharded
suite, repository 78% floor, and accumulated 90% subsystem gates; thresholds
were not lowered and the exact projects 90% gate was added.
Remaining Risks And Follow-Up
The guide ingest action intentionally remains unavailable. After this PR merges,
AUTH
WS-XINT-002-04Ainstalls the exact Project Manager adapter and activatesonly guide ingestion. ART-03B remains a separate explicit-start successor.
Human Review Focus And Merge Ownership
Confirm the two authorization points, commit-before-provider boundary,
confirmed-missing replay, and strict 03A scope. The user retains approval and
merge ownership for this specific PR.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation