feat(plugin-completion): support Bun runtimes#556
Conversation
Resolve completion script executables for Bun source execution and likely Bun single-file binaries while keeping Deno as an unsupported runtime path. Split executable resolution into testable helpers, quote shell command parts consistently, and document the Bun compile heuristic with upstream references. Escape executable commands for the @bomb.sh/tab generated shell assignment before eval to avoid command substitution while preserving token quoting.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughRefactors completion tooling to detect runtimes (Bun/Deno/Node), produce completion-safe shell command strings, add Bun single-binary heuristics, and validate behavior with unit and e2e tests; also updates documentation and generated docs anchors. ChangesBun Runtime Support and Command Composition Refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
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. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
e2e/bun.spec.ts (1)
9-11: ⚡ Quick winNote: POSIX quoting works on current CI but consider centralizing if Windows support is added.
shellQuoteuses POSIX single-quote escaping, andrunCommandexecutes commands via shell (exec). This is not a current risk since E2E CI runs only on ubuntu-latest, but would become relevant if Windows CI is ever introduced. Consider moving this helper to a shared utilities file or documenting the Unix-only assumption.🤖 Prompt for 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. In `@e2e/bun.spec.ts` around lines 9 - 11, shellQuote currently implements POSIX single-quote escaping and is used by runCommand which invokes a shell; centralize or document this Unix-only assumption to avoid surprises if Windows CI is added: move the shellQuote function into a shared utilities module (e.g., a test-utils or e2e-utils file), update callers (runCommand) to import it, and either (a) add clear inline comments and README/test docs stating it is POSIX-only and CI runs on ubuntu-latest, or (b) replace/guard it with a cross-platform implementation when detecting Windows; reference the shellQuote function and runCommand so the helper is relocated and consumers updated accordingly.
🤖 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 `@packages/plugin-completion/README.md`:
- Line 16: Update the user-facing sentence in
packages/plugin-completion/README.md to use correct grammar: change "This
package support Node.js and Bun runtime only. Deno support is not available
yet." to use "supports" (e.g., "This package supports Node.js and Bun runtimes
only; Deno support is not available yet.") so the verb agrees with the singular
subject and the wording is clearer.
In `@packages/plugin-completion/src/utils.ts`:
- Line 230: The error message thrown in packages/plugin-completion/src/utils.ts
is grammatically incorrect; update the thrown Error's message (the throw new
Error(...) in the completion script generation path) to a clear, correct phrase
such as "Unsupported JavaScript runtime for completion script generation." so
diagnostics are readable and consistent.
---
Nitpick comments:
In `@e2e/bun.spec.ts`:
- Around line 9-11: shellQuote currently implements POSIX single-quote escaping
and is used by runCommand which invokes a shell; centralize or document this
Unix-only assumption to avoid surprises if Windows CI is added: move the
shellQuote function into a shared utilities module (e.g., a test-utils or
e2e-utils file), update callers (runCommand) to import it, and either (a) add
clear inline comments and README/test docs stating it is POSIX-only and CI runs
on ubuntu-latest, or (b) replace/guard it with a cross-platform implementation
when detecting Windows; reference the shellQuote function and runCommand so the
helper is relocated and consumers updated accordingly.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55b699ea-6258-4e84-ac24-6bbc004af862
📒 Files selected for processing (9)
e2e/bun.spec.tspackages/plugin-completion/README.mdpackages/plugin-completion/docs/functions/default.mdpackages/plugin-completion/docs/interfaces/CompletionConfig.mdpackages/plugin-completion/docs/interfaces/CompletionOptions.mdpackages/plugin-completion/docs/interfaces/CompletionParams.mdpackages/plugin-completion/docs/type-aliases/CompletionHandler.mdpackages/plugin-completion/src/utils.test.tspackages/plugin-completion/src/utils.ts
@gunshi/bone
@gunshi/combinators
@gunshi/definition
@gunshi/docs
gunshi
@gunshi/plugin
@gunshi/plugin-completion
@gunshi/plugin-dryrun
@gunshi/plugin-global
@gunshi/plugin-i18n
@gunshi/plugin-renderer
@gunshi/resources
@gunshi/shared
commit: |
kazupon
left a comment
kazupon
left a comment
There was a problem hiding this comment.
Thank you for your contribution!
I've just reviewed your PR!
Please check it!
There was a problem hiding this comment.
I think we can keep the README simpler here.
Instead of adding a dedicated Runtime Support section with Bun-specific implementation details, it may be enough to update the existing warning and the Shell Completion Setup prerequisites so users know that Node.js and Bun are supported while Deno is not supported yet.
The current Bun-specific text explains internal details such as the compiled-executable detection heuristic, which feels more appropriate for code comments, tests, or the PR description than for end-user documentation.
For the README, I think it is clearer to keep the focus on what users need to do to set up shell completions.
There was a problem hiding this comment.
Addressed in ea008a9. I removed the dedicated Runtime Support section and kept the README focused on the warning and setup prerequisites.
| * @param globals - Runtime globals to inspect | ||
| * @returns The detected runtime name | ||
| */ | ||
| export function detectRuntime(globals: RuntimeGlobals = getRuntimeGlobals()): Runtime { |
There was a problem hiding this comment.
Do we need globals in the public signature here?
For actual runtime detection, the natural usage seems to be detectRuntime() with the current environment. Passing custom globals looks useful mainly for tests, but I do not see a production use case for callers to provide them intentionally.
If the goal is testability, would it be clearer to keep detectRuntime() as the environment-facing function and extract a separate pure helper such as detectRuntimeFromGlobals(globals) for the injected case?
There was a problem hiding this comment.
Addressed in 36d0b5e. detectRuntime() now reads the current environment, and tests use detectRuntimeFromGlobals(globals) for injected globals.
Keep the README focused on supported runtimes and setup prerequisites while moving Bun heuristic details out of the user-facing docs.
Expose detectRuntime() as the environment-facing path and move injected global detection into detectRuntimeFromGlobals() for tests. Update quoteExec() to use the environment detector, clean up the unsupported runtime message, and document the POSIX-only E2E shell quoting assumption.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/plugin-completion/src/utils.test.ts (1)
117-138: ⚡ Quick winAdd coverage for the
B:/~BUN/virtual-entry branch.
isBunVirtualEntry()accepts both Windows spellings, but this suite only exercises the backslash variant. That leaves one branch of the compiled-binary heuristic unguarded, so a regression in the forward-slash path would slip through.🧪 Proposed test addition
test('resolves Bun Windows virtual compiled entry', () => { expect( resolveBunExecParts( createProcess({ argv: ['C:\\tools\\bun.exe', 'B:\\~BUN\\root.js'], execPath: 'C:\\tools\\bun.exe' }) ) ).toEqual(['C:\\tools\\bun.exe']) }) + + test('resolves Bun Windows virtual compiled entry with forward slashes', () => { + expect( + resolveBunExecParts( + createProcess({ + argv: ['C:\\tools\\bun.exe', 'B:/~BUN/root.js'], + execPath: 'C:\\tools\\bun.exe' + }) + ) + ).toEqual(['C:\\tools\\bun.exe']) + }) })🤖 Prompt for 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. In `@packages/plugin-completion/src/utils.test.ts` around lines 117 - 138, Add a test exercising the forward-slash Windows virtual-entry path so the branch in isBunVirtualEntry that matches "B:/~BUN/" is covered: update the tests that call resolveBunExecParts (and use createProcess) to include a case where argv contains 'B:/~BUN/root.js' with execPath set to the bun executable (e.g., '/usr/local/bin/bun' or 'C:\\tools\\bun.exe' as appropriate) and assert the same expected output as the backslash variant; this ensures the forward-slash branch of isBunVirtualEntry and the compiled-binary heuristic in resolveBunExecParts are exercised.
🤖 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.
Nitpick comments:
In `@packages/plugin-completion/src/utils.test.ts`:
- Around line 117-138: Add a test exercising the forward-slash Windows
virtual-entry path so the branch in isBunVirtualEntry that matches "B:/~BUN/" is
covered: update the tests that call resolveBunExecParts (and use createProcess)
to include a case where argv contains 'B:/~BUN/root.js' with execPath set to the
bun executable (e.g., '/usr/local/bin/bun' or 'C:\\tools\\bun.exe' as
appropriate) and assert the same expected output as the backslash variant; this
ensures the forward-slash branch of isBunVirtualEntry and the compiled-binary
heuristic in resolveBunExecParts are exercised.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df0a763b-9c49-4a73-a5fd-c430e020f2ec
📒 Files selected for processing (4)
e2e/bun.spec.tspackages/plugin-completion/README.mdpackages/plugin-completion/src/utils.test.tspackages/plugin-completion/src/utils.ts
✅ Files skipped from review due to trivial changes (1)
- packages/plugin-completion/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/bun.spec.ts
Remove the current-environment runtime assertion from the unit tests and keep detectRuntime() as an internal quoteExec() helper.
2 participants
Description
Resolve completion script executables for Bun source execution and likely Bun single-file binaries while keeping Deno as an unsupported runtime path.
This PR splits executable resolution into testable helpers, quotes shell command parts consistently, and documents the Bun compile heuristic with upstream references. It also escapes executable commands for the
@bomb.sh/tabgenerated shell assignment beforeevalto avoid command substitution while preserving token quoting.Implementation references:
process.argvbehavior: https://github.com/oven-sh/bun/blob/main/test/regression/issue/22157.test.ts#L47-L55@bomb.sh/tabgeneratedrequestComp+evalpatterns:Test coverage added:
process.execArgv./$bunfs/,B:\~BUN\, andB:/~BUN/.bun build --compilebinaries:complete zshcomplete -- --config vite.configLinked Issues
Closes #557
Additional context
Reviewer focus:
Validation:
pnpm fix:oxfmtpnpm test:plugin-completionpnpm --filter @gunshi/plugin-completion typecheck:denopnpm exec vitest run --config vitest.e2e.config.ts e2e/bun.spec.tspnpm lintSummary by CodeRabbit
New Features
Tests
Documentation