fix(completion): harden auto-refresh paths and add coverage#390
Merged
toiroakr merged 13 commits intoMay 11, 2026
Merged
Conversation
- Throw from --install / --loader error paths so runMain exits non-zero; process.exitCode was overwritten by process.exit(result.exitCode). - Pass `-L` to shell `stat` in bash/zsh loaders and fish self-refresh so the shell-side mtime matches Node's fs.statSync (which follows symlinks). Without this, npm/pnpm bin shims would never match the embedded sig and the cache would regenerate on every shell startup. - Use a per-PID temp suffix (`$fish_pid`) for the fish self-rewriting refresh, matching the bash/zsh and Node atomic-write paths, so two concurrent fish sessions can't clobber each other's write.
…with shell - refreshIfStale now bails out when the target cache is missing or doesn't carry our `# politty-bin-sig:` header. The runMain hook fires on every CLI invocation, so without this it would silently create a fish autoload at `~/.config/fish/completions/<prog>.fish` (or any cache file) the user never opted into via `--install` or the rc loader. - Resolve the binary path through `resolveBinPath`, which walks `$PATH` for `programName` before falling back to `process.argv[1]`. The header sig and the shell-side stat now point at the same shim file, so pnpm/npm bin shims no longer trigger a regeneration on every shell startup just because Node sees the real entrypoint while the loader sees the shim. - Cover both new behaviors with unit tests.
`stat -L -f '%m' file` is BSD format-mode but GNU file-system mode, so on Linux it would dump filesystem info to stdout (and exit non-zero), the `||` fallback would append the epoch on a second line, and `_sig` would never match the embedded `# politty-bin-sig:` header — causing the cache to regenerate on every shell startup or fish TAB. Try `stat -L -c '%Y'` first (GNU); fall back to `stat -L -f '%m'` only when `-c` is rejected (BSD/macOS).
The bash/zsh loader wrapped a hardcoded `cacheDir` in double quotes, so any `$VAR`, backtick, or `$(...)` in the path would expand when the snippet is sourced — looking in a different directory than `install()` wrote to, and executing arbitrary commands when the path comes from env or config. Switch to single-quote escaping (`'…'\\''…'`) so shell metachars in the path stay inert.
`command -v` returns alias text or a function name when the user has shadowed the CLI in their rc, and the subsequent `stat "$_bin"` fails, so the loader returns before sourcing the cache and completions vanish for that shell. Use bash's `type -P` and zsh's `whence -p`, which look up only executables on `$PATH` and ignore aliases, functions, and builtins.
…mpletion on regen failure Two fixes for the auto-refresh paths: 1. `runMain` now bypasses user-provided `setup`/`cleanup`/`prompt` and the `globalArgs` schema for any registered `__`-prefixed subcommand. The runMain hook spawns `__refresh-completion` as a detached child, so without this any CLI with a global setup or required prompt would re-run that lifecycle in the refresher — duplicate connection effects, stuck background prompts, validation failures the user never opted into. Limited to *registered* `__`-prefixed subcommands so a stray argument can't accidentally skip lifecycle. 2. The bash/zsh rc loader now sources an existing cache even when regeneration fails (e.g. an upgraded binary with a broken `completion` command, or a transient I/O error). The previous `return 0` left the shell with no completion at all despite a perfectly usable stale cache being on disk, contradicting the best-effort behavior described elsewhere.
…n-session The bash/zsh rc loader and fish self-rewriting block both shelled out to `<bin> completion <shell>` for regeneration, which is a foreground command path. CLIs that wire up `setup`/`prompt` or required `globalArgs` would re-run those hooks during refresh, silently failing or blocking the user's shell. fish additionally only rewrote the file on disk — the *current* session kept the stale function bodies it had already loaded, so completions stayed stale until a manual reload. - Switch every refresh path (bash loader, zsh loader, fish autoload) to invoke the hidden `__refresh-completion` subcommand. `runMain` bypasses user lifecycle for `__`-prefixed registered subcommands, so refresh runs without side effects or validation. - After the fish refresh writes the new file, `source` it so the current session immediately picks up the new function bodies. - Move the "don't write a cache the user never opted into" guard from `refreshIfStale` into the runMain hook (`maybeSpawnRefresh` now checks `hasManagedCache` before spawning). This lets the rc loader and the fish autoload — both of which only run after the user has installed completion — refresh unconditionally, while still preventing plain CLI runs from creating completion files.
…ish refresh Three follow-ups for the auto-refresh paths: 1. Schema-aware bypass detection. `isInternalSubcommandInvocation` used the naive "first non-flag token" scan, so an ordinary invocation with an option value like `--name __refresh-completion` was misclassified as internal and ran without setup/cleanup/prompt or globalArgs validation. Switch to `findFirstPositional` and feed it an extracted global schema so option values are skipped. 2. Auto-register `__refresh-completion` on direct `createCompletionCommand` callers. Previously only `withCompletionCommand` registered the hidden subcommand, so the loaders/fish autoload it generated would shell out to a subcommand the host CLI never exposed; refresh would silently fail and leave the stale cache in place. Mirror the existing `__complete` mutate-register pattern so both APIs expose the same surface. 3. Stop the stale fish body after a successful refresh. The autoload file's tail used to keep defining stale helper functions and `complete -c` lines after `source $_target` had already loaded the new definitions, so the current session ended up with stale completions on top of the fresh ones. Capture the refresh status into a `_politty_refreshed` flag and `return` from the source so the rest of the old file is skipped.
…eferences
- Drop duplicate `detectShellEnv` in `install.ts`; reuse the existing
`detectShell` from `index.ts`. Both did the same `$SHELL`-based scan.
- Remove the unused `export { computeBinSig }` re-export from
`install.ts` — no caller imports it from here.
- Fix three references to `completion install <shell>` (a positional-
subcommand syntax that doesn't exist) to the actual flag form
`completion <shell> --install` in the user-visible fish error and
JSDoc.
Hoist `refreshExtra`, `installCtxBase`, and `loaderOptsBase` once at
the top of `createCompletionCommand` and reuse them in the install /
loader / run branches instead of rebuilding the same conditional
spreads three times. Collapse the three `...(extra.X !== undefined &&
{X: extra.X})` checks in `createRefreshCompletionCommand` to
`...extra` since `InstallContext`'s optional fields type-include
`undefined`. Replace the `let extra = {}; if (...) extra.X = X`
pattern in `withCompletionCommand` with an object literal using
conditional spreads to match the rest of the file. Drop the dead
`else` branch from the `effectiveOptions` ternary in `runMain`.
Pure refactor — no behavior change.
`computeBinSig` already returns `"0"` when `statSync` throws, so the outer `binPath ? computeBinSig(binPath) : "0"` ternary in `buildHeaderLines` was dead defensive code. Inline the call.
…safety
Fill the coverage gaps the new auto-refresh paths created:
- Fish self-rewriting autoload: assert it calls `__refresh-completion`
(not the foreground `completion` command), captures
`_politty_refreshed` and returns from the source on success, probes
GNU `stat -c` before BSD `stat -f`, and embeds the resolved bin-sig.
- `completion --install` / `--loader` flag wiring: bash + zsh + fish
branches, including the fish-loader rejection.
- `__refresh-completion` subcommand registration via both
`withCompletionCommand` and the direct `createCompletionCommand`
entry point.
- `runMain` `runMainHook`: invoked once with parsed argv, thrown
errors are swallowed so the user command still runs and exits 0.
- `maybeSpawnRefresh` gates: spawns the detached refresher for
ordinary invocations; skips on `POLITTY_NO_COMPLETION_REFRESH=1`,
on `completion` / `__complete` / `__refresh-completion`, when no
managed cache exists, and when `$SHELL` is unrecognized.
`vi.mock("node:child_process", ...)` is hoisted to the top of
`tests/completion.test.ts` so the new tests can spy on `spawn`
without breaking the dynamic-completion tests that need `execSync`.
- `docs/api-reference.md`: list the new `--install` / `--loader` flags and the hidden `__refresh-completion` subcommand under `withCompletionCommand`. Fill in missing rows for the `WithCompletionOptions` and `CompletionOptions` tables (`globalArgsSchema`, `cacheDir`, `programVersion`, `binPath`, `includeSubcommands`). - `src/core/runner.ts`: extend `runMain` JSDoc to mention the `runMainHook` invocation and the lifecycle bypass for `__`-prefixed registered subcommands.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #349. Hardens the new auto-refresh paths and aligns tests and docs.
Verification
End-to-end run against a real binary and bash:
mycli completion bash --installwrites the cache and embeds# politty-bin-sig:mycli completion bash --loaderprints the rc snippet to stdoutsourcethe loader after bumping the binary's mtime triggers__refresh-completionand rewrites the cachePOLITTY_NO_COMPLETION_REFRESH=1skips the background spawn__refresh-completion bashruns without invoking usersetup/cleanupMain Changes
--install/--loadererror paths sorunMainexits non-zero (process.exitCodewas overwritten byprocess.exit(result.exitCode)).-Lto shellstatand probe GNU-c '%Y'before BSD-f '%m'so npm/pnpm shims and Linux do not trip the freshness check.type -Pand zshwhence -pso an alias or function shadowing the CLI name does not silently disable completion.cacheDirin the rc loader so shell metachars in the path stay inert.refreshIfStaleand intomaybeSpawnRefresh(hasManagedCache) so background hooks no longer create fish autoload files on plain CLI runs.resolveBinPath(PATH walk, thenprocess.argv[1]) so the embedded sig and the shell-side stat target the same file.setup/cleanup/promptand requiredglobalArgsfor registered__-prefixed subcommands. Use schema-awarefindFirstPositionalso an option value like--name __internaldoes not trip the bypass.__refresh-completionin every refresh path so refresh runs without foreground lifecycle side effects.sourcethe new file andreturnfrom the old autoload body so stale function definitions do not run on top of the fresh ones.__refresh-completionincreateCompletionCommandso direct callers expose the subcommand the loaders invoke.Notes
detectShellEnvtodetectShell), fixcompletion install <shell>references to the actualcompletion <shell> --installsyntax, deduplicate option-shape construction increateCompletionCommand, drop the dead empty-binPath guard inbuildHeaderLines.--install/--loaderflag plumbing,__refresh-completionregistration via both APIs,runMainrunMainHookinvocation and throw-safety,maybeSpawnRefreshgates, andrunMaininternal subcommand bypass.--install/--loaderflags and the__refresh-completionsubcommand underwithCompletionCommand, and fills theWithCompletionOptionsandCompletionOptionstables.runMainJSDoc mentions therunMainHookinvocation and the__-prefix bypass.