Skip to content

fix(linux): isolate host processes from AppImage libraries - #273

Merged
Vect0rM merged 2 commits into
AtomicBot-ai:mainfrom
xDenside:codex/fix-appimage-host-env
Sep 7, 2026
Merged

fix(linux): isolate host processes from AppImage libraries#273
Vect0rM merged 2 commits into
AtomicBot-ai:mainfrom
xDenside:codex/fix-appimage-host-env

Conversation

@xDenside

@xDenside xDenside commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Describe Your Changes

  • Sanitize AppImage runtime variables before Atomic Chat launches host executables. This prevents Fedora's system curl and other host tools from loading the Ubuntu 22.04 libssl.so.3 bundled inside the AppImage.
  • Apply the shared cleanup to Launch-page installers and terminals, MCP stdio servers, and Agent shell / PTY processes. The cleanup is gated on APPIMAGE or APPDIR, so source and development runs preserve intentional environment overrides.
  • Run every Unix curl | shell agent installer through Bash with pipefail, including the current Atomic Agent installer, so a failed download cannot be reported as a successful install.
  • Reuse the existing AppImage restart variable list and record the host-process boundary policy in an ADR.

This replaces the older closed draft #229. It ports the original diagnosis to current main, covers Atomic Agent (added after that draft), covers Agent shell / PTY processes, and avoids the draft's post-install re-detection change because the memoized login-shell PATH can legitimately remain stale until restart.

Fixes Issues

Verification

  • cargo check --manifest-path src-tauri/Cargo.toml --lib
  • cargo test --manifest-path src-tauri/Cargo.toml --lib core::process_env::tests — 3 passed
  • cargo test --manifest-path src-tauri/Cargo.toml --lib core::system::commands::tests — 7 passed
  • cargo clippy --manifest-path src-tauri/Cargo.toml --lib — passes; existing unrelated warnings remain
  • Targeted rustfmt --check for every changed Rust file
  • git diff --check

Self Checklist

  • Added relevant comments, especially around the AppImage/host-process boundary
  • Updated docs with an engineering decision record
  • Reused the existing issue and closed draft rather than creating a duplicate

Vect0rM commented Sep 6, 2026

Copy link
Copy Markdown
Member

Thanks for picking this back up, @xDenside — and for porting the diagnosis forward rather than reviving #229 as-is. Deliberately dropping the draft's post-install re-detection change, with the reason stated (the memoised login-shell PATH can legitimately stay stale until restart), is the part I'd have worried about most; good that it's called out rather than quietly carried along.

The three-flavour helper set is the right shape. std::process::Command, tokio::process::Command and portable_pty::CommandBuilder have genuinely different env models, and a single generic wrapper would have papered over that.

Verified on your branch merged onto current main (merges cleanly):

  • cargo check --lib — exit 0. (One new warning, see below.)
  • cargo test --lib core::process_env::tests — 3 passed.
  • cargo test --lib core::system::commands::tests — 7 passed, including the new curl_agent_installers_propagate_download_failures.
  • Full cargo test --lib — 817 passed, 2 failed. Both failures are core::agent::tools::skill_run_script::tests::{cancellation,timeout}_terminates_descendant_processes, and both fail identically on main in my container — process-group signalling doesn't behave here. Pre-existing and environmental, not yours.
  • rustfmt --check — clean on all six changed files.

One thing I checked rather than took on trust, because it's the load-bearing claim of the whole PR and your test can't actually prove it. sanitizes_pty_commands sets the variables on the builder first and then removes them, so it would pass even if env_remove merely cleared a pending override without suppressing an inherited variable. I read portable-pty 0.9.0 to settle it: CommandBuilder::new() seeds envs from a full std::env::vars_os() snapshot (get_base_env, each entry flagged is_from_base_env), and as_command() does env_clear() and then applies that map wholesale. So env_remove really does remove the variable from the spawned child, and the PTY path is genuinely fixed. The mechanism is right — the test just doesn't demonstrate it. Worth strengthening it to construct the builder without pre-setting the vars, so it pins the behaviour that matters.

Three things, one of which needs a diff.

1. New compiler warning

warning: unused import: `APPIMAGE_RUNTIME_ENV_VARS`
  --> src/core/system/commands.rs:15:60

main emits zero unused-import warnings; your branch emits this one on a plain Linux --lib check. The import is gated #[cfg(any(target_os = "linux", test))], but after the move APPIMAGE_RUNTIME_ENV_VARS is only referenced inside mod tests (commands.rs:5493), while strip_appimage_std_command is used in non-test code — so the two names no longer share a cfg. Splitting them fixes it:

#[cfg(any(target_os = "linux", test))]
use crate::core::process_env::strip_appimage_std_command;
#[cfg(test)]
use crate::core::process_env::APPIMAGE_RUNTIME_ENV_VARS;

Your verification list says clippy passes with "existing unrelated warnings" — this one is new, so it looks like it got counted as pre-existing.

2. The ADR states a policy; the code enforces it at roughly half the boundaries

This is the one worth a conversation, and I'm not asking you to widen the PR unilaterally.

You cover Launch-page installers and terminals, MCP stdio, and the agent shell and PTY. But these all resolve a host executable from PATH and spawn it with no sanitize_* call:

core/agent/tools/git.rs:17                    Command::new("git")
core/agent/tools/media.rs:62,227,284,285      whisper, yt-dlp, ffmpeg  (require_binary over $PATH)
core/agent/tools/skill_run_script.rs:369      user skill scripts via host interpreters
core/agent/tools/clipboard.rs:18,25           wl-paste, xclip
core/system/commands.rs:250,271               xdg-open
core/system/commands.rs:663                   the `which` probe

Two of those are the same failure you're fixing, not analogues of it. git links the system libcurl and OpenSSL — that is precisely "Fedora's binary meets Ubuntu's bundled libssl.so.3", just reached through the agent's git tool instead of the installer. And yt-dlp is Python, with PYTHONHOME and PYTHONPATH both on your strip list: handing a host yt-dlp the AppImage's Python roots doesn't subtly misbehave, it fails outright. skill_run_script has the same exposure for any Python or Perl skill.

The reason I'd rather not merge this as-is isn't the individual gaps — the PR is strictly better than main either way. It's that the ADR records a general host-process boundary policy, and a policy enforced in four places out of ten is the kind that rots: the next person adds a spawn site, sees no rule being consistently applied, and doesn't call the helper. Either extend the coverage, or keep the scope and have the ADR name the boundaries that are deliberately still unsanitised, with a follow-up issue. Your call which — just make it explicit.

3. Question: does your AppRun also prefix PATH?

PATH is deliberately absent from APPIMAGE_RUNTIME_ENV_VARS, which is right for apply_login_path — it overwrites PATH from the login shell anyway. But MCP stdio, the agent shell and the PTY get sanitised without apply_login_path, so they keep whatever PATH the AppImage runtime handed them. If AppRun prepends $APPDIR/usr/bin (the common template does), those processes still resolve bundled binaries ahead of host ones, and the library isolation you've just added is partly undone by the lookup order.

The AppRun is generated by tauri-bundler, so it isn't in the repo and I couldn't check it. Can you confirm from a built AppImage — strings on the AppRun, or just env | grep PATH from a terminal launched inside the packaged app? If it does prefix, that probably wants handling alongside this rather than after.

4. shbash is right, but it's a new hard requirement

set -o pipefail genuinely fixes a real bug — a failed curl reporting a successful install is worse than a visible failure. But the prerequisite check for these specs is still "curl", and bash is now required where sh was before. It's near-universal on desktop Linux and present on macOS, so I'm not worried in practice; I'd just like the ADR to say it, so it's a recorded decision rather than an incidental consequence of the pipefail fix.

Nits

Fix 1, tell me which way you want to go on 2, and answer 3 — then this is ready. Tracing a Fedora curl failure back to an AppImage's LD_LIBRARY_PATH is not an obvious diagnosis, and the fix is in the right place 🐧


Generated by Claude Code

@Vect0rM

Vect0rM commented Sep 7, 2026

Copy link
Copy Markdown
Member

Synced this branch with main (merge commit, no rebase — your commits are unchanged). No conflicts; the branch was already mergeable and just needed to be current.

Re-verified on the merged tree (macOS host):

  • cargo check --manifest-path src-tauri/Cargo.toml --lib — 0 errors (10 pre-existing warnings)
  • cargo test --lib core::process_env — 3 passed
  • cargo test --lib core::system::commands — 42 passed, including the new curl_agent_installers_propagate_download_failures
  • cargo clippy --lib — no errors; only the pre-existing warnings
  • rustfmt --check on all six changed Rust files — clean

The sanitize_std_command call in apply_login_path is a nice catch: that is the shared boundary for Launch-page detection and installs, so every one of those spawns is covered by the single call rather than each call site having to remember.

Two small follow-ups, neither blocking this PR:

  1. The Unix installers now spawn bash (for set -o pipefail) while prerequisite still reports curl. On a Linux host without bash — rare for an AppImage desktop target, but Alpine and minimal NixOS qualify — the failure would surface as a missing-program error naming bash while the UI points the user at curl. Worth teaching the prerequisite check about the interpreter at some point.
  2. docs/decisions/INDEX.md here says 233 records. If fix: stop estimating ChatGPT subscription token speed #270 lands first it will also have taken that number, so whichever of the two merges second needs a 234. Both PRs edit different sections, so the entries themselves will not collide — only the counter line. I will push the one-line bump to this branch once fix: stop estimating ChatGPT subscription token speed #270 is in.

Otherwise ready to merge.

@Vect0rM
Vect0rM merged commit 61ce769 into AtomicBot-ai:main Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants