feat(29-7): jaquayed layout — cycle detection, ring placement, closure validation#443
Merged
feat(29-7): jaquayed layout — cycle detection, ring placement, closure validation#443
Conversation
…e validation
Adds cycle-aware dungeon layout built on top of the 29-6 shared-wall tree
placer. Three new public functions in sidequest-game::tactical::layout:
- detect_cycles: iterative three-colour DFS on the room graph, returning
fundamental cycles from back edges. Undirected traversal skips the
immediate-parent edge and ignores Black neighbours to prevent double-
counting. Determinism via BTreeMap-sorted adjacency.
- layout_cycle: walks a cycle in order, placing each successor using the
first-fit opposite-wall exit pair. After N rooms are placed, validates
the closing edge from the last room back to the first — if the computed
offset does not match the committed offset, returns CycleClosureFailed
with the cycle members and an actionable detail message.
- layout_dungeon: top-level entry point. Delegates to layout_tree for
acyclic graphs (behaviour-preserving), places a single cycle as a ring
then BFS-attaches tree branches via the existing 29-6 overlap-aware
placement logic, and fails loudly on multi-cycle graphs (deferred scope).
LayoutError gains a CycleClosureFailed { cycle_rooms, detail } variant,
preserving #[non_exhaustive] and std::error::Error. Display carries the
participating rooms and detail for authoring feedback.
Wires the validate CLI (sidequest-validate/src/tactical.rs) to call
layout_dungeon instead of layout_tree so cyclic rooms.yaml files validate
correctly — the no-half-wired-features rule.
Tests: 24/24 in layout_story_29_7_tests.rs. Two test fixtures fixed during
green (1-cell east exit width mismatch, and the oversize-grid test hitting
the 10000-cell parser limit).
Applied 4 high-confidence findings from the simplify fan-out: 1. Stale module doc in layout.rs said 'cycle handling is deferred to 29-7', but 29-7 is the story that implemented it. Updated to point at layout_dungeon as the cyclic entry point. 2. Stale docstring on validate_layout() called the engine 'tree-topology' — now dispatches to layout_dungeon, which handles cyclic topologies too. 3. Redundant .clone() on used_gaps entries in layout_cycle (two places) and layout_dungeon (one place). The pattern .entry(k).or_default().clone() inserts an empty HashSet into the map as a side effect just to read it back. Replaced with .get(k).cloned().unwrap_or_default() — non-mutating read, same semantics. The subsequent success-path .entry().or_default() .insert() calls still use the entry API where mutation is intended. Deferred findings (flagged, not applied): - High-confidence: extract shared BFS placement helper between layout_tree (29-6) and layout_dungeon (29-7). 60-line refactor that touches 29-6 code and is out of scope for this story. - High-confidence: same redundant-clone pattern in layout_tree and unused _b_exit parameter on shared_boundary_positions — both pure 29-6 touches. - Medium-confidence: duplicate exit-pair search in layout_tree's error reporting path.
Applied 7 fixes from the reviewer fan-out (preflight, silent-failure,
test-analyzer, comment-analyzer, type-design, rule-checker):
**Documentation**
1. Module doc no longer claims 'BFS from the entrance room' as the universal
rule — that's only true for layout_tree. layout_dungeon places the cycle
ring in cycle-walk order from cycle[0], which is not guaranteed to be the
entrance. Doc now distinguishes the two entry points.
2. detect_cycles rustdoc misattributed undirected-graph deduplication to
the Black-node skip. The actual undirected dedup is the parent-edge skip;
the Black-node skip prevents re-extracting cycles through already-
completed DFS subtrees. Doc now names both mechanisms separately.
3. layout_dungeon rustdoc previously said multi-cycle graphs 'fail loudly
with LayoutError::CycleClosureFailed' without clarifying that multi-cycle
is a capability gap, not a geometric impossibility. A reader parsing
production errors would have misdiagnosed a 'not yet implemented' as a
genuine authoring closure mismatch. Now explicit.
4. Test file header dropped the stale 'RED phase — failing tests' language.
**Semantics**
5. layout_dungeon's BFS branch-attachment failure path used
LayoutError::Overlap { cells: vec![] } to report 'no opposite-wall exit
pair'. LayoutError::Overlap's cells field is documented as 'positions
where non-void cells collide' — an empty vec for a topology/authoring
error is a semantic mismatch. Replaced with CycleClosureFailed carrying
the ring room and branch room plus a descriptive detail.
**Tests**
6. cycle_closure_error_display_is_non_empty used room IDs 'a'/'d' and then
asserted the Display output contained 'a' and 'd' — a tautology since
single characters appear in virtually any non-empty string and the
detail field already contained both. Rewrote to use non-overlapping
tokens (xyzzy, plover, fumble) and the expected comma separator, so
the assertion actually proves cycle_rooms is rendered.
7. layout_dungeon_places_two_disjoint_cycles_without_overlap accepted both
Ok and Err as valid outcomes. Since the current impl unconditionally
returns Err for multi-cycle graphs, the Ok branch was unreachable and
the Err branch only asserted non-empty message — a guaranteed pass
against an unimplemented AC. Renamed to
layout_dungeon_fails_loud_on_multi_cycle_graphs_not_yet_supported and
rewrote to assert the CycleClosureFailed variant, that detail contains
'multi-cycle ... not yet supported', and that cycle_rooms names members
from both rings. A future implementation of multi-cycle support will
be forced to update this test rather than silently change behaviour.
**Deferred findings** (tracked for follow-up, not fixed):
- OTEL spans for layout decisions (rule-checker #4 — layout is validate-
only plumbing, not a runtime dispatch subsystem; OTEL requirement is
scoped to subsystems the GM panel observes)
- as u32 casts in bounding box + overlap filter (type-design / rule #7 —
same pre-existing pattern used throughout layout_tree, cell_at safety
net catches the negative case)
- Typed CycleFailureReason enum replacing detail: String (type-design #3)
- Extract shared BFS placement helper between layout_tree and layout_dungeon
(verify-phase reuse finding)
- Real geometric-overlap fixture for AC-6 (current test exercises the
no-pair path, not an actual cell collision)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
sidequest_game::tactical::layout:detect_cycles,layout_cycle,layout_dungeon, plusLayoutError::CycleClosureFailed.layout_dungeondelegates tolayout_treefor acyclic graphs (behaviour-preserving per the explicitlayout_dungeon_linear_chain_matches_tree_behaviourtest), handles single-cycle dungeons via ring placement + BFS tree-branch attachment, and fails loud on multi-cycle graphs with an actionable "not yet supported" detail.sidequest-validateCLI fromlayout_treetolayout_dungeonso cyclicrooms.yamlfiles validate correctly — the no-half-wired-features rule.Implementation highlights
detect_cyclesuses iterative three-colour DFS with an explicit stack to keep the current path visible for back-edge cycle extraction. Undirected handling: parent-edge skip to treat the graph as undirected, Black-node skip to prevent re-extracting cycles through already-completed subtrees.BTreeMap-sorted adjacency for deterministic output.layout_cyclewalks the cycle in input order, placing each successor via the first-fit opposite-wall exit pair, then validates the closing edge by computing the hypothetical offset of the first room from the last room and comparing to (0, 0). Mismatch →CycleClosureFailedwith actionable detail.layout_dungeonshort-circuits tolayout_treefor acyclic graphs, places single cycles as rings + BFS-attaches branches using the existing 29-6 overlap-aware logic, and fails loud for multi-cycle graphs (deferred capability gap).LayoutErrorremains#[non_exhaustive];Displayandstd::error::Errorextend to the new variant.Test plan
layout_story_29_7_tests.rs(cycle detection, ring placement, closure validation, tree-branch attachment, multi-cycle fail-loud, Rust rule enforcement, wiring signatures)layout_story_29_6_tests.rs— no regressionssidequest-gametest suite (2065 tests) +sidequest-validatesuite (30 tests) passcargo clippy -p sidequest-game -p sidequest-validate -- -D warningscleansidequest-validate/src/tactical.rs:9,317imports and callslayout_dungeonReview cycle
Ran the reviewer fan-out (preflight, silent-failure, test-analyzer, comment-analyzer, type-design, rule-checker). Applied 7 in-scope fixes (stale module doc, misleading multi-cycle rustdoc, wrong undirected-dedup explanation, semantic error-variant misuse in BFS failure path, tautological Display test, vacuous multi-cycle test, stale RED-phase test header). Deferred OTEL instrumentation (validate-only subsystem, not runtime dispatch),
as u32cast cleanup (pre-existing pattern,cell_atsafety net), and a real geometric-overlap fixture for AC-6 as follow-ups.Epic 29 follow-ups identified:
layout_dungeonlayout_dungeoninto server session init / room loader (currently only consumed by the validate CLI)layout_treeandlayout_dungeon(95% duplicated)