Skip to content

feat(1-3): genre pack models, loader, and validation#4

Merged
slabgorb merged 6 commits intodevelopfrom
feat/1-3-genre-pack-models
Mar 25, 2026
Merged

feat(1-3): genre pack models, loader, and validation#4
slabgorb merged 6 commits intodevelopfrom
feat/1-3-genre-pack-models

Conversation

@slabgorb
Copy link
Copy Markdown
Owner

Summary

  • Port 70+ Python genre pack models to Rust serde structs with deny_unknown_fields
  • Unified load_genre_pack() replaces Python's 4 different loading patterns
  • Trope inheritance resolution with multi-level extends and cycle detection
  • Two-phase validation: serde catches structural errors, validate() catches cross-references
  • Scenario pack loading (ScenarioPack, AssignmentMatrix, ClueGraph, AtmosphereMatrix)
  • 60 tests passing (model deserialization, real YAML loading, trope inheritance, validation)

Test plan

  • 34 model deserialization tests (all YAML file types)
  • 25 integration tests (real YAML loading across 4 genre packs)
  • Trope inheritance: extends resolution, cycle detection, missing parent
  • Two-phase validation: achievement→trope, region adjacents, clue→suspect
  • deny_unknown_fields rejection tests (5 types)
  • Clippy clean, cargo fmt clean

🤖 Generated with Claude Code

slabgorb and others added 6 commits March 25, 2026 16:44
Write 35 tests covering all 7 ACs for story 1-3:
- Model deserialization for 16 YAML file types (PackMeta, RulesConfig,
  NpcArchetype, TropeDefinition, ProgressionConfig, AxesConfig, etc.)
- deny_unknown_fields rejection (5 tests)
- Real YAML loading (mutant_wasteland genre pack)
- Trope inheritance resolution with extends
- Cycle detection in extends chains
- Two-phase validation (cross-reference checks)
- GenreError #[non_exhaustive] enforcement

All tests fail at compile time — types don't exist yet.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add 12 more tests covering ScenarioPack, AssignmentMatrix, ClueGraph,
AtmosphereMatrix, and ScenarioNpc models. Uses pulp_noir genre pack
as the only pack with scenario fixtures (bottle episode prototype).

Total: ~55 failing tests covering all 7 ACs for story 1-3.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Port all 70+ Python genre pack models to Rust with serde derives:
- PackMeta, RulesConfig, GenreTheme, Lore, NpcArchetype, TropeDefinition,
  ProgressionConfig, AxesConfig, AudioConfig, CartographyConfig, Culture,
  CharCreationScene, VisualStyle, Prompts, and 50+ supporting types
- ScenarioPack, AssignmentMatrix, ClueGraph, AtmosphereMatrix, ScenarioNpc
- deny_unknown_fields on stable-schema types, relaxed on genre-variable types
- NonBlankString for validated name fields (from protocol crate)

Unified loader (load_genre_pack):
- Single function reads all YAML files from genre pack directory
- Discovers and loads worlds from worlds/*/
- Discovers and loads scenarios from scenarios/*/
- Resolves trope inheritance during world loading

Trope inheritance resolver:
- Multi-level extends chain resolution (fixes Python's one-level limit)
- Cycle detection via visited-set algorithm
- Missing parent detection with typed errors
- Child fields override parent fields

Two-phase validation:
- Phase 1: serde deserialization catches structural errors
- Phase 2: validate() checks cross-references (achievement→trope,
  region adjacents, clue→suspect)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Extract duplicated slugify() to shared util.rs module
- Extract load_subdirectories() generic helper (DRYs load_worlds/load_scenarios)
- Extract load_error() helper (DRYs 8 GenreError::LoadError constructions)
- Remove now-unused load_worlds() and load_scenarios() functions

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix tension_level sentinel bug: change from f64 to Option<f64> so 0.0
  is a valid value, not a sentinel for "inherit from parent" (finding #1)
- Slugify extends value before parent_map lookup so authors can write
  extends: "The Mentor" instead of requiring extends: the-mentor (finding #2)
- Add MAX_INHERITANCE_DEPTH (64) to detect_cycle to prevent stack overflow
  on deeply nested non-cyclic chains (CWE-674, finding #3)
- Add #[non_exhaustive] to Landmark enum (Rule #2, finding #4)
- Fix misleading docstring on resolve_trope_inheritance (finding #5)
- Validate starting_region against region slugs in validate_cartography (finding #7)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@slabgorb slabgorb force-pushed the feat/1-3-genre-pack-models branch from f05c5dc to a36a061 Compare March 25, 2026 20:48
@slabgorb slabgorb merged commit e793107 into develop Mar 25, 2026
slabgorb added a commit that referenced this pull request Mar 25, 2026
…delta, persistence

Story 1-8: 38/38 tests GREEN. Implements:

- state.rs: GameSnapshot composing all domain types (port lesson #4),
  typed patches (WorldStatePatch, CombatPatch, ChasePatch) replacing
  Python's 255-line apply_patch() god-method
- delta.rs: StateSnapshot and StateDelta capturing ALL client-visible
  changes (port lesson #11) via JSON comparison
- persistence.rs: GameStore with rusqlite (ADR-006 schema), atomic
  auto-save via transactions (ADR-023), narrative log append/query
- session.rs: SessionManager wrapping GameStore with active session
- turn.rs: Barrier semantics — single-player immediate, multi-player
  waits for all inputs, dedup via HashSet
- Serde derives added to CombatState, ChaseState, NarrativeEntry,
  TurnManager, and supporting types for JSON round-trip

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@slabgorb slabgorb deleted the feat/1-3-genre-pack-models branch March 25, 2026 23:53
slabgorb added a commit that referenced this pull request Mar 25, 2026
…delta, persistence

Story 1-8: 38/38 tests GREEN. Implements:

- state.rs: GameSnapshot composing all domain types (port lesson #4),
  typed patches (WorldStatePatch, CombatPatch, ChasePatch) replacing
  Python's 255-line apply_patch() god-method
- delta.rs: StateSnapshot and StateDelta capturing ALL client-visible
  changes (port lesson #11) via JSON comparison
- persistence.rs: GameStore with rusqlite (ADR-006 schema), atomic
  auto-save via transactions (ADR-023), narrative log append/query
- session.rs: SessionManager wrapping GameStore with active session
- turn.rs: Barrier semantics — single-player immediate, multi-player
  waits for all inputs, dedup via HashSet
- Serde derives added to CombatState, ChaseState, NarrativeEntry,
  TurnManager, and supporting types for JSON round-trip

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Mar 25, 2026
…sistence

* test: add failing tests for story 1-8 game state composition

RED phase — 35 tests covering all 7 ACs:
- GameSnapshot JSON round-trip (4 tests)
- StateDelta completeness for ALL client-visible fields (10 tests)
- TurnManager barrier semantics: single/multi-player (3 tests)
- Persistence: rusqlite save/load/list (3 tests)
- Narrative log: append and ordered retrieval (2 tests)
- Auto-save atomicity via transactions (2 tests)
- last_saved_at UTC timestamp on save (2 tests)
- Typed patches: WorldStatePatch, CombatPatch, ChasePatch (5 tests)
- SessionManager lifecycle (2 tests)
- Rule enforcement: non_exhaustive, validated constructors, serde bypass (3 tests)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat(1-8): implement game state composition — GameSnapshot, patches, delta, persistence

Story 1-8: 38/38 tests GREEN. Implements:

- state.rs: GameSnapshot composing all domain types (port lesson #4),
  typed patches (WorldStatePatch, CombatPatch, ChasePatch) replacing
  Python's 255-line apply_patch() god-method
- delta.rs: StateSnapshot and StateDelta capturing ALL client-visible
  changes (port lesson #11) via JSON comparison
- persistence.rs: GameStore with rusqlite (ADR-006 schema), atomic
  auto-save via transactions (ADR-023), narrative log append/query
- session.rs: SessionManager wrapping GameStore with active session
- turn.rs: Barrier semantics — single-player immediate, multi-player
  waits for all inputs, dedup via HashSet
- Serde derives added to CombatState, ChaseState, NarrativeEntry,
  TurnManager, and supporting types for JSON round-trip

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor: simplify code per verify review

- Extract prepare_for_save() and parse_rfc3339_or_now() helpers in persistence.rs
- Extract to_json() helper in delta.rs for repeated serialization
- Fix variable shadowing (dr → regions/routes) in state.rs apply_world_patch
- Remove unused StateSnapshot/StateDelta imports from test file

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* chore(fmt): fix Rust formatting

* chore(lint): fix clippy warnings in delta.rs and main.rs

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Mar 26, 2026
…g, fmt

- Propagate read_to_string errors via map_err instead of .ok() (Rule #1)
- Add tracing::warn!/error! to send() error paths (Rule #4)
- Add #[non_exhaustive] to SectionCategory (Rule #2)
- Remove redundant content() getter (fields are pub — Rule #9)
- Fix vacuous tautological assertion in whitespace test (Rule #6)
- Restore distinctness check in section_category test (Rule #6)
- Add error variant check in nonexistent command test
- Add empty prompt guard in send()
- Fix Identity/Role doc overlap, registry() doc
- Run cargo fmt

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Mar 27, 2026
- Deprecate ReminderConfig::new() in favor of try_new() (Rule #5)
- Add tracing::warn! to try_new() rejection paths (Rule #4)
- Add #[instrument] to run_reminder() async fn (Rule #4)
- Remove vacuous config_fields_are_private test (Rule #6)
- Apply cargo fmt

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 10, 2026
Two wires closed in one story:

**LoRA wire (the original story scope)**
- VisualStyle gains lora: Option<String> and lora_trigger: Option<String>
  (both serde default). ADR-032 canonical field names.
- RenderParams gains lora_path: Option<String> and lora_scale: Option<f32>
  with skip_serializing_if = "Option::is_none" so absence means absence
  (daemon uses params.get with None sentinel).
- RenderJob and RenderQueue::enqueue() extended to accept lora_path and
  lora_scale. The render_fn closure signature grows from 7 to 10
  positional args.
- dispatch/render.rs resolves visual_style.lora relative to the genre
  pack directory, substitutes lora_trigger for positive_suffix when
  the LoRA is active (per ADR-032 lines 230-239 — daemon does NOT
  auto-prepend), and emits a watcher event (render / lora_activated)
  before enqueue so the GM panel can verify LoRA is engaged (lie-
  detector pattern per OTEL Observability Principle).

**Variant wire (companion fix — previously dead code)**
- The `_image_model` parameter in enqueue() was prefixed with `_`
  meaning unused: `vs.preferred_model` from the genre pack was read in
  Rust, passed as a function argument, and silently dropped. Fourteen
  genre packs declared `preferred_model: flux` (or "flux-1.0") to no
  effect — the daemon hardcoded the Flux variant per tier.
- Renamed `_image_model` -> `variant`, stored in RenderJob, plumbed
  through the 10-arg closure into RenderParams.variant (serde empty-
  string-skip so no-override means absence).
- dispatch/render.rs passes vs.preferred_model as the variant. The
  non-visual_style fallback branch now passes empty strings for both
  model and variant (the pre-existing "flux-schnell" literal there was
  never a valid daemon variant — silent dead string, removed).
- dispatch/audio.rs mood image render path updated from "flux-dev"
  (dead string) to canonical "dev".

**Test coverage**
- 30 new tests across four story-35-15 test files (0 vacuous).
- AC-5 regression guardrail (non-LoRA genres unchanged) written FIRST
  in each file.
- Rule-coverage self-check against .pennyfarthing/gates/lang-review/
  rust.md passes.
- All 69 pre-existing regression tests continue to pass including the
  story 4-4 render queue suite updated to the 10-arg closure signature.

**Architect deviations enforced by tests**
- #1 Field name is `lora_trigger` not `trigger_word` (ADR-032 authority)
- #2 Rust substitutes trigger into prompt (daemon does NOT auto-prepend)
- #3 LoRA paths genre-pack-relative, resolved to absolute in dispatch
- #4 AC-3 scoped to wiring verification (no .safetensors files exist yet)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 10, 2026


Reviewer rejected 35-15 with 4 HIGH + 8 MEDIUM findings. This rework RED
commit adds/strengthens tests for the testable findings:

- Finding #1 (HIGH) — wiring_dispatch_render_warns_when_lora_has_no_trigger:
  new source-pattern test requiring tracing::warn! or WatcherEventBuilder
  with ValidationWarning in the (Some, None) match arm.

- Finding #2 (HIGH, R16) — visual_style_rejects_unknown_fields +
  visual_style_rejects_another_unknown_field: two new tests enforcing
  #[serde(deny_unknown_fields)] on VisualStyle per sidequest-genre
  convention. NOTE: Dev must delete dead portrait_style/poi_style fields
  from spaghetti_western/visual_style.yaml (zero Rust consumers) before
  deny_unknown_fields can land.

- Finding #3 (HIGH) — visual_style_has_lora_scale_field +
  visual_style_without_lora_scale_defaults_to_none: new tests forcing the
  lora_scale decision. Path (a) wire it: tests pass. Path (b) remove it:
  delete the lora_scale tests instead.

- Finding #5 (MEDIUM) — wiring_dispatch_audio_reads_visual_style_preferred_model:
  new source-pattern test requiring audio.rs to read vs.preferred_model
  and vs.lora instead of hardcoding.

- Finding #6 (MEDIUM) — wiring_dispatch_render_validates_lora_path_stays_in_genre_pack_dir:
  new test requiring a path-traversal guard (starts_with, canonicalize,
  or newtype).

- Finding #8 (MEDIUM) — wiring_dispatch_render_substitutes_trigger_into_prompt
  STRENGTHENED: now requires Some(trigger) + trigger.to_string pattern,
  not mere identifier co-occurrence.

- Finding #9 (MEDIUM) — wiring_dispatch_render_preserves_non_lora_path
  STRENGTHENED: removed OR disjunction, split into separate assertions,
  added negative check that flux-schnell literal stays removed.

- Finding #10 (MEDIUM) — enqueue_signature_accepts_none_lora_explicitly
  STRENGTHENED: replaced zero-info runtime assertion with a capturing
  closure that asserts lora_path/lora_scale/variant arrive verbatim as
  None/None/"" at the worker.

Comment fixes (findings #11, #12) and fmt (finding #4) are Dev GREEN
tasks — no tests needed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 10, 2026
…wn_fields, lora_scale wire, fmt, audio consistency, path traversal)

- #1 Silent no-op: added (Some(lora), None) match arm in dispatch/render.rs
  that emits tracing::warn! + WatcherEventBuilder(ValidationWarning) with
  action=lora_trigger_missing. LoRA loads, trained style doesn't activate,
  GM panel sees the misconfiguration.
- #2 deny_unknown_fields: added #[serde(deny_unknown_fields)] to VisualStyle.
  Companion content-repo commit deletes dead portrait_style/poi_style from
  spaghetti_western/visual_style.yaml (pre-existing dead YAML with zero Rust
  consumers).
- #3 lora_scale dead wire: picked resolution path (a) - added lora_scale:
  Option<f32> to VisualStyle with serde(default), wired through dispatch/render.rs
  and dispatch/audio.rs to enqueue. Daemon defaults to 1.0 when None.
- #4 fmt: ran cargo fmt --all.
- #5 audio.rs mood-image consistency: mood images now read vs.preferred_model,
  vs.lora, vs.lora_trigger, vs.lora_scale and resolve the LoRA path the same
  way render.rs does. Scene and mood images stay visually consistent on
  LoRA-enabled genres.
- #6 path traversal: added starts_with(base_dir) guard after LoRA path
  resolution in both dispatch/render.rs and dispatch/audio.rs. Escaping
  paths fail loudly with a tracing::error and a watcher event.

Doc-only findings #11 (stale line numbers) and #12 (RED phase labels)
deferred — MEDIUM non-blocking, acknowledged for trivial follow-up.

All 35-15 tests pass + 50 story 4-4 regression tests pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 10, 2026
…line (Pass 2 rework — A+B+C+D+E+F+G+I) (#398)

* test: add failing tests for 35-15 (LoRA wire through render pipeline)

RED phase tests for story 35-15. Covers all 5 ACs plus the architect's
four design deviations logged in the session file.

Test files:
- sidequest-genre/tests/visual_style_lora_story_35_15_tests.rs
  AC-1 + Design Deviation #1 (lora_trigger, not trigger_word)
- sidequest-daemon-client/tests/lora_render_params_story_35_15_tests.rs
  AC-2 (JSON serialization with skip_serializing_if)
- sidequest-game/tests/render_queue_lora_story_35_15_tests.rs
  enqueue() signature extension, worker closure arg capture
- sidequest-server/tests/render_lora_wiring_story_35_15_tests.rs
  Dispatch-layer source introspection, OTEL wiring, trigger substitution

25 tests, 67 assertions. AC-5 regression guardrail (non-LoRA genres) is
the first test in each file per SM assessment.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat(35-15): wire LoRA path and Flux variant through render pipeline

Two wires closed in one story:

**LoRA wire (the original story scope)**
- VisualStyle gains lora: Option<String> and lora_trigger: Option<String>
  (both serde default). ADR-032 canonical field names.
- RenderParams gains lora_path: Option<String> and lora_scale: Option<f32>
  with skip_serializing_if = "Option::is_none" so absence means absence
  (daemon uses params.get with None sentinel).
- RenderJob and RenderQueue::enqueue() extended to accept lora_path and
  lora_scale. The render_fn closure signature grows from 7 to 10
  positional args.
- dispatch/render.rs resolves visual_style.lora relative to the genre
  pack directory, substitutes lora_trigger for positive_suffix when
  the LoRA is active (per ADR-032 lines 230-239 — daemon does NOT
  auto-prepend), and emits a watcher event (render / lora_activated)
  before enqueue so the GM panel can verify LoRA is engaged (lie-
  detector pattern per OTEL Observability Principle).

**Variant wire (companion fix — previously dead code)**
- The `_image_model` parameter in enqueue() was prefixed with `_`
  meaning unused: `vs.preferred_model` from the genre pack was read in
  Rust, passed as a function argument, and silently dropped. Fourteen
  genre packs declared `preferred_model: flux` (or "flux-1.0") to no
  effect — the daemon hardcoded the Flux variant per tier.
- Renamed `_image_model` -> `variant`, stored in RenderJob, plumbed
  through the 10-arg closure into RenderParams.variant (serde empty-
  string-skip so no-override means absence).
- dispatch/render.rs passes vs.preferred_model as the variant. The
  non-visual_style fallback branch now passes empty strings for both
  model and variant (the pre-existing "flux-schnell" literal there was
  never a valid daemon variant — silent dead string, removed).
- dispatch/audio.rs mood image render path updated from "flux-dev"
  (dead string) to canonical "dev".

**Test coverage**
- 30 new tests across four story-35-15 test files (0 vacuous).
- AC-5 regression guardrail (non-LoRA genres unchanged) written FIRST
  in each file.
- Rule-coverage self-check against .pennyfarthing/gates/lang-review/
  rust.md passes.
- All 69 pre-existing regression tests continue to pass including the
  story 4-4 render queue suite updated to the 10-arg closure signature.

**Architect deviations enforced by tests**
- #1 Field name is `lora_trigger` not `trigger_word` (ADR-032 authority)
- #2 Rust substitutes trigger into prompt (daemon does NOT auto-prepend)
- #3 LoRA paths genre-pack-relative, resolved to absolute in dispatch
- #4 AC-3 scoped to wiring verification (no .safetensors files exist yet)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor: extract make_capturing_queue helper in 35-15 test fixtures

Verify-phase simplify pass — applied a simplify-reuse high-confidence
finding. Three of four tests in render_queue_lora_story_35_15_tests.rs
were duplicating a 25-line closure that captured worker call args into
an Arc<Mutex<Vec<CapturedCall>>>. Extracted to a single
`make_capturing_queue(captures: Arc<...>)` helper after `test_config()`.

The fourth test (enqueue_signature_accepts_none_lora_explicitly) keeps
its inline no-op closure because it doesn't capture — it's a compile-
time signature guard that only needs to verify None Option semantics.

Net: ~75 lines removed, 4/4 tests still pass, 50 pre-existing render
queue tests still pass.

Did NOT extract a similar helper for render_queue_story_4_4_tests.rs.
That file has 20+ closures with diverse semantics (Ok/Err returns,
specific dummy values for hash dedup tests, delay-injection for
race tests). A single helper would not fit, and a multi-helper
extraction belongs with the dedicated closure-signature refactor
story (where RenderQueue::spawn takes a RenderParams struct instead
of 10 positional args). Bundling them avoids changing the same call
sites twice.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(35-15): add rework tests for reviewer findings #1-#3, #5-#6, #8-#10

Reviewer rejected 35-15 with 4 HIGH + 8 MEDIUM findings. This rework RED
commit adds/strengthens tests for the testable findings:

- Finding #1 (HIGH) — wiring_dispatch_render_warns_when_lora_has_no_trigger:
  new source-pattern test requiring tracing::warn! or WatcherEventBuilder
  with ValidationWarning in the (Some, None) match arm.

- Finding #2 (HIGH, R16) — visual_style_rejects_unknown_fields +
  visual_style_rejects_another_unknown_field: two new tests enforcing
  #[serde(deny_unknown_fields)] on VisualStyle per sidequest-genre
  convention. NOTE: Dev must delete dead portrait_style/poi_style fields
  from spaghetti_western/visual_style.yaml (zero Rust consumers) before
  deny_unknown_fields can land.

- Finding #3 (HIGH) — visual_style_has_lora_scale_field +
  visual_style_without_lora_scale_defaults_to_none: new tests forcing the
  lora_scale decision. Path (a) wire it: tests pass. Path (b) remove it:
  delete the lora_scale tests instead.

- Finding #5 (MEDIUM) — wiring_dispatch_audio_reads_visual_style_preferred_model:
  new source-pattern test requiring audio.rs to read vs.preferred_model
  and vs.lora instead of hardcoding.

- Finding #6 (MEDIUM) — wiring_dispatch_render_validates_lora_path_stays_in_genre_pack_dir:
  new test requiring a path-traversal guard (starts_with, canonicalize,
  or newtype).

- Finding #8 (MEDIUM) — wiring_dispatch_render_substitutes_trigger_into_prompt
  STRENGTHENED: now requires Some(trigger) + trigger.to_string pattern,
  not mere identifier co-occurrence.

- Finding #9 (MEDIUM) — wiring_dispatch_render_preserves_non_lora_path
  STRENGTHENED: removed OR disjunction, split into separate assertions,
  added negative check that flux-schnell literal stays removed.

- Finding #10 (MEDIUM) — enqueue_signature_accepts_none_lora_explicitly
  STRENGTHENED: replaced zero-info runtime assertion with a capturing
  closure that asserts lora_path/lora_scale/variant arrive verbatim as
  None/None/"" at the worker.

Comment fixes (findings #11, #12) and fmt (finding #4) are Dev GREEN
tasks — no tests needed.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(35-15): address reviewer findings #1-#6 (silent no-op, deny_unknown_fields, lora_scale wire, fmt, audio consistency, path traversal)

- #1 Silent no-op: added (Some(lora), None) match arm in dispatch/render.rs
  that emits tracing::warn! + WatcherEventBuilder(ValidationWarning) with
  action=lora_trigger_missing. LoRA loads, trained style doesn't activate,
  GM panel sees the misconfiguration.
- #2 deny_unknown_fields: added #[serde(deny_unknown_fields)] to VisualStyle.
  Companion content-repo commit deletes dead portrait_style/poi_style from
  spaghetti_western/visual_style.yaml (pre-existing dead YAML with zero Rust
  consumers).
- #3 lora_scale dead wire: picked resolution path (a) - added lora_scale:
  Option<f32> to VisualStyle with serde(default), wired through dispatch/render.rs
  and dispatch/audio.rs to enqueue. Daemon defaults to 1.0 when None.
- #4 fmt: ran cargo fmt --all.
- #5 audio.rs mood-image consistency: mood images now read vs.preferred_model,
  vs.lora, vs.lora_trigger, vs.lora_scale and resolve the LoRA path the same
  way render.rs does. Scene and mood images stay visually consistent on
  LoRA-enabled genres.
- #6 path traversal: added starts_with(base_dir) guard after LoRA path
  resolution in both dispatch/render.rs and dispatch/audio.rs. Escaping
  paths fail loudly with a tracing::error and a watcher event.

Doc-only findings #11 (stale line numbers) and #12 (RED phase labels)
deferred — MEDIUM non-blocking, acknowledged for trivial follow-up.

All 35-15 tests pass + 50 story 4-4 regression tests pass.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* chore(35-15): remove committed test runner artifacts

build.log and test.log were accidentally captured by 'git add -A' in the
rework commit. These are test runner output files and should not be in
the repo. Removing them here to keep the branch clean.

* fix(35-15): revert deny_unknown_fields, add lora_scale validator (Pass 2 A+F)

Rework Pass 2 — Findings A and F from the reviewer's second-pass assessment.

Finding A (reviewer self-correction): Revert #[serde(deny_unknown_fields)]
on VisualStyle. The first-pass review's finding #2 was wrong — it contradicts
the pre-existing `visual_style_accepts_extra_fields` test in
tests/model_tests.rs:799 which intentionally documents VisualStyle as an
exempt from the convention for genre extensibility. Added a doc comment on
the struct explaining the exemption so future rule-checker sweeps don't
repeat the mistake. Deleted the two `visual_style_rejects_unknown_fields`
tests that the Pass 1 rework added in support of the wrong finding.

Finding F: Add custom `validate_lora_scale` deserializer to VisualStyle.
`serde_yaml` happily deserializes `.nan`, `.inf`, `-.inf` into f32 with no
validation, and Flux LoRA behavior on non-finite scales is unspecified.
The validator rejects NaN, ±infinity, values < 0.0, and values > 2.0
(the canonical Flux LoRA range). Each rejection branch emits a specific
error message citing the offending value. Added 6 tests covering the
rejection cases (.nan, .inf, -.inf, negative, > 2.0) and a boundary test
for the accepted range [0.0, 2.0].

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(35-15): close symmetry + telemetry gaps in LoRA dispatch (Pass 2 B/C/D/E/G/I)

Rework Pass 2 — Findings B, C, D, E, G, and I from the reviewer's second-pass
assessment. The Pass 1 rework added LoRA handling to dispatch/audio.rs so
mood images would use the same visual style as scene images, but it copied
the happy path only — every loud-failure branch from dispatch/render.rs was
left as a silent no-op, and dispatch/render.rs itself had three new
second-order gaps surfaced by the reviewer's edge-hunter analysis.

Finding D infrastructure (lib.rs): Add `lora_warned_genres: Mutex<HashSet>`
to AppStateInner with a `mark_lora_warned(genre_slug)` method that returns
true on first insert. Process-scoped debounce (not per-session) for log-flood
prevention — a misconfigured genre fires exactly one warning per process
lifetime, regardless of how many turns or sessions render with that genre.

Findings B + C + E + D wire-up in dispatch/render.rs: Restructure the
visual_style match to return (base_style: String, lora_active: bool).
Gate the (Some(lora), None) warn arm on `state.mark_lora_warned()` for the
debounce (D). Gate `lora_abs` resolution on `lora_active` so a LoRA with
no trigger word returns None from the path resolution — the subsequent
`lora_activated` SubsystemExerciseSummary watcher event is gated on
`lora_path.is_some()` and therefore does not fire (E). This removes the
contradictory telemetry pair (ValidationWarning saying "LoRA will not
activate" + lora_activated saying "LoRA is engaged") that confused the GM
panel consumer on the same render turn.

Findings B + C + E + D + I wire-up in dispatch/audio.rs: Mirror the full
restructured pattern from render.rs into the mood-image dispatch path.
Replace the silent `_` wildcard with an explicit (Some(lora), None) warn
arm emitting tracing::warn + WatcherEventBuilder(ValidationWarning,
action=lora_trigger_missing), gated by mark_lora_warned (C+D). Replace the
silent `return None` on path traversal with tracing::error +
WatcherEventBuilder(ValidationWarning, action=lora_path_traversal_rejected)
— the action code and field shape exactly mirror render.rs (B). Gate
lora_abs on lora_active (E). Add `lora_trigger` to the match tuple and emit
a dedicated `lora_activated` SubsystemExerciseSummary event on the mood
image render path with `source: "mood_image"` field — without this, the GM
panel saw lora_activated events for scene images but was silent on mood
images, the exact compound opacity the Devil's Advocate section of the
reviewer's Pass 2 assessment flagged (E).

Finding I (architect call): Apply `visual_tag_overrides` location-based
lookup in dispatch/audio.rs, mirroring dispatch/render.rs. The Pass 1
rework's own comment cited "mood images and scene images stay visually
consistent within a session" as the motivation; skipping tag_override
directly contradicted that principle. ~10 lines of mirrored code — mood
images in a "wasteland" location now pick up the same genre-pack style
overrides as scene images. Extracted the tag_override lookup outside the
match so it composes cleanly with base_style regardless of whether a LoRA
is active. Also added the `extract_location_header` import from
crate::extraction that audio.rs was missing.

Finding G: Replace `resolved.starts_with(&base)` with std::fs::canonicalize
on both base and resolved paths, then compare canonicalized forms. Three
distinct failure modes get distinct watcher action codes so the GM panel
can discriminate them:
  - lora_base_not_accessible — genre pack dir missing/unreadable
  - lora_file_not_found — LoRA .safetensors file missing
  - lora_path_traversal_rejected — canonicalized path escapes base
    (now catches symlink escapes that the un-canonicalized starts_with
    missed — e.g., `genre_packs/x/lora -> /etc`)
As a side effect, this closes a latent gap: before canonicalize(), a
missing LoRA file would propagate through to the daemon and fail
cryptically. Now it fails loudly in the Rust dispatch layer with a
specific action code. Applied identically in both render.rs and audio.rs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 13, 2026
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)
slabgorb added a commit that referenced this pull request Apr 13, 2026
…e validation (#443)

* test: add failing tests for 29-7 jaquayed layout

* feat(29-7): jaquayed layout — cycle detection, ring placement, closure 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).

* refactor: simplify code per verify review

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.

* fix(29-7): address reviewer findings

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)
slabgorb added a commit that referenced this pull request Apr 14, 2026
…stness

Post-review improvements from Colonel Potter's review team:

1. comment-analyzer #1: the prompt.rs code comment claimed 37-13 routes
   re-emits "on every case" but only listed three of six. Rewrote to
   name Cases C/D/E explicitly, note that A/B are reached via the
   is_none() branch, and document that the block sits outside the
   def-guard intentionally so a broken-def encounter can still be
   recovered via a transition menu.

2. edge-hunter #6 / test-analyzer #4: the
   transition_block_iterates_confrontation_defs test used a fixed
   2000-byte window past the marker, which was fragile to block growth
   and could sweep into neighbouring AVAILABLE CONFRONTATIONS iteration.
   Replaced with a semantic window bounded by === TRANSITION
   CONFRONTATION === and === END TRANSITION CONFRONTATION ===.

3. rule-checker check #6: the phrase test accepted any of six
   candidates, which made future wording swaps invisible to CI. Now
   that GREEN settled on the canonical "re-emit the `confrontation`"
   phrasing, narrowed the assertion to that exact string so any reword
   surfaces in code review.

4. comment-analyzer #2: the test module doc said "six observable cases"
   but enumerated only five outcome names. Rewrote to note that Cases
   A and B share the `Created` outcome name, resolving the
   six-cases-five-names contradiction.

All 7 Story 37-12 tests still pass, 419-test integration suite green,
clippy clean, cargo fmt no-op.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 14, 2026
…452)

* test(37-12): add failing tests for narrator confrontation re-declaration

Source-scan tests on dispatch/prompt.rs verify the seven ACs for Story
37-12. Follows the Story 28-4 pattern (include_str! scan) since
DispatchContext is too heavy to instantiate in integration tests.

All 7 tests fail RED as expected:
- prompt_no_longer_tells_narrator_to_only_emit_on_start
- prompt_includes_transition_confrontation_section_marker
- prompt_instructs_narrator_to_reemit_on_scene_shift
- transition_block_iterates_confrontation_defs
- prompt_emits_transition_guidance_otel_event
- otel_transition_event_carries_alternative_count_field
- transition_guidance_is_below_build_prompt_context_declaration

412 unrelated tests still passing.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(37-12): narrator re-declares confrontation on scene transitions

Adds a TRANSITION CONFRONTATION section to the narrator prompt when an
encounter is active, telling the narrator it may re-emit the
`confrontation` field when the scene shifts to a different type. Lists
the other confrontation types from the genre pack as concrete targets
and excludes the current type (handled by the 37-13 gate's Case C
redeclare no-op).

Emits `encounter.transition_guidance_injected` with
`current_encounter_type` and `alternative_count` fields so the GM panel
can verify the narrator was shown N transition options on each turn.

Removes the contradictory "Only emit confrontation on the turn the
encounter STARTS" instruction from the is_none() branch — the 37-13
encounter gate was built to route re-emits across six cases, but the
prompt was actively telling the narrator not to send them.

Scope: prompt-string edit plus one watcher event. No new types, no new
public API, no behavioral change to encounter_gate.rs.

7/7 Story 37-12 tests pass, 412 unrelated integration tests still pass,
clippy clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* refactor(37-12): drop Vec allocation on narrator prompt hot path

Applied one high-confidence simplify-efficiency finding from TEA verify:
the TRANSITION CONFRONTATION block in build_prompt_context() collected
filtered confrontation defs into an intermediate Vec, then iterated the
Vec to format list items. Replaced with a direct filter+counter loop
that produces the same string output and the same alternative_count
OTEL field without the intermediate allocation.

The cost of a ~30-element Vec allocation is tiny compared to the
downstream Claude CLI subprocess, but the counter variant is strictly
simpler and has no tradeoff.

7/7 Story 37-12 tests still pass, 419-test integration suite green,
clippy clean, cargo fmt no-op.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* review(37-12): apply four reviewer nits — comment honesty + test robustness

Post-review improvements from Colonel Potter's review team:

1. comment-analyzer #1: the prompt.rs code comment claimed 37-13 routes
   re-emits "on every case" but only listed three of six. Rewrote to
   name Cases C/D/E explicitly, note that A/B are reached via the
   is_none() branch, and document that the block sits outside the
   def-guard intentionally so a broken-def encounter can still be
   recovered via a transition menu.

2. edge-hunter #6 / test-analyzer #4: the
   transition_block_iterates_confrontation_defs test used a fixed
   2000-byte window past the marker, which was fragile to block growth
   and could sweep into neighbouring AVAILABLE CONFRONTATIONS iteration.
   Replaced with a semantic window bounded by === TRANSITION
   CONFRONTATION === and === END TRANSITION CONFRONTATION ===.

3. rule-checker check #6: the phrase test accepted any of six
   candidates, which made future wording swaps invisible to CI. Now
   that GREEN settled on the canonical "re-emit the `confrontation`"
   phrasing, narrowed the assertion to that exact string so any reword
   surfaces in code review.

4. comment-analyzer #2: the test module doc said "six observable cases"
   but enumerated only five outcome names. Rewrote to note that Cases
   A and B share the `Created` outcome name, resolving the
   six-cases-five-names contradiction.

All 7 Story 37-12 tests still pass, 419-test integration suite green,
clippy clean, cargo fmt no-op.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 15, 2026
…ta, real integration test

Addresses Reviewer pass-1 REJECTED findings #3, #4, #6 from the test side.
Fixes #1 (breadcrumb gate), #2 (escalation_failed event), #5 (action= vs
event= field keys in encounter.rs) are Dev's implementation work.

Changes to src/beat_dispatch_story_37_14_tests.rs:
- Module doc matrix updated: Case E is now "pre-resolved encounter emits
  SkippedResolved"; ApplyFailed variant dropped entirely (there is no
  live code path that reaches apply_beat's Err branch after the helper's
  resolved-check short-circuits first).
- case_a destructures Applied { encounter_type, beat_id } and verifies
  field contents. The new struct variant carries already-resolved data
  so handle_applied_side_effects no longer needs triple-expect() lookups.
- case_e renamed to case_e_pre_resolved_encounter_emits_skipped_resolved.
  Asserts SkippedResolved outcome, encounter.beat_skipped_resolved event
  (ValidationWarning, legitimate multi-actor post-resolution condition,
  not an error), and that no misleading beat_apply_failed / beat_applied
  event fires. Locks in the rework fix for the pass-1 regression where
  legitimate multi-actor turns triggered beat_apply_failed.

New file tests/integration/beat_dispatch_wiring_story_37_14_tests.rs:
- Real integration test satisfying CLAUDE.md's wiring-test rule. Imports
  apply_beat_dispatch from the crate root (public API), constructs a live
  GameSnapshot with a combat encounter, drives the helper, and asserts
  the encounter.beat_applied event reaches the real global telemetry
  channel (sidequest_telemetry::subscribe_global — the same channel the
  GM panel consumes) with source=narrator_beat_selection attribution.
- The compile dependency on `sidequest_server::apply_beat_dispatch` and
  `sidequest_server::BeatDispatchOutcome` forces Dev to add a pub use
  re-export at the crate root. A dead-but-present helper would fail this
  test even if every src-level unit test passed.

RED state confirmed: three compile errors:
  1. Applied variant missing encounter_type/beat_id fields
  2. SkippedResolved variant doesn't exist
  3. apply_beat_dispatch/BeatDispatchOutcome not at crate root

Dev must fix all three compile errors + the three behavioral fixes
(#1, #2, #5) during green rework.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 15, 2026
Apply all 8 Reviewer-required HIGH fixes from pass-2 review plus 4
rolled-in MEDIUM fixes:

Fix #1 — move is_resolved short-circuit to fire immediately after the
NoEncounter arm, before NoDef and UnknownBeatId. A resolved encounter
short-circuits highest priority among encounter-live branches — once the
encounter is done, every incoming beat is skipped regardless of beat_id
or def validity. Closes the ordering bug where a hallucinated beat_id
against a resolved encounter fired false Severity::Error on a normal
multi-actor post-resolution scenario. case_f now green.

Fix #2 — replace .unwrap_or(0) / .unwrap_or(false) silent defaults in
handle_applied_side_effects with a single .expect() binding matching the
def/beat .expect() precedent six lines above. Contract violation is now
a loud crash rather than silent zero-default telemetry.

Fix #3 — add .field("source", "narrator_beat_selection") to the
encounter.player_beat_from_narrator_ignored emission in dispatch/mod.rs.
Attribution contract now consistent across every beat-dispatch event.
wiring_player_beat_from_narrator_ignored_carries_source_field green.

Fix #4 option (a) — remove BeatDispatchOutcome::ApplyFailed variant
entirely. Delete the Err(e) match arm. The NoEncounter / SkippedResolved /
NoDef / UnknownBeatId short-circuits exhaust apply_beat's Err causes;
the call is now .expect()-wrapped with rationale. #[non_exhaustive] on
the outcome enum still provides forward-compat if a future apply_beat
Err cause needs its own variant. Subsumes finding #9. Closes code/doc
contradiction — the test module doc narrative is now truthful.

Fix #5 — rewrite apply_beat_dispatch doc comment. Drop the false "always
emits exactly one WatcherEvent" contract claim. State honestly that
non-Applied outcomes emit exactly one event, Applied triggers an
additional state-machine-layer event (or two) from
StructuredEncounter::apply_beat. Document the outcome ordering
explicitly.

Fix #6 — align tracing severity with OTEL severity on three branches:
  UnknownBeatId: Severity::Warn (was Error) — 4xx narrator bad input.
  SkippedResolved: tracing::warn! (was info!) — anomalous-but-expected.
  ApplyFailed: subsumed by fix #4 (variant removed entirely).

Fix #8 — update dispatch/mod.rs inline event list to include
beat_skipped_resolved and note ApplyFailed removal.

Fix #12 — rewrite integration test doc contrast to state the real
distinction (public-API reachability via `use sidequest_server::`),
not a fictitious difference in telemetry channel.

Verified via full 37-14 suite (11/11 pass), full sidequest-server
crate (lib + 423 integration), targeted sidequest-game regression
sweep, sidequest-agents sweep, and clippy -D warnings clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 15, 2026
…458)

* test(37-14): failing tests for beat dispatch silent-drop

Mirror 37-13's encounter-gate pattern one layer down. Every narrator-emitted
beat_selection must produce exactly one canonical WatcherEvent on the
encounter component, keyed by the event= field so the GM panel's standard
filter picks it up.

Test matrix — one case per BeatDispatchOutcome variant:
  A. Encounter live, def+beat found, apply OK   -> Applied
  B. snapshot.encounter is None                 -> NoEncounter
  C. Encounter live, its type has no def        -> NoDef
  D. beat_id not in def.beats                   -> UnknownBeatId
  E. apply_beat returns Err (already resolved)  -> ApplyFailed

Plus a multi-call regression guard for the playtest-2 symptom
(2-3 beats/turn for 20 min with zero events) and two source-scanning
wiring tests against dispatch/mod.rs:
  - positive: apply_beat_dispatch( must appear
  - negative: ctx.snapshot.encounter.is_some() outer guard must be gone

RED: unresolved imports for apply_beat_dispatch + BeatDispatchOutcome,
plus module beat being private. Dev extracts the helper during GREEN,
mirroring encounter_gate.rs (37-13).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat(37-14): beat dispatch — every branch observable

Mirror 37-13's encounter_gate.rs pattern one layer down. Every narrator-
emitted beat_selection now produces exactly one canonical encounter.* event
on the GM panel, keyed by event= (not action=) and carrying
source: "narrator_beat_selection" for attribution.

Five outcomes, one event per branch:
  Applied        -> encounter.beat_applied        (StateTransition)
  NoEncounter    -> encounter.beat_no_encounter   (ValidationWarning)
  NoDef          -> encounter.beat_no_def         (ValidationWarning)
  UnknownBeatId  -> encounter.beat_id.unknown     (ValidationWarning)
  ApplyFailed    -> encounter.beat_apply_failed   (ValidationWarning)

dispatch/beat.rs split into:
- apply_beat_dispatch(snapshot, beat_id, defs) -> BeatDispatchOutcome
  Narrow helper, unit-testable with minimal fixture, owns the per-branch
  OTEL contract.
- handle_applied_side_effects(ctx, beat_id)
  Wide wrapper over DispatchContext for gold-delta inventory mutation,
  resolver classification, the legacy beat_dispatched / stat_check_resolved
  breadcrumbs, and escalation handling. Only invoked when the helper
  returned Applied.

dispatch/mod.rs:
- mod beat -> pub(crate) mod beat (so the test module can reach it)
- Removed `if ctx.snapshot.encounter.is_some()` outer guard around the
  beat_selection loop — primary 37-14 silent-drop site.
- Removed inner `is_none_or(|e| e.resolved)` short-circuit continue —
  ApplyFailed events now fire instead of silently skipping.
- Loop calls beat::apply_beat_dispatch(...) directly, then
  beat::handle_applied_side_effects(...) only when outcome == Applied.

Test file imports normalised (single use path) so no re-export warning
in production builds.

GREEN: 8/8 story tests pass + workspace cargo test clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(37-14): reviewer rework — doc lies, tracing regression, escalation source

Address 6 focused findings from Reviewer (Potter) on the 37-14 PR. All
1-2 line fixes; no architectural changes.

1. Stale doc comments referencing the deleted `dispatch_beat_selection`
   function, now corrected to `apply_beat_dispatch` / `handle_applied_side_effects`:
   - beat.rs module doc (was claiming the wrapper lives in dispatch/mod.rs)
   - dispatch/mod.rs Story 28-9 comment (encounter.resolved attribution)
   - dispatch/mod.rs Story 37-14 loop comment (call relationship inverted)
   - dispatch/mod.rs DELETED tombstone comment (scene_intent fallback)
   - sidequest-protocol/src/message.rs BeatSelection doc (protocol path)

2. Restore `tracing::info!` on the apply_beat_dispatch Applied success
   path. Fix #14 meta-check regression: the pre-37-14 code logged the
   beat_id/stat_check/metric_current/resolved breadcrumb to the tracing
   subscriber (server stdout sink). The refactor preserved the
   WatcherEvent (GM panel sink) but deleted the tracing call. Restored
   without altering the WatcherEvent emission — both sinks now receive
   the success signal.

3. Add source="narrator_beat_selection" attribution to the
   encounter.escalation_started WatcherEvent. Every other event in the
   beat dispatch pipeline carries this field; a GM-panel filter keyed on
   source=narrator_beat_selection was silently dropping escalation
   events. One-line field addition.

4. Update the 28-8 wiring test (apply_beat_called_per_npc_actor) to
   scan for `apply_beat_dispatch` as the canonical token while keeping
   the legacy `dispatch_beat_selection` / `dispatch_npc_beat` / `apply_beat`
   entries in the OR chain for backwards-compat. The integration test
   still passes (and was passing via the `apply_beat` substring match
   before this change).

Tests: 8/8 story tests green, 28-8 wiring test green, clippy clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(37-14): rework RED — SkippedResolved variant, Applied carries data, real integration test

Addresses Reviewer pass-1 REJECTED findings #3, #4, #6 from the test side.
Fixes #1 (breadcrumb gate), #2 (escalation_failed event), #5 (action= vs
event= field keys in encounter.rs) are Dev's implementation work.

Changes to src/beat_dispatch_story_37_14_tests.rs:
- Module doc matrix updated: Case E is now "pre-resolved encounter emits
  SkippedResolved"; ApplyFailed variant dropped entirely (there is no
  live code path that reaches apply_beat's Err branch after the helper's
  resolved-check short-circuits first).
- case_a destructures Applied { encounter_type, beat_id } and verifies
  field contents. The new struct variant carries already-resolved data
  so handle_applied_side_effects no longer needs triple-expect() lookups.
- case_e renamed to case_e_pre_resolved_encounter_emits_skipped_resolved.
  Asserts SkippedResolved outcome, encounter.beat_skipped_resolved event
  (ValidationWarning, legitimate multi-actor post-resolution condition,
  not an error), and that no misleading beat_apply_failed / beat_applied
  event fires. Locks in the rework fix for the pass-1 regression where
  legitimate multi-actor turns triggered beat_apply_failed.

New file tests/integration/beat_dispatch_wiring_story_37_14_tests.rs:
- Real integration test satisfying CLAUDE.md's wiring-test rule. Imports
  apply_beat_dispatch from the crate root (public API), constructs a live
  GameSnapshot with a combat encounter, drives the helper, and asserts
  the encounter.beat_applied event reaches the real global telemetry
  channel (sidequest_telemetry::subscribe_global — the same channel the
  GM panel consumes) with source=narrator_beat_selection attribution.
- The compile dependency on `sidequest_server::apply_beat_dispatch` and
  `sidequest_server::BeatDispatchOutcome` forces Dev to add a pub use
  re-export at the crate root. A dead-but-present helper would fail this
  test even if every src-level unit test passed.

RED state confirmed: three compile errors:
  1. Applied variant missing encounter_type/beat_id fields
  2. SkippedResolved variant doesn't exist
  3. apply_beat_dispatch/BeatDispatchOutcome not at crate root

Dev must fix all three compile errors + the three behavioral fixes
(#1, #2, #5) during green rework.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat(37-14): rework beat dispatch silent-drop (pass 2)

Apply all 6 Reviewer-required fixes from pass-1 review:

1. Gate per-actor breadcrumb on Applied outcome (dispatch/mod.rs)
2. Add encounter.escalation_failed WatcherEvent on escalate_to_combat None branch
3. Restructure BeatDispatchOutcome::Applied { encounter_type, beat_id } to
   collapse triple-expect lookup chain to double-expect with caller-guaranteed
   contract
4. pub use apply_beat_dispatch + BeatDispatchOutcome at crate root to support
   a true integration test on the real global telemetry channel
5. Rename action=X to event=encounter.state.X across 4 StructuredEncounter
   emissions; state. prefix disambiguates from dispatch-layer encounter.X
   events (Option A collision resolution)
6. Add SkippedResolved variant with pre-apply_beat short-circuit on
   encounter.resolved; removes pass-1 regression where multi-actor
   post-resolution beats misleadingly emitted beat_apply_failed

Verified via targeted regression sweeps on sidequest-game (encounter/standoff/
social/genre_confrontation/beat_filter stories) and sidequest-agents. New
integration test drives a live GameSnapshot through apply_beat_dispatch and
subscribes to sidequest_telemetry::subscribe_global — same channel the GM
panel consumes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* test(37-14): pass-3 RED — drive Reviewer fixes #1, #3, #7, #10, #11

Pass-2 Reviewer REJECTED with 8 HIGH findings. Three needed test-side
drivers:

- case_f_resolved_encounter_with_unknown_beat_id_emits_skipped_resolved
  drives fix #1: ordering bug where UnknownBeatId is checked before
  SkippedResolved. RED: current code returns UnknownBeatId on a resolved
  encounter + unknown beat_id, firing Severity::Error on a legitimate
  multi-actor post-resolution scenario. Dev must move the is_resolved
  short-circuit to fire immediately after the NoEncounter match arm.

- wiring_player_beat_from_narrator_ignored_carries_source_field drives
  fix #3: source-scan anchored on the exact event identifier with a
  400-byte lookahead window requiring the .field("source",
  "narrator_beat_selection") fingerprint. RED: the emission at
  dispatch/mod.rs:1823 has no source field.

- wiring_per_actor_breadcrumb_gated_on_applied_outcome locks in fix
  #11 as a structural regression guard. Byte-offset scan asserts both
  quoted breadcrumb literals appear AFTER the BeatDispatchOutcome::
  Applied { anchor. Passes on current (correct) production code;
  tripwire for future refactors that re-break the Applied gating.

Plus two shim self-tests for fix #10 (find_events_by_action shim in
28-2 tests had no self-test — now covers both the encounter state-key
branch and the legacy action-key fallback) and one test-quality
tightening for fix #7 (npc_turns substring replaced the vacuous
OR chain with a tight beat::apply_beat_dispatch( qualified-call match).

Building a runtime DispatchContext fixture for fixes #3 and #11 would
require constructing 30+ struct fields pulling from half the server
crate — source-scans with tight anchored windows are the pragmatic
alternative. Logged as a design deviation.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* feat(37-14): rework beat dispatch silent-drop (pass 3)

Apply all 8 Reviewer-required HIGH fixes from pass-2 review plus 4
rolled-in MEDIUM fixes:

Fix #1 — move is_resolved short-circuit to fire immediately after the
NoEncounter arm, before NoDef and UnknownBeatId. A resolved encounter
short-circuits highest priority among encounter-live branches — once the
encounter is done, every incoming beat is skipped regardless of beat_id
or def validity. Closes the ordering bug where a hallucinated beat_id
against a resolved encounter fired false Severity::Error on a normal
multi-actor post-resolution scenario. case_f now green.

Fix #2 — replace .unwrap_or(0) / .unwrap_or(false) silent defaults in
handle_applied_side_effects with a single .expect() binding matching the
def/beat .expect() precedent six lines above. Contract violation is now
a loud crash rather than silent zero-default telemetry.

Fix #3 — add .field("source", "narrator_beat_selection") to the
encounter.player_beat_from_narrator_ignored emission in dispatch/mod.rs.
Attribution contract now consistent across every beat-dispatch event.
wiring_player_beat_from_narrator_ignored_carries_source_field green.

Fix #4 option (a) — remove BeatDispatchOutcome::ApplyFailed variant
entirely. Delete the Err(e) match arm. The NoEncounter / SkippedResolved /
NoDef / UnknownBeatId short-circuits exhaust apply_beat's Err causes;
the call is now .expect()-wrapped with rationale. #[non_exhaustive] on
the outcome enum still provides forward-compat if a future apply_beat
Err cause needs its own variant. Subsumes finding #9. Closes code/doc
contradiction — the test module doc narrative is now truthful.

Fix #5 — rewrite apply_beat_dispatch doc comment. Drop the false "always
emits exactly one WatcherEvent" contract claim. State honestly that
non-Applied outcomes emit exactly one event, Applied triggers an
additional state-machine-layer event (or two) from
StructuredEncounter::apply_beat. Document the outcome ordering
explicitly.

Fix #6 — align tracing severity with OTEL severity on three branches:
  UnknownBeatId: Severity::Warn (was Error) — 4xx narrator bad input.
  SkippedResolved: tracing::warn! (was info!) — anomalous-but-expected.
  ApplyFailed: subsumed by fix #4 (variant removed entirely).

Fix #8 — update dispatch/mod.rs inline event list to include
beat_skipped_resolved and note ApplyFailed removal.

Fix #12 — rewrite integration test doc contrast to state the real
distinction (public-API reachability via `use sidequest_server::`),
not a fictitious difference in telemetry channel.

Verified via full 37-14 suite (11/11 pass), full sidequest-server
crate (lib + 423 integration), targeted sidequest-game regression
sweep, sidequest-agents sweep, and clippy -D warnings clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

* fix(37-14): close pass-3 review findings in place

Reviewer pass-3 adversarial review found 4 new HIGH findings plus
1 pre-existing observability lie discovered during pass-3. Fixed
in place rather than round-tripping another TEA/Dev cycle.

Fix A — Lying comment at beat.rs:223-244. The pass-3 comment claimed
"apply_beat_dispatch's match below will fail to compile (we use a
non-exhaustive match with no wildcard on the Result)" but the actual
code uses .expect(), not match. Rewritten to honestly describe the
runtime-panic contract, explicitly enumerate the two exhausted Err
causes (SkippedResolved exhausts `self.resolved`; UnknownBeatId
exhausts `def.beats` lookup), and reference the EncounterApplyError
delivery finding as the stronger compile-time design. The expect
string now names the specific Err causes it short-circuits.

Fix B — Per-actor breadcrumb wrong metric on escalation (pre-existing
pass-2 bug discovered by pass-3 edge hunter). The stat_check_result
was read from ctx.snapshot.encounter AFTER handle_applied_side_effects,
which replaces the encounter on escalation — the breadcrumb carried
the STARTING metric of the escalated combat encounter instead of the
metric of the beat that just resolved. Moved the read BEFORE the
helper call so the breadcrumb lands on the correct pre-escalation
value.

Fix C — Redundant `if let Some(ref encounter)` guard at the escalation
check in handle_applied_side_effects. Currently safe because nothing
between lines 371-390 can null ctx.snapshot.encounter, but
structurally inconsistent with the line-371 .expect() fix and would
silently skip the escalation check under a future refactor. Replaced
with a matching .expect() that cites the line-371 invariant.

Fix D — SkippedResolved event missing `resolved=true` field. Every
other encounter event involving a resolved encounter carries this
field; the event name itself is primary but the field is diagnostic
context for GM-panel filtering. Added.

Fix E — Test module doc stale counts. Matrix listed cases A-E
(pass-3 added Case F), and mentioned "two source-scanning wiring
tests" (pass 3 added two more bringing the total to four). Rewritten
with Case F, four wiring tests listed by name, and the new regression
guard noted.

Fix F — Per-actor breadcrumb wiring test used byte-offset-only
check. A refactor moving the emission BELOW the Applied block's
closing brace but still above the anchor in byte terms would have
silently passed. Replaced with a balanced-brace scan that finds the
actual `if let Applied { .. } = &outcome { .. }` block bounds and
asserts every breadcrumb literal lives strictly inside them.

Fix G — New regression-lock source-scan test
`wiring_no_silent_defaults_in_handle_applied_side_effects`. The
pass-2 .unwrap_or(0)/.unwrap_or(false) silent-default regression
fix was not locked in by any test. This scan strips // comments
from the function body, forbids any .unwrap_or(0) / .unwrap_or(false)
/ .unwrap_or_default(), and positively asserts the .as_ref().expect()
pattern exists. A future refactor reverting the silent defaults
fails this test loudly.

All 12/12 beat_dispatch_story_37_14 tests pass. Full sidequest-server
crate clean (lib + integration). Targeted sidequest-game regression
sweep clean. clippy -D warnings clean.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 18, 2026
Review round 1 rejected for CRITICAL/HIGH findings: the chokepoint's
Option<String> return was discarded at all three call sites, so when
dedup fired the old player_id persisted in turn_barrier, perception_
filters, and pending_dice_requests — reproducing the playtest-2026-04-12
deadlock one subsystem up (phantom-free `players`, phantom-present
barrier roster).

Round 2 fixes:

1. Add SharedGameSession::reconcile_removed_player(old_pid) — evicts
   the stale pid from turn_barrier, perception_filters, and any
   pending_dice_requests whose rolling_player_id matched. Emits
   tracing::info! + WatcherEvent (phantom_player_reconciled).

2. #[must_use] on insert_player_dedup_by_name with a message that
   spells out the contract — the compiler now warns if a future caller
   drops the return. This is the type-system-as-lie-detector pattern.

3. lib.rs:2478 — bind the return and call reconcile_removed_player on
   Some(old_pid). This is the site that introduced the original bug
   via playtest 2026-04-12.

4. connect.rs — both call sites now handle the return defensively.
   Reconnect-transfer site adds a debug_assert that the return is
   None (its caller manually removed the old entry above, so dedup
   should find no phantom) plus a release-build safety net that
   reconciles if the invariant ever drifts. Fresh-connect site
   reconciles if is_reconnect==false somehow still yields a phantom.

5. Downgrade `pub players` to `pub(crate)` with explanatory comment —
   rule #9: fields carrying invariants must not be writable from
   outside the module. Added contains_player() and
   has_perception_filter() read-only accessors for tests.

6. Add tracing::warn! on the phantom-removal branch of the chokepoint
   — rule #4 requires both OTEL (GM panel) and tracing (log
   aggregators).

7. Test fixes:
   - AC-1/AC-2 now use contains_player() getter (pub(crate) field)
   - AC-2 asserts None return for both distinct-name inserts
   - AC-3/AC-4/AC-5 assert the setup insert's None return explicitly
     (the #[must_use] warning on discarded setup calls was legitimate)
   - AC-6/AC-7 use format! args so {raw_inserts} actually interpolates
   - AC-9 tightened from any-substring-anywhere to adjacency-to-
     WatcherEventBuilder::new + .field("event", ...), rejects
     comment-only hits
   - AC-10 (new): grep each production call site for
     reconcile_removed_player adjacency — locks the wiring invariant
   - AC-11 (new): full end-to-end runtime integration test — install
     TurnBarrier with old-pid, install PerceptionFilter with old-pid,
     reconnect under new-pid, feed returned pid through reconcile,
     assert players AND both downstream rosters collapsed. This is
     the test that would have caught the round-1 bug.

8. Rustdoc updates: chokepoint doc no longer overstates the wiring
   test scope; stale comment at connect.rs reconnect-transfer site
   clarified as a no-op that exists for code-path uniformity.

Tests: 11/11 green (9 original + AC-10 + AC-11).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 18, 2026
* test(37-19): failing tests for phantom-player dedup chokepoint

Adds RED harness for Story 37-19 (phantom-player dedup on resume).
Locks a single dedup chokepoint — SharedGameSession::insert_player_dedup_by_name
— as the contract that both insertion sites (dispatch/connect.rs and
lib.rs) must route through. Behavioral tests exercise the chokepoint;
source-level wiring tests forbid raw `players.insert` to prevent a
future third insertion site from bypassing dedup.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(37-19): route player insertion through dedup chokepoint

Add SharedGameSession::insert_player_dedup_by_name — on reconnect with
the same player_name under a new player_id, the old entry is removed and
its pid returned so callers can reconcile turn barrier, perception
filters, and external rosters. Emits a phantom_player_removed OTEL
watcher event so the GM panel can see the dedup fire.

Both insertion sites now route through the chokepoint:
- src/lib.rs returning-player path (was the phantom source from playtest
  2026-04-12 — unconditional insert under new pid, no reconnect check)
- src/dispatch/connect.rs transfer and new-player paths (reconnect
  detection was already correct, but routing through the chokepoint
  keeps the invariant enforced in one place)

Source-level wiring tests forbid raw players.insert() outside
shared_session.rs so a future third insertion site can't regress the
phantom-player bug silently.

Incidental cargo fmt drift in dispatch/{mod,sealed_letter,telemetry}.rs
and four sibling test files — whitespace-only, bundled per project
policy.

Tests: 9/9 green (5 behavioral, 3 wiring, 1 OTEL).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(37-19): reconcile downstream rosters after dedup + enforce contract

Review round 1 rejected for CRITICAL/HIGH findings: the chokepoint's
Option<String> return was discarded at all three call sites, so when
dedup fired the old player_id persisted in turn_barrier, perception_
filters, and pending_dice_requests — reproducing the playtest-2026-04-12
deadlock one subsystem up (phantom-free `players`, phantom-present
barrier roster).

Round 2 fixes:

1. Add SharedGameSession::reconcile_removed_player(old_pid) — evicts
   the stale pid from turn_barrier, perception_filters, and any
   pending_dice_requests whose rolling_player_id matched. Emits
   tracing::info! + WatcherEvent (phantom_player_reconciled).

2. #[must_use] on insert_player_dedup_by_name with a message that
   spells out the contract — the compiler now warns if a future caller
   drops the return. This is the type-system-as-lie-detector pattern.

3. lib.rs:2478 — bind the return and call reconcile_removed_player on
   Some(old_pid). This is the site that introduced the original bug
   via playtest 2026-04-12.

4. connect.rs — both call sites now handle the return defensively.
   Reconnect-transfer site adds a debug_assert that the return is
   None (its caller manually removed the old entry above, so dedup
   should find no phantom) plus a release-build safety net that
   reconciles if the invariant ever drifts. Fresh-connect site
   reconciles if is_reconnect==false somehow still yields a phantom.

5. Downgrade `pub players` to `pub(crate)` with explanatory comment —
   rule #9: fields carrying invariants must not be writable from
   outside the module. Added contains_player() and
   has_perception_filter() read-only accessors for tests.

6. Add tracing::warn! on the phantom-removal branch of the chokepoint
   — rule #4 requires both OTEL (GM panel) and tracing (log
   aggregators).

7. Test fixes:
   - AC-1/AC-2 now use contains_player() getter (pub(crate) field)
   - AC-2 asserts None return for both distinct-name inserts
   - AC-3/AC-4/AC-5 assert the setup insert's None return explicitly
     (the #[must_use] warning on discarded setup calls was legitimate)
   - AC-6/AC-7 use format! args so {raw_inserts} actually interpolates
   - AC-9 tightened from any-substring-anywhere to adjacency-to-
     WatcherEventBuilder::new + .field("event", ...), rejects
     comment-only hits
   - AC-10 (new): grep each production call site for
     reconcile_removed_player adjacency — locks the wiring invariant
   - AC-11 (new): full end-to-end runtime integration test — install
     TurnBarrier with old-pid, install PerceptionFilter with old-pid,
     reconnect under new-pid, feed returned pid through reconcile,
     assert players AND both downstream rosters collapsed. This is
     the test that would have caught the round-1 bug.

8. Rustdoc updates: chokepoint doc no longer overstates the wiring
   test scope; stale comment at connect.rs reconnect-transfer site
   clarified as a no-op that exists for code-path uniformity.

Tests: 11/11 green (9 original + AC-10 + AC-11).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* style(37-19): apply cargo fmt — review round-2 preflight fix

Mechanical whitespace normalization only; no logic changes. Reviewer
preflight caught 3 fmt diffs in round-2 review (approved-with-findings).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 19, 2026
- Add #[non_exhaustive] to EdgeDeltaOutcome (matches BeatDispatchOutcome
  precedent; 39-5/39-6 will grow the struct).
- Update apply_beat_edge_deltas docstring to name the resource_deltas
  carve-out explicitly so the "No silent fallbacks" claim is accurate.
- Replace `if let Some(enc)` on composure-break path with .expect();
  composure_break is only set when a live encounter mutated it, so a
  missing encounter at emit time is a broken invariant, not graceful.
- Add tracing::warn! to the resource_deltas Err branch to match the
  gold-delta logging precedent (rule #4 — tracing coverage).
- Drop stale "RED:" prefix from test module preambles now that the
  tests ship green alongside the implementation.
- New test: target_debit_opponent_actor_without_matching_npc_panics_loudly
  pins the second panic path (opponent named in actors, missing from
  snapshot.npcs) distinctly from the role-missing panic.
- New test: resource_deltas_positive_value_credits_named_resource_pool
  exercises the ResourcePatchOp::Add branch and verifies delta=+1.0 on
  the patched event.
- New test: resource_deltas_unknown_resource_emits_warning_and_continues
  pins the documented silent-warn carve-out for unknown resource names.
- cargo fmt fixes on edge_config_* test files (pre-existing formatting
  debt touched by the fmt pass).

All 4 genre + 11 server integration tests pass. cargo clippy and
cargo fmt --check both clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Apr 19, 2026
…ring (#493)

* test(39-4): add failing tests for BeatDef edge_delta dispatch

RED phase for story 39-4. Pins the contract for:
- BeatDef.edge_delta / target_edge_delta / resource_deltas schema
- apply_beat_edge_deltas public helper (self-debit, target-debit,
  composure break, resource routing)
- handle_applied_side_effects wiring (source-scan guard)

Fails with 8 E0609 errors on the missing BeatDef fields, plus an
unresolved sidequest_server::{apply_beat_edge_deltas, EdgeDeltaOutcome}
import in the integration test — the expected RED signal.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(39-4): BeatDef edge_delta + target_edge_delta + beat dispatch wiring

GREEN phase for story 39-4. Makes the epic-39 Edge subsystem real in
beat dispatch:

- BeatDef grows three optional fields: edge_delta (self-debit),
  target_edge_delta (primary opponent debit), resource_deltas
  (named ResourcePool routing). Legacy beats keep parsing unchanged.
- New pub helper apply_beat_edge_deltas(snapshot, beat, encounter_type)
  -> EdgeDeltaOutcome on dispatch/beat.rs, re-exported at the crate
  root. Emits creature.edge_delta per debit and encounter.composure_break
  on Edge<=0, auto-resolves the encounter.
- No silent fallback: panics if target_edge_delta is set but no primary
  opponent (role="opponent") is declared in the encounter.
- handle_applied_side_effects now calls apply_beat_edge_deltas on the
  Applied outcome, wiring it into the production dispatch loop.
- CharacterBuilder: hardcoded Fighter +2 edge_max advancement stub,
  tagged REPLACED IN 39-5 (ADR-078). Emits chargen.advancement_stub_applied
  OTEL for GM-panel visibility.
- Integration tests: 8 tests covering self-debit, target-debit, missing-
  opponent panic, composure break (both sides), resource delta routing,
  public API wiring, and source-scan guard on handle_applied_side_effects.
- Genre schema tests: 4 tests pinning the three new BeatDef fields.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* review(39-4): address reviewer findings — EdgeDeltaOutcome + coverage

- Add #[non_exhaustive] to EdgeDeltaOutcome (matches BeatDispatchOutcome
  precedent; 39-5/39-6 will grow the struct).
- Update apply_beat_edge_deltas docstring to name the resource_deltas
  carve-out explicitly so the "No silent fallbacks" claim is accurate.
- Replace `if let Some(enc)` on composure-break path with .expect();
  composure_break is only set when a live encounter mutated it, so a
  missing encounter at emit time is a broken invariant, not graceful.
- Add tracing::warn! to the resource_deltas Err branch to match the
  gold-delta logging precedent (rule #4 — tracing coverage).
- Drop stale "RED:" prefix from test module preambles now that the
  tests ship green alongside the implementation.
- New test: target_debit_opponent_actor_without_matching_npc_panics_loudly
  pins the second panic path (opponent named in actors, missing from
  snapshot.npcs) distinctly from the role-missing panic.
- New test: resource_deltas_positive_value_credits_named_resource_pool
  exercises the ResourcePatchOp::Add branch and verifies delta=+1.0 on
  the patched event.
- New test: resource_deltas_unknown_resource_emits_warning_and_continues
  pins the documented silent-warn carve-out for unknown resource names.
- cargo fmt fixes on edge_config_* test files (pre-existing formatting
  debt touched by the fmt pass).

All 4 genre + 11 server integration tests pass. cargo clippy and
cargo fmt --check both clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant