Skip to content

Harden installer and path handling#189

Open
Golenspade wants to merge 3 commits intodifferent-ai:devfrom
Golenspade:fix/engine-install-pipe
Open

Harden installer and path handling#189
Golenspade wants to merge 3 commits intodifferent-ai:devfrom
Golenspade:fix/engine-install-pipe

Conversation

@Golenspade
Copy link
Contributor

Summary

  • Pipe the installer script into bash stdin to avoid shell pipelines and surface curl errors.
  • Add workspace state file locking plus safer environment/config path handling.
  • Canonicalize OpenCode binaries/sidecar paths and add skill name length guardrails.

Testing

  • pnpm install --frozen-lockfile
  • pnpm --filter @different-ai/openwork build:web
  • pnpm --filter @different-ai/openwork test:e2e
  • cargo check --manifest-path packages/desktop/src-tauri/Cargo.toml --locked
  • cargo test --manifest-path packages/desktop/src-tauri/Cargo.toml --locked

Copilot AI review requested due to automatic review settings January 22, 2026 05:57
@github-actions
Copy link
Contributor

The following comment was made by an LLM, it may be inaccurate:

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements security and robustness improvements to the installer and path handling across the desktop application. The changes focus on hardening file operations, environment variable handling, and executable resolution.

Changes:

  • Added file locking (shared/exclusive) for workspace state operations using the fs2 crate
  • Enhanced path validation to ensure absolute paths for HOME, USERPROFILE, and XDG_CONFIG_HOME environment variables
  • Improved installer script by piping curl output directly to bash stdin, avoiding shell pipeline issues and better surfacing curl errors
  • Added canonicalization for OpenCode executable and sidecar paths with symlink validation
  • Added skill name length validation (1-100 characters)

Reviewed changes

Copilot reviewed 8 out of 9 changed files in this pull request and generated no comments.

Show a summary per file
File Description
packages/desktop/src-tauri/src/workspace/state.rs Added file locking (shared for reads, exclusive for writes) and improved file handling with explicit truncation and sync operations
packages/desktop/src-tauri/src/paths.rs Added absolute path validation for HOME and USERPROFILE environment variables, enhanced XDG environment variable handling
packages/desktop/src-tauri/src/engine/paths.rs Added canonicalization helper for executable path resolution to resolve symlinks and validate file existence
packages/desktop/src-tauri/src/config.rs Added env_path helper function to validate environment variable paths are absolute, improved config directory resolution
packages/desktop/src-tauri/src/commands/skills.rs Added maximum length validation (100 characters) for skill names
packages/desktop/src-tauri/src/commands/engine.rs Refactored installer to pipe curl output to bash stdin instead of shell pipeline, improved error handling and stderr collection
packages/desktop/src-tauri/build.rs Added symlink detection to prevent overwriting symlinks during sidecar setup, added canonicalization for source path resolution
packages/desktop/src-tauri/Cargo.toml Added fs2 = "0.4" dependency for file locking functionality
packages/desktop/src-tauri/Cargo.lock Updated lock file with fs2 dependency and its transitive dependencies

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@benjaminshafii
Copy link
Member

Automated message: the hardening changes look good and align with the security/least‑privilege goals (file locks, canonical paths, symlink guard, safer installer piping).

Two notes:

  • PR is currently CONFLICTING; please rebase on dev.
  • Optional: consider adding a short failure message if bash exits non‑zero so users see the installer error quickly (you already capture curl stderr, which is great).

Happy to re-review once rebased.

@benjaminshafii
Copy link
Member

@Golenspade thx.

we made a few changes over the last days to make sidecar the default path so we can control the experience a bit more. we're still giving PATH as an escape hatch.

What does this PR intend to bring to the experience?

@Golenspade
Copy link
Contributor Author

@benjaminshafii thx for the review:Intent is UX hardening, not a path-precedence change. This PR makes install + binary resolution more deterministic and safer: we canonicalize candidate binaries, avoid symlink overwrites, validate env-derived paths, and lock workspace state to prevent corruption. The user impact is fewer mysterious install failures or “wrong binary” issues, while still keeping PATH as an escape hatch. The new sidecar-default policy you mentioned is compatible; this just adds guardrails around whatever path wins.

@Golenspade Golenspade force-pushed the fix/engine-install-pipe branch from c59705a to 3771d9e Compare January 26, 2026 02:47
@Golenspade
Copy link
Contributor Author

@benjaminshafii rebased and added the bash exit error alert

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