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

feat(client): LocalBridge TCP transport and 8-directional movement (#79) #5

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

Summary

  • Add LocalBridge GDScript TCP transport with 4-byte BE length-prefix framing matching server framing.rs
  • Add ServerProcess subprocess manager for spawning/stopping the Rust simulation server
  • Integrate LocalBridge into SimBridge with _process() polling loop, connection state machine, and error handling
  • Add 8-directional movement support: 4 diagonal variants (NE, SE, SW, NW) in InputMapper, wire mapping, and project.godot
  • 12 new tests for framing roundtrips, cross-layer Protocol+framing, and diagonal wire mapping
  • 4 new diagonal movement cross-language fixtures (Rust to GDScript, D-030 Layer 1)
  • 33/33 client tests passing

Test plan

  • make test-client: 33/33 tests pass
  • godot4 --headless --path client --quit: clean load, no errors
  • Test mode unchanged (SimBridge with test_mode=true still works)
  • Framing matches server framing.rs (4-byte BE length prefix)
  • Cross-language diagonal fixtures decode correctly
  • End-to-end TCP test deferred to ticket #81 (requires server TCP listener)
## Summary - Add LocalBridge GDScript TCP transport with 4-byte BE length-prefix framing matching server framing.rs - Add ServerProcess subprocess manager for spawning/stopping the Rust simulation server - Integrate LocalBridge into SimBridge with _process() polling loop, connection state machine, and error handling - Add 8-directional movement support: 4 diagonal variants (NE, SE, SW, NW) in InputMapper, wire mapping, and project.godot - 12 new tests for framing roundtrips, cross-layer Protocol+framing, and diagonal wire mapping - 4 new diagonal movement cross-language fixtures (Rust to GDScript, D-030 Layer 1) - 33/33 client tests passing ## Test plan - [x] make test-client: 33/33 tests pass - [x] godot4 --headless --path client --quit: clean load, no errors - [x] Test mode unchanged (SimBridge with test_mode=true still works) - [x] Framing matches server framing.rs (4-byte BE length prefix) - [x] Cross-language diagonal fixtures decode correctly - [ ] End-to-end TCP test deferred to ticket #81 (requires server TCP listener)
jpmschweitzer added 4 commits 2026-02-11 21:00:24 +01:00
feat(client): add LocalBridge TCP transport and ServerProcess manager 6dfaf2831f 
LocalBridge wraps StreamPeerTCP with 4-byte big-endian length-prefix
framing matching server/src/bridge/framing.rs. ServerProcess manages
the Rust server as a subprocess via OS.create_process(). Together
they form the D-020 IPC transport layer for ticket #79.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(client): integrate LocalBridge transport and 8-directional movement 2ecafff2ed 
Wire SimBridge to use LocalBridge for TCP transport in non-test mode:
_process() polls for incoming snapshots and flushes outbound inputs.
Add 4 diagonal movement variants (NE, SE, SW, NW) to InputMapper and
wire mapping, ordered clockwise. Register diagonal input actions in
project.godot.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
feat(client): add LocalBridge framing tests and diagonal fixtures 0f75c3194d 
12 new tests covering framing roundtrips, cross-layer Protocol+framing
integration, and diagonal wire mapping. 4 new diagonal movement
fixtures generated from Rust for D-030 Layer 1 cross-language
verification. 33/33 tests pass.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
chore(meta): update changelog c4e1b07346 
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer commented 2026-02-11 21:09:48 +01:00
Author
Owner
Copy Link
Copy Source

Dual-Agent Review: client -> main

Hoshe (Code Quality): REQUEST_CHANGES

Solid implementation with good test coverage (12 new tests, 4 fixtures). Three critical issues identified.

# File Severity Issue
1 sim_bridge.gd connect_to_sim critical Race condition: spawns server subprocess then immediately connects TCP — server hasn't bound to port yet. Needs retry logic or startup handshake.
2 local_bridge.gd:~87 _try_extract_message critical Buffer corruption on oversized message: _read_buffer.clear() discards ALL buffered data including valid subsequent messages. Should disconnect (fail-safe) or skip only the bad frame.
3 sim_bridge.gd send_input critical Silent input drop on encode failure — returns without warning to caller.
4 project.godot diagonals warning All 4 diagonal input actions have empty event arrays — no keybindings. Intentional deferral or incomplete?
5 server_process.gd start warning No validation that server_path exists/is executable before create_process().
6 server_process.gd stop warning OS.kill() sends SIGKILL with no graceful shutdown attempt.
7 sim_bridge.gd receive_bytes warning Snapshot overwrite discards unconsumed data — no queue or interpolation.
8 test_local_bridge.gd suggestion Missing test for multiple messages in single buffer (where issue #2 would manifest).
9 local_bridge.gd send_message suggestion Two separate put_data() calls — partial write risk. Consider single concatenated call.

Tyre (Architecture): APPROVE

Solid architectural work directly implementing D-020 and D-030. Framing matches Rust server (4-byte BE length prefix). Three-layer IPC testing built correctly (L1 fixtures complete, L2 mock protocol implemented, L3 deferred as expected). D-010 client-server separation respected.

# File Severity Issue
1 local_bridge.gd statics suggestion Add comments noting static frame_encode/decode are for D-030 Layer 2 testing, not production use.
2 server_process.gd is_alive warning OS.is_process_running() doesn't distinguish starting/healthy/zombie. Flag for future health checks.
3 project.godot diagonals warning Empty event arrays — clarify if intentional deferral or incomplete.
4 sim_bridge.gd _action_enum_to_wire suggestion Match statement duplicates InputMapper knowledge — vulnerable to drift. Consider enum coverage test.
5 test_local_bridge.gd suggestion Add partial-read scenario test (half frame, then other half) to exercise state machine.

Verdict: CHANGES REQUESTED

Fix 3 critical issues before merge: (1) TCP connect race condition, (2) buffer corruption on oversized messages, (3) silent input drop.

## Dual-Agent Review: client -> main ### Hoshe (Code Quality): REQUEST_CHANGES Solid implementation with good test coverage (12 new tests, 4 fixtures). Three critical issues identified. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `sim_bridge.gd` connect_to_sim | critical | Race condition: spawns server subprocess then immediately connects TCP — server hasn't bound to port yet. Needs retry logic or startup handshake. | | 2 | `local_bridge.gd:~87` _try_extract_message | critical | Buffer corruption on oversized message: `_read_buffer.clear()` discards ALL buffered data including valid subsequent messages. Should disconnect (fail-safe) or skip only the bad frame. | | 3 | `sim_bridge.gd` send_input | critical | Silent input drop on encode failure — returns without warning to caller. | | 4 | `project.godot` diagonals | warning | All 4 diagonal input actions have empty event arrays — no keybindings. Intentional deferral or incomplete? | | 5 | `server_process.gd` start | warning | No validation that server_path exists/is executable before create_process(). | | 6 | `server_process.gd` stop | warning | OS.kill() sends SIGKILL with no graceful shutdown attempt. | | 7 | `sim_bridge.gd` receive_bytes | warning | Snapshot overwrite discards unconsumed data — no queue or interpolation. | | 8 | `test_local_bridge.gd` | suggestion | Missing test for multiple messages in single buffer (where issue #2 would manifest). | | 9 | `local_bridge.gd` send_message | suggestion | Two separate put_data() calls — partial write risk. Consider single concatenated call. | ### Tyre (Architecture): APPROVE Solid architectural work directly implementing D-020 and D-030. Framing matches Rust server (4-byte BE length prefix). Three-layer IPC testing built correctly (L1 fixtures complete, L2 mock protocol implemented, L3 deferred as expected). D-010 client-server separation respected. | # | File | Severity | Issue | |---|------|----------|-------| | 1 | `local_bridge.gd` statics | suggestion | Add comments noting static frame_encode/decode are for D-030 Layer 2 testing, not production use. | | 2 | `server_process.gd` is_alive | warning | OS.is_process_running() doesn't distinguish starting/healthy/zombie. Flag for future health checks. | | 3 | `project.godot` diagonals | warning | Empty event arrays — clarify if intentional deferral or incomplete. | | 4 | `sim_bridge.gd` _action_enum_to_wire | suggestion | Match statement duplicates InputMapper knowledge — vulnerable to drift. Consider enum coverage test. | | 5 | `test_local_bridge.gd` | suggestion | Add partial-read scenario test (half frame, then other half) to exercise state machine. | ### Verdict: CHANGES REQUESTED Fix 3 critical issues before merge: (1) TCP connect race condition, (2) buffer corruption on oversized messages, (3) silent input drop.
jpmschweitzer added 1 commit 2026-02-11 21:14:57 +01:00
fix(client): address PR #5 review — race condition, buffer corruption, input error 3befd2309c 
Critical fixes from Hoshe review:
1. Race condition: defer TCP connect to _process() with retry logic
   (MAX_CONNECT_RETRIES=20, 100ms interval) so server has time to bind.
2. Buffer corruption: disconnect on oversized message instead of
   clearing valid buffered data (_corrupt flag, fail-safe).
3. Silent input drop: send_input() returns Error so callers can detect
   encode/validation failures.

Warnings addressed:
- ServerProcess validates server_path exists before spawning
- SIGKILL and health check TODOs documented for future work
- Diagonal keybindings documented as intentional deferral
- send_message uses single put_data() call (no partial write risk)
- Static frame helpers documented as D-030 Layer 2 test-only

New tests (36 total, up from 33):
- Partial read scenario (chunked TCP delivery)
- Multi-message sequential decode (exercises buffer corruption fix)
- send_input error return on invalid action

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
jpmschweitzer closed this pull request 2026-02-11 21:26:12 +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#5