Skip to content

fix: invoke CLI installer with bash instead of sh#1260

Open
ytallo wants to merge 1 commit intomainfrom
fix/cli-installer-requires-bash
Open

fix: invoke CLI installer with bash instead of sh#1260
ytallo wants to merge 1 commit intomainfrom
fix/cli-installer-requires-bash

Conversation

@ytallo
Copy link
Contributor

@ytallo ytallo commented Mar 7, 2026

Summary

  • The engine install script invokes the CLI install script via sh, but the CLI script uses set -o pipefail (a bash feature)
  • On Ubuntu/Debian where sh is dash, this fails with: Illegal option -o pipefail
  • Fix: use bash instead of sh to invoke the CLI installer

This was the second fix from #1259 that was lost during squash merge.

Test plan

  • Run curl -fsSL https://install.iii.dev/iii/main/install.sh | sh on Ubuntu and verify CLI installs without "Illegal option" error

Summary by CodeRabbit

  • Bug Fixes
    • Modified the CLI installer script to use bash instead of sh for script execution in both installation paths.

The CLI install script uses `set -o pipefail` which requires bash.
When the engine installer invoked it via `sh`, dash (the default sh on
Ubuntu/Debian) rejected the option with "Illegal option -o pipefail".
@vercel
Copy link
Contributor

vercel bot commented Mar 7, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
iii-docs Ready Ready Preview, Comment Mar 7, 2026 10:48pm

Request Review

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Mar 7, 2026

📝 Walkthrough

Walkthrough

The CLI installer invocation in the installation script is updated to execute using bash instead of sh across both conditional branches (when INSTALL_DIR is set and when it is not set).

Changes

Cohort / File(s) Summary
CLI Shell Invocation
engine/install.sh
Changed shell interpreter from sh to bash for executing the CLI installer script in both deployment paths.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~2 minutes

Possibly related PRs

Poem

🐰 Hop hop, from sh to bash we go,
A shell swap here, a shell swap there,
Scripts now run with features bright,
Bash-specific tricks take flight! 🚀

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: updating the CLI installer invocation from sh to bash to support bash-specific features.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/cli-installer-requires-bash

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
engine/install.sh (1)

13-16: Consider adding a bash availability check for clearer error messaging.

If bash is not installed on the target system, the error message would be a generic "command not found" rather than a clear diagnostic. Adding an early check in install_cli would improve the user experience on minimal systems.

💡 Suggested improvement
 install_cli() {
   echo ""
   echo "installing iii cli..."
+
+  if ! command -v bash >/dev/null 2>&1; then
+    echo "warning: bash is required for CLI installation, skipping" >&2
+    return 1
+  fi
 
   cli_tmpdir=$(mktemp -d 2>/dev/null || mktemp -d -t iii-cli-install)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@engine/install.sh` around lines 13 - 16, The install_cli function should
first verify bash is available and fail with a clear message if not; add an
early check in install_cli (e.g., using command -v bash or type bash >/dev/null
2>&1) and if the check fails call echo with a helpful diagnostic like "bash is
required to install the iii cli" and exit 1, ensuring the rest of install_cli
only runs when bash is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@engine/install.sh`:
- Around line 13-16: The install_cli function should first verify bash is
available and fail with a clear message if not; add an early check in
install_cli (e.g., using command -v bash or type bash >/dev/null 2>&1) and if
the check fails call echo with a helpful diagnostic like "bash is required to
install the iii cli" and exit 1, ensuring the rest of install_cli only runs when
bash is present.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0c47d1a1-a3e8-42b2-9919-7a086b3c7052

📥 Commits

Reviewing files that changed from the base of the PR and between 7d94c80 and a1b832c.

📒 Files selected for processing (1)
  • engine/install.sh

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