fix(minecraft-crossplay): Java port is not a setting #8

Merged

greg merged 2 commits from fix/port-minecraft-crossplay into main 2026-10-26 00:34:57 +00:00

Conversation 2 Commits 2 Files Changed 1

+1 -8

greg commented 2026-10-25 19:50:36 +00:00

Owner

Copy link

Copy source

Nodes always publish container port 25565/tcp and only move the host side when servers share a node, so an owner who changed SERVER_PORT made Java Edition unreachable.

• SERVER_PORT is removed from defaultConfig and configSchema; itzg/minecraft-server already defaults to 25565.
• pingPortKey is now null, so the status ping uses pingPort: 25565.
• HostPort: '{SERVER_PORT}' in the Xbox session properties is the broadcast service's own variable, not this setting, so it is unchanged.

Found by the portSettings draft check.

🤖 Generated with Claude Code

Nodes always publish container port `25565/tcp` and only move the host side when servers share a node, so an owner who changed `SERVER_PORT` made Java Edition unreachable. - `SERVER_PORT` is removed from `defaultConfig` and `configSchema`; itzg/minecraft-server already defaults to 25565. - `pingPortKey` is now `null`, so the status ping uses `pingPort: 25565`. - `HostPort: '{SERVER_PORT}'` in the Xbox session properties is the broadcast service's own variable, not this setting, so it is unchanged. Found by the `portSettings` draft check. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

👀 1

greg added 1 commit 2026-10-25 19:50:36 +00:00

fix(minecraft-crossplay): Java port is not a setting ... 133b78b730

Nodes always publish container port 25565, so an owner-set SERVER_PORT made Java Edition unreachable. The image already defaults to 25565; the status ping uses pingPort 25565 instead of reading the setting.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

git_ai_bot reviewed 2026-10-25 19:52:09 +00:00

git_ai_bot left a comment

Member

Copy link

Copy source

🤖 AI Code Review

Review

The three changes are consistent and correct. Removing SERVER_PORT as a user-facing setting fixes a real footgun: the itzg/minecraft-server container is published at 25565/tcp on the container side, so if a user set SERVER_PORT to anything else, the in-container server would listen on the wrong port and Java clients would be unreachable. The fix is to simply not expose a setting that can only break things.

1. Blocking issues

None.

2. Non-blocking suggestions

• Orphaned user configs. Any existing user who had explicitly set SERVER_PORT to a non-default value (e.g. 25570) will now have a stale key in their stored config that the schema no longer recognizes. Depending on how GBR handles unknown keys on re-render, this could either be silently ignored (fine) or cause a validation warning. If the latter, consider a one-time cleanup migration or at minimum a note in the release/deploy changelog. Worth verifying with the config-rendering code path.

• pingPortKey: null vs. omitting the key. I'd like to confirm the consuming code treats an explicit null the same as an absent key when falling back to the static pingPort. If the code does a truthiness check on the key's presence rather than its value, null and absent are equivalent; but if it checks key != null after a YAML map lookup that returns "" or a zero-value for missing keys, the two could diverge. A quick grep of the ping-resolution logic would confirm. (If it's already been validated by the portSettings draft check mentioned in the description, this is moot.)

3. Tests

• The repo has a games/minecraft-crossplay/test.yaml and a cmd/gbrgames/draft_test.go / boottest_test.go suite. Please confirm that:

  • The draft/validation check (which caught this bug) now passes with the schema no longer declaring SERVER_PORT.
  • The boottest still passes, since it presumably starts a real container and pings port 25565.
  • No test asserts the presence of SERVER_PORT in the rendered config or the schema.

  These are likely already covered since the PR was "found by the portSettings draft check," but a green CI run on this branch would close the loop.

4. Overall assessment

Clean, minimal, well-motivated fix. The three-line change correctly removes a setting that could only break connectivity and that the upstream Docker image already defaults appropriately. Ship it.

---

Automated review by AI Git Bot

## 🤖 AI Code Review ## Review The three changes are consistent and correct. Removing `SERVER_PORT` as a user-facing setting fixes a real footgun: the `itzg/minecraft-server` container is published at `25565/tcp` on the container side, so if a user set `SERVER_PORT` to anything else, the in-container server would listen on the wrong port and Java clients would be unreachable. The fix is to simply not expose a setting that can only break things. ### 1. Blocking issues None. ### 2. Non-blocking suggestions - **Orphaned user configs.** Any existing user who had explicitly set `SERVER_PORT` to a non-default value (e.g. `25570`) will now have a stale key in their stored config that the schema no longer recognizes. Depending on how GBR handles unknown keys on re-render, this could either be silently ignored (fine) or cause a validation warning. If the latter, consider a one-time cleanup migration or at minimum a note in the release/deploy changelog. Worth verifying with the config-rendering code path. - **`pingPortKey: null` vs. omitting the key.** I'd like to confirm the consuming code treats an explicit `null` the same as an absent key when falling back to the static `pingPort`. If the code does a truthiness check on the key's *presence* rather than its value, `null` and absent are equivalent; but if it checks `key != null` after a YAML map lookup that returns `""` or a zero-value for missing keys, the two could diverge. A quick grep of the ping-resolution logic would confirm. (If it's already been validated by the `portSettings` draft check mentioned in the description, this is moot.) ### 3. Tests - The repo has a `games/minecraft-crossplay/test.yaml` and a `cmd/gbrgames/draft_test.go` / `boottest_test.go` suite. Please confirm that: - The draft/validation check (which caught this bug) now passes with the schema no longer declaring `SERVER_PORT`. - The boottest still passes, since it presumably starts a real container and pings port 25565. - No test asserts the presence of `SERVER_PORT` in the rendered config or the schema. These are likely already covered since the PR was "found by the `portSettings` draft check," but a green CI run on this branch would close the loop. ### 4. Overall assessment Clean, minimal, well-motivated fix. The three-line change correctly removes a setting that could only break connectivity and that the upstream Docker image already defaults appropriately. Ship it. --- *Automated review by AI Git Bot*

greg added 1 commit 2026-10-26 00:26:19 +00:00

Merge branch 'main' into fix/port-minecraft-crossplay 68c6a9c281

git_ai_bot reviewed 2026-10-26 00:26:57 +00:00

git_ai_bot left a comment

Member

Copy link

Copy source

🤖 AI Code Review

Review of updated PR

The diff is the same three changes as before, now confirmed against the full file content. Everything checks out.

1. Blocking issues

None.

2. Non-blocking suggestions

• Orphaned user configs. For any existing user who explicitly set SERVER_PORT (e.g. to 25570), the key will now be absent from the schema. Since configFileFormat: INI, the rendering code will still write SERVER_PORT=25570 into the generated INI/env file if it's stored in the user's config record. In practice this is harmless because itzg/minecraft-server does not recognise SERVER_PORT as an environment variable (the port is controlled by server-port in server.properties, which the image defaults to 25565). So the stale key is a no-op at runtime. That said, if you want to be tidy, a one-liner in the config-import/render path that drops keys not present in the schema would prevent surprise. Low priority.

• pingPortKey: null vs. omitting the key. In YAML, pingPortKey: null and omitting the key are semantically equivalent (both produce a null/absent value in a Go map[string]interface{}). Just confirming the consuming Go code (likely in cmd/gbrgames/) does a nil-check and falls back to the static pingPort field. If the draft-check test passed, this is already verified — just noting for completeness.

3. Tests

• The PR description states this was caught by the portSettings draft check (cmd/gbrgames/draftchecks_test.go). Please confirm CI is green on this branch, which would exercise:

  • draft_test.go / draftchecks_test.go — schema/config consistency.
  • boottest_test.go — spins up the actual container and pings 25565.
  • games/minecraft-crossplay/test.yaml — if it asserts the presence or absence of specific config keys, it should still pass.

  If all green, no additional tests are needed for a three-line deletion.

4. Overall assessment

Correct, minimal, well-motivated fix. The old SERVER_PORT setting was a misleading no-op: itzg/minecraft-server doesn't read that env var, the port mapping in defaultPorts is fixed at 25565/tcp, and the only code path that referenced it (pingPortKey) is now correctly pinned to the static pingPort: 25565. The Xbox broadcast plugin reads its port from server.properties (always 25565 in this image), so it is unaffected. No regressions identified. Good to merge.

---

Automated review by AI Git Bot

## 🤖 AI Code Review ## Review of updated PR The diff is the same three changes as before, now confirmed against the full file content. Everything checks out. ### 1. Blocking issues None. ### 2. Non-blocking suggestions - **Orphaned user configs.** For any existing user who explicitly set `SERVER_PORT` (e.g. to `25570`), the key will now be absent from the schema. Since `configFileFormat: INI`, the rendering code will still write `SERVER_PORT=25570` into the generated INI/env file if it's stored in the user's config record. In practice this is harmless because `itzg/minecraft-server` does not recognise `SERVER_PORT` as an environment variable (the port is controlled by `server-port` in `server.properties`, which the image defaults to 25565). So the stale key is a no-op at runtime. That said, if you want to be tidy, a one-liner in the config-import/render path that drops keys not present in the schema would prevent surprise. Low priority. - **`pingPortKey: null` vs. omitting the key.** In YAML, `pingPortKey: null` and omitting the key are semantically equivalent (both produce a null/absent value in a Go `map[string]interface{}`). Just confirming the consuming Go code (likely in `cmd/gbrgames/`) does a nil-check and falls back to the static `pingPort` field. If the draft-check test passed, this is already verified — just noting for completeness. ### 3. Tests - The PR description states this was caught by the `portSettings` draft check (`cmd/gbrgames/draftchecks_test.go`). Please confirm CI is green on this branch, which would exercise: - `draft_test.go` / `draftchecks_test.go` — schema/config consistency. - `boottest_test.go` — spins up the actual container and pings 25565. - `games/minecraft-crossplay/test.yaml` — if it asserts the presence or absence of specific config keys, it should still pass. If all green, no additional tests are needed for a three-line deletion. ### 4. Overall assessment Correct, minimal, well-motivated fix. The old `SERVER_PORT` setting was a misleading no-op: `itzg/minecraft-server` doesn't read that env var, the port mapping in `defaultPorts` is fixed at `25565/tcp`, and the only code path that referenced it (`pingPortKey`) is now correctly pinned to the static `pingPort: 25565`. The Xbox broadcast plugin reads its port from `server.properties` (always 25565 in this image), so it is unaffected. No regressions identified. Good to merge. --- *Automated review by AI Git Bot*

greg merged commit dccbaa6d3d into main 2026-10-26 00:34:57 +00:00

greg deleted branch fix/port-minecraft-crossplay 2026-10-26 00:34:57 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

git_ai_bot

Labels

No items

No labels

Milestone

No items

No Milestone

Projects

Clear projects

No projects

Assignees

Clear assignees

gbr-bot git_ai_bot git_pipeline greg (Gregory Reed) openhands

No Assignees

2 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gbreed-public/gbr-games#8