feat(completion): add auto-refresh for shell completion caches#349
Open
toiroakr wants to merge 16 commits into
Open
feat(completion): add auto-refresh for shell completion caches#349toiroakr wants to merge 16 commits into
toiroakr wants to merge 16 commits into
Conversation
Generated bash/zsh/fish scripts now embed a `# politty-bin-sig: <mtime>` header. Two complementary refresh paths keep the on-disk cache in sync with the binary so subcommand renames or new options take effect on the very next TAB without manual reinstall: - An rc-loader snippet (printed by `<program> completion <shell> --loader`) that bash/zsh source on startup; it stat-compares the binary against the cached header and rewrites the cache when stale. - A detached `__refresh-completion` child fired from `runMain` on every CLI invocation, keeping caches warm even when shells aren't restarted. For fish, the autoload file written by `--install` ends with a self-rewriting block that runs on TAB and replaces itself when stale. The runner stays decoupled from the completion module via a generic `CommandBase.runMainHook` field that `withCompletionCommand` populates. Set `POLITTY_NO_COMPLETION_REFRESH=1` to disable the background hook. Adds `--install` and `--loader` flags to the `completion` subcommand, plus `WithCompletionOptions.cacheDir` and `programVersion`. Closes #345 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
commit: |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
The --install and __refresh-completion paths were dropping globalArgsSchema, so the background refresh could overwrite a correct cache with one missing global options. Thread the schema through InstallContext and createRefreshCompletionCommand. Also fix a test bug where new Date((mtimeMs + 5000) / 1000) produced a 1970 timestamp instead of bumping by 5s. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
- 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.
…o-refresh fix(completion): harden auto-refresh paths and add coverage
|
Devin: please take over PR #349 as the repair/execution agent. Objective: make this PR merge-ready without manual maintainer babysitting. Current known state:
Action requested:
Do not expand scope beyond PR #349. |
|
Merge-readiness check after Devin Review removal / unavailable state: Status: GO for maintainer merge. Evidence checked:
Merge blocker from this integration:
Required maintainer action:
No code changes made by this check. |
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
3 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.
Summary
# politty-bin-sig: <mtime>header in every generated bash/zsh/fish script so refresh paths can detect when the on-disk cache is stale relative to the binary.<program> completion <shell> --loader) that stat-compares the binary on shell startup and rewrites the cache when stale before sourcing it.__refresh-completionchild fromrunMainon every CLI invocation so caches stay warm without restarting the shell. Decoupled fromcore/runner.tsvia a genericCommandBase.runMainHookfield thatwithCompletionCommandpopulates.--installends with a self-rewriting block that runs on TAB and replaces itself in place when the binary's mtime changes.<program> completion <shell> --install(write to cache/autoload location) and--loader(print rc-loader snippet). NewWithCompletionOptions.cacheDir/programVersion. Opt out withPOLITTY_NO_COMPLETION_REFRESH=1.Closes #345.
Test plan
pnpm vitest run— 1247 passed, 4 skipped (incl. 14 new unit tests for header/install/refresh/loader/defaultCacheDir)pnpm lint— 0 errors (verifiedcore/does not importcompletion/)pnpm typecheck— cleanpnpm build— cleanmycli completion bash --installwrites the cache;mycli completion bash --loader >> ~/.bashrc; bumping the binary's mtime and opening a new shell rewrites the cache automaticallymycli completion fish --installwrites the autoload file; bumping mtime causes the next TAB to rewrite the file🤖 Generated with Claude Code
Code Metrics Report
Details
Code coverage of files in pull request scope (89.9% → 92.5%)
Reported by octocov