Adopt the Whitaker Dylint suite in the lint gate and CI - #269
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe pull request adds DBSP behaviour and ECS output pipelines, extracts primary map lifecycle systems, makes test circuit construction fallible, introduces reusable physics test types, and expands Whitaker linting, CodeScene configuration, and test documentation. ChangesBehaviour streams
DBSP output bridge
Map lifecycle extraction
Fallible DBSP test infrastructure
Test utility package
Quality tooling and documentation
Sequence Diagram(s)sequenceDiagram
participant Inputs
participant movement_decision_stream
participant apply_movement
participant ECS
Inputs->>movement_decision_stream: provide fear, targets, and positions
movement_decision_stream->>apply_movement: emit deduplicated movement decisions
apply_movement->>ECS: emit updated positions
Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 7 warnings)
✅ Passed checks (10 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Prepare the codebase for adopting the Whitaker Dylint suite by fixing every finding it reports: - Split `map`, `dbsp_circuit::streams::behaviour`, `dbsp_sync::output`, and `test_utils::physics` into sibling submodules so no module exceeds the 400-line budget (`module_max_lines`). - Add `//!` inner doc comments to `__macro_deps`, thirteen in-source test modules, and the `dbsp_test_support` helper module, and move the `#![cfg(feature = "test-support")]` attribute below the existing doc comment in the shared test-support files (`module_must_have_inner_docs`). - Make test fixtures fallible instead of panicking: `new_circuit` (both the `test_utils` crate and the in-crate stream helper), `build_circuit`, `run_health_delta`, `fresh_state`, `make_state`, and `setup_app` now return `Result` and propagate errors; the test bodies unwrap, so a failure is the test verdict (`no_expect_outside_tests`). - Replace the tick counter's panicking `checked_add().expect()` with `saturating_add` inside the generator closure. - Convert `assert_health_delta` into a macro so panic locations point at the calling test and the fallible circuit run executes inside the test body. - Build the rspec `Configuration` directly from its public fields instead of the fallible builder, removing the panicking `unwrap_or_else` fallback (`no_unwrap_or_else_panic`). - Add a direct `dbsp` dependency to `test_utils` so its fixtures can name `dbsp::Error` in their return types.
Wire the Whitaker Dylint suite into the standard lint path as part of the estate-wide rollout (see leynos/netsuke#410): - Makefile: add a `WHITAKER ?= whitaker` tool variable and run the suite after Clippy in the `lint` target with `-D warnings` across all targets and features. - CI: install the suite via a pinned `whitaker-installer` (0.2.5) before the lint step, preferring `cargo binstall` and falling back to `cargo install --locked` when binstall is unavailable, with the installer binary and binstall cache keyed by OS, architecture, and installer version. The lint step now runs the suite after Clippy. The repository's existing `dylint.toml` already excludes the `build_support` build-script crate from `no_std_fs_operations`; no further exclusions were needed.
Deduplicate the record-twice duplicate-counting tests in `dbsp_sync::duplicate_filter` behind an `assert_duplicate_counted!` macro shared by the sequenced and unsequenced ingestion paths. A macro rather than a helper function keeps panic locations pointing at the calling test. Add a narrowly scoped `.codescene/code-health-rules.json` disabling Primitive Obsession for `test_utils/src/physics/units.rs` only: the module consists of newtype wrapper constructors, which necessarily accept primitives; it exists to remove primitive obsession from call sites, so the metric is a categorical false positive there.
The initial exact-file `matching_content_path` coincided with a CodeScene delta-analysis error. Use the documented directory-glob form scoped to the `test_utils` crate and drop the optional `usage` field, matching CodeScene's published examples.
Installer 0.2.5 provisions cargo-dylint 4.1.0, whose dylint driver cannot build on the suite's new nightly-2026-05-28 pin; 0.2.6 provisions cargo-dylint 6.0.1.
The Whitaker adoption branch's lint-driven refactors (helper extractions and module splits) landed without tests for the extracted paths, dropping line coverage to 84.3% against the CodeScene ratchet baseline of 85.4%. Cover the undercovered code introduced or moved by this branch: - Exercise the DBSP output guard paths: f32-from-f64 conversion bounds, out-of-range position and velocity records, and health deltas for unmappable, unknown, despawned, or component-less entities, plus clamping overkill damage to zero. Split the output tests into a directory module to stay within the file size budget. - Exercise the movement-decision accumulator: zero-displacement decisions, conflicting targets normalising to one averaged decision, and opposing targets cancelling to a zero vector. - Cover the map lifecycle helpers that need no asset backend: asset-path validation rejects empty, absolute, and parent-traversal paths, and the build-time spawn skips when disabled, already spawned, invalid, or missing an asset server. - Cover the sequenced duplicate filter's unsequenced early return. - Cover the test_utils constructors and assertion helpers that were previously only exercised by doctests, which the coverage run does not instrument. Line coverage rises from 84.67% to 89.65% under the CI feature set.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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_sync/duplicate_filter.rs Comment on file #[rstest]
fn sequenced_event_reapplied_in_next_frame_is_ignored() {
let mut state = fresh_state();
let mut state = fresh_state().expect("failed to initialise DbspState");❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
CodeScene flagged duplication between unsequenced_event_is_not_a_sequenced_duplicate and sequenced_event_reapplied_in_next_frame_is_ignored: both built a fresh DbspState and an empty seen set the same way before exercising their distinct branches of record_duplicate_sequenced_damage. Factor that construction into fresh_sequenced_filter, placed alongside fresh_state, and destructure its result in each test while keeping the expect calls local so failures still attribute to the calling test.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@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_sync/duplicate_filter.rs Comment on lines +156 to +157 let (mut state, mut seen) =
fresh_sequenced_filter().expect("failed to initialise DbspState");❌ New issue: Code Duplication |
Replace exact float equality assertions in the DBSP output edge-case tests with epsilon-bounded checks, matching the tolerance pattern used elsewhere in the test suite. Move monitor_primary_map_load_state ahead of the lifecycle test module so no items trail the test module.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Current PR revision =='
git rev-parse --short HEAD
git status --short
printf '%s\n' '== File outline =='
ast-grep outline src/dbsp_sync/duplicate_filter.rs --items all
printf '%s\n' '== Relevant tests and fixtures =='
rg -n -A180 -B20 \
'fn fresh_state\b|fn fresh_sequenced_filter\b|fn unsequenced_event_is_not_a_sequenced_duplicate\b|fn sequenced_event_reapplied_in_next_frame_is_ignored\b|fn sequenced_event\b|fn unsequenced_event\b' \
src/dbsp_sync/duplicate_filter.rs
printf '%s\n' '== Production duplicate-filter branches =='
rg -n -A100 -B15 \
'fn record_duplicate_sequenced_damage\b|fn record_duplicate_unsequenced_damage\b' \
src/dbsp_sync/duplicate_filter.rsLength of output: 11652
Suppress this CodeScene finding. Validate against current PR head Paste the following into the CodeScene diagnostic suppression field:
|
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| duplicate_filter.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
There was a problem hiding this comment.
Actionable comments posted: 17
🤖 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 @.github/workflows/ci.yml:
- Line 15: Update the Whitaker setup logic that uses WHITAKER_INSTALLER_VERSION
so an existing whitaker-installer is reused only when its --version matches the
pinned version; otherwise perform installation or setup for the requested
version. Do not rely on command -v alone, and preserve the existing behavior for
a matching installation.
In `@src/dbsp_circuit/streams/behaviour/apply.rs`:
- Around line 75-105: Update apply_movement’s movement stream before the
base_idx.join call to fold duplicate decisions by entity into one movement
decision, mirroring dedupe_movement_decisions in decide.rs. Use the deduplicated
stream for both duplicate validation and joining, while preserving the existing
warning/debug assertion behavior.
In `@src/dbsp_circuit/streams/behaviour/decide.rs`:
- Around line 215-240: Remove the total_weight == 0 reset blocks from
MovementAccumulator::apply and MovementAccumulator::merge, leaving their
additive sum and weight updates intact. Preserve into_decision’s existing
handling of zero total weight.
In `@src/dbsp_circuit/streams/behaviour/fear.rs`:
- Around line 11-19: Remove the speculative StreamConcat trait and its
implementation, then update the affected call site to invoke plus directly on
the fears stream with missing as its argument. Preserve the existing
concatenation behavior.
- Around line 77-88: Remove the redundant clone in the fear stream construction
by binding `explicit` directly to the borrowed `fears` reference. Keep the
existing `map_index` and `concat` calls unchanged, relying on their `&self`
receivers.
In `@src/dbsp_circuit/streams/behaviour/tests.rs`:
- Around line 119-122: Remove the #[expect(unused_mut)] annotation and the
unnecessary mutability from the circuit binding in this test, matching the
non-mut circuit bindings used by the other tests while preserving the existing
.step() call.
- Around line 65-73: Replace the inline RootCircuit construction in the affected
tests with the existing build_decision_circuit() fixture. Apply this to all
three duplicated setup blocks, remove now-unused target input/push handling
where applicable, and preserve each test’s existing assertions and inputs.
In `@src/dbsp_sync/output/mod.rs`:
- Around line 111-114: Remove the cognitive-complexity #[expect] from the
function containing should_apply_health_delta by extracting the entity/Health
resolution guard block into a helper such as resolve_health_target. Have the
helper return Option<Mut<Health>>, preserve all existing validation and
early-return behavior, and use its result in the parent function so the lint
passes without suppression.
- Around line 14-38: Replace the apply_output_records! macro with an explicit
helper function that resolves a record’s entity through state.id_map and
retrieves the mutable Transform, VelocityComp, and Health components from
DbspWriteQuery, returning None when either lookup fails. Update both call sites
to iterate records and apply their record-specific logic directly, preserving
the current skip behavior while avoiding pattern-fragment macro binding.
- Around line 115-190: In apply_health_deltas, inspect each consolidated OrdZSet
weight before calling should_apply_health_delta or mutating Health; apply only
positive weights and skip negative retractions (and zero weights). Ensure
retractions do not update applied_health or world_handle, rather than relying
solely on expected_health_retractions.
- Around line 26-38: Update the apply_output_records! macro to inspect each
record’s Z-set weight before mutating ECS state, and skip records with
non-positive weights (or explicitly handle retractions) so only positive output
records reach the query.get_mut and body operations. Preserve the existing
entity lookup and pattern-matching behavior for applicable records.
- Around line 200-225: Ensure apply_dbsp_outputs_system clears circuit inputs
even when state.step_circuit() returns an error. Move
state.circuit.clear_inputs() into a cleanup path that executes before both
success and early-return error paths, while preserving the existing output
application and error-trigger behavior.
In `@src/dbsp_sync/plugin.rs`:
- Line 95: Update the affected documentation wording in src/dbsp_sync/plugin.rs
at lines 95-95 and src/logging.rs at lines 58-58, replacing
“initialises”/“initialisation” with “initializes”/“initialization” as specified;
no code behavior changes are needed.
In `@src/map/lifecycle.rs`:
- Line 127: Update the asset path validation around the visible contains check
to split the path on `/` and reject only when a component equals `..`, allowing
filenames such as `maps/primary..backup.tmx`; add a test covering this accepted
case.
- Around line 295-301: Update build_spawn_rejects_invalid_path_without_spawning
to register an observer for InvalidPrimaryMapAssetPath before calling
try_spawn_primary_map_on_build, then assert the observer was triggered. Retain
the existing assertion that no PrimaryTiledMap entity is spawned, ensuring the
test verifies both rejection and absence of a map.
- Around line 190-193: Update the #[expect(clippy::too_many_arguments)]
suppressions in src/map/lifecycle.rs at lines 190-193, 231-234, and 312-315:
either add an issue or roadmap reference to each suppression to track the
in-progress work, or remove the suppressions if no tracking reference exists.
- Around line 325-327: Update the handle binding in the tracking lifecycle logic
to borrow via tracking.handle.as_ref() instead of cloning the Option or handle.
Preserve the early return when no handle is present, and continue obtaining the
identifier through handle.id().
🪄 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: c0808875-ccf2-4cdc-8055-445495eedf8f
📒 Files selected for processing (41)
.codescene/code-health-rules.json.github/workflows/ci.ymlMakefilesrc/dbsp_circuit/streams/behaviour.rssrc/dbsp_circuit/streams/behaviour/apply.rssrc/dbsp_circuit/streams/behaviour/decide.rssrc/dbsp_circuit/streams/behaviour/fear.rssrc/dbsp_circuit/streams/behaviour/mod.rssrc/dbsp_circuit/streams/behaviour/tests.rssrc/dbsp_circuit/streams/floor/tests.rssrc/dbsp_circuit/streams/health/tests.rssrc/dbsp_circuit/streams/kinematics/tests/motion.rssrc/dbsp_circuit/streams/kinematics/tests/position_floor.rssrc/dbsp_circuit/streams/test_utils.rssrc/dbsp_circuit/tests.rssrc/dbsp_sync/damage_inbox.rssrc/dbsp_sync/duplicate_filter.rssrc/dbsp_sync/input/mod.rssrc/dbsp_sync/mod.rssrc/dbsp_sync/output.rssrc/dbsp_sync/output/mod.rssrc/dbsp_sync/output/tests/edge_cases.rssrc/dbsp_sync/output/tests/mod.rssrc/dbsp_sync/plugin.rssrc/dbsp_sync/state.rssrc/lib.rssrc/logging.rssrc/map/lifecycle.rssrc/map/mod.rssrc/map/spawn.rssrc/map/translate.rssrc/numeric.rssrc/physics.rssrc/presentation.rstest_utils/Cargo.tomltest_utils/src/lib.rstest_utils/src/physics/mod.rstest_utils/src/physics/units.rstests/support/map_fixture.rstests/support/map_test_plugins.rstests/support/rspec_runner.rs
💤 Files with no reviewable changes (2)
- src/dbsp_sync/output.rs
- src/dbsp_circuit/streams/behaviour.rs
| env: | ||
| CARGO_TERM_COLOR: always | ||
| BUILD_PROFILE: debug | ||
| WHITAKER_INSTALLER_VERSION: '0.2.6' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- .github/workflows/ci.yml (relevant lines) ---'
nl -ba .github/workflows/ci.yml | sed -n '1,120p'
echo
echo '--- Whitaker references ---'
rg -n "WHITAKER_INSTALLER_VERSION|whitaker-installer|Whitaker" .github/workflows . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' || trueRepository: leynos/lille
Length of output: 240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- .github/workflows/ci.yml (first 140 lines with line numbers) ---'
awk '{ printf "%6d %s\n", NR, $0 }' .github/workflows/ci.yml | sed -n '1,140p'
echo
echo '--- Whitaker references ---'
rg -n "WHITAKER_INSTALLER_VERSION|whitaker-installer|Whitaker" .github/workflows . -g '!**/node_modules/**' -g '!**/dist/**' -g '!**/build/**' || trueRepository: leynos/lille
Length of output: 5453
Check the installed Whitaker version before skipping setup. command -v only proves the binary exists, so a preinstalled or stale whitaker-installer can bypass the pinned WHITAKER_INSTALLER_VERSION and run a different suite. Validate whitaker-installer --version before reusing it.
🤖 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 @.github/workflows/ci.yml at line 15, Update the Whitaker setup logic that
uses WHITAKER_INSTALLER_VERSION so an existing whitaker-installer is reused only
when its --version matches the pinned version; otherwise perform installation or
setup for the requested version. Do not rely on command -v alone, and preserve
the existing behavior for a matching installation.
| let mv = mv_base.inspect(|batch| { | ||
| // Accumulate counts per entity to catch duplicates emitted within a | ||
| // single tick. Duplicates indicate a bug upstream; release builds log | ||
| // the issue while debug builds panic for visibility. | ||
| use std::collections::HashMap; | ||
|
|
||
| let mut counts: HashMap<i64, i64> = HashMap::new(); | ||
| for (entity, _mv, weight) in batch.iter() { | ||
| *counts.entry(entity).or_default() += weight; | ||
| } | ||
| for (entity, total) in counts { | ||
| if total > 1 { | ||
| warn!("duplicate movement decisions for entity {entity}"); | ||
| } | ||
| debug_assert!( | ||
| total <= 1, | ||
| "duplicate movement decisions for entity {entity}" | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| let moved = base_idx.join(&mv, |_, p, &(dx, dy)| Position { | ||
| entity: p.entity, | ||
| x: OrderedFloat(p.x.into_inner() + dx.into_inner()), | ||
| y: OrderedFloat(p.y.into_inner() + dy.into_inner()), | ||
| z: p.z, | ||
| }); | ||
| let mv_entities = mv.map(|(e, _)| *e).map_index(|e| (*e, ())); | ||
| let unmoved = base_idx.antijoin(&mv_entities).map(|(_, p)| *p); | ||
| unmoved.plus(&moved) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Duplicate movement decisions are only detected, never corrected, in release builds.
debug_assert! (line 89) is compiled out in release, leaving only a warn!. The subsequent base_idx.join(&mv, ...) (line 96) still performs a full join, so if the invariant is ever violated an entity yields multiple Position rows in moved, silently corrupting the output Z-set that later drives ECS state — the invariant is checked, not enforced. apply_movement is public and exported independently of movement_decision_stream's dedupe, so nothing stops a future caller from feeding it un-deduplicated movement data.
Fold duplicate decisions per entity into a single one before joining (mirroring the dedupe_movement_decisions approach in decide.rs), rather than relying on the caller to have already deduplicated.
As per coding guidelines, "Commands must honestly enforce or delegate the invariants they claim to enforce" and code must not "rely on caller discipline or untrusted client-side checks."
🤖 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/apply.rs` around lines 75 - 105, Update
apply_movement’s movement stream before the base_idx.join call to fold duplicate
decisions by entity into one movement decision, mirroring
dedupe_movement_decisions in decide.rs. Use the deduplicated stream for both
duplicate validation and joining, while preserving the existing warning/debug
assertion behavior.
Source: Coding guidelines
| fn apply(&mut self, movement: &MovementDecision, weight: i64) { | ||
| let dx = movement.dx.into_inner(); | ||
| let dy = movement.dy.into_inner(); | ||
| #[expect( | ||
| clippy::cast_precision_loss, | ||
| reason = "Movement counts remain tiny so converting to f64 is exact" | ||
| )] | ||
| let scaled = weight as f64; | ||
| self.sum_dx = OrderedFloat(self.sum_dx.into_inner() + dx * scaled); | ||
| self.sum_dy = OrderedFloat(self.sum_dy.into_inner() + dy * scaled); | ||
| self.total_weight += weight; | ||
| if self.total_weight == 0 { | ||
| self.sum_dx = OrderedFloat(0.0); | ||
| self.sum_dy = OrderedFloat(0.0); | ||
| } | ||
| } | ||
|
|
||
| fn merge(&mut self, other: &Self) { | ||
| self.sum_dx = OrderedFloat(self.sum_dx.into_inner() + other.sum_dx.into_inner()); | ||
| self.sum_dy = OrderedFloat(self.sum_dy.into_inner() + other.sum_dy.into_inner()); | ||
| self.total_weight += other.total_weight; | ||
| if self.total_weight == 0 { | ||
| self.sum_dx = OrderedFloat(0.0); | ||
| self.sum_dy = OrderedFloat(0.0); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant file and nearby definitions.
file="src/dbsp_circuit/streams/behaviour/decide.rs"
wc -l "$file"
sed -n '1,320p' "$file"
# Search for the accumulator type and any related tests/docs.
rg -n "MovementAccumulator|Fold::with_output|into_decision|total_weight == 0|sum_dx|sum_dy" src docs tests -SRepository: leynos/lille
Length of output: 11749
🌐 Web query:
DBSP Fold with_output accumulator persists across circuit.step per key semantics
💡 Result:
In the DBSP (Differential Dataflow) computational model, state persistence across circuit steps is governed by the nature of the operators within the dataflow graph [1][2]. When using aggregation operators—which are conceptually equivalent to "fold" operations—DBSP maintains state to support incremental computation [1][3]. 1. State Persistence: DBSP operators that perform accumulations (such as group-by aggregates) are designed to be stateful. This state is preserved across calls to circuit.step [2]. Because DBSP is fundamentally incremental, these operators maintain an internal representation (an accumulator) that reflects the current state of the computation, which is updated with new input changes (deltas) in subsequent steps rather than being recomputed from scratch [1][2]. 2. Accumulator Semantics: The accumulator is effectively an internal state variable. For aggregation functions, this accumulator must implement specific traits (such as MonoidValue, Add, and MulByRef) to ensure that updates can be performed correctly using differential changes [3]. When a new batch of data is processed via circuit.step, the operator applies these updates to the existing state, persisting it automatically until the next step [1][2]. 3. Circuit Steps: A circuit.step call triggers the evaluation of the circuit for a single logical clock tick [4][5]. During this evaluation, the dataflow graph propagates changes, and any stateful operators update their internal accumulators based on the current input deltas [1][2]. The "per key" semantics are inherent to these operators, where the accumulator state is partitioned by the grouping key, ensuring that each key's aggregate is computed and persisted independently [3]. There is no "with_output" parameter that alters this fundamental persistence behavior in the context of the core DBSP library's aggregation operators [2][3]. If you are encountering "with_output" in relation to folding or state, it likely refers to a higher-level framework built on top of DBSP (such as application-specific pipelines or specialized export/manifest logic) rather than a core feature of the DBSP circuit execution model [6][7][8][9]. In standard DBSP, you control output through output handles (e.g., OutputHandle) which allow you to read the state of a stream at the end of a step [10].
Citations:
- 1: https://mihaibudiu.github.io/work/dbsp-spec.pdf
- 2: https://docs.rs/dbsp/latest/dbsp/circuit/index.html
- 3: https://docs.feldera.com/sql/udf/
- 4: https://docs.rs/dbsp/latest/dbsp/circuit/struct.DBSPHandle.html
- 5: https://docs.rs/dbsp/latest/dbsp/circuit/circuit_builder/struct.CircuitHandle.html
- 6: https://docs.rs/helios-persistence/latest/src/helios_persistence/core/bulk_export.rs.html
- 7: https://github.com/builtbyberry/laravel-swarm/blob/main/docs/streaming-substrate-author-guide.md
- 8: https://github.com/builtbyberry/laravel-swarm/releases/tag/v0.15.0
- 9: https://docs.fedimint.org/src/fedimint_client_module/transaction/builder.rs.html
- 10: https://docs.rs/dbsp/0.28.0/dbsp/tutorial/index.html
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '200,270p' src/dbsp_circuit/streams/behaviour/decide.rs
printf '\n--- tests/docs ---\n'
rg -n "MovementAccumulator|zero-reset|total_weight == 0|into_decision|Fold::with_output" src docs tests -SRepository: leynos/lille
Length of output: 4091
🔍 Additional research query:
Deepwiki: dbsp Fold operator accumulator persistent across steps semantics
💡 Result:
Yes—if you mean DBSP/Feldera’s aggregate/fold-style operator, the accumulator is persistent operator state across steps, not something reinitialized each step. DBSP is described as a standing-query engine that updates outputs from input changes while keeping only the state needed for future changes, and Feldera says aggregation operators store both input and output; for user-defined aggregates, the accumulator type must be storable in relations and can be spilled to disk. I infer from that that the accumulator is carried forward across steps as part of the operator’s persistent state. (docs.feldera.com)
Nuance: for linear aggregates like SUM, COUNT, AVG, and STDDEV, Feldera can avoid storing the full input relation, but it still maintains the aggregate state needed for incremental updates. (docs.feldera.com)
Delete the zero-reset in MovementAccumulator::apply and merge.
src/dbsp_circuit/streams/behaviour/decide.rs:215-240 — into_decision already returns None when total_weight == 0, so clearing sum_dx/sum_dy here only drops additive state and can skew the next update after a zero-crossing.
🤖 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.rs` around lines 215 - 240, Remove
the total_weight == 0 reset blocks from MovementAccumulator::apply and
MovementAccumulator::merge, leaving their additive sum and weight updates
intact. Preserve into_decision’s existing handling of zero total weight.
| trait StreamConcat { | ||
| fn concat(&self, other: &Self) -> Self; | ||
| } | ||
|
|
||
| impl StreamConcat for Stream<RootCircuit, OrdZSet<FearLevel>> { | ||
| fn concat(&self, other: &Self) -> Self { | ||
| self.plus(other) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Drop the speculative StreamConcat trait; call .plus() directly.
The trait exists purely to rename a single .plus() call at line 87; it adds a layer without a reuse story.
♻️ Proposed fix
-trait StreamConcat {
- fn concat(&self, other: &Self) -> Self;
-}
-
-impl StreamConcat for Stream<RootCircuit, OrdZSet<FearLevel>> {
- fn concat(&self, other: &Self) -> Self {
- self.plus(other)
- }
-}
-
/// Merges explicit fear inputs with entity positions, defaulting to zero.Then replace the trailing call: fears.plus(&missing).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| trait StreamConcat { | |
| fn concat(&self, other: &Self) -> Self; | |
| } | |
| impl StreamConcat for Stream<RootCircuit, OrdZSet<FearLevel>> { | |
| fn concat(&self, other: &Self) -> Self { | |
| self.plus(other) | |
| } | |
| } | |
| /// Merges explicit fear inputs with entity positions, defaulting to zero. |
🤖 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/fear.rs` around lines 11 - 19, Remove the
speculative StreamConcat trait and its implementation, then update the affected
call site to invoke plus directly on the fears stream with missing as its
argument. Preserve the existing concatenation behavior.
Source: Coding guidelines
| let explicit = fears.clone(); | ||
|
|
||
| let missing = positions | ||
| .map_index(|p| (p.entity, ())) | ||
| .antijoin(&explicit.map_index(|f| (f.entity, ()))) | ||
| .map(|(entity, ())| FearLevel { | ||
| entity: *entity, | ||
| level: OrderedFloat(0.0), | ||
| }); | ||
|
|
||
| explicit.concat(&missing) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Drop the redundant .clone().
fears is already a reference and both .map_index() and StreamConcat::concat() take &self; cloning buys nothing here.
♻️ Proposed fix
- let explicit = fears.clone();
-
let missing = positions
.map_index(|p| (p.entity, ()))
- .antijoin(&explicit.map_index(|f| (f.entity, ())))
+ .antijoin(&fears.map_index(|f| (f.entity, ())))
.map(|(entity, ())| FearLevel {
entity: *entity,
level: OrderedFloat(0.0),
});
- explicit.concat(&missing)
+ fears.concat(&missing)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let explicit = fears.clone(); | |
| let missing = positions | |
| .map_index(|p| (p.entity, ())) | |
| .antijoin(&explicit.map_index(|f| (f.entity, ()))) | |
| .map(|(entity, ())| FearLevel { | |
| entity: *entity, | |
| level: OrderedFloat(0.0), | |
| }); | |
| explicit.concat(&missing) | |
| } | |
| let missing = positions | |
| .map_index(|p| (p.entity, ())) | |
| .antijoin(&fears.map_index(|f| (f.entity, ()))) | |
| .map(|(entity, ())| FearLevel { | |
| entity: *entity, | |
| level: OrderedFloat(0.0), | |
| }); | |
| fears.concat(&missing) | |
| } |
🤖 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/fear.rs` around lines 77 - 88, Remove the
redundant clone in the fear stream construction by binding `explicit` directly
to the borrowed `fears` reference. Keep the existing `map_index` and `concat`
calls unchanged, relying on their `&self` receivers.
Source: Coding guidelines
|
|
||
| #[cfg(test)] | ||
| mod tests { | ||
| //! Tests for the DBSP synchronisation plugin. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Oxford spelling consistently in Rust documentation.
Replace initialises/initialisation with initializes/initialization in the affected test documentation.
src/dbsp_sync/plugin.rs#L95-L95: update the related test-module wording to use Oxford spelling.src/logging.rs#L58-L58: replaceinitialisationwithinitialization.
As per coding guidelines and path instructions, comments and documentation must use en-GB-oxendict spelling.
📍 Affects 2 files
src/dbsp_sync/plugin.rs#L95-L95(this comment)src/logging.rs#L58-L58
🤖 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_sync/plugin.rs` at line 95, Update the affected documentation
wording in src/dbsp_sync/plugin.rs at lines 95-95 and src/logging.rs at lines
58-58, replacing “initialises”/“initialisation” with
“initializes”/“initialization” as specified; no code behavior changes are
needed.
Sources: Coding guidelines, Path instructions
| }); | ||
| } | ||
|
|
||
| if asset_path.contains("..") { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject traversal components, not filename substrings.
Line 127 rejects valid relative assets such as maps/primary..backup.tmx. Split on / and reject only a segment equal to ..; add that filename as an accepted case.
🤖 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/map/lifecycle.rs` at line 127, Update the asset path validation around
the visible contains check to split the path on `/` and reject only when a
component equals `..`, allowing filenames such as `maps/primary..backup.tmx`;
add a test covering this accepted case.
| #[expect( | ||
| clippy::too_many_arguments, | ||
| reason = "Bevy observer systems require query parameters; grouping would obscure intent." | ||
| )] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '160,340p' src/map/lifecycle.rsRepository: leynos/lille
Length of output: 6547
Link or remove these #[expect] suppressions
src/map/lifecycle.rs:190-193, 231-234, and 312-315 all need an issue or roadmap reference; keep them only as tracked in-progress work, or drop them.
📍 Affects 1 file
src/map/lifecycle.rs#L190-L193(this comment)src/map/lifecycle.rs#L231-L234src/map/lifecycle.rs#L312-L315
🤖 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/map/lifecycle.rs` around lines 190 - 193, Update the
#[expect(clippy::too_many_arguments)] suppressions in src/map/lifecycle.rs at
lines 190-193, 231-234, and 312-315: either add an issue or roadmap reference to
each suppression to track the in-progress work, or remove the suppressions if no
tracking reference exists.
Sources: Coding guidelines, Path instructions
| fn build_spawn_rejects_invalid_path_without_spawning() { | ||
| let mut app = app_with_settings(true, "/absolute/path.tmx"); | ||
| try_spawn_primary_map_on_build(&mut app); | ||
| let world = app.world_mut(); | ||
| let mut maps = world.query_filtered::<Entity, With<PrimaryTiledMap>>(); | ||
| assert!(maps.iter(world).next().is_none()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section of the file with line numbers.
sed -n '1,380p' src/map/lifecycle.rs | cat -n
echo
echo '--- SEARCH InvalidPrimaryMapAssetPath ---'
rg -n "InvalidPrimaryMapAssetPath|try_spawn_primary_map_on_build|PrimaryTiledMap|AssetServer" src -SRepository: leynos/lille
Length of output: 16763
Make this build test assert rejection. Register an observer for InvalidPrimaryMapAssetPath and assert it fires; checking only for “no spawned map” still passes if try_spawn_primary_map_on_build returns early because AssetServer is absent.
🤖 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/map/lifecycle.rs` around lines 295 - 301, Update
build_spawn_rejects_invalid_path_without_spawning to register an observer for
InvalidPrimaryMapAssetPath before calling try_spawn_primary_map_on_build, then
assert the observer was triggered. Retain the existing assertion that no
PrimaryTiledMap entity is spawned, ensuring the test verifies both rejection and
absence of a map.
Sources: Coding guidelines, Path instructions
| let Some(handle) = tracking.handle.clone() else { | ||
| return; | ||
| }; |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/map/lifecycle.rs (around lines 300-360) ---'
sed -n '300,360p' src/map/lifecycle.rs
printf '\n%s\n' '--- search for handle type / id() usages ---'
rg -n "handle\.id\(\)|tracking\.handle|struct .*Handle|type .*Handle|enum .*Handle" src -g '!**/target/**'Repository: leynos/lille
Length of output: 2351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/map/lifecycle.rs (top and relevant sections) ---'
sed -n '1,140p' src/map/lifecycle.rs
printf '\n%s\n' '--- src/map/lifecycle.rs (handle query section) ---'
sed -n '236,262p' src/map/lifecycle.rs
printf '\n%s\n' '--- handle type definition / imports in file ---'
rg -n "use .*Handle|struct PrimaryMapAssetTracking|handle:" src/map/lifecycle.rsRepository: leynos/lille
Length of output: 5895
Borrow the tracked handle instead of cloning it.
src/map/lifecycle.rs:252 — Bind tracking.handle.as_ref() and call handle.id(). Cloning here bumps the strong count on every monitor tick without using the extra ownership.
🤖 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/map/lifecycle.rs` around lines 325 - 327, Update the handle binding in
the tracking lifecycle logic to borrow via tracking.handle.as_ref() instead of
cloning the Option or handle. Preserve the early return when no handle is
present, and continue obtaining the identifier through handle.id().
Source: Coding guidelines
Act on the review comments left on the Whitaker Dylint adoption (#269), verifying each finding against the current tree before changing it. - CI: reuse a cached whitaker-installer only when its --version matches the pinned WHITAKER_INSTALLER_VERSION rather than trusting `command -v` alone. - behaviour: dedupe movement decisions per entity inside apply_movement before the join (sharing decide.rs's dedupe helper) and add end-to-end tests; drop the total_weight == 0 reset blocks; remove the speculative StreamConcat wrapper and a redundant fears clone; reuse build_decision_circuit and drop the unused_mut expect in the tests. - dbsp_sync/output: replace the apply_output_records! macro with explicit resolve_write_target/resolve_health_target helpers, skip non-positive Z-set weights before mutating ECS, clear circuit inputs on the step-error path, and cover the failure and negative-weight paths with tests. - map/lifecycle: reject only whole `..` path components (accepting names such as maps/primary..backup.tmx), assert InvalidPrimaryMapAssetPath via an observer, and borrow tracking.handle instead of cloning it. - docs: use Oxford -ize spelling for initialize/initialization in plugin and logging prose. - Makefile: add `set -euo pipefail` to the lint recipe so a failing Clippy step no longer hides behind the .ONESHELL exit code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Installer 0.2.7 stops executing `dylint-link` as a health check (leynos/whitaker#299), so the verified repository artefact is accepted instead of triggering a source build that cannot compile on this repository's pinned toolchain. Adoption PR #269 merged with the 0.2.6 pin, which left the lint gate on `main` unable to install the suite. Co-authored-by: leynos <leynos@rohga>
Act on the review comments left on the Whitaker Dylint adoption (#269), verifying each finding against the current tree before changing it. - CI: reuse a cached whitaker-installer only when its --version matches the pinned WHITAKER_INSTALLER_VERSION rather than trusting `command -v` alone. - behaviour: dedupe movement decisions per entity inside apply_movement before the join (sharing decide.rs's dedupe helper) and add end-to-end tests; drop the total_weight == 0 reset blocks; remove the speculative StreamConcat wrapper and a redundant fears clone; reuse build_decision_circuit and drop the unused_mut expect in the tests. - dbsp_sync/output: replace the apply_output_records! macro with explicit resolve_write_target/resolve_health_target helpers, skip non-positive Z-set weights before mutating ECS, clear circuit inputs on the step-error path, and cover the failure and negative-weight paths with tests. - map/lifecycle: reject only whole `..` path components (accepting names such as maps/primary..backup.tmx), assert InvalidPrimaryMapAssetPath via an observer, and borrow tracking.handle instead of cloning it. - docs: use Oxford -ize spelling for initialize/initialization in plugin and logging prose. - Makefile: add `set -euo pipefail` to the lint recipe so a failing Clippy step no longer hides behind the .ONESHELL exit code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 participant
Summary
This pull request adopts the Whitaker Dylint suite in lille's lint gate and continuous integration, as part of the estate-wide rollout that began with leynos/netsuke#410. The suite runs after Clippy in
make lint, and CI installs it with a pinnedwhitaker-installer(0.2.5) usingcargo binstallwhere available and a lockedcargo installfallback otherwise.Running the suite surfaced findings across four lints, all of which are fixed here rather than suppressed:
module_max_lines: themap,dbsp_circuit::streams::behaviour,dbsp_sync::output, andtest_utils::physicsmodules exceeded the 400-line budget and are now split into focused sibling submodules, following the directory layout already used bystreams::floorandstreams::health.module_must_have_inner_docs:__macro_deps, thirteen in-source test modules, and thedbsp_test_supporthelper module gained//!inner doc comments; the shared test-support files now place their doc comment before the#![cfg(feature = "test-support")]attribute.no_expect_outside_tests: test fixtures (new_circuitin both thetest_utilscrate and the in-crate stream helper,build_circuit,run_health_delta,fresh_state,make_state, andsetup_app) now returnResultand propagate construction errors; unwrapping happens only in test bodies, where a failure is the test verdict. The sharedassert_health_deltahelper became a macro so panic locations point at the calling test.no_unwrap_or_else_panic: the rspec runner constructs itsConfigurationdirectly from public fields instead of using the fallible builder with a panicking fallback.The existing
dylint.tomlexclusion of thebuild_supportbuild-script crate fromno_std_fs_operationsremains sufficient; no new exclusions were required.Review walkthrough
WHITAKER ?= whitakervariable and the suite appended to thelinttarget.fear,decide,apply, andtestssubmodules.new_circuit.Configurationconstruction.Validation
env -u WHITAKER RUSTFLAGS="-D warnings" whitaker --all -- --all-targets --all-featuresexits 0.env -u WHITAKER make check-fmt lint typecheck test markdownlint nixiepasses in full: formatting clean,cargo docand Clippy warning-free, the Whitaker suite clean, all unit, integration, and behavioural tests green, Markdown lint clean, and Mermaid validation reporting no errors.