Repository navigation
Phase 6g: bind source records to verified portable content - #72
Conversation
|
Warning Review limit reached
Next review available in: 47 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 (3)
📝 WalkthroughWalkthroughThe PR binds non-null ChangesSource-content closure
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Sequence Diagram(s)sequenceDiagram
participant Client
participant App
participant ArtifactStore
participant Store
participant ReleaseBuilder
participant OfflineVerifier
Client->>App: submit source create or version
App->>ArtifactStore: verify CAS bytes and source metadata
App->>Store: persist validated source record
ReleaseBuilder->>Store: collect source content_hash
ReleaseBuilder->>ArtifactStore: include exact source artifact
OfflineVerifier->>ReleaseBuilder: validate artifact inventory and metadata
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
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 (1)
src/artifacts/mod.rs (1)
26-31: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winError text still says "symbolic link" although reparse points are now rejected.
src/release.rsupdated its parallel messages to "links or reparse points"; these two remain symlink-only, so a Windows operator hitting a junction gets a misleading diagnostic.📝 Proposed wording fix
return Err(AppError::new( "MCL_ARTIFACT_ROOT_UNSAFE", - "artifact root may not be a symbolic link", + "artifact root may not be a link or reparse point", false, "Configure a real directory under the instance root.", ));return Err(AppError::new( "MCL_ARTIFACT_PATH_UNSAFE", format!( - "symbolic link is forbidden in artifact path: {}", + "link or reparse point is forbidden in artifact path: {}", current.display() ),Also applies to: 285-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 `@src/artifacts/mod.rs` around lines 26 - 31, Update both MCL_ARTIFACT_ROOT_UNSAFE error messages in the artifact-root validation flow to describe rejection of links or reparse points, matching the wording used in src/release.rs; leave the error code and remediation guidance unchanged.
🧹 Nitpick comments (2)
src/app.rs (1)
9724-9803: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant no-op restore write before the substitution test.
Lines 9732-9736 rewrite the exact BH bytes, but they're immediately overwritten by the substituted-bytes write on the next line with no assertion between them — the "restore" has no observable effect and reads as if it verifies something it doesn't.
♻️ Suggested cleanup
- fs::write( - &restarted_bh_artifact, - include_bytes!("../fixtures/bh_formalization/SOURCE-LOCK.md"), - ) - .expect("exact BH source bytes restore"); fs::write(&restarted_bh_artifact, b"substituted source bytes") .expect("test substitutes copied BH source bytes");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app.rs` around lines 9724 - 9803, Remove the redundant fs::write call that restores SOURCE-LOCK.md immediately before the substitution write in the restarted bundle integrity test. Keep the initial restoration needed after the missing-artifact check and preserve the substituted-bytes assertion and subsequent exact restoration.src/artifacts/mod.rs (1)
327-341: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
is_link_or_reparseis duplicated byte-for-byte in two modules. Both copies define the same Windows reparse-point predicate, including the localFILE_ATTRIBUTE_REPARSE_POINTconstant. As a security boundary check, the two can silently drift if only one is later hardened (for example to cover mount points or non-regular entries).
src/artifacts/mod.rs#L327-L341: keep this as the single definition, or move it to a shared path-safety module, and export it for reuse.src/release.rs#L1348-L1362: delete the local copy and import the shared helper instead.🤖 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 `@src/artifacts/mod.rs` around lines 327 - 341, The is_link_or_reparse security predicate is duplicated across two modules and must have a single reusable definition. In src/artifacts/mod.rs lines 327-341, retain or move is_link_or_reparse to a shared path-safety module and export it; in src/release.rs lines 1348-1362, remove the local copy and import and use the shared helper instead.
🤖 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 `@src/artifacts/mod.rs`:
- Around line 26-31: Update both MCL_ARTIFACT_ROOT_UNSAFE error messages in the
artifact-root validation flow to describe rejection of links or reparse points,
matching the wording used in src/release.rs; leave the error code and
remediation guidance unchanged.
---
Nitpick comments:
In `@src/app.rs`:
- Around line 9724-9803: Remove the redundant fs::write call that restores
SOURCE-LOCK.md immediately before the substitution write in the restarted bundle
integrity test. Keep the initial restoration needed after the missing-artifact
check and preserve the substituted-bytes assertion and subsequent exact
restoration.
In `@src/artifacts/mod.rs`:
- Around line 327-341: The is_link_or_reparse security predicate is duplicated
across two modules and must have a single reusable definition. In
src/artifacts/mod.rs lines 327-341, retain or move is_link_or_reparse to a
shared path-safety module and export it; in src/release.rs lines 1348-1362,
remove the local copy and import and use the shared helper instead.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a70e673d-a0b9-4ce0-8d24-bb728f7811a2
📒 Files selected for processing (20)
.gitattributesdocs/decisions/ADR-0017-source-content-is-a-verified-portable-binding.mddocs/implementation/RELEASE_CHECKLIST.mddocs/implementation/STATUS.mddocs/trust/ARTIFACTS.mddocs/trust/RELEASES.mddocs/trust/SOURCES.mdfixtures/bh_formalization/LICENSEfixtures/bh_formalization/NOTICEfixtures/bh_formalization/README.mdfixtures/bh_formalization/SOURCE-LOCK.mdmigrations/0013_source_content_closure.sqlscripts/publication-candidate.shsrc/app.rssrc/artifacts/mod.rssrc/domain/schemas.rssrc/release.rssrc/store/mod.rstests/mcp_stdio.rstests/phase1_cli.rs
|
Addressed all three CodeRabbit findings in
Review-fix validation: format check, warnings-denied all-target Clippy, the Windows CAS-junction test, the database-free portable-release test, diff check. |
Closes #71
Summary
source/1.content_hashto resolve to registered, byte-verified CAS content with reviewedsource_contentprovenance and compatible license/restriction metadata before CLI/MCP dry-run, persistence, or exact retry.8875bef812375d7e6fd00bff95764cd5c057c7fe, tree600f3e1230ca88f9784837933e2417c319aaad3e, Git blobc7e3805ab78327f1cd561421e2c0168a009eb161, SHA-256249f62a4e2780235239cf9f4e781f274f8dadba7f3aeba3aab51f46b0a6fc5c5.The issue's earlier
c8209676...digest was the CRLF rendering from a Windows checkout. A directgh apicontent read establishes the 1,809-byte Git object above as the platform-independent portable identity.Trust boundary
This preserves
content_hash: nulland the existingsource/1schema bytes. Exact source retention creates no paper correspondence, fidelity, Lean-kernel, Comparator, mathematical-status, definition, reuse, coverage, or contribution authority; Pilot C remains incomplete.Validation
cargo fmt --all -- --checkcargo clippy --workspace --all-targets --all-features -- -D warningscargo test --workspace(191 library, 1 default Lean-worker, 4 MCP, 21 CLI)MCL_RUN_LEAN_INTEGRATION=1 cargo test --test lean_worker -- --nocapturebash -n scripts/publication-candidate.shgit diff --checkSummary by CodeRabbit
New Features
Bug Fixes
Documentation