refactor(protocol): migrate plugin transport to canonical xcore-protocol model #5
refactor(protocol): migrate plugin transport to canonical xcore-protocol model #5
Conversation
Map outbound BanData publishes to the canonical moderation.ban.created contract so the protocol-first migration can proceed without changing Redis stream or envelope semantics. Keep Gson serialization and runtime transport ownership in XCore-plugin while validating the new slice with targeted and full transport checks.
Replace the sibling source-set wiring with the published xcore-protocol Java artifact so the moderation migration branch builds through the shared Maven path. Keep the current protocol mapping slice intact while aligning local and CI resolution with the canonical publish flow.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ade8c23680
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (event instanceof BanData banData) { | ||
| return gson.toJson(ModerationProtocolMapper.toBanCreated( | ||
| banData, | ||
| config.server, | ||
| Instant.ofEpochMilli(now) | ||
| )); |
There was a problem hiding this comment.
send now rewrites every BanData event to ModerationBanCreatedV1 JSON, but the receive path still deserializes moderation.ban payloads as BanData (dispatchStreamMessage uses gson.fromJson(payloadJson, type) for type == BanData.class). That makes subscribe(BanData.class, ...) decode mostly default/empty fields because keys like uuid, name, and expireDate are no longer present in the payload. Any consumer using the existing BanData subscription contract will silently get corrupted moderation data.
Useful? React with 👍 / 👎.
Replace legacy moderation wire payloads with canonical xcore-protocol DTO wrappers so Redis routes, publishers, and handlers share one transport contract. Update moderation transport tests to lock the canonical stream and command identities across the plugin.
Send and receive moderation transport messages as generated xcore-protocol DTOs so the wire layer no longer depends on local wrapper events. Align routing, handlers, and tests with the canonical moderation contracts while keeping stream identities and build validation green.
Drop unused moderation wrapper records so the transport layer matches the direct DTO moderation flow. This reduces leftover legacy surface without changing runtime behavior.
Switch the Discord link and admin transport flows to released xcore-protocol DTOs and canonical routes so XCore-plugin no longer depends on local wrapper events for this family.
Generalize RPC request and response handling so generated protocol DTOs can participate in Redis roundtrips, then switch the maps.list flow to canonical request and response messages. This keeps the transport surface aligned with xcore-protocol while preserving existing maps.remove behavior for a later slice.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 61e5d38efb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return new ModerationAuditAppendedV1( | ||
| toAuditEntryType(record.action), | ||
| new PlayerRefV1( | ||
| requireNonBlank(record.target == null ? null : record.target.uuid, "audit target uuid"), |
There was a problem hiding this comment.
ModerationService.tempBanByUuidOrIp and tempUnban both allow uuid to be null, and auditTarget(...) normalizes that to an empty string for audit records. This mapper now enforces requireNonBlank on record.target.uuid, so valid IP-only moderation operations throw IllegalArgumentException when posting ModerationAuditAppendedV1, failing the request after persistence has already happened.
Useful? React with 👍 / 👎.
| ) { | ||
| return new ModerationKickBannedCommandV1( | ||
| new PlayerCommandTargetV1( | ||
| requireNonBlank(playerUuid, "playerUuid"), |
There was a problem hiding this comment.
The command mapper now requires a non-blank playerUuid, but the moderation service explicitly supports UUID-less IP-based actions (tempBanByUuidOrIp/tempUnban). For those valid inputs, building the kick/pardon command throws instead of publishing, so cross-server enforcement and pardon propagation break for IP-only moderation flows.
Useful? React with 👍 / 👎.
Consume xcore-protocol 0.2.0 and relax moderation command and audit mapping so UUID-or-IP targets can round-trip through the plugin. Skip ban-created emission for IP-only temporary bans while preserving the stricter event contract.
Switch the remaining maps.remove RPC flow to generated xcore-protocol DTOs and canonical route metadata so the maps RPC family no longer depends on local transport request and response wrappers.
Switch the heartbeat producer and transport metadata to the generated server.heartbeat DTO so Phase 5 starts on the canonical protocol surface. Remove the dead local heartbeat wrapper now that routing and validation are fully aligned.
Switch the chat message and global chat transport flow to generated xcore-protocol DTOs and canonical routes so the Phase 5 broadcast path no longer depends on local wrapper events.
Switch the Discord ingress transport flow to the generated protocol command and canonical route metadata so the Phase 5 chat ingress path no longer depends on a local wrapper event.
Consume xcore-protocol 0.3.0 and switch the cross-server private-message relay to the canonical chat.private contract so the Phase 5 chat flow no longer depends on a local wrapper event.
Move server action and player join/leave traffic onto generated protocol DTOs and remove the raw fallback route so Redis transport only emits canonical metadata. This completes the remaining Phase 5 tail work and adds regression coverage for unsupported payload rejection.
Move map load transport onto the generated protocol command so the plugin uses the canonical maps.load.command contract across routing and tests.
Remove obsolete maps request and response scaffolding now that the plugin uses generated protocol DTOs for the live maps transport path.
Switch the custom nickname, active badge, and badge symbol color mode transport flow to generated protocol commands so plugin routing and handlers use the canonical server-targeted contract. This also updates the protocol dependency to the snapshot containing the new chat-family player-session commands and removes the replaced legacy transport records.
…s to V1 Switch producer (BadgeAdminController) and consumer (ModerationTransportHandler) from legacy TransportEvents records to ChatMessages V1 command DTOs. Update routing (RedisRouteRegistry, RedisTransportTopology) to use PAYLOAD_SERVER resolver and canonical event type names. Remove dead legacy types PlayerBadgeInventoryChanged/PlayerPasswordReset from TransportEvents. Add ModerationTransportHandler consumer tests.
Update plugin mappers and transport-facing tests to consume the\nlatest canonical xcore-protocol actor/source DTO shapes, enum\nmodels, and map file naming.\n\nThis keeps XCore-plugin aligned with the generated protocol\nsurface and avoids reintroducing local contract drift.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e66238f6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| network.post(new ServerHeartbeatV1( | ||
| config.server, | ||
| config.discordChannelId, | ||
| Math.toIntExact(config.discordChannelId), |
There was a problem hiding this comment.
Converting config.discordChannelId with Math.toIntExact(...) will throw for normal Discord snowflake IDs (they are 64-bit and typically exceed Integer.MAX_VALUE), so servers configured with real channel IDs will fail to publish heartbeats every cycle and silently lose liveness telemetry. The field is already stored as long in Config, so this narrowing conversion introduces a runtime failure path in production.
Useful? React with 👍 / 👎.
Force CI build, snapshot publish, and release publish jobs to refresh Gradle dependencies before running tests. This reduces stale xcore-protocol snapshot cache issues in GitHub Actions for the migration branch.
Flush Redis between integration tests to avoid shared stream state leaking across cases. Align moderation payload assertions with the runtime Gson payload shape so CI validates the actual transport contract.
Publish heartbeat events with the full Discord snowflake instead of narrowing the configured channel ID to int. Update the router test to cover the widened protocol contract that now resolves from the refreshed protocol snapshot.
osp54
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3830a2a84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (Structs.contains(e.expectServers(), config.server)) return; | ||
| } else if (e.expectServers().length > 0 && !Structs.contains(e.expectServers(), config.server)) { | ||
| network.subscribe(ServerCommandExecuteCommandV1.class, e -> { | ||
| if (!e.targetServers().isEmpty()) { |
There was a problem hiding this comment.
The new execute-command handler assumes targetServers is always present and calls e.targetServers().isEmpty() unconditionally, but during phased migration the same Redis stream still carries legacy ExecuteCommand payloads that do not contain this field. Those messages deserialize with targetServers == null, causing a NullPointerException and dropping distributed command execution for that message batch.
Useful? React with 👍 / 👎.
| network.subscribe(TransportEvents.PardonPlayer.class, e -> { | ||
| Administration.PlayerInfo info = netServer.admins.getInfoOptional(e.uuid()); | ||
| network.subscribe(ModerationPardonCommandV1.class, e -> { | ||
| Administration.PlayerInfo info = netServer.admins.getInfoOptional(e.target().playerUuid()); |
There was a problem hiding this comment.
The pardon listener dereferences e.target() without a null check. Legacy PardonPlayer payloads on the same stream (or malformed payloads) deserialize without a nested target object, so this throws before any unban cleanup runs. In mixed-version rollout, a single legacy unban command can repeatedly fail instead of being processed.
Useful? React with 👍 / 👎.
| public void registerListeners() { | ||
| network.subscribe(TransportEvents.DiscordLinkConfirmEvent.class, e -> { | ||
| network.subscribe(DiscordLinkConfirmCommandV1.class, e -> { | ||
| Integer playerPid = e.player().playerPid(); |
There was a problem hiding this comment.
The confirm-link listener immediately reads e.player().playerPid() with no guard for a missing player object. Legacy confirm payloads on the same Redis command stream use top-level player fields, so player deserializes as null and the handler throws, preventing link confirmation in mixed-version deployments.
Useful? React with 👍 / 👎.
…ization BREAKING: Redis payload_json now emitted via ProtocolPayload.toPayload() which injects messageType/messageVersion and canonical nested ref shapes. - Bump xcore-protocol-java: 0.3.0-SNAPSHOT → 0.4.0 - RedisNetworkBackend.payloadJson() uses toPayload() for ProtocolPayload - RPC request/response serialization uses same path - Integration tests assert messageType/messageVersion in payload_json - Clean up unused TransportEvents imports
…ical-only migration
osp54
changed the title
1 participant
Summary