feat(kernel): strand contract — cycle 0004#315
Merged
flyingrobots merged 11 commits intomainfrom Apr 4, 2026
Merged
Conversation
Move KERNEL_strand-contract from up-next into docs/design/0004-strand-contract/ with a full design doc. Defines Strand, BaseRef, SupportPin, StrandLifecycle, StrandRegistry. Six invariants (INV-S1 through INV-S6). TTD mapping to LaneKind::STRAND and LaneRef.parentId. Implementation in warp-core/src/strand.rs.
Address all MUST items from design review: 1. Kill lifecycle/head-state duplication. No StrandLifecycle field. Strand either exists in registry (live) or doesn't (dropped). Operational state derived from heads — single source of truth. 2. Hard-delete drop semantics. No tombstone, no Dropped state. drop_strand returns a DropReceipt (strand_id, child_worldline_id, final tick). get() returns None after drop. 3. Pin BaseRef exactly. fork_tick is last included tick. commit_hash is at fork_tick. boundary_hash is output boundary (state root after applying patch). All five fields same coordinate. Add provenance_ref for substrate-native lookups. 4. Add four invariants: INV-S7 (distinct worldlines), INV-S8 (head ownership), INV-S9 (empty support_pins in v1), INV-S10 (clean drop — no runnable heads remain). 5. Require integration test proving strand heads excluded from live scheduler runnable set. 6. Define create/drop atomicity with rollback sequence. 7. Document registry ordering, writer_heads cardinality-1 in v1, get/contains API surface. 8. Fix TTD mapping: LaneKind is type not lifecycle.
Add crates/warp-core/src/strand.rs with: - StrandId (domain-separated hash, prefix "strand:") - BaseRef (immutable fork coordinate with exact semantics) - SupportPin (braid geometry placeholder, empty in v1) - DropReceipt (hard-delete proof) - Strand (relation descriptor, no lifecycle field) - StrandRegistry (BTreeMap<StrandId, Strand> with CRUD) - StrandError enum Add integration tests (14 assertions, all passing on types/registry): - INV-S2/S8: head ownership - INV-S4/S10: dormant heads excluded from runnable set - INV-S5: base_ref field consistency - INV-S7: distinct worldlines - INV-S9: empty support_pins - Registry: insert, get, duplicate, remove, list_by_base Shell invariant tests: 12/12 RED (invariant doc not yet written).
Write docs/invariants/STRAND-CONTRACT.md with ten normative invariants (INV-S1 through INV-S10) covering immutable base_ref, own heads, session scope, manual tick, complete base_ref, inherited quantum, distinct worldlines, head ownership, empty support_pins, and clean drop semantics. All tests passing: 14 Rust integration tests + 12 shell assertions.
Retro and witness for cycle 0004. All 26 tests passing (14 Rust + 12 shell). Design review caught and fixed three issues: lifecycle/ head-state duplication, drop semantics inconsistency, and BaseRef coordinate imprecision. New debt: orchestrated create/drop operations not yet wired through coordinator, LocalProvenanceStore lacks remove_worldline.
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 17 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughIntroduces a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be444a7326
ℹ️ 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".
Comment on lines
+199
to
+203
| pub fn insert(&mut self, strand: Strand) -> Result<(), StrandError> { | ||
| if self.strands.contains_key(&strand.strand_id) { | ||
| return Err(StrandError::AlreadyExists(strand.strand_id)); | ||
| } | ||
| self.strands.insert(strand.strand_id, strand); |
There was a problem hiding this comment.
StrandRegistry::insert only checks duplicate strand_id and accepts any other shape of Strand, so callers can register entries that violate the contract (for example child_worldline_id == base_ref.source_worldline_id, writer_heads from another worldline, or non-empty support_pins in v1). Because downstream code will treat registry contents as authoritative, this can silently corrupt strand topology/scheduling assumptions and break INV-S7/INV-S8/INV-S9 despite the module docs saying these invariants are enforced.
Useful? React with 👍 / 👎.
- Remove redundant `pub use echo_runtime_schema::HeadId` from strand.rs (already re-exported from lib.rs via head module) - Add PartialEq + Eq derives to Strand struct (all fields support it) - Add happy-path provenance fork integration test: commit 3 ticks, fork at tick 1, verify child has correct prefix length and base_ref fields match the provenance coordinate exactly
…ippy Add invariant validation to StrandRegistry::insert: - INV-S7: reject if child_worldline_id == source_worldline_id - INV-S8: reject if any writer head belongs to wrong worldline - INV-S9: reject if support_pins is non-empty in v1 Add StrandError::InvariantViolation variant. Add 3 tests proving invalid strands are rejected. Fix u64-to-u8 cast truncation warning in happy-path fork test (use u8 loop variable with explicit u64 conversion).
Owner
Author
|
Fixed in 034592a: |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/warp-core/src/strand.rs`:
- Around line 205-208: The remove method currently returns Option<Strand> which
silently accepts missing IDs; change pub fn remove(&mut self, strand_id:
&StrandId) -> Option<Strand> to return Result<Strand, StrandError> and return
Err(StrandError::NotFound(*strand_id)) (or appropriate clone) when the id is
absent to match StrandError::NotFound; update all callers of
StrandRegistry::remove (or the remove function) to handle the Result error path
so drop failures are surfaced explicitly.
- Around line 224-229: Introduce a zero-allocation iterator API and make the
Vec-allocating API opt-in: add a new method (e.g., iter_by_base or
strands_by_base_iter) that returns an iterator over &Strand (signature like pub
fn iter_by_base<'a>(&'a self, base_worldline_id: &'a WorldlineId) -> impl
Iterator<Item = &'a Strand>) implemented as self.strands.values().filter(move
|s| &s.base_ref.source_worldline_id == base_worldline_id); then rewrite
list_by_base to be a thin wrapper that calls the new iterator and collects into
a Vec (keeping existing behavior for callers that need materialization). This
keeps the Strands map scanning lazy/zero-allocation for hot paths while
preserving the existing Vec-producing convenience API.
- Around line 133-144: The Strand struct currently exposes public fields
allowing invalid instances into the registry; add a Strand::validate_v1(&self)
-> Result<(), StrandError> that enforces INV-S7 (child_worldline_id !=
base_ref.source_worldline_id), v1 writer_heads cardinality == 1, and INV-S9
(support_pins.is_empty()), add a StrandError::InvariantViolation variant, call
validate_v1() from the registry insertion path (the code around the insertion
that currently only enforces ID uniqueness) and reject inserts on validation
failure, and either make Strand fields private with a validated constructor or
ensure all public construction sites call validate_v1(); note INV-S8 (writer
head membership) should be checked in validate_v1 once WriterHeadKey exposes a
worldline accessor.
In `@crates/warp-core/tests/strand_contract_tests.rs`:
- Around line 60-90: The tests currently use make_test_strand() which constructs
a perfectly valid Strand (with BaseRef, one writer_head and empty support_pins),
so INV-S2/S5/S7/S8/S9/cardinality checks only re-assert the fixture; change the
tests to either (A) stop calling these "contract-validation" tests or (B) add
negative and edge cases that exercise the real enforcement surface: create
Strand instances with invalid BaseRef/provenance (mismatched
provenance_ref.worldline_id or wrong commit_hash), with zero or multiple
writer_heads where cardinality forbids, with duplicate writer_heads, and with
non-empty support_pins that violate constraints, and then pass those instances
into the actual validation routine in warp_core::strand (e.g., call the module's
validate function such as validate_strand or Strand::validate) to assert the
validator rejects them rather than relying on the helper fixture.
- Around line 386-417: The test currently builds BaseRef from child_entry and
compares fields to the child copy; instead fetch the source entry via
store.entry(base_id, wt(1)) (e.g., let base_entry = store.entry(base_id,
wt(1)).expect(...)) and assert that base_entry.expected.commit_hash and
base_entry.expected.state_root equal the BaseRef.commit_hash and
BaseRef.boundary_hash respectively, and also assert
base_entry.expected.commit_hash/ state_root equal the
child_entry.expected.commit_hash/state_root to ensure fork() didn’t rewrite
those on the child; keep existing checks on BaseRef.provenance_ref.worldline_id,
worldline_tick, and commit_hash.
In `@docs/method/retro/0004-strand-contract/retro.md`:
- Around line 36-42: Update the playback table and surrounding prose in retro.md
to accurately reflect only the APIs and behaviors currently implemented and
tested: remove or reclassify any claims that "create_strand" or full drop
cleanup are delivered when they are not (per the later note that the
orchestrated create/drop API and provenance cleanup are future work), and
instead list only what the tests actually cover (e.g., that create_strand
returns the expected fields as verified by the 14 Rust tests, base_ref pinning
as shown by inv_s5, strand head states and exclusion from runnable set as shown
by inv_s4 and inv_s4_s10, and that drop-related behavior covered by
registry_remove and drop_receipt tests is limited to what those tests assert).
Ensure the table rows (create_strand, drop) and the explanatory lines around
them reference the actual test witnesses (inv_s4, inv_s5, inv_s4_s10,
registry_remove, drop_receipt, 14 Rust tests) and remove any forward-looking or
incorrect "delivered" wording, adding a short note that orchestrated create/drop
API and provenance cleanup remain future work.
In `@docs/method/retro/0004-strand-contract/witness/rust-test-output.txt`:
- Around line 2-18: The recorded test output is stale: rerun the test suite for
crates/warp-core (which now has 15 #[test] functions, including
provenance_fork_happy_path_child_has_correct_prefix) and update
docs/method/retro/0004-strand-contract/witness/rust-test-output.txt to reflect
the new output (should show "running 15 tests" and include the new test name and
the final summary of 15 passed); ensure the new file content exactly matches the
fresh cargo test output for the current branch.
In `@scripts/tests/strand_contract_invariant_test.sh`:
- Around line 38-40: The loop uses grep -q "${code}" which allows substring
matches (e.g., INV-S1 matching INV-S10); update the check so it only matches the
whole invariant token by anchoring the pattern or using
fixed-strings+line-anchoring against the invariant file: modify the assert call
that wraps grep -q (referencing the loop variable "${code}", the assert
invocation, and the "${invariant}" file) to use an anchored match (for example
grep with -x or a ^...$ regex or grep -F -x) so each code is matched exactly and
no false positives occur.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 54b5f288-3dfa-4ea4-a6e5-76b06746734f
📒 Files selected for processing (10)
crates/warp-core/src/lib.rscrates/warp-core/src/strand.rscrates/warp-core/tests/strand_contract_tests.rsdocs/design/0004-strand-contract/KERNEL_strand-contract.mddocs/design/0004-strand-contract/design.mddocs/invariants/STRAND-CONTRACT.mddocs/method/retro/0004-strand-contract/retro.mddocs/method/retro/0004-strand-contract/witness/rust-test-output.txtdocs/method/retro/0004-strand-contract/witness/shell-test-output.txtscripts/tests/strand_contract_invariant_test.sh
crates/warp-core/src/strand.rs
Outdated
Comment on lines
+205
to
+208
| /// Removes a strand from the registry, returning it if it existed. | ||
| pub fn remove(&mut self, strand_id: &StrandId) -> Option<Strand> { | ||
| self.strands.remove(strand_id) | ||
| } |
Contributor
There was a problem hiding this comment.
remove silently accepts missing IDs; this weakens drop correctness.
Line 206 returns Option<Strand>, so a missing strand can be ignored accidentally. This is inconsistent with StrandError::NotFound (Lines 153-155) and makes drop-path failures easier to miss.
Proposed fix (explicit failure)
- pub fn remove(&mut self, strand_id: &StrandId) -> Option<Strand> {
- self.strands.remove(strand_id)
- }
+ pub fn remove(&mut self, strand_id: &StrandId) -> Result<Strand, StrandError> {
+ self.strands
+ .remove(strand_id)
+ .ok_or(StrandError::NotFound(*strand_id))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// Removes a strand from the registry, returning it if it existed. | |
| pub fn remove(&mut self, strand_id: &StrandId) -> Option<Strand> { | |
| self.strands.remove(strand_id) | |
| } | |
| /// Removes a strand from the registry, returning it if it existed. | |
| pub fn remove(&mut self, strand_id: &StrandId) -> Result<Strand, StrandError> { | |
| self.strands | |
| .remove(strand_id) | |
| .ok_or(StrandError::NotFound(*strand_id)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/warp-core/src/strand.rs` around lines 205 - 208, The remove method
currently returns Option<Strand> which silently accepts missing IDs; change pub
fn remove(&mut self, strand_id: &StrandId) -> Option<Strand> to return
Result<Strand, StrandError> and return Err(StrandError::NotFound(*strand_id))
(or appropriate clone) when the id is absent to match StrandError::NotFound;
update all callers of StrandRegistry::remove (or the remove function) to handle
the Result error path so drop failures are surfaced explicitly.
docs/method/retro/0004-strand-contract/witness/rust-test-output.txt
Outdated
Show resolved
Hide resolved
grep -q "INV-S1" would false-match INV-S10. Use anchored pattern that matches heading format or spaced reference format.
…tor API, source-based BaseRef test - StrandRegistry::remove now returns Result<Strand, StrandError> instead of Option, surfacing NotFound errors explicitly - Add iter_by_base() zero-allocation iterator; list_by_base() is now a thin collect() wrapper - Happy-path fork test now compares BaseRef against the SOURCE entry, not the child copy — catches incorrect rewrites by fork()
Retro playback table now accurately reflects 18 tests and notes that orchestrated create/drop APIs are future work. Witness files re-recorded from current branch.
Owner
Author
CodeRabbit Review — All Issues Addressed
All 18 Rust tests + 12 shell assertions passing. Zero clippy warnings. |
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Strand,BaseRef,SupportPin,DropReceipt,StrandRegistrytypes inwarp-core/src/strand.rsdocs/invariants/STRAND-CONTRACT.mdDesign decisions
StrandLifecyclefield — strand exists in registry = live, not in registry = gone. Heads are the single source of truth for operational state.DropReceipt. No tombstone.fork_tick= last included tick,commit_hashat fork_tick,boundary_hash= output boundary (state root after patch),provenance_reffor substrate-native lookups.support_pinsempty in v1 (INV-S9) — field exists to prevent future struct breakage.What this enables
LaneKind::STRANDandLaneRef.parentIdTest plan
RunnableWriterSetCloses cycle 0004.
Summary by CodeRabbit
Release Notes
New Features
Tests
Documentation