Skip to content

fix(completion): install the right completions for fish users - #1364

Merged
clay-good merged 2 commits into
mainfrom
fix/completion-detect-parent-shell
Jul 17, 2026
Merged

fix(completion): install the right completions for fish users#1364
clay-good merged 2 commits into
mainfrom
fix/completion-detect-parent-shell

Conversation

@clay-good

@clay-good clay-good commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Closes the problem reported in discussion #1197.

What was wrong

openspec completion install detected your shell only from $SHELL, which is your login shell. If you run fish interactively but your login shell is bash (the default on Fedora Silverblue and many container/immutable setups), the command installed bash completions and edited .bashrc — silently doing the wrong thing:

❯ openspec completion install
⠋ Installing bash completion script...
✓ Completion script installed and .bashrc configured successfully

Fish completions were already fully supported — they just never got selected.

What it does

Detection now looks at the parent process (the shell that actually launched openspec) before falling back to $SHELL. So running the command from fish installs fish completions.

  • Only a parent that maps to a supported shell (bash/zsh/fish) is trusted; npx, npm scripts, and other non-shell parents fall back to $SHELL exactly as before.
  • POSIX-only (ps); Windows/PowerShell detection is untouched.
  • Best-effort: any failure reading the parent silently falls back to $SHELL, so nothing regresses.

Proof it works

New unit tests cover the fish-on-bash case, macOS full-path ps output, and every fallback path. Verified end-to-end against the real ps (fish isn't installed locally, so these use bash/zsh to prove the parent shell overrides $SHELL — the exact mechanism a fish user needs):

Parent shell $SHELL (login) Detected
bash /usr/bin/fish bash ✓ (parent wins)
zsh /bin/bash zsh ✓ (mirrors #1197)
node (npx-style) /bin/bash bash ✓ (falls back to $SHELL)
bash /bin/bash bash ✓ (unchanged)

Notes

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Fixed shell completion installation choosing the wrong shell when the interactive shell differs from the login shell.
    • Improved shell detection by preferring the interactive shell derived from the running parent process, with accurate handling for Fish, Bash, and Zsh (including executable-path cases).
    • Strengthened fallback behavior when parent shell detection fails.
    • Preserved Windows PowerShell detection without running Unix-specific detection.

`openspec completion install` read only $SHELL, the login shell, so users
whose interactive shell differs (e.g. fish users on distros where the login
shell is bash) got bash completions installed by default (#1197).

Detection now consults the parent process via `ps` before falling back to
$SHELL. It only trusts a parent that maps to a supported shell, so npx/npm
and other non-shell parents still fall back cleanly; Windows is unaffected.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@clay-good
clay-good requested a review from TabishB as a code owner July 15, 2026 14:54
@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: bfe2819a-2885-432e-806c-c62c9e028fe7

📥 Commits

Reviewing files that changed from the base of the PR and between 7d292d4 and b70be5e.

📒 Files selected for processing (2)
  • src/utils/shell-detection.ts
  • test/utils/shell-detection.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • test/utils/shell-detection.test.ts

📝 Walkthrough

Walkthrough

Shell detection now checks the parent process for supported shells before falling back to $SHELL. Tests cover shell parsing, fallback behavior, failures, and Windows handling. A patch changeset documents the update.

Changes

Shell Detection

Layer / File(s) Summary
Parent-process shell detection
src/utils/shell-detection.ts
Maps shell names, inspects the parent process with ps on non-Windows systems, and falls back to $SHELL when needed.
Detection validation and release metadata
test/utils/shell-detection.test.ts, .changeset/completion-detect-parent-shell.md
Mocks parent-process inspection, tests shell and fallback cases across platforms, and records a patch release changeset.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant detectShell
  participant execFileSync
  participant Environment
  detectShell->>execFileSync: inspect parent process with ps
  execFileSync-->>detectShell: parent shell name or failure
  detectShell->>Environment: read SHELL when parent detection fails
  Environment-->>detectShell: supported shell value
Loading

Suggested reviewers: tabishb, alfred-openspec

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: fixing completion installation for fish users.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/completion-detect-parent-shell

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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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 `@src/utils/shell-detection.ts`:
- Around line 21-25: Update matchSupportedShell to extract the executable
basename and return a supported shell only when that basename exactly equals
zsh, bash, or fish; do not classify names such as fish-lsp or
bash-language-server. Add a regression test using a fish-lsp parent that
verifies detection falls back to $SHELL.
🪄 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: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 80812885-299e-4b76-a42d-4aac350c9659

📥 Commits

Reviewing files that changed from the base of the PR and between 0a99f41 and 7d292d4.

📒 Files selected for processing (3)
  • .changeset/completion-detect-parent-shell.md
  • src/utils/shell-detection.ts
  • test/utils/shell-detection.test.ts

Comment thread src/utils/shell-detection.ts Outdated
…ent-process tests

Two fixes from CI and review on #1364:

- matchSupportedShell now matches the executable basename exactly
  (stripping a login-shell leading dash) instead of substring matching,
  so parents like fish-lsp or bash-language-server no longer get
  mistaken for the shell (CodeRabbit review).
- The parent-process tests pin process.platform to linux so they
  exercise the ps path on Windows CI, where detection otherwise
  short-circuits and the tests failed.

Adds regression tests for -zsh login shells and fish-lsp fallback.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@alfred-openspec alfred-openspec left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified parent-process precedence, exact basename and login-shell handling, POSIX fallback, and the Windows guard. The focused regression matrix and full CI are green.

@clay-good
clay-good added this pull request to the merge queue Jul 17, 2026
Merged via the queue into main with commit f58b445 Jul 17, 2026
14 checks passed
@clay-good
clay-good deleted the fix/completion-detect-parent-shell branch July 17, 2026 17:26
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