Repository navigation
feat(storage): version payload checksums and retire legacy verification - #1640
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: CurateLabs/graphforge/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (53)
💤 Files with no reviewable changes (6)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughGraph payload inventory and compact manifest formats now persist XXH64 checksums. Capture, installation, and admission paths use these checksums with exact-length checks. Legacy graph snapshots and standalone project verification are removed. ChangesGraph payload integrity
Legacy snapshot rejection
Standalone verification retirement
Priority: ⬇️ Low Estimated code review effort: 4 (Complex) | ~50 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant GraphFilesInventory
participant GraphObjectStore
participant CompactManifest
participant PayloadReader
GraphFilesInventory->>GraphObjectStore: Install payload and capture SHA-256 and XXH64
GraphObjectStore->>CompactManifest: Provide installed payload checksum
CompactManifest->>PayloadReader: Supply checksum and exact length
PayloadReader->>PayloadReader: Validate payload checksum and length
Merge Risk: ⚪ Minimal · up to The change introduces checksummed payload admission and intentionally retires legacy formats and standalone verification while retaining portable verification. No concrete blocker remains; merge after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The inspected paths retain SHA-256 checks for object installation and portable-package verification, while using XXH64 for subsequent corruption detection. No attacker-reachable authentication bypass was established. The main design risks are format compatibility and reliance on a trusted local object store; incomplete coverage prevents a minimal-risk assessment. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
Hardening Proposals
🚥 Pre-merge checks | ✅ 3 | ❌ 1 | ❓ 1❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
Full details: Linked Issues checkExplanation The PR implements the main coding objectives in Resolution Provide reviewable evidence for the required ADR and architecture-note content, or make those documents available to this assessment. The evidence must cover the format versions, checksum versus authentication responsibilities, trust assumptions, and the pre-v1 retirement decision. Full details: Docstring CoverageExplanation Docstring coverage is 59.48% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 153 functions across 45 files. (2 skipped: 2 unsupported.)
✨ 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 |
|
Heads-up from a parallel session: ADR number 0048 is also claimed by PR #1647 ( |
|
Independent integration review at 36f7906 reproduced the two current CI roots:
The previous four Clippy errors are fixed at this head. The current-only checksum/retirement implementation passes the static acceptance review; remaining safe CI lanes are still running. There is also an integration collision: #1647 adds ADR 0048 for the build authority, so this PR must use the next available ADR number and regenerate references/indices after that prerequisite record lands. |
|
Reviewed |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
The dangling Bazel target is fixed in The final head |
…ayload-checksums # Conflicts: # docs-site/astro.config.mjs # docs-site/scripts/sync-content.mjs # docs/adr/README.md # docs/engineering/adrs/README.md
|
ADR #1647 has merged to main737df3dced71de25688f116447731c31241372af and #1648 is closed. This PR now has the expected ADR-index merge conflict. I am resolving only that integration in my isolated |
|
ADR 0048 is now taken on |
|
The author concurrently pushed the main integration as |
Description
Published graph payloads previously required repeated SHA-256 reads to refuse same-inode, same-length corruption. Versioned graph/files formats now persist mandatory seed-zero XXH64 checksums and use them with exact length for payload admission. SHA-256 still names CAS objects and authenticates control metadata, receipts, and existing trust boundaries.
The maintainer's pre-v1 policy retires standalone
gf verifyand legacy graph formats. This removes compatibility readers, SHA-256 admission fallbacks, the publication checksum upgrade path, and the whole-store forensic scanner. Portable-package verification and internal construction rollback snapshots remain required.Related Issues
Fixes #1637.
The canonical policy owner is #1617. Its broader digest inventory, opt-in diagnostics, identity caching, and complete import/query/export counter requirements remain open.
Changes
u64values encoded as exactly 16 lowercase hex digits. Patricia branches retain version 3; buckets require version 4.portable verify.CURRENTafter rejected snapshot publication. Update fixtures to current checksums and versions.Validation
cargo clippy -p graphforge-storage -p graphforge-api -p graphforge-cli -- -D warnings: passed locally.cargo fmt --all -- --check,git diff --check,make pre-push-fast, ADR index and docs-tree checks: passed locally.python3 scripts/ci/test-non-cypher-surface-gate.py: 13 passed. The ledger gate passes with 453 public methods, 94 algorithms, and 22 search contracts.d046140e43c566e61635e2771353b54faea95d7c: full workspace nextest, custom-harness tests, doctests, real tiny/ownership-growth lifecycle execution, bindings, contract lanes, native durability, and CI Gate. CodeRabbit completed its GitHub review with no actionable comments; independent final review also found no remaining blocker. Main subsequently gained ADR 0048; its generated-index conflict is resolved by regeneration, retaining both records exactly once. Independent review confirms that the runtime files are identical to the reviewed/testedd046140e4head. The refreshed13cc28af27c616174e0ae830773eb3a4421392eehead passed the complete exact-head CI Gate and passed the merge-group CI run. The squash commitaf93b16ff55d7b45cfa9ccd448f65864948226b8is verified on main and feat(storage): version published graph payload checksums for read-time corruption refusal #1637 is closed with all acceptance criteria met.Format policy
Pre-v1 projects using retired formats must be recreated. No legacy compatibility reader or in-place format migration is supplied. Checksums detect accidental corruption under the existing threat assumptions; they do not establish cryptographic identity. Ordinary payload admission still reads bytes for checksum validation. This PR does not claim zero SHA-256 across the entire facade/query path or change fsync policy.