Skip to content

feat(agents): prompt framework — PromptSection, attention zones, SOUL.md parser#5

Merged
slabgorb merged 2 commits intodevelopfrom
feat/1-9-prompt-framework
Mar 25, 2026
Merged

feat(agents): prompt framework — PromptSection, attention zones, SOUL.md parser#5
slabgorb merged 2 commits intodevelopfrom
feat/1-9-prompt-framework

Conversation

@slabgorb
Copy link
Copy Markdown
Owner

Summary

  • PromptSection with AttentionZone ordering (5 zones: Primacy → Recency)
  • SectionCategory (7 variants) and RuleTier (Critical/Firm/Coherence)
  • SOUL.md parser — regex-based principle extraction from markdown
  • PromptComposer trait — zone-ordered registration, filtering by category/zone, compose
  • 63 tests, all passing. Reviewer approved.

Test plan

  • cargo test --workspace — 209 passing
  • cargo clippy — clean on new code
  • Reviewer approved

🤖 Generated with Claude Code

slabgorb and others added 2 commits March 25, 2026 16:39
49 failing tests covering:
- AttentionZone ordering (Primacy < Early < Valley < Late < Recency)
- SectionCategory and RuleTier enum completeness and serde
- PromptSection construction, token estimation, serde roundtrip
- SOUL.md parser (principle extraction, title, description, edge cases)
- SoulData methods (len, is_empty, get, as_prompt_text)
- PromptComposer trait (zone-ordered registration, filtering, compose)

All types are stubbed with todo!() — compiles but fails (RED state).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Implement all todo!() stubs for AttentionZone ordering, PromptSection
construction/methods, SoulData methods, and parse_soul_md with regex
extraction. All 63 tests pass.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@slabgorb slabgorb merged commit 60c2f12 into develop Mar 25, 2026
slabgorb added a commit that referenced this pull request Mar 25, 2026
* test: add failing tests for genre pack models (RED)

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>

* test: add scenario pack tests (pulp_noir prototype)

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>

* feat(1-3): implement genre pack models, loader, and validation

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>

* refactor: simplify code per verify review

- 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>

* style: apply cargo fmt

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

* fix: address reviewer findings from PR #4

- 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>

---------

Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@slabgorb slabgorb deleted the feat/1-9-prompt-framework branch March 25, 2026 23:07
slabgorb added a commit that referenced this pull request Mar 26, 2026
RED phase — 37 tests covering all 11 ACs:
- CharacterBuilder state machine (InProgress → AwaitingFollowup → Confirmation)
- Scene progression, choice selection, freeform input
- Hook prompt / followup flow
- Back/revert with SceneResult stack
- NarrativeHook extraction from mechanical effects
- LoreAnchor auto-fill
- Stat generation (standard_array)
- Starting inventory from item_hints
- WebSocket message construction
- Error variants and invalid input handling
- Rust lang-review rule enforcement (#2, #5, #6, #9)

Compile errors expected:
- sidequest_game::builder module does not exist yet
- CharCreationScene::hook_prompt field needs adding to genre crate

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
slabgorb added a commit that referenced this pull request Mar 26, 2026
* test: add failing tests for story 2-2 session actor

RED phase — 7 ACs covered:
- Session state machine (AwaitingConnect → Creating → Playing)
- Genre binding via connect
- State-appropriate message dispatch
- Out-of-phase rejection
- Session cleanup
- Connected response with has_character
- Multiple independent sessions

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

* feat(2-2): session actor — state machine, genre binding, phase dispatch

Implement Session type with AwaitingConnect → Creating → Playing states:
- Session::new(), handle_connect(), complete_character_creation()
- State queries: is_awaiting_connect(), is_creating(), is_playing()
- Phase-appropriate message dispatch via can_handle_message_type()
- Genre/world/player_name binding on connect
- Connected response with has_character flag
- Cleanup resets to AwaitingConnect

58/58 tests passing (18 new + 25 story-2-1 + 15 story-1-12).

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

* test: add failing tests for story 2-3 CharacterBuilder

RED phase — 37 tests covering all 11 ACs:
- CharacterBuilder state machine (InProgress → AwaitingFollowup → Confirmation)
- Scene progression, choice selection, freeform input
- Hook prompt / followup flow
- Back/revert with SceneResult stack
- NarrativeHook extraction from mechanical effects
- LoreAnchor auto-fill
- Stat generation (standard_array)
- Starting inventory from item_hints
- WebSocket message construction
- Error variants and invalid input handling
- Rust lang-review rule enforcement (#2, #5, #6, #9)

Compile errors expected:
- sidequest_game::builder module does not exist yet
- CharCreationScene::hook_prompt field needs adding to genre crate

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

* feat(2-3): implement CharacterBuilder state machine

- Add builder module to sidequest-game with typed BuilderPhase enum
  (InProgress, AwaitingFollowup, Confirmation)
- SceneResult stack for clean revert (one pop, not six parallel lists)
- NarrativeHook extraction from mechanical effects (race→Origin,
  class/personality→Trait, relationship→Relationship, goals→Goal,
  item→Possession)
- Standard array stat generation from RulesConfig
- Starting inventory from accumulated item_hints
- Auto-fill lore anchors (faction, npc, location) when missing
- CharacterCreation WebSocket message construction
- BuilderError with typed variants (InvalidChoice, WrongPhase, etc.)
- Add hook_prompt field to CharCreationScene in genre crate

All 51 tests GREEN.

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 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 Mar 31, 2026
RED phase — 25 tests across 3 crates covering all 5 ACs:
- AC1 (Parse): ResourceDeclaration deserialization, RulesConfig.resources field
- AC2 (Inject): register_resource_section on PromptRegistry (Valley/State zone)
- AC3 (Track): apply_resource_deltas with clamp and unknown-resource handling
- AC4 (Persist): GameSnapshot.resource_state serde roundtrip, old save compat
- AC5 (All genres): Empty resources default, no section for empty declarations

Rule coverage: #5 (validated constructors), #8 (Deserialize bypass)

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

* test: add failing tests for 16-1 narrator resource injection

RED phase — 25 tests across 3 crates covering all 5 ACs:
- AC1 (Parse): ResourceDeclaration deserialization, RulesConfig.resources field
- AC2 (Inject): register_resource_section on PromptRegistry (Valley/State zone)
- AC3 (Track): apply_resource_deltas with clamp and unknown-resource handling
- AC4 (Persist): GameSnapshot.resource_state serde roundtrip, old save compat
- AC5 (All genres): Empty resources default, no section for empty declarations

Rule coverage: #5 (validated constructors), #8 (Deserialize bypass)

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

* feat(16-1): implement narrator resource injection

Add ResourceDeclaration to genre crate with serde(try_from) validation
(max >= min, starting in range). Add resource_state HashMap and
resource_declarations Vec to GameSnapshot with apply_resource_deltas
for clamped delta application. Add register_resource_section to
PromptRegistry — Valley zone, State category, human-readable format
with voluntary/involuntary flags and decay rates. Falls back to
starting value when state is missing. Empty declarations produce no
section (backward compatible for genres without resources).

27/27 tests GREEN across 3 crates. 599 existing lib tests unaffected.

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 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 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>
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