Draft
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThis PR introduces a binary protocol for gameplay message serialization, enabling efficient WebSocket communication. It adds code generation and runtime infrastructure for binary encoding/decoding, annotates schemas with binary metadata, updates client and server transports to handle binary payloads alongside JSON, and switches hash computation to 32-bit signed arithmetic. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant WebSocket
participant Server
Client->>WebSocket: connect()
Client->>WebSocket: send JSON "start" message
WebSocket->>Server: receive JSON "start"
Server->>Server: initialize binaryContext
Server->>Client: send JSON "start" response
Client->>Client: activate binary gameplay
Note over Client,Server: Binary message phase
Client->>Client: serialize intent as binary
Client->>WebSocket: send Uint8Array (binary intent)
WebSocket->>Server: receive Uint8Array
Server->>Server: decode binary intent
Server->>Server: process gameplay logic
Server->>Server: serialize turn as binary
Server->>WebSocket: send Uint8Array (binary turn)
WebSocket->>Client: receive Uint8Array
Client->>Client: decode binary turn
Client->>Client: update game state
sequenceDiagram
participant EventBus
participant Transport
participant WebSocket
participant BinaryCodec
EventBus->>Transport: SendAttackIntentEvent
Transport->>Transport: queue intent (pre-start)
Transport->>Transport: check if binary gameplay active
alt Binary gameplay active
Transport->>BinaryCodec: encodeBinaryClientGameplayMessage
BinaryCodec->>BinaryCodec: apply 32-bit arithmetic
BinaryCodec->>Transport: return Uint8Array
Transport->>WebSocket: send(Uint8Array)
else JSON fallback
Transport->>WebSocket: send(JSON string)
end
WebSocket->>Server: transmit frame
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches⚔️ Resolve merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
package.json (1)
4-4: Use the publishedtsxbin instead ofdist/cli.mjs.This hardcodes a package-internal file path. A
tsxupgrade or a different install layout can break generation even though npm already puts the supported executable on PATH for scripts.♻️ Suggested change
- "gen:binary-gameplay": "node node_modules/tsx/dist/cli.mjs scripts/gen-binary-gameplay.ts", + "gen:binary-gameplay": "tsx scripts/gen-binary-gameplay.ts",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@package.json` at line 4, The script "gen:binary-gameplay" currently invokes node node_modules/tsx/dist/cli.mjs which hardcodes an internal path; update the package.json script to call the published tsx binary instead (e.g., "tsx scripts/gen-binary-gameplay.ts") so npm/sh resolves the installed executable on PATH; edit the "gen:binary-gameplay" entry to replace the node node_modules/... invocation with the direct "tsx" command while keeping the same script file path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/Transport.ts`:
- Around line 336-337: The transport turns off binaryGameplayActive immediately
on reconnect which allows gameplay messages (ClientMessage: intent/ping/hash) to
be sent as JSON before activateBinaryGameplay() runs on the post-rejoin "start"
message; fix by gating gameplay sends on binaryGameplayActive (or a new
pendingGameplayAllowed flag) so send/serialize logic (the Transport
send/serializeOutgoing path for ClientMessage) either 1) drops or 2) enqueues
messages into a pending queue on reconnect and then flushes/serializes them only
when activateBinaryGameplay() is called after handling the "start" message;
update the code paths that emit gameplay messages (intent/ping/hash) to check
this gate and flush the queue in activateBinaryGameplay().
In `@src/core/BinaryProtocol.ts`:
- Around line 49-52: The exported BinaryClientGameplayMessage is too broad
because it unions all ClientIntentMessage variants including JSON-only intents;
restrict it to only binary-safe intents by deriving the intent subset from
BINARY_INTENT_DEFINITIONS and using that subset instead of ClientIntentMessage
in the union. Update the type so BinaryClientGameplayMessage =
BinaryClientIntentMessage | ClientHashMessage | ClientPingMessage (or similar),
where BinaryClientIntentMessage is constructed from the keys/entries of
BINARY_INTENT_DEFINITIONS (or a filtered mapping of ClientIntentMessage to only
those opcodes) to ensure TypeScript only allows opcodes the binary protocol
actually supports.
In `@src/core/protocol/BinaryGenerator.ts`:
- Around line 292-353: The current implementation in buildIntentDefinitions and
buildMessageDefinitions assigns wire IDs using definitions.length + 1 and index
+ 1 which makes opcode and messageType order-dependent; instead, derive fixed
IDs from explicit metadata or a checked-in mapping (e.g., attach a numeric id to
each intent schema in AllIntentSchema metadata or to registerBinaryMessage
entries) and use those values for opcode and messageType; update
buildIntentDefinitions and buildMessageDefinitions to read the pinned id (fail
generation if missing or if a duplicate id is detected), and add a collision
check that throws an error when two schemas claim the same pinned id so
reordering or inserts cannot silently renumber the protocol.
In `@src/core/protocol/BinaryRuntime.ts`:
- Around line 125-129: The writeString method can silently wrap lengths > 65535:
after encoding the string (textEncoder.encode in writeString) check if
encoded.length > 0xFFFF and throw a clear RangeError (or similar) indicating the
string is too long for the u16 length prefix (mentioning the field like
CancelAttackIntentSchema.attackID if applicable) instead of calling writeUint16;
this ensures callers fail fast on oversized attackID or other unconstrained
z.string() values and prevents emitting corrupt frames.
---
Nitpick comments:
In `@package.json`:
- Line 4: The script "gen:binary-gameplay" currently invokes node
node_modules/tsx/dist/cli.mjs which hardcodes an internal path; update the
package.json script to call the published tsx binary instead (e.g., "tsx
scripts/gen-binary-gameplay.ts") so npm/sh resolves the installed executable on
PATH; edit the "gen:binary-gameplay" entry to replace the node node_modules/...
invocation with the direct "tsx" command while keeping the same script file
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ff7570f2-0299-43f7-b6d2-25e7933ea4ca
📒 Files selected for processing (14)
package.jsonscripts/gen-binary-gameplay.tssrc/client/Transport.tssrc/core/BinaryCodec.tssrc/core/BinaryProtocol.tssrc/core/Schemas.tssrc/core/protocol/BinaryGenerator.tssrc/core/protocol/BinaryRuntime.tssrc/core/protocol/BinaryWire.tssrc/server/GameServer.tstests/core/BinaryCodec.test.tstests/core/BinaryGenerator.test.tstests/server/GameServerBinaryProtocol.test.tstsconfig.json
github-project-automation
bot
moved this from Triage
to Development
in OpenFront Release Management
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/protocol/BinaryRuntime.ts (1)
74-78: Consider removing this wrapper.
binaryContextFromGameStartInfojust callscreateBinaryProtocolContextwith identical signature. Having both adds indirection without benefit. If the alias is needed for re-export convenience only, a simpleexport { createBinaryProtocolContext as binaryContextFromGameStartInfo }would be cleaner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/protocol/BinaryRuntime.ts` around lines 74 - 78, The function binaryContextFromGameStartInfo simply forwards to createBinaryProtocolContext with the same signature and return type (BinaryProtocolContext), so remove the redundant wrapper and either delete binaryContextFromGameStartInfo and update imports to use createBinaryProtocolContext, or replace the function with a direct export alias (export { createBinaryProtocolContext as binaryContextFromGameStartInfo }) to preserve the public name; update any callers that import binaryContextFromGameStartInfo to import the chosen symbol (createBinaryProtocolContext or the alias) and keep the same types (GameStartInfo, BinaryProtocolContext).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/protocol/BinaryRuntime.ts`:
- Around line 60-72: The createBinaryProtocolContext function may silently
misalign indices when gameStartInfo.players contains duplicate clientID values;
update createBinaryProtocolContext to detect duplicates by checking whether
playerIdToIndex already has a clientID before setting it (or compare
playerIds[index] vs existing mapping), and when a duplicate is found throw or
log a clear error/assertion including the offending clientID and indices so
callers are alerted; ensure you reference gameStartInfo.players, playerIds, and
playerIdToIndex in the check and fail-fast instead of allowing silent
overwrites.
---
Nitpick comments:
In `@src/core/protocol/BinaryRuntime.ts`:
- Around line 74-78: The function binaryContextFromGameStartInfo simply forwards
to createBinaryProtocolContext with the same signature and return type
(BinaryProtocolContext), so remove the redundant wrapper and either delete
binaryContextFromGameStartInfo and update imports to use
createBinaryProtocolContext, or replace the function with a direct export alias
(export { createBinaryProtocolContext as binaryContextFromGameStartInfo }) to
preserve the public name; update any callers that import
binaryContextFromGameStartInfo to import the chosen symbol
(createBinaryProtocolContext or the alias) and keep the same types
(GameStartInfo, BinaryProtocolContext).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: db5445a0-15ab-4813-9a01-52321d36ba10
📒 Files selected for processing (7)
package.jsonsrc/client/Transport.tssrc/core/BinaryCodec.tssrc/core/protocol/BinaryRuntime.tstests/client/TransportBinaryProtocol.test.tstests/core/BinaryCodec.test.tstests/server/GameServerBinaryProtocol.test.ts
✅ Files skipped from review due to trivial changes (2)
- tests/server/GameServerBinaryProtocol.test.ts
- tests/core/BinaryCodec.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
src/core/protocol/BinaryRuntime.ts
Outdated
Comment on lines
+60
to
+72
| export function createBinaryProtocolContext( | ||
| gameStartInfo: Pick<GameStartInfo, "players">, | ||
| ): BinaryProtocolContext { | ||
| const playerIds = gameStartInfo.players.map((player) => player.clientID); | ||
| const playerIdToIndex = new Map<ClientID, number>(); | ||
| playerIds.forEach((clientID, index) => { | ||
| playerIdToIndex.set(clientID, index); | ||
| }); | ||
| return { | ||
| playerIds, | ||
| playerIdToIndex, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
Duplicate clientIDs in players array could cause silent index mismatch.
If gameStartInfo.players contains duplicate clientID values (no schema-level uniqueness constraint exists per src/core/Schemas.ts:566-580), the Map overwrites the first entry while the array keeps both. This causes playerIds[index] to return a different ID than playerIdToIndex.get(id) would expect.
Consider adding a guard or at least a debug assertion:
🛡️ Optional guard
export function createBinaryProtocolContext(
gameStartInfo: Pick<GameStartInfo, "players">,
): BinaryProtocolContext {
const playerIds = gameStartInfo.players.map((player) => player.clientID);
const playerIdToIndex = new Map<ClientID, number>();
playerIds.forEach((clientID, index) => {
+ if (playerIdToIndex.has(clientID)) {
+ throw new Error(`Duplicate clientID in players: ${clientID}`);
+ }
playerIdToIndex.set(clientID, index);
});
return {
playerIds,
playerIdToIndex,
};
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/core/protocol/BinaryRuntime.ts` around lines 60 - 72, The
createBinaryProtocolContext function may silently misalign indices when
gameStartInfo.players contains duplicate clientID values; update
createBinaryProtocolContext to detect duplicates by checking whether
playerIdToIndex already has a clientID before setting it (or compare
playerIds[index] vs existing mapping), and when a duplicate is found throw or
log a clear error/assertion including the offending clientID and indices so
callers are alerted; ensure you reference gameStartInfo.players, playerIds, and
playerIdToIndex in the check and fail-fast instead of allowing silent
overwrites.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/core/game/HashContract.test.ts (1)
47-57: Consider adding a test case with units.Current test covers players with only troops (no tiles, no units). Since
PlayerImpl.hash()also folds inunit.hash()values, a test case that spawns units would give fuller coverage of the hash chain throughUnitImpl.hash().This is not blocking - the arithmetic contract is well-tested - but would strengthen confidence in the full pipeline.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/game/HashContract.test.ts` around lines 47 - 57, Add a new test in HashContract.test.ts that covers units by creating a player with at least one UnitImpl (or by using the game's spawnUnit/addUnit API) with deterministic properties, ensure the unit's hash (UnitImpl.hash()) contributes to the player's hash (PlayerImpl.hash()), call game.executeNextTick() and collect the GameUpdateType.Hash update as in the existing test, then assert the observed hash equals the expected combined hash that includes the unit's hash; use the same pattern (hashUpdates = updatesOfType(...), expect(hashUpdate).toBeDefined(), expect(hashUpdate.hash).toBe(expectedHash)) so the test mirrors the current tick assertion and verifies the full hash chain.src/core/BinaryCodec.ts (1)
104-118: Single-source the opcode inencodeAutoMessage.
messageTyperepeatsdefinition.messageType. Keeping both makes future copy/paste edits easy to mismatch. Let the helper read the opcode fromdefinitionso the frame metadata stays in one place.♻️ Suggested cleanup
function encodeAutoMessage( - messageType: number, definition: typeof hashMessageDefinition, source: Record<string, unknown>, context: BinaryProtocolContext, ): Uint8Array { const writer = new BinaryWriter(); - return writer.writeFrame(messageType, () => { + return writer.writeFrame(definition.messageType, () => { if (definition.allowedFlags !== 0) { writer.writeUint16( encodeFlags(definition.type, definition.fields, source), ); } encodeDefinedFields(writer, definition.fields, source, context); }); } @@ return encodeAutoMessage( - BinaryMessageType.Hash, hashMessageDefinition, message as Record<string, unknown>, context, ); @@ return encodeAutoMessage( - BinaryMessageType.Ping, pingMessageDefinition, message as Record<string, unknown>, context, ); @@ return encodeAutoMessage( - BinaryMessageType.Desync, desyncMessageDefinition, message as Record<string, unknown>, context, );Also applies to: 241-246, 265-270, 332-337
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/BinaryCodec.ts` around lines 104 - 118, The function encodeAutoMessage currently accepts a redundant messageType parameter while the opcode lives on definition.messageType; remove the messageType parameter from encodeAutoMessage, read the opcode from definition.messageType where BinaryWriter.writeFrame is called, and update all callers to stop passing messageType (and instead rely on definition.messageType). Locate encodeAutoMessage and any sibling helpers that duplicate the opcode pattern (the other occurrences noted in the review) and make the same change so the frame metadata is single-sourced; ensure signatures and call sites compile after the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/core/BinaryCodec.ts`:
- Around line 104-118: The function encodeAutoMessage currently accepts a
redundant messageType parameter while the opcode lives on
definition.messageType; remove the messageType parameter from encodeAutoMessage,
read the opcode from definition.messageType where BinaryWriter.writeFrame is
called, and update all callers to stop passing messageType (and instead rely on
definition.messageType). Locate encodeAutoMessage and any sibling helpers that
duplicate the opcode pattern (the other occurrences noted in the review) and
make the same change so the frame metadata is single-sourced; ensure signatures
and call sites compile after the change.
In `@tests/core/game/HashContract.test.ts`:
- Around line 47-57: Add a new test in HashContract.test.ts that covers units by
creating a player with at least one UnitImpl (or by using the game's
spawnUnit/addUnit API) with deterministic properties, ensure the unit's hash
(UnitImpl.hash()) contributes to the player's hash (PlayerImpl.hash()), call
game.executeNextTick() and collect the GameUpdateType.Hash update as in the
existing test, then assert the observed hash equals the expected combined hash
that includes the unit's hash; use the same pattern (hashUpdates =
updatesOfType(...), expect(hashUpdate).toBeDefined(),
expect(hashUpdate.hash).toBe(expectedHash)) so the test mirrors the current tick
assertion and verifies the full hash chain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e016358b-aee0-44f6-8d02-6393cc07940c
📒 Files selected for processing (9)
src/client/LocalServer.tssrc/core/BinaryCodec.tssrc/core/Util.tssrc/core/game/GameImpl.tssrc/core/game/PlayerImpl.tssrc/core/game/UnitImpl.tssrc/core/protocol/BinaryRuntime.tstests/core/BinaryCodec.test.tstests/core/game/HashContract.test.ts
✅ Files skipped from review due to trivial changes (1)
- tests/core/BinaryCodec.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/protocol/BinaryRuntime.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/core/protocol/zb.ts (1)
21-23: Extract the shared game-id validator.
zb.playerRef()andIDinsrc/core/Schemas.tsare validating the same value with two regex copies. If one changes and the other does not, the JSON and binary paths will disagree on valid player refs. Please move this into a small shared module and reuse it from both sides.Also applies to: 254-266
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/protocol/zb.ts` around lines 21 - 23, The GAME_ID_REGEX duplication should be extracted into a shared validator module and both consumers should import it: create a small module exporting a single validator (e.g., isValidGameId or GAME_ID_REGEX constant) and replace the local GAME_ID_REGEX in this file and the ID validation used by zb.playerRef() and the ID schema in Schemas.ts to import and use that shared symbol; update zb.playerRef() and the schema definition that currently duplicates the regex so both binary and JSON paths call the same validator (also replace the other duplicate found around the other regex copy) to ensure a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/protocol/BinaryWire.ts`:
- Around line 45-47: orderedBinaryMessages and messageRegistrationOrder are
currently global mutable state causing duplicate/mutation issues; make
registration idempotent and avoid exposing the live array: change the
registration flow in buildMessageDefinitions()/any registerBinaryMessage* helper
to check a stable key (e.g., message type name or schema identifier) in a
Set/Map and skip re-registration if already registered (do not push duplicates
or increment messageRegistrationOrder twice), and change
getBinaryGameplayMessageSchemas() to return a shallow copy (e.g.,
[...orderedBinaryMessages]) or a frozen copy so callers cannot mutate the
internal array; apply the same dedupe and copy-return behavior for the other
similar registries referenced in the file (the code around the other
registration blocks and getters).
---
Nitpick comments:
In `@src/core/protocol/zb.ts`:
- Around line 21-23: The GAME_ID_REGEX duplication should be extracted into a
shared validator module and both consumers should import it: create a small
module exporting a single validator (e.g., isValidGameId or GAME_ID_REGEX
constant) and replace the local GAME_ID_REGEX in this file and the ID validation
used by zb.playerRef() and the ID schema in Schemas.ts to import and use that
shared symbol; update zb.playerRef() and the schema definition that currently
duplicates the regex so both binary and JSON paths call the same validator (also
replace the other duplicate found around the other regex copy) to ensure a
single source of truth.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a20b5fe1-dc51-4b4b-b182-5ce4d5f963f6
📒 Files selected for processing (6)
src/core/Schemas.tssrc/core/protocol/BinaryGenerator.tssrc/core/protocol/BinaryWire.tssrc/core/protocol/zb.tstests/core/BinaryGenerator.test.tstests/core/zb.test.ts
✅ Files skipped from review due to trivial changes (1)
- src/core/protocol/BinaryGenerator.ts
Remove Zod prototype/module patching from zb internals and attach binary helper metadata only on the final built schema. Add explicit .schema() terminals for helper-bearing zb chains so numeric/playerRef helpers can keep their metadata without wrapping Zod methods or mutating Zod instances globally.
Move generic auto-envelope framing into BinaryRuntime and route top-level binary messages through envelope handler tables in BinaryCodec. Remove the unused per-message encode/decode wrapper exports and make the client-vs-server decode parse policy explicit in one place without changing generated protocol output.
Keep binary gameplay message membership and envelope policy schema-adjacent in Schemas via explicit routing config. Generate top-level binary message types and client-message predicates from that config, re-export the generated types from BinaryProtocol, and add tests that lock routing parity and top-level decode rejection behavior.
Replace the flat field-plus-envelope protocol model with a recursive payload AST that supports objects, arrays, discriminated unions, and projected scalar leaves. Remove the special top-level intent and packed-turn codec paths, make binary message participation schema-driven without envelope metadata, and encode stamped client ids through an explicit clientIndex projection helper.
- rewrite BinaryGenerator to emit direct codec functions - remove generic BinaryRuntime encode/decode walkers - simplify BinaryCodec to generated message dispatch - update binary generator and codec tests
Contributor
Author
|
@coderabbitai full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/core/protocol/BinaryRuntime.ts (1)
24-36:⚠️ Potential issue | 🟡 MinorMissing duplicate
clientIDcheck in context creation.A past review flagged that duplicate
clientIDvalues ingameStartInfo.playerswould cause silent index mismatches. The guard was not added. Consider:export function binaryContextFromGameStartInfo( gameStartInfo: Pick<GameStartInfo, "players">, ): BinaryProtocolContext { const playerIds = gameStartInfo.players.map((player) => player.clientID); const playerIdToIndex = new Map<ClientID, number>(); playerIds.forEach((clientID, index) => { + if (playerIdToIndex.has(clientID)) { + throw new Error(`Duplicate clientID in players: ${clientID}`); + } playerIdToIndex.set(clientID, index); }); return { playerIds, playerIdToIndex, }; }While duplicates shouldn't occur in production (server validates uniqueness), a guard here would catch bugs earlier in development.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/protocol/BinaryRuntime.ts` around lines 24 - 36, binaryContextFromGameStartInfo currently builds playerIds and playerIdToIndex but doesn't check for duplicate clientID values, which can silently overwrite indices; update binaryContextFromGameStartInfo to detect duplicates while iterating gameStartInfo.players (or when inserting into playerIdToIndex) and throw a clear error including the duplicate clientID (and optionally the conflicting indices) if a clientID is already present, referencing the symbols playerIds, playerIdToIndex and gameStartInfo.players so callers fail fast during development.
🧹 Nitpick comments (6)
tests/core/BinaryGenerator.test.ts (1)
183-209: Registry mutation tests could affect parallel test runs.The tests at lines 183-209 directly mutate
binaryGameplayMessageRegistryand clean up withtry/finally. This works for sequential test execution within this file. However, if Vitest runs test files in parallel, other files importing this registry might see inconsistent state.Consider using
beforeEach/afterEachhooks or test isolation if parallel execution becomes an issue. For now, thetry/finallypattern is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/BinaryGenerator.test.ts` around lines 183 - 209, Tests mutate the shared binaryGameplayMessageRegistry directly (in the "rejects duplicate annotated binary message types across top-level unions" and "rejects orphan top-level binary message metadata" tests) which can leak state when Vitest runs tests in parallel; change these tests to register schemas in beforeEach and remove them in afterEach (or use a fresh registry instance per test) instead of relying on try/finally around collectGeneratedBinaryModel, ensuring you add/remove ServerPingMessageSchema and orphanSchema via beforeEach/afterEach (or swap in a test-local registry used by collectGeneratedBinaryModel) so state is isolated across tests.src/core/BinaryCodec.ts (1)
42-47: Consider validating server gameplay messages for consistency.
decodeBinaryClientGameplayMessagevalidates withClientMessageSchema.parse, butdecodeBinaryServerGameplayMessageskips schema validation. Since the server is trusted, this is lower risk. However, for consistency and defense-in-depth (e.g., catching protocol bugs early), you might add validation here too.♻️ Optional: Add schema validation for server messages
+import { ServerTurnMessageSchema, ServerDesyncSchema } from "./Schemas"; + export function decodeBinaryServerGameplayMessage( data: ArrayBuffer | Uint8Array, context: BinaryProtocolContext, ): BinaryServerGameplayMessage { - return decodeGeneratedBinaryServerGameplayMessage(data, context); + const decoded = decodeGeneratedBinaryServerGameplayMessage(data, context); + // Validate against appropriate schema based on type + return decoded; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/BinaryCodec.ts` around lines 42 - 47, decodeBinaryServerGameplayMessage currently just returns decodeGeneratedBinaryServerGameplayMessage(data, context) without validating the result; add the same defensive schema validation used for client messages by parsing the decoded message with the server message schema before returning. Locate decodeBinaryServerGameplayMessage and call the appropriate schema parse (analogous to ClientMessageSchema.parse) on the value returned by decodeGeneratedBinaryServerGameplayMessage, throwing or logging on parse failure so only validated BinaryServerGameplayMessage values are returned.src/core/Schemas.ts (1)
482-485: Type annotation and schema diverge onclientIDrepresentation.The
StampedIntenttype (line 485) declaresclientID: ClientID(string), but the schema useszb.clientIndexRef()which encodes to a numeric index on the wire. This is correct for runtime behavior, but the inline type annotation on line 485 could mislead readers into thinking the schema validates plain strings without the index projection.Consider adding a comment clarifying that the type represents the logical value while the schema handles binary serialization:
export const StampedIntentSchema = AllIntentSchema.and( zb.object({ clientID: zb.clientIndexRef().schema() }), ); +// clientID remains a string at runtime; clientIndexRef() handles binary index conversion export type StampedIntent = Intent & { clientID: ClientID };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/Schemas.ts` around lines 482 - 485, StampedIntent's TypeScript type (StampedIntent) and runtime schema (StampedIntentSchema using zb.clientIndexRef()) diverge: the type uses ClientID (string) while the schema serializes the value as a numeric client index; add a brief clarifying comment next to StampedIntentSchema / StampedIntent explaining that ClientID is the logical/string representation used in code but zb.clientIndexRef() encodes it as an index for on-wire/binary serialization so readers understand the intentional mismatch.src/core/protocol/zb.ts (2)
67-78: Type casts in builder methods are pragmatic but fragile.The
as unknown as z.ZodNumbercasts inmax()andnonnegative()bypass TypeScript's type checking. This works because the builder is only created fromz.number()in practice, but nothing prevents callingzb.u16().nullable().max(5)which would fail at runtime.The current approach is acceptable given the controlled usage, but adding a runtime check would be safer:
max(value) { if (!(schema instanceof z.ZodNumber)) { throw new Error("max() can only be called on number schemas"); } return createNumberBuilder(schema.max(value), wireType); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/protocol/zb.ts` around lines 67 - 78, The builder methods max and nonnegative currently cast schema with "as unknown as z.ZodNumber" which bypasses type safety and can cause runtime failures (e.g., calling zb.u16().nullable().max(5)); add a runtime type guard that checks schema is an instance of z.ZodNumber before calling schema.max(...) or schema.nonnegative(), and throw a clear Error like "max() can only be called on number schemas" (and similarly for nonnegative) if the check fails, then pass the validated schema to createNumberBuilder so only real ZodNumber schemas reach createNumberBuilder.
15-15:GAME_ID_REGEXis duplicated from Schemas.ts.This regex is also defined in
src/core/Schemas.ts(line 290). Consider importing it to avoid drift:-const GAME_ID_REGEX = /^[A-Za-z0-9]{8}$/; +import { GAME_ID_REGEX } from "../Schemas";Or export it from a shared location if Schemas.ts imports from zb.ts (which would create a cycle). The current duplication is low risk but could diverge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/protocol/zb.ts` at line 15, Remove the duplicated GAME_ID_REGEX constant from zb.ts and reuse the single source of truth by importing the exported GAME_ID_REGEX from Schemas.ts (or move the regex into a new shared module and import it from both places if Schemas.ts currently imports from zb.ts to avoid a cycle); update any references in zb.ts to use the imported GAME_ID_REGEX and remove the local declaration to prevent future drift.src/core/protocol/BinaryGenerator.ts (1)
392-403: Order-based tag assignment is intentional; however, ensure schema changes require version bumps.Tags are derived from
AllIntentSchemaorder by design (see test "derives intent opcodes from AllIntentSchema order while excluding json-only intents"). ABINARY_PROTOCOL_VERSIONmechanism exists and is validated on decode, but there is no enforcement that schema reordering or new intents trigger a version bump. Consider documenting this requirement or adding a build-time check to catch schema changes that require a version increment.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/protocol/BinaryGenerator.ts` around lines 392 - 403, The current tag assignment in BinaryGenerator.ts (variants computed from options using getDiscriminantLiteral, seenTypes, and compileObjectSchema based on AllIntentSchema) relies on array order but there's no enforcement that reordering or adding intents bumps BINARY_PROTOCOL_VERSION; add a determinism check: after building variants (the const variants = options.map(...) block), compute a stable fingerprint (e.g., hash of the ordered discriminant list from getDiscriminantLiteral) and compare it at build/test time against a checked-in EXPECTED_INTENT_SCHEMA_FINGERPRINT (or tie it to BINARY_PROTOCOL_VERSION); if it differs, fail the build/test with a clear message instructing to bump BINARY_PROTOCOL_VERSION or update the fingerprint. Ensure you reference the symbols variants, getDiscriminantLiteral, AllIntentSchema, and BINARY_PROTOCOL_VERSION so maintainers can locate where to add the check.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/client/LocalServer.ts`:
- Around line 204-206: The code reads
this.replayTurns[clientMsg.turnNumber].hash without first ensuring the replay
turn exists, which can throw when clientMsg.turnNumber is out of bounds; update
the logic in LocalServer (around the archivedHash assignment) to first check
that this.replayTurns[clientMsg.turnNumber] is defined (e.g., check index bounds
or use optional chaining) before accessing .hash, and handle the missing-turn
case (log/warn and return/skip) rather than relying on the later nullish check
on archivedHash; adjust the archivedHash assignment and subsequent flow
accordingly.
---
Duplicate comments:
In `@src/core/protocol/BinaryRuntime.ts`:
- Around line 24-36: binaryContextFromGameStartInfo currently builds playerIds
and playerIdToIndex but doesn't check for duplicate clientID values, which can
silently overwrite indices; update binaryContextFromGameStartInfo to detect
duplicates while iterating gameStartInfo.players (or when inserting into
playerIdToIndex) and throw a clear error including the duplicate clientID (and
optionally the conflicting indices) if a clientID is already present,
referencing the symbols playerIds, playerIdToIndex and gameStartInfo.players so
callers fail fast during development.
---
Nitpick comments:
In `@src/core/BinaryCodec.ts`:
- Around line 42-47: decodeBinaryServerGameplayMessage currently just returns
decodeGeneratedBinaryServerGameplayMessage(data, context) without validating the
result; add the same defensive schema validation used for client messages by
parsing the decoded message with the server message schema before returning.
Locate decodeBinaryServerGameplayMessage and call the appropriate schema parse
(analogous to ClientMessageSchema.parse) on the value returned by
decodeGeneratedBinaryServerGameplayMessage, throwing or logging on parse failure
so only validated BinaryServerGameplayMessage values are returned.
In `@src/core/protocol/BinaryGenerator.ts`:
- Around line 392-403: The current tag assignment in BinaryGenerator.ts
(variants computed from options using getDiscriminantLiteral, seenTypes, and
compileObjectSchema based on AllIntentSchema) relies on array order but there's
no enforcement that reordering or adding intents bumps BINARY_PROTOCOL_VERSION;
add a determinism check: after building variants (the const variants =
options.map(...) block), compute a stable fingerprint (e.g., hash of the ordered
discriminant list from getDiscriminantLiteral) and compare it at build/test time
against a checked-in EXPECTED_INTENT_SCHEMA_FINGERPRINT (or tie it to
BINARY_PROTOCOL_VERSION); if it differs, fail the build/test with a clear
message instructing to bump BINARY_PROTOCOL_VERSION or update the fingerprint.
Ensure you reference the symbols variants, getDiscriminantLiteral,
AllIntentSchema, and BINARY_PROTOCOL_VERSION so maintainers can locate where to
add the check.
In `@src/core/protocol/zb.ts`:
- Around line 67-78: The builder methods max and nonnegative currently cast
schema with "as unknown as z.ZodNumber" which bypasses type safety and can cause
runtime failures (e.g., calling zb.u16().nullable().max(5)); add a runtime type
guard that checks schema is an instance of z.ZodNumber before calling
schema.max(...) or schema.nonnegative(), and throw a clear Error like "max() can
only be called on number schemas" (and similarly for nonnegative) if the check
fails, then pass the validated schema to createNumberBuilder so only real
ZodNumber schemas reach createNumberBuilder.
- Line 15: Remove the duplicated GAME_ID_REGEX constant from zb.ts and reuse the
single source of truth by importing the exported GAME_ID_REGEX from Schemas.ts
(or move the regex into a new shared module and import it from both places if
Schemas.ts currently imports from zb.ts to avoid a cycle); update any references
in zb.ts to use the imported GAME_ID_REGEX and remove the local declaration to
prevent future drift.
In `@src/core/Schemas.ts`:
- Around line 482-485: StampedIntent's TypeScript type (StampedIntent) and
runtime schema (StampedIntentSchema using zb.clientIndexRef()) diverge: the type
uses ClientID (string) while the schema serializes the value as a numeric client
index; add a brief clarifying comment next to StampedIntentSchema /
StampedIntent explaining that ClientID is the logical/string representation used
in code but zb.clientIndexRef() encodes it as an index for on-wire/binary
serialization so readers understand the intentional mismatch.
In `@tests/core/BinaryGenerator.test.ts`:
- Around line 183-209: Tests mutate the shared binaryGameplayMessageRegistry
directly (in the "rejects duplicate annotated binary message types across
top-level unions" and "rejects orphan top-level binary message metadata" tests)
which can leak state when Vitest runs tests in parallel; change these tests to
register schemas in beforeEach and remove them in afterEach (or use a fresh
registry instance per test) instead of relying on try/finally around
collectGeneratedBinaryModel, ensuring you add/remove ServerPingMessageSchema and
orphanSchema via beforeEach/afterEach (or swap in a test-local registry used by
collectGeneratedBinaryModel) so state is isolated across tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0838c8aa-8f5e-4e11-9ec2-48a3428d9ed7
📒 Files selected for processing (22)
package.jsonscripts/gen-binary-gameplay.tssrc/client/LocalServer.tssrc/client/Transport.tssrc/core/BinaryCodec.tssrc/core/Schemas.tssrc/core/Util.tssrc/core/game/GameImpl.tssrc/core/game/PlayerImpl.tssrc/core/game/UnitImpl.tssrc/core/protocol/BinaryGenerator.tssrc/core/protocol/BinaryRuntime.tssrc/core/protocol/BinaryWire.tssrc/core/protocol/zb.tssrc/server/GameServer.tstests/client/TransportBinaryProtocol.test.tstests/core/BinaryCodec.test.tstests/core/BinaryGenerator.test.tstests/core/game/HashContract.test.tstests/core/zb.test.tstests/server/GameServerBinaryProtocol.test.tstsconfig.json
Comment on lines
204
to
206
| const archivedHash = this.replayTurns[clientMsg.turnNumber].hash; | ||
| if (!archivedHash) { | ||
| if (archivedHash === undefined || archivedHash === null) { | ||
| console.warn( |
Contributor
There was a problem hiding this comment.
Guard missing replay turn before reading .hash.
Line 204 can throw if clientMsg.turnNumber is outside this.replayTurns bounds, so the nullish check on Line 205 never runs.
🐛 Proposed fix
- const archivedHash = this.replayTurns[clientMsg.turnNumber].hash;
- if (archivedHash === undefined || archivedHash === null) {
+ const archivedTurn = this.replayTurns[clientMsg.turnNumber];
+ if (!archivedTurn) {
+ console.warn(
+ `no archived turn found for turn ${clientMsg.turnNumber}, client hash: ${clientMsg.hash}`,
+ );
+ return;
+ }
+ const archivedHash = archivedTurn.hash;
+ if (archivedHash === undefined || archivedHash === null) {
console.warn(
`no archived hash found for turn ${clientMsg.turnNumber}, client hash: ${clientMsg.hash}`,
);
return;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const archivedHash = this.replayTurns[clientMsg.turnNumber].hash; | |
| if (!archivedHash) { | |
| if (archivedHash === undefined || archivedHash === null) { | |
| console.warn( | |
| const archivedTurn = this.replayTurns[clientMsg.turnNumber]; | |
| if (!archivedTurn) { | |
| console.warn( | |
| `no archived turn found for turn ${clientMsg.turnNumber}, client hash: ${clientMsg.hash}`, | |
| ); | |
| return; | |
| } | |
| const archivedHash = archivedTurn.hash; | |
| if (archivedHash === undefined || archivedHash === null) { | |
| console.warn( | |
| `no archived hash found for turn ${clientMsg.turnNumber}, client hash: ${clientMsg.hash}`, | |
| ); | |
| return; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/client/LocalServer.ts` around lines 204 - 206, The code reads
this.replayTurns[clientMsg.turnNumber].hash without first ensuring the replay
turn exists, which can throw when clientMsg.turnNumber is out of bounds; update
the logic in LocalServer (around the archivedHash assignment) to first check
that this.replayTurns[clientMsg.turnNumber] is defined (e.g., check index bounds
or use optional chaining) before accessing .hash, and handle the missing-turn
case (log/warn and return/skip) rather than relying on the later nullish check
on archivedHash; adjust the archivedHash assignment and subsequent flow
accordingly.
evanpelle
reviewed
Mar 31, 2026
Collaborator
There was a problem hiding this comment.
mind moving everything zb related into its own dir? maybe like core/zodbinary/
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
If this PR fixes an issue, link it below. If not, delete these two lines.
Resolves #(issue number)
Description:
Describe the PR.
wip
Please complete the following:
Please put your Discord username so you can be contacted if a bug or regression is found:
DISCORD_USERNAME