From f816cdeec1a91fa0189ca2e8e8693db359e1ae0d Mon Sep 17 00:00:00 2001 From: Ryan de Melo Date: Fri, 9 Oct 2026 20:34:40 +0800 Subject: [PATCH 1/2] perf(completion): keep Tab completion off telemetry and heavy imports Shell completion scripts run `openspec __complete ` on every Tab press. Each press loaded 344 modules (zod, yaml, fast-glob, ora) and, with telemetry on, waited for the usage request before exiting. - Skip the telemetry and completion-tip hooks for `__complete`. The scripts send stderr to /dev/null, so the first-run notice would also be marked seen there without anyone reading it. - Load `ora` only in install and uninstall, which use the spinner. - Load `listSchemas` from the resolver inside `getSchemaNames` instead of through the artifact-graph barrel, which also pulls in fast-glob. Completing changes or specs now loads 48 modules (commander only), and schemas loads 219. Part of #2026, items 1 and 2. --- .changeset/faster-tab-completion.md | 5 ++++ src/cli/index.ts | 21 +++++++++++++++++ src/commands/completion.ts | 5 +++- src/core/completions/completion-provider.ts | 6 +++-- test/cli-e2e/startup-modules.test.ts | 26 +++++++++++++++++++++ test/core/cli-is-json-run.test.ts | 24 ++++++++++++++++++- 6 files changed, 83 insertions(+), 4 deletions(-) create mode 100644 .changeset/faster-tab-completion.md diff --git a/.changeset/faster-tab-completion.md b/.changeset/faster-tab-completion.md new file mode 100644 index 0000000000..f6b253e784 --- /dev/null +++ b/.changeset/faster-tab-completion.md @@ -0,0 +1,5 @@ +--- +"@fission-ai/openspec": patch +--- + +Shell Tab completion is faster. `openspec __complete`, which the completion scripts run on every Tab press, now loads 48 modules instead of 344 when completing changes or specs, and no longer records usage, so a press never waits on the telemetry request and can't use up the first-run telemetry notice where nobody sees it. Completion output is unchanged. diff --git a/src/cli/index.ts b/src/cli/index.ts index 3501d0f5bf..4504e44432 100644 --- a/src/cli/index.ts +++ b/src/cli/index.ts @@ -123,6 +123,14 @@ export function isCompletionRun(commandPath: string): boolean { return commandPath.split(':')[0] === 'completion' || commandPath === '__complete'; } +/** + * True for the hidden `__complete` resolver alone. It runs on every Tab press, + * so the hooks skip telemetry and the completion tip for it entirely. + */ +export function isTabCompletionRun(commandPath: string): boolean { + return commandPath === '__complete'; +} + /** * True when the first-run completions tip must be deferred rather than shown. * @@ -196,6 +204,13 @@ program.hook('preAction', async (thisCommand, actionCommand) => { process.env.NO_COLOR = '1'; } + // `__complete` runs on every Tab press, with stderr sent to /dev/null by the + // generated scripts. Tracking it would hold each press open for the request, + // and the first-run notice would be marked seen without anyone reading it. + if (isTabCompletionRun(getCommandPath(actionCommand))) { + return; + } + // Show first-run telemetry notice (if not seen). It's written to stderr, so it // never pollutes stdout — but --json runs still defer it (see isJsonRun) so the // very first invocation stays free of any incidental output on either stream. @@ -210,6 +225,12 @@ program.hook('preAction', async (thisCommand, actionCommand) => { // Shutdown telemetry after command completes program.hook('postAction', async (_thisCommand, actionCommand) => { + // Nothing to flush (preAction tracked nothing), and the tip is always + // deferred for completion runs, so skip loading either module. + if (isTabCompletionRun(getCommandPath(actionCommand))) { + return; + } + // Show the first-run shell-completions tip (on stderr, so piped stdout stays // clean). postAction, not preAction: the tip trails the command's own output // instead of pushing an error message or `init`'s setup summary down the diff --git a/src/commands/completion.ts b/src/commands/completion.ts index a0487e5740..89f3c46d1c 100644 --- a/src/commands/completion.ts +++ b/src/commands/completion.ts @@ -1,4 +1,3 @@ -import ora from 'ora'; import { CompletionFactory } from '../core/completions/factory.js'; import { COMMAND_REGISTRY } from '../core/completions/command-registry.js'; import { detectShell, SupportedShell } from '../utils/shell-detection.js'; @@ -125,6 +124,9 @@ export class CompletionCommand { const generator = CompletionFactory.createGenerator(shell); const installer = CompletionFactory.createInstaller(shell); + // Loaded here, not at the top: `__complete` shares this module and runs on + // every Tab press, so it must not pay for the spinner. + const { default: ora } = await import('ora'); const spinner = ora(`Installing ${shell} completion script...`).start(); try { @@ -234,6 +236,7 @@ export class CompletionCommand { } } + const { default: ora } = await import('ora'); const spinner = ora(`Uninstalling ${shell} completion script...`).start(); try { diff --git a/src/core/completions/completion-provider.ts b/src/core/completions/completion-provider.ts index 0159131486..75606f4e0f 100644 --- a/src/core/completions/completion-provider.ts +++ b/src/core/completions/completion-provider.ts @@ -1,5 +1,4 @@ import { getActiveChangeIds, getSpecIds } from '../../utils/item-discovery.js'; -import { listSchemas } from '../artifact-graph/index.js'; /** * Cache entry for completion data @@ -96,7 +95,10 @@ export class CompletionProvider { return this.schemaCache.data; } - // Fetch fresh data + // Fetch fresh data. The resolver (and the yaml and zod it parses schemas + // with) loads only here: completing changes or specs never needs it, and + // the artifact-graph barrel would also pull in fast-glob. + const { listSchemas } = await import('../artifact-graph/resolver.js'); const schemaNames = listSchemas(this.projectRoot); // Update cache diff --git a/test/cli-e2e/startup-modules.test.ts b/test/cli-e2e/startup-modules.test.ts index 3eadf4d913..6f7cd4302d 100644 --- a/test/cli-e2e/startup-modules.test.ts +++ b/test/cli-e2e/startup-modules.test.ts @@ -118,6 +118,32 @@ describe('CLI startup loads only what the command needs', () => { } ); + // Shell completion scripts run `__complete` on every Tab press. It needs no + // package beyond commander, and never telemetry: tracking would hold each + // press open for the request. + it.each(['changes', 'specs'])( + '`openspec __complete %s` loads only commander and no telemetry', + async (type) => { + await ensureCliBuilt(); + const { own, packages, status } = loadedModules(['__complete', type]); + + expect(status).toBe(0); + expect([...packages]).toEqual(['commander']); + expect(implementationsLoaded(own)).toEqual(['completion']); + expect(own).not.toContain('telemetry/index.js'); + } + ); + + it('`openspec __complete schemas` loads the schema reader but not fast-glob or ora', async () => { + await ensureCliBuilt(); + const { own, packages, status } = loadedModules(['__complete', 'schemas']); + + expect(status).toBe(0); + expect(packages).not.toContain('fast-glob'); + expect(packages).not.toContain('ora'); + expect(own).not.toContain('telemetry/index.js'); + }); + it.each([ ['config list --json', 'config'], ['config path', 'config'], diff --git a/test/core/cli-is-json-run.test.ts b/test/core/cli-is-json-run.test.ts index 420dcf57cd..2b7a9b722b 100644 --- a/test/core/cli-is-json-run.test.ts +++ b/test/core/cli-is-json-run.test.ts @@ -1,7 +1,12 @@ import { describe, it, expect } from 'vitest'; import { Command, Option } from 'commander'; -import { isJsonRun, isCompletionRun, shouldDeferCompletionTip } from '../../src/cli/index.js'; +import { + isJsonRun, + isCompletionRun, + isTabCompletionRun, + shouldDeferCompletionTip, +} from '../../src/cli/index.js'; /** * Reproduce the three ways `--json` reaches a command in the real CLI, so a @@ -106,6 +111,23 @@ describe('isCompletionRun', () => { ); }); +describe('isTabCompletionRun', () => { + /** + * Only the hidden resolver skips telemetry. The user-facing `completion` + * commands are run by a person and are tracked like any other command. + */ + it('matches the hidden resolver', () => { + expect(isTabCompletionRun('__complete')).toBe(true); + }); + + it.each(['completion', 'completion:install', 'completion:generate', 'list'])( + 'does not match "%s"', + (commandPath) => { + expect(isTabCompletionRun(commandPath)).toBe(false); + } + ); +}); + describe('shouldDeferCompletionTip', () => { /** * The tip must survive every run that cannot display it. Deferring (rather From 54d011df715bcfdf1914b52a7d4becde7bcbe3a8 Mon Sep 17 00:00:00 2001 From: Clay Good Date: Tue, 13 Oct 2026 15:53:31 -0500 Subject: [PATCH 2/2] test(completion): pin the privacy fix, the skip's narrowness, and zsh's redirect The change is sound - completion output is byte-identical on all four types, the hook skip matches the command path exactly, and all four generated scripts discard `__complete` stderr. What was thin was the coverage around it. The startup assertions pin that `telemetry/index.js` is not loaded. They do not pin the consequence that makes this worth doing. The disclosure goes to stderr, every completion script sends `__complete` stderr to /dev/null, and `maybeShowTelemetryNotice` marks `noticeSeen` before `trackCommand` re-reads it - so a user whose first-ever interaction was a Tab press had the notice burned where nobody could read it and collection started on that same press. A new e2e test runs `__complete` with telemetry left enabled and an isolated HOME, and asserts stderr is empty and no config file is written. Two traps in that test, both of which made it pass vacuously before they were fixed. `HOME` has to be isolated as well as `XDG_CONFIG_HOME`, or `migrateLegacyTelemetryConfig` imports the developer's real config. And the opt-out variables must be deleted rather than set to `''`: both read as "set to anything that is not an explicit on-value", so blanking them switches telemetry off and the assertions then hold on any build. Verified against main, where the test now fails on the notice appearing in stderr. Added a negative control, because nothing asserted that normal commands still reach the hooks. `isTabCompletionRun` matches exactly, while the adjacent `isCompletionRun` prefix-matches `completion:*`; widening one into the other would silently stop tracking that whole group with every existing assertion still green. The zsh generator was the one shell whose test did not pin the stderr redirect, though it has the most call sites and is the macOS default. It now checks every real invocation, skipping the comment lines that name the command without running it. Also made the schemas test say what its name promises: it now pins that the resolver loads and the artifact-graph barrel does not, since the barrel re-exports the same function but pulls in fast-glob. 6414 tests pass; lint clean. Co-Authored-By: Claude Opus 5 --- test/cli-e2e/startup-modules.test.ts | 75 ++++++++++++++++++- .../generators/zsh-generator.test.ts | 30 ++++++++ 2 files changed, 102 insertions(+), 3 deletions(-) diff --git a/test/cli-e2e/startup-modules.test.ts b/test/cli-e2e/startup-modules.test.ts index 6f7cd4302d..2ee546d5dc 100644 --- a/test/cli-e2e/startup-modules.test.ts +++ b/test/cli-e2e/startup-modules.test.ts @@ -1,6 +1,6 @@ import { afterAll, describe, expect, it } from 'vitest'; import { spawnSync } from 'node:child_process'; -import { mkdtempSync, readFileSync, realpathSync, rmSync } from 'node:fs'; +import { existsSync, mkdtempSync, readFileSync, realpathSync, rmSync } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; @@ -139,9 +139,78 @@ describe('CLI startup loads only what the command needs', () => { const { own, packages, status } = loadedModules(['__complete', 'schemas']); expect(status).toBe(0); - expect(packages).not.toContain('fast-glob'); - expect(packages).not.toContain('ora'); + expect([...packages]).not.toContain('fast-glob'); + expect([...packages]).not.toContain('ora'); expect(own).not.toContain('telemetry/index.js'); + // The name promises a schema reader, so pin it: the resolver directly, not + // the artifact-graph barrel, which re-exports it but also pulls outputs.js + // and with it fast-glob. + expect(own).toContain('core/artifact-graph/resolver.js'); + expect(own).not.toContain('core/artifact-graph/index.js'); + }); + + // Negative control for the two cases above. `isTabCompletionRun` matches the + // command path exactly, unlike the adjacent `isCompletionRun`, which + // prefix-matches `completion:*`. Widening one into the other would silently + // stop tracking every user-facing command in that group, and every assertion + // above would still pass. + it.each([['list --json'], ['completion generate zsh']])( + '`openspec %s` still reaches the telemetry hooks', + async (invocation) => { + await ensureCliBuilt(); + const { own } = loadedModules(invocation.split(' ')); + + expect(own).toContain('telemetry/index.js'); + expect(own).toContain('core/completion-tip.js'); + } + ); + + // The module-count assertions above pin that telemetry is not LOADED. They do + // not pin the user-visible consequence, which is the reason the skip exists: + // the first-run disclosure goes to stderr, every generated completion script + // sends `__complete` stderr to /dev/null, and `maybeShowTelemetryNotice` marks + // `noticeSeen` before `trackCommand` re-reads it. So before this change a + // user's first-ever interaction being a Tab press burned the disclosure where + // nobody could read it and began collecting on that same press. + // + // Telemetry is deliberately left ENABLED here: the point is that an enabled + // run still records nothing. HOME must be isolated as well as XDG_CONFIG_HOME, + // or `migrateLegacyTelemetryConfig` imports the developer's real config from + // ~/.config/openspec/config.json and the test passes vacuously. + it('`openspec __complete` leaves the first-run telemetry notice unconsumed', async () => { + await ensureCliBuilt(); + const home = mkdtempSync(path.join(os.tmpdir(), 'openspec-notice-')); + workDirs.push(home); + const configHome = path.join(home, 'config'); + + // These have to be DELETED, not blanked. Both opt-outs read as "set to + // anything that is not an explicit on-value", so `OPENSPEC_TELEMETRY: ''` + // and `DO_NOT_TRACK: ''` would switch telemetry OFF and the assertions + // below would hold on any build, passing vacuously. + const env = { + ...process.env, + HOME: home, + USERPROFILE: home, + XDG_CONFIG_HOME: configHome, + XDG_DATA_HOME: path.join(home, 'data'), + }; + delete env.OPENSPEC_TELEMETRY; + delete env.DO_NOT_TRACK; + delete env.CI; + + const result = spawnSync(process.execPath, [binPath, '__complete', 'changes'], { + cwd: home, + env, + encoding: 'utf-8', + timeout: 30_000, + windowsHide: true, + }); + + expect(result.status).toBe(0); + // No disclosure written to a stream the completion scripts discard. + expect(result.stderr).toBe(''); + // And no config file, so neither noticeSeen nor an anonymousId was minted. + expect(existsSync(path.join(configHome, 'openspec', 'config.json'))).toBe(false); }); it.each([ diff --git a/test/core/completions/generators/zsh-generator.test.ts b/test/core/completions/generators/zsh-generator.test.ts index 376f96350e..2f8425d222 100644 --- a/test/core/completions/generators/zsh-generator.test.ts +++ b/test/core/completions/generators/zsh-generator.test.ts @@ -421,5 +421,35 @@ describe('ZshGenerator', () => { expect(script).toContain('_openspec_view() {'); expect(script).toContain('_arguments'); }); + + // The bash, fish and powershell generator tests each pin this redirect; + // zsh was the one that did not, and it has the most `__complete` call + // sites and is the default shell on macOS. The redirect is what keeps a + // Tab press silent, so anything `__complete` ever writes to stderr - a + // deprecation notice, a Node experimental warning - would otherwise print + // into the middle of the user's prompt with no test objecting. + it('sends every __complete call stderr to /dev/null', () => { + const commands: CommandDefinition[] = [ + { name: 'archive', description: 'Archive a change', flags: [] }, + { name: 'show', description: 'Show an item', flags: [] }, + { name: 'new', description: 'Create a change', flags: [] }, + ]; + + const script = generator.generate(commands); + + expect(script).toContain('openspec __complete changes 2>/dev/null'); + expect(script).toContain('openspec __complete specs 2>/dev/null'); + expect(script).toContain('openspec __complete schemas 2>/dev/null'); + // No invocation may survive a template edit that drops a redirect. Only + // real call sites count: the script also explains itself in comments + // that name the command without running it. + const invocations = script + .split('\n') + .filter((line) => line.includes('openspec __complete') && !line.trim().startsWith('#')); + expect(invocations.length).toBeGreaterThan(0); + for (const line of invocations) { + expect(line, `unredirected __complete call: ${line.trim()}`).toContain('2>/dev/null'); + } + }); }); });