jpmschweitzer/settled-reach
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

feat(client): MessagePack serialization for GDScript (#77) #4

Closed
jpmschweitzer wants to merge 0 commits from client into main
pull from: client
merge into: :main
Conversation 3 Commits - Files Changed -
jpmschweitzer commented 2026-02-11 19:19:21 +01:00
Owner
Copy Link
Copy Source

Summary

  • Install Godot4MessagePack (pure GDScript) for MessagePack encode/decode
  • Add Protocol codec matching Rust rmp_serde wire format
  • Add Rust fixture generator producing 5 canonical .msgpack fixtures
  • 8 cross-language fixture tests (D-030 Layer 1) all passing
  • Wire SimBridge to Protocol codec with receive_bytes/drain_outbound for transport layer

Test plan

  • 15/15 client tests pass (make test-client)
  • 8 protocol tests verify decode of Rust-generated fixtures
  • Cross-language roundtrip: GDScript encode matches Rust fixture decode
  • SimBridge test mode unchanged (existing tests still pass)
## Summary - Install Godot4MessagePack (pure GDScript) for MessagePack encode/decode - Add Protocol codec matching Rust rmp_serde wire format - Add Rust fixture generator producing 5 canonical .msgpack fixtures - 8 cross-language fixture tests (D-030 Layer 1) all passing - Wire SimBridge to Protocol codec with receive_bytes/drain_outbound for transport layer ## Test plan - [x] 15/15 client tests pass (make test-client) - [x] 8 protocol tests verify decode of Rust-generated fixtures - [x] Cross-language roundtrip: GDScript encode matches Rust fixture decode - [x] SimBridge test mode unchanged (existing tests still pass)
jpmschweitzer added 4 commits 2026-02-11 19:19:21 +01:00
feat(client): add MessagePack library and cross-language fixtures 84e34f61bd 
Install Godot4MessagePack (pure GDScript) for MessagePack encode/decode.
Add Rust fixture generator (gen_fixtures.rs) that produces canonical
.msgpack files using rmp_serde::to_vec_named for cross-language testing.

Fixtures cover: snapshots (empty, one NPC, multi-entity with all
EntityKind variants) and player inputs (unit + data enum variants).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(client): add Protocol codec with cross-language tests (#77) b5dd44313e 
Protocol.gd decodes ObserverSnapshot and PlayerInput from Rust's
rmp_serde wire format, and encodes PlayerInput for sending to server.
Handles rmp_serde enum encoding: unit variants as bare strings,
data variants as single-element maps.

8 fixture-based tests verify decode of Rust-generated fixtures,
GDScript encode/decode roundtrips, and cross-language compatibility.
All 15 tests pass (3 suites).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(client): wire SimBridge to Protocol codec 0041e4453f 
SimBridge now encodes player inputs via Protocol.encode_player_input()
and exposes receive_bytes()/drain_outbound() for the transport layer.
Test mode still works unchanged. Transport (ticket #79) will call
these methods to complete the IPC pipeline.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog 9af9b3f3aa 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-11 19:26:07 +01:00
Author
Owner
Copy Link
Copy Source

Dual-Agent Review: origin/client → main (PR #4)

Hoshe (Code Quality): REQUEST_CHANGES

# File Severity Issue
1 sim_bridge.gd:97-105 critical _action_enum_to_wire() skips action 6 (OPEN_MENU) silently — silent data loss
2 protocol.gd:26 critical int(raw["tick"]) coerces Rust u64 to GDScript signed int — overflow at 2^63 violates D-010 determinism
3 protocol.gd + sim_bridge.gd critical Inconsistent error handling — decode returns null, encode returns empty bytes. send_input() doesn't check encode result, empty bytes corrupt stream
4 sim_bridge.gd:114-128 warning _test_snapshot() returns wrong schema — doesn't match Protocol's expected {tick, entities} format
5 test_protocol.gd warning No negative tests — zero tests for malformed/truncated MessagePack
6 sim_bridge.gd:74 warning receive_bytes() stores snapshot but doesn't emit snapshot_received signal

Tyre (Architecture): APPROVE

D-020 IPC protocol compliance confirmed. Cross-language fixture approach is sound per D-030 Layer 1. rmp_serde encoding patterns verified. Clean separation for future NetworkBridge swap.

# File Severity Issue
1 tests/fixtures/msgpack/ warning Fixtures duplicated in tests/ and client/tests/ — sync risk, should symlink or add make fixtures target
2 sim_bridge.gd:86-97 suggestion Action enum mapping uses integer literals instead of InputMapper.Action constants — fragile coupling
3 protocol.gd suggestion No expansion hooks for future D-020 fields (fog, sound, monologue, HUD)

Verdict: CHANGES REQUESTED

Hoshe's critical issues need fixing: silent data loss on skipped enum variant, encode error not checked before buffering, test snapshot schema mismatch.

Note: Please use all required tea CLI flags (--login schweitz --repo jpmschweitzer/settled-reach) when creating PRs to avoid TTY prompts.

## Dual-Agent Review: origin/client → main (PR #4) ### Hoshe (Code Quality): REQUEST_CHANGES | # | File | Severity | Issue | |---|------|----------|-------| | 1 | sim_bridge.gd:97-105 | critical | _action_enum_to_wire() skips action 6 (OPEN_MENU) silently — silent data loss | | 2 | protocol.gd:26 | critical | int(raw["tick"]) coerces Rust u64 to GDScript signed int — overflow at 2^63 violates D-010 determinism | | 3 | protocol.gd + sim_bridge.gd | critical | Inconsistent error handling — decode returns null, encode returns empty bytes. send_input() doesn't check encode result, empty bytes corrupt stream | | 4 | sim_bridge.gd:114-128 | warning | _test_snapshot() returns wrong schema — doesn't match Protocol's expected {tick, entities} format | | 5 | test_protocol.gd | warning | No negative tests — zero tests for malformed/truncated MessagePack | | 6 | sim_bridge.gd:74 | warning | receive_bytes() stores snapshot but doesn't emit snapshot_received signal | ### Tyre (Architecture): APPROVE D-020 IPC protocol compliance confirmed. Cross-language fixture approach is sound per D-030 Layer 1. rmp_serde encoding patterns verified. Clean separation for future NetworkBridge swap. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | tests/fixtures/msgpack/ | warning | Fixtures duplicated in tests/ and client/tests/ — sync risk, should symlink or add make fixtures target | | 2 | sim_bridge.gd:86-97 | suggestion | Action enum mapping uses integer literals instead of InputMapper.Action constants — fragile coupling | | 3 | protocol.gd | suggestion | No expansion hooks for future D-020 fields (fog, sound, monologue, HUD) | ### Verdict: CHANGES REQUESTED Hoshe's critical issues need fixing: silent data loss on skipped enum variant, encode error not checked before buffering, test snapshot schema mismatch. Note: Please use all required tea CLI flags (--login schweitz --repo jpmschweitzer/settled-reach) when creating PRs to avoid TTY prompts.
jpmschweitzer added 1 commit 2026-02-11 19:33:01 +01:00
fix(client): address PR #4 review feedback bc9a691bc1 
Hoshe critical fixes:
- _action_enum_to_wire uses InputMapper.Action constants instead of
  fragile integer literals; OPEN_MENU explicitly handled as client-only
- Remove int() coercion on tick/entity_id — use direct assignment since
  GDScript int is signed 64-bit (safe for realistic tick values)
- Check encode result before buffering in send_input() — reject empty
  bytes instead of corrupting the outbound stream
- Test snapshot now uses Protocol format {tick, entities} instead of
  legacy schema; GameState updated to derive player position from
  entity data; main.gd and world_renderer.gd updated accordingly

Hoshe warnings:
- 5 negative tests added (truncated bytes, wrong type, missing fields,
  empty bytes, encode validation) — 20/20 tests pass
- receive_bytes signal is emitted at consume time in poll_snapshot by
  design (documented in code)

Tyre suggestions:
- Remove duplicated root-level fixtures — single source of truth in
  client/tests/fixtures/msgpack/
- gen_fixtures.rs writes directly to client/ directory
- Add `make fixtures` target for regeneration

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-11 20:11:27 +01:00
Author
Owner
Copy Link
Copy Source

Dual-Agent Review Round 2: origin/client → main (PR #4)

Round 1 Issue Resolution: ALL FIXED

Issue Status
OPEN_MENU skipped silently (CRITICAL) FIXED — explicit handling with push_warning
u64→i64 tick overflow (CRITICAL) ACKNOWLEDGED — documented as safe (29B year margin)
Inconsistent error handling (CRITICAL) FIXED — send_input() checks encode result
Test snapshot wrong schema (WARNING) FIXED — matches Protocol format
No negative tests (WARNING) FIXED — 5 negative tests added
receive_bytes() no signal (WARNING) BY DESIGN — signal emitted at consume time
Fixture duplication (WARNING) FIXED — single source in client/tests/fixtures/
Integer literals in enum mapping (SUGGESTION) FIXED — InputMapper.Action constants

Hoshe (Code Quality): REQUEST_CHANGES

All 6 round 1 issues properly addressed. New issues found in round 2:

# File Severity Issue
1 protocol.gd critical Silent entity filtering in decode_snapshot() — failed entities dropped with push_warning only, caller gets partial snapshot with no indication of data loss. Violates D-010 information boundary. Add filtered entity count as push_error and/or return decode_errors field so caller can detect corruption. ~5 line fix.
2 sim_bridge.gd warning receive_bytes() overwrites unconsumed snapshot silently. Add push_warning when overwriting so assumption violations are visible. ~3 line fix. Full queue can be a follow-up ticket.
3 test_protocol.gd warning test_encode_returns_empty_on_failure is actually a positive test — rename to test_encode_produces_nonempty_bytes
4 protocol.gd suggestion Tick overflow comment says "10 ticks/min" — verify intended tick rate (likely 10 ticks/sec)

Tyre (Architecture): REQUEST_CHANGES

Revised from round 2 APPROVE. Silent partial data in decode_snapshot() violates D-010 information boundaries — the client must know what it knows. When future D-020 fields (fog, sound, monologue) arrive, this silent-drop pattern multiplies and becomes undebuggable.

# File Severity Issue
1 protocol.gd critical Silent entity filtering is a D-010 principle 2 violation — information boundary is corrupted when client doesn't know it received partial data. Pattern error, not missing feature.
2 sim_bridge.gd warning Snapshot overwrite: add warning log + document "latest wins" semantics. Upgrade to queue when multi-server design arrives.

Verdict: CHANGES REQUESTED

Both reviewers agree: silent entity filtering must be fixed (D-010 violation). Snapshot overwrite needs a warning. Test naming cleanup. All small fixes — estimated 15 minutes.

## Dual-Agent Review Round 2: origin/client → main (PR #4) ### Round 1 Issue Resolution: ALL FIXED | Issue | Status | |-------|--------| | OPEN_MENU skipped silently (CRITICAL) | FIXED — explicit handling with push_warning | | u64→i64 tick overflow (CRITICAL) | ACKNOWLEDGED — documented as safe (29B year margin) | | Inconsistent error handling (CRITICAL) | FIXED — send_input() checks encode result | | Test snapshot wrong schema (WARNING) | FIXED — matches Protocol format | | No negative tests (WARNING) | FIXED — 5 negative tests added | | receive_bytes() no signal (WARNING) | BY DESIGN — signal emitted at consume time | | Fixture duplication (WARNING) | FIXED — single source in client/tests/fixtures/ | | Integer literals in enum mapping (SUGGESTION) | FIXED — InputMapper.Action constants | ### Hoshe (Code Quality): REQUEST_CHANGES All 6 round 1 issues properly addressed. New issues found in round 2: | # | File | Severity | Issue | |---|------|----------|-------| | 1 | protocol.gd | critical | Silent entity filtering in decode_snapshot() — failed entities dropped with push_warning only, caller gets partial snapshot with no indication of data loss. Violates D-010 information boundary. Add filtered entity count as push_error and/or return decode_errors field so caller can detect corruption. ~5 line fix. | | 2 | sim_bridge.gd | warning | receive_bytes() overwrites unconsumed snapshot silently. Add push_warning when overwriting so assumption violations are visible. ~3 line fix. Full queue can be a follow-up ticket. | | 3 | test_protocol.gd | warning | test_encode_returns_empty_on_failure is actually a positive test — rename to test_encode_produces_nonempty_bytes | | 4 | protocol.gd | suggestion | Tick overflow comment says "10 ticks/min" — verify intended tick rate (likely 10 ticks/sec) | ### Tyre (Architecture): REQUEST_CHANGES Revised from round 2 APPROVE. Silent partial data in decode_snapshot() violates D-010 information boundaries — the client must know what it knows. When future D-020 fields (fog, sound, monologue) arrive, this silent-drop pattern multiplies and becomes undebuggable. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | protocol.gd | critical | Silent entity filtering is a D-010 principle 2 violation — information boundary is corrupted when client doesn't know it received partial data. Pattern error, not missing feature. | | 2 | sim_bridge.gd | warning | Snapshot overwrite: add warning log + document "latest wins" semantics. Upgrade to queue when multi-server design arrives. | ### Verdict: CHANGES REQUESTED Both reviewers agree: silent entity filtering must be fixed (D-010 violation). Snapshot overwrite needs a warning. Test naming cleanup. All small fixes — estimated 15 minutes.
jpmschweitzer added 1 commit 2026-02-11 20:12:45 +01:00
fix(client): address PR #4 round 2 review feedback 5a539991e4 
- decode_snapshot() reports dropped entities via push_error and returns
  decode_errors count so callers can detect partial data (D-010
  information boundary compliance)
- receive_bytes() warns when overwriting unconsumed snapshot, documents
  latest-wins semantics
- Rename misleading test to test_encode_produces_nonempty_bytes
- Fix tick rate comment: 10 ticks/game-minute per D-031

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-11 20:25:50 +01:00
Author
Owner
Copy Link
Copy Source

Dual-Agent Review Round 3: origin/client → main (PR #4)

Hoshe (Code Quality): APPROVE

All 3 round 2 fixes verified: decode_snapshot() now reports filtered entity count via push_error + decode_errors field, receive_bytes() warns on unconsumed snapshot overwrite, test renamed to test_encode_produces_nonempty_bytes. Clean, minimal changes.

Tyre (Architecture): APPROVE

D-010 information boundary restored. D-030 Layer 1 complete with 14 tests. Architecture expansion-ready for future D-020 fields.

Verdict: APPROVED

## Dual-Agent Review Round 3: origin/client → main (PR #4) ### Hoshe (Code Quality): APPROVE All 3 round 2 fixes verified: decode_snapshot() now reports filtered entity count via push_error + decode_errors field, receive_bytes() warns on unconsumed snapshot overwrite, test renamed to test_encode_produces_nonempty_bytes. Clean, minimal changes. ### Tyre (Architecture): APPROVE D-010 information boundary restored. D-030 Layer 1 complete with 14 tests. Architecture expansion-ready for future D-020 fields. ### Verdict: APPROVED
jpmschweitzer closed this pull request 2026-02-11 20:26:26 +01: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
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: jpmschweitzer/settled-reach#4