fix(installer): harden Linux AppImage installs#13
Conversation
|
@codex review |
1 similar comment
|
@codex review |
![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: 81943fd569
ℹ️ 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".
| printf ' if command -v ldconfig >/dev/null 2>&1 && ldconfig -p 2>/dev/null | grep -q '"'"'libfuse[.]so[.]2'"'"'; then\n' | ||
| printf ' return 0\n' | ||
| printf ' fi\n' | ||
| printf ' command -v fusermount >/dev/null 2>&1\n' |
There was a problem hiding this comment.
On FUSE3-only distros that still expose a fusermount compatibility name, this returns success even though libfuse.so.2 is absent; Debian's fuse3 package publishes fusermount(1) as fusermount3 (https://manpages.debian.org/unstable/fuse3/fusermount.1.en.html), and upstream libfuse 3 renamed the helper to fusermount3 for co-installation (https://github.com/libfuse/libfuse/blob/master/ChangeLog.rst#L575). In that environment the wrapper execs the AppImage without APPIMAGE_EXTRACT_AND_RUN, so the new FUSE3 fallback is bypassed and the install still fails on the systems it is meant to support. Only skip the fallback when the libfuse2 library is actually present, or after a real AppImage probe succeeds.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf74eff: the wrapper now requires libfuse2 before skipping APPIMAGE_EXTRACT_AND_RUN; fusermount alone no longer bypasses the fallback.
| if [ -n "${PICKGAUGE_RELEASE_API_URL:-}" ]; then | ||
| printf '%s\n' "$PICKGAUGE_RELEASE_API_URL" |
There was a problem hiding this comment.
When PICKGAUGE_RELEASE_API_URL is set together with GITHUB_TOKEN, this override is passed to fetch_stdout, which unconditionally adds the GitHub Authorization header. That means anyone using the override for a local mirror/test endpoint can leak their GitHub token to a non-GitHub host, whereas before the token only went to the hard-coded GitHub API URL. Gate the auth header to trusted GitHub API URLs or make the override bypass token forwarding.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf74eff: release API overrides no longer receive the GitHub Authorization header, with smoke coverage for the token guard.
| "smoke:auth-profile": "node scripts/validate-playwright-authenticated-profile.mjs", | ||
| "smoke:preflight": "node scripts/collect-smoke-preflight.mjs", | ||
| "prepare:sidecar": "node scripts/prepare-playwright-sidecar.mjs", | ||
| "test:installer": "node tests/install-script-smoke.mjs", |
There was a problem hiding this comment.
This adds test:installer, but the checked CI path still runs only bun run test in .github/workflows/ci.yml lines 61-62, and the test script does not call the new installer smoke test. As a result, these AppImage upgrade and desktop-integration checks will not run on PRs or pushes unless a developer remembers the separate command, so future installer regressions can land without CI coverage.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf74eff: bun run test now runs the installer smoke before the existing Vitest/sidecar checks, so CI covers it.
| printf 'script_dir=$(CDPATH= cd -- "$(dirname -- "$0")" && pwd -P) || exit 1\n' | ||
| printf 'appimage_path="$script_dir/%s.AppImage"\n' "$APP_NAME" |
There was a problem hiding this comment.
When the installed pickgauge wrapper is launched through another symlink, such as a user linking it into a directory that is already on PATH, $0 points at that symlink and script_dir becomes the symlink's directory. The wrapper then looks for PickGauge.AppImage next to the symlink and exits 127, whereas the previous symlink-to-AppImage install still worked in this setup. Resolve the wrapper file before deriving appimage_path, or write the absolute installed AppImage path into the wrapper.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in cf74eff: the generated wrapper embeds the absolute installed AppImage path instead of deriving it from $0.
ElbertePlinio
force-pushed
the
fix/reliable-linux-installer
branch
from
81943fd to
cf74eff
Compare
|
@codex review |
|
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". |
1 participant
Changed
Validated
bun run test:installerbun run checkbun run testsh -n scripts/install.shgit diff --checkNot tested