M2.1: DPO concurrency litmus (spec + tests) #247
Conversation
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughAdds a DPO concurrency litmus test suite and supporting documentation: new Rust tests exercise commuting, conflicting, and overlapping-but-composable rule pairs via terminal-digest and receipt assertions; specification SPEC-0003 and planning/execution docs updated to track and prioritize issue Changes
Sequence Diagram(s)(Skipped — changes are test additions and documentation; no new multi-component control flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
crates/warp-core/tests/dpo_concurrency_litmus.rsdocs/decision-log.mddocs/execution-plan.mddocs/spec-mwmr-concurrency.mddocs/spec/SPEC-0003-dpo-concurrency-litmus-v0.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runcargo clippy --all-targets -- -D missing_docsbefore every PR; CI will expect a zero-warning, fully documented surface
Every source file must start with exactly the SPDX header:// SPDX-License-Identifier: Apache-2.0and// © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots>
Files:
crates/warp-core/tests/dpo_concurrency_litmus.rs
🧠 Learnings (6)
📓 Common learnings
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/echo-core/**/*.rs : Tests and benchmarks are mandatory for runtime changes once the harness exists
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/echo-core/**/*.rs : Avoid introducing sources of nondeterminism without a mitigation plan in runtime code
📚 Learning: 2026-01-03T02:01:23.896Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Update `docs/execution-plan.md` and `docs/decision-log.md` when a PR touches non-doc code
Applied to files:
docs/decision-log.mddocs/execution-plan.md
📚 Learning: 2026-01-03T02:01:23.897Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.897Z
Learning: Start each session by updating 'Today's Intent' in `docs/execution-plan.md` and capture milestones, blockers, and decisions
Applied to files:
docs/decision-log.mddocs/execution-plan.md
📚 Learning: 2026-01-03T02:01:23.896Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/echo-core/**/*.rs : Avoid introducing sources of nondeterminism without a mitigation plan in runtime code
Applied to files:
docs/decision-log.mddocs/spec/SPEC-0003-dpo-concurrency-litmus-v0.mdcrates/warp-core/tests/dpo_concurrency_litmus.rs
📚 Learning: 2026-01-03T02:01:23.896Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/echo-core/**/*.rs : Tests and benchmarks are mandatory for runtime changes once the harness exists
Applied to files:
crates/warp-core/tests/dpo_concurrency_litmus.rs
📚 Learning: 2026-01-03T02:01:23.896Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/**/*.rs : Public APIs across crates (`warp-core`, `warp-ffi`, `warp-wasm`, etc.) must carry rustdoc comments that explain intent, invariants, and usage
Applied to files:
crates/warp-core/tests/dpo_concurrency_litmus.rs
🧬 Code graph analysis (1)
crates/warp-core/tests/dpo_concurrency_litmus.rs (10)
crates/warp-core/src/demo/ports.rs (1)
port_rule(94-112)crates/warp-core/src/payload.rs (1)
encode_motion_atom_payload(98-103)crates/warp-core/src/ident.rs (2)
make_node_id(61-66)make_type_id(53-58)crates/warp-core/src/warp_state.rs (1)
store(61-63)crates/warp-core/src/footprint.rs (1)
pack_port_key(206-214)crates/warp-core/src/demo/motion.rs (1)
motion_rule(148-160)crates/warp-core/src/tick_patch.rs (1)
policy_id(361-363)crates/warp-core/src/engine_impl.rs (1)
apply(283-290)crates/warp-geom/src/temporal/manifold.rs (1)
entity(28-30)crates/warp-core/src/receipt.rs (1)
entries(55-57)
🔇 Additional comments (15)
docs/spec-mwmr-concurrency.md (1)
117-117: LGTM — cross-reference is consistent with the PR scope.The reference addition aligns with the new SPEC-0003 document introduced in this PR.
docs/spec/SPEC-0003-dpo-concurrency-litmus-v0.md (2)
1-5: LGTM — spec document is well-structured and honest about scope.The "low ceremony" framing (line 13) is appropriate — this pins executable evidence without overclaiming categorical DPO compliance. The mapping table (lines 39–44) is crisp.
46-59: Litmus taxonomy aligns with the implemented tests.The three families map 1:1 to
dpo_litmus_commuting_independent_pair,dpo_litmus_conflicting_pair_is_deterministically_rejected, anddpo_litmus_overlapping_scope_disjoint_ports_are_composable. No drift detected.crates/warp-core/tests/dpo_concurrency_litmus.rs (8)
1-8: SPDX header and module doc are correct.Header matches the required format per coding guidelines. Module doc explains the litmus test purpose without fluff.
10-14: Imports are tight and used.No glob imports, no dead weight. Acceptable.
16-24: Rule ID derivation follows the established pattern.Domain separation (
b"rule:") matchesport_rule()convention. No complaints.
26-30: Intentionally trivial matcher/executor.The no-op executor is correct — these litmus rules exist to exercise footprint conflict mechanics, not to mutate state. The matcher's existence check is sufficient for the test scope.
32-48: Footprints correctly model disjoint boundary ports.Port 0 vs port 1 with matching direction (
dir_in=true) ensures the overlapping-scope test actually exercises disjointb_insets. The defensiveis_some()guard is fine for test code.
50-76: Rule constructors are boilerplate-heavy but clear.The duplication between
litmus_port_read_0_ruleandlitmus_port_read_1_ruleis tolerable for test code.ConflictPolicy::Abortensures conflicts surface in receipts rather than being silently retried.
78-94: Engine setup is clean.Proper
?propagation, registers all required rules. No dead code, no magic.
96-116: Digest assertion helper is comprehensive.All
Snapshotfields relevant to determinism are compared with descriptive failure messages. This matches the "terminal digest" definition in SPEC-0003 (lines 63–68).docs/decision-log.md (2)
9-9: Decision log entry is correctly formatted.Table row follows existing schema. Rationale ("plan drift causes duplicate triage") and consequence ("issue-driven again") are concrete.
117-121: Expanded entry provides sufficient context.The "Next" item aligns with the actual deliverables in this PR (spec note + litmus tests). No contradictions with the PR contents.
docs/execution-plan.md (2)
38-58: Today's Intent accurately reflects PR deliverables.Evidence block (lines 51–53) points to the actual artifacts created:
SPEC-0003-dpo-concurrency-litmus-v0.mdanddpo_concurrency_litmus.rs. Status is correctly marked DONE.
1056-1060: Next Up Queue is now issue-driven.The shift from merge-train workflow to
M2.1: DPO concurrency theorem coverage (issue #206)as item 1 aligns with the decision-log entry. No stale items detected.
|
✅ Addressed in commit 8a7667f Acked review threads:
|
|
@coderabbitai Please re-review the latest commit 8a7667f and submit a new approval to update the review status. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
crates/warp-core/tests/dpo_concurrency_litmus.rs
🧰 Additional context used
📓 Path-based instructions (1)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Runcargo clippy --all-targets -- -D missing_docsbefore every PR; CI will expect a zero-warning, fully documented surface
Every source file must start with exactly the SPDX header:// SPDX-License-Identifier: Apache-2.0and// © James Ross Ω FLYING•ROBOTS <https://github.com/flyingrobots>
Files:
crates/warp-core/tests/dpo_concurrency_litmus.rs
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/echo-core/**/*.rs : Tests and benchmarks are mandatory for runtime changes once the harness exists
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/echo-core/**/*.rs : Avoid introducing sources of nondeterminism without a mitigation plan in runtime code
📚 Learning: 2026-01-03T02:01:23.896Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/echo-core/**/*.rs : Tests and benchmarks are mandatory for runtime changes once the harness exists
Applied to files:
crates/warp-core/tests/dpo_concurrency_litmus.rs
📚 Learning: 2026-01-03T02:01:23.896Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/echo-core/**/*.rs : Avoid introducing sources of nondeterminism without a mitigation plan in runtime code
Applied to files:
crates/warp-core/tests/dpo_concurrency_litmus.rs
📚 Learning: 2026-01-03T02:01:23.896Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/**/*.rs : Public APIs across crates (`warp-core`, `warp-ffi`, `warp-wasm`, etc.) must carry rustdoc comments that explain intent, invariants, and usage
Applied to files:
crates/warp-core/tests/dpo_concurrency_litmus.rs
🔇 Additional comments (10)
crates/warp-core/tests/dpo_concurrency_litmus.rs (10)
1-9: LGTM!SPDX header exactly matches the required format. Module-level documentation is concise and clearly states the invariants under test: enqueue-order independence for commuting cases and deterministic rejection for conflicts.
10-14: LGTM!Imports are clean and tightly scoped to exactly what's needed.
26-30: LGTM!Minimal matcher (existence check) and no-op executor are appropriate for footprint-only litmus tests. The underscore-prefixed parameters correctly suppress unused warnings.
32-48: LGTM!Footprint functions correctly construct read sets with disjoint boundary port keys. The duplication is tolerable for test clarity—future maintainers can immediately see the port index difference without tracing through a parameterized helper.
50-76: LGTM, but verifyConflictPolicy::Abortis intentional.The litmus rules use
ConflictPolicy::Abort. This is fine if abort-on-conflict is the desired semantics for these tests, but if the litmus suite is meant to test the scheduler's conflict resolution (winner selection), ensure this policy doesn't short-circuit the mechanism you're testing.
78-94: LGTM!Engine construction is clean: root node inserted, four rules registered. Error propagation via
Resultis correct.
96-116: LGTM!Comprehensive equality check across all digest fields with clear failure messages. This is thorough—no digest component can silently diverge.
118-200: LGTM! Past review concern addressed.Both
receipt_aandreceipt_bentries are now explicitly verified for length and disposition. The test correctly exercises the commuting-pair invariant: disjoint footprints → both applied, terminal digests identical regardless of enqueue order.
202-283: LGTM! Past review concerns addressed.The conflict surface is now documented (lines 207–209): both rules write the same α-plane attachment slot. The test verifies structural equality of
a_entriesandb_entries(line 278), which is stricter than digest comparison alone. Exactly-one-applied, exactly-one-rejected invariant is correctly asserted.
285-346: LGTM! Past review concern addressed.Receipt_b entries are now verified for structural equality (line 340–342). The test correctly exercises the overlapping-scope-but-composable case: shared
n_read(read-only) and disjointb_inports yield no conflict—both candidates admitted, terminal digests identical.
🧩 Analysis chain🏁 Script executed: Length of output: 3642 🏁 Script executed: Length of output: 25983 🏁 Script executed: Length of output: 3431 @flyingrobots: Reviewing commit 8a7667f with MAXIMUM SCRUTINY engaged. Analysis: The additions systematically strengthen determinism validation by verifying receipt structure equality beyond digest equality—catching an entire class of potential bugs where digests might match but actual execution outcomes differ. This is a significant defensive improvement. Strengths:
Pedantic observation (non-blocking): The commuting case (lines 183-198) validates dispositions via Verdict: The commit is CORRECT and represents a substantial quality improvement to determinism validation. The asymmetry noted above is truly minor—the existing Approving. These assertions are exactly the kind of defense-in-depth that separates production-grade determinism claims from wishful thinking. 🧠 Learnings used✅ Actions performedComments resolved and changes approved. |
Signed-off-by: James Ross <james@flyingrobots.dev>
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 (1)
docs/execution-plan.md (1)
60-67: Duplicate/redundant entry structure for PR #213.Lines 60–67 repeat PR #213 context with the same scope/outcome, creating a confusing timeline where the same PR is described twice with slightly different framing (first as "no open PRs remained," now as "merge cleanly by…"). This violates the DRY principle and suggests either a copy-paste artifact or incomplete consolidation during a prior merge/rebase. Consolidate into a single entry for PR #213 with a unified narrative: goal, scope, exit criteria, and evidence (commit hashes). If there were multiple fix rounds, use a single entry with sub-bullets for each round rather than duplicate top-level entries.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
docs/decision-log.mddocs/execution-plan.md
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/echo-core/**/*.rs : Tests and benchmarks are mandatory for runtime changes once the harness exists
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/echo-core/**/*.rs : Avoid introducing sources of nondeterminism without a mitigation plan in runtime code
📚 Learning: 2026-01-03T02:01:23.897Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.897Z
Learning: Start each session by updating 'Today's Intent' in `docs/execution-plan.md` and capture milestones, blockers, and decisions
Applied to files:
docs/execution-plan.mddocs/decision-log.md
📚 Learning: 2026-01-03T02:01:23.896Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Update `docs/execution-plan.md` and `docs/decision-log.md` when a PR touches non-doc code
Applied to files:
docs/execution-plan.mddocs/decision-log.md
📚 Learning: 2026-01-03T02:01:23.896Z
Learnt from: CR
Repo: flyingrobots/echo PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-03T02:01:23.896Z
Learning: Applies to packages/echo-core/**/*.rs : Avoid introducing sources of nondeterminism without a mitigation plan in runtime code
Applied to files:
docs/decision-log.md
🔇 Additional comments (2)
docs/execution-plan.md (2)
38-53: Evidence section references files not available in PR review context.Lines 52–53 claim evidence in
docs/spec/SPEC-0003-dpo-concurrency-litmus-v0.mdandcrates/warp-core/tests/dpo_concurrency_litmus.rs, but these files are not provided in this code review. Cannot verify:
- Whether SPEC-0003 adequately documents the DPO concurrency mapping (footprints, scheduler independence checks, etc.) per PR objectives,
- Whether
dpo_concurrency_litmus.rsactually contains the three promised test cases (commuting, conflicting, overlapping),- Whether test assertions match the stated outcomes (terminal digest checks, receipt structure equality, etc.).
Recommendation: Include the actual spec and test files in the review context, or acknowledge in the execution-plan entry that evidence is pending code review and cross-link to the PR/branch where tests are available. As-is, this entry claims completion without reviewable artifacts.
38-53: This review comment is based on a misreading of the actual execution-plan content.The Outcome section (line 50) explicitly states "litmus tests (commuting vs conflicting vs overlapping-but-safe)"—the exact text shown in your code snippet. The three test cases are all present in
dpo_concurrency_litmus.rs:
dpo_litmus_commuting_independent_pair()(lines 119–200) — independent/commuting pairdpo_litmus_conflicting_pair_is_deterministically_rejected()(lines 203–283) — conflicting pairdpo_litmus_overlapping_scope_disjoint_ports_are_composable()(lines 286–346) — overlapping-but-safe caseThe spec file
SPEC-0003-dpo-concurrency-litmus-v0.md(lines 46–59) similarly documents all three case families. No gap exists.Likely an incorrect or invalid review comment.
| | 2026-01-03 | Planning hygiene + M2.1 kickoff | Refresh `docs/execution-plan.md` to reflect current GitHub state and begin issue #206 (DPO concurrency litmus: spec note + tests). | The execution plan had drifted (closed issues and PR-queue work still marked as active), which wastes triage time. Issue #206 is a leverage point: it turns DPO concurrency claims into pinned, executable cases. | The active frontier is now issue-driven again, and #206 work proceeds as a single-purpose branch with deterministic litmus tests + a small spec note. | | ||
| | 2026-01-03 | CI: CodeRabbit merge gate | Remove the `PR Merge Gate / CodeRabbit approval required` workflow and treat CodeRabbit approval as a procedural merge requirement instead of a racing status check. | The gate job commonly runs before CodeRabbit can submit a review, producing expected failures and stale red checks; it adds noise without increasing correctness. | PR checks are less noisy; CodeRabbit approval remains required by policy/procedure rather than a flaky “must-pass” status. (Tracking: GitHub issue #248.) | |
There was a problem hiding this comment.
Missing explicit evidence links in decision-log entry.
The 2026-01-03 M2.1 decision captures rationale and consequence but omits file references (SPEC-0003-dpo-concurrency-litmus-v0.md, crates/warp-core/tests/dpo_concurrency_litmus.rs). Per the retrieved learning ("record milestones, blockers, and decisions"), including the concrete deliverable names here keeps triage deterministic and makes it trivial to locate the artifacts 12 months from now. The execution-plan already has these links; consistency demands the decision-log also carry them as inline parenthetical references or footnote.
🤖 Prompt for AI Agents
In docs/decision-log.md around lines 9 to 10, the 2026-01-03 M2.1 decision is
missing explicit evidence links to the concrete deliverables; add inline
parenthetical references (or a footnote) identifying
SPEC-0003-dpo-concurrency-litmus-v0.md and
crates/warp-core/tests/dpo_concurrency_litmus.rs (and optionally the related
issue/PR numbers), matching the same link/format style used in
docs/execution-plan.md so readers can jump directly to the artifacts.
| > 2026-01-03 — Merge-train: oldest open PRs (#220 → #227 → #242) (DONE) | ||
| - Outcome: no open PRs remained by the end of the merge-train; the last merges landed on `main` (see merge commit `8082bbb`). | ||
| - Next: treat subsequent work as issue-driven (one branch per issue), starting with #206. |
There was a problem hiding this comment.
PR #213 entry conflates merge outcome with a procedural policy decision.
Lines 55–58 describe "no open PRs remained" and "treat subsequent work as issue-driven," but these are two distinct events: (1) a concrete outcome of a specific PR merge, and (2) a governance/process change. The outcome statement is vague (which commits merged? where are the hashes?) and lacks evidence. The policy restatement ("treat subsequent work as issue-driven") is a principle, not a consequence of PR #213. Recommend: split into two entries—one for the merge completion (with commit hashes) and one for the process policy adoption (under a separate planning/governance decision). Alternatively, clearly scope this entry to "we closed the merge-train" and defer the policy rationale to a dedicated governance entry.
🤖 Prompt for AI Agents
In docs/execution-plan.md around lines 55 to 58, the PR #213 note currently
mixes a merge outcome with a policy decision; split them into two distinct
entries: one concise merge-train outcome that records which commits/merge hashes
landed (add the specific merge commit hashes and any relevant commit messages or
links), and a separate governance/planning entry that states the new
"issue-driven" workflow decision and rationale; alternatively, scope this line
to only confirm the merge-train closure and move the policy sentence into a new
governance section with its own date and reference.
| 1. M2.1: DPO concurrency theorem coverage (issue #206) | ||
| 2. Demo 2: Splash Guy docs course modules (issue #226) | ||
| 3. Demo 3: Tumble Tower Stage 1 physics (issue #232) | ||
| 4. TT1 follow-ons: dt policy / retention / merge semantics / capabilities (issues #243–#246) |
There was a problem hiding this comment.
Next Up Queue prioritization lacks context or blockers.
The reprioritized queue (M2.1 → Demo 2 → Demo 3 → TT1 → ECS) provides a linear order but no justification, blocking criteria, or dependencies. Per the learnings ("capture milestones, blockers, and decisions"), the queue should note:
- Any hard blockers (e.g., Demo 2 blocked on M2.1 completion?),
- Why items were reordered (relative importance, new discoveries, external constraints?),
- Whether Demo 2/3 and TT1 are newly added or promoted from the backlog.
As written, the queue looks like a snapshot taken at a moment in time with no guardrails against silent drift. Add brief context comments or cross-references to GitHub issues/milestones so future Codex knows the reasoning.
🤖 Prompt for AI Agents
docs/execution-plan.md around lines 1061 to 1064: the reprioritized next-up
queue lists items in order but lacks context, blockers, and rationale; update
the list to annotate each entry with (1) any hard blockers or dependencies
(e.g., link to issue/milestone that blocks it), (2) the reason for its new
priority (e.g., impact, customer request, discovery, resource availability), and
(3) whether the item was promoted from backlog or newly added, by appending
brief parenthetical notes and cross-references to GitHub issues/milestones so
future readers can understand the decisions and know where to find detailed
discussion.
Closes #206
Validation: