Conversation
…owershell Add `workos completion <shell>` command that generates shell-specific completion scripts. A hidden `--get-yargs-completions` fast path is intercepted before yargs parses to avoid validation errors on partial tab input. The completion engine walks the existing help-json.ts command registry, normalizing its mixed flat/nested naming into a uniform tree so both `auth login` style entries and `skills > install` style entries resolve correctly for subcommand drilling. Supports descriptions alongside candidates in shells that render them (zsh with fzf-tab, fish, powershell).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds shell autocompletion: a completion engine that normalizes the command registry, computes completions for commands and options, emits a server-format response, and generates Bash/Zsh/Fish/PowerShell scripts. Adds an early CLI fast-path for yargs completion requests and a ChangesShell Completion Engine
🚥 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)
Comment |
- Hoist completion intercept above heavy imports (yargs, clack, semver) so Tab presses skip ~150ms of module loading - Export raw command/option arrays from help-json.ts, eliminating double buildCommandTree() call and 'in' type narrowing in completion.ts - Remove dead DIRECTIVE.DEFAULT constant - Remove unused _resetCache export - Remove duplicate shell validation in bin.ts (yargs choices + generateShellScript throw already cover it) - Add 7 new tests (option value skipping, empty args, partial prefix filtering, hidden commands, descriptions)
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3311d846-0107-405b-8cfa-5f37121338ed
📒 Files selected for processing (5)
README.mdsrc/bin.tssrc/utils/completion.spec.tssrc/utils/completion.tssrc/utils/help-json.ts
Greptile SummaryThis PR adds
Confidence Score: 5/5Safe to merge — the change is additive (new command + fast-path exit), the three bugs raised in prior review rounds are all corrected, and the completion engine cannot affect existing command behavior. The completion fast-path exits before any existing yargs/clack/middleware code runs, so there is no regression risk for any current command. All three prior findings (BSD head portability, zsh colon-escaping, PowerShell tab literal) are fixed and covered by explicit test assertions. Registry normalization no longer mutates the original commandRegistry objects. The new middleware exclusion for completion is a one-liner addition to an existing allowlist. No files require special attention. Important Files Changed
|
There was a problem hiding this comment.
🟡 completion command not excluded from unclaimed-env middleware
The completion command is missing from the middleware exclusion list at src/bin.ts:200. The middleware comment explicitly states that utility commands like skills, doctor, env, and debug are excluded because the warning is unnecessary — completion is also a utility command but was not added. When completion runs through the middleware, maybeWarnUnclaimed() may make an API call (adding latency) and emit a stderr warning. While it won't corrupt the stdout script output, running eval "$(workos completion bash)" could flash a confusing "Unclaimed environment" warning.
(Refers to lines 200-202)
Was this helpful? React with 👍 or 👎 to provide feedback.
| if ($line -match '^:(\\d+)$') { | ||
| $directive = [int]$matches[1] | ||
| } elseif ($line.Trim()) { | ||
| $parts = $line.Split("\\t", 2) |
There was a problem hiding this comment.
🔴 PowerShell completion script splits on literal \t instead of tab character
The generated PowerShell completion script uses $line.Split("\t", 2) to parse tab-separated completions. However, in PowerShell, "\t" is the literal two-character string backslash-t — PowerShell uses backtick escapes ("`t") for tab characters, not backslash escapes. The .Split() method is not regex-based, so it won't interpret \t as a tab.
Impact on different PowerShell versions
On PowerShell 7+ (.NET 6+): .Split("\t", 2) matches String.Split(String, Int32) and looks for the literal two-character string \t, which never appears in the output (the actual output uses real tab characters from src/utils/completion.ts:29). The entire line becomes $comp, including the tab and description text.
On PowerShell 5 (.NET Framework): .Split("\t", 2) resolves to Split(Char[], Int32), splitting on either \ or t as individual characters. This incorrectly splits any completion containing the letter t (e.g., --git-check splits into --gi and -check...).
Prompt for agents
In src/utils/completion.ts, the generatePowershell function at line 364 outputs `$parts = $line.Split("\\t", 2)` which generates PowerShell code `$parts = $line.Split("\t", 2)`. In PowerShell, `"\t"` is a literal backslash-t, not a tab character. PowerShell uses backtick escapes for special characters.
The fix needs to produce PowerShell code that splits on an actual tab character. Two options:
1. Use PowerShell's [char]9 for a tab: change the line to produce `$parts = $line.Split([char]9, 2)` — this works on both PowerShell 5 and 7.
2. Use PowerShell's backtick escape: change to produce `$parts = $line.Split("`t", 2)` — but this requires careful escaping in the JavaScript template literal since backtick is the template delimiter.
Option 1 is simpler and avoids JS escaping complexity. In the JS template literal, the line should be: `$parts = $line.Split([char]9, 2)`
Was this helpful? React with 👍 or 👎 to provide feedback.
Add parameterized test that asserts every public bin.ts command has a matching entry in the help-json registry. Catches forgotten registry updates at CI time (which would cause missing tab completions and missing --help --json entries). Also add 7 completion edge case tests: option value skipping, empty args, partial prefix filtering, hidden commands, descriptions.
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1178ac09-8af7-4e1f-b019-2d396251ab1b
📒 Files selected for processing (2)
src/utils/completion.spec.tssrc/utils/completion.ts
Co-Authored-By: nick.nisi@workos.com <nick.nisi@workos.com>
Summary
workos completion <shell>command supporting bash, zsh, fish, and powershell--get-yargs-completionsfast path intercepted before yargs parses, avoiding validation errors on partial tab inputhelp-json.tscommand registry with normalization to handle the mixed flat/nested naming (e.g.auth loginvsskills > install)head -n-1:in zsh descriptions before passing candidates to_describe`tcompletionfrom unclaimed-environment warning middlewarehelp-jsoncommand registry during normalizationReview & Testing Checklist for Human
eval "$(workos completion bash)",eval "$(workos completion zsh)", fish, and PowerShell setup each produce usable completions in a real shell sessionhead: illegal line counterror appears:render correctly and do not break candidate parsingworkos completion <shell>does not show an unclaimed-environment warningNotes
Usage examples:
Local validation run:
pnpm test -- src/utils/completion.spec.ts— full Vitest suite passed, 125 files / 1675 testspnpm lintpnpm typecheckpnpm format:checkpnpm buildworkos --get-yargs-completions auth ""Summary by CodeRabbit
New Features
Documentation
Tests
Link to Devin session: https://app.devin.ai/sessions/d1f3c75f768e47d49b8fa040482284bd
Requested by: @nicknisi