refactor: align Strand with static typestates and close gap analysis specs#545
Conversation
|
Warning Review limit reached
More reviews will be available in 36 minutes and 46 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughIntroduces compile-time causal posture typestates on ChangesStrand Typestates, Posture Gating, and Evolving Braids
Sequence Diagram(s)sequenceDiagram
participant Client as Settlement Caller
participant Strand as Strand<P>
participant SettlementService
participant BraidShell
Client->>Strand: try_into_shared()
alt posture == Shared
Strand-->>Client: Ok(Strand<Shared>)
Client->>Strand: plan(runtime, provenance)
Strand->>SettlementService: plan_with_policy(shared_strand, ...)
SettlementService->>SettlementService: shared_strand validation
SettlementService->>BraidShell: build_braid_shell(retention_posture)
BraidShell->>BraidShell: select Revealed(id) vs Sealed(commitment)
BraidShell-->>SettlementService: BraidShell
SettlementService-->>Strand: SettlementPlan
Strand-->>Client: Ok(SettlementPlan)
else posture != Shared
Strand-->>Client: Err(PostureMismatch)
Client->>SettlementService: plan_with_policy(dynamic_strand, ...)
SettlementService->>SettlementService: try_into_shared() fails
SettlementService-->>Client: NonSharedStrand { posture }
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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: d9460957f9
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/warp-core/src/coordinator.rs (1)
2828-2962: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a digest-lock regression test for the new
StrandError::Posturehashing path.The new posture obstruction hashing matrix is deterministic, but it is currently protected only by implementation details. A small tag rename here will silently change scheduler fault digests.
Suggested test addition
+ #[test] + fn runtime_error_fault_digest_includes_posture_obstruction_payload() { + let err = RuntimeError::Strand(StrandError::Posture( + PostureObstruction::MissingAdmissionScope { + posture: CausalPosture::Shared, + }, + )); + let digest = scheduler_error_cause_digest(&err); + + let mut expected = blake3::Hasher::new(); + expected.update(b"echo.scheduler-fault-cause.error"); + expected.update(b"strand"); + expected.update(b"posture"); + expected.update(b"missing-admission-scope"); + expected.update(&[CausalPosture::Shared.canonical_tag()]); + let expected: Hash = expected.finalize().into(); + + assert_eq!(digest, expected); + }As per coding guidelines,
crates/warp-core/**is the deterministic kernel, so hash-stability regressions should be pinned by tests.🤖 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 `@crates/warp-core/src/coordinator.rs` around lines 2828 - 2962, The new hashing code for StrandError::Posture variant and the hash_posture_obstruction function, along with its helper functions hash_causal_posture and hash_posture_derivation, lack regression test coverage to ensure hash stability. A small change to any of the tag constants (like the b"narrowing-refused" strings) would silently change the scheduler fault digests without detection. Add a digest-lock regression test in the appropriate test module that verifies the computed hash digests for StrandError::Posture and all PostureObstruction variants match expected pinned values, ensuring that any tag renames or hashing logic changes are intentional and tracked.Source: Coding guidelines
🤖 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 `@crates/warp-core/src/braid_shell.rs`:
- Around line 679-692: The duplicate detection logic in the pattern match within
check_unique_member_strands compares sealed member references only by their
blinded_commitment value, but sealed member identity should include both
blinded_commitment and authority. Update the sealed reference comparison branch
to extract and compare both the blinded_commitment and authority fields from
each BraidMemberRef::Sealed variant, ensuring that sealed members with the same
commitment but different authorities are not incorrectly treated as duplicates.
In `@crates/warp-core/src/braid.rs`:
- Around line 87-128: The fold method currently returns Result<Self, String>
which prevents callers from pattern-matching specific error types. Define a new
structured error enum (such as BraidFoldError with variants for
EmptyEventStream, MissingInitialCreation, DuplicateCreation, and
SequenceMismatch) and update the fold method's return type to Result<Self,
BraidFoldError>. Then replace all the string error returns in the fold method
with the appropriate enum variant constructors instead of creating String
errors.
- Around line 33-46: The documentation comment for the `members` field in the
Braid struct claims it is a "Set of current member references" but the
implementation uses a `Vec` which can contain duplicates. Either update the doc
comment on the `members` field to accurately reflect that it is an "Ordered list
of woven member references" instead of a set, or modify the `apply` method to
enforce uniqueness constraints when adding members to the Vec to match the set
semantics described in the documentation.
- Around line 68-85: The apply method has an integer overflow vulnerability at
the MemberWoven event handler where sequence_num is incremented with
`sequence_num + 1`. When sequence_num reaches u64::MAX, this unchecked addition
will panic in debug builds and wrap to zero in release builds, violating
determinism guarantees. Replace the unchecked addition with checked arithmetic
(such as checked_add) that will either safely handle the increment or return an
error that should be propagated appropriately, ensuring deterministic behavior
regardless of the input values.
- Around line 68-85: The `apply` method is public and accepts BraidEvent without
validating state transitions, allowing invalid operations like overwriting
braid_id multiple times, updating sequence_num out of order, or adding duplicate
members. Either restrict the method visibility from `pub` to `pub(crate)` to
limit direct calls, or add validation guards within the match arms: for
BraidCreated, verify braid_id hasn't been set; for MemberWoven, verify
sequence_num matches expected next_sequence_num and the member_ref isn't already
in self.members; for SettlementFinalized, verify the new settlement hasn't been
finalized already.
In `@crates/warp-core/src/coordinator.rs`:
- Line 3397: The pull request fails required gates and must pass all checks
before merging. Fix the cargo clippy violations: replace expect_used calls in
crates/warp-core/src/observation.rs and crates/warp-core/src/settlement.rs with
proper error handling (using Result types or the ? operator), address
large_types_passed_by_value violations by passing large types by reference, and
remove redundant_clone calls. Additionally, fix the ban-globals.sh check
failures by removing or refactoring install_contract_package pattern matches in
crates/warp-core/src/coordinator.rs and related files. Run cargo clippy
--all-targets and bash scripts/ban-globals.sh locally to identify all violations
and fix them systematically until both gates pass cleanly.
In `@crates/warp-core/src/proof.rs`:
- Around line 37-47: The verify method in ProofEnvelope is not performing actual
cryptographic proof verification. Currently it only checks if proof_bytes is
non-empty and validates the public_inputs_hash, but ignores the kind field and
does not actually verify the proof cryptographically. Fix this by adding logic
that uses the kind field to determine which proof verification function to call,
and invoke the appropriate cryptographic verification on the proof_bytes. This
will ensure that proofs are actually validated according to their type rather
than simply accepting any non-empty payload.
In `@crates/warp-core/src/settlement.rs`:
- Around line 1424-1436: The blinded_commitment computation in the
BraidMemberRef::Sealed branch lacks the required randomness for a cryptographic
commitment scheme. The hash currently only includes a constant prefix and the
strand_id, making it vulnerable to enumeration attacks. Modify the
commitment_hasher in the else branch (when strand_posture is not
CausalPosture::Shared) to include authority-derived entropy by updating the
hasher with the retention_posture.authority field before finalizing. This
incorporates the necessary randomness to provide the hiding property promised by
the BraidMemberRef::Sealed documentation.
In `@crates/warp-core/src/strand.rs`:
- Around line 257-281: The try_into_shared method in Strand<DynamicPosture>
returns NarrowingRefused when the actual posture does not match Shared, but this
error variant is semantically incorrect because this is not a narrowing
operation (Shared is broader than AuthorOnly)—it is a failed type assertion.
Replace the NarrowingRefused variant with a more appropriate error variant that
semantically describes a posture mismatch, such as PostureMismatch with actual
and expected fields, to correctly represent the failure mode where the strand's
actual posture does not match the requested Shared posture.
---
Outside diff comments:
In `@crates/warp-core/src/coordinator.rs`:
- Around line 2828-2962: The new hashing code for StrandError::Posture variant
and the hash_posture_obstruction function, along with its helper functions
hash_causal_posture and hash_posture_derivation, lack regression test coverage
to ensure hash stability. A small change to any of the tag constants (like the
b"narrowing-refused" strings) would silently change the scheduler fault digests
without detection. Add a digest-lock regression test in the appropriate test
module that verifies the computed hash digests for StrandError::Posture and all
PostureObstruction variants match expected pinned values, ensuring that any tag
renames or hashing logic changes are intentional and tracked.
🪄 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: 29d21db2-fe70-4346-9823-375913cc8ac8
📒 Files selected for processing (17)
CHANGELOG.mdbacklog/cool-ideas/CI-003-append-only-braid-membership.mdcrates/warp-core/src/braid.rscrates/warp-core/src/braid_shell.rscrates/warp-core/src/coordinator.rscrates/warp-core/src/lib.rscrates/warp-core/src/neighborhood.rscrates/warp-core/src/observation.rscrates/warp-core/src/proof.rscrates/warp-core/src/provenance_store.rscrates/warp-core/src/revelation.rscrates/warp-core/src/settlement.rscrates/warp-core/src/strand.rscrates/warp-core/tests/strand_contract_tests.rscrates/warp-wasm/src/lib.rscrates/warp-wasm/src/warp_kernel.rsdocs/design/0028-strand-typestates-proof-envelopes-and-evolving-braids/design.md
🐇 Code Rabbit Self-Code ReviewA pedantic, strict code audit has been performed on the diff between cc: @codex for verification.
Detailed Findings & Mitigations1. Spec/Code Mismatch in Design Document
2. String-based error type in
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e152afc8ad
ℹ️ 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".
|
SELF-REVIEW NOTES — BLOCKING I’m treating this as blocking feedback on invariant/security grounds. The direction is strong, but the current implementation does not yet enforce the claims the PR makes. Main blockers:
These are fixable, but they should be fixed before merge because this PR is explicitly about closing posture/proof/braid invariants, not just introducing the types. Detailed notes: 1. CausalPostureState / Strand is forgeableThe typestate boundary is not actually closed. CausalPostureState is public and unsealed, and Strand exposes both retention_posture and _marker. A downstream caller can construct a Strand whose runtime retention_posture.causal_posture is still AuthorOnly, then call Strand::plan/settle or public settlement internals. Fix: seal the typestate trait, make the marker/private fields inaccessible, and validate P::causal_posture() against retention_posture at construction/conversion time. Also defensively re-check inside plan_with_policy_internal and settle_with_policy_internal. The internal methods currently trust the type parameter too much. 2. ProofEnvelope::verify is not proof verificationverify() currently accepts any non-empty byte string when public_inputs_hash equals the expected witness hash. It ignores kind, does not dispatch to per-proof verification, and does not bind ObserverHonestyClaim. That is fine for a placeholder, but not under names/docs claiming cryptographic proof verification. Fix options:
3. Proof bytes are swappable without changing shell digestBraidShell stores proof: Option, and validate() checks it, but neither proof.kind nor proof.proof_bytes participates in witness_digest or digest. That means two shell values can have the same digest but different proof bytes. For digest-addressed retained records, that’s a footgun wearing a tuxedo. Fix: either hash a canonical proof/claim digest into the shell digest, or make proof evidence explicitly out-of-band and remove it from the digest-addressed BraidShell value. 4. Braid::apply bypasses fold invariantsfold() validates event order before calling apply(), but apply() itself is public and unchecked. Callers can:
Fix: make apply private or rename it to apply_unchecked, then expose: and have both fold() and public mutation use that path. 5. Braid::fold still accepts duplicate membersThe design says folding checks duplicate membership, and BraidShell rejects duplicate members, but Braid::fold accepts duplicate MemberWoven events for the same member_ref at different sequence numbers. Fix: add BraidError::DuplicateMember { member_ref } and reject a member already present in braid.members. Verdict: do not merge as-is. The architecture is the right shape, but this PR currently closes the gaps mostly at the vocabulary/API level, not at the invariant level. |
@codex can you please verify and if you agree, please formally request changes? I can't request changes on my own PR. |
|
@CodeRabbit please review Oops, sorry about that, I meant to @coderabbitai |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6df2b5c53d
ℹ️ 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".
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Reviewed independently. All five blocking concerns from the self-review are confirmed by the code.
1. Strand is forgeable. retention_posture and _marker are public fields, so any caller can struct-literal-construct Strand::<Shared> with a non-Shared posture and call plan/settle. Fix: make _marker private (or remove it in favor of a private constructor), seal the trait (sealed-trait pattern or pub-in-crate bound), and defensively re-check posture inside plan_with_policy_internal/settle_with_policy_internal.
2. ProofEnvelope::verify is not proof verification. The method only checks non-empty bytes and hash equality. Kind is ignored. Rename to validate_envelope_shape / validate_binding until per-kind cryptographic dispatch is implemented, or add a clear doc comment that this is a placeholder with no security guarantee.
3. Proof bytes are excluded from shell digest. compute_shell_digest and compute_witness_digest do not hash proof.kind, proof.proof_bytes, or proof.public_inputs_hash. Two shells with identical content but different proof bytes produce the same digest. Either include a canonical proof commitment in the digest, or move proof evidence entirely out-of-band and off the digest-addressed struct.
4. Braid::apply is public and unchecked. fold() validates before calling apply(), but apply is callable directly. A second BraidCreated resets status to Active. Weaving after Finalized and collapsing before Finalized are not guarded. Make apply private or rename it apply_unchecked, expose a pub fn try_apply(&mut self, event: BraidEvent) -> Result<(), BraidError> that both fold and public mutation use.
5. Braid::fold accepts duplicate member refs. Sequence-number monotonicity is enforced but member-ref uniqueness is not. Two MemberWoven events with the same member_ref at different sequence numbers pass validation. Add BraidError::DuplicateMember { member_ref: BraidMemberRef } and reject it in fold.
Already resolved in this PR (no action needed):
Braid::foldno longer returnsResult<Self, String>;BraidErrorwith thiserror exists.SEALED_MEMBER_DOMAINis a module-level const, not an inline literal.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat crates/warp-core/src/braid.rs
echo "---FILE_BREAK---"
cat crates/warp-core/src/proof.rs
echo "---FILE_BREAK---"
cat crates/warp-core/src/revelation.rs
echo "---FILE_BREAK---"
cat crates/warp-core/src/strand.rs
echo "---FILE_BREAK---"
cat crates/warp-core/src/braid_shell.rs
echo "---FILE_BREAK---"
# Check settlement internals
rg -n "plan_with_policy_internal\|settle_with_policy_internal\|CausalPostureState\|Strand<Shared>" crates/warp-core/src/settlement.rs -A 5Length of output: 179177 🏁 Script executed: #!/bin/bash
gh pr review 545 \
--repo flyingrobots/echo \
--request-changes \
--body "Reviewed independently. All five blocking concerns from the self-review are confirmed by the code.
**1. Strand<Shared> is forgeable.** \`retention_posture\` and \`_marker\` are public fields, so any caller can struct-literal-construct \`Strand::<Shared>\` with a non-Shared posture and call \`plan\`/\`settle\`. Fix: make \`_marker\` private (or remove it in favor of a private constructor), seal the trait (sealed-trait pattern or pub-in-crate bound), and defensively re-check posture inside \`plan_with_policy_internal\`/\`settle_with_policy_internal\`.
**2. ProofEnvelope::verify is not proof verification.** The method only checks non-empty bytes and hash equality. Kind is ignored. Rename to \`validate_envelope_shape\` / \`validate_binding\` until per-kind cryptographic dispatch is implemented, or add a clear doc comment that this is a placeholder with no security guarantee.
**3. Proof bytes are excluded from shell digest.** \`compute_shell_digest\` and \`compute_witness_digest\` do not hash \`proof.kind\`, \`proof.proof_bytes\`, or \`proof.public_inputs_hash\`. Two shells with identical content but different proof bytes produce the same \`digest\`. Either include a canonical proof commitment in the digest, or move proof evidence entirely out-of-band and off the digest-addressed struct.
**4. Braid::apply is public and unchecked.** \`fold()\` validates before calling \`apply()\`, but \`apply\` is callable directly. A second \`BraidCreated\` resets status to Active. Weaving after Finalized and collapsing before Finalized are not guarded. Make \`apply\` private or rename it \`apply_unchecked\`, expose a \`pub fn try_apply(&mut self, event: BraidEvent) -> Result<(), BraidError>\` that both \`fold\` and public mutation use.
**5. Braid::fold accepts duplicate member refs.** Sequence-number monotonicity is enforced but member-ref uniqueness is not. Two \`MemberWoven\` events with the same \`member_ref\` at different sequence numbers pass validation. Add \`BraidError::DuplicateMember { member_ref: BraidMemberRef }\` and reject it in \`fold\`.
---
Already resolved in this PR (no action needed):
- \`Braid::fold\` no longer returns \`Result<Self, String>\`; \`BraidError\` with thiserror exists.
- \`SEALED_MEMBER_DOMAIN\` is a module-level const, not an inline literal."Length of output: 2636 🏁 Script executed: #!/bin/bash
gh pr view 545 --repo flyingrobots/echo --json reviews --jq '.reviews[-3:]'Length of output: 4689
Two of the original pedantic findings were already closed in this PR — |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
docs/design/0028-strand-typestates-proof-envelopes-and-evolving-braids/design.md (4)
31-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate the
BraidMemberRefschema to match the implementation.The doc still shows
Sealed(Hash)and a seal overstrand_idonly. The actual type carriesblinded_commitmentplusauthority, and the seal binds additional worldline context. This is an API contract drift, not a cosmetic rename.Also applies to: 95-127
🤖 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 `@docs/design/0028-strand-typestates-proof-envelopes-and-evolving-braids/design.md` around lines 31 - 34, Update the `BraidMemberRef` schema documentation to accurately reflect the actual implementation. The design doc currently shows `Sealed(Hash)` binding only `strand_id`, but the real type carries both `blinded_commitment` and `authority` fields, with the seal binding additional worldline context beyond just the strand ID. Update the schema definition and description around lines 31-34 to document the actual variants and fields of `BraidMemberRef`, and ensure the description of what the seal commits to includes the worldline context. Apply the same corrections to the affected sections around lines 95-127 that reference or describe this `BraidMemberRef` structure and its sealing behavior.
47-61:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDo not describe
CausalPostureStateas sealed yet.
crates/warp-core/src/revelation.rscurrently exports a publicCausalPostureStatetrait and public marker types, so theprivate::Sealedwording overstates the implemented guarantee. Either update the spec to the public API shape or seal the trait in code first.🤖 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 `@docs/design/0028-strand-typestates-proof-envelopes-and-evolving-braids/design.md` around lines 47 - 61, The design document describes the CausalPostureState trait as sealed with private::Sealed, but this overstates the current API guarantee since crates/warp-core/src/revelation.rs exports CausalPostureState and its marker types (Shared, AuthorOnly, Scratch, DynamicPosture) as public without sealing. Either update the design document to reflect the public API by removing the private::Sealed reference, or first implement the sealing in the revelation.rs code (with the Sealed trait pattern) to match what the design describes.
37-40:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix the braid-log invariants section to match the current state machine.
The summary omits
BraidCollapsed, and the folding text says duplicate membership is already rejected even though duplicate-member acceptance is still called out as a blocker in the PR objectives. Update the event list and invariant text, or add the missing rejection before documenting it as enforced.Also applies to: 181-234
🤖 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 `@docs/design/0028-strand-typestates-proof-envelopes-and-evolving-braids/design.md` around lines 37 - 40, The Evolving Braid Logs section describing BraidEvent types is incomplete and contains contradictory claims about invariant enforcement. Add the missing BraidCollapsed event type to the list of state transition events in the BraidEvent summary at the anchor location (lines 37-40). Additionally, resolve the inconsistency where the folding text asserts that duplicate membership rejection is enforced, but the PR objectives still list duplicate-member acceptance as a blocking issue: either implement the rejection logic and then document it as enforced, or update the invariant documentation text to accurately reflect that duplicate member handling remains a pending task. Apply the same corrections to the sibling section at lines 181-234 in the same file to ensure consistency across the design document.
35-36:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCorrect the proof-envelope model before treating it as a cryptographic guarantee.
ProofKindis stale, andProofEnvelopedoes not contain the honesty claim. The current implementation only validates envelope shape, not a full proof or claim binding, so the doc should say that explicitly or the verifier needs to be brought up to spec.Also applies to: 133-177
🤖 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 `@docs/design/0028-strand-typestates-proof-envelopes-and-evolving-braids/design.md` around lines 35 - 36, The design document at lines 35-36 (anchor) and lines 133-177 (sibling) incorrectly presents ProofKind, ProofEnvelope, and the proof-validation logic in BraidShell::assemble_with_proof as cryptographic guarantees when the current implementation has gaps: ProofKind is stale, ProofEnvelope does not include the honesty claim, and validation only checks envelope shape rather than full proof and claim binding. Either update both documentation sections to explicitly state these limitations and clarify that the current implementation validates only envelope structure without cryptographic proof verification, or update the implementation to fully support ProofEnvelope with embedded ObserverHonestyClaim and complete the verifier logic to validate actual proofs and claim bindings. Ensure both locations (lines 35-36 and lines 133-177) are consistent in their description of what the proof mechanism actually provides.crates/warp-core/src/settlement.rs (2)
720-725:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftRevalidate posture here or this public entrypoint still bypasses the settlement gate.
plan_with_policy_internaltrustsStrand<Shared>and never checks thatstrand.retention_posture.causal_postureis actuallyShared. In this crate the typestate marker and runtime posture are independently constructible, so a forgedStrand<Shared>can still plan (and then settle) anAuthorOnly/Scratchstrand into base history. That leaves the blocking typestate-forgery invariant open.Make this entrypoint non-public, or reject any runtime posture other than
CausalPosture::Sharedbefore producing a plan.settle_with_policy_internalshould keep the same defensive check through this path.Fix sketch
- pub fn plan_with_policy_internal( + pub(crate) fn plan_with_policy_internal( runtime: &WorldlineRuntime, provenance: &ProvenanceService, strand: &crate::strand::Strand<crate::revelation::Shared>, policy: &SettlementPolicy, ) -> Result<SettlementPlan, SettlementError> { + if strand.retention_posture.causal_posture != CausalPosture::Shared { + return Err(SettlementError::NonSharedStrand { + strand_id: strand.strand_id, + posture: strand.retention_posture.causal_posture, + }); + } let strand_id = strand.strand_id; let strand_posture = strand.retention_posture.causal_posture;🤖 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 `@crates/warp-core/src/settlement.rs` around lines 720 - 725, The public function plan_with_policy_internal in crates/warp-core/src/settlement.rs has a security vulnerability where it trusts the typestate marker Strand<Shared> without validating that the runtime posture actually matches. Either make this function non-public by removing the pub visibility modifier, or add a defensive validation check that explicitly verifies strand.retention_posture.causal_posture equals CausalPosture::Shared before proceeding with the plan, rejecting the request if the actual posture does not match the claimed typestate. Apply the same defensive validation to settle_with_policy_internal to ensure it also validates the runtime posture matches Shared through this code path.
2191-2201:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the concrete settle error here.
This regression only checks
is_err(). Ifsettle()starts getting past the posture gate and fails later for an unrelated reason, the test still passes. AssertSettlementError::NonSharedStrand { .. }for settle too so the test proves the rejection happens at the intended boundary.Fix sketch
let mut runtime_for_settle = runtime.clone(); let mut provenance_for_settle = provenance.clone(); - assert!( - SettlementService::settle( - &mut runtime_for_settle, - &mut provenance_for_settle, - strand_id - ) - .is_err(), - "settle must not bypass the planning gate" - ); + let err = SettlementService::settle( + &mut runtime_for_settle, + &mut provenance_for_settle, + strand_id, + ) + .expect_err("settle must reject non-shared strands at the posture gate"); + assert!(matches!( + err, + SettlementError::NonSharedStrand { + strand_id: rejected, + posture: CausalPosture::AuthorOnly, + } if rejected == strand_id + ));🤖 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 `@crates/warp-core/src/settlement.rs` around lines 2191 - 2201, The test for SettlementService::settle only asserts that is_err() returns true, but does not verify the concrete error type, which means the test would pass even if settle fails for an unrelated reason after the planning gate. Replace the is_err() assertion with a pattern match that explicitly asserts the error is SettlementError::NonSharedStrand { .. } to ensure the rejection happens at the intended posture gate boundary, not at some later stage.crates/warp-core/src/braid_shell.rs (3)
486-543:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop calling this proof verification until it actually verifies proofs.
This path turns
p.verify()into a hard security guarantee (ProofVerificationFailed), but the PR's blocking note saysProofEnvelope::verify()currently only validates envelope shape/public-input binding. In the current implementation, arbitrary non-empty bytes with the rightpublic_inputs_hashare accepted as “verified.” Either dispatch real per-ProofKindverification here, or rename the API/error/docs to “envelope validation” so callers do not treat this as cryptographic proof enforcement.🤖 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 `@crates/warp-core/src/braid_shell.rs` around lines 486 - 543, The assemble_with_proof method calls p.verify() and treats the result as cryptographic proof verification by returning ProofVerificationFailed, but ProofEnvelope::verify() currently only validates envelope shape and public-input binding, not actual proof validity. Either implement real per-ProofKind cryptographic verification logic in the verify() method, or rename verify() to something like validate_envelope(), rename ProofVerificationFailed to ProofValidationFailed, and update related documentation to clarify this only validates envelope structure, not cryptographic proof security. Ensure the API naming and error types do not mislead callers into treating this as a cryptographic guarantee.
674-695:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
has_member_strandis now revealed-only, but the public API still reads as if it covers all members.After this change, a shell whose members are all
BraidMemberRef::Sealedreturnsfalsefromhas_member_strand, andBraidShellQuery::matches()consequently cannot find non-shared members bymember_strandat all. That is a public contract break, not just a harder privacy boundary. Either rename the existing API/query field to make the revealed-only behavior explicit, or add a secure query variant that accepts the child worldline needed to match sealed refs.🤖 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 `@crates/warp-core/src/braid_shell.rs` around lines 674 - 695, The has_member_strand method only checks BraidMemberRef::Revealed references and returns false for BraidMemberRef::Sealed, but the public API suggests it covers all members. This breaks the public contract. Either rename has_member_strand and the related BraidShellQuery::matches() method to explicitly indicate they only handle revealed members (e.g., has_member_strand_revealed), or add a secure query variant to BraidShellQuery that accepts a child_worldline_id parameter and can match both revealed and sealed references using the matches_strand logic, similar to how has_member_strand_secure works.
493-567:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
proofis swappable without changing the retained shell digest.
proofis stored onBraidShell, but neithercompute_witness_digestnorcompute_shell_digestbinds it. Two shells with identical bodies and different proof material therefore produce the samedigestand both passvalidate(). That breaks the digest-addressed record contract for retained shells and matches the swappable-proof footgun called out in the PR objectives. Either hash a canonical proof/claim digest into the witness/shell digest, or keep proof outside the digest-addressed shell value entirely.🤖 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 `@crates/warp-core/src/braid_shell.rs` around lines 493 - 567, The `proof` field is included in the BraidShell struct but is not bound into the digest computation performed by `compute_witness_digest` or `compute_shell_digest`, allowing two shells with different proof values to produce identical digests. Choose one of two approaches: (1) compute a canonical digest from the proof and include it as a parameter to `compute_witness_digest` and `compute_shell_digest` so the proof material influences the final digest, or (2) remove the `proof` field from the BraidShell struct definition and store it separately outside the digest-addressed value. Implement your chosen approach consistently across all digest computation calls and the struct definition.
♻️ Duplicate comments (1)
crates/warp-core/src/braid.rs (1)
120-144:⚠️ Potential issue | 🔴 Critical
applyremains public without validation (duplicate, blocking).
applyis stillpub(line 121) and performs no state validation. This allows:
- Multiple
BraidCreatedevents (line 124 overwritesbraid_id)- Out-of-order
sequence_numinMemberWoven(line 132 blindly updates)- Duplicate members (line 131 pushes without checking)
- Events in wrong status (no status checks)
Additionally, line 132 has unchecked overflow:
sequence_num + 1panics atu64::MAXin debug, wraps in release (non-deterministic).Both issues were flagged in prior review. The PR author explicitly marked unchecked
applyas blocking concern#4.🤖 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 `@crates/warp-core/src/braid.rs` around lines 120 - 144, Make the `apply` method private (remove `pub` visibility) to prevent unvalidated state mutations, then add validation logic at the beginning of the method to check: (1) BraidCreated events only occur when braid_id is unset, (2) sequence_num in MemberWoven increments properly and does not overflow (use checked_add instead of unchecked addition to safely handle the sequence_num + 1 operation), (3) members are not duplicated before pushing to self.members, and (4) status transitions are valid (e.g., SettlementFinalized and BraidCollapsed should only occur when status is Active). Return an error or panic for any invalid state transition detected.
🤖 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 `@crates/warp-core/src/braid.rs`:
- Around line 169-181: The duplicate member check in the BraidEvent::MemberWoven
handler will create O(n²) fold complexity if implemented with a naive Vec linear
search using contains(). To fix this, either accept the O(n²) complexity with a
simple Vec scan (if large braids are unlikely), implement a
BTreeSet<BraidMemberRef> for O(n log n) performance (requiring an Ord trait
implementation on BraidMemberRef to maintain determinism), or move duplicate
detection to a separate validation pass outside the fold loop. Choose the
approach based on expected braid size and performance requirements, and add the
duplicate member check in the BraidEvent::MemberWoven pattern match with the
selected data structure or validation strategy.
- Around line 169-181: The MemberWoven event handler in the fold method
validates sequence number progression but fails to prevent duplicate members
from being woven multiple times. Add a new DuplicateMember error variant to the
BraidError enum, then in the BraidEvent::MemberWoven pattern match block (lines
169-181), extract the member_ref from the event and add a validation check using
braid.members.contains before allowing the apply call to proceed. If a duplicate
member_ref is detected, return the DuplicateMember error to prevent the same
member from appearing in multiple MemberWoven events.
- Around line 22-50: The BraidError enum is defined in a private braid module
but is referenced in the documentation of the public fold() function, making it
inaccessible and causing rustdoc errors. Fix this by either making the braid
module public in lib.rs or by adding a public re-export of BraidError at the
crate root using pub use braid::BraidError; in lib.rs. Choose the approach that
best fits your crate's public API design and visibility strategy.
---
Outside diff comments:
In `@crates/warp-core/src/braid_shell.rs`:
- Around line 486-543: The assemble_with_proof method calls p.verify() and
treats the result as cryptographic proof verification by returning
ProofVerificationFailed, but ProofEnvelope::verify() currently only validates
envelope shape and public-input binding, not actual proof validity. Either
implement real per-ProofKind cryptographic verification logic in the verify()
method, or rename verify() to something like validate_envelope(), rename
ProofVerificationFailed to ProofValidationFailed, and update related
documentation to clarify this only validates envelope structure, not
cryptographic proof security. Ensure the API naming and error types do not
mislead callers into treating this as a cryptographic guarantee.
- Around line 674-695: The has_member_strand method only checks
BraidMemberRef::Revealed references and returns false for
BraidMemberRef::Sealed, but the public API suggests it covers all members. This
breaks the public contract. Either rename has_member_strand and the related
BraidShellQuery::matches() method to explicitly indicate they only handle
revealed members (e.g., has_member_strand_revealed), or add a secure query
variant to BraidShellQuery that accepts a child_worldline_id parameter and can
match both revealed and sealed references using the matches_strand logic,
similar to how has_member_strand_secure works.
- Around line 493-567: The `proof` field is included in the BraidShell struct
but is not bound into the digest computation performed by
`compute_witness_digest` or `compute_shell_digest`, allowing two shells with
different proof values to produce identical digests. Choose one of two
approaches: (1) compute a canonical digest from the proof and include it as a
parameter to `compute_witness_digest` and `compute_shell_digest` so the proof
material influences the final digest, or (2) remove the `proof` field from the
BraidShell struct definition and store it separately outside the
digest-addressed value. Implement your chosen approach consistently across all
digest computation calls and the struct definition.
In `@crates/warp-core/src/settlement.rs`:
- Around line 720-725: The public function plan_with_policy_internal in
crates/warp-core/src/settlement.rs has a security vulnerability where it trusts
the typestate marker Strand<Shared> without validating that the runtime posture
actually matches. Either make this function non-public by removing the pub
visibility modifier, or add a defensive validation check that explicitly
verifies strand.retention_posture.causal_posture equals CausalPosture::Shared
before proceeding with the plan, rejecting the request if the actual posture
does not match the claimed typestate. Apply the same defensive validation to
settle_with_policy_internal to ensure it also validates the runtime posture
matches Shared through this code path.
- Around line 2191-2201: The test for SettlementService::settle only asserts
that is_err() returns true, but does not verify the concrete error type, which
means the test would pass even if settle fails for an unrelated reason after the
planning gate. Replace the is_err() assertion with a pattern match that
explicitly asserts the error is SettlementError::NonSharedStrand { .. } to
ensure the rejection happens at the intended posture gate boundary, not at some
later stage.
In
`@docs/design/0028-strand-typestates-proof-envelopes-and-evolving-braids/design.md`:
- Around line 31-34: Update the `BraidMemberRef` schema documentation to
accurately reflect the actual implementation. The design doc currently shows
`Sealed(Hash)` binding only `strand_id`, but the real type carries both
`blinded_commitment` and `authority` fields, with the seal binding additional
worldline context beyond just the strand ID. Update the schema definition and
description around lines 31-34 to document the actual variants and fields of
`BraidMemberRef`, and ensure the description of what the seal commits to
includes the worldline context. Apply the same corrections to the affected
sections around lines 95-127 that reference or describe this `BraidMemberRef`
structure and its sealing behavior.
- Around line 47-61: The design document describes the CausalPostureState trait
as sealed with private::Sealed, but this overstates the current API guarantee
since crates/warp-core/src/revelation.rs exports CausalPostureState and its
marker types (Shared, AuthorOnly, Scratch, DynamicPosture) as public without
sealing. Either update the design document to reflect the public API by removing
the private::Sealed reference, or first implement the sealing in the
revelation.rs code (with the Sealed trait pattern) to match what the design
describes.
- Around line 37-40: The Evolving Braid Logs section describing BraidEvent types
is incomplete and contains contradictory claims about invariant enforcement. Add
the missing BraidCollapsed event type to the list of state transition events in
the BraidEvent summary at the anchor location (lines 37-40). Additionally,
resolve the inconsistency where the folding text asserts that duplicate
membership rejection is enforced, but the PR objectives still list
duplicate-member acceptance as a blocking issue: either implement the rejection
logic and then document it as enforced, or update the invariant documentation
text to accurately reflect that duplicate member handling remains a pending
task. Apply the same corrections to the sibling section at lines 181-234 in the
same file to ensure consistency across the design document.
- Around line 35-36: The design document at lines 35-36 (anchor) and lines
133-177 (sibling) incorrectly presents ProofKind, ProofEnvelope, and the
proof-validation logic in BraidShell::assemble_with_proof as cryptographic
guarantees when the current implementation has gaps: ProofKind is stale,
ProofEnvelope does not include the honesty claim, and validation only checks
envelope shape rather than full proof and claim binding. Either update both
documentation sections to explicitly state these limitations and clarify that
the current implementation validates only envelope structure without
cryptographic proof verification, or update the implementation to fully support
ProofEnvelope with embedded ObserverHonestyClaim and complete the verifier logic
to validate actual proofs and claim bindings. Ensure both locations (lines 35-36
and lines 133-177) are consistent in their description of what the proof
mechanism actually provides.
---
Duplicate comments:
In `@crates/warp-core/src/braid.rs`:
- Around line 120-144: Make the `apply` method private (remove `pub` visibility)
to prevent unvalidated state mutations, then add validation logic at the
beginning of the method to check: (1) BraidCreated events only occur when
braid_id is unset, (2) sequence_num in MemberWoven increments properly and does
not overflow (use checked_add instead of unchecked addition to safely handle the
sequence_num + 1 operation), (3) members are not duplicated before pushing to
self.members, and (4) status transitions are valid (e.g., SettlementFinalized
and BraidCollapsed should only occur when status is Active). Return an error or
panic for any invalid state transition detected.
🪄 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: 0cd50f32-7ce0-46d7-af60-b27564d23a86
📒 Files selected for processing (5)
crates/warp-core/src/braid.rscrates/warp-core/src/braid_shell.rscrates/warp-core/src/observation.rscrates/warp-core/src/settlement.rsdocs/design/0028-strand-typestates-proof-envelopes-and-evolving-braids/design.md
Code Lawyer: Phase I Discovery & Radical TransparencyThe self-audit has identified the following additional issues on branch
Cc: @codex for second opinion. |
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/warp-core/src/revelation.rs (1)
76-116: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueCausalPostureState trait is unsealed — runtime validation is the safety net.
External crates can implement this trait and construct
Strand<ForgeryType>with arbitrarycausal_posture()return values. The compile-time typestate guarantee relies on callers only using the provided marker types.Per context snippet from
settlement.rs:1041-1052,ensure_shared_runtime_posturevalidates the runtimeretention_posture.causal_posturefield before settlement proceeds, so forgery attempts fail at the admission gate. This defense-in-depth approach is acceptable but not ideal.Consider sealing the trait if stronger compile-time guarantees are needed later (e.g., via a private super-trait or private method).
🤖 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 `@crates/warp-core/src/revelation.rs` around lines 76 - 116, The CausalPostureState trait is currently unsealed, allowing external crates to implement it arbitrarily and bypass the compile-time typestate guarantee. To seal the trait, introduce a private super-trait or private module that restricts implementations to only the provided marker types (Shared, AuthorOnly, Scratch, and DynamicPosture). For example, add a private trait as a super-trait of CausalPostureState or move the trait and its implementations into a private module, then only re-export the concrete marker types. This prevents external crates from creating custom implementations while maintaining the public API of the marker types themselves.crates/warp-core/src/braid_shell.rs (1)
738-760: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReplace the quadratic duplicate scan with a deterministic set.
validate()and replay hit this path, and the currentmembers[..index]scan is O(n²).BraidMemberRefalready hasOrd, so aBTreeSetkeeps the deterministic ordering requirement while avoiding the hot-path quadratic walk.Use `BTreeSet` for deterministic duplicate detection
fn check_unique_member_strands(members: &[BraidShellMember]) -> Result<(), BraidShellError> { if let Some(first) = members.first() { let first_is_sealed = first.member_ref.is_sealed(); if members .iter() .any(|member| member.member_ref.is_sealed() != first_is_sealed) { return Err(BraidShellError::MixedMemberReferencePosture); } } - for (index, member) in members.iter().enumerate() { - if members[..index] - .iter() - .any(|earlier| earlier.member_ref == member.member_ref) - { + let mut seen = std::collections::BTreeSet::new(); + for member in members { + if !seen.insert(member.member_ref) { return Err(BraidShellError::DuplicateMemberStrand { member_ref: member.member_ref, }); } } Ok(()) }As per coding guidelines,
crates/warp-core/**is deterministic kernel code and “Performance matters — flag unnecessary allocations in hot paths.”🤖 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 `@crates/warp-core/src/braid_shell.rs` around lines 738 - 760, Replace the O(n²) nested loop duplicate detection in the `check_unique_member_strands` function with a deterministic BTreeSet approach. Remove the inner nested loop that checks `members[..index]` and instead create a BTreeSet to track seen member references as you iterate through the members. For each member, check if its member_ref is already in the set before adding it; if found, return the DuplicateMemberStrand error. This reduces the complexity from O(n²) to O(n log n) while maintaining deterministic ordering since BraidMemberRef already implements Ord.Source: Coding guidelines
🤖 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 `@CHANGELOG.md`:
- Around line 10-11: Update docstrings to explicitly clarify that cryptographic
proof verification is NOT performed. In design.md (lines 49–53 for
`validate_shape` method and lines 496–498 for
`BraidShell::assemble_with_proof`), add explicit disclaimers stating "Does not
perform cryptographic proof verification; only validates envelope structure and
public-input hash binding" to the `validate_shape` docstring and "Proof
cryptographic validity is not verified; only envelope shape and public-input
binding are validated" to the `assemble_with_proof` docstring. The CHANGELOG.md
anchor entries (lines 10–11) document these API changes and require no direct
modification—they serve as evidence of the implementation changes that
necessitate the docstring updates.
In `@crates/warp-core/src/braid_shell.rs`:
- Around line 99-112: The secure member lookup in matches_strand currently
allows false positives by matching only blinded_commitment for sealed identities
while ignoring the authority field. Add the authority field to
BraidShellMemberQuery, then update the matches_strand method at lines 99-112
(anchor) to match both blinded_commitment and authority when comparing against
the expected sealed identity. Apply the same fix at the sibling locations
(708-718, 1209-1215, 1247-1253) where sealed member matching also occurs,
ensuring that the authority is included in all secure path validations to
prevent false positive matches.
- Around line 1207-1229: Remove the derived `Debug` trait from the
`BraidShellMemberQuery` struct and implement a custom `Debug` impl that redacts
the `blinding_secret` field instead of exposing it in the debug output. This
custom implementation will automatically be used by the derived `Debug` on
`BraidShellQuery` when it formats the `secure_member` field, preventing the
transitive leak of the non-public secret material.
In `@crates/warp-core/src/settlement.rs`:
- Around line 1467-1472: The settlement_member_blinding function is mixing in
unstable settlement/frontier coordinates (like target_worldline,
target_base_ref, and basis_report.realized_parent_ref) when calculating the
blinding secret, causing the same hidden member to get different sealed
commitments after parent frontier movement. Modify the
settlement_member_blinding function or how it's called to remove
settlement-instance coordinates and keep the blinding scoped only to stable
member and retention material, since BraidMemberRef::seal already binds the
stable strand_id and child worldline identifiers. Apply this same fix at all
locations where settlement_member_blinding is used (also at lines 1507-1525).
In `@crates/warp-core/tests/braid_public_api_tests.rs`:
- Around line 6-10: The test file braid_public_api_tests.rs is designed to
verify the crate-root public API exports, but the import of make_strand_id from
warp_core::strand submodule violates this principle by testing module visibility
rather than crate-root re-exports. Either import make_strand_id directly from
warp_core (the crate root) if it is part of the public API contract, or refactor
the test to construct BraidMemberRef using only types and functions that are
directly exported from the crate root to properly validate the intended public
surface.
---
Outside diff comments:
In `@crates/warp-core/src/braid_shell.rs`:
- Around line 738-760: Replace the O(n²) nested loop duplicate detection in the
`check_unique_member_strands` function with a deterministic BTreeSet approach.
Remove the inner nested loop that checks `members[..index]` and instead create a
BTreeSet to track seen member references as you iterate through the members. For
each member, check if its member_ref is already in the set before adding it; if
found, return the DuplicateMemberStrand error. This reduces the complexity from
O(n²) to O(n log n) while maintaining deterministic ordering since
BraidMemberRef already implements Ord.
In `@crates/warp-core/src/revelation.rs`:
- Around line 76-116: The CausalPostureState trait is currently unsealed,
allowing external crates to implement it arbitrarily and bypass the compile-time
typestate guarantee. To seal the trait, introduce a private super-trait or
private module that restricts implementations to only the provided marker types
(Shared, AuthorOnly, Scratch, and DynamicPosture). For example, add a private
trait as a super-trait of CausalPostureState or move the trait and its
implementations into a private module, then only re-export the concrete marker
types. This prevents external crates from creating custom implementations while
maintaining the public API of the marker types themselves.
🪄 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: e0c467b1-ae9d-46fc-b3df-4a7bb38260d8
📒 Files selected for processing (13)
CHANGELOG.mdcrates/warp-core/src/braid.rscrates/warp-core/src/braid_shell.rscrates/warp-core/src/coordinator.rscrates/warp-core/src/lib.rscrates/warp-core/src/proof.rscrates/warp-core/src/revelation.rscrates/warp-core/src/settlement.rscrates/warp-core/src/strand.rscrates/warp-core/tests/braid_public_api_tests.rscrates/warp-core/tests/strand_contract_tests.rscrates/warp-wasm/src/warp_kernel.rsdocs/design/0028-strand-typestates-proof-envelopes-and-evolving-braids/design.md
Activity SummaryCode Lawyer pass completed through head
Local witnesses run:
Remote status at posting time: CodeRabbit status is |
|
@coderabbitai it shows that you still have an outstanding change request. What is it? If it has been resolved, please approve this merge. Otherwise, make some noise. 🥕 |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# 1. Check if Braid::apply is private (or try_apply is the public interface)
rg -n "pub fn apply\|fn apply\|pub fn try_apply" crates/warp-core/src/braid.rs
echo "---"
# 2. Check DuplicateMember variant and usage in fold
rg -n "DuplicateMember\|member_index\|contains" crates/warp-core/src/braid.rs
echo "---"
# 3. Check proof digest is included in shell digest
rg -n "proof.*digest\|digest.*proof\|proof_digest\|ProofEnvelope" crates/warp-core/src/braid_shell.rs | head -40
echo "---"
# 4. Check settlement re-validates posture at runtime
rg -n "revalidat\|validate\|PostureMismatch\|causal_posture\|Shared" crates/warp-core/src/settlement.rs | head -40
echo "---"
# 5. Check CausalPostureState is sealed (private module or sealed trait pattern)
rg -n "pub trait CausalPostureState\|mod private\|seal\|Sealed" crates/warp-core/src/revelation.rs | head -20
echo "---"
# 6. Check BraidStatus is re-exported from lib.rs
rg -n "BraidStatus\|BraidError" crates/warp-core/src/lib.rs
echo "---"
# 7. Check proof.rs validate_shape (renamed from verify)
rg -n "fn validate_shape\|fn verify\|ProofShapeValidationFailed" crates/warp-core/src/proof.rs
echo "---"
# 8. Check Braid fields are private with accessors
rg -n "pub events\|pub members\|pub next_seq\|pub status\|pub fn events\|pub fn status\|pub fn frontier" crates/warp-core/src/braid.rsLength of output: 1626 🏁 Script executed: #!/bin/bash
# Find the actual file locations
fd "braid.rs|braid_shell.rs|proof.rs|revelation.rs|settlement.rs" --type f
echo "---CRATE LIB---"
fd "lib.rs" --type f | grep "warp-core"
echo "---TREE---"
tree crates/ -L 3 2>/dev/null || ls -R crates/ 2>/dev/null | head -60Length of output: 10141 🏁 Script executed: #!/bin/bash
echo "=== 1. Braid::apply visibility ==="
rg -n "pub fn apply|fn apply|pub fn try_apply" crates/warp-core/src/braid.rs
echo "=== 2. DuplicateMember in braid.rs ==="
rg -n "DuplicateMember|member_index" crates/warp-core/src/braid.rs
echo "=== 3. Proof digest in shell digest (braid_shell.rs) ==="
rg -n "proof|digest" crates/warp-core/src/braid_shell.rs | grep -i "proof" | head -30
echo "=== 4. Settlement posture revalidation ==="
rg -n "PostureMismatch|revalidat|causal_posture.*Shared\|Shared.*causal_posture|validate.*posture\|NonSharedStrand" crates/warp-core/src/settlement.rs | head -30
echo "=== 5. CausalPostureState sealing ==="
rg -n "pub trait CausalPostureState|mod private|sealed|__Sealed" crates/warp-core/src/revelation.rs | head -20
echo "=== 6. BraidStatus/BraidError in lib.rs ==="
rg -n "BraidStatus|BraidError" crates/warp-core/src/lib.rs
echo "=== 7. validate_shape vs verify in proof.rs ==="
rg -n "fn validate_shape|fn verify|ProofShapeValidationFailed" crates/warp-core/src/proof.rs
echo "=== 8. Braid field visibility ==="
rg -n "pub\s+\w\+\s*:" crates/warp-core/src/braid.rs | head -20
grep -n "pub fn events\|pub fn status\|pub fn frontier\|pub fn next_sequence\|pub fn latest_settlement\|pub fn braid_id" crates/warp-core/src/braid.rsLength of output: 6840 🏁 Script executed: #!/bin/bash
echo "=== Settlement NonSharedStrand ==="
rg -n "NonSharedStrand" crates/warp-core/src/settlement.rs | head -20
echo "=== Settlement Shared posture runtime check ==="
rg -n "causal_posture\|CausalPosture\|Shared\|PostureMismatch" crates/warp-core/src/settlement.rs | head -30
echo "=== Strand struct field visibility ==="
grep -n "pub\s" crates/warp-core/src/strand.rs | head -40
echo "=== Braid::apply body (lines 140-220) ==="
sed -n '140,220p' crates/warp-core/src/braid.rs
echo "=== CausalPostureState sealed pattern ==="
sed -n '70,120p' crates/warp-core/src/revelation.rsLength of output: 8114
Secondary fixes confirmed: All blocking invariants are enforced at runtime. Approving. [approve] |
flyingrobots
dismissed
coderabbitai[bot]’s stale review
Dismissed as stale: CodeRabbit later verified all five blocking issues resolved in #545 (comment), all review threads are resolved, and CI/CodeRabbit status checks are green.
Self-Code Review: Code Lawyer Follow-up@codex please verify these findings. The stale CodeRabbit
Checks run during this review:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f8c5fa206d
ℹ️ 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".
Activity Summary — Self-Review ResolutionAll discovered self-review/Codex findings have been resolved and their review threads are marked resolved.
Local witnesses passed:
Pre-push also passed exact affected slices and pushed |
Code Lawyer: Final Phase I Self-Audit Finding@codex please verify this final self-audit finding before merge. Worktree was clean,
|
Activity Summary — Final Code Lawyer PassFinal self-audit completed on head
Local merge gate after push is green: Merge gate is waiting only for GitHub Actions to finish on the final pushed head before the authorized admin merge. |
There was a problem hiding this comment.
💡 Codex Review
echo/crates/warp-core/src/braid_shell.rs
Line 800 in b8fbb2f
Validate collapse shells against their plural parent
When a Derived shell carries collapsed_from, this accepts the shell regardless of whether its members actually match the named plural parent (or even contain any plural verdicts). Since append_braid_shell only calls validate() and replay_braid_shell only checks that the parent digest resolves to some plural shell, a malformed retained shell can name a valid plural parent while carrying unrelated all-derived members/result refs and still replay as a lawful collapse.
ℹ️ 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".
| DuplicateMember { | ||
| member_ref: BraidMemberRef, | ||
| }, | ||
| EmptyCollapseWitness, |
There was a problem hiding this comment.
When this design doc is used as the braid API contract, the BraidError snippet under-documents two implemented rejection cases: crates/warp-core/src/braid.rs defines MixedMemberReferencePosture and EmptySettlementFrontier, but the documented enum jumps from DuplicateMember to EmptyCollapseWitness. Consumers following the snippet would not know to handle mixed revealed/sealed members or empty settlement frontiers, so the design listing should be kept in sync with the implementation.
Useful? React with 👍 / 👎.
| fn default_member_blinding_salt(policy_id: Hash) -> MemberBlindingSalt { | ||
| let mut hasher = Hasher::new(); | ||
| hasher.update(DEFAULT_MEMBER_BLINDING_SALT_DOMAIN); | ||
| hasher.update(&policy_id); | ||
| MemberBlindingSalt(hasher.finalize().into()) |
There was a problem hiding this comment.
For callers that use SettlementPolicy::default() or allow_plural_over_footprint_overlap() without overriding the salt, this default is just a deterministic hash of the public policy id, so the same strand/child-worldline/retention tuple produces the same sealed blinded_commitment across retained shells. That turns the sealed member reference into a stable cross-shell tracking handle, which undercuts the privacy/linkability boundary this feature is meant to provide; the default salt needs non-public per-settlement entropy or the API should force callers to supply it.
Useful? React with 👍 / 👎.
| /// | ||
| /// Returns a posture obstruction error if the strand's actual posture is not Shared. | ||
| pub fn try_into_shared(self) -> Result<Strand<Shared>, StrandError> { | ||
| if self.retention_posture.causal_posture == crate::revelation::CausalPosture::Shared { |
There was a problem hiding this comment.
When a dynamic strand is hand-built or mutated through its public fields, setting only retention_posture.causal_posture to Shared is enough for try_into_shared() to return Strand<Shared>, even if RetentionPosture::validate() would reject the bundle for a missing admission scope or invalid source-disclosure policy. That leaks an invalid posture through the static Shared type for any caller that relies on typestate evidence outside the registry-backed settlement path, so this conversion should revalidate the whole retention posture before constructing the Shared handle.
Useful? React with 👍 / 👎.
1 participant
This PR closes all gaps from the gap analysis report:
Summary by CodeRabbit
INVALID_STRAND.