fix(appimage): cd "$APPDIR" in AppRun before exec to fix sharun preload CWD resolution#2829
Conversation
…arun preload CWD resolution (tinyhumansai#2822) sharun resolves its --preload argument and library search paths relative to the process CWD rather than the AppDir. When a user launches OpenHuman_0.56.0_amd64.AppImage from any directory other than the AppDir (e.g. double-click from ~/Downloads), ld.so cannot find anylinux.so or libcef.so even though SHARUN_DIR is set correctly. Fix: add a new `patch_apprun_sharun_cwd` function to scripts/release/strip-appimage-graphics-libs.sh that injects `cd "$APPDIR" && ` before the `exec "$@"` line in the AppRun shell script during the post-build repackaging step. This guarantees that CWD == AppDir by the time sharun runs, so relative preload/library paths resolve correctly regardless of where the user launches from. The patch: - Only applies when the AppImage uses a sharun launcher (detected by the existing `uses_sharun_launcher` guard). - Skips ELF-binary AppRun entries (cannot be sed-patched). - Is idempotent (skips if `cd "$APPDIR"` is already present). - Emits a clear warning if the exec line pattern is not found rather than silently missing the fix. - Triggers a repack of the AppImage (same path as the loader/lib.path fixes) and is re-signed alongside other mutations. Verified workaround from the bug report: sed -i 's|^ exec "\$@"| cd "$APPDIR" \&\& exec "$@"|' squashfs-root/AppRun This commit generalises that one-liner into the release pipeline so future releases ship with the fix pre-applied.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds an idempotent AppRun patch for sharun-based AppImages: new patch_apprun_sharun_cwd injects ChangesAppRun CWD Patching for Sharun
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/release/strip-appimage-graphics-libs.sh`:
- Around line 369-372: The current idempotency guard uses a too-broad grep
('cd.*"$APPDIR"') against the AppRun file (variable apprun), which can match
comments or unrelated lines and cause false positives leaving exec "$@"
unpatched; tighten the check to look for an exact cd invocation (e.g. anchored
line allowing only whitespace, optional sudo, and optional && or || constructs
around cd "$APPDIR") so you only treat a file as already patched when a real cd
"$APPDIR" command is present, update the grep/regex used in the block that
references apprun and replicate the same stricter check at the analogous
location referenced in the comment (the occurrence around line 385) so both
idempotency and post-write checks are robust.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0417ed6f-d418-444d-a56d-5f1c2b17f7a3
📒 Files selected for processing (1)
scripts/release/strip-appimage-graphics-libs.sh
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Summary
This PR fixes a real problem — AppImage failing to launch from directories other than the AppDir. The solution (injecting cd "$APPDIR" in AppRun before exec) is sound and well-implemented.
However, CodeRabbit flagged a correctness issue with the grep patterns used for idempotency and validation. The patterns are too loose and could match false positives (e.g., comments containing the string). This needs to be tightened.
Key Issues
[major] lines 369, 385: Idempotency and post-write grep patterns too loose
The pattern cd.*"\$APPDIR" can match comments, unrelated lines, or incomplete commands. If someone adds a comment like # cd "$APPDIR" to AppRun, the script will incorrectly think it's already patched and skip the fix. Similarly, the post-write validation could pass even if the sed only partially matched.
Suggestion: Tighten to match the complete command:
- Idempotency check:
grep -Eq '^[[:space:]]*cd[[:space:]]+"\$APPDIR"[[:space:]]*&&[[:space:]]*exec[[:space:]]+"\$@"[[:space:]]*$'— only match if the exact pattern exists - sed pattern: anchor to end-of-line:
's|^\([[:space:]]*\)exec "\$@"$|\1cd "$APPDIR" \&\& exec "\$@"|'— avoid issues with trailing whitespace - Post-write validation: use the same stricter grep
[minor] line 363: Shellcheck warning (SC2016) — expressions don't expand in single quotes
Using single quotes in the comment means the $APPDIR example doesn't render. Minor style issue, but worth fixing to avoid confusing future readers.
CI Status
One check failing: PR Submission Checklist — the N/A items need to be explicitly checked with [x], not just annotated in text. The CI is right to fail here; please update the checklist boxes even for N/A items.
Process
- Tighten the grep patterns as suggested above
- Check all PR Submission Checklist items (even N/A ones need
[x]) - Re-run the tests
- I'll re-review and approve once both are fixed
| # typically have a line of the form (possibly with leading whitespace): | ||
| # exec "$@" | ||
| # Patch it to: | ||
| # cd "$APPDIR" && exec "$@" |
There was a problem hiding this comment.
[major] The sed pattern should anchor to end-of-line to avoid partial matches with trailing whitespace:
's|^\([[:space:]]*\)exec "\$@"$|\1cd "$APPDIR" \&\& exec "\$@"|'
Without the $, sed could match and partially corrupt lines with extra content afterward.
There was a problem hiding this comment.
Fixed in 0438d08. Anchored the sed pattern with [[:space:]]*$ so an exec line with trailing content (e.g. exec "$@" >> /tmp/log 2>&1) is left untouched and the warning branch fires, rather than partially absorbing the trailing tokens into the new cd && exec sequence. Verified with a trailing-redirect fixture.
| tmp_apprun="$(mktemp)" | ||
| if sed 's|^\([[:space:]]*\)exec "\$@"|\1cd "$APPDIR" \&\& exec "$@"|' \ | ||
| "$apprun" > "$tmp_apprun" \ | ||
| && grep -q 'cd.*"\$APPDIR"' "$tmp_apprun"; then |
There was a problem hiding this comment.
[major] Same issue as line 369 — use the stricter grep pattern here too:
grep -Eq '^[[:space:]]*cd[[:space:]]+"\$APPDIR"[[:space:]]*&&[[:space:]]*exec[[:space:]]+"\$@"[[:space:]]*$'
This post-write validation must match the actual patched pattern, not a loose substring.
There was a problem hiding this comment.
Fixed in 0438d08. Post-write validation now reuses the same $patched_line_re variable as the idempotency check, so we only consider the patch successful when the new file actually contains the canonical patched line.
…pattern Address review feedback on tinyhumansai#2829 from @graycyrus and CodeRabbit: 1. Idempotency guard used a loose substring grep (cd.*"$APPDIR") that could false-positive on comments like '# cd "$APPDIR"' or unrelated lines, leaving the real exec "$@" line unpatched. Tighten to an anchored regex matching the full patched line: ^[[:space:]]*cd[[:space:]]+"$APPDIR"[[:space:]]*&&[[:space:]]*exec[[:space:]]+"$@"[[:space:]]*$ so only a genuine patched-by-us line trips the guard. 2. sed substitution lacked an end-of-line anchor, so an exec line with trailing content (extra args, redirections, comments) would have been partially rewritten with the trailing content silently absorbed into the cd && exec sequence. Anchor the pattern to $ so we only rewrite a clean `exec "$@"` line; otherwise the warning branch fires and the AppRun is left untouched. 3. Post-write validation now uses the same stricter regex so success is only declared when the new file actually contains the expected line. Co-Authored-By: Claude <noreply@anthropic.com>
|
Pushed 0438d08 addressing both reviews:
Verified the new logic locally with four fixture AppRun files (clean exec, commented |
|
Actionable comments posted: 0 |
There was a problem hiding this comment.
Good follow-up commit. I've reviewed your changes and all three prior findings have been properly addressed:
[major] Grep pattern fix (line 407): Tightened to ^[[:space:]]*cd[[:space:]]+"\$APPDIR"[[:space:]]*&&[[:space:]]*exec[[:space:]]+"\$@"[[:space:]]*$ — exact match, no false-positives on comments.
[major] sed anchoring (line 419): Pattern now anchors to end-of-line (exec "\$@"$) to avoid partial matches.
[major] Post-write validation (line 428): Uses the same strict grep pattern to ensure the patch actually applied.
[admin] Submission Checklist: All N/A items now properly checked with [x].
Code review
Logic is sound:
- Idempotency guard prevents re-patching (critical for safety)
- Temp file + mv avoids truncation on failure
- Uses sharun launcher check + ELF binary guard to avoid touching unrelated AppRun files
- Error message + return code communicate success/failure clearly
- Integration with
strip_one_appimageis clean
All fixes verified against prior review requests.
CI Status
One check is failing: Coverage Gate (diff-cover ≥ 80%). This is expected — bash script changes don't meet coverage gates designed for TypeScript/Rust. Once pending tests complete and this false-positive is resolved (or explicitly skipped for scripts/release/), I'll come back and approve.
Let me know if you need help with the coverage gate issue or if there's a way to exempt bash release scripts from that check.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
Approving from shepherd queue (round 2). All CI green or known flakes re-running; pnpm review fix round complete. Happy to merge once required reviewers concur.
|
Hi @graycyrus — your 14:34Z comment said "Good follow-up commit. I've reviewed your changes and all three prior findings have been properly addressed" on this PR (✓ grep pattern, ✓ sed anchoring, ✓ post-write validation). Could you flip the review to APPROVE so this can merge? CI is at 30 success / 0 fail on head 0438d08, CodeRabbit APPROVED at 14:22Z. The only thing blocking merge is your prior CHANGES_REQUESTED still being sticky. |
graycyrus
left a comment
graycyrus
left a comment
There was a problem hiding this comment.
Prior changes all addressed — grep patterns are now tight, sed is anchored, validation is strict, and PR Submission Checklist properly filled. CI is green across the board. This is good to merge.
3 participants
Summary
patch_apprun_sharun_cwdfunction toscripts/release/strip-appimage-graphics-libs.shthat injectscd "$APPDIR" &&beforeexec "$@"in the AppRun shell script during post-build AppImage repackaging.strip_one_appimagealongside the existingensure_sharun_interpreter/rewrite_sharun_lib_pathfixes.~/Downloads).Problem
OpenHuman_0.56.0_amd64.AppImagefails to launch when the user's CWD is not the AppDir:SHARUN_DIRis set correctly — sharun detects the AppDir fine — but the--preloadargument and library search path it hands told.souse bare/relative paths that are resolved relative to the process CWD, notSHARUN_DIR. So when CWD ≠ AppDir,anylinux.soandlibcef.soare never found.The failure is purely a CWD mismatch: the app works if and only if the user first
cds into the AppDir before launching.Solution
AppRun(the shell script entry point) tocd "$APPDIR"immediately before theexec "$@"call. This guarantees CWD == AppDir by the time sharun runs, making all relative preload/library paths resolve correctly.strip-appimage-graphics-libs.shpost-build repackaging step — no build system changes needed.uses_sharun_launcheris true; skips ELF-binary AppRun entries; idempotent (skips ifcd "$APPDIR"already present); emits a warning (not a hard failure) if theexec "$@"pattern is not found so the script stays robust to future AppRun layout changes.The workaround reported in issue #2822:
sed -i 's|^ exec "\$@"| cd "$APPDIR" \&\& exec "$@"|' squashfs-root/AppRunThis PR generalises that one-liner into the release pipeline.
Submission Checklist
sedsubstitution with an idempotency guard, and correctness is validated by the existing AppImage smoke test in the release process.scripts/release/bash scripts.Closes #2822in the Related section.Impact
~/Downloads). No impact on macOS, Windows, or.debusers.strip-appimage-graphics-libs.shnow patches AppRun during its existing repack step. No new tools or dependencies required. Repackaged AppImages are re-signed as before.Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
pnpm --filter openhuman-app format:check— only a bash script was changedpnpm typecheck— only a bash script was changedscripts/release/bash scriptsValidation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
~/Downloads), not only when CWD == AppDir.OpenHuman_*.AppImageno longer fails withanylinux.sopreload error or missinglibcef.sowhen double-clicked or launched from a shell outside the AppDir.Parity Contract
strip-appimage-graphics-libs.shbehavior (graphics lib stripping, sharun interpreter bundling, lib.path rewriting, re-signing) is unchanged.uses_sharun_launcherguard ensures non-sharun AppImages are not touched; ELF AppRun guard preserves binary entry points; idempotency guard prevents double-patching.Duplicate / Superseded PR Handling
Summary by CodeRabbit