Conversation
slabgorb
added a commit
to slabgorb/sidequest
that referenced
this pull request
Apr 10, 2026
…combatant, party_reconciliation, progression Final commit d277562 implements Architect Option A for the combatant subsystem after three reviewer round-trips: - consequence.wish_evaluated: emits on every evaluate() call - combatant.bloodied: emits from state::broadcast_state_changes() (called from dispatch/mod.rs:1737 every turn), gated on delta.characters_changed(), fires when any friendly character is below half HP. Combatant trait reverted to telemetry-free pure accessors. - party_reconciliation.reconciled: emits on all three reconcile() outcomes - progression.stat_scaled: emits from level_to_hp/damage/defense Tests: 19/19 OTEL integration + 487/487 sidequest-game lib tests GREEN. Non-blocking improvements deferred to follow-up tickets: - Stale doc on state.rs::lowest_friendly_hp_ratio (rework #2 carryover) - Missing is_friendly=false negative test (defensive guard untested) - delta.rs::to_json silent fallback (pre-existing, separate ticket) - Wiring test rigor inconsistency across the four 35-10 subsystems - Story 5-7 wiring debt (lowest_friendly_hp_ratio not wired into pacing) PR slabgorb/sidequest-api#397 merged to develop. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…y_reconciliation/progression Adds 18 failing tests covering four sidequest-game subsystems that currently emit zero OTEL watcher events (per the epic-35 audit). Follows the exact pattern from the shipped 35-9 NPC subsystems test file. Tests by subsystem: consequence (3): WishConsequenceEngine::evaluate() must emit consequence.wish_evaluated for both is_power_grab=true (with category + rotation_counter) and is_power_grab=false (with null category and unchanged counter). Distinguishing 'engine declined' from 'engine never called' is the whole point. combatant (4): Combatant::hp_fraction() must emit combatant.bloodied exactly when the result is < 0.5. Negative tests cover: full HP, exactly 0.5 (strict <), and degenerate max_hp=0. Trait-level instrumentation with a noise-controlled threshold avoids flooding the channel. party_reconciliation (3): PartyReconciliation::reconcile() must emit party_reconciliation.reconciled for all three result variants — no_action_needed, split_party_allowed, and reconciled (with target_location and moved_count). Single component, single action, result discriminated by the 'result' field. progression (4): level_to_hp/level_to_damage/level_to_defense must emit progression.stat_scaled when level > 1. Level 1 is the no-op default and must NOT emit (negative test included). Stat identified by the 'stat' field so all three scaling functions share one event type. A5 wiring assertions (4): grep-based checks confirm each subsystem still has a non-test consumer in production code: - consequence ← sidequest-server/src/dispatch/mod.rs - party_reconciliation ← sidequest-server/src/dispatch/connect.rs - progression ← sidequest-server/src/dispatch/state_mutations.rs - combatant ← sidequest-game/src/state.rs (broadcast_state_changes) RED verified locally: test result: FAILED. 8 passed; 10 failed - 8 passing: 4 wiring + 3 combatant negative cases + 1 progression no-op - 10 failing: every OTEL emission assertion (events not yet wired) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Closes the wiring gap identified by the epic-35 audit. Four production
subsystems were fully implemented but emitted zero OTEL events, so the
GM panel could not distinguish actual subsystem engagement from Claude
improvising. Follows the 35-8 / 35-9 / 35-13 pattern using
WatcherEventBuilder.
consequence.rs — WishConsequenceEngine::evaluate() now emits
consequence.wish_evaluated on BOTH the power-grab and non-power-grab
branches. The non-power-grab event has category=null and unchanged
rotation_counter; this distinguishes "engine declined" from "engine
never called" — exactly the silent-fallback case the OTEL principle
is meant to expose.
combatant.rs — Combatant::hp_fraction() default trait method now emits
combatant.bloodied when the result is strictly less than 0.5.
Threshold-gated to avoid flooding the channel on every state-build
query. Skipped when max_hp == 0 (no meaningful HP state to report).
Default-method instrumentation propagates to all implementors
(Character, Npc, CreatureCore) without per-impl changes.
party_reconciliation.rs — PartyReconciliation::reconcile() now emits
party_reconciliation.reconciled for ALL three result variants
(no_action_needed / split_party_allowed / reconciled). Single
component+action with the variant on the `result` field;
target_location and moved_count populated only on the reconciled
outcome. Extracted to a small emit_watcher_event() helper to keep
the four call sites consistent.
progression.rs — level_to_hp / level_to_damage / level_to_defense now
emit progression.stat_scaled, distinguished by the `stat` field
("hp" / "damage" / "defense"). Gated on level > 1 — level 1 is the
no-op default and would flood the channel on every starter character
query. Extracted to an emit_stat_scaled() helper. xp_for_level() is
intentionally NOT instrumented; it is called every action just for
the threshold check and the meaningful "level-up" event belongs at
the call site in dispatch/state_mutations.rs as a follow-up.
Tests: 18/18 in otel_subsystems_story_35_10_tests, 487/487 in lib,
clippy clean. No regressions.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
simplify-efficiency review (verify phase) flagged six unnecessary heap allocations in the watcher event emission added by 35-10. WatcherEventBuilder::field() takes (key: &str, value: impl Serialize), and &str / &'static str / Option<&str> all already implement Serialize — the .to_string() conversions were pure waste, especially in Combatant::hp_fraction() which is called on every state-build query. - consequence.rs: drop player_name.to_string() (×2) and category_str(category).to_string() - combatant.rs: drop self.name().to_string() in the bloodied emission - party_reconciliation.rs: drop result.to_string(); collapse the Option<&str> -> serde_json::Value match into a direct .field() call (Option<T: Serialize> serializes to null/string automatically) - progression.rs: drop stat.to_string() (was &'static str) Tests: 18/18 in otel_subsystems_story_35_10_tests, 487/487 in lib. No regressions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reviewer rework. The previous wiring assertion checked
`Combatant::hp(` and `Combatant::max_hp(` in state.rs, both of
which exist at lines 797-798 (UFCS form) for unrelated reasons.
The grep gave false confidence: state.rs never calls hp_fraction(),
so the combatant.bloodied OTEL event added by 35-10 is unreachable
from production code paths.
Specifically, state.rs::lowest_friendly_hp_ratio() at line 402
manually inlines `c.hp() as f64 / max as f64` (with the max == 0
short-circuit duplicated) instead of delegating to hp_fraction().
That's the canonical "is anyone bloodied?" check in the codebase
and it bypasses the new instrumentation entirely.
This commit corrects the wiring assertion to grep for the literal
`hp_fraction(` call. Until production code (state.rs:402) is
updated to delegate to hp_fraction(), this assertion fails RED:
test wiring_combatant_hp_fraction_reached_by_state ... FAILED
state.rs must call Combatant::hp_fraction() (not inline the
hp/max_hp ratio math) — without this call the combatant.bloodied
OTEL event is unreachable from production state code...
Dev's task in the GREEN phase: replace the inline math at
state.rs:402 with `c.hp_fraction()`. Same semantics (hp_fraction
already short-circuits on max_hp == 0), and the OTEL event becomes
reachable.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reviewer rework. The combatant.bloodied OTEL event added in 35-10 was unreachable from production because state.rs:: lowest_friendly_hp_ratio() inlined the fraction math (c.hp() as f64 / max as f64 with a max == 0 short-circuit) instead of delegating to Combatant::hp_fraction(). The canonical "is anyone bloodied?" check in the codebase bypassed the new instrumentation entirely. Replaced the inline computation with c.hp_fraction(). Same semantics — the trait method already short-circuits on max_hp == 0 (combatant.rs:33-35) — and the OTEL event is now reachable from every state-build that consults party HP. This is a 4-line change (DRY win + closes the wiring gap): - 7 lines of inline math → 1 line delegation - The redundant `max_hp == 0` guard goes away - Added a doc comment explaining the OTEL reachability rationale Test verification: - otel_subsystems_story_35_10_tests: 18/18 PASS (was 17/18 with the corrected wiring assertion failing RED) - sidequest-game lib: 487/487 PASS (no regressions) Closes the HIGH-severity Reviewer finding. Medium-severity findings (rotation_counter timing comment, hp_fraction docstring, missing negative tests, category_str serde drift) are noted in the session as follow-up improvements but were intentionally NOT included in this rework — keeping the change surgical. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…bloodied Rework architect decision (Option A): the combatant.bloodied emission moves from Combatant::hp_fraction() (a pure accessor with zero production callers) to state::broadcast_state_changes() — the function dispatched from sidequest-server/src/dispatch/mod.rs:1737 every turn to build PARTY_STATUS. Emission is gated on delta.characters_changed() so it only fires on turns where combat math has actually mutated a character. Replace the four TestCombatant-based hp_fraction() tests with five GameSnapshot + StateDelta behavioral tests that exercise broadcast_state_changes directly: - emits when friendly drops below half hp - does NOT emit at full hp - does NOT emit at exactly 0.5 hp (strict less-than threshold) - does NOT emit when max_hp = 0 (degenerate) - does NOT emit when delta.characters_changed() is false (new debounce test) Replace the source-grep wiring test (which was vacuous against doc comments) with a behavioral integration test that calls broadcast_state_changes and asserts event emission — no more include_str! loopholes. RED state: 19 tests, 2 failures (the two positive assertions). Dev will turn this GREEN by relocating the emission per the architect spec in the session file's Reviewer Addendum. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…e_changes (Option A) Architect decision (Naomi Nagata, design mode): the combatant.bloodied OTEL emission moves out of Combatant::hp_fraction() (a pure accessor with zero production callers — story-5-7 wiring debt) and into the per-turn state-ship site state::broadcast_state_changes(), gated on delta.characters_changed(). This is the same pattern disposition::apply_delta and belief_state::add_belief already use: telemetry at the mutation/transition site, never inside a pure accessor. broadcast_state_changes is dispatched from sidequest-server/src/dispatch/mod.rs:1737 every turn to build the PARTY_STATUS message, so this site is the canonical chokepoint where the GM panel can observe combat engagement (CLAUDE.md OTEL Observability Principle). Changes: - combatant.rs: revert hp_fraction() to a pure accessor. Drop the WatcherEventBuilder block and the sidequest_telemetry import. Update the doc comment to point at the new emission site. - state.rs: add the bloodied threshold check + WatcherEventBuilder emission inside broadcast_state_changes after the PARTY_STATUS push, gated on delta.characters_changed() so it only fires on turns where combat math has actually mutated a character. Field shape unchanged: action="bloodied", name, hp, max_hp, hp_fraction. - state.rs: add the sidequest_telemetry import (was previously transitive via combatant.rs). The Combatant trait is now telemetry-free again — closes the "first default trait method with a side effect" architectural concern definitively, not just by argument. NOT touched in this rework: - consequence, party_reconciliation, progression — verified clean in re-review #2 - state::lowest_friendly_hp_ratio — story-5-7 dead code, separate concern, delegation-to-hp_fraction added in rework #2 left in place (it is semantically correct, just unreachable from production) Tests: - otel_subsystems_story_35_10_tests: 19/19 PASS (was 17/19 before this fix) - 2 positive assertions flipped GREEN (broadcast_state_changes_emits_* and wiring_broadcast_state_changes_reaches_combatant_bloodied_in_production) - 4 negative assertions still PASS (full HP, exactly half, max_hp=0, delta unchanged) — guards against false positives - sidequest-game --lib: 487/487 PASS — no regression - combatant.rs inline unit tests untouched (they only exercise the math contract, never asserted emission) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
d277562 to
d3b4b63
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Instruments four sidequest-game subsystems with OTEL watcher events so the GM panel can verify they are engaged and making decisions:
consequence.wish_evaluatedevent fires whenevaluate()is called (both true and false branches per no-silent-fallbacks)combatant.bloodiedevent emitted frombroadcast_state_changes(), gated on friendly characters below 50% HPparty_reconciliation.reconciledevent fires whenreconcile()completes a mergeprogression.stat_scaledevent for all three scaling functions (level_to_hp,level_to_damage,level_to_defense)Implementation Details
Rework History
Initial approach (reviews #1–#2): Instrumented
combatant.bloodiedinsideCombatant::hp_fraction()default trait method.Rejection root cause: The method had zero non-test production callers —
state.rs::lowest_friendly_hp_ratio()andbroadcast_state_changes()both inline the fraction math instead of delegating.Final approach (rework #3, Architect Option A): Relocated
combatant.bloodiedemission tostate::broadcast_state_changes()where the actual state transition is observed. Revertedhp_fraction()to a pure accessor. This restores the OTEL pattern: "emit at mutation sites, not inside pure accessors."Test Status
broadcast_state_changes()→ OTEL channel (not source-grep)Design Deviations
TEA (test design)
Combatant trait instrumented at the `hp_fraction()` default method instead of at production call sites → ✗ FLAGGED by Reviewer: this deviation is the root cause of the rejection. TEA's reasoning was internally consistent, but the assumption that production state-build code uses `hp_fraction()` was never verified. It doesn't. `state.rs::lowest_friendly_hp_ratio()` (line 402) and `broadcast_state_changes()` (lines 797-798, 890-891) all call `Combatant::hp()` and `Combatant::max_hp()` directly and inline the fraction math. The instrumentation TEA placed in `hp_fraction()` is decorative — it's reachable from tests but not from any production code path. The wiring assertion TEA wrote checked for `Combatant::hp(` strings (which exist in state.rs for unrelated reasons), giving false confidence. Rework required: either move the instrumentation to where bloodied is actually computed (`state.rs:402`), or change the production code to delegate to `hp_fraction()`. The latter is a 2-line fix and is the recommended path.
minor→ HIGH (escalated by Reviewer) — the assumption that callers go through `hp_fraction()` is false in the existing codebase, making the instrumentation unreachable.Single `progression.stat_scaled` event for all three scaling functions instead of three separate events → ✓ ACCEPTED by Reviewer: matches the 35-9 precedent of grouping related sub-decisions under one event component with a discriminant field. All three scaling functions are correctly distinguished by the `stat` field.
`xp_for_level()` is NOT instrumented → ✓ ACCEPTED by Reviewer: correct call. `xp_for_level()` fires on every action via the threshold check — instrumenting it would flood the watcher channel.
`consequence.wish_evaluated` fires on BOTH power-grab true and power-grab false → ✓ ACCEPTED by Reviewer: textbook application of the no-silent-fallbacks rule.
Dev (implementation)
Forward Impact
Fixes MSSCI-35. Approved by Reviewer in re-review #3 with final commit d277562 (all tests GREEN).