settled-reach/docs/test-reports/pr-004-protocol-codec-review.md

title, description, type, status, ticket, created, updated

title	description	type	status	ticket	created	updated
PR #4 Protocol Codec Review	QA review of PR #4 implementing the MessagePack-based wire protocol for Rust-Godot IPC. Verdict: REQUEST_CHANGES — 5 critical issues and 8 warnings identified covering missing enum variant, type coercion, error handling, and negative test gaps.	test-report	archived	#4	2026-02-11	2026-02-26

Test Report: PR #4 Protocol Codec Review

• Date: 2026-02-11
• Branch: origin/client
• Commits: 9af9b3f, 0041e44, b5dd443, 84e34f6
• Spec reference: D-020, D-030
• Reviewer: Hoshe (QA Engineer)

Summary

This PR implements the MessagePack-based wire protocol for Rust↔Godot IPC per D-020, with cross-language fixture tests per D-030 Layer 1. The implementation correctly decodes Rust-generated MessagePack fixtures and encodes GDScript inputs matching rmp_serde's named-field format.

Verdict: REQUEST_CHANGES

The core protocol implementation is sound and test coverage is strong for the happy path. However, there are 5 critical issues and 8 warnings that must be addressed before merge. Key concerns: missing enum variant in wire mapping, inconsistent error handling, type coercion edge cases, and insufficient negative testing.

Files Reviewed

Core Implementation

• /var/mnt/data/projects/settled-reach/main/client/scripts/protocol/protocol.gd (new, 114 lines)
• /var/mnt/data/projects/settled-reach/main/client/scripts/autoloads/sim_bridge.gd (modified, +51 lines)
• /var/mnt/data/projects/settled-reach/main/server/tests/gen_fixtures.rs (new, 63 lines)

Tests

• /var/mnt/data/projects/settled-reach/main/client/tests/test_protocol.gd (new, 137 lines, 9 tests)

Vendor Library (noted, not reviewed)

• client/addons/messagepack/messagepack.gd (368 lines, third-party)

Critical Issues (Must Fix)

1. Missing enum variant in wire mapping

File: client/scripts/autoloads/sim_bridge.gd Severity: CRITICAL Line: 97-105 (match statement in _action_enum_to_wire)

The InputMapper.Action enum has 8 values (0-7: MOVE_NORTH through PAUSE), but the wire mapping skips action 6 (OPEN_MENU). The comment "Note: action 6 is skipped in the match" indicates intentional omission, but this is dangerous:

1. Silent data loss: If InputMapper sends action 6, it maps to the default case, logs a warning, and returns empty string. send_input() then silently drops the action without notifying the caller.
2. Spec mismatch: D-020 says all semantic actions should cross the boundary. If OPEN_MENU is client-side only, this should be documented in the Rust PlayerAction enum comments.
3. Maintenance hazard: If InputMapper enum values change (reorder, insert), this mapping will silently break.

Required fix: Either (a) add OPEN_MENU to Rust PlayerAction enum + wire mapping, or (b) explicitly filter OPEN_MENU at the call site in send_input() with a comment explaining it's client-side only, or (c) document in D-020 which actions are client-local vs server-bound.

2. Type coercion may lose precision

File: client/scripts/protocol/protocol.gd Severity: CRITICAL Line: 26 ("tick": int(raw["tick"]))

MessagePack encodes Rust u64 as 64-bit unsigned integers. GDScript's int() coercion truncates to 64-bit signed. For tick values above 2^63 - 1 (9.2 quintillion), this will overflow and produce negative ticks.

While this is unlikely to occur in practice (at 10 tps, 2^63 ticks = 29 billion years), D-010 principle 4 requires deterministic simulation. Overflowing ticks would break determinism.

Required fix: Add bounds validation or use GDScript's native int (which is arbitrarily large in Godot 4). At minimum, add a comment acknowledging the 2^63 tick limit and referencing D-010.

3. Inconsistent error handling between encode/decode paths

File: client/scripts/protocol/protocol.gd Severity: CRITICAL Line: Multiple (encode vs decode error handling)

Decode functions (decode_snapshot, _decode_entity) return null on error and log via push_error() or push_warning(). Encode functions return PackedByteArray() (empty array) on error. Callers must check different failure modes:

# Decode: check null
var snapshot = Protocol.decode_snapshot(bytes)
if snapshot == null: # handle error

# Encode: check empty array
var bytes = Protocol.encode_player_input(tick, action)
if bytes.size() == 0: # handle error

This inconsistency makes the API error-prone. The calling code in sim_bridge.gd doesn't check encode results at all — if encoding fails, empty bytes are appended to _outbound_buffer and will corrupt the stream.

Required fix: Standardize error returns. Options:

• Return null for both (check if result == null)
• Return { "ok": bool, "value": Variant, "error": String } dict (explicit Result type)
• Add assertions in debug builds to fail-fast on encoding errors

4. Missing fixture files in repository

File: tests/fixtures/msgpack/*.msgpack Severity: CRITICAL Impact: Tests cannot run

The diff shows 5 binary fixture files added, but they are not present in the current main branch. The test suite test_protocol.gd will fail on first run with "file not found" errors.

Per D-030, cross-language fixture tests are Phase 1 critical path. If fixtures aren't committed, CI will fail.

Required fix: Verify fixtures are tracked in git and not ignored by .gitignore. Confirm binary files commit correctly (git may need *.msgpack LFS configuration or explicit git add -f).

5. No validation of fixture integrity

File: server/tests/gen_fixtures.rs Severity: CRITICAL Line: Entire file

The fixture generator writes bytes to disk but never validates they can be read back. If rmp_serde's serialization changes (Rust version bump, serde version change), fixtures may become corrupt and tests will pass with stale data.

Per D-030, fixtures must be regenerated on schema changes. There's no mechanism to detect when this is needed.

Required fix: Add a bidirectional test in gen_fixtures.rs:

1. Serialize to bytes
2. Deserialize bytes back to Rust types
3. Assert round-trip equality
4. Write bytes to disk

This ensures fixtures are valid at generation time.

Warnings (Should Fix)

6. Hardcoded fixture path is fragile

File: client/tests/test_protocol.gd Severity: WARNING Line: 5 (const FIXTURE_DIR = "res://tests/fixtures/msgpack/")

The path is hardcoded and not validated. If the directory doesn't exist or is misplaced, FileAccess.open() returns null and tests fail with unclear errors.

Suggested fix: Add a setup check in a before-all hook that verifies the directory exists and contains expected files. Fail fast with a clear error message if not.

7. No test coverage for malformed MessagePack

File: client/tests/test_protocol.gd Severity: WARNING Impact: Negative test gap per D-030

All 9 tests validate the happy path (well-formed fixtures decode correctly). There are zero tests for malformed inputs:

• Truncated MessagePack bytes
• Wrong type for required fields (tick as string, entity_id as float)
• Missing required fields
• Extra/unexpected fields
• Invalid enum variant names

Per D-030, negative tests are required to verify information boundaries. If malformed data can crash the client or leak unintended state, that's a security/robustness gap.

Suggested fix: Add a test suite test_protocol_malformed.gd with at least 5 cases covering truncated bytes, wrong types, missing fields, unknown enum variants, and oversized payloads.

8. Missing snapshot_received signal emission in receive_bytes()

File: client/scripts/autoloads/sim_bridge.gd Severity: WARNING Line: 74-76

receive_bytes() decodes and stores the snapshot in _last_snapshot, but doesn't emit the snapshot_received signal. Only poll_snapshot() emits it. This means:

• Listeners waiting on the signal won't be notified until poll_snapshot() is called
• If polling stops, snapshots accumulate silently

This may be intentional (poll-driven architecture), but the signal name implies immediate notification.

Suggested fix: Either (a) emit signal in receive_bytes(), or (b) rename signal to snapshot_polled, or (c) document the poll-before-signal behavior in a comment.

9. SimBridge test mode returns wrong snapshot structure

File: client/scripts/autoloads/sim_bridge.gd Severity: WARNING Line: 114-128 (_test_snapshot())

The test mode snapshot has keys { tick, player, entities, fog, hud } but Protocol expects { tick, entities } where entities have { entity_id, x, y, z, kind }. The test snapshot's entities have { id, type, position, name } — completely different schema.

This means test mode snapshots will break any code that consumes poll_snapshot() results expecting Protocol-compliant structure.

Suggested fix: Make _test_snapshot() return Protocol-compliant structure. If additional fields are needed for development, add them under a debug key that production code ignores.

10. No test for empty action name

File: client/scripts/autoloads/sim_bridge.gd Severity: WARNING Line: 63-64

If _action_enum_to_wire() returns empty string (unknown action), send_input() silently returns without encoding. There's no test verifying this early-exit behavior works correctly.

Suggested fix: Add a unit test for SimBridge that calls send_input() with an invalid action enum, then checks _outbound_buffer is still empty.

11. No cross-language test for data variant encoding

File: client/tests/test_protocol.gd Severity: WARNING Line: 123-131 (test_gdscript_encode_matches_rust_fixture)

The cross-language test only validates MoveNorth (unit variant). There's no equivalent test for UsePerceptionMode (data variant). Encoding a data variant as { "UsePerceptionMode": "thermal" } must exactly match rmp_serde's map encoding, but this isn't verified.

Suggested fix: Add a second cross-language test that encodes UsePerceptionMode with "thermal" and compares decoded structure to the Rust-generated input_perception_mode.msgpack fixture.

12. Protocol class is stateless but not documented

File: client/scripts/protocol/protocol.gd Severity: WARNING (documentation) Line: 1 (class declaration)

Protocol only has static functions, but the class comment doesn't mention this is a stateless utility. Future developers might try to instantiate it.

Suggested fix: Add "Static utility class — all functions are static, no instantiation needed" to the class comment.

13. Float comparison uses hardcoded tolerance

File: client/tests/test_protocol.gd Severity: WARNING Line: Multiple (assert_float(entity.x).is_equal_approx(10.0, 0.001))

All float assertions use 0.001 tolerance without explanation. For position values serialized as f32, this tolerance is reasonable, but if future fields use f64 or require exact comparison (e.g., currency amounts), the hardcoded value will cause false negatives.

Suggested fix: Define const FLOAT_TOLERANCE = 0.001 at the top of the test file with a comment explaining it's appropriate for f32 spatial coordinates per D-020.

Test Coverage Analysis

What's tested (9 tests, all passing per commit message)

✅ Decode snapshot with 1 NPC entity ✅ Decode empty snapshot ✅ Decode snapshot with 3 entities (all EntityKind variants) ✅ Decode unit variant input (MoveNorth) ✅ Decode data variant input (UsePerceptionMode) ✅ Encode-decode roundtrip for unit variant (MoveEast) ✅ Encode-decode roundtrip for data variant (UsePerceptionMode) ✅ Cross-language roundtrip (GDScript encode matches Rust fixture decode)

What's NOT tested (gaps per D-030)

❌ Malformed MessagePack (truncated, wrong types, missing fields) ❌ Unknown enum variants (future-proofing for schema evolution) ❌ Oversized payloads (DOS protection — 10MB snapshot?) ❌ Tick overflow (u64 > i64 boundary) ❌ Empty string vs null for optional fields ❌ SimBridge encode error handling (empty bytes in outbound buffer) ❌ SimBridge action 6 (OPEN_MENU) behavior ❌ Protocol thread safety (if called from non-main thread) ❌ Data variant encoding cross-language match (UsePerceptionMode fixture comparison) ❌ Fixture integrity (Rust roundtrip validation)

Coverage estimate

Happy path: 85% (excellent) Error handling: 30% (critical gap) Integration: 40% (SimBridge wiring not tested end-to-end) Overall: 55% (acceptable for Layer 1, but blocking gaps for Layer 2)

Performance Considerations (D-020 budget)

Per D-020, IPC serialization must stay under 1-5ms per tick. No benchmarks are included in this PR, but qualitative assessment:

• MessagePack decoding: Fast (binary format, no parsing)
• Struct field lookups: O(1) hash maps in MessagePack
• Entity array iteration: O(n) where n = entity count (expected 30-80 per snapshot per D-026)

Estimated latency: ~0.5-1ms for typical snapshot (50 entities). Within budget.

Concern: No test for worst-case (500 entities, max z-level stack). Add a performance test in Phase 2 per D-030.

Architectural Compliance

D-020 alignment

✅ MessagePack for client-server boundary ✅ Named-field encoding matches rmp_serde ✅ ObserverSnapshot is the only data structure crossing boundary ✅ PlayerInput uses semantic actions (not raw keys) ✅ Timestamped actions for deterministic processing ⚠️ Variable HUD composition mentioned in D-020 but not yet in ObserverSnapshot (future work, acceptable)

D-010 alignment (client-server separation)

✅ SimBridge abstracts transport (receive_bytes/drain_outbound are transport-agnostic) ✅ No game logic in GDScript codec (pure serialization) ✅ Deterministic input format (timestamped) ⚠️ Tick overflow could break determinism (see Critical Issue #2)

D-030 alignment (testability)

✅ Layer 1 fixture-based tests implemented (8 of 9 pass happy path) ❌ Layer 2 mock subprocess not yet started (expected in ticket #79) ❌ Layer 3 real subprocess integration deferred (expected in ticket #79) ⚠️ Negative tests missing (error handling gap)

Recommendations

Before merge (blocking)

1. Fix Critical Issue #1: Resolve OPEN_MENU enum mapping (add to server, or document as client-local)
2. Fix Critical Issue #3: Standardize encode error handling (add empty-bytes check in SimBridge.send_input)
3. Fix Critical Issue #4: Verify fixture files are committed and tracked (check .gitignore)
4. Fix Critical Issue #5: Add roundtrip validation to gen_fixtures.rs
5. Add 3-5 negative tests: Malformed MessagePack, missing fields, unknown enum variant (test_protocol_malformed.gd)

Before Layer 2 (ticket #79, high priority)

6. Fix Critical Issue #2: Add tick overflow handling or document 2^63 limit
7. Fix Warning #9: Make test mode snapshot Protocol-compliant
8. Add Warning #11: Cross-language test for data variant encoding
9. Add Warning #10: Unit test for invalid action enum handling

Before v0.1 release (medium priority)

10. Add Warning #7: Comprehensive malformed input test suite (10+ cases)
11. Add performance test: Worst-case snapshot (500 entities) decode time
12. Add Warning #8: Clarify snapshot_received signal semantics (poll-driven vs immediate)

Conclusion

The protocol implementation is structurally sound and demonstrates strong understanding of D-020 requirements. Cross-language fixture testing is correctly implemented per D-030 Layer 1. Code quality is high with clear comments and error logging.

However, 5 critical issues prevent immediate merge:

1. Missing enum variant risks silent data loss
2. Type overflow risks determinism violation (D-010)
3. Inconsistent error handling risks stream corruption
4. Missing fixtures break CI
5. No fixture validation risks stale test data

Additionally, 8 warnings represent gaps in error handling and negative testing that should be addressed before Layer 2 integration.

Recommended action: REQUEST_CHANGES. Address blocking issues #1, #3, #4, #5, and add minimal negative test coverage (3-5 tests). Then re-review and approve.

---

QA sign-off: Hoshe (QA Engineer) Next review: After fixes, before Layer 2 mock subprocess integration (ticket #79)