Files
settled-reach/docs/architecture/sprint-37-878-audit.md
T
jpmschweitzerandClaude Opus 4.6 b86bb00a55 refactor(simulation): PR #137 review — audit override + regression tests + docs
Addresses Tyre, Hoshe, and lead review comments on PR #137:

- **Audit doc amendment** (Tyre E1 / Hoshe H1 / Lead): add "Lead override
  (2026-04-21)" section at top of docs/architecture/sprint-37-878-audit.md.
  Rewrites the conclusion to "DECISION: STRIP" with the cascade-based
  rationale. Preserves the original audit body as the pre-override record.

- **Regression tests** (Lead 2a-2b / Hoshe H2 / H3): add POSITIVE
  assertions of the new uniform behavior so silent reintroduction fails.
  - `phase2_container_verb_labels_uniform_regardless_of_player_state` —
    two trials (empty KG, POI-bearing KG) assert container verb labels
    equal Phase-1 defaults.
  - `monologue_pool_selection_uniform_no_archetype_key` — two observers
    with divergent MonologueState both draw from OBSERVE_NPC_LINES.

- **Decision record amendments** (Lead 3 / Tyre S2): D-032, D-035, and
  D-057 amended with Phase 6 deferral wording. "Retired pending Phase 6,
  not deferred with scaffolding." Reintroduction gate: a confirmed
  Phase 6 character-model design.

- **types.rs doc fixes** (Tyre S1 / Hoshe H5): StartupMessage protocol-
  flow comment updated to reflect no-version handshake (D-192).
  ObserverSnapshot version-history block grows a "Sprint 37 wire-format
  shifts" section documenting D-192 + #878 schema drops.

- **observer/tests.rs:944 comment** (Hoshe H6): rewritten to cite
  cascade rationale instead of the stale D-032-SUPERSEDED premise.

- **tests/run-atlas-determinism exit** (Hoshe H7): exit 0 when EXIT_CODE=2
  (venv/DB missing = skip, not fail). Preserves skip semantics for
  tests/run-all on machines without the Python venv.

Follow-up tickets filed:
- #895 (server, low): expand check-systems-db-stamp GENERATOR_SOURCES
  to cover gemma_naming.py + naming_core.py (Tyre S3).
- #896 (planning, low): add CLAUDE.md carveout for server wiki writes
  closing coverage gates (Tyre S4 / Hoshe H8).

H4 investigation: v01_integration_playthrough.rs was not the only E2E
handshake→tick→snapshot test; coverage preserved by bridge_ipc.rs,
bridge_tcp.rs, and game_loop.rs (the latter is pre-existing-broken
per #885). No replacement test needed.

1142/1142 lib tests pass. cargo clippy -- -D warnings clean.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-04-22 10:09:35 +02:00

11 KiB
Raw Blame History

Sprint 37 #878 — CharacterArchetype audit (D-032 obsolete claim)

Author: Tyre (architecture) Date: 2026-04-21 (audit); 2026-04-22 (lead override amendment) Ticket: #878 — Audit and purge dead smuggler/detective character enum (D-032 obsolete) Status: CLOSED — lead override 2026-04-21: strip stays. See override section below.


Lead override (2026-04-21)

Decision: STRIP the full CharacterArchetype trace from the server. The original audit (below) recommended Option A (no-op, docs-only) on the basis that grep identified five load-bearing consumers. The lead reframed the live-vs-filler determination:

"It is live because we have not scrapped the system loading it in the client. This is not live gameplay. Only the character creation elements and the insert screens are actual production code. The rest is uncleaned filler — per the cascade, we don't deal with character and NPC instructions." — Jeroen, 2026-04-21

Rationale — the cascade framing. Per CLAUDE.md, development follows a strict six-phase cascade. Current focus is Phase 1 (wiki content). Character model, NPC differentiation, verb-label relabeling, and character-keyed monologue pools all belong to Phase 6 (detail coloring). Code that executes at runtime is not automatically production — if the topic belongs to a later cascade phase, it is pre-cascade filler regardless of how deeply it is wired in.

The audit's identification of archetype_verb_label as "live feature code" was mechanically correct: the code runs, produces output, and is observed on the wire. But the feature itself is premature. Container verb differentiation (Smuggler Move/Stash vs Detective Scan/Flag) and archetype-keyed monologue pools are Phase 6 detail, not Phase 13 scaffolding. Uniform verb labels and a single monologue pool are the intended end state until a Phase 6 character system is designed — not a regression.

Audit correction. The audit's TL;DR conclusion ("v0.2 dropped reverses D-117 which reverses D-032-obsolete, therefore keep the enum") was the wrong frame. The supersession chain collapsed — yes — but the correct reading is that all three decisions sit below the current cascade floor, so the enum is cruft on cascade grounds independent of the D-117 revocation. The audit should have consulted the cascade phase before grep-counting consumers; that is the non-obvious precedent captured in memory (feedback_running_code_not_production.md).

Scope of the strip (Sprint 37, #878, merged via PR #137). Commit cae3d3ab — "refactor(simulation): strip archetype trace + HeritageRoot per cascade (#877, #878)" — is the authoritative file list; the summary below is the high-signal view:

  • server/src/bridge/types.rsCharacterArchetype enum + StartupMessage.character_archetype field removed. Protocol break rides the PROTOCOL_VERSION drop in #874 (D-192), co-shipped in the same PR.
  • server/src/perception/observer/mod.rsapply_phase2_verb_filter loses its archetype parameter; archetype_verb_label helper + the container-relabeling block deleted.
  • server/src/simulation/monologue.rsMonologueState.character field + Default value removed. Pool selection is now uniform.
  • server/src/simulation/examine.rsgenerate_examine_text collapses two archetype-specific branches into a single detective-style frame.
  • server/src/test_world/mod.rs + server/src/main.rssetup_gauntlet and setup_proof_room drop the archetype parameter; all internal callsites + the production main loop updated.
  • server/tests/archetype_monologue.rs — deleted (regression guard for #587's archetype→monologue wiring; wiring itself deleted).
  • server/tests/v01_integration_playthrough.rs — deleted (5 archetype-using integration tests; superseded by Gauntlet coverage).
  • server/content/schemas/drama_module.schema.yaml — deleted.
  • server/content/modules/tier1/smuggling_ring_v0_1.yaml — deleted.

Client follow-up: ticket #882"Strip archetype-driven client code (follow-up to #878 server)". The server strip leaves client-side code referencing the removed character_archetype wire field and archetype- keyed palette/monologue branches. Client cleanup preserves the character- creation UI and insert screens (production per the lead call) and strips lattice_profile branching, character.txt session I/O, and protocol decoding of the removed field. Blocked by this PR; cross-referenced in the server task description.

Decision record amendments (2026-04-22):

  • decisions/content.md D-032 [SUPERSEDED] header rewritten to cite the cascade instead of the dropped D-117.
  • decisions/content.md D-035 monologue character enum note updated (was [Obsolete post-D-117], now cites the cascade strip).
  • decisions/perception.md D-057 marked [PARTIALLY SUPERSEDED] with the archetype verb relabeling portion crossed out; vertical-list + Phase 1/2 split preserved.

Regression guards for the new uniform behavior are being added under separate tasks (Hoshe, Sprint 37) — positive assertions that container verb labels and monologue pool selection are archetype-independent, to prevent silent reintroduction.

DECISION: STRIP.

The original audit body below is retained as a historical record of the pre-override analysis. Do not take its recommendation as current.


TL;DR (original audit, SUPERSEDED by lead override above)

The ticket's premise — that CharacterArchetype (Smuggler/Detective) is dead code per D-032's "[Obsolete post-D-117]" footnote — is stale. The footnote relied on D-117 (tycoon is the v0.2 bookmark), but CLAUDE.md now declares "v0.2 target is dropped. No scoping negotiations." That revocation is the most recent architectural directive, and it rolls back the premise that justified marking the enum obsolete.

Recommendation: do not delete CharacterArchetype. Instead, update the decision record to clear the stale obsolete footnote, close the ticket as "no-op — premise superseded," and (optionally) claim a small D-record documenting the reversal chain.

Audit

Grep was run against server/, tooling/, and tests/. CharacterArchetype has five load-bearing consumers plus content-schema users:

1. IPC protocol surface

  • server/src/bridge/types.rs:51-54StartupMessage.character_archetype: CharacterArchetype. Field is serialized into the session handshake. Removing it is a protocol break.
  • server/src/bridge/types.rs:500-518 — enum + as_monologue_key() helper + Default = Detective.
  • Roundtrip tests at lines 1133, 1146, 1157, 1166, 1173, 1184 exercise the field.

2. Observer pipeline (D-057, #422) — real runtime behavior

  • server/src/perception/observer/mod.rs:81,117,138,200 — archetype flows through apply_phase2_verb_filter.
  • server/src/perception/observer/mod.rs:657-760archetype_verb_label() swaps container verb labels based on archetype (Smuggler sees Move/Stash, Detective sees Scan/Flag). This is live feature code, not scaffolding.

3. Monologue pool selection (D-032, #587, #595)

  • server/src/simulation/monologue.rs:134-152MonologueState.character: String is initialized from CharacterArchetype.as_monologue_key() at session start. Pool partition is by string key, but the string is derived from the enum.

4. Gauntlet test-world plumbing

  • server/src/test_world/mod.rs:98pub fn setup_gauntlet(app: &mut App, archetype: CharacterArchetype).
  • 5 internal callsites (lines 702, 725, 742, 761, 778) plus the external archetype_monologue.rs integration suite.

5. Regression test suite

  • server/tests/archetype_monologue.rs — entire file is a regression guard against #587 (archetype→monologue character wiring). Seven tests, four explicitly assert Smuggler vs Detective behavior. Deleting the enum requires deleting this guard, which is the thing that catches the bug it was built for.

6. Content schemas (authoring)

  • server/content/schemas/drama_module.schema.yaml:231,233,234,280,558 — schema enumerates smuggler | detective | any for dialogue/monologue partitioning in drama modules.

Decision chain (why the ticket premise is stale)

D-027 (v0.1 vertical slice = smuggler + detective)
   └─ superseded by D-117 (2026-03-05: tycoon is the v0.2 bookmark)
         └─ superseded by "v0.2 target is dropped" (CLAUDE.md, current)

The obsolete footnote in D-035 line 83 and the [SUPERSEDED] header on D-032 both point at D-117 as the supersession. With v0.2 dropped, we are back to the v0.1 smuggler/detective frame as the implemented base until the 6-phase cascade reaches Phase 4 (Player control) — and even then, the cascade describes a 2-floor test map + character rendering, not a wholesale character-model replacement.

Proposed alternative scope for #878

Three options, cheapest first:

  • Strip the [Obsolete post-D-117] footnote from decisions/content.md:83 (D-035 tag taxonomy).
  • Remove the [SUPERSEDED] marker from decisions/content.md:47 (D-032 header) or add a "supersession reversed" note.
  • Optionally claim a new D-record in decisions/scope.md documenting that the v0.2-dropped directive implicitly reverses D-117's character frame revocation.
  • Zero code changes. Build stays green. 30 minutes.

B. Narrow the ticket to the authoring-side leftovers

  • If there are authoring artifacts (half-written tycoon monologue partitioning, stale schema fields) that were added in anticipation of D-117 and never used, those can be purged. But a quick scan of drama_module.schema.yaml shows the schema is consistent with v0.1 usage.
  • Requires a content team review — server-team scope alone cannot confirm what is live in authoring.

C. Rename without removing (if lead wants distance from v0.1 framing)

  • Rename CharacterArchetypePlayerCharacterRole (or similar) and its variants to preserve behavior while shedding the "smuggler/detective investigation framing" language. Higher risk, touches ~45 files, and doesn't actually change runtime. Not recommended unless the lead specifically wants the naming to match post-cascade vocabulary.

Recommendation (SUPERSEDED — see "Lead override" at top)

Go with Option A. The enum is architecturally sound, the ticket is a casualty of the v0.2→cascade pivot, and the cleanup is documentation- only.

Actual decision: strip (Option D, not enumerated above — full trace purge driven by cascade framing, not by the D-117/v0.2 supersession chain). The audit's grep-count-first methodology was the wrong starting heuristic; cascade phase comes first. See top of document.