Address Whitaker Dylint suite review follow-ups (leynos/netsuke#410 rollout)

[leynos]

Summary

This pull request addresses the review feedback on the Whitaker Dylint suite adoption (merged in #269), part of the estate-wide rollout that began with leynos/netsuke#410. Each review finding was re-verified against the current tree before being changed; still-valid issues were fixed and the rest skipped with a reason.

Changes

CI

• The installer step now reuses a cached whitaker-installer only when its --version output matches the pinned WHITAKER_INSTALLER_VERSION, rather than trusting command -v alone; a mismatched binary triggers a fresh install.

Behavioural streams (src/dbsp_circuit/streams/behaviour/)

• apply_movement now folds duplicate decisions per entity before the join, sharing decide.rs's dedupe_movement_decisions helper, so the join can never apply a doubled delta. Added end-to-end tests feeding base positions plus movement decisions (including duplicates) and asserting the emitted positions.
• Removed the total_weight == 0 reset blocks from MovementAccumulator::apply/merge; into_decision's zero-weight handling is unchanged.
• Removed the speculative StreamConcat trait — the call site now uses fears.plus(&missing) directly — and dropped a redundant fears clone.
• The tests reuse the existing build_decision_circuit fixture for all three previously-inlined circuit builds, and the #[expect(unused_mut)] binding is gone.

DBSP output application (src/dbsp_sync/output/)

• Replaced the apply_output_records! macro with explicit resolve_write_target/resolve_health_target helpers, removing the pattern-fragment binding and the cognitive_complexity suppression.
• Position, velocity, and health application now inspect each consolidated Z-set weight and skip non-positive (retraction/zero) records before mutating ECS state; retractions no longer touch applied_health or the world handle.
• apply_dbsp_outputs_system clears circuit inputs on the step-error path too, so buffered inputs cannot replay on the next tick.
• Added tests for the failure/retry path and the negative-weight path.

Map lifecycle (src/map/lifecycle.rs)

• Asset-path validation splits on / and rejects only whole .. components, so filenames such as maps/primary..backup.tmx are accepted; added a covering case.
• build_spawn_rejects_invalid_path_without_spawning now registers an observer for InvalidPrimaryMapAssetPath and asserts it fired, so the test cannot pass spuriously when the AssetServer is absent.
• monitor_primary_map_load_state borrows tracking.handle.as_ref() instead of cloning it.

Documentation and tooling

• Oxford -ize spelling for initialize/initialization in plugin.rs and logging.rs prose (identifiers left untouched).
• The Makefile lint recipe gained set -euo pipefail so a failing Clippy step is no longer masked by the .ONESHELL exit code — the gate was previously reporting success even when Clippy failed.

Deliberately not changed

• The #[expect(clippy::too_many_arguments)] / #[expect(clippy::needless_pass_by_value)] suppressions in lifecycle.rs were left in place: only one is too_many_arguments, and all three document permanent Bevy observer/system-signature constraints rather than in-progress work. They are load-bearing #[expect]s — removing them re-triggers the very lints — and there is no issue to reference because there is no pending work to track.
• The warning-level asks for a new docs/developers-guide.md and for property/proof tests (proptest/Kani/Verus) are out of scope for this minimal follow-up; the concrete example tests requested inline were added.

Validation

• make check-fmt, make lint (Clippy and the Whitaker Dylint suite), make typecheck, and make test all pass — 162 unit tests, all integration/behavioural suites, and 42 doc-tests, with 0 failures.

References

• Estate-wide rollout: leynos/netsuke#410
• Original adoption PR: Adopt the Whitaker Dylint suite in the lint gate and CI #269
• Lody session: https://lody.ai/leynos/sessions/61b9149a-da4e-46f1-a226-ed09d1a4e3ad

🤖 Generated with Claude Code

[sourcery-ai]

Sorry @leynos, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The changes update movement deduplication, DBSP output application and rollback handling, primary-map path validation, Whitaker CI installation, lint execution, diagnostics, tests, and related documentation.

Changes

Behaviour stream processing

Layer / File(s)	Summary
Movement deduplication and application src/dbsp_circuit/streams/behaviour/decide/*, src/dbsp_circuit/streams/behaviour/apply.rs	Movement accumulators now preserve pending direction through net-zero merges, emit only valid consolidated decisions, and validate movement application with circuit tests.
Behaviour stream validation src/dbsp_circuit/streams/behaviour/fear.rs, src/dbsp_circuit/streams/behaviour/tests.rs	Fear streams use direct antijoins and additions, while behavioural tests use shared circuit construction and verify consolidated multiplicity.

DBSP output synchronisation

Layer / File(s)	Summary
Frame tracking and step failure handling src/dbsp_sync/state.rs, src/dbsp_sync/input/mod.rs, src/dbsp_sync/output/mod.rs	Per-frame backups and undo logs now commit after successful steps and restore health, damage, and deduplication state after failures.
Output target resolution and application src/dbsp_sync/output/mod.rs	DBSP entity IDs resolve explicitly to ECS components; positive-weight position, velocity, and health records are converted and applied with validation.
Output failure-path validation src/dbsp_sync/output/tests/*	Tests cover step errors, input clearing, rollback, unresolved entities, and ignored negative-weight outputs.
Synchronisation documentation docs/dbsp-synchronization-guide.md, docs/lille-physics-engine-design.md, docs/README.md	Documentation describes scheduling, rollback, step failures, output weights, testing, and the new guide links.

Map lifecycle handling

Layer / File(s)	Summary
Map path validation and dependency tracking src/map/lifecycle/*, docs/users-guide.md, docs/lille-isometric-tiled-maps-design.md	Primary-map paths now reject empty, rooted, absolute, and component-based traversal paths while preserving valid relative paths; lifecycle tests cover errors and spawn guards.

Tooling and diagnostics maintenance

Layer / File(s)	Summary
Lint installation and shell handling .github/workflows/ci.yml, Makefile	CI checks the pinned Whitaker installer version and falls back to locked Cargo installation; lint commands use strict shell handling.
Initialization diagnostics src/logging.rs, src/dbsp_sync/plugin.rs	Logging context, documentation, and diagnostic wording are standardised.

Sequence Diagram(s)

sequenceDiagram
  participant BevyInput
  participant DbspState
  participant DbspCircuit
  participant BevyOutput
  BevyInput->>DbspState: begin and stash frame rollback state
  BevyInput->>DbspCircuit: push cached inputs
  BevyOutput->>DbspCircuit: step_circuit()
  DbspCircuit-->>BevyOutput: outputs or step error
  BevyOutput->>DbspState: rollback and clear inputs on error
  BevyOutput->>BevyOutput: apply positive-weight outputs on success
  BevyOutput->>DbspState: commit frame tracking on success

Loading

Possibly related PRs

• leynos/lille#218: Introduces the health and damage bookkeeping restored by this PR’s DBSP rollback flow.
• leynos/lille#269: Modifies the same Whitaker CI and Makefile lint integration points.
• leynos/shared-actions#361: Implements related cached Whitaker installation and version-selection behaviour.

Suggested labels: Issue

Suggested reviewers: codescene-access

Poem

Movements merge, then choose their way,
DBSP rolls back a broken day.
Paths stay relative, maps stand still,
Whitaker sharpens every linting quill.
Tests keep the circuit bright—
Safe frames return to flight.

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (1 error, 3 warnings)

Check name	Status	Explanation	Resolution
Testing (Overall)	❌ Error	record_unsequenced_undo is only unit-tested in state.rs; no end-to-end test drives an unsequenced DamageEvent through DamageInbox, so removing the call in input/mod.rs would still pass.	Add a behavioural regression that enqueues an unsequenced damage event, forces a failed step, and asserts applied_unsequenced is restored after rollback.
Developer Documentation	⚠️ Warning	docs/developers-guide.md lacks the new dbsp_sync APIs; the rollback and output contract lives only in dbsp-synchronization-guide.md.	Add the new DBSP internal APIs and Whitaker/CI lint requirements to docs/developers-guide.md, and tick any completed roadmap/execplan items.
Testing (Property / Proof)	⚠️ Warning	The PR adds finite rstest matrices but no proptest/Kani/Verus coverage; the new guide explicitly says property-testing frameworks are not adopted, which contradicts the check.	Add property/bounded-model coverage for the new rollback, dedupe, output-weight, and path-validation invariants, or document an approved exception.
Architectural Complexity And Maintainability	⚠️ Warning	FAIL: apply_one_health_delta keeps an unimplemented if delta.death future hook, adding a dormant extension point with no caller or reuse plan.	Delete the branch or replace it with a tracked issue reference; keep only live behaviour in the output path.

✅ Passed checks (16 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the Whitaker Dylint review-follow-up changes and rollout scope.
Description check	✅ Passed	The description is directly aligned with the code changes and validation work in the pull request.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
User-Facing Documentation	✅ Passed	PASS: docs/users-guide.md documents the new primary_map validation rules and rejection behaviour, and docs/README.md links to the guide.
Module-Level Documentation	✅ Passed	Every Rust module in the touched subtrees has a leading //! doc header, and the new docs describe each module’s role and relations.
Testing (Unit And Behavioural)	✅ Passed	PASS: Added bounded unit tests for dedupe, rollback, path validation, and error/edge cases, plus circuit/App-level behavioural tests that step real DBSP and Bevy workflows.
Testing (Compile-Time / Ui)	✅ Passed	PASS: The PR adds runtime regression tests only; no new compile-time/UI surface is introduced, and the repo already has a separate trybuild compile-pass harness.
Unit Architecture	✅ Passed	Fallibility stays explicit via Result/Option, side-effects sit in system/command boundaries, and tests exercise the real App/RootCircuit seams.
Domain Architecture	✅ Passed	Core dbsp_circuit stays free of Bevy/filesystem/transport deps; adapter concerns are isolated in dbsp_sync and map/lifecycle.
Observability	✅ Passed	PASS: Accept the new failure paths as observable; step/init, map-path, and load errors emit logged events, while dedupe/clamp warnings cover degraded-state branches.
Security And Privacy	✅ Passed	PASS: Added path validation and rollback/logging changes avoid secrets, over-broad permissions, and unsafe sinks; no new credentials or sensitive data exposure found.
Performance And Resource Use	✅ Passed	Only docs/comment text changed in the touched code; no new runtime loops, allocations, clones, blocking I/O, or growth risks were introduced.
Concurrency And State	✅ Passed	DbspState stays a NonSend resource, the sync chain is serialised with .chain(), and rollback/retry tests cover failed-step replay and undo restoration.
Rust Compiler Lint Integrity	✅ Passed	No dead_code/unused suppressions or lint-appeasement anchors were added; remaining clones are cheap, intentional copies or rollback snapshots.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch adopt-whitaker-followups

---

_{Comment @coderabbitai help to get the list of available commands.}

[leynos]

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph.

If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced.

src/dbsp_circuit/streams/behaviour/apply.rs

Comment on lines +182 to +196

    fn applies_movement_to_targeted_entity() {
        let (circuit, (base_in, movement_in, out)) =
            build_apply_circuit().expect("failed to build apply circuit");
        base_in.push(position(1, 0.0, 0.0), 1);
        movement_in.push(movement(1, 1.0, 0.0), 1);

        circuit.step().expect("dbsp step");

        // Entity 1 shifts by (1, 0) and is emitted exactly once.
        let (moved, weight) = single_position(&collect_positions(&out));
        assert_eq!(weight, 1);
        assert_eq!(moved.entity, 1);
        assert_relative_eq!(moved.x.into_inner(), 1.0);
        assert_relative_eq!(moved.y.into_inner(), 0.0);
    }

❌ New issue: Code Duplication
The module contains 2 functions with similar structure: tests.applies_movement_to_targeted_entity,tests.passes_unmoved_entity_through

[sourcery-ai]

Sorry @leynos, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c07bc3a75c

ℹ️ 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".

[buzzybee-df12]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[pandalump]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[leynos]

@coderabbitai Have the following now been resolved?

src/dbsp_circuit/streams/behaviour/decide.rs (1)

243-246: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add a cancellation-and-readdition regression test.
Exercise an accumulator containing east +1 and north -1, merge it to

net-zero weight, then add north +1; assert the final decision is east. The

previous reset logic returns north, so this test protects the fixed

order-sensitive behaviour.
As per coding guidelines, “bug fixes must include a regression unittest” and

tests “must fail for plausible incorrect implementations.”

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @src/dbsp_circuit/streams/behaviour/decide.rs around lines 243 - 246, Add a
regression unit test alongside the accumulator decision tests for the
order-sensitive cancellation case. Build an accumulator with east +1 and north
-1, merge it to net-zero weight, then add north +1 and assert the final decision
is east. Ensure the test exercises the existing merge method and fails if reset
logic incorrectly returns north.

Source: Coding guidelines

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="src/dbsp_circuit/streams/behaviour/decide.rs"

ast-grep outline "$file" --items all --view expanded
printf '\n--- accumulator implementation and tests ---\n'
rg -n -A35 -B12 \
  'struct MovementAccumulator|impl MovementAccumulator|fn (merge|into_decision)|#\[test\]|accumulator|cancell|readdition|east|north' \
  "$file"

Length of output: 7408

---

@leynos: Mark this finding as resolved.

src/dbsp_circuit/streams/behaviour/decide.rs now contains tests::net_zero_merge_preserves_pending_direction at Lines 305–333. It precisely covers the requested order-sensitive sequence:

1. Apply east with weight +1.
2. Merge north with weight -1, producing net-zero weight.
3. Apply north with weight +1.
4. Assert that the final decision is east (dx = 1.0, dy = 0.0).

The test invokes MovementAccumulator::merge directly and documents that the former reset-on-zero implementation would incorrectly yield north. The current MovementAccumulator::merge implementation at Lines 243–247 preserves the accumulated displacement across a zero-weight merge, so both the implementation and required regression coverage are present.

[pandalump]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/dbsp_circuit/streams/behaviour/decide/mod.rs (1)

172-186: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Add a rustdoc example for the deduplication contract.

Show duplicate weighted input producing one normalized decision and net-zero input
producing no decision. As per coding guidelines, “Function documentation must
include clear usage and outcome examples”.

Triage: [type:docstyle]

🤖 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 `@src/dbsp_circuit/streams/behaviour/decide/mod.rs` around lines 172 - 186,
Expand the rustdoc example for dedupe_movement_decisions to show duplicate
weighted MovementDecision inputs collapsing into one normalized decision, and
show opposing inputs whose weights net to zero producing no decision. Use the
existing function and MovementAccumulator contract, documenting both the input
and resulting output behavior.

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 `@docs/developers-guide.md`:
- Around line 42-44: Update the Rust code fences in the developer guide,
including the shown block and the additionally affected block, by replacing the
GitHub-unsupported `rust,ignore` fence identifier with `rust`. Leave the code
examples unchanged.

In `@docs/lille-physics-engine-design.md`:
- Around line 79-82: Update the synchronization-contract reference in the
documentation to use the inline Markdown link text “DBSP synchronization
developer's guide” targeting developers-guide.md, replacing the code-formatted
path while preserving the surrounding list and wording.

In `@docs/users-guide.md`:
- Around line 54-56: Update the Markdown link in the design-rationale reference
so the destination remains contiguous with the `](` marker, using either a
same-line destination or an angle-bracket destination; preserve the existing
link text and target.
- Around line 48-52: Update the primary_map validation description to state that
the plugin triggers and logs LilleMapError::InvalidPrimaryMapAssetPath before
skipping primary-map spawning. Remove the claim that runtime or test runs fail;
clarify that only test assertions may fail while the plugin continues without
panicking.

---

Outside diff comments:
In `@src/dbsp_circuit/streams/behaviour/decide/mod.rs`:
- Around line 172-186: Expand the rustdoc example for dedupe_movement_decisions
to show duplicate weighted MovementDecision inputs collapsing into one
normalized decision, and show opposing inputs whose weights net to zero
producing no decision. Use the existing function and MovementAccumulator
contract, documenting both the input and resulting output behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0c68ea94-9344-44e3-8e78-520fcc84d106

📥 Commits

Reviewing files that changed from the base of the PR and between 7ca0dc2 and 99ed30c.

📒 Files selected for processing (10)

• docs/README.md
• docs/developers-guide.md
• docs/lille-physics-engine-design.md
• docs/users-guide.md
• src/dbsp_circuit/streams/behaviour/decide/mod.rs
• src/dbsp_circuit/streams/behaviour/decide/tests.rs
• src/dbsp_circuit/streams/behaviour/tests.rs
• src/dbsp_sync/output/tests/edge_cases.rs
• src/dbsp_sync/state.rs
• src/map/lifecycle/tests.rs

[leynos]

@coderabbitai Have the following now been resolved?

src/dbsp_sync/state.rs (1)

127-130: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the frame-rollback lifecycle.
Add Rustdoc explaining when callers begin and stash a frame, record undo state before mutation, and commit or roll back, including usage and outcome examples. As per coding guidelines, “Function documentation must include clear usage and outcome examples.”
Also applies to: 148-152, 157-160, 167-187

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @src/dbsp_sync/state.rs around lines 127 - 130, Document the frame-rollback
lifecycle across begin_frame_rollback and the related stash, commit, and
rollback methods: explain when callers start and save a frame, capture undo
state before mutation, and either commit or roll back. Add clear Rustdoc usage
and outcome examples covering both successful commit and rollback paths, without
changing behavior.

Source: Coding guidelines

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="src/dbsp_sync/state.rs"

printf '%s\n' '--- DbspState outline ---'
ast-grep outline "$file" --items all --view expanded

printf '%s\n' '--- frame rollback methods and their Rustdoc ---'
rg -n -A45 -B12 \
  '///|pub\(crate\) fn (begin_frame_rollback|stash_frame_rollback|record_unsequenced_undo|commit_frame_tracking|rollback_frame_tracking)\s*\(' \
  "$file"

printf '%s\n' '--- lifecycle call sites ---'
rg -n -A12 -B10 \
  '\.(begin_frame_rollback|stash_frame_rollback|record_unsequenced_undo|commit_frame_tracking|rollback_frame_tracking)\s*\(' \
  src/dbsp_sync/input/mod.rs src/dbsp_sync/output/mod.rs src/dbsp_sync/state.rs

Length of output: 35188

---

@leynos src/dbsp_sync/state.rs — DbspState::{begin_frame_rollback, stash_frame_rollback, record_unsequenced_undo, commit_frame_tracking, rollback_frame_tracking}

Mark this finding as resolved.

DbspState::begin_frame_rollback now documents the complete frame-rollback lifecycle at Lines 120–163:

1. Start a fresh log at the cache pass.
2. Record applied_unsequenced undo state before mutation.
3. Stash the extracted health snapshots and pending damage retractions.
4. Commit tracking after a successful circuit step, or clear inputs and roll back tracking after a failed step.

The Rustdoc includes separate successful-commit and failed-step rollback examples, with stated outcomes. The implementation call sites match that documented order: src/dbsp_sync/input/mod.rs begins, records, and stashes the frame state; src/dbsp_sync/output/mod.rs clears inputs then rolls back on failure, and commits on success.

Validation used current-head source-graph inspection. Run the documentation build and relevant test suite in CI/local tooling for executable validation.

[buzzybee-df12]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[codescene-access]

No quality gates enabled for this code.

[leynos]

@coderabbitai resume@coderabbitai Have the following now been resolved?

src/dbsp_sync/output/tests/mod.rs (1)

118-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Use push_velocity_input here.
The position push already routes through its helper; the velocity push re-borrows DbspState by hand two lines later. Use the helper that was just added.

♻️ Proposed tidy-up

-    let state = app.world_mut().non_send_resource_mut::<DbspState>();
-    state.circuit.velocity_in().push(
+    push_velocity_input(
+        app,
         Velocity {
             entity: 1,
             vx: 1.0.into(),
             vy: 0.0.into(),
             vz: 0.0.into(),
         },
         1,
     );

As per coding guidelines, "Reuse equivalent in-repository implementations instead of introducing parallel implementations".

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @src/dbsp_sync/output/tests/mod.rs around lines 118 - 127, Replace the direct
DbspState velocity input mutation in the test with the existing
push_velocity_input helper, matching the position push path. Preserve the same
Velocity values and timestamp while removing the manual non-send resource borrow
and circuit access.

Source: Coding guidelines

src/dbsp_sync/state.rs (1)

258-286: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Route the pre-existing tests through the new state fixture too.
The fixture landed to kill the construct-and-expect boilerplate, yet these three tests still repeat it verbatim. Finish the job so there is one construction path.

♻️ Proposed adoption

#[rstest]
-    fn entity_lookup_uses_mapping() {
-        let mut state = DbspState::new().expect("failed to initialise DbspState for tests");
+    fn entity_lookup_uses_mapping(#[from(state)] state_result: Result<DbspState, dbsp::Error>) {
+        let mut state = state_result.expect("failed to initialise DbspState for tests");
         let entity = Entity::from_bits(42);

As per coding guidelines, "Use rstest fixtures for shared setup and to avoid repetition between tests."

🤖 Detailed instructions

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @src/dbsp_sync/state.rs around lines 258 - 286, Update the existing tests
new_state_starts_empty, entity_lookup_uses_mapping, and
duplicate_counter_reports_value to accept and use the shared state fixture
instead of constructing DbspState with DbspState::new().expect(...). Preserve
each test’s current assertions and setup mutations while ensuring all three use
the single fixture construction path.

Source: Coding guidelines

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- working tree ---'
git status --short

printf '%s\n' '--- output test module outline ---'
ast-grep outline src/dbsp_sync/output/tests/mod.rs --items all --view expanded

printf '%s\n' '--- output helpers and priming path ---'
rg -n -A42 -B12 \
  'fn (prime_state|push_position_input|push_velocity_input)\s*\(' \
  src/dbsp_sync/output/tests/mod.rs

printf '%s\n' '--- state test module outline ---'
ast-grep outline src/dbsp_sync/state.rs --items all --view expanded

printf '%s\n' '--- state fixture and specified tests ---'
rg -n -A45 -B14 \
  '#\[fixture\]|fn (state|new_state_starts_empty|entity_lookup_uses_mapping|duplicate_counter_reports_value)\s*\(' \
  src/dbsp_sync/state.rs

printf '%s\n' '--- direct DbspState construction in state tests ---'
rg -n -A5 -B5 \
  'DbspState::new\s*\(\)\s*\.expect|DbspState::new\s*\(\)' \
  src/dbsp_sync/state.rs

Length of output: 5044

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- candidate state modules ---'
fd -t f -a 'state(\.rs)?' src

printf '%s\n' '--- DbspState declarations and state-test symbols ---'
rg -n -A50 -B16 \
  'struct DbspState|impl DbspState|#\[fixture\]|fn (state|new_state_starts_empty|entity_lookup_uses_mapping|duplicate_counter_reports_value)\s*\(' \
  src/dbsp_sync

printf '%s\n' '--- direct DbspState construction in DBSP sync tests ---'
rg -n -A6 -B6 \
  'DbspState::new\s*\(\)\s*(?:\.expect)?' \
  src/dbsp_sync

Length of output: 25383

---

@leynos: Both findings are resolved at the current PR head.

1. src/dbsp_sync/output/tests/mod.rs — prime_state
   Lines 118–127 now call push_velocity_input(app, Velocity { ... }, 1) rather than borrowing DbspState and pushing directly into velocity_in. The velocity values and Z-set weight remain unchanged.

2. src/dbsp_sync/state/tests.rs — new_state_starts_empty, entity_lookup_uses_mapping, duplicate_counter_reports_value
   The state code has been reorganized into src/dbsp_sync/state/mod.rs and src/dbsp_sync/state/tests.rs. The three specified tests now each use #[rstest] with the shared #[fixture] fn state() -> Result<DbspState, dbsp::Error> via #[from(state)] state_result, then retain their original setup and assertions.

Mark both review findings resolved. Source-graph inspection confirms the refactors; run the formatter and relevant test suite in CI/local tooling for executable validation.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- relevant current files ---'
fd -t f . src docs .github | rg '(^|/)(input|output|state|failure_paths|edge_cases|tests|mod)\.rs$|developers-guide\.md$|dbsp.*guide\.md$|roadmap|execplan|plan'

printf '%s\n' '--- structure ---'
for file in \
  src/dbsp_sync/input/mod.rs \
  src/dbsp_sync/output/mod.rs \
  src/dbsp_sync/output/tests/mod.rs \
  src/dbsp_sync/output/tests/failure_paths.rs \
  src/dbsp_sync/output/tests/edge_cases.rs \
  src/dbsp_sync/state.rs \
  src/dbsp_circuit/streams/behaviour/decide/tests.rs \
  src/map/lifecycle/tests.rs
do
  if [ -f "$file" ]; then
    echo "### $file"
    ast-grep outline "$file" --items all --view expanded
  fi
done

printf '%s\n' '--- unsequenced input → rollback path and behavioural tests ---'
rg -n -A40 -B16 \
  '\b(DamageInbox|DamageEvent|ingest_damage_events|record_unsequenced_undo|record_duplicate_unsequenced_damage|begin_frame_rollback|stash_frame_rollback|rollback_frame_tracking|step_failure|failed_step)\b' \
  src/dbsp_sync

printf '%s\n' '--- output health-death hook ---'
rg -n -A35 -B15 \
  '\bapply_one_health_delta\b|\bdelta\.death\b|Future hook|death hook' \
  src/dbsp_sync/output

printf '%s\n' '--- bounded/property invariant coverage ---'
rg -n -i -A28 -B10 \
  '\b(proptest|quickcheck|kani|verus|bounded|matrix|invariant|rollback|dedupe|non.?positive|negative.?weight|validate_asset_path)\b' \
  src/dbsp_sync/state.rs \
  src/dbsp_sync/output/tests \
  src/dbsp_circuit/streams/behaviour/decide/tests.rs \
  src/map/lifecycle/tests.rs \
  Cargo.toml 2>/dev/null || true

printf '%s\n' '--- developer documentation and directly related planning artefacts ---'
for file in docs/developers-guide.md docs/dbsp-synchronization-guide.md docs/README.md; do
  if [ -f "$file" ]; then
    echo "### $file"
    sed -n '1,360p' "$file"
  else
    echo "MISSING: $file"
  fi
done
fd -t f . docs | rg -i '(roadmap|execplan|plan)' | sort | while read -r file; do
  echo "### $file"
  rg -n -i -A14 -B8 'dbsp|rollback|synchroni[sz]|whitaker|dylint|output' "$file" || true
done

Length of output: 251711

---

@leynos Current-head verdict: all 4 checks are resolved.

Check	Status	Current-head evidence
Testing (Overall)	Resolved	src/dbsp_sync/output/tests/failure_paths.rs::failed_step_rolls_back_unsequenced_damage_dedupe_state queues an unsequenced DamageEvent through DamageInbox, forces step_circuit() failure, runs the actual application pipeline, and asserts that DbspState::applied_unsequenced exactly matches its pre-frame state. The test explicitly covers the absent-before-frame case, so removing record_unsequenced_undo from src/dbsp_sync/input/mod.rs::ingest_damage_events would leave the added entry behind and fail the test.
Developer Documentation	Resolved	docs/developers-guide.md now includes DBSP synchronization. It documents cache_state_for_dbsp_system → apply_dbsp_outputs_system, all five rollback lifecycle methods, failure behaviour (clear inputs, restore tracking, emit DbspSyncError, skip ECS writes), and positive-weight-only output application. Its Commit gates section documents make lint, rustdoc, Clippy, Whitaker, and CI installer behaviour. It links to docs/dbsp-synchronization-guide.md for the detailed contract. No directly related open roadmap or execplan item exists to tick.
Testing (Property / Proof)	Resolved	The PR uses documented bounded-model coverage: applied_unsequenced_rollback_matrix covers rollback/commit state transitions; dedupe_emits_one_decision_for_positive_weight_and_none_for_zero covers weighted movement dedupe; validate_asset_path_component_matrix covers separators, traversal insertion positions, safe .. substrings, and rooted paths; negative-weight output regressions cover position, velocity, and health. docs/dbsp-synchronization-guide.md records the finite-domain rationale and links the approved bounded-rstest ADR.
Architectural Complexity and Maintainability	Resolved	src/dbsp_sync/output/mod.rs::apply_one_health_delta no longer contains an if delta.death branch, future hook, or stale documentation. It now contains only live target resolution, dedupe/retraction filtering, clamping, component mutation, and bookkeeping.

Run make check-fmt, make lint, and make test in CI or locally to validate compilation and execution. This review verified the current source graph only; the sandbox does not execute repository code.

[leynos]

@coderabbitai Have the following failed checks now been resolved?

If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures.

Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. (Treat documentation and validation coverage as in scope).

❌ Failed checks (1 error, 3 warnings)

Check name	Status	Explanation	Resolution
Testing (Overall)	❌ Error	record_unsequenced_undo is only unit-tested in state.rs; no end-to-end test drives an unsequenced DamageEvent through DamageInbox, so removing the call in input/mod.rs would still pass.	Add a behavioural regression that enqueues an unsequenced damage event, forces a failed step, and asserts applied_unsequenced is restored after rollback.
Developer Documentation	⚠️ Warning	docs/developers-guide.md lacks the new dbsp_sync APIs; the rollback and output contract lives only in dbsp-synchronization-guide.md.	Add the new DBSP internal APIs and Whitaker/CI lint requirements to docs/developers-guide.md, and tick any completed roadmap/execplan items.
Testing (Property / Proof)	⚠️ Warning	The PR adds finite rstest matrices but no proptest/Kani/Verus coverage; the new guide explicitly says property-testing frameworks are not adopted, which contradicts the check.	Add property/bounded-model coverage for the new rollback, dedupe, output-weight, and path-validation invariants, or document an approved exception.
Architectural Complexity And Maintainability	⚠️ Warning	FAIL: apply_one_health_delta keeps an unimplemented if delta.death future hook, adding a dormant extension point with no caller or reuse plan.	Delete the branch or replace it with a tracked issue reference; keep only live behaviour in the output path.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '%s\n' '--- relevant files ---'
fd -t f . src docs | rg '(^|/)(input|output|state|failure_paths|edge_cases|tests|mod)\.rs$|developers-guide\.md$|dbsp.*guide\.md$|roadmap|execplan|plan' | sort

printf '%s\n' '--- focused outlines ---'
for file in \
  src/dbsp_sync/input/mod.rs \
  src/dbsp_sync/output/mod.rs \
  src/dbsp_sync/output/tests/failure_paths.rs \
  src/dbsp_sync/state.rs \
  src/dbsp_sync/state/mod.rs \
  src/dbsp_sync/state/tests.rs \
  src/dbsp_circuit/streams/behaviour/decide/tests.rs \
  src/map/lifecycle/tests.rs
do
  if [ -f "$file" ]; then
    echo "### $file"
    ast-grep outline "$file" --items all --view expanded
  fi
done

printf '%s\n' '--- unsequenced damage ingestion, failure rollback, and behavioural regression ---'
rg -n -A55 -B18 \
  '\b(DamageInbox|DamageEvent|ingest_damage_events|record_unsequenced_undo|record_duplicate_unsequenced_damage|rollback_frame_tracking|failed_step_rolls_back_unsequenced_damage_dedupe_state|force.*fail|step_failure)\b' \
  src/dbsp_sync

printf '%s\n' '--- health-output implementation and dormant death hook search ---'
rg -n -A45 -B18 \
  '\b(apply_one_health_delta|apply_health_deltas|delta\.death|Future hook|death hook)\b' \
  src/dbsp_sync/output

printf '%s\n' '--- bounded invariant and output-weight coverage ---'
rg -n -i -A32 -B10 \
  '\b(applied_unsequenced_rollback_matrix|dedupe_emits_one_decision_for_positive_weight_and_none_for_zero|validate_asset_path_component_matrix|negative_weight_(position|velocity|health)|bounded|matrix|invariant)\b' \
  src/dbsp_sync src/dbsp_circuit/streams/behaviour/decide src/map/lifecycle

printf '%s\n' '--- developer documentation ---'
for file in docs/developers-guide.md docs/dbsp-synchronization-guide.md; do
  if [ -f "$file" ]; then
    echo "### $file"
    rg -n -A22 -B8 \
      'DBSP|synchroni[sz]|begin_frame_rollback|record_unsequenced_undo|stash_frame_rollback|commit_frame_tracking|rollback_frame_tracking|positive.weight|Whitaker|make lint|CI' \
      "$file"
  else
    echo "MISSING: $file"
  fi
done

printf '%s\n' '--- DBSP-related roadmap/plan references ---'
fd -t f . docs | rg -i '(roadmap|execplan|plan)' | while read -r file; do
  echo "### $file"
  rg -n -i -A12 -B8 'dbsp|synchroni[sz]|rollback|whitaker|dylint' "$file" || true
done

Length of output: 236047

---

@leynos: Mark all four checks resolved. Current-head codegraph inspection confirms the required implementation, documentation, and coverage are present.

Check	Status	Current-head evidence
Testing (Overall)	Resolved	src/dbsp_sync/output/tests/failure_paths.rs::failed_step_rolls_back_unsequenced_damage_dedupe_state queues an unsequenced DamageEvent through DamageInbox, runs the real DbspPlugin cache→output chain, forces step_circuit() to fail, and asserts DbspState::applied_unsequenced is restored exactly to its pre-frame state. It specifically proves the absent-before-frame entry is removed after rollback, so deleting src/dbsp_sync/input/mod.rs::ingest_damage_events’ call to record_unsequenced_undo would fail the regression.
Developer Documentation	Resolved	docs/developers-guide.md now has a DBSP synchronization section documenting the chained input/output order, all five rollback lifecycle methods, failed-step handling, and positive-weight-only ECS output application. Its Commit gates section documents rustdoc, Clippy, Whitaker Dylint, and the CI installer/version-validation flow. It links to docs/dbsp-synchronization-guide.md for the detailed contract. No directly relevant unfinished roadmap or execplan item was identified.
Testing (Property / Proof)	Resolved	The repository uses bounded, finite-domain invariant coverage with its established rstest stack: applied_unsequenced_rollback_matrix covers prior/absent state, repeated undo recording, and commit versus rollback; dedupe_emits_one_decision_for_positive_weight_and_none_for_zero covers duplicate and cancelling movement decisions; validate_asset_path_component_matrix covers both separators, traversal insertion positions, safe .. substrings, and rooted paths. Dedicated negative-weight tests cover position, velocity, and health outputs. The developer documentation records this bounded-model strategy and rationale.
Architectural Complexity and Maintainability	Resolved	src/dbsp_sync/output/mod.rs::apply_one_health_delta contains only live health-update behaviour. The no-op if delta.death future hook and associated stale wording are absent.

Run make check-fmt, make lint, and make test in CI or locally before merging. This verification inspected the current source graph only and did not execute the Rust toolchain.