Review mirror: Linux-hosted build, authoring tools, and server workflow#1
Review mirror: Linux-hosted build, authoring tools, and server workflow#1LAP87 wants to merge 5 commits into
Conversation
|
@codex review Please perform a deep, adversarial, whole-PR review of this Linux port. Treat this as release-critical mod-development infrastructure, not a style pass. Inspect both commits and follow data/control flow across C++, CMake/shell, GitHub Actions, the schema registry/validator, LSP, and VS Code extension. Primary acceptance goal: a Linux host must be able to build and package the same Win64 PalSchema UE4SS C++ mod used on Windows, safely deploy/rollback it for the Palworld client or Win64 Dedicated Server under Proton/Wine, and use the new authoring tools across major Linux distributions. UE4SS, UEPseudo, Palworld-derived data, and proxy DLLs must never be bundled. Please try hard to find concrete correctness, security, portability, ABI, lifecycle, race, and false-confidence issues. In particular:
For each finding, provide severity, exact file/line, a realistic failure scenario, and the smallest robust fix. Distinguish blockers from follow-ups. Do not report speculative style preferences. If an issue depends on RE-UE4SS or Palworld behavior, inspect the pinned code/config in this PR and state the assumption precisely. Also identify missing tests that would have caught each issue. End with a clear merge recommendation and a list of claims you were able to independently substantiate. |
![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: 0e7cece557
ℹ️ 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".
|
@codex review Please perform a fresh, adversarial re-review of current HEAD The first pass reported 23 findings. Explicitly verify, with file-and-line evidence, that the new code closes every category: fail-closed Then try to break the remediations themselves. Treat these as trust boundaries and attack them:
Important context to avoid false positives: private-submodule Windows and Linux-xwin jobs are intentionally restricted to trusted pushes to Report only concrete P0/P1/P2 defects that remain on this exact HEAD. For every finding, include a reproducible failure path and explain why existing tests do not catch it. If a previous finding is fully fixed, do not repeat it. If no actionable defects remain, say that plainly and state whether the 23-finding remediation is complete. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2436fd4fa
ℹ️ 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".
|
@codex review Please perform a fresh, adversarial review of current HEAD First verify the actual fixes for all prior findings:
Regression coverage was added for stale generated-schema refresh, source symlink exfiltration, malformed/canonical index metadata, watch failure exit status, stale and bounded LSP diagnostics, independent multi-root startup, relocated PE flag decoys, decoded/fixed VERSIONINFO drift, verifier output aliases, markerless xwin caches, isolated Rust selection, nested backup symlinks, the process-start deployment race, and relative server command lines. Attack the new implementation rather than trusting this summary. In particular try to find:
Only report concrete P0/P1/P2 defects in the current diff. For every finding, provide the exact reachable path/reproduction and explain why existing tests miss it. If no actionable issue remains, say so clearly and react with 👍. |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f6fdefed7
ℹ️ 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(DEFINED ENV{XDG_CACHE_HOME} AND NOT "$ENV{XDG_CACHE_HOME}" STREQUAL "") | ||
| set(_palschema_xwin_dir "$ENV{XDG_CACHE_HOME}/palschema/xwin") | ||
| elseif(DEFINED ENV{HOME} AND NOT "$ENV{HOME}" STREQUAL "") | ||
| set(_palschema_xwin_dir "$ENV{HOME}/.cache/palschema/xwin") |
There was a problem hiding this comment.
When PALSCHEMA_CACHE_ROOT is overridden without also setting XWIN_DIR, palschema_configure_build_environment and bootstrap-linux.sh validate the SDK under $PALSCHEMA_CACHE_ROOT/xwin, but this toolchain independently selects $XDG_CACHE_HOME/palschema/xwin or $HOME/.cache/palschema/xwin. Consequently, scripts/build-linux.sh can pass its bootstrap check and then configure against a different, missing SDK directory. Derive the toolchain default from PALSCHEMA_CACHE_ROOT as well, or export the bootstrap-selected XWIN_DIR before invoking CMake.
Useful? React with 👍 / 👎.
| @@ -0,0 +1,73 @@ | |||
| { | |||
| "version": 6, | |||
There was a problem hiding this comment.
The project and build documentation advertise CMake 3.22, but the CMake presets manual lists preset schema version 6 as added in CMake 3.25. On a supported 3.22–3.24 installation, every documented cmake --preset ... command therefore fails while reading this file, before cmakeMinimumRequired or the project can be evaluated. Use a preset schema supported by 3.22 or consistently require CMake 3.25 or newer.
Useful? React with 👍 / 👎.
| configure_only=false | ||
|
|
||
| if (($# > 0)); then | ||
| shift | ||
| fi | ||
|
|
There was a problem hiding this comment.
Because the first argument is always assigned to build_flavor and shifted, the advertised default flavor cannot be used together with an option: scripts/build-linux.sh --configure-only treats the flag as an unknown flavor. The same parsing pattern in package-linux.sh makes scripts/package-linux.sh --output-dir dist fail even though its usage declares the flavor optional and initializes a Shipping default. Only consume the first token when it is dev or shipping, or parse flavor and options in one loop.
Useful? React with 👍 / 👎.
|
|
||
| if [[ "$incomplete" == true ]]; then | ||
| return 2 | ||
| fi | ||
| return 1 | ||
| } |
There was a problem hiding this comment.
On a host where /proc is mounted with hidepid=2, processes owned by other users are omitted from the glob entirely rather than producing an unreadable cmdline, so this function returns 1 and deploy-proton.sh interprets the scan as complete. If a dedicated server runs under a service account while deployment is performed by another account with write access to UE4SS, the script can therefore replace the live mod despite its stopped-process guard. Detect a /proc configuration that hides PIDs and fail closed, or otherwise establish that the scan has complete process visibility.
Useful? React with 👍 / 👎.
1 participant
Purpose
This is the Codex-review mirror for the Linux port submitted upstream as Okaetsu/PalSchema#126. Both PRs point at current head
8f6fdefed7117e34c46d65216540fe8ec0e02172and are ready for maintainer review.Delivered scope
Review hardening
Two adversarial Codex rounds produced 44 review threads in total. All threads are resolved, each actionable finding has regression coverage where practical, and the final review of
8f6fdefreported: “Didn't find any major issues. Nice work!”Hardening includes fail-closed
/procscanning, atomic live-mod exchange, fsynced transaction/cache publication, recursive symlink containment, staged schema-pack replacement, static schema checksums, decoded PE resources and fixed version identity, output-alias protection, pinned xwin and MSVC action versions, credential-free PR checkout, serialized/isolated VS Code lifecycle, stale-diagnostic suppression, diagnostic bounds, canonical schema IDs and destructive copy-overlap guards.Verification
mainpushesRuntime boundary
UE4SS remains a separate user-managed runtime installation. This repository does not bundle UE4SS, UEPseudo, Microsoft SDK payloads or generated Palworld game data.
The verified Linux-host path still produces a Win64 DLL for Palworld under Wine/Proton. Downstream native Linux UE4SS experiments may accept
CppUserModBase/main.soat the loader boundary, but PalSchema's native Linux signatures, layouts, hooks, threading and teardown have not been pinned and verified; nativePalServer-Linux-Shippingparity is therefore not claimed here.