5+3 council: harden lance-graph-hydrate (fast-follow to merged #957) - #958
Conversation
Phase 0 spec + Phase 1 (5 savants, 26 distinct findings) + Phase 2 (consolidated draft v2, 20 committed decisions C1-C20) landed. Phase 3 (3 reviewers) in progress. This is a checkpoint commit of the working record per this repo's own incremental-plan-update convention; Phase 4 fixes + Phase 5 ratification land in a follow-up commit once Phase 3 reports. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw
PR #957 merged mid-council (before Phase 4 landed) — this is the follow-up applying all 20 committed decisions (C1-C20) plus 6 Phase-3 reviewer FIX items, zero BLOCK from any of the 3 reviewers. Real defects found and fixed: - within-process staging-nonce collision (pid+nanos alone can repeat within one clock tick) - new shared staging::staging_suffix() (pid + atomic counter + nanos) - every fetch-phase I/O error leaked its staging directory/file - now cleaned up on every path in both copy.rs and file.rs - an Ok-path cleanup failure could silently falsify hydrate_dir's own "leaves nothing" postcondition - now propagates instead of `let _ =` - hydrate_file's entry-only existence check left the actual danger case unguarded: a file-onto-file rename SILENTLY CLOBBERS an existing destination on POSIX rather than failing - now re-checked immediately before the rename, not only on the rename's Err branch - release_dir could structurally never return Err, making its own missing-directory test vacuous by this repo's own falsifiability rule - now returns Err on a genuinely unreadable root, Ok(0) only for NotFound, with a two-sided test pair - WarmMarker's 2-bare-integer format had no version tag and permissive arity - now "v1 <mtime> <len>", exact 3-token, both a legacy-shaped and a future-wider line refused by tests Overclaims corrected (doc-only, no behavior change): - LifecycleState was claimed "encoded as a transition guard, not caller discipline" - false, referenced by zero function signatures; corrected, and dirty::lifecycle_of() added to actually close the gap - is_dirty was claimed to compare against "the version recorded at hydration time" - false, nothing records it; hydrated_at_version is and remains caller-supplied - the idempotency boundary was described with "the" (singular) condition; corrected to name both - (b) enforced by this crate, (a) structurally the caller's (or the SHA-256 pin, for hydrate_file) - copy.rs's module doc overclaimed the publish-by-rename mechanism was "generalized from hydration_probe.rs's proven mechanism" - that probe has no staging step at all; only the byte-copy property is inherited Council record: .claude/plans/hydrate-crate-hardening-council-v1.md (5 savants, 26 findings -> draft v2, 20 decisions -> 3 reviewers, 6 FIX, zero BLOCK -> this commit). Board hygiene: new dated LATEST_STATE.md entry correcting the three wrong claims in the original 2026-08-17 entry (append-only - that entry is left unedited). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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 |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: serverGenReqId_760b927b-fd2e-4bdc-9f39-927ddeba27ad) |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Closes the one gap left by the council's own Phase 4: the dilution-collapse-sentinel's P1 on C5 asked for a durable ISSUES.md pointer for the deferred hydrate_aside_then_publish merge, and that was addressed only in lib.rs's crate doc. Code docs travel with the code but are not where this repo tracks open work; a plan file's own status changes once the council closes. Three entries, each naming why it was deferred rather than just that it was: - ISS-HYDRATE-ENV-READER-IS-A-SECOND-COPY-OF-DEV-S3-ENV: the drift risk is live and unenforceable; the exit is lance-graph re-exporting FROM the leaf crate, not the leaf depending on the spine - ISS-HYDRATE-DIR-AND-FILE-DUPLICATE-THEIR-STAGING-BODIES: the audit surface stays doubled; cheap to merge now only while consumers are zero - ISS-HYDRATE-NAME-COLLIDES-WITH-TWO-EXISTING-WORKSPACE-MEANINGS: records that "module paths disambiguate" answers a narrower question than the finding raised Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NMeiLmtDKhomJNSo2ecbJw
Summary
PR #957 (the new
lance-graph-hydratecrate) merged mid-council — before Phase 4 (fix) landed. This PR applies the council's findings as a fast-follow rather than reopening/amending #957.Process: Full 5+3 council (
.claude/agents/5+3-council.md) on the crate: Phase 0 spec → Phase 1 (5 savants in parallel, 26 distinct findings) → Phase 2 (consolidated into 20 committed decisions) → Phase 3 (3 reviewers on the consolidated draft, zero BLOCK, 6 FIX) → Phase 4/5 (this commit). Full record:.claude/plans/hydrate-crate-hardening-council-v1.md.Real defects found and fixed
pid+nanosalone can repeat within one clock tick — two concurrent calls could interleave writes into what each believed was its own private staging directory. New sharedstaging::staging_suffix()(pid + atomic counter + nanos), 1000-iteration uniqueness test.hydrate_dirnorhydrate_filecleaned up their staging directory/file on a fetch/write error — only the two explicitly-handled paths (empty-prefix, checksum-mismatch) did. Now every path cleans up.hydrate_dir's empty-prefix cleanup discarded its own error on theOkreturn path, so a filesystem failure there could leave debris while the function still claimed "leaves nothing." Now propagates.hydrate_file's actual danger case was unguarded: the existingAlreadyPublishedcheck only ran at function entry. On POSIX, a file-onto-filerenamesilently clobbers an existing destination rather than failing — so the one check that mattered (immediately before the rename) didn't exist. Fixed.release_dircould never returnErr: every failure was swallowed, making its own "missing directory returns zero, not an error" test vacuous by this repo's own falsifiability rule (nothing could make it fail). Now returnsErron a genuinely unreadable root,Ok(0)only forNotFound, with a real two-sided test pair.WarmMarker's format had no version tag and permissive arity (the>= Nshape this repo's CLAUDE.md specifically warns against). Nowv1 <mtime> <len>, exact 3-token arity, both a legacy-shaped 2-token line and a future 4-token line refused by tests.Overclaims corrected (doc-only)
LifecycleStatewas claimed "encoded as a transition guard, not caller discipline" — false, it appeared in zero function signatures anywhere in the crate. Corrected, anddirty::lifecycle_of()was added to actually start closing that gap.is_dirtywas claimed to compare against "the version recorded at hydration time" — false, nothing records it; the version is and remains caller-supplied.copy.rs's module doc overclaimed the publish-by-rename mechanism was "generalized fromhydration_probe.rs's proven mechanism" — that probe has no staging step at all; only the raw byte-copy property is actually inherited from it.Board hygiene
New dated
LATEST_STATE.mdentry correcting the three wrong claims made in the original (now-merged) 2026-08-17 entry — that entry is left unedited per this repo's append-only rule.Verification status
Same honest caveat as #957: this session's container cannot build the
lance/arrow/aws-lc-rssub-tree locally (repeatedENOSPC). The pure-std modules (lifecycle.rs,staging.rs,marker.rs's non-test code) were syntax/type-checked directly viarustc --emit=metadata— clean. Thelance/object_store-touching changes were not locally compiled; deferred to this repo's CI, which I'll watch and fix on any failure.Test plan
rust-test.ymlet al.) compiles and runs the crate's now-larger test suitecargo clippy -p lance-graph-hydrateclean🤖 Generated with Claude Code
Generated by Claude Code