gbreed-public/gbr-games
5
0 Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(palworld): announce the published port, don't make it a setting #9

Merged
greg merged 2 commits from fix/port-palworld into main 2026-10-26 00:40:34 +00:00
Conversation 2 Commits 2 Files Changed 1
+3 -7
greg commented 2026-10-25 19:50:36 +00:00
Owner
Copy link
Copy source

Nodes always publish container port 8211/udp and only move the host side when servers share a node. PublicPort was an editable setting, so an owner could announce a port nobody can reach, and on a shared node the default 8211 announced the wrong one.

  • PublicPort is now '{PUBLIC_PORT_8211}' in defaultConfig (the port players actually reach) and is no longer in configSchema.
  • PublicPort is in unquotedKeys, so the INI gets a number, not a quoted string.
  • The startup command binds -port=8211 directly.

Note: servers whose stored config already holds PublicPort: 8211 keep that value, since stored config overrides defaults.

Found by the portSettings draft check.

🤖 Generated with Claude Code

Nodes always publish container port `8211/udp` and only move the host side when servers share a node. `PublicPort` was an editable setting, so an owner could announce a port nobody can reach, and on a shared node the default 8211 announced the wrong one. - `PublicPort` is now `'{PUBLIC_PORT_8211}'` in `defaultConfig` (the port players actually reach) and is no longer in `configSchema`. - `PublicPort` is in `unquotedKeys`, so the INI gets a number, not a quoted string. - The startup command binds `-port=8211` directly. Note: servers whose stored config already holds `PublicPort: 8211` keep that value, since stored config overrides defaults. 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(palworld): announce the published port, don't make it a setting
Validate game templates / validate (push) Successful in 31s
Details
Validate game templates / boot-test (push) Successful in 3m45s
Details
Validate game templates / publish (push) Skipped
Details
f6c2bda86a 
Nodes always publish container port 8211 and only move the host side. PublicPort (what the community server list announces) is now {PUBLIC_PORT_8211}, written unquoted, and the server binds -port=8211 directly.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
git_ai_bot left a comment
Member
Copy link
Copy source

🤖 AI Code Review

Review

1. Blocking issues

None. The change is internally consistent and the reasoning is sound.

2. Non-blocking suggestions

  • Stored-config staleness (acknowledged in the PR, but worth tracking): As you note, existing deployments whose saved config already has PublicPort: 8211 will keep that value even on shared nodes where the published port differs. Since the key is now outside configSchema, the user has no UI path to correct it either. Consider whether a one-time migration (or a platform-level override that always injects the resolved PUBLIC_PORT_8211 into the rendered INI regardless of stored state) is warranted. If that's intentionally left to a follow-up, a tracking issue would prevent it from being lost.

  • RCONPort / RESTAPIPort in unquotedKeys: You added PublicPort because its value arrives as a template-substituted string. RCONPort: 25575 and RESTAPIPort: 8212 are plain integers in defaultConfig and presumably serialize unquoted already, so they're fine today. But if anyone ever changes them to template placeholders (e.g. {RCON_PORT_25575}), they'd hit the same quoting issue. A brief comment on the unquotedKeys list explaining why entries are there (template-resolved strings vs. native integers) could save a future reviewer the investigation.

  • Startup command port hardcoding is correct, but fragile if the container port ever changes: -port=8211 and defaultPorts: 8211:8211/udp and the PUBLIC_PORT_8211 placeholder all independently reference 8211. If the container-internal port ever needs to change (e.g. Palworld updates), all three spots must be updated together. Not something to fix now, but a short comment near defaultPorts noting the coupling could help.

3. Tests

  • The PR description mentions the change was surfaced by the portSettings draft check. Confirm that check now validates:
    • PublicPort is absent from configSchema (so it can't be user-edited).
    • PublicPort is present in defaultConfig with a PUBLIC_PORT_* template placeholder.
    • PublicPort is listed in unquotedKeys.
    • The startup command binds the container-internal port that matches defaultPorts.
  • If the draft.go / draft_test.go logic is what enforces this, a regression test asserting all four of the above for Palworld would guard against a future refactor silently dropping one of the pieces.

4. Overall assessment

Clean, well-scoped fix. Removing a user-editable setting that could only cause confusion (announcing an unreachable or wrong port) in favor of a platform-resolved template is the right call. The unquotedKeys addition prevents a subtle INI serialization bug. No blocking concerns — merge after confirming the draft-check test covers the four invariants listed above.

Automated review by AI Git Bot

## 🤖 AI Code Review ## Review ### 1. Blocking issues None. The change is internally consistent and the reasoning is sound. ### 2. Non-blocking suggestions - **Stored-config staleness (acknowledged in the PR, but worth tracking):** As you note, existing deployments whose saved config already has `PublicPort: 8211` will keep that value even on shared nodes where the published port differs. Since the key is now outside `configSchema`, the user has no UI path to correct it either. Consider whether a one-time migration (or a platform-level override that always injects the resolved `PUBLIC_PORT_8211` into the rendered INI regardless of stored state) is warranted. If that's intentionally left to a follow-up, a tracking issue would prevent it from being lost. - **`RCONPort` / `RESTAPIPort` in `unquotedKeys`:** You added `PublicPort` because its value arrives as a template-substituted string. `RCONPort: 25575` and `RESTAPIPort: 8212` are plain integers in `defaultConfig` and presumably serialize unquoted already, so they're fine today. But if anyone ever changes them to template placeholders (e.g. `{RCON_PORT_25575}`), they'd hit the same quoting issue. A brief comment on the `unquotedKeys` list explaining *why* entries are there (template-resolved strings vs. native integers) could save a future reviewer the investigation. - **Startup command port hardcoding is correct, but fragile if the container port ever changes:** `-port=8211` and `defaultPorts: 8211:8211/udp` and the `PUBLIC_PORT_8211` placeholder all independently reference `8211`. If the container-internal port ever needs to change (e.g. Palworld updates), all three spots must be updated together. Not something to fix now, but a short comment near `defaultPorts` noting the coupling could help. ### 3. Tests - The PR description mentions the change was surfaced by the `portSettings` draft check. Confirm that check now validates: - `PublicPort` is **absent** from `configSchema` (so it can't be user-edited). - `PublicPort` is **present** in `defaultConfig` with a `PUBLIC_PORT_*` template placeholder. - `PublicPort` is listed in `unquotedKeys`. - The startup command binds the container-internal port that matches `defaultPorts`. - If the `draft.go` / `draft_test.go` logic is what enforces this, a regression test asserting all four of the above for Palworld would guard against a future refactor silently dropping one of the pieces. ### 4. Overall assessment Clean, well-scoped fix. Removing a user-editable setting that could only cause confusion (announcing an unreachable or wrong port) in favor of a platform-resolved template is the right call. The `unquotedKeys` addition prevents a subtle INI serialization bug. No blocking concerns — merge after confirming the draft-check test covers the four invariants listed above. --- *Automated review by AI Git Bot*
greg added 1 commit 2026-10-26 00:34:58 +00:00
Merge branch 'main' into fix/port-palworld
Validate game templates / validate (push) Successful in 23s
Details
Validate game templates / boot-test (push) Successful in 3m47s
Details
Validate game templates / publish (push) Skipped
Details
b1fb77f284
git_ai_bot left a comment
Member
Copy link
Copy source

🤖 AI Code Review

Review

1. Blocking issues

None. The changes are correct and internally consistent.

2. Non-blocking suggestions

  • Stored-config override (acknowledged in PR description): You note that existing deployments with PublicPort: 8211 in their saved config will retain that value, overriding the '{PUBLIC_PORT_8211}' template. On non-shared nodes this is harmless (published port is 8211). On shared nodes it will announce the wrong port, and since the key is no longer in configSchema the user has no UI path to fix it. If the platform's config-merge logic always applies saved values over defaults, consider whether a one-time cleanup job (or a forced template re-injection for this key) is needed. If that's intentionally deferred, a tracking issue would be good.

  • Coupling between -port=8211, defaultPorts, and the PUBLIC_PORT_8211 placeholder name: All three reference 8211 independently. If the container-internal port ever changes, all three must be updated together. A brief inline comment near defaultPorts like # container port; also referenced in startup command and PUBLIC_PORT_8211 would save a future maintainer some grep. Optional.

3. Tests

The PR description mentions the portSettings draft check surfaced this. Worth confirming that draftchecks_test.go (or equivalent) now asserts all four invariants for Palworld:

  1. PublicPort is absent from configSchema.properties.
  2. PublicPort in defaultConfig matches the pattern '{PUBLIC_PORT_<containerport>}' where <containerport> equals the first element of defaultPorts.
  3. PublicPort appears in configFormatOptions.unquotedKeys.
  4. defaultStartupCommand contains -port=<containerport> matching defaultPorts.

If the existing draft check only validates a subset, adding the missing assertions would prevent a future refactor from silently breaking one leg of the chain.

4. Overall assessment

Clean, minimal, well-reasoned fix. Removing a user-editable port setting that could only announce an unreachable address, replacing it with a platform-resolved template, and ensuring correct INI serialization via unquotedKeys — all correct. The -port=8211 startup flag correctly pins the container-internal listen port. No blocking concerns; ready to merge once the stored-config migration question is consciously decided (even if the decision is "handle in a follow-up").

Automated review by AI Git Bot

## 🤖 AI Code Review ## Review ### 1. Blocking issues None. The changes are correct and internally consistent. ### 2. Non-blocking suggestions - **Stored-config override (acknowledged in PR description):** You note that existing deployments with `PublicPort: 8211` in their saved config will retain that value, overriding the `'{PUBLIC_PORT_8211}'` template. On non-shared nodes this is harmless (published port *is* 8211). On shared nodes it will announce the wrong port, and since the key is no longer in `configSchema` the user has no UI path to fix it. If the platform's config-merge logic always applies saved values over defaults, consider whether a one-time cleanup job (or a forced template re-injection for this key) is needed. If that's intentionally deferred, a tracking issue would be good. - **Coupling between `-port=8211`, `defaultPorts`, and the `PUBLIC_PORT_8211` placeholder name:** All three reference `8211` independently. If the container-internal port ever changes, all three must be updated together. A brief inline comment near `defaultPorts` like `# container port; also referenced in startup command and PUBLIC_PORT_8211` would save a future maintainer some grep. Optional. ### 3. Tests The PR description mentions the `portSettings` draft check surfaced this. Worth confirming that `draftchecks_test.go` (or equivalent) now asserts all four invariants for Palworld: 1. `PublicPort` is **absent** from `configSchema.properties`. 2. `PublicPort` in `defaultConfig` matches the pattern `'{PUBLIC_PORT_<containerport>}'` where `<containerport>` equals the first element of `defaultPorts`. 3. `PublicPort` appears in `configFormatOptions.unquotedKeys`. 4. `defaultStartupCommand` contains `-port=<containerport>` matching `defaultPorts`. If the existing draft check only validates a subset, adding the missing assertions would prevent a future refactor from silently breaking one leg of the chain. ### 4. Overall assessment Clean, minimal, well-reasoned fix. Removing a user-editable port setting that could only announce an unreachable address, replacing it with a platform-resolved template, and ensuring correct INI serialization via `unquotedKeys` — all correct. The `-port=8211` startup flag correctly pins the container-internal listen port. No blocking concerns; ready to merge once the stored-config migration question is consciously decided (even if the decision is "handle in a follow-up"). --- *Automated review by AI Git Bot*
greg merged commit d1505991e4 into main 2026-10-26 00:40:34 +00:00
greg deleted branch fix/port-palworld 2026-10-26 00:40:34 +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
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gbreed-public/gbr-games#9