settled-reach/docs/workshops/test-architecture/tyre-round2.md

title, description, type, status, workshop, agent, round, created

title	description	type	status	workshop	agent	round	created
Tyre — Round 2 Analysis: Test Client Binary, Cross-Review, Open Questions	Tyre's test client binary architecture, cross-review of all round 1 outputs, and open question resolution	workshop	archived	test-architecture	tyre	2	2026-02-17

Tyre — Round 2 Analysis: Test Client Binary, Cross-Review, Open Questions

Workshop: QA Strategy & Test Architecture Round: 2 (Cross-review and integration) Date: 2026-02-17 Inputs: Stig Round 1 (32 client tests, text renderer), Dudley Round 1 (Gauntlet, determinism, replay, serialization), Hoshe Round 1 (boundary values, Layer 3, CI), Justine Round 1 (CI pipeline, perf regression, golden files)

---

1. Test Client Binary Architecture

The lead overruled my Round 1 recommendation of a server-side text renderer. The reasoning is sound: a separate binary avoids server bloat, keeps the testing landscape clean, and — critically — exercises the real TCP boundary. I argued against it on simplicity grounds, but the architectural purity argument wins. Let me design it properly.

Where It Lives

server/src/bin/test_client.rs — a second binary target in the server crate.

Add to server/Cargo.toml:

[[bin]]
name = "settled-reach-test-client"
path = "src/bin/test_client.rs"

Rationale for same crate, separate binary:

Option	Pros	Cons
server/src/bin/test_client.rs	Shares bridge types + framing code. Zero new crate management. Built with cargo build --bin settled-reach-test-client.	"In the server crate" may look like server bloat at first glance.
New workspace crate test-client/	Clean separation. Can't accidentally use server internals.	New crate boilerplate, workspace config, dependency management. Types need to be either duplicated or extracted to a shared crate.
tooling/test-client/	Consistent with project layout.	Same dependency issues as workspace crate. Not under cargo workspace.

The second binary target wins because:

1. Shared types are the point. The test client needs ObserverSnapshot, PlayerInput, read_framed, write_framed — all defined in the server crate's library. A separate crate would need to depend on the server crate anyway (or we'd extract a protocol crate, which is premature).
2. No server bloat. Rust compiles each binary target independently. Code in test_client.rs doesn't end up in the settled-reach-server binary. Dead code elimination handles shared library code that only the test client calls.
3. Build integration. cargo build builds both binaries. cargo test runs tests in both. cargo nextest picks up integration tests that use either binary.

How It Connects

┌────────────────────┐     TCP      ┌────────────────────────┐
│  settled-reach-     │◄───────────►│  settled-reach-test-    │
│  server             │  framed     │  client                 │
│                     │  msgpack    │                         │
│  --test-mode        │             │  --connect host:port    │
│  --port 0           │             │  --text (stdout render) │
│  --seed 42          │             │  --ticks 50             │
└────────────────────┘             │  --replay inputs.jsonl  │
                                    │  --golden tick50.json   │
                                    └────────────────────────┘

Connection protocol:

1. Test client connects to server's TCP port
2. Each tick: client sends Vec<PlayerInput> via write_framed, server responds with ObserverSnapshot via send_bridge_snapshot
3. Test client deserializes snapshot, formats text output, optionally compares against golden file
4. After --ticks N, client disconnects cleanly

CLI interface:

settled-reach-test-client [OPTIONS]

Connection:
  --connect <host:port>     Connect to running server (default: 127.0.0.1:9876)

Input:
  --replay <file.jsonl>     Send inputs from file (one JSON PlayerInput per line)
  --interactive             Read inputs from stdin (future: human-in-the-loop testing)

Output:
  --text                    Render each snapshot as structured text to stdout
  --json                    Dump each snapshot as JSON to stdout (for golden files)
  --quiet                   No output (assertions only, for CI)

Assertions:
  --golden <file.json>      Compare final snapshot against golden file, exit 1 on diff
  --ticks <N>               Disconnect after N ticks (default: unlimited)

Debugging:
  --dump-raw                Hex dump raw MessagePack bytes before deserializing

Text Output Format

=== Tick 42 | Player (15,10) facing East | Stance: Walk | TickRate: Full ===
Game time: Day 0, 04:12 (Morning)
Entities (5):
  npc:100     (18,10) Forward  rel:Neutral     vis:Visible
  npc:101     (20,10) Forward  rel:Unknown     vis:Remembered
  obj:200     (16,9)  Forward  rel:n/a         vis:Visible
  npc:102     (12,8)  Periph   rel:Hostile      vis:Visible
  npc:103     (22,14) Periph   rel:Friendly     vis:Visible
Tiles: 31 visible
Pending recognitions: 1 [npc:104 at (19,12) 3/8 ticks]
Interactions (2):
  npc:100 [Talk(1), ExamineNpc(2)] distance=3
  obj:200 [Read(1), Observe(2)] distance=1
Inventory: 2/9 [item:300(slot-0), item:301(slot-3)]
Monologue: "Something about this manifest doesn't add up."
===

Format rules:

• Entities identified by kind:entity_id (e.g., npc:100, obj:200). No display names on the wire — see OQ-5 answer below.
• One line per entity, sorted by distance from player (nearest first).
• === tick separators for clean diff output.
• Positions as integer tile coords (the f32 render offset is irrelevant for testing).
• All fields directly from ObserverSnapshot — no additional server data needed.

Text Renderer Implementation

// server/src/bridge/text_renderer.rs (library code, used by test client binary)

/// Format an ObserverSnapshot as structured text for human verification.
/// The test client binary calls this; the server binary does not.
pub fn format_snapshot_text(snapshot: &ObserverSnapshot) -> String {
    let mut out = String::with_capacity(2048);
    // Header
    writeln!(out, "=== Tick {} | Player ({},{}) facing {:?} | Stance: {:?} | TickRate: {:?} ===",
        snapshot.tick,
        snapshot.entities.iter().find(|e| matches!(e.kind, EntityKind::Player))
            .map(|p| p.x as i32).unwrap_or(-1),
        snapshot.entities.iter().find(|e| matches!(e.kind, EntityKind::Player))
            .map(|p| p.y as i32).unwrap_or(-1),
        snapshot.player_facing,
        snapshot.player_stance,
        snapshot.game_time.tick_rate,
    ).ok();
    // ... entities, tiles, interactions, inventory, monologue
    writeln!(out, "===").ok();
    out
}

This lives in the server crate's library (not in any binary). The test client binary calls it. Integration tests can also call it for debug output. The server binary never references it — zero bloat.

Integration with cargo nextest (Layer 3)

The test client binary enables proper Layer 3 subprocess testing. Integration tests spawn both binaries:

// server/tests/layer3_subprocess.rs

#[test]
#[ignore] // Slow — nightly/pre-merge only
fn server_and_test_client_subprocess_roundtrip() {
    // 1. Build both binaries
    // 2. Launch server: settled-reach-server --test-mode --port 0 --seed 42
    // 3. Parse port from server stdout
    // 4. Launch test client: settled-reach-test-client --connect 127.0.0.1:{port}
    //        --replay gauntlet_basic.jsonl --ticks 10 --golden tick10.json --quiet
    // 5. Test client exits 0 if golden file matches, 1 if not
    // 6. Kill server, assert clean exit
}

This is the test that would have caught Bug #1 (blocking TCP read stalling bevy Update). The test client is a real subprocess exercising the real wire protocol — not an in-process mock.

Difficulty Tier

Feasible. ~2-3 days total.

• Binary scaffolding + CLI parsing: 0.5 day
• Text renderer function: 0.5-1 day
• Replay file loading + tick-scheduled sending: 0.5 day
• Golden file comparison (JSON deserialize + field diff): 0.5 day
• Layer 3 integration test wiring: 0.5 day

---

2. Stig's 32 Client Test Proposals — Architectural Validation

Stig proposed 32 tests across 6 categories. All structural scene tree assertions, no pixel comparison. Let me evaluate each category for soundness, redundancies, and gaps.

Camera System (7 tests) — APPROVED

All 7 are architecturally sound:

• Position after ready, anchored flag, smoothing lifecycle, follows movement, static during pause, handles rapid snapshots

No redundancies. Each tests a distinct camera behavior. The "static during pause" test directly prevents Bug #2 class regressions (camera doesn't update when no snapshots arrive).

One refinement: The "handles rapid snapshots" test should verify that the camera smoothing doesn't overshoot or oscillate when snapshots arrive faster than the lerp completes. Assert convergence within a frame budget, not just "doesn't crash."

Entity Rendering (7 tests) — APPROVED with note

• Peripheral alpha == 0.5, forward alpha == 1.0, D-033 colors by kind, lifecycle (create/remove)

Sound. These directly test the information-to-visual mapping that is the client's core responsibility.

Refinement: The lifecycle test should explicitly verify that entities leaving LOS are removed from the scene tree (not just hidden). Godot nodes that are hidden but not freed accumulate memory. Assert get_node_or_null() returns null after LOS exit, not just visible == false.

Z-Layer Ordering (4 tests) — APPROVED, low priority

• Fog rect z=900, fog entities z=950, insert canvas=10, UI canvas=20

These are effectively constant checks. They verify the z-layer conventions documented in the rendering architecture. Worth having as a safety net, but low regression risk — z-layer values don't change accidentally.

No redundancy with entity rendering tests. Z-layers are about ordering between categories (fog vs entities vs UI), not about individual entity rendering.

Fog Shader State (3 tests) — APPROVED, P1

• Visibility texture updates from positions, exploration persistence (255 -> 128), peripheral dimming (180)

Critical for information boundaries. The fog system is the primary visual enforcement of D-010's asymmetric information principle. If fog breaks, the player sees things they shouldn't.

The byte values (255/180/128/0) should be named constants in the fog shader code. Stig's open question to Hoshe is correct — these are assertion targets and should be documented as such. Recommend: const FOG_VISIBLE: int = 255, const FOG_PERIPHERAL: int = 180, const FOG_EXPLORED: int = 128, const FOG_UNEXPLORED: int = 0.

UI Elements (8 tests) — APPROVED, P0 for monologue

• Monologue consumed once per tick, monologue not lost on overwrite (Bug #5), interaction list, dialogue, inventory grid, stance indicator

Bug #5 regression guard (monologue not lost on overwrite) is the highest-priority client test in the entire suite. This was a real bug that the carry-forward fix addressed, and the test should be the first one written.

Sound overall. The interaction list test should verify sorting order (nearest first, matching server nearby_interactions order).

Entity Lerp (3 tests) — APPROVED

• Target set on update, snap on first appearance, convergence after N frames

Sound. The "snap on first appearance" test is subtle and important — entities should appear at their correct position immediately, not lerp from (0,0). This was a visual glitch pattern in early development.

Summary: Missing Coverage

Stig's 32 tests cover the client's core rendering and UI responsibilities well. Gaps I'd add:

#	Missing Test	Category	Why
33	Pending recognition blob rendering	Entity rendering	Cognitive delay (D-060) renders entities as grey blobs. Test that pending_recognitions entries appear as blobs, not full entities. Bug #7 class prevention.
34	Recognition transition animation	Entity lerp	When recognition completes (entity moves from pending_recognitions to entities), verify visual transition from blob to full entity over ~0.3s.
35	Tick rate HUD indicator	UI elements	GameTime.tick_rate changes should update the HUD. Test Full/Half/Paused display.
36	Inventory full visual state	UI elements	When inventory is 9/9, verify visual feedback (e.g., slot highlight change, "Full" indicator).
37	Sprint interaction suppression	UI elements	During Sprint stance, interaction buffer is suppressed (D-055). Verify interaction list is hidden/empty during Sprint.
38	Entity modulate for Remembered state	Entity rendering	Entities with observation: Remembered should render differently from Visible (e.g., translucent, desaturated). Test the modulate/shader difference.

Total with additions: 38 tests. All structural, all scene tree assertions. No pixel comparison.

Priority Ranking for Implementation

Priority	Tests	Count	Rationale
P0	Monologue not lost on overwrite (Bug #5)	1	Direct regression guard for shipped fix
P0	Camera static during pause (Bug #2)	1	Direct regression guard
P1	Fog shader state (all 3)	3	Information boundary enforcement
P1	Entity lifecycle (create/remove)	1	Memory + correctness
P1	Pending recognition blob	1	D-060 cognitive delay visual
P2	Remaining camera tests (5)	5	Camera behavior suite
P2	Entity alpha + color (4)	4	Visual fidelity
P2	UI elements (remaining 7)	7	HUD and interaction
P3	Z-layer ordering (4)	4	Constants checks
P3	Entity lerp (3)	3	Animation refinement
P3	Added tests 34-38	5	New coverage

---

3. OQ-4: WalkabilityMap.chunks HashMap -> BTreeMap?

Answer: No. Leave it as HashMap.

Dudley asked this because WalkabilityMap.chunks: HashMap<ChunkCoord, ChunkData> could become a determinism hazard if we ever iterate chunks (e.g., for save/load serialization).

Current usage audit (confirmed in Round 1):

• can_move_to(pos) — point lookup via self.chunks.get(&chunk_coord) → safe
• set(pos, walkable) — point insertion via self.chunks.entry(coord).or_insert_with(...) → safe
• WalkabilityMap::new() — constructs with pre-allocated chunks → deterministic (insertion order = grid iteration order, but irrelevant since we never iterate)
• Nowhere in the codebase does any code call .iter(), .keys(), or .values() on chunks

Why not convert preemptively:

1. HashMap is faster for point lookups. The WalkabilityMap is queried in the movement validation hot path (validate_movement calls can_move_to for every mover every tick). HashMap lookup is O(1) amortized vs BTreeMap's O(log N). With N=25 chunks, that's ~1 comparison vs ~5 comparisons. Per-call it's nanoseconds, but it's called hundreds of times per tick during pathfinding.
2. No iteration today, no iteration planned. Save/load is Sprint 10+ scope. When it arrives, the save system should collect chunks into a BTreeMap or sorted Vec for serialization — the WalkabilityMap's internal HashMap doesn't need to change.
3. The determinism hygiene rule covers this. The rule is: "HashMap/HashSet for point lookups only, BTreeMap/BTreeSet where iteration order affects output." WalkabilityMap falls cleanly in the first category.

Action: Document in the determinism hygiene section that WalkabilityMap.chunks is a known-safe HashMap usage. If anyone adds iteration over chunks, they must convert to BTreeMap first.

---

4. OQ-5: Entity display_name — Wire Protocol or Server-Side Lookup?

Answer: Neither for now. Use kind:entity_id labels in the test client.

The lead's hint is correct: since the test client is Rust code in the same crate, it has access to content data. But accessing content data from the test client creates two sources of truth for entity identity (wire ID vs content canonical_id) and requires the test client to load content — which adds variables to the testing landscape (the exact thing the lead wants to avoid).

For the test client text renderer:

Entities are labeled as kind:entity_id, e.g., npc:100, obj:200, player:0. This is:

• Unambiguous: the wire entity_id is the canonical identifier in the snapshot
• Zero-cost: no additional data needed, no content loading
• Stable: doesn't break when content names change
• Sufficient: Gauntlet test assertions use named constants that map to known IDs

// In text renderer
fn entity_label(entity: &VisibleEntity) -> String {
    let kind = match entity.kind {
        EntityKind::Player => "player",
        EntityKind::Npc => "npc",
        EntityKind::Object => "obj",
        EntityKind::Terrain => "terrain",
    };
    format!("{}:{}", kind, entity.entity_id)
}

For test assertions, the Gauntlet coordinate constants provide the human-readable mapping:

// server/src/content/gauntlet/constants.rs
pub const GUARD_1: GauntletEntity = GauntletEntity {
    wire_id: 100,  // Assigned by EntityRegistry in deterministic spawn order
    name: "guard-1",
    position: TilePosition::new(18, 10, 0),
};

Test code reads naturally: assert_entity_visible(&snapshot, GUARD_1.wire_id, GUARD_1.position).

When to add display_name to the wire protocol:

Add display_name: Option<String> to VisibleEntity when the Godot client needs to render entity name labels in the UI (e.g., hovering over an NPC shows their name). That's a gameplay feature (likely Sprint 9-10), not a testing requirement. When it ships, the test client gets it for free.

Why not add it now:

1. The lead explicitly wants to avoid adding variables to the testing landscape. A wire protocol change is a variable.
2. display_name involves information boundary decisions: does the player see the name before recognition completes? Before first conversation? These are gameplay questions, not testing questions.
3. kind:entity_id is honest — it shows exactly what the wire protocol contains, which is what we're testing.

---

5. Cross-Review: Hoshe's CI Tier Proposal

Hoshe proposed three tiers: Commit (lint + content, <2min), PR (build + test + fixtures, <10min), Nightly (Layer 3 + golden + perf, <30min). Justine proposed a compatible pipeline with parallel server/client jobs, fixture artifacts, and performance baselines. The lead confirmed CI is deferred (manual make ci stays), so this is future design validation.

Tier Structure: APPROVED

The three-tier model (Commit / PR / Nightly) is the right design. Every tier has a clear purpose:

Tier	Purpose	Gate?	Hoshe's Budget	My Assessment
Commit	Fast syntax + content feedback	No (advisory)	<2 min	Correct. Lint + content validation catches the most common errors with the lowest cost.
PR	Merge gate — build + test + fixture	Yes (blocks merge)	<10 min	Correct, but budget <15 min — full rebuilds from cache miss take 5-7 min for server alone.
Nightly	Deep integration + regression	No (alerts)	<30 min	Correct. Layer 3 subprocess + golden files + perf belong here.

Specific Feedback

Commit tier — good as-is. make lint-server, make lint-client, make validate-content, make check-fact-ids. Fast, catches the obvious stuff.

PR tier — one addition. Hoshe's fixture staleness check (make fixtures && git diff --exit-code client/tests/fixtures/) is excellent. This catches protocol changes where the developer forgot to regenerate fixtures. Justine's parallel server/client jobs with fixture artifact passing is the correct CI implementation.

Add to PR tier: Content cross-reference validation (the new validate_cross_references() function from the lead's confirmed decisions). This should run after build, before tests. Budget impact: <2s, negligible.

PR tier time budget: Revise from <10 min to <15 min. Rationale:

• Server build (clean cache): ~5-7 min on a self-hosted runner
• Server tests (cargo nextest): ~1-2 min
• Client build: ~30s (Godot project scan)
• Client tests (gdUnit4 headless): ~1-2 min
• Fixture generation + staleness check: ~10s
• Content validation: ~5s
• With caching: ~3-5 min total. Without caching: ~12 min.
• 15 min budget covers the worst case without caching.

Nightly tier — one addition. Add a content scaling test: boot the server with a stress content pack (Crowd Plaza density: 15+ NPCs), tick 100 times, verify no tick exceeds the p95 budget. Hoshe's "Level 5 stress" test from T5-H3 belongs here.

Merge-Blocking Policy — Alignment with Justine

Hoshe and Justine are aligned on the merge policy:

Check	Hoshe	Justine	My Assessment
Test failures	BLOCKER	BLOCKER	Correct
Lint failures	BLOCKER	BLOCKER	Correct
Content validation	BLOCKER	BLOCKER	Correct
Golden file diff	Not explicit	WARNING	Should be WARNING, not blocker. Golden files change legitimately when behavior changes. Require reviewer ack.
Fixture staleness	Not explicit	WARNING	Should be BLOCKER. Stale fixtures mean the client is testing against outdated protocol data. This is a real correctness bug, not a warning.
Performance delta	Not explicit	INFO	Correct — CI variance makes perf unreliable as a gate.

Fixture staleness should be a BLOCKER, not a WARNING. If make fixtures && git diff --exit-code fails, it means the server's wire format changed but the fixtures weren't regenerated. The client tests are running against stale data — any passing client test is a false positive. This is the exact class of bug (protocol mismatch) that the entire serialization testing track is designed to prevent.

CI Readiness When We Wire It Up

Hoshe correctly identified that there is no CI pipeline at all — no .gitea/workflows/. The Gitea instance at git.schweitz.internal supports Gitea Actions (GitHub Actions compatible), and we have self-hosted infrastructure. When the lead greenlights CI:

1. Start with the PR tier only (merge gate). This gives the most value for the least setup.
2. Add Commit tier for fast feedback on feature branches.
3. Add Nightly tier last — it requires the Gauntlet test world to exist first.

Estimated wiring effort: ~1 day for a Gitea Actions workflow that runs make ci. The Makefile targets already exist.

---

Summary: Round 2 Deliverables

#	Deliverable	Status
1	Test client binary architecture (location, CLI, text format, Layer 3 integration)	Complete
2	Stig's 32 client tests validated + 6 additions = 38 total with priority ranking	Complete
3	OQ-4: WalkabilityMap HashMap stays (point-lookup only, no iteration)	Complete
4	OQ-5: No wire protocol change now. Use kind:entity_id labels. Add display_name when client needs name labels.	Complete
5	Hoshe's CI tier proposal cross-reviewed. APPROVED with: 15min PR budget, fixture staleness → BLOCKER, content scaling in nightly.	Complete

Integration Points with Other Round 2 Outputs

• Dudley (Task #10): The test client binary depends on --test-mode + --port 0 server flags (confirmed for Sprint 8). Dudley's server-side test infrastructure and the test client binary are complementary — the server provides the run_gauntlet() headless helper for unit/integration tests, the test client binary provides Layer 3 subprocess testing.
• Stig (Task #11): The 6 additional client tests I proposed should be reviewed by Stig for client-side feasibility. The pending recognition blob test (#33) depends on the cognitive delay visual implementation.
• Hoshe (Task #12): Fixture staleness as BLOCKER (not WARNING) is a cross-review finding. Hoshe should confirm this aligns with the Layer 1/2/3 testing strategy.
• Justine (Task #13): Performance baseline strategy is sound. The self-hosted runner recommendation is critical for stable perf numbers.