Define the durable segment store protocol - #54
Conversation
|
Warning Review limit reached
Next review available in: 14 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (19)
WalkthroughThe change defines ChangesDurable Segment Store v1
Estimated code review effort: 4 (Complex) | ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
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 |
Self-review findings
All focused regressions, the complete debug/release Rust matrix, fuzz workspace, dependency policy/audits, Markdown, links, workflows, source structure, and diff checks are green at @codex please confirm the protocol and remediation queue, with particular scrutiny on crash-state completeness, bounded recovery, physical namespace assumptions, and golden-format compatibility. |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 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 `@docs/formats/segment-store-v1/publication.md`:
- Around line 141-147: Define crash-recovery and reuse/finalization rules for
leftover head.next in the publication protocol around KEEP-CRASH-021 through
KEEP-CRASH-026, ensuring a prior interrupted attempt cannot block the next
publication. Update docs/formats/segment-store-v1/publication.md lines 141-147
accordingly, and update docs/formats/segment-store-v1/recovery.md lines 54-73 to
either add a root-level head.next unlink followed by root-directory
synchronization or route verified head.next recovery through the existing safe
finalization path.
- Around line 41-43: Update the publication flow described before the
immutable-pool link to eliminate the verification-to-link race: after creating
the digest-derived pool entry, revalidate that the linked artifact’s
content/digest still matches the verified staged segment and canonical catalog,
including idempotent reuse cases. Alternatively, explicitly enforce and document
that the staging root cannot be modified outside the writer; do not treat a
mismatched link as successful.
In `@docs/formats/segment-store-v1/recovery.md`:
- Around line 54-66: Update the recovery discard flow described in the “Discard”
protocol to enforce the artifact-specific maximum before computing
stage_fingerprint: use MAX_SEGMENT_LENGTH, MAX_CATALOG_LENGTH, or
PUBLICATION_HEAD_LENGTH according to the stage type, and enforce the limit
across the complete bounded stream. Reject and explicitly classify oversized
evidence before hashing, while preserving the existing canonical-name,
observed-length, and digest validation behavior for inputs within the limit.
In `@xtask/tests/layout_format_contract.rs`:
- Around line 81-89: Extend format_registry_reports_flat_layout_as_implemented
to assert the Durable Segment Store v1 registry row remains present with its
expected coordinate, planned status, and corpus link, using the exact symbols
and wording established in docs/formats/README.md.
In `@xtask/tests/segment_store_protocol_contract.rs`:
- Around line 214-231: Update protocol_index_routes_each_semantic_owner to
validate exact Markdown link targets for each required specification page rather
than using SPECIFICATION_INDEX.contains(page). Assert the expected link syntax
and target paths so prose or unrelated filename substrings cannot satisfy the
routing checks.
- Around line 189-199: Update
conformance_provenance_has_one_issue_prefix_per_owner to parse the normalized
CONFORMANCE_ORIGIN records, group or inspect entries by owner, and assert that
each owner has exactly one issue reference. Replace the narrow adjacent “Issue
Issue” substring check while preserving the existing normalization and clear
assertion failures.
- Around line 114-136: Update the exact-transition assertions in the test loop
over TRANSITIONS so every expected literal contains all seven TSV fields,
including post_state and recovery_posture. Compare complete rows rather than
using substring containment, while preserving the existing transition
identifiers and expected values.
In `@xtask/tests/segment_store_protocol_contract/fixture_oracle.rs`:
- Line 60: Replace the hard-coded catalog lengths passed to build_head at both
call sites with the length derived from the Catalog bytes field produced by
build_catalog. Ensure the derived value remains synchronized with the catalog
header length while preserving the existing head construction flow.
In
`@xtask/tests/segment_store_protocol_contract/fixture_oracle/bundle_encoding.rs`:
- Around line 144-170: Update hex_nibble in the decode_hex path to accept only
ASCII digits 0–9 and lowercase letters a–f, rejecting uppercase A–F and all
other bytes with the existing nonhexadecimal error. Keep decode_hex’s canonical
whitespace, suffix, and length validation unchanged.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 8d61be14-bf06-47ed-883f-9f10cb39e2c2
⛔ Files ignored due to path filters (2)
conformance/segment-store/v1/artifacts.tsvis excluded by!**/*.tsvconformance/segment-store/v1/transitions.tsvis excluded by!**/*.tsv
📒 Files selected for processing (26)
CHANGELOG.mdconformance/segment-store/v1/ORIGIN.mdconformance/segment-store/v1/README.mdconformance/segment-store/v1/empty-segment.hexconformance/segment-store/v1/one-zero-bundle-catalog.hexconformance/segment-store/v1/one-zero-bundle-head.hexconformance/segment-store/v1/one-zero-bundle-segment.hexconformance/segment-store/v1/one-zero-catalog.hexconformance/segment-store/v1/one-zero-head.hexconformance/segment-store/v1/one-zero-segment.hexdocs/adr/0005-durable-segment-store-protocol.mddocs/adr/README.mddocs/formats/README.mddocs/formats/segment-store-v1/README.mddocs/formats/segment-store-v1/catalog.mddocs/formats/segment-store-v1/publication.mddocs/formats/segment-store-v1/rationale.mddocs/formats/segment-store-v1/recovery.mddocs/formats/segment-store-v1/requirements.mddocs/formats/segment-store-v1/segment.mdxtask/tests/layout_format_contract.rsxtask/tests/segment_store_protocol_contract.rsxtask/tests/segment_store_protocol_contract/fixture_oracle.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/bundle_encoding.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/encoding.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/fixture_assertion.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Rust quality gates
- GitHub Check: Documentation and workflow integrity
- GitHub Check: Runtime fuzz smoke
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
Use stable, pinned Rust with edition 2024;
cargo fmt --checkand Clippy with-D warningsmust pass.
Files:
xtask/tests/segment_store_protocol_contract/fixture_oracle/fixture_assertion.rsxtask/tests/layout_format_contract.rsxtask/tests/segment_store_protocol_contract/fixture_oracle.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/encoding.rsxtask/tests/segment_store_protocol_contract.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/bundle_encoding.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Do not useunwrap,expect,panic!,todo!,unimplemented!,dbg!, stdout/stderr printing, unchecked indexing, lossy casts, or unsafe code. Unsafe requires a dedicated crate with documented invariants, tests, and measured necessity.
Use checked arithmetic for externally influenced values andTryFrom/try_frominstead of potentially lossyasconversions.
Public functions must not take boolean parameters; use enums instead. Prefer typed newtypes for IDs, lengths, offsets, generations, and namespaces.
Parse and validate untrusted bytes before admitting them as trusted types; make illegal states unrepresentable where practical.
Prefer concrete types, synchronous core APIs, and traits only at real substitution boundaries; do not introduce async without demonstrated consumer need.
Do not let HashMap iteration order affect identity, serialization, tests, or behavior.
Use hexagonal architecture: domain and port modules own invariants and semantic capabilities, dependencies point inward, and core/ports must not import adapters, filesystems, networks, CLIs, runtimes, or application policy.
Keep codecs at ingress and egress boundaries. Ports exchange semantic or validated types, not serializer-owned JSON, CBOR, or wire values.
Keep files near 200 lines, review files over 300, and do not exceed 500; keep functions near 20 logical lines, review over 40, do not exceed 60; limit nesting to 3 and parameters to 5.
Do not create genericutils.rs,helpers.rs,common.rs,misc.rs,shared.rs,manager.rs,service.rs,types.rs, ormodels.rs; name files after the concept they own.
Keep items private by default; usepub(crate)unless external consumers require public visibility. Public validated types must have private fields and checked constructors, and consequential staged work, plans, commits, and results should use#[must_use].
UseOptionfor normal absence, typed errors by boundary, preserved error sources, and expected/observed state in...
Files:
xtask/tests/segment_store_protocol_contract/fixture_oracle/fixture_assertion.rsxtask/tests/layout_format_contract.rsxtask/tests/segment_store_protocol_contract/fixture_oracle.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/encoding.rsxtask/tests/segment_store_protocol_contract.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/bundle_encoding.rs
**/*.{rs,json,cbor}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,json,cbor}: Identity, persistence, comparison, tests, and protocol behavior must be deterministic and independent of iteration order, host state, clocks, locale, and serializer defaults.
Boundary JSON and CBOR must use a named canonical profile with golden fixtures; reject duplicate fields and noncanonical identity-bearing encodings. Hash only typed, domain-separated canonical bytes.
Files:
xtask/tests/segment_store_protocol_contract/fixture_oracle/fixture_assertion.rsxtask/tests/layout_format_contract.rsxtask/tests/segment_store_protocol_contract/fixture_oracle.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/encoding.rsxtask/tests/segment_store_protocol_contract.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/bundle_encoding.rs
**/*.{rs,bin}
📄 CodeRabbit inference engine (AGENTS.md)
Durable formats must define magic bytes, versioning, canonical encoding, explicit endianness, bounds, checksums, and golden fixtures; reject trailing bytes, duplicate fields, noncanonical encodings, overflow, invalid ordering, excessive depth, and unknown mandatory flags. Do not use arbitrary Serde or Rust struct output as a durable format.
Files:
xtask/tests/segment_store_protocol_contract/fixture_oracle/fixture_assertion.rsxtask/tests/layout_format_contract.rsxtask/tests/segment_store_protocol_contract/fixture_oracle.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/encoding.rsxtask/tests/segment_store_protocol_contract.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/bundle_encoding.rs
docs/adr/**/*.md
📄 CodeRabbit inference engine (AGENTS.md)
Cross-subsystem decisions affecting identity, formats, durability, recovery, concurrency, GC, encryption, or public compatibility require a slugged ADR under
docs/adr/, never a bare number.
Files:
docs/adr/README.mddocs/adr/0005-durable-segment-store-protocol.md
**/rationale.md
📄 CodeRabbit inference engine (AGENTS.md)
Decisions affecting identity, formats, durability, recovery, concurrency, GC, encryption, or public compatibility require a colocated
rationale.mdfor one-page scope.
Files:
docs/formats/segment-store-v1/rationale.md
🧠 Learnings (1)
📚 Learning: 2026-07-27T22:37:16.896Z
Learnt from: flyingrobots
Repo: flyingrobots/keep PR: 49
File: src/layout/record_length.rs:29-29
Timestamp: 2026-07-27T22:37:16.896Z
Learning: This repository targets Rust 1.96 (per `Cargo.toml` `rust-version` and `rust-toolchain.toml`). When writing or reviewing Rust code, only use APIs/language features stabilized in Rust 1.96 or earlier. Avoid using newer std/library APIs that wouldn’t be available on Rust 1.96 (e.g., you may rely on `u64::is_multiple_of` since it’s stabilized by 1.96).
Applied to files:
xtask/tests/segment_store_protocol_contract/fixture_oracle/fixture_assertion.rsxtask/tests/layout_format_contract.rsxtask/tests/segment_store_protocol_contract/fixture_oracle.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/encoding.rsxtask/tests/segment_store_protocol_contract.rsxtask/tests/segment_store_protocol_contract/fixture_oracle/bundle_encoding.rs
🪛 LanguageTool
conformance/segment-store/v1/ORIGIN.md
[style] ~25-~25: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ... same for catalog and head publication. Issue #17 must implement the transition ledge...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
docs/adr/0005-durable-segment-store-protocol.md
[style] ~66-~66: Consider replacing this word to strengthen your wording.
Context: ...ation is an explicit fallible operation and never occurs from Drop. Version 1 us...
(AND_THAT)
🔇 Additional comments (26)
docs/adr/0005-durable-segment-store-protocol.md (1)
1-159: LGTM!docs/formats/segment-store-v1/README.md (1)
1-53: LGTM!docs/formats/segment-store-v1/segment.md (1)
1-174: LGTM!docs/formats/segment-store-v1/catalog.md (1)
1-169: LGTM!docs/formats/segment-store-v1/rationale.md (1)
1-148: LGTM!docs/formats/segment-store-v1/requirements.md (1)
1-84: LGTM!CHANGELOG.md (1)
65-72: LGTM!docs/adr/README.md (1)
59-59: LGTM!docs/formats/README.md (1)
10-11: LGTM!xtask/tests/layout_format_contract.rs (1)
5-5: LGTM!xtask/tests/segment_store_protocol_contract.rs (4)
1-113: LGTM!
137-188: LGTM!
200-213: LGTM!
232-240: LGTM!conformance/segment-store/v1/ORIGIN.md (1)
1-30: LGTM!conformance/segment-store/v1/README.md (1)
1-90: LGTM!conformance/segment-store/v1/empty-segment.hex (1)
1-1: LGTM!conformance/segment-store/v1/one-zero-bundle-catalog.hex (1)
1-1: LGTM!conformance/segment-store/v1/one-zero-bundle-head.hex (1)
1-1: LGTM!xtask/tests/segment_store_protocol_contract/fixture_oracle/bundle_encoding.rs (1)
1-142: LGTM!conformance/segment-store/v1/one-zero-bundle-segment.hex (1)
1-1: LGTM!conformance/segment-store/v1/one-zero-catalog.hex (1)
1-1: LGTM!conformance/segment-store/v1/one-zero-head.hex (1)
1-1: LGTM!conformance/segment-store/v1/one-zero-segment.hex (1)
1-1: LGTM!xtask/tests/segment_store_protocol_contract/fixture_oracle/encoding.rs (1)
1-201: LGTM!xtask/tests/segment_store_protocol_contract/fixture_oracle/fixture_assertion.rs (1)
1-76: LGTM!
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2f5b184e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Additional self-review finding
@codex please confirm the ownership split once the follow-up commit lands. |
|
To use Codex here, create an environment for this repo. |
Activity Summary — second review passAll six fresh PR findings and three follow-on self-audit findings were handled as isolated RED → GREEN → VERIFY → COMMIT loops. The exact remote head is
Verification is green locally: full debug/release/doc test matrix; all/no-default-feature checks; Clippy with @codex Please confirm the remote head has no remaining correctness, recovery, determinism, format, or structure defect. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7d59bf3e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| KEEP-CRASH-002 segment write-header empty-segment-stage truncated-tail-or-reusable-stage reusable-segment-stage verify-prefix | ||
| KEEP-CRASH-003 segment append-record reusable-segment-stage truncated-tail-or-reusable-stage reusable-segment-stage verify-prefix | ||
| KEEP-CRASH-004 segment flush-record-prefix reusable-segment-stage truncated-tail-or-reusable-stage flushed-segment-stage verify-prefix | ||
| KEEP-CRASH-005 segment sync-record-prefix flushed-segment-stage truncated-tail-or-reusable-stage durable-reusable-segment-stage verify-prefix |
There was a problem hiding this comment.
When power loss occurs at KEEP-CRASH-005, the staged file may have synchronized contents, but staging has never been synchronized since current.seg was created, so the directory entry may disappear entirely; neither truncated-tail-or-reusable-stage nor durable-reusable-segment-stage admits that lawful outcome. The same gap affects the catalog stage before KEEP-CRASH-018, so recovery or crash-conformance code can reject a clean pre-transaction state as ambiguity. Either synchronize the parent directory after exclusive stage creation or admit an absent-stage outcome until the first pool-directory synchronization.
AGENTS.md reference: AGENTS.md:L83-L88
Useful? React with 👍 / 👎.
| KEEP-CRASH-022 head write-next-head empty-next-head truncated-tail-or-valid-next-head valid-next-head verify-generation-and-catalog | ||
| KEEP-CRASH-023 head flush-next-head valid-next-head truncated-tail-or-valid-next-head flushed-next-head verify-generation-and-catalog | ||
| KEEP-CRASH-024 head sync-next-head flushed-next-head truncated-tail-or-valid-next-head durable-next-head verify-generation-and-catalog | ||
| KEEP-CRASH-025 head replace-current-head durable-next-head valid-next-head-or-published-generation-or-ambiguity replaced-current-head verify-one-atomic-head |
There was a problem hiding this comment.
If the writer process dies after KEEP-CRASH-025 replaces HEAD but before KEEP-CRASH-026 synchronizes the root, the valid new HEAD remains visible in the kernel while head.next is absent, making the namespace indistinguishable from the synchronized post-state to the read-only opener. Classifying this as published-generation after content verification therefore admits a generation whose directory entry can still roll back on later power loss; publication needs persistent synchronization evidence or a writer-locked recovery root sync before reader admission rather than treating a valid visible head as durable.
AGENTS.md reference: AGENTS.md:L80-L88
Useful? React with 👍 / 👎.
| refusal when the count exceeds `MAX_RECOVERY_INVENTORY_ENTRY_COUNT`; because | ||
| it stops on the first excess entry, the refusal reports an observed-at-least | ||
| count of `2,097,153`, not a host-order-dependent exact total. A configured | ||
| limit may be lower but never higher. |
There was a problem hiding this comment.
When the inventory is just below either the protocol cap or a lower configured cap, the writer's pre-publication recovery scan can pass and then the segment/catalog stage and pool links can push the namespace over the limit; publication may return success, or a crash may retain the lawful stage/pool duplicate at that peak. The next recovery then stops on the first excess entry before it can classify or complete that state, and version 1 forbids deleting the valid artifacts that caused it. Reserve capacity for the transaction's peak transient and final entry counts before any mutation, with a typed capacity refusal when it cannot fit.
AGENTS.md reference: AGENTS.md:L84-L88
Useful? React with 👍 / 👎.
Problem
Keep has exact logical identity but no accepted physical protocol for proving what survives a crash or which immutable records form one published view.
Invariant
A published identity resolves to exactly its verified bytes through one durable head and catalog, or the store refuses.
Approach
Define
keep.segment-store/v1as one contract covering canonical segment/catalog/head bytes, bounded recovery inventory, one-writer publication, 28 stable crash boundaries, explicit recovery, platform refusal, and implementation-independent golden artifacts.Alternatives rejected
One-file-per-chunk storage, embedded databases as the protocol, Git objects, one monolithic append log, presence-based stale locks, automatic repair, and directory-enumeration publication.
Failure modes
The protocol distinguishes reusable staging, valid orphans, truncated tails, corrupt sealed state, stale generations, and unrecoverable ambiguity. Path-aliasing and unproven filesystem semantics fail closed.
Tests
cargo denyandcargo auditfor both lockfilesBenchmark impact
None. This PR adds protocol, corpus, and contract evidence only; production I/O remains in issues #15–#17.
Format/API compatibility
This accepts the new version-1 physical byte and state-machine commitments. It changes no shipped Rust public API or existing logical identity format.
Recovery implications
Recovery is observational until an explicit executor runs. Destructive truncated-stage discard has its own fingerprint and crash-safe unlink/directory-sync boundaries.
Security implications
Checksums provide integrity, not authentication or confidentiality. Counts, lengths, paths, and recovery scans are bounded; symlinks, path aliases, and unsupported filesystems are refused.
Closes #14
Closes #53