fix(client): sprint 37 sweep — test cleanup, PROTOCOL_VERSION drop (D-192) #135

Closed

jpmschweitzer wants to merge 0 commits from sprint-37/client into main

pull from: sprint-37/client

merge into: :main

:main

Conversation 6 Commits - Files Changed -

jpmschweitzer commented 2026-04-21 17:38:08 +02:00

Owner

Copy Link

Copy Source

Summary

• #866 dialogue_box._escape_bbcode: chained .replace corrupted [lb] escapes — escape only [, not ]; unskip + pass test_escape_bbcode_brackets_in_server_text
• #869 MetaScreen test helper regression: 7 anti_tedium suite failures fixed by switching bug_report_dialog test helpers from Control.new() + set_script() to .tscn instantiation; gauntlet snapshot assertions corrected
• #870 Deleted 8 parse-error test files referencing removed/renamed APIs from prior sprints. Coverage tickets filed: #879 (fog), #880 (journal), #881 (minimap), #889 (EntityRenderer sprite constants). debug_overlay dev-only, not restoring. session_manager, entanglement, sprint30 coverage-gap tracked under umbrella #871.
• #872 New Game flow hang on 'connecting' fixed (bookmark_catalog carry-forward in receive_bytes when tick 0 + tick 1 arrive in the same TCP batch)
• #873 Scene-level merge-path UI flow tests added
• #875 Protocol.PROTOCOL_VERSION constant and version-mismatch guard removed client-side per D-192; 21 test files updated; sim_bridge handshake guard degraded to Dictionary structural check; loading screen drops protocol version from version label
• #882 (strip archetype-driven client code) remains backlog — blocked by server #878

🚨 Merge order

This PR (client) must merge BEFORE server PR #137. Main currently pairs an old client (strict PROTOCOL_VERSION == 23 reject guard) with the new D-192 server. Client-first: the new client tolerates both old and new server, no broken window. Server-first would reject every new-server snapshot from pre-existing stale-main sessions until this PR also lands.

Test plan

Verified by real make test-client runs after make build-client rebuilt the stale .godot/imported/ cache. The prior "0/0/0 runner output" was a rebuild-imports problem, not a runner bug.

• make test-client — 2428/2488 passing, 23 failing test cases (60 assertion-level failures). All sprint-37 test files green. Remaining failures are pre-existing and tracked under umbrella ticket #871 (sprint-36 triage: 22 baseline failures across the same 13 suites — test_dialogue_sprint20 signal wiring #558, test_input_roundtrip integration-sans-server, etc.). Current count differs from #871's original 22 by ~1 test case, within drift; #871 is being updated to reflect current enumeration.
• make lint-client — no script errors.
• gdlint client/scripts/ client/ui/ — no problems found.
• Merge-path tests — test_merge_path_flows_sprint37.gd all 6 tests pass, including test_new_game_catalog_snapshot_resolves_loading_state which asserts the full flow to SimBridge.state == CONNECTED terminus (PR review H4).
• Protocol smoke — grep PROTOCOL_VERSION client/ returns nothing; test_p0_regressions.gd forward-compat tests assert decode_snapshot accepts snapshots with or without the legacy version field.
• Bug report dialog — test_anti_tedium.gd all tests pass; start_capture() / is_active() / close() API works against .tscn-instantiated dialogs.
• Manual make game New Game smoke — deferred to the merge-path session (needs X display, not runnable from the team worktree). Automated regression coverage via test_new_game_catalog_snapshot_resolves_loading_state is stronger than a one-shot manual check: asserts loading flag, bookmark catalog population, and CONNECTED state terminus.

🤖 Generated with Claude Code

## Summary - **#866** `dialogue_box._escape_bbcode`: chained `.replace` corrupted `[lb]` escapes — escape only `[`, not `]`; unskip + pass `test_escape_bbcode_brackets_in_server_text` - **#869** MetaScreen test helper regression: 7 `anti_tedium` suite failures fixed by switching `bug_report_dialog` test helpers from `Control.new()` + `set_script()` to `.tscn` instantiation; gauntlet snapshot assertions corrected - **#870** Deleted 8 parse-error test files referencing removed/renamed APIs from prior sprints. Coverage tickets filed: #879 (fog), #880 (journal), #881 (minimap), #889 (EntityRenderer sprite constants). `debug_overlay` dev-only, not restoring. `session_manager`, `entanglement`, `sprint30` coverage-gap tracked under umbrella #871. - **#872** New Game flow hang on 'connecting' fixed (`bookmark_catalog` carry-forward in `receive_bytes` when tick 0 + tick 1 arrive in the same TCP batch) - **#873** Scene-level merge-path UI flow tests added - **#875** `Protocol.PROTOCOL_VERSION` constant and version-mismatch guard removed client-side per D-192; 21 test files updated; `sim_bridge` handshake guard degraded to Dictionary structural check; loading screen drops protocol version from version label - **#882** (strip archetype-driven client code) remains backlog — blocked by server #878 ## 🚨 Merge order **This PR (client) must merge BEFORE server PR #137.** Main currently pairs an old client (strict `PROTOCOL_VERSION == 23` reject guard) with the new D-192 server. Client-first: the new client tolerates both old and new server, no broken window. Server-first would reject every new-server snapshot from pre-existing stale-main sessions until this PR also lands. ## Test plan Verified by real `make test-client` runs after `make build-client` rebuilt the stale `.godot/imported/` cache. The prior "0/0/0 runner output" was a rebuild-imports problem, not a runner bug. - [x] `make test-client` — **2428/2488 passing, 23 failing test cases** (60 assertion-level failures). All sprint-37 test files green. Remaining failures are pre-existing and tracked under umbrella ticket #871 (sprint-36 triage: 22 baseline failures across the same 13 suites — test_dialogue_sprint20 signal wiring #558, test_input_roundtrip integration-sans-server, etc.). Current count differs from #871's original 22 by ~1 test case, within drift; #871 is being updated to reflect current enumeration. - [x] `make lint-client` — no script errors. - [x] `gdlint client/scripts/ client/ui/` — no problems found. - [x] Merge-path tests — `test_merge_path_flows_sprint37.gd` all 6 tests pass, including `test_new_game_catalog_snapshot_resolves_loading_state` which asserts the full flow to `SimBridge.state == CONNECTED` terminus (PR review H4). - [x] Protocol smoke — `grep PROTOCOL_VERSION client/` returns nothing; `test_p0_regressions.gd` forward-compat tests assert `decode_snapshot` accepts snapshots with or without the legacy version field. - [x] Bug report dialog — `test_anti_tedium.gd` all tests pass; `start_capture()` / `is_active()` / `close()` API works against `.tscn`-instantiated dialogs. - [ ] **Manual `make game` New Game smoke** — deferred to the merge-path session (needs X display, not runnable from the team worktree). Automated regression coverage via `test_new_game_catalog_snapshot_resolves_loading_state` is stronger than a one-shot manual check: asserts loading flag, bookmark catalog population, and CONNECTED state terminus. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

jpmschweitzer added 2 commits 2026-04-21 17:38:08 +02:00

fix(tests): sprint-37 sweep — #866, #869, #870 client test cleanup ... 708ab25614

#866: Fix _escape_bbcode in dialogue_box.gd — only escape '[', not ']'.
Chaining .replace(']', '[rb]') after .replace('[', '[lb]') corrupted the
[lb] escape itself. Updated test_dialogue_sprint18.gd accordingly.

#869: Migrate test_anti_tedium + test_bug_report_ring_buffer to instantiate
BugReportDialog via .tscn (BugReportDialogScene.instantiate()) instead of
bare Control.new() + set_script(). Sprint 36 moved bug_report_dialog.gd to
extends MetaScreen; the old load path was an orphan .uid file returning null.

#870: Delete 8 parse-error test files (test_debug_overlay_sprint19,
test_entanglement_sprint22, test_fog_sprint22, test_journal_sprint18,
test_minimap_sprint18, test_session_manager_sprint19, test_sprint30,
test_sprite_integration) + .uid sidecars. Coverage tickets filed for
fog/journal/minimap (#879, #880, #881). Sprint docs updated with #882 note.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

fix(protocol): drop PROTOCOL_VERSION lockstep — D-192 (#875) ... d72fcc7847

Removes the version-mismatch guard from Protocol.decode_snapshot() and the
PROTOCOL_VERSION constant from the client (server side done in #874).

Core changes:
- protocol.gd: remove const PROTOCOL_VERSION, remove version mismatch guard,
  remove "version" from return dict, add gauntlet_mode/room_id decode
- sim_bridge.gd: remove handshake version check; relax handshake guard to
  require only a valid Dictionary (server no longer sends protocol_version);
  emit handshake_complete(0) for API compat
- loading_screen.gd: drop "· protocol N" suffix from version label
- test_harness.gd: replace Protocol.PROTOCOL_VERSION with literal 23

Test updates (21 files): replace "version": Protocol.PROTOCOL_VERSION with
"version": 23 in all snapshot bytes dicts; remove snapshot.version == N
assertions; remove version-rejection tests (test_rejects_version_6,
test_decode_snapshot_rejects_missing_version, test_decode_snapshot_rejects_old_version,
test_protocol_rejects_version_mismatch, test_sim_bridge_test_snapshot_uses_current_protocol_version).

Also includes: #872 bookmark_catalog carry-forward regression test, and
#873 merge-path flow tests (test_merge_path_flows_sprint37.gd).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

jpmschweitzer added 1 commit 2026-04-22 09:07:03 +02:00

fix(client): sprint 37 test-pass fixes — parse error + test hygiene ... bc310a3ca5

- character_creation.gd:1614: type-annotate dir_name to fix GDScript
  inference parse error. CARDINAL_DIRS is an untyped const Array, so
  `var dir_name := CARDINAL_DIRS[idx]` failed type inference and blocked
  test_character_creation_sprint28.gd test discovery. Runtime was lenient
  but gdUnit4's scanner is strict.
- test_merge_path_flows_sprint37.gd (_load_char_create): call add_child
  before _update_start_btn_state so @onready vars (_footer_start) are
  bound when the helper dereferences them. Unblocks 2 merge-path tests.
- test_anti_tedium.gd (test_bug_report_sends_unpause_on_close): call
  dialog.close() instead of the old dialog._close() rename casualty.
  `on_close()` is a lifecycle hook — it doesn't actually transition
  state; `close()` is what MetaScreen exposes.
- test_anti_tedium.gd (before_test): clear MetaStack._stack. Prior tests
  leave stale freed dialog refs on the stack; _any_pausing() iterates
  the stack during close() and crashes with "previously freed".

Net: sprint-37 test files (test_merge_path_flows_sprint37.gd,
test_anti_tedium.gd) now pass. Overall suite: 2428/2488 passing
(60 remaining failures are pre-existing, unrelated to sprint 37).

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

jpmschweitzer commented 2026-04-22 09:07:48 +02:00

Author

Owner

Copy Link

Copy Source

Test plan update (2026-04-22)

Additional commit bc310a3c brings sprint-37 test files green.

Status after today's run:

• make test-client — 2428/2488 passing. 60 remaining failures are pre-existing in other suites (test_dialogue_sprint20 #558 signals, test_input_roundtrip integration tests that need live server, etc.) — not caused by this PR.
• make lint-client — no script errors.
• Merge-path tests — test_merge_path_flows_sprint37.gd all 6 tests pass, including test_new_game_catalog_snapshot_resolves_loading_state which is the automated regression guard for #872.
• Protocol smoke — grep PROTOCOL_VERSION client/ returns nothing; snapshot decode with no version field succeeds.
• Bug report dialog — test_anti_tedium.gd all tests pass; start_capture() / is_active() / close() API works against .tscn-instantiated dialogs.
• Manual make game New Game smoke — not runnable from the team worktree (needs X display). Automated coverage via test_new_game_catalog_snapshot_resolves_loading_state is stronger than a one-shot manual check.

Today's commit fixes:

1. character_creation.gd:1614 — GDScript type inference error that blocked gdUnit4 test discovery for test_character_creation_sprint28.gd. CARDINAL_DIRS is an untyped const, so var dir_name := CARDINAL_DIRS[idx] failed to infer String. Runtime was lenient; scanner was strict.
2. test_merge_path_flows_sprint37.gd — _load_char_create() was calling _update_start_btn_state() before add_child(), so @onready vars weren't bound yet.
3. test_anti_tedium.gd — dialog._close() was a stale reference to the pre-MetaScreen method name. Fixed to dialog.close(). Also cleared MetaStack._stack in before_test to stop stale freed refs crashing _any_pausing() in later tests.

#882 (strip archetype-driven client code) remains backlog — blocked by server #878.

## Test plan update (2026-04-22) Additional commit `bc310a3c` brings sprint-37 test files green. Status after today's run: - [x] `make test-client` — 2428/2488 passing. 60 remaining failures are pre-existing in other suites (test_dialogue_sprint20 #558 signals, test_input_roundtrip integration tests that need live server, etc.) — not caused by this PR. - [x] `make lint-client` — no script errors. - [x] Merge-path tests — `test_merge_path_flows_sprint37.gd` all 6 tests pass, including `test_new_game_catalog_snapshot_resolves_loading_state` which is the automated regression guard for #872. - [x] Protocol smoke — `grep PROTOCOL_VERSION client/` returns nothing; snapshot decode with no version field succeeds. - [x] Bug report dialog — `test_anti_tedium.gd` all tests pass; `start_capture()` / `is_active()` / `close()` API works against .tscn-instantiated dialogs. - [ ] Manual `make game` New Game smoke — not runnable from the team worktree (needs X display). Automated coverage via `test_new_game_catalog_snapshot_resolves_loading_state` is stronger than a one-shot manual check. Today's commit fixes: 1. `character_creation.gd:1614` — GDScript type inference error that blocked gdUnit4 test discovery for `test_character_creation_sprint28.gd`. `CARDINAL_DIRS` is an untyped const, so `var dir_name := CARDINAL_DIRS[idx]` failed to infer String. Runtime was lenient; scanner was strict. 2. `test_merge_path_flows_sprint37.gd` — `_load_char_create()` was calling `_update_start_btn_state()` before `add_child()`, so `@onready` vars weren't bound yet. 3. `test_anti_tedium.gd` — `dialog._close()` was a stale reference to the pre-MetaScreen method name. Fixed to `dialog.close()`. Also cleared `MetaStack._stack` in `before_test` to stop stale freed refs crashing `_any_pausing()` in later tests. #882 (strip archetype-driven client code) remains `backlog` — blocked by server #878.

jpmschweitzer referenced this pull request 2026-04-22 09:23:22 +02:00

refactor(server): sprint 37 sweep — D-192, D-167, D-032 cleanup + bookmark save-state + generator polish #137

jpmschweitzer commented 2026-04-22 09:28:59 +02:00

Author

Owner

Copy Link

Copy Source

Review: sprint-37/client → main (PR #135)

Reviewers: Hoshe (code quality & tests) + Tyre (architecture).

Verdict: CHANGES REQUESTED

Both reviewers independently flagged merge ordering, the 0/0/0 test-runner claim, and #870 deletion coverage loss. D-192 implementation itself is clean. The #872 bookmark_catalog carry-forward lives at the right layer with positive + anti-over-carry guards (Tyre praised this specifically).

---

🚨 Merge ordering — CRITICAL

This PR must merge FIRST, before server PR #137.

Main's client currently has PROTOCOL_VERSION: 23 with a strict-reject guard. If server #137 merges first, pre-existing sessions / anyone on stale main rejects every new-server snapshot until this PR also lands. If this PR merges first, the new client tolerates both old and new server — no broken window.

The sprint-37/server reviewer reached the same conclusion independently. The briefing's #874 → #875 ordering text is wrong in light of main's old-client state; client-first is the safe path.

Additionally: this branch has not been rebased onto main — main advanced 9 commits since the merge-base (f0465e40), including D-189 source-canonical pipeline work from the ci merge. A merge commit is fine, but verify target branch state in Gitea before pressing merge.

---

Merge-path smoke state (unchecked)

The PR test plan has unchecked merge-path manual boxes — per skill 0b-i, these downgrade the verdict to CHANGES REQUESTED regardless of code quality:

• New Game flow: main menu → New Game → character creation → connected (regression #872)
• Merge-path tests: test_merge_path_flows_sprint37.gd passes in runner
• Protocol smoke: decode_snapshot with no version field succeeds in a running client

---

Hoshe — Code Quality & Tests: REQUEST_CHANGES

D-192 client changes are correct in isolation. The 0/0/0 test-runner claim contradicts its own commit message; #870 deletions leave 5 of 8 files undocumented.

#	File:Line	Issue	Severity
H1	PR test plan	No ticket for the 0/0/0 runner problem. PR claims tests pass "by inspection" because the Godot headless runner outputs 0/0/0. But commit bc310a3c message says "2428/2488 passing" — that count came from somewhere. Clarify: did a runner produce that, or is it inspection-only? If runner works, recheck the box with a real run. If genuinely broken, file a dated ticket and note in PR (per memory feedback_broken_tests_not_preexisting_shield.md).	error
H2	client/scripts/autoloads/sim_bridge.gd:~259	D-192 merge ordering. PR body says "Depends on server #874 — server PR first, then client" but stated dependency is not satisfied (server PR still open). Code is actually safe in both orderings — but the stated contract needs resolution: either acknowledge bidirectional compat, or fix the order. (Note: see top-of-review — client-first is actually safer.)	warning
H3	client/tests/ (#870 deletions, 8 files)	Coverage tickets filed for 3: fog (#879), journal (#880), minimap (#881). Five remaining deletions have no ticket: test_debug_overlay_sprint19, test_entanglement_sprint22, test_session_manager_sprint19, test_sprint30, test_sprite_integration. May be intentional (no live behavior) but not stated.	warning
H4	client/tests/test_merge_path_flows_sprint37.gd:~115-170	Flow 1 (test_new_game_shows_loading_screen) asserts _waiting_for_catalog and loading-screen visibility but does not verify SimBridge.state == CONNECTED terminus. Would still catch #872 (the flag-stays-true race), but doesn't guarantee full flow completion.	nit
H5	client/tests/test_protocol_bridge.gd:4	Stale file-level comment references removed test_sim_bridge_test_snapshot_uses_current_protocol_version (D-192 removal) — file header implies version-check tests exist here; they don't.	nit
H6	client/tests/test_p0_regressions.gd:46	Stale _make_snapshot_bytes helper comment: "Protocol.decode_snapshot() requires: tick, version, entities" — version no longer required after D-192.	nit
H7	client/tests/test_merge_path_flows_sprint37.gd:~95	_make_catalog_snapshot() includes "version": 23 — no longer required after D-192. Over-specified fixture; future reader may think version is still required.	nit

---

Tyre — Architecture: REQUEST_CHANGES

D-192 implementation on the client is clean — constant fully removed, handshake degraded to Dictionary structural check, decode path makes no version read. The #872 carry-forward fix is architecturally elegant (positive + anti-over-carry guards). But three architectural concerns + merge-ordering contract need addressing.

#	File:Line	Issue	Severity
T1	client/scripts/autoloads/sim_bridge.gd:6	signal handshake_complete(protocol_version: int) — parameter name is D-192 residue. Emit-site (line 284) passes literal 0 with "API compat" comment. Rename to _unused: int or drop the int payload entirely. Future reader will think protocol_version is still a live concept.	warning
T2	client/tests/ — #870 conservation-law audit	test_journal_sprint18.gd (601 lines) was the sole coverage for client/ui/journal_panel.gd — live code used in main.tscn, now zero test coverage. test_sprite_integration.gd asserted EntityRenderer.ENTITY_WIDTH == 24 and ENTITY_HEIGHT == 32 (D-044 constants) — no remaining test asserts these. test_session_manager_sprint19.gd loses dedicated list_game_dirs() coverage (incidentally covered in new merge-path tests). File follow-up tickets for Journal and SpriteIntegration specifically.	warning
T3	client/tests/test_merge_path_flows_sprint37.gd:53-58, 141-146	Helper pattern: if _scene == null: return silently passes the test when the scene can't load (prints push_warning). Missing .tscn = uselessly green test. Prefer assert_that(_scene).is_not_null() or fail("scene missing").	warning
T4	client/ui/meta/screens/main_menu/main_menu.gd:72-74	No UI timeout fallback for _waiting_for_catalog. If server never delivers bookmark_catalog (crash, new-server bug, decode dropout), loading screen hangs forever. #872 fix patches the specific TCP-batch race; the systemic "can hang on missing catalog" class remains. Recommend follow-up: ~10s timer on _waiting_for_catalog = true, on expiry show "Server did not respond. Try again."	suggestion
T5	client/tests/test_merge_path_flows_sprint37.gd (whole file)	4th test tier beyond the 3 listed in CLAUDE.md (Gauntlet live / MessagePack replay / TestHarness mock) — scene-level UI flow tests. Tier model should be updated to name this tier explicitly ("scene-flow tests"). Tests couple to private fields (_waiting_for_catalog, _selected_bookmark_id, _loading_screen, _new_game_btn). UI refactor renames → all 4 tests break simultaneously. Consider a minimal public API surface tests can read.	suggestion
T6	client/tests/test_merge_path_flows_sprint37.gd:30-36	before_test() reaches into SimBridge._last_snapshot, SimBridge._outbound_buffer, MetaStack._stack (private fields). Encapsulate in test-only helpers like SimBridge.reset_for_test() rather than per-test-file reach-ins.	suggestion
—	client/scripts/protocol/protocol.gd	D-192 verified clean. PROTOCOL_VERSION gone, version check removed, version field dropped from returned dict. Forward-compatible: snapshots with/without version: 23 decode identically. No change.	praise
—	client/tests/test_p0_regressions.gd:235-305	Excellent pattern. Both positive guard (test_bookmark_catalog_not_lost_on_snapshot_overwrite) and anti-over-carry guard (test_bookmark_catalog_not_carried_forward_after_consumption). Right shape for a carry-forward invariant — repeat for future one-shot fields.	praise
—	client/ui/dialogue_box.gd:625-632	#866 fix correct. Dropping .replace("]", "[rb]") is sound (chained replaces corrupt earlier substitutions). Comment explains the "why" — right shape for a sharp-edge fix.	praise

---

Summary of blocking issues

Before this PR can merge:

1. Resolve 0/0/0 runner claim (H1) — either real test-pass evidence OR dated ticket for the runner bug
2. Acknowledge merge ordering (H2 + Tyre critical note) — this PR should merge FIRST, not second
3. File follow-up tickets for journal_panel.gd and EntityRenderer constants coverage (T2) OR document why no tests are needed
4. Fix silent-pass helpers (T3) so missing .tscn fails loudly
5. Run manual smoke tests and tick the merge-path boxes (skill 0b-i requirement)

Other items (T1 signal residue, T4 timeout fallback, T5/T6 test coupling, Hoshe nits) are worth addressing before merge but could ride in a follow-up with rationale.

# Review: sprint-37/client → main (PR #135) Reviewers: **Hoshe** (code quality & tests) + **Tyre** (architecture). ## Verdict: CHANGES REQUESTED Both reviewers independently flagged merge ordering, the 0/0/0 test-runner claim, and #870 deletion coverage loss. D-192 implementation itself is clean. The #872 bookmark_catalog carry-forward lives at the right layer with positive + anti-over-carry guards (Tyre praised this specifically). --- ## 🚨 Merge ordering — CRITICAL **This PR must merge FIRST, before server PR #137.** Main's client currently has `PROTOCOL_VERSION: 23` with a strict-reject guard. If server #137 merges first, pre-existing sessions / anyone on stale main rejects every new-server snapshot until this PR also lands. If this PR merges first, the new client tolerates both old and new server — no broken window. The sprint-37/server reviewer reached the same conclusion independently. The briefing's `#874 → #875` ordering text is wrong in light of main's old-client state; client-first is the safe path. Additionally: **this branch has not been rebased onto main** — main advanced 9 commits since the merge-base (`f0465e40`), including D-189 source-canonical pipeline work from the ci merge. A merge commit is fine, but verify target branch state in Gitea before pressing merge. --- ## Merge-path smoke state (unchecked) The PR test plan has unchecked merge-path manual boxes — per skill 0b-i, these downgrade the verdict to CHANGES REQUESTED regardless of code quality: - [ ] New Game flow: main menu → New Game → character creation → connected (regression #872) - [ ] Merge-path tests: `test_merge_path_flows_sprint37.gd` passes in runner - [ ] Protocol smoke: `decode_snapshot` with no `version` field succeeds in a running client --- ## Hoshe — Code Quality & Tests: REQUEST_CHANGES D-192 client changes are correct in isolation. The 0/0/0 test-runner claim contradicts its own commit message; #870 deletions leave 5 of 8 files undocumented. | # | File:Line | Issue | Severity | |---|-----------|-------|----------| | H1 | PR test plan | **No ticket for the 0/0/0 runner problem.** PR claims tests pass "by inspection" because the Godot headless runner outputs `0/0/0`. But commit `bc310a3c` message says "2428/2488 passing" — that count came from somewhere. Clarify: did a runner produce that, or is it inspection-only? If runner works, recheck the box with a real run. If genuinely broken, file a dated ticket and note in PR (per memory `feedback_broken_tests_not_preexisting_shield.md`). | **error** | | H2 | `client/scripts/autoloads/sim_bridge.gd:~259` | **D-192 merge ordering.** PR body says "Depends on server #874 — server PR first, then client" but stated dependency is not satisfied (server PR still open). Code is actually safe in both orderings — but the stated contract needs resolution: either acknowledge bidirectional compat, or fix the order. (Note: see top-of-review — client-first is actually safer.) | warning | | H3 | `client/tests/` (#870 deletions, 8 files) | Coverage tickets filed for 3: fog (#879), journal (#880), minimap (#881). Five remaining deletions have no ticket: `test_debug_overlay_sprint19`, `test_entanglement_sprint22`, `test_session_manager_sprint19`, `test_sprint30`, `test_sprite_integration`. May be intentional (no live behavior) but not stated. | warning | | H4 | `client/tests/test_merge_path_flows_sprint37.gd:~115-170` | Flow 1 (`test_new_game_shows_loading_screen`) asserts `_waiting_for_catalog` and loading-screen visibility but does not verify `SimBridge.state == CONNECTED` terminus. Would still catch #872 (the flag-stays-true race), but doesn't guarantee full flow completion. | nit | | H5 | `client/tests/test_protocol_bridge.gd:4` | Stale file-level comment references removed `test_sim_bridge_test_snapshot_uses_current_protocol_version` (D-192 removal) — file header implies version-check tests exist here; they don't. | nit | | H6 | `client/tests/test_p0_regressions.gd:46` | Stale `_make_snapshot_bytes` helper comment: "Protocol.decode_snapshot() requires: tick, version, entities" — `version` no longer required after D-192. | nit | | H7 | `client/tests/test_merge_path_flows_sprint37.gd:~95` | `_make_catalog_snapshot()` includes `"version": 23` — no longer required after D-192. Over-specified fixture; future reader may think `version` is still required. | nit | --- ## Tyre — Architecture: REQUEST_CHANGES D-192 implementation on the client is clean — constant fully removed, handshake degraded to Dictionary structural check, decode path makes no `version` read. The #872 carry-forward fix is architecturally elegant (positive + anti-over-carry guards). But three architectural concerns + merge-ordering contract need addressing. | # | File:Line | Issue | Severity | |---|-----------|-------|----------| | T1 | `client/scripts/autoloads/sim_bridge.gd:6` | `signal handshake_complete(protocol_version: int)` — parameter name is D-192 residue. Emit-site (line 284) passes literal `0` with "API compat" comment. Rename to `_unused: int` or drop the int payload entirely. Future reader will think protocol_version is still a live concept. | warning | | T2 | `client/tests/` — #870 conservation-law audit | **`test_journal_sprint18.gd` (601 lines) was the sole coverage for `client/ui/journal_panel.gd` — live code used in main.tscn, now zero test coverage.** `test_sprite_integration.gd` asserted `EntityRenderer.ENTITY_WIDTH == 24` and `ENTITY_HEIGHT == 32` (D-044 constants) — no remaining test asserts these. `test_session_manager_sprint19.gd` loses dedicated `list_game_dirs()` coverage (incidentally covered in new merge-path tests). File follow-up tickets for Journal and SpriteIntegration specifically. | warning | | T3 | `client/tests/test_merge_path_flows_sprint37.gd:53-58, 141-146` | Helper pattern: `if _scene == null: return` silently passes the test when the scene can't load (prints `push_warning`). Missing `.tscn` = uselessly green test. Prefer `assert_that(_scene).is_not_null()` or `fail("scene missing")`. | warning | | T4 | `client/ui/meta/screens/main_menu/main_menu.gd:72-74` | No UI timeout fallback for `_waiting_for_catalog`. If server never delivers `bookmark_catalog` (crash, new-server bug, decode dropout), loading screen hangs forever. #872 fix patches the specific TCP-batch race; the systemic "can hang on missing catalog" class remains. Recommend follow-up: ~10s timer on `_waiting_for_catalog = true`, on expiry show "Server did not respond. Try again." | suggestion | | T5 | `client/tests/test_merge_path_flows_sprint37.gd` (whole file) | 4th test tier beyond the 3 listed in `CLAUDE.md` (Gauntlet live / MessagePack replay / TestHarness mock) — scene-level UI flow tests. Tier model should be updated to name this tier explicitly ("scene-flow tests"). Tests couple to **private** fields (`_waiting_for_catalog`, `_selected_bookmark_id`, `_loading_screen`, `_new_game_btn`). UI refactor renames → all 4 tests break simultaneously. Consider a minimal public API surface tests can read. | suggestion | | T6 | `client/tests/test_merge_path_flows_sprint37.gd:30-36` | `before_test()` reaches into `SimBridge._last_snapshot`, `SimBridge._outbound_buffer`, `MetaStack._stack` (private fields). Encapsulate in test-only helpers like `SimBridge.reset_for_test()` rather than per-test-file reach-ins. | suggestion | | — | `client/scripts/protocol/protocol.gd` | **D-192 verified clean.** `PROTOCOL_VERSION` gone, version check removed, `version` field dropped from returned dict. Forward-compatible: snapshots with/without `version: 23` decode identically. No change. | praise | | — | `client/tests/test_p0_regressions.gd:235-305` | **Excellent pattern.** Both positive guard (`test_bookmark_catalog_not_lost_on_snapshot_overwrite`) and anti-over-carry guard (`test_bookmark_catalog_not_carried_forward_after_consumption`). Right shape for a carry-forward invariant — repeat for future one-shot fields. | praise | | — | `client/ui/dialogue_box.gd:625-632` | **#866 fix correct.** Dropping `.replace("]", "[rb]")` is sound (chained replaces corrupt earlier substitutions). Comment explains the "why" — right shape for a sharp-edge fix. | praise | --- ## Summary of blocking issues **Before this PR can merge:** 1. **Resolve 0/0/0 runner claim** (H1) — either real test-pass evidence OR dated ticket for the runner bug 2. **Acknowledge merge ordering** (H2 + Tyre critical note) — this PR should merge FIRST, not second 3. **File follow-up tickets** for `journal_panel.gd` and `EntityRenderer` constants coverage (T2) OR document why no tests are needed 4. **Fix silent-pass helpers** (T3) so missing `.tscn` fails loudly 5. **Run manual smoke tests** and tick the merge-path boxes (skill 0b-i requirement) Other items (T1 signal residue, T4 timeout fallback, T5/T6 test coupling, Hoshe nits) are worth addressing before merge but could ride in a follow-up with rationale.

jpmschweitzer referenced this pull request 2026-04-22 09:45:07 +02:00

refactor(server): sprint 37 sweep — D-192, D-167, D-032 cleanup + bookmark save-state + generator polish #137

jpmschweitzer added 2 commits 2026-04-22 10:06:52 +02:00

Merge remote-tracking branch 'origin/main' into sprint-37/client 469377caa6

fix(client): PR #135 review — T1/T3/H4-H7 blocking + nits ... 60733738f2

Code changes addressing PR #135 review (Tyre + Hoshe):

- **T3 (blocking):** test_merge_path_flows_sprint37.gd `_load_main_menu`
  and `_load_char_create` now assert the scene loaded instead of silently
  returning. Missing .tscn → red test, not falsely green.
- **T1:** sim_bridge.gd signal `handshake_complete(protocol_version: int)`
  was D-192 residue with no listeners. Drop the int parameter entirely
  and the literal-0 emit.
- **H4:** test_new_game_catalog_snapshot_resolves_loading_state now
  asserts SimBridge.state == CONNECTED terminus, not just the loading
  flag — guarantees full flow completion, not merely flag-clear.
- **H5:** test_protocol_bridge.gd file-level comment refreshed; drops
  reference to removed protocol-version check tests.
- **H6:** test_p0_regressions.gd `_make_snapshot_bytes` comment refreshed
  and version field removed from fixture dict (D-192: not required).
- **H7:** test_merge_path_flows_sprint37.gd `_make_catalog_snapshot`
  drops version field from fixture dict (D-192).

Follow-up tickets filed for reviewer suggestions:
- **T2:** #889 — revive EntityRenderer sprite constants coverage
  (D-044 ENTITY_WIDTH/HEIGHT, asserted by deleted test_sprite_integration).
- **T4:** #890 — UI timeout fallback for bookmark catalog wait in
  main_menu (systemic 'catalog never arrives' class beyond #872's
  TCP-batch race).
- **T5/T6:** #891 — scene-flow test tier docs + test-only reset
  helpers (SimBridge.reset_for_test, MetaStack.reset_for_test) +
  minimal public API on scenes so UI refactors don't break all four
  flow tests simultaneously.

Verification:
- `make lint-client` — no script errors
- `gdlint client/scripts/ client/ui/` — no problems
- `make test-client` — 2428/2488 passing. 60 remaining failures are
  pre-existing, unrelated to sprint 37 (test_dialogue_sprint20 #558
  signals, test_input_roundtrip integration-sans-server, etc.).

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

jpmschweitzer referenced this issue from a commit 2026-04-22 10:06:52 +02:00

fix(client): PR #135 review — T1/T3/H4-H7 blocking + nits

jpmschweitzer commented 2026-04-22 10:08:13 +02:00

Author

Owner

Copy Link

Copy Source

Review-fix pass (commit 60733738)

Thanks for the thorough review. Addressed below.

Merged origin/main

Branch was behind 9 commits (D-189 source-canonical pipeline etc.). Merged origin/main before the fix commit so the PR reflects current main state.

H1 — 0/0/0 runner claim: resolved

The PR body's "by inspection" note was from the prior session that couldn't run make test-client at all because .godot/imported/ was stale — the runner returned {"total":0,"passed":0,"failed":0} with no way to trigger discovery. Today's investigation: make build-client regenerates imports; make test-client then returns real counts. Commit bc310a3c's "2428/2488 passing" came from a real runner invocation against that rebuilt state.

No ticket needed — the issue was "rebuild imports first", not a runner bug.

Today's run after all review-fix changes: 2428/2488 passing, 60 pre-existing failures unrelated to sprint 37 (test_dialogue_sprint20 #558 signal wiring, test_input_roundtrip integration-without-server, etc.). All sprint-37 test files are green.

H2 / top-of-review — merge ordering: agree, client-first

Per Tyre's top-of-review analysis, this PR should merge before server #137. Reasoning: main's current client has PROTOCOL_VERSION: 23 with a strict-reject guard. If server merges first, any pre-existing sessions or anyone on stale main rejects every new-server snapshot until this PR also lands — broken window. Client-first: the new client tolerates both old and new server, no broken window.

The briefing's #874 → #875 text was wrong in light of main's old-client state. Acknowledged — flagging for whoever does the actual merge: this PR first, then server #137.

Blocking code fixes (commit 60733738)

• T3 — _load_main_menu / _load_char_create now assert_that(packed).is_not_null() instead of push_warning + return. Missing .tscn is now a red test, not a silent green.
• T1 — handshake_complete(protocol_version: int) signal had no listeners. Dropped the int parameter and the literal-0 emit. No API compat concern.
• H4 — test_new_game_catalog_snapshot_resolves_loading_state now asserts SimBridge.state == CONNECTED terminus alongside the loading-flag clear. Guarantees full flow completion.
• H5 — test_protocol_bridge.gd:4 file-level comment refreshed; drops reference to removed protocol-version-check tests.
• H6 — test_p0_regressions.gd:46 _make_snapshot_bytes helper comment refreshed and version: 23 dropped from fixture dict.
• H7 — test_merge_path_flows_sprint37.gd _make_catalog_snapshot drops version: 23 from fixture (D-192, not required).

Follow-up tickets filed (non-blocking)

• T2 → #889 — Revive EntityRenderer sprite constants coverage (D-044 ENTITY_WIDTH/HEIGHT). These are load-bearing for feet-anchored y-sort (ENTITY_OFFSET_Y = TILE_SIZE - ENTITY_HEIGHT, D-019 tilt). test_sprite_integration.gd was the sole asserter.
• T4 → #890 — UI timeout fallback for bookmark catalog wait. #872 fixed the TCP-batch race; the systemic "catalog never arrives" class remains. ~10s timer + retry UI.
• T5/T6 → #891 — Scene-flow test tier doc + encapsulated reset helpers (SimBridge.reset_for_test, MetaStack.reset_for_test) + minimal public API on scenes so UI refactors don't break all four flow tests simultaneously.

H3 remaining coverage tickets

#879 (fog), #880 (journal), #881 (minimap) cover three of the eight #870 deletions. #889 (this pass) covers sprite_integration. The remaining four:

• test_session_manager_sprint19 — incidentally covered by the new merge-path tests (test_load_game_save_selection_sets_pending_load_path). No dedicated ticket needed.
• test_debug_overlay_sprint19 — debug tooling, not gameplay; not restoring.
• test_entanglement_sprint22 — only surviving reference is a comment in game_state.gd:74. Entanglement is a server-side seed concept (#178); no live client behavior to cover.
• test_sprint30 — catch-all file from a prior sprint; no unique live coverage lost (investigated the API references during the delete).

Unchecked smoke box

The last test plan box — manual make game New Game flow — remains unchecked from the team worktree. It needs an X display session; covered in automated form by test_new_game_catalog_snapshot_resolves_loading_state which exercises the same code path end-to-end. If a live manual smoke is required before merge, that needs to happen in a desktop session outside this worktree.

## Review-fix pass (commit `60733738`) Thanks for the thorough review. Addressed below. ### Merged origin/main Branch was behind 9 commits (D-189 source-canonical pipeline etc.). Merged `origin/main` before the fix commit so the PR reflects current main state. ### H1 — 0/0/0 runner claim: resolved The PR body's "by inspection" note was from the prior session that **couldn't run `make test-client` at all** because `.godot/imported/` was stale — the runner returned `{"total":0,"passed":0,"failed":0}` with no way to trigger discovery. Today's investigation: `make build-client` regenerates imports; `make test-client` then returns real counts. Commit `bc310a3c`'s "2428/2488 passing" came from a real runner invocation against that rebuilt state. No ticket needed — the issue was "rebuild imports first", not a runner bug. Today's run after all review-fix changes: **2428/2488 passing, 60 pre-existing failures** unrelated to sprint 37 (test_dialogue_sprint20 #558 signal wiring, test_input_roundtrip integration-without-server, etc.). All sprint-37 test files are green. ### H2 / top-of-review — merge ordering: agree, client-first Per Tyre's top-of-review analysis, this PR should merge **before** server #137. Reasoning: main's current client has `PROTOCOL_VERSION: 23` with a strict-reject guard. If server merges first, any pre-existing sessions or anyone on stale main rejects every new-server snapshot until this PR also lands — broken window. Client-first: the new client tolerates both old and new server, no broken window. The briefing's `#874 → #875` text was wrong in light of main's old-client state. Acknowledged — flagging for whoever does the actual merge: **this PR first, then server #137.** ### Blocking code fixes (commit `60733738`) - **T3** — `_load_main_menu` / `_load_char_create` now `assert_that(packed).is_not_null()` instead of `push_warning + return`. Missing .tscn is now a red test, not a silent green. - **T1** — `handshake_complete(protocol_version: int)` signal had no listeners. Dropped the int parameter and the literal-0 emit. No API compat concern. - **H4** — `test_new_game_catalog_snapshot_resolves_loading_state` now asserts `SimBridge.state == CONNECTED` terminus alongside the loading-flag clear. Guarantees full flow completion. - **H5** — `test_protocol_bridge.gd:4` file-level comment refreshed; drops reference to removed protocol-version-check tests. - **H6** — `test_p0_regressions.gd:46` `_make_snapshot_bytes` helper comment refreshed and `version: 23` dropped from fixture dict. - **H7** — `test_merge_path_flows_sprint37.gd` `_make_catalog_snapshot` drops `version: 23` from fixture (D-192, not required). ### Follow-up tickets filed (non-blocking) - **T2 → #889** — Revive EntityRenderer sprite constants coverage (D-044 ENTITY_WIDTH/HEIGHT). These are load-bearing for feet-anchored y-sort (ENTITY_OFFSET_Y = TILE_SIZE - ENTITY_HEIGHT, D-019 tilt). test_sprite_integration.gd was the sole asserter. - **T4 → #890** — UI timeout fallback for bookmark catalog wait. #872 fixed the TCP-batch race; the systemic "catalog never arrives" class remains. ~10s timer + retry UI. - **T5/T6 → #891** — Scene-flow test tier doc + encapsulated reset helpers (`SimBridge.reset_for_test`, `MetaStack.reset_for_test`) + minimal public API on scenes so UI refactors don't break all four flow tests simultaneously. ### H3 remaining coverage tickets #879 (fog), #880 (journal), #881 (minimap) cover three of the eight #870 deletions. #889 (this pass) covers sprite_integration. The remaining four: - **test_session_manager_sprint19** — incidentally covered by the new merge-path tests (`test_load_game_save_selection_sets_pending_load_path`). No dedicated ticket needed. - **test_debug_overlay_sprint19** — debug tooling, not gameplay; not restoring. - **test_entanglement_sprint22** — only surviving reference is a comment in game_state.gd:74. Entanglement is a server-side seed concept (#178); no live client behavior to cover. - **test_sprint30** — catch-all file from a prior sprint; no unique live coverage lost (investigated the API references during the delete). ### Unchecked smoke box The last test plan box — manual `make game` New Game flow — remains unchecked from the team worktree. It needs an X display session; covered in automated form by `test_new_game_catalog_snapshot_resolves_loading_state` which exercises the same code path end-to-end. If a live manual smoke is required before merge, that needs to happen in a desktop session outside this worktree.

jpmschweitzer commented 2026-04-22 10:17:16 +02:00

Author

Owner

Copy Link

Copy Source

Review: sprint-37/client → main (PR #135) — Round 2

Reviewers: Hoshe (code quality & tests) + Tyre (architecture).

Verdict: CHANGES REQUESTED

Blockers are documentation hygiene only — no code changes needed. Three PR-description edits + one ticket update close the round.

---

Hoshe — Code Quality & Tests: REQUEST_CHANGES

All nits fixed. Coverage tickets for #870 deletions all filed and verified (#879 fog, #880 journal, #881 minimap, #889 sprite_integration). Two real blockers on test-evidence hygiene.

Round-1 status

#	Item	Status
H1	0/0/0 runner claim	PARTIALLY FIXED — commit 60733738 reports make test-client — 2428/2488 passing, but the PR body's test plan section still reads "0/0/0 runner output is a known infra problem, not caused by these changes; individual test files verified by inspection" with all six checkboxes unchecked. The comment update is better than nothing; the body edit is missing. Plus a 60 vs 22 failure-count gap — commit names test_dialogue_sprint20 #558 signals and test_input_roundtrip integration-sans-server as examples of the 60 "pre-existing" failures. Ticket #871 (sprint-36 pre-existing triage) covers 22 failures. The 60 > 22 delta means 38 failures are currently untracked per memory feedback_broken_tests_not_preexisting_shield.md. Either update #871 to document the full 60 (with rationale for the extra 38 — e.g. "newly visible after sprint-37 import rebuild"), or file a companion ticket.
H2	Merge ordering acknowledged	FIXED — client-first ordering clearly stated in a PR comment. Still not in PR body (see B3 below) but the contract is on record
H3	Coverage-loss tickets for 8 deletions	FIXED — #879 fog, #880 journal (covers journal_panel.gd 601-line surface: player_knowledge, scene lifecycle, UIStrings, _state_color), #881 minimap, #889 EntityRenderer sprite constants. debug_overlay acceptably exempt (dev-only, disabled in export builds). entanglement / sprint30 / session_manager covered by umbrella #871 though the "incidentally covered by merge-path tests" claim for session_manager is weak — none of the merge-path tests assert SessionManager API contracts (ID format, uniqueness, resume_game()). Rephrase the exemption to "coverage gap tracked under #871."
H4	CONNECTED state assertion	FIXED — test_new_game_catalog_snapshot_resolves_loading_state asserts SimBridge.state == SimBridge.ConnectionState.CONNECTED with explicit failure message
H5	test_protocol_bridge.gd:4 stale header	FIXED
H6	test_p0_regressions.gd:46 stale helper comment	FIXED — _make_snapshot_bytes dict no longer contains version key
H7	_make_catalog_snapshot over-specified version: 23	FIXED — version field removed from fixture

Merge-path smoke gate (skill 0b-i)

PR body still shows all six boxes unchecked, including the three merge-path items:

• New Game flow: main menu → New Game → character creation → connected (regression #872)
• Merge-path tests: test_merge_path_flows_sprint37.gd runner coverage
• Protocol smoke: decode snapshot with no version field succeeds

Automated coverage exists for all three (test_new_game_catalog_snapshot_resolves_loading_state asserts the full chain; protocol smoke is covered by forward-compat test). Update the PR body to reflect this — either tick the automated boxes with a note "covered by automated test" or keep them unchecked and explicitly say what's being deferred to a manual smoke by whom.

---

Tyre — Architecture: REQUEST_CHANGES

T1, T3 code fixes are clean. Deferrals #889, #890, #891 are substantive (not placeholders). B1 retracted — see below.

Round-1 status

#	Item	Status
T1	signal handshake_complete(protocol_version: int) residue	FIXED — signal declared with no param, emit site passes no args, grep confirms zero listeners passing int args anywhere in client/
T2	#870 coverage loss (journal + EntityRenderer)	FIXED — #880 exists for journal_panel (filed round-1, missed in my scan; retracted below as B1), #889 covers EntityRenderer sprite constants
T3	Silent-pass test helpers	FIXED — _load_main_menu / _load_char_create now use assert_that(packed).override_failure_message(...).is_not_null(). Missing .tscn fails loudly
T4	Catalog-wait timeout	DEFERRED → #890 (substantive — names main_menu.gd:72-74 poll loop, proposes ~10s timer + retry UX)
T5	Scene-flow test tier doc	DEFERRED → #891 (substantive)
T6	Test reach-into-private fields	DEFERRED → #891 (same ticket — proposes SimBridge.reset_for_test(), MetaStack.reset_for_test(), minimal public API)
Critical	Rebase + client-first merge ordering	Rebase FIXED (commit 469377ca merges main). Client-first ordering acknowledged in a PR comment but not in the PR body — see B3

Round-1 B1 retracted

I flagged "journal_panel coverage companion ticket missing" as a round-2 blocker. Incorrect: #880 was filed 2026-04-21 and covers the full journal_panel.gd test surface (player_knowledge snapshot parsing, scene lifecycle, entry rendering, UIStrings confidence/source keys, _state_color for Contradicted/Stale/Active). I missed the ticket in my scan — Hoshe found it. Retracted, no fix needed on B1.

Round-2 blockers (documentation only)

#	What	Severity
B2	PR body test plan outdated — commit 60733738 reports 2428/2488 passing, PR body still reads "0/0/0 runner output is a known infra problem" with all six boxes unchecked. Mutually inconsistent. Either the runner works (update the prose, tick the automated boxes) or it doesn't (the commit message overstates).	blocker
B3	Merge-ordering not visible on the PR. Round-1 flagged that client #135 must merge before server #137. Fix is in code (branch merged main), acknowledged in a PR comment, but a different human reviewer has no indication from PR description or commit trailers. Add one line to the PR description: Merge order: this PR (client) before server PR #137 — main currently pairs an old client with the D-192 server.	blocker

---

Summary

All round-1 code concerns resolved. Three remaining blockers are all documentation:

1. Update PR body test plan — replace stale 0/0/0 language with the real 2428/2488 run; tick automated boxes or explicitly defer unchecked ones with a reason
2. Reconcile the 60-vs-22 failure count — update #871 to document the full 60 (with rationale for the delta) or file a companion ticket
3. Add merge-ordering line to PR body — so non-review-thread mergers know

No code changes needed. Fastest close yet if picked up promptly.

# Review: sprint-37/client → main (PR #135) — Round 2 Reviewers: **Hoshe** (code quality & tests) + **Tyre** (architecture). ## Verdict: CHANGES REQUESTED Blockers are documentation hygiene only — no code changes needed. Three PR-description edits + one ticket update close the round. --- ## Hoshe — Code Quality & Tests: REQUEST_CHANGES All nits fixed. Coverage tickets for #870 deletions all filed and verified (#879 fog, #880 journal, #881 minimap, #889 sprite_integration). Two real blockers on test-evidence hygiene. ### Round-1 status | # | Item | Status | |---|------|--------| | H1 | 0/0/0 runner claim | **PARTIALLY FIXED** — commit `60733738` reports `make test-client — 2428/2488 passing`, but the PR body's test plan section still reads "0/0/0 runner output is a known infra problem, not caused by these changes; individual test files verified by inspection" with all six checkboxes unchecked. The comment update is better than nothing; the body edit is missing. **Plus a 60 vs 22 failure-count gap** — commit names `test_dialogue_sprint20 #558 signals` and `test_input_roundtrip integration-sans-server` as examples of the 60 "pre-existing" failures. Ticket #871 (sprint-36 pre-existing triage) covers 22 failures. The 60 > 22 delta means 38 failures are currently untracked per memory `feedback_broken_tests_not_preexisting_shield.md`. Either update #871 to document the full 60 (with rationale for the extra 38 — e.g. "newly visible after sprint-37 import rebuild"), or file a companion ticket. | | H2 | Merge ordering acknowledged | FIXED — client-first ordering clearly stated in a PR comment. Still not in PR body (see B3 below) but the contract is on record | | H3 | Coverage-loss tickets for 8 deletions | FIXED — #879 fog, #880 journal (covers `journal_panel.gd` 601-line surface: `player_knowledge`, scene lifecycle, UIStrings, `_state_color`), #881 minimap, #889 EntityRenderer sprite constants. `debug_overlay` acceptably exempt (dev-only, disabled in export builds). `entanglement` / `sprint30` / `session_manager` covered by umbrella #871 though the "incidentally covered by merge-path tests" claim for `session_manager` is weak — none of the merge-path tests assert `SessionManager` API contracts (ID format, uniqueness, `resume_game()`). Rephrase the exemption to "coverage gap tracked under #871." | | H4 | CONNECTED state assertion | FIXED — `test_new_game_catalog_snapshot_resolves_loading_state` asserts `SimBridge.state == SimBridge.ConnectionState.CONNECTED` with explicit failure message | | H5 | `test_protocol_bridge.gd:4` stale header | FIXED | | H6 | `test_p0_regressions.gd:46` stale helper comment | FIXED — `_make_snapshot_bytes` dict no longer contains `version` key | | H7 | `_make_catalog_snapshot` over-specified `version: 23` | FIXED — version field removed from fixture | ### Merge-path smoke gate (skill 0b-i) PR body still shows all six boxes unchecked, including the three merge-path items: - [ ] New Game flow: main menu → New Game → character creation → connected (regression #872) - [ ] Merge-path tests: `test_merge_path_flows_sprint37.gd` runner coverage - [ ] Protocol smoke: decode snapshot with no version field succeeds Automated coverage exists for all three (`test_new_game_catalog_snapshot_resolves_loading_state` asserts the full chain; protocol smoke is covered by forward-compat test). **Update the PR body to reflect this** — either tick the automated boxes with a note "covered by automated test" or keep them unchecked and explicitly say what's being deferred to a manual smoke by whom. --- ## Tyre — Architecture: REQUEST_CHANGES T1, T3 code fixes are clean. Deferrals #889, #890, #891 are substantive (not placeholders). **B1 retracted** — see below. ### Round-1 status | # | Item | Status | |---|------|--------| | T1 | `signal handshake_complete(protocol_version: int)` residue | FIXED — signal declared with no param, emit site passes no args, grep confirms zero listeners passing int args anywhere in `client/` | | T2 | `#870` coverage loss (journal + EntityRenderer) | FIXED — #880 exists for journal_panel (filed round-1, missed in my scan; retracted below as B1), #889 covers EntityRenderer sprite constants | | T3 | Silent-pass test helpers | FIXED — `_load_main_menu` / `_load_char_create` now use `assert_that(packed).override_failure_message(...).is_not_null()`. Missing `.tscn` fails loudly | | T4 | Catalog-wait timeout | DEFERRED → #890 (substantive — names `main_menu.gd:72-74` poll loop, proposes ~10s timer + retry UX) | | T5 | Scene-flow test tier doc | DEFERRED → #891 (substantive) | | T6 | Test reach-into-private fields | DEFERRED → #891 (same ticket — proposes `SimBridge.reset_for_test()`, `MetaStack.reset_for_test()`, minimal public API) | | Critical | Rebase + client-first merge ordering | Rebase FIXED (commit `469377ca` merges main). Client-first ordering acknowledged in a PR comment but not in the PR body — see B3 | ### Round-1 B1 retracted I flagged "journal_panel coverage companion ticket missing" as a round-2 blocker. Incorrect: **#880** was filed 2026-04-21 and covers the full `journal_panel.gd` test surface (player_knowledge snapshot parsing, scene lifecycle, entry rendering, UIStrings confidence/source keys, `_state_color` for Contradicted/Stale/Active). I missed the ticket in my scan — Hoshe found it. Retracted, no fix needed on B1. ### Round-2 blockers (documentation only) | # | What | Severity | |---|------|----------| | B2 | **PR body test plan outdated** — commit `60733738` reports 2428/2488 passing, PR body still reads "0/0/0 runner output is a known infra problem" with all six boxes unchecked. Mutually inconsistent. Either the runner works (update the prose, tick the automated boxes) or it doesn't (the commit message overstates). | blocker | | B3 | **Merge-ordering not visible on the PR.** Round-1 flagged that client #135 must merge before server #137. Fix is in code (branch merged main), acknowledged in a PR comment, but a different human reviewer has no indication from PR description or commit trailers. Add one line to the PR description: `Merge order: this PR (client) before server PR #137 — main currently pairs an old client with the D-192 server.` | blocker | --- ## Summary All round-1 code concerns resolved. Three remaining blockers are all documentation: 1. **Update PR body test plan** — replace stale 0/0/0 language with the real 2428/2488 run; tick automated boxes or explicitly defer unchecked ones with a reason 2. **Reconcile the 60-vs-22 failure count** — update #871 to document the full 60 (with rationale for the delta) or file a companion ticket 3. **Add merge-ordering line to PR body** — so non-review-thread mergers know No code changes needed. Fastest close yet if picked up promptly.

jpmschweitzer referenced this pull request 2026-04-22 10:17:22 +02:00

refactor(server): sprint 37 sweep — D-192, D-167, D-032 cleanup + bookmark save-state + generator polish #137

jpmschweitzer referenced this pull request 2026-04-22 10:26:48 +02:00

refactor(server): sprint 37 sweep — D-192, D-167, D-032 cleanup + bookmark save-state + generator polish #137

jpmschweitzer commented 2026-04-22 10:29:36 +02:00

Author

Owner

Copy Link

Copy Source

Round-2 blockers addressed (documentation only)

• B2 — PR body test plan: rewritten. Replaces the stale "0/0/0 runner / by inspection" language with the real make test-client result (2428/2488 passing, 23 failing test cases tracked under #871). Automated boxes ticked; the manual make game New Game smoke is the only one still unchecked, with an explicit defer rationale (needs X display, stronger automated coverage exists).
• B3 — Merge ordering: added a ## 🚨 Merge order section to the PR body so anyone opening the PR sees this PR (client) must merge BEFORE server PR #137 without needing to find the review thread.
• 60 vs 22 failure count: the 60 in commit 60733738's message was assertion-level (gdUnit4 JSON failed); the 22-23 in #871 is test-case level. Same 13 suites, same tests, different counting mode. #871 updated with the current 23-case enumeration, the delta vs sprint-36 (no new suites, no upward regressions — several suites improved, test_bug_report_ring_buffer and test_game_state dropped out entirely thanks to sprint-37 fixes), and an umbrella note for the three sprint-37 #870 deletions that don't have dedicated revive tickets (entanglement, sprint30, session_manager).
• Hoshe H3 nit — session_manager exemption wording: #871 description now reads "SessionManager API contracts (ID format, uniqueness, resume_game) — none directly asserted in the new merge-path tests; tracked here rather than filing a dedicated ticket." No more "incidentally covered by merge-path tests" claim.

No code changes this pass. Ready for re-review.

## Round-2 blockers addressed (documentation only) - **B2 — PR body test plan:** rewritten. Replaces the stale "0/0/0 runner / by inspection" language with the real `make test-client` result (2428/2488 passing, 23 failing test cases tracked under #871). Automated boxes ticked; the manual `make game` New Game smoke is the only one still unchecked, with an explicit defer rationale (needs X display, stronger automated coverage exists). - **B3 — Merge ordering:** added a `## 🚨 Merge order` section to the PR body so anyone opening the PR sees `this PR (client) must merge BEFORE server PR #137` without needing to find the review thread. - **60 vs 22 failure count:** the 60 in commit `60733738`'s message was assertion-level (gdUnit4 JSON `failed`); the 22-23 in #871 is test-case level. Same 13 suites, same tests, different counting mode. **#871 updated** with the current 23-case enumeration, the delta vs sprint-36 (no new suites, no upward regressions — several suites improved, test_bug_report_ring_buffer and test_game_state dropped out entirely thanks to sprint-37 fixes), and an umbrella note for the three sprint-37 #870 deletions that don't have dedicated revive tickets (entanglement, sprint30, session_manager). - **Hoshe H3 nit — session_manager exemption wording:** #871 description now reads "SessionManager API contracts (ID format, uniqueness, resume_game) — none directly asserted in the new merge-path tests; tracked here rather than filing a dedicated ticket." No more "incidentally covered by merge-path tests" claim. No code changes this pass. Ready for re-review.

jpmschweitzer commented 2026-04-22 10:34:48 +02:00

Author

Owner

Copy Link

Copy Source

Review: sprint-37/client → main (PR #135) — Round 3

Reviewers: Hoshe (code quality & tests) + Tyre (architecture) — direct verification, no code changes this round.

Verdict: APPROVED

Three round-2 blockers resolved without new commits — all were documentation / ticket edits.

Round-2 status

#	Blocker	Status
1	PR body test plan outdated ("0/0/0 runner output" + unchecked boxes)	FIXED — rewritten: 2428/2488 passing, 23 failing test cases (60 assertion-level failures). All sprint-37 test files green. 5 of 6 boxes now ticked; the make game manual smoke box is explicitly deferred with rationale: no X display from team worktree, and the automated test_new_game_catalog_snapshot_resolves_loading_state asserts the full CONNECTED-state terminus — stronger than a one-shot manual check
2	#871 vs 60-failure reconciliation	FIXED — #871 description rewritten with a per-suite failing-test-case table (23 cases across 13 suites), delta-vs-sprint-36-baseline per suite (test_bug_report_ring_buffer 2→0, test_rendering 5→3, test_input_roundtrip 6→3, etc.), and the 23-vs-60 counting-mode discrepancy explained (test-case vs assertion-level). Umbrella scope explicitly covers the three unticketed #870 deletions (entanglement, sprint30, session_manager)
3	Merge ordering not in PR body	FIXED — dedicated "🚨 Merge order" section at top of PR body: "This PR (client) must merge BEFORE server PR #137" with pair-old-client rationale

Round-1/2 summary

• Code: all verified clean in round 2 (T1 signal residue, T3 silent-pass helpers, H4 CONNECTED assertion, H5–H7 stale comments — all FIXED). No code changed since.
• Deferrals: #880 (journal coverage), #889 (EntityRenderer sprite constants), #890 (UI timeout fallback), #891 (scene-flow tier doc + reset helpers) — all filed with substantive scope.
• Merge-path smoke gate (skill 0b-i): 5 of 6 boxes ticked via automated coverage; manual smoke box deferred with justification that the automated path is stronger. Acceptable per gate semantics.

Merge reminder

Client first, then server. Main currently pairs an old client (PROTOCOL_VERSION: 23 strict reject) with the D-192-aware server on origin/main. Client merges → new client tolerates both old and new server during the rollout. Server merges second → all clients now accept no-version snapshots.

Summary

Three-round review arc on PR #135:

• Round 1: 13 items (4 Hoshe + 6 Tyre + 3 deferrals)
• Round 2: 3 blockers (all docs hygiene)
• Round 3: 0 blockers (docs resolved)

Fastest round-3 close of sprint 37. Ready to merge.

# Review: sprint-37/client → main (PR #135) — Round 3 Reviewers: **Hoshe** (code quality & tests) + **Tyre** (architecture) — direct verification, no code changes this round. ## Verdict: APPROVED Three round-2 blockers resolved without new commits — all were documentation / ticket edits. ### Round-2 status | # | Blocker | Status | |---|---------|--------| | 1 | PR body test plan outdated ("0/0/0 runner output" + unchecked boxes) | **FIXED** — rewritten: `2428/2488 passing, 23 failing test cases (60 assertion-level failures). All sprint-37 test files green.` 5 of 6 boxes now ticked; the `make game` manual smoke box is explicitly deferred with rationale: no X display from team worktree, and the automated `test_new_game_catalog_snapshot_resolves_loading_state` asserts the full CONNECTED-state terminus — stronger than a one-shot manual check | | 2 | #871 vs 60-failure reconciliation | **FIXED** — #871 description rewritten with a per-suite failing-test-case table (23 cases across 13 suites), delta-vs-sprint-36-baseline per suite (test_bug_report_ring_buffer 2→0, test_rendering 5→3, test_input_roundtrip 6→3, etc.), and the 23-vs-60 counting-mode discrepancy explained (test-case vs assertion-level). Umbrella scope explicitly covers the three unticketed #870 deletions (entanglement, sprint30, session_manager) | | 3 | Merge ordering not in PR body | **FIXED** — dedicated "🚨 Merge order" section at top of PR body: "This PR (client) must merge BEFORE server PR #137" with pair-old-client rationale | ## Round-1/2 summary - **Code:** all verified clean in round 2 (T1 signal residue, T3 silent-pass helpers, H4 CONNECTED assertion, H5–H7 stale comments — all FIXED). No code changed since. - **Deferrals:** #880 (journal coverage), #889 (EntityRenderer sprite constants), #890 (UI timeout fallback), #891 (scene-flow tier doc + reset helpers) — all filed with substantive scope. - **Merge-path smoke gate (skill 0b-i):** 5 of 6 boxes ticked via automated coverage; manual smoke box deferred with justification that the automated path is stronger. Acceptable per gate semantics. ## Merge reminder **Client first, then server.** Main currently pairs an old client (`PROTOCOL_VERSION: 23` strict reject) with the D-192-aware server on origin/main. Client merges → new client tolerates both old and new server during the rollout. Server merges second → all clients now accept no-version snapshots. ## Summary Three-round review arc on PR #135: - **Round 1:** 13 items (4 Hoshe + 6 Tyre + 3 deferrals) - **Round 2:** 3 blockers (all docs hygiene) - **Round 3:** 0 blockers (docs resolved) Fastest round-3 close of sprint 37. Ready to merge.

jpmschweitzer closed this pull request 2026-04-22 10:37:29 +02:00

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

No items

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

jpmschweitzer (Jeroen Schweitzer)

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#135