Establish the workspace and the demo harness contract (1.1) - #7
Conversation
Plan roadmap step 1.1: converting the single-crate repository into a Cargo workspace with one crate per architectural plane plus `harness` and `demos` crates, and implementing the shared demo harness proven by a `demo-empty` binary. The plan was revised after a six-lens design review before any implementation. Notable outcomes: - Camera contract types (`Quadrant`, `ZoomBounds`) live in `thysalion-presentation`, not the harness, so the shipping presentation plane never depends on demo scaffolding. - The harness is a public two-plugin contract (`HarnessCorePlugin` headless, `DemoHarnessPlugin` windowed) with a non-exhaustive builder-constructed `HarnessConfig` and Bevy 0.19 message-based actions. - Bevy is adopted with `default-features = false` and a curated feature list derived in a clean-container spike, because the default feature set would break all four CI workflows on missing system headers. - The coverage ratchet gains a documented headless exclusion boundary, and release builds are scoped with `-p thysalion` plus a workflow-shape guard. No implementation is included; the plan awaits approval per the ExecPlan gate.
|
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:
Summary
Automated checks pass. Manual GPU verification remains for interactive controls, overlay values, zoom bounds, and screenshot artefacts. WalkthroughThe change converts the repository into a Cargo workspace, adds presentation and harness contracts, introduces the ChangesWorkspace and build tooling
Sequence Diagram(s)sequenceDiagram
participant DemoEmpty
participant DemoHarnessPlugin
participant HarnessCore
participant WindowedHarness
participant ScreenshotSystem
DemoEmpty->>DemoHarnessPlugin: create HarnessConfig and start app
DemoHarnessPlugin->>HarnessCore: install core plugin and process actions
DemoHarnessPlugin->>WindowedHarness: spawn camera and overlay
WindowedHarness->>ScreenshotSystem: forward Screenshot actions
ScreenshotSystem-->>DemoEmpty: save numbered screenshot
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 19 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reverse the earlier deferral of `rstest-bdd` on user direction. The headless behavioural test becomes a Gherkin specification (`crates/harness/tests/features/harness.feature`, embedded in Stage B) run through an in-repo Bevy harness adapter using the crate's extensible harness support: `HarnessAdapter` with `type Context = bevy::app::App`, steps borrowing the app via the reserved `rstest_bdd_harness_context` fixture, per the third-party harness adapter cookbook in the rstest-bdd users' guide. Add the `rstest-bdd` 0.6.0-beta3 family to the approved dependency set and `[workspace.dependencies]`, add a Stage A spike proving the adapter compiles against Bevy 0.19, extend the developers' guide section to document the behavioural-test convention, and record the change in a second revision note. Unit-level mathematics remains plain rstest.
Convert the single-crate repository into a Cargo workspace with one crate per architectural plane (`thysalion-world`, `thysalion-sim`, `thysalion-knowledge`, `thysalion-presentation`) plus `thysalion- harness` and `thysalion-demos`, and implement the shared demo harness proven by the `demo-empty` binary. - The root `thysalion` package stays as the composition root; the lint deny table moves to `[workspace.lints]` and every member inherits it. - Bevy 0.19 is adopted with `default-features = false` and a curated feature list; the trim was verified in a clean container to need no system dev headers, and 0.19's umbrella features would otherwise drag in gamepad (libudev) and Wayland build dependencies. - The presentation crate owns the pure camera contract of design §8.2 (`Quadrant`, `ZoomBounds`); the harness consumes it, exposing the two-plugin contract (`HarnessCorePlugin` headless, `DemoHarnessPlugin` windowed) with a non-exhaustive builder-built `HarnessConfig`, message-based `HarnessAction`, and `DiagnosticsStore` paths including the `TICK_TIME` seam phase 4 writes through. - Behavioural tests use rstest-bdd 0.6.0-beta3 through a Bevy harness adapter whose context is a `MinimalPlugins` app; when `InputPlugin` is absent the core plugin expires synthetic input at end of frame, fixing a genuine double-rotate defect the suite caught. - Windowed-only modules and demo binaries sit outside the coverage ratchet via `cfg_attr(coverage_nightly, coverage(off))` and llvm-cov ignore patterns. - `make` targets gain `--workspace`; the release workflow builds `-p thysalion` only (demo crates do not cross-compile for the six release targets), guarded by a new workflow-shape test; Whitaker is bumped to installer 0.2.7 whose lint suite matches the pinned nightly-2026-05-28 toolchain (older suites cannot compile Bevy 0.19). Gates: `make check-fmt`, `make lint` (rustdoc, Clippy, Whitaker), `make test` (38 tests), and `make spelling` all pass.
`cargo run --bin demo-empty` fails in a workspace with a root package: Cargo only searches the root package's targets. Add `-p thysalion-demos` to the `make demo` target and the binary's doc comment. Verified by a 15-second windowed smoke run under WSLg: the window "Thysalion — demo-empty" opens and the renderer initializes without panics (software Vulkan on this GPU-less host).
Record the roadmap 1.1 delivery in the documentation set: - Add ADR-005 covering the workspace crate layout: the root package as composition root, `crates/*` members, the dependency-sink rule, the curated Bevy feature list, the coverage boundary, and the release `-p thysalion` scoping. - Rewrite repository-layout.md around the six-member workspace. - Add a demo harness section to the developers' guide (two-plugin contract, `HarnessSet` ordering, headless testing recipe) and a running-the-demos section to the users' guide (key bindings, screenshots directory). - Back-reference the crate layout from thysalion-design.md §6.1 and list the new documents in contents.md. - Tick roadmap tasks 1.1.1 and 1.1.2 as complete. - Close out the ExecPlan: progress, outcomes and retrospective, status COMPLETE. CodeRabbit reviewed the full branch with zero findings before this milestone.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @leynos, your pull request is larger than the review limit of 150000 diff characters
Log the post-delivery evidence gathered after the docs milestone: - Both pull-request CI runs green: 13 m 28 s on the code push and 4 m 24 s warm cache on the docs push, well inside the 30-minute tolerance; the Whitaker 0.2.7 installer path worked in CI unmodified. - CodeRabbit agent reviews of both milestones completed with zero findings. - The maintainer verified quadrant rotation in the windowed demo; remaining manual checks (zoom bounds, F3 overlay, F12 screenshot) stay tracked as the open post-delivery item.
The maintainer captured screenshots/demo-empty-1784931927.png with F12 in the windowed demo. The capture confirms the isometric ground-plane projection and shows the F3 overlay reporting live frame diagnostics (14 fps under llvmpipe software rendering; "tick: n/a" is correct because nothing measures into TICK_TIME until the simulation plane exists). Zoom bounds remain the only open manual check.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Address ten of the eleven findings from CodeRabbit's first PR review (one grammar suggestion declined in a reply — "an rstest-bdd" is correct, as the letter r opens with a vowel sound): - Clamp the initial zoom into the configured bounds in `RigState::from_config`, so a range excluding 1.0 starts in bounds. - Add a process-local sequence counter to screenshot filenames (`<slug>-<seconds>-<sequence>.png`), so two captures within the same second cannot collide. - Narrow the presentation crate's `float_arithmetic` expectation from module level to `Quadrant::yaw_radians`, `ZoomBounds::viewport_height`, and test-local sites; add a shared `assert_close` helper. - Share an rstest `bounds` fixture across the zoom tests. The fixture returns `ZoomBounds::default()` (the same 0.5–4.0 range): Whitaker's `no_expect_outside_tests` rejects `.expect()` in a fixture fn, and a single-line fixture body trips `unused_braces` through the macro expansion, so the body stays multi-line and infallible. - Harden the workflow-shape test: a `shared-actions` reference without `@<sha>` now fails instead of being skipped. - Grammar and clarity fixes in the presentation crate docs, the developers' guide, thysalion-design §6.1, and the ExecPlan; the ExecPlan status line now names the open zoom-bounds manual check.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Address all seven findings from CodeRabbit's second PR review: - Narrow the windowed camera module's `float_arithmetic` expectation from file level to the three functions that perform floating-point geometry (`sync_camera`, `camera_transform`, `shortest_angle_delta`); update ADR-005's allowance wording to match the function-scoped form. - Spawn one screenshot capture per queued action instead of coalescing multiple requests into a single capture. - Make the per-demo feature convention real: declare `demo-empty = []`, add `required-features` to the `[[bin]]`, and pass `--features demo-$(DEMO)` from the `make demo` target, so feature-gated demos stay runnable through the documented interface. - Workspace-qualify the demo run commands in the ExecPlan and the binary's doc comment (`make demo`, or the full cargo invocation). - Trim the ExecPlan retrospective's open-items list to the zoom-bounds check; the other items are recorded complete in Progress. - Name `SCREENSHOT_KEY` alongside `KEY_BINDINGS` in the users' guide, and document the screenshot filename's new sequence component in both guides. - Use "independently of" in ADR-005. CodeRabbit withdrew its round-1 "an rstest-bdd" grammar finding after the pronunciation-rule reply.
![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.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Address both findings from CodeRabbit's third PR review (all round-1 and round-2 threads were confirmed resolved): - Add screenshot scheduling tests: consecutive capture paths are distinct within one second, hostile slugs stay confined to the screenshots directory, and a mixed action batch spawns one capture entity per screenshot action (behavioural test over a minimal app). - Sanitize slugs to ASCII alphanumerics and hyphens when building capture paths, so a slug containing path separators cannot escape the screenshots directory. Record in the ExecPlan that `make demo` input validation is delegated to Cargo (an unknown DEMO fails with Cargo's own missing-feature error), and that the zoom-bounds manual check is the only open item.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The demo-empty scene test checked only that the ground mesh asset existed, so it would still pass if the plane were rebuilt at the wrong dimensions. It now computes the mesh bounding box and asserts the half-extents match `GROUND_SIZE / 2` in x and z, with a zero y extent proving the ground stays flat. Verified by mutation rather than by inspection: halving the plane's x size fails the test with "the ground must span 24 world units in x and z, got half-extents Vec3A(6.0, 0.0, 12.0)". The mutation was reverted and the suite re-run green.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
CodeScene flagged the previous commit's `startup_spawns_one_lit_ground_ plane` as a Large Method: 72 lines against a 70-line threshold. Splitting it four ways — entity counts, key-light contract, mesh extent, material appearance — over shared `ground_handles` and `key_light` helpers keeps every test under the threshold (largest is 26 lines) and makes a failure name the property that broke rather than the whole scene. No assertion was dropped or weakened.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
Close the follow-up "Testing (Overall)" finding. Every existing plugin test used the default configuration, so a regression that ignored a demo's settings and fell back to `HarnessConfig::default()` would have passed unnoticed. `core_plugin_tests.rs` drives a non-default configuration (slug `demo-custom`, zoom range 2.0-3.0, initial quadrant `SouthWest`) through `HarnessCorePlugin::new(...)` rather than by inserting resources. The range deliberately excludes the 1.0 baseline and shares no limit with the 0.5-4.0 default, so the initial clamp and both saturation points would each expose a fallback. Zoom actions flow through the plugin's own scheduled systems, in bounded loops. `windowed_tests/custom_configuration.rs` does the same through `DemoHarnessPlugin`: the camera spawns aimed at the configured quadrant, and the fixed-vertical viewport height derives from the configured bounds both at startup and after saturating zoom. It also asserts that the expected height differs from the default-bounds height, so the test can actually detect the fallback it is written to catch. Screenshot configuration is covered through the installed slug rather than by capturing, keeping the filesystem out of the test. `demo_guard.rs` gains two positive launcher tests beside the existing negative one; neither launches the windowed binary. They are complementary because no single invocation shows everything: `make --dry-run demo` prints the recipe before the shell runs it, proving the `--features` and `--bin` arguments stay quoted but leaving the slug as `demo-$DEMO_SLUG`; running with `CARGO` overridden to an echo stub lets the shell expand everything, proving the default path resolves to `demo-empty` and to no other slug, by which point the quotes are gone. `windowed_tests` was split into `mod.rs` plus `custom_configuration.rs` when the additions pushed it past the 400-line module limit.
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. crates/harness/src/core_plugin_tests.rs Comment on lines +127 to +138 fn zooming_in_clamps_at_the_configured_maximum(custom_config: HarnessConfig) {
let mut app = core_app(custom_config);
for _ in 0..ZOOM_STEPS_TO_SATURATE {
send_and_update(&mut app, HarnessAction::ZoomIn);
}
let zoom = rig_zoom(&app);
assert!(
(zoom - CUSTOM_MAX_ZOOM).abs() < f32::EPSILON,
"zoom must clamp at the configured maximum {CUSTOM_MAX_ZOOM}, got {zoom} (the default \
maximum is 4.0)"
);
}❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
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 state and relevant commits =='
git status --short
git log --oneline -15 -- \
crates/harness/src/input.rs \
crates/demos/src/bin/demo-empty.rs \
crates/harness/tests \
crates/harness/src \
crates/demos
printf '%s\n' '== Structural outlines =='
ast-grep outline crates/harness/src/input.rs --items all
ast-grep outline crates/demos/src/bin/demo-empty.rs --items all
printf '%s\n' '== Input paths and adjacent tests =='
rg -n -C 8 \
'(fn read_input|SCREENSHOT_KEY|AccumulatedMouseScroll|HarnessAction::(ZoomIn|ZoomOut|Screenshot)|just_released|#\[cfg\(test\)\]|#\[test\]|#\[rstest\])' \
crates/harness/src/input.rs crates/harness/tests crates/harness/src
printf '%s\n' '== Demo scene setup and tests =='
rg -n -C 10 \
'(fn (main|spawn_ground)|add_systems\(Startup|DirectionalLight|Mesh3d|MeshMaterial3d|StandardMaterial|illuminance|shadow_maps_enabled|#\[cfg\(test\)\]|#\[test\])' \
crates/demos/src/bin/demo-empty.rs crates/demos
printf '%s\n' '== Focused full files =='
cat -n crates/harness/src/input.rs
cat -n crates/demos/src/bin/demo-empty.rs
printf '%s\n' '== ExecPlan validation references =='
rg -n -C 3 \
'(scroll|F12|screenshot|demo-empty|spawn_ground|validation evidence|Validation)' \
docs/execplans/1-1-establish-the-workspace-and-the-demo-harness-contract.md \
|| trueLength of output: 146664
Current code at commit
The ExecPlan also records validation evidence for these additions. No further implementation, AI-agent prompt, or follow-up issue is required for this finding. |
CodeScene flagged `zooming_in_clamps_at_the_configured_maximum` and `zooming_out_clamps_at_the_configured_minimum` as duplicates: both built the same app, drove the same bounded loop, and asserted epsilon equality against one configured bound. They are now a single `zooming_clamps_at_the_configured_bound` with two `#[case]` entries carrying the action, the expected bound, its label, and the default bound for that direction. Two lint interactions shaped the result: - Four `#[case]` parameters plus an injected fixture would be five arguments, over the workspace `too_many_arguments` limit, so the configuration is built directly from the same fixture functions rather than injected. - `rstest` moves a `#[case]`-parameterized body into generated case functions, where a function-level `#[expect]` no longer applies, so the epsilon comparison moved into an `assert_zoom_at` helper carrying the existing `float_arithmetic` rationale. The diagnostic still names the configured bound, the observed value, and the default it must be distinguished from; verified by temporarily pointing a case at a wrong bound, which reports "zoom must clamp at the configured maximum 2.75, got 3 (the default maximum is 4)".
|
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 state and recent relevant commits =='
git status --short
git log --oneline -15 -- \
crates/harness/src/core_plugin_tests.rs \
crates/harness/src/windowed_tests.rs \
crates/harness/src/screenshot.rs \
crates/harness/tests \
tests/demo_guard.rs \
Makefile
printf '%s\n' '== Candidate test/module outlines =='
for file in \
crates/harness/src/core_plugin_tests.rs \
crates/harness/src/windowed_tests.rs \
crates/harness/src/screenshot.rs \
tests/demo_guard.rs
do
if test -f "$file"; then
printf '\n-- %s --\n' "$file"
ast-grep outline "$file" --items all
fi
done
printf '%s\n' '== Custom-config and launcher evidence =='
rg -n -C 8 \
'(Harness(Core|Demo)Plugin::new|HarnessConfig::new|with_(window_title|zoom_bounds|initial_quadrant)|CUSTOM_(MIN|MAX)_ZOOM|custom_config|CaptureSequence|capture_path|make --dry-run demo|--features|--bin|DEMO_ALLOWED|DEMO_SLUG|DEMO must be one of)' \
crates/harness tests Makefile \
|| true
printf '%s\n' '== Focused test source =='
for file in \
crates/harness/src/core_plugin_tests.rs \
crates/harness/src/windowed_tests.rs \
crates/harness/src/screenshot.rs \
crates/harness/tests/headless/support.rs \
tests/demo_guard.rs \
Makefile
do
if test -f "$file"; then
printf '\n== %s ==\n' "$file"
cat -n "$file"
fi
done
printf '%s\n' '== Existing open issues potentially covering remaining work =='
gh issue list --repo leynos/thysalion --state open --limit 100 \
--json number,title,url,body \
--jq '.[] | "#\(.number) \(.title)\n\(.url)\n\(.body)\n---"' \
|| trueLength of output: 95996
No further implementation work or follow-up GitHub issue is required for this check. Source inspection confirms the coverage exists; validate the command results in CI or a full development environment. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Rename the parameterized clamp test's case parameters and the helper's signature to `expected_bound`, `bound_name`, and `default_bound`, so each reads as the role it plays at the call site rather than as a bare `expected`/`bound` pair. Behaviour and assertion output are unchanged.
|
@coderabbitai please create a new GitHub issue for the following: Work on GitHub PR Resolve the remaining CodeScene code-duplication finding in:
Verify the current code before editing. The two tests both build the real Replace both tests with one parameterized #[rstest] named: zooming_clamps_at_the_configured_bound Add exactly these two cases:
Pass the case values as:
Keep the test’s existing behaviour:
Do not alter production harness code, HarnessConfig values, scheduling, Run and report actual results for:
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== Target test module and named symbols =='
ast-grep outline crates/harness/src/core_plugin_tests.rs --items all
rg -n -C 16 \
'fn zooming_(in_clamps_at_the_configured_maximum|out_clamps_at_the_configured_minimum)|ZOOM_STEPS_TO_SATURATE|send_and_update|rig_zoom|CUSTOM_(MIN|MAX)_ZOOM' \
crates/harness/src/core_plugin_tests.rs
printf '%s\n' '== Existing open issues that could cover this follow-up =='
gh issue list \
--repo leynos/thysalion \
--state open \
--limit 100 \
--search 'CodeScene duplication HarnessCorePlugin zoom clamp' \
--json number,title,url,body \
--jq '.[] | "#\(.number) \(.title)\n\(.url)\n\(.body)\n---"' || trueLength of output: 8405
No equivalent open issue exists, and opening one for already-completed work would create stale tracking. |
|
@coderabbitai review |
✅ Action performedReview finished.
|
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)
crates/demos/src/bin/demo-empty.rs (1)
93-108: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the temporary
Vecallocations from these singleton queries.
crates/demos/src/bin/demo-empty.rs#L93-L108: usequery.single(app.world()).expect("startup must spawn exactly one ground entity"), then clone the handles.crates/demos/src/bin/demo-empty.rs#L110-L124: usequery.single(app.world()).expect("startup must spawn exactly one directional light"), then copy the light.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/demos/src/bin/demo-empty.rs` around lines 93 - 108, Replace the temporary collection in ground_handles with a singleton query using query.single(app.world()).expect("startup must spawn exactly one ground entity"), then clone the mesh and material handles. Apply the same change in crates/demos/src/bin/demo-empty.rs lines 110-124 for the directional-light query: use single(...).expect("startup must spawn exactly one directional light") and copy the light value.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/harness/src/windowed_tests/custom_configuration.rs`:
- Around line 28-39: Convert custom_bounds and custom_config into rstest
fixtures by adding the appropriate #[fixture] attributes, preserving their
existing values and construction logic. Update all three tests in this module to
receive the relevant fixture parameters through rstest injection instead of
calling the setup functions directly.
In `@tests/demo_guard.rs`:
- Around line 9-11: Update the module-level documentation comment in
tests/demo_guard.rs to insert “in” between “launcher” and “two complementary
ways,” preserving the surrounding wording.
- Around line 28-33: Update run_make to remove inherited GNUMAKEFLAGS, CARGO,
and DEMO variables before spawning make, while preserving explicit command-line
arguments and the existing MAKEFLAGS/MFLAGS isolation. Apply the same
environment isolation to the other run_make call sites covered by this comment.
---
Outside diff comments:
In `@crates/demos/src/bin/demo-empty.rs`:
- Around line 93-108: Replace the temporary collection in ground_handles with a
singleton query using query.single(app.world()).expect("startup must spawn
exactly one ground entity"), then clone the mesh and material handles. Apply the
same change in crates/demos/src/bin/demo-empty.rs lines 110-124 for the
directional-light query: use single(...).expect("startup must spawn exactly one
directional light") and copy the light value.
🪄 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: e1fc3e86-df6e-4407-ab06-9b463e135021
📒 Files selected for processing (7)
crates/demos/src/bin/demo-empty.rscrates/harness/src/core_plugin_tests.rscrates/harness/src/lib.rscrates/harness/src/windowed_tests/custom_configuration.rscrates/harness/src/windowed_tests/mod.rsdocs/execplans/1-1-establish-the-workspace-and-the-demo-harness-contract.mdtests/demo_guard.rs
| fn custom_bounds() -> ZoomBounds { | ||
| match ZoomBounds::new(CUSTOM_MIN_ZOOM, CUSTOM_MAX_ZOOM) { | ||
| Ok(bounds) => bounds, | ||
| Err(error) => panic!("the custom zoom range must be valid: {error}"), | ||
| } | ||
| } | ||
|
|
||
| fn custom_config() -> HarnessConfig { | ||
| HarnessConfig::new(CUSTOM_SLUG) | ||
| .with_zoom_bounds(custom_bounds()) | ||
| .with_initial_quadrant(CUSTOM_QUADRANT) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- target file ---\n'
cat -n crates/harness/src/windowed_tests/custom_configuration.rs
printf '\n--- nearby tests using custom_bounds/custom_config ---\n'
rg -n "custom_bounds|custom_config|ZoomBounds::new\\(|HarnessConfig::new\\(" crates/harness/src/windowed_tests -S
printf '\n--- file outline ---\n'
ast-grep outline crates/harness/src/windowed_tests/custom_configuration.rs --view expandedRepository: leynos/thysalion
Length of output: 245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target file ---'
cat -n crates/harness/src/windowed_tests/custom_configuration.rs
printf '\n%s\n' '--- nearby tests using custom_bounds/custom_config ---'
rg -n "custom_bounds|custom_config|ZoomBounds::new\(|HarnessConfig::new\(" crates/harness/src/windowed_tests -S
printf '\n%s\n' '--- file outline ---'
ast-grep outline crates/harness/src/windowed_tests/custom_configuration.rs --view expandedRepository: leynos/thysalion
Length of output: 7945
Convert the shared setup into rstest fixtures. Use #[fixture] for custom_bounds and custom_config, then inject them into the three tests so the shared configuration lives in one place.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/harness/src/windowed_tests/custom_configuration.rs` around lines 28 -
39, Convert custom_bounds and custom_config into rstest fixtures by adding the
appropriate #[fixture] attributes, preserving their existing values and
construction logic. Update all three tests in this module to receive the
relevant fixture parameters through rstest injection instead of calling the
setup functions directly.
Source: Path instructions
| //! Neither positive test launches the windowed demo, which would open a | ||
| //! window and block. They inspect the launcher two complementary ways, | ||
| //! because no single invocation shows everything: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the missing preposition.
Change “They inspect the launcher two complementary ways” to “They inspect the launcher in two complementary ways”.
Triage: [type:grammar]
Proposed correction
-//! window and block. They inspect the launcher two complementary ways,
+//! window and block. They inspect the launcher in two complementary ways,🤖 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 `@tests/demo_guard.rs` around lines 9 - 11, Update the module-level
documentation comment in tests/demo_guard.rs to insert “in” between “launcher”
and “two complementary ways,” preserving the surrounding wording.
Source: Path instructions
| fn run_make(args: &[&str]) -> std::process::Output { | ||
| match Command::new("make") | ||
| .args(args) | ||
| .current_dir(env!("CARGO_MANIFEST_DIR")) | ||
| .env_remove("MAKEFLAGS") | ||
| .env_remove("MFLAGS") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the launcher tests hermetic.
run_make inherits DEMO and CARGO. An inherited CARGO can change the dry-run command, while an inherited DEMO can select another demo or make the “default” test fail before reaching the echo stub. Clear these child-process variables while retaining explicit command-line overrides.
Proposed isolation fix
.env_remove("MAKEFLAGS")
.env_remove("MFLAGS")
+ .env_remove("GNUMAKEFLAGS")
+ .env_remove("CARGO")
+ .env_remove("DEMO")
.output()Also applies to: 77-78, 103-107
🤖 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 `@tests/demo_guard.rs` around lines 28 - 33, Update run_make to remove
inherited GNUMAKEFLAGS, CARGO, and DEMO variables before spawning make, while
preserving explicit command-line arguments and the existing MAKEFLAGS/MFLAGS
isolation. Apply the same environment isolation to the other run_make call sites
covered by this comment.
3 participants
Summary
Delivers roadmap step 1.1 — both tasks 1.1.1 (workspace crate layout)
and 1.1.2 (shared demo harness) — per the ExecPlan at
docs/execplans/1-1-establish-the-workspace-and-the-demo-harness-contract.md(now Status: COMPLETE, with full progress notes, decision log, and
retrospective).
Workspace (1.1.1)
package
thysalionas composition root pluscrates/{world,sim, knowledge,presentation,harness,demos}.thysalion-worldis the dependency sink.[workspace.lints]) preserve the strict Clippypolicy across every member;
[workspace.dependencies]is the singleversion source (Bevy 0.19 with a curated feature list that needs
zero extra system packages in CI).
-p thysalionso demo crates neverenter the cross-compilation graph, guarded by a workflow-shape test.
Demo harness (1.1.2)
thysalion-presentationowns the pure camera contract:Quadrant(four yaw quadrants, cyclic) and
ZoomBounds(validated, clamping).thysalion-harnessis a two-plugin contract:HarnessCorePlugin(headless-safe: rig state, input mapping, diagnostics registration,
synthetic-input expiry) and
DemoHarnessPlugin(camera entity,F3 diagnostics overlay, F12 screenshots).
demo-emptyopens a window, renders a lit ground plane, and reportsdiagnostics. Run it with
make demo.harness through a Bevy
HarnessAdapterunderMinimalPlugins(38 tests green overall).
Documentation
ADR-005 (workspace crate layout), a rewritten repository-layout.md,
demo-harness sections in the developers' and users' guides, and roadmap
ticks for 1.1.1/1.1.2.
Verification
make check-fmt,make lint(rustdoc + Clippy + Whitaker dylint),make test(38/38),make spelling,make markdownlint, andmake nixieall pass locally before each commit.software Vulkan without panicking.
and zoom bounds, F3 overlay values, and an F12 screenshot artefact
(this host has no GPU passthrough).