WS-ART-001-03B2: verify and classify guide materialization - #223
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 (2)
📝 WalkthroughWalkthroughAdds hidden guide-source materialization with exact lineage authorization, bounded scratch inspection, deterministic format classification, immutable classification/incident storage, migration safeguards, and focused architecture and database-backed tests. ChangesGuide materialization and classification
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant ArtifactMaterializationService
participant ArtifactPreparationService
participant GuideFormatDetector
participant Database
Client->>ArtifactMaterializationService: materialize_guide_source(request)
ArtifactMaterializationService->>Database: load and lock verified lineage
ArtifactMaterializationService->>ArtifactPreparationService: prepare and verify source
ArtifactPreparationService->>GuideFormatDetector: inspect bounded scratch reader
GuideFormatDetector-->>ArtifactMaterializationService: classification result
ArtifactMaterializationService->>Database: revalidate and persist classification or incident
Database-->>Client: materialization result
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 review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (6)
backend/alembic/versions/0040_guide_materialization_classification.py (1)
1-14: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFilename doesn't match the embedded revision id.
The file is named
0040_guide_materialization_classification.py, butrevision = "0040_guide_materialization"(no_classificationsuffix). This matchesdown_revisionchains,test_alembic.py's 8 revision assertions, anddocs/spec_artifact_storage_service.md's reference to`0040_guide_materialization`, but not the filename itself or the chunk contract's`0040_guide_materialization_classification`reference. The prior migration0039_guide_source_bindings.pykeeps filename and revision id aligned; this one doesn't, which will confuse anyone trying to locate the migration by its documented/DB-recorded name. Alembic itself doesn't require filename/revision parity (it reads therevisionvariable), so this won't break upgrades/downgrades, but it's worth aligning one way or the other.♻️ Align filename with the embedded revision id
-backend/alembic/versions/0040_guide_materialization_classification.py +backend/alembic/versions/0040_guide_materialization.py🤖 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/0040_guide_materialization_classification.py` around lines 1 - 14, Align the migration filename with the embedded revision identifier by renaming it to match revision = "0040_guide_materialization", preserving the existing revision chain and migration contents.backend/app/interfaces/artifact_operations.py (1)
270-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate
ArtifactMaterializationPortdocstring — no longer "only two" forms.The docstring
"""Materialize only the two canonical immutable source forms."""(line 271, unchanged) predates this diff, but this diff adds a third method,materialize_guide_source(279-284), to the same port. The docstring now understates the port's actual surface, which is misleading precisely for a PR whose stated goal is enforcing "the canonical materialization boundary."✏️ Update the stale docstring
class ArtifactMaterializationPort(Protocol): - """Materialize only the two canonical immutable source forms.""" + """Materialize only the canonical immutable source forms."""🤖 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/interfaces/artifact_operations.py` around lines 270 - 284, Update the ArtifactMaterializationPort class docstring to remove the stale claim that it materializes only two source forms, and replace it with wording that accurately describes the broader canonical materialization boundary including materialize_prepared_bundle and materialize_guide_source.backend/tests/test_guide_bindings.py (1)
464-511: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplay conflict branch is uncovered.
The suite covers first classification and identical replay, but not the
"guide classification conflicts"branch (an existing classification whose stored facts diverge from the freshly detected ones). Given that branch is the immutability guarantee for persisted evidence, a focused test (e.g. mutatedetected_formatorclassification_factsof the persisted row, then re-materialize) would lock it in.🤖 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_guide_bindings.py` around lines 464 - 511, Add a focused test alongside test_materialization_verifies_classifies_replays_and_cleans_scratch that performs an initial materialization, mutates the persisted GuideSourceFormatClassification detected_format or classification_facts, then re-materializes the same request and asserts the “guide classification conflicts” branch raises the expected error while preserving the stored evidence.backend/app/modules/artifacts/guide_formats.py (2)
309-340: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueJPEG scan assumes every marker carries a length field.
Standalone markers (
0xD8,0xD9,0xD0–0xD7,0x01) have no length, sooffset += 2 + lengthmisinterprets the following bytes as a segment length. The failure mode is benign here (loop terminates andNoneis returned, falling through to the declared/text path), but skipping the length read for standalone markers makes detection deterministic for such files.🤖 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_formats.py` around lines 309 - 340, The JPEG parsing logic in the format-detection function should handle standalone markers before reading a segment length. In the marker loop, recognize 0xD8, 0xD9, 0xD0–0xD7, and 0x01, advance past their marker bytes without interpreting following bytes as a length, and preserve the existing length-based handling for markers that carry segment lengths.
248-255: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNested archive members are fully buffered in memory.
archive.read(info)materializes each nested.zipmember (up to the accumulatedmaximum_decompressed_bytesbudget of 128 MiB) into aBytesIO, and recursion can hold several such buffers alive simultaneously up tomaximum_nesting_depth. Consider adding a per-entry byte ceiling for nested archives (e.g. rejectinfo.file_sizeabove a small nested limit) so peak memory is bounded independently of the aggregate budget.🤖 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_formats.py` around lines 248 - 255, Update the nested archive handling in _inspect_archive to enforce a per-entry size limit before calling archive.read(info) for .zip members. Skip or reject nested members whose info.file_size exceeds the defined nested-archive ceiling, while preserving recursion for entries within the limit and the existing aggregate decompression budget.backend/app/modules/artifacts/sources.py (1)
14-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
_InspectionResultis invariant but only appears in a return position.mypy enforces protocol variance: a type variable used solely as a protocol method's return type must be declared covariant, otherwise it reports “Invariant type variable "_InspectionResult" used in protocol where covariant one is expected”. Declaring a dedicated covariant variable avoids the error and keeps the protocol assignable for subtypes.
♻️ Proposed covariant protocol type variable
-_InspectionResult = TypeVar("_InspectionResult") +_InspectionResult = TypeVar("_InspectionResult") +_InspectionResultCo = TypeVar("_InspectionResultCo", covariant=True) -class PreparedArtifactInspector(Protocol[_InspectionResult]): +class PreparedArtifactInspector(Protocol[_InspectionResultCo]): """Typed artifact-owned inspection capability over one scratch reader.""" - def inspect(self, reader: BinaryIO) -> _InspectionResult: + def inspect(self, reader: BinaryIO) -> _InspectionResultCo: """Return bounded structural facts without retaining the reader."""#!/bin/bash # Confirm the project's type-check configuration and whether protocol variance is enforced. fd -H -t f -e toml -e cfg -e ini . -d 2 --exec grep -lniE 'mypy|pyright' {} \; fd -H -t f -e toml -e cfg -e ini . -d 2 --exec sed -n '1,200p' {} \; | grep -niE -A6 '\[(tool\.)?(mypy|pyright)' rg -n 'PreparedArtifactInspector' backend | head -50🤖 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/sources.py` around lines 14 - 21, Declare a dedicated covariant type variable for the return-only result type and use it in PreparedArtifactInspector instead of the invariant _InspectionResult. Leave the inspect method signature and reader behavior unchanged.
🤖 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/alembic/versions/0040_guide_materialization_classification.py`:
- Around line 1-14: Align the migration filename with the embedded revision
identifier by renaming it to match revision = "0040_guide_materialization",
preserving the existing revision chain and migration contents.
In `@backend/app/interfaces/artifact_operations.py`:
- Around line 270-284: Update the ArtifactMaterializationPort class docstring to
remove the stale claim that it materializes only two source forms, and replace
it with wording that accurately describes the broader canonical materialization
boundary including materialize_prepared_bundle and materialize_guide_source.
In `@backend/app/modules/artifacts/guide_formats.py`:
- Around line 309-340: The JPEG parsing logic in the format-detection function
should handle standalone markers before reading a segment length. In the marker
loop, recognize 0xD8, 0xD9, 0xD0–0xD7, and 0x01, advance past their marker bytes
without interpreting following bytes as a length, and preserve the existing
length-based handling for markers that carry segment lengths.
- Around line 248-255: Update the nested archive handling in _inspect_archive to
enforce a per-entry size limit before calling archive.read(info) for .zip
members. Skip or reject nested members whose info.file_size exceeds the defined
nested-archive ceiling, while preserving recursion for entries within the limit
and the existing aggregate decompression budget.
In `@backend/app/modules/artifacts/sources.py`:
- Around line 14-21: Declare a dedicated covariant type variable for the
return-only result type and use it in PreparedArtifactInspector instead of the
invariant _InspectionResult. Leave the inspect method signature and reader
behavior unchanged.
In `@backend/tests/test_guide_bindings.py`:
- Around line 464-511: Add a focused test alongside
test_materialization_verifies_classifies_replays_and_cleans_scratch that
performs an initial materialization, mutates the persisted
GuideSourceFormatClassification detected_format or classification_facts, then
re-materializes the same request and asserts the “guide classification
conflicts” branch raises the expected error while preserving the stored
evidence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2ef6aeeb-f146-4d22-81d0-fefe9249f0b1
📒 Files selected for processing (22)
.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-03B2-guide-materialization-classification.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B2-internal-review-evidence.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B2-pr-trust-bundle.mdbackend/alembic/versions/0040_guide_materialization_classification.pybackend/app/db/models.pybackend/app/interfaces/artifact_operations.pybackend/app/modules/artifacts/guide_formats.pybackend/app/modules/artifacts/guide_materialization.pybackend/app/modules/artifacts/models.pybackend/app/modules/artifacts/preparation.pybackend/app/modules/artifacts/schemas.pybackend/app/modules/artifacts/service.pybackend/app/modules/artifacts/sources.pybackend/scripts/run_test_lanes.pybackend/tests/conftest.pybackend/tests/test_alembic.pybackend/tests/test_artifact_architecture.pybackend/tests/test_artifact_preparation.pybackend/tests/test_guide_bindings.pybackend/tests/test_guide_formats.pydocs/spec_artifact_storage_service.md
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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-03B2-external-review-response.md:
- Around line 28-29: Update the command descriptions in the listed test bullets
to use unambiguous compound modifiers: “Focused guide-format and preparation
tests” and “Focused database-backed guide-materialization tests.”
- Around line 25-32: Separate the Hosted Backend and Agent Gates entry from the
completed reruns in the review-response document. Move it to a
pending/required-checks section, or replace it with the exact PR head and
recorded gate results only if those gates were actually executed.
🪄 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: 0fe45c71-7ec7-4aea-955a-96664ed178ad
📒 Files selected for processing (10)
.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-03B2-guide-materialization-classification.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B2-external-review-response.md.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B2-pr-trust-bundle.mdbackend/alembic/versions/0040_guide_materialization.pybackend/app/interfaces/artifact_operations.pybackend/app/modules/artifacts/guide_formats.pybackend/app/modules/artifacts/sources.pybackend/tests/test_guide_bindings.pybackend/tests/test_guide_formats.py
🚧 Files skipped from review as they are similar to previous changes (7)
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B2-guide-materialization-classification.md
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/REVIEW_LOG.md
- backend/app/interfaces/artifact_operations.py
- .agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/reviews/WS-ART-001-03B2-pr-trust-bundle.md
- backend/tests/test_guide_bindings.py
- backend/app/modules/artifacts/guide_formats.py
- backend/tests/test_guide_formats.py
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
2 participants
Workstream PR Trust Bundle
Chunk
WS-ART-001-03B2— Guide Materialization And Classification (L1)Goal
Read exact verified guide-source bytes through the fixed guide-reader boundary,
recompute their identity in bounded private scratch, and persist only immutable
syntactic classification or bounded ART incident evidence.
Intent And Planning Context
text, JSON, or image formats; they are not contributor submission ZIPs.
.agent-loop/initiatives/WS-ART-001-immutable-artifact-storage/chunks/WS-ART-001-03B2-guide-materialization-classification.mdWhat Changed
bounded scratch inspection.
incidents, migration
0040, tests, documentation, and review evidence.Why It Changed
Workstream must prove the exact verified guide bytes before later extraction or
sufficiency processing; object-store presence or caller metadata is insufficient.
Design Chosen
Transaction A locks exact lineage and consumes prepared authority before the
provider read. The canonical store streams into canonical scratch. Transaction B
relocks the same facts before persisting one immutable result.
Alternatives Rejected
serialized prepared handles, caller excerpts, and parsing during upload.
Scope Control
Allowed Files Changed
focused tests, ART specification, and this chunk's evidence.
Files Outside Stated Scope
Product Behavior
materialization/classification exists, but
artifact.guide_source.readremains planned and unavailable until AUTH-04B.
Evidence
Commands Run
Result Summary
Acceptance Criteria Proof
Test Delta
Tests Added
typed inspection, materialization lineage, incidents, replay, cancellation,
timeout, conflict, migration, and architecture fences.
Tests Modified
integration coverage.
Tests Removed Or Skipped
Internal Reviewer Results
Reviewed code SHA:
1381d371Reviewed at: 2026-07-29
Reviewer run IDs: recorded in the paired internal-review evidence.
External Review
1381d371CI And Gate Integrity
Remaining Risks
Classification is syntactic by design. Semantic extraction belongs to 03B3A;
live fixed-reader authorization belongs to AUTH-04B.
Follow-Up Work
03B3A adds extraction, 03B3B adds durable continuation, 03B4 finalizes the
hidden manifest, AUTH-04B activates exact fixed-service actions, and 03C later
performs the separate legacy cutover.
Human Review Focus
Please inspect active namespace fencing, pre/post generation locks, complete
rehash comparison, nested-archive bounds, incident privacy, and deny-only AUTH.
Human Merge Ownership