-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(telemetry): suppress first-run notice in --json mode #1609
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
77d41eb
9d4c119
6164504
6b1568b
2781119
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,8 @@ | ||
| --- | ||
| "@fission-ai/openspec": patch | ||
| --- | ||
|
|
||
| Suppress the first-run telemetry disclosure notice when `--json` is used. On a | ||
| first-ever run the notice was written to stdout and could break `--json` | ||
| consumers; it is now deferred to the first later non-JSON run, keeping `--json` | ||
| output valid while still guaranteeing the disclosure. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,2 @@ | ||
| schema: spec-driven | ||
| created: 2026-08-07 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| # Suppress the first-run telemetry notice in --json mode | ||
|
|
||
| ## Why | ||
|
|
||
| `openspec <cmd> --json` is meant to emit exactly one machine-readable JSON | ||
| document on stdout so agents and automation can parse it. Spinner suppression | ||
| and structured JSON errors already ship on main, but one stdout writer remains: | ||
| the first-run telemetry disclosure notice. | ||
|
|
||
| On a user's first-ever command, `maybeShowTelemetryNotice()` runs from the | ||
| global `preAction` hook and `console.log`s the disclosure to **stdout** — before | ||
| the command's JSON payload. A `--json` consumer parsing that first run gets | ||
| invalid JSON. It is first-run-only (the notice sets `noticeSeen`), but that is | ||
| exactly the run an automation is most likely to hit on a fresh machine or CI | ||
| image. | ||
|
|
||
| ## What Changes | ||
|
|
||
| - `maybeShowTelemetryNotice()` accepts a `silent` option. When silent, it prints | ||
| nothing **and** leaves `noticeSeen` unset, so the disclosure is deferred rather | ||
| than skipped. | ||
| - The `preAction` hook passes `silent: true` when the executing command asked | ||
| for JSON, decided by `isJsonRun(command)`. `--json` reaches commands three | ||
| ways, so a single parsed option (`opts().json`) is not enough: on the leaf | ||
| (`status --json`), on a parent group read via `optsWithGlobals` | ||
| (`workset --json list`), and as a residual arg on permissive groups that never | ||
| declare the option (`openspec store --json`). `isJsonRun` checks | ||
| `optsWithGlobals().json` and `command.args`, covering all three. | ||
|
|
||
| Net effect: any `--json` invocation never emits the notice on stdout; the user | ||
| still sees the disclosure on their first later non-JSON run. Suppressing is | ||
| always safe — worst case the disclosure defers one run. Telemetry remains opt-out | ||
| and otherwise unchanged. | ||
|
|
||
| Out of scope: a few commands write scriptable output to stdout without a `--json` | ||
| flag (`completion generate`, `config get`, `config path`, the hidden `__complete`). | ||
| Their first-run notice pollution is a separate, pre-existing issue not addressed | ||
| here. | ||
|
|
||
| ## Impact | ||
|
|
||
| - Affected specs: `telemetry` (MODIFIED: First-run telemetry notice) | ||
| - Affected code: `src/telemetry/index.ts`, `src/cli/index.ts` | ||
| - No change to non-JSON behavior; no new events or data collected. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| ## MODIFIED Requirements | ||
|
|
||
| ### Requirement: First-run telemetry notice | ||
| The system SHALL display a one-line telemetry disclosure notice on the first command execution, before any telemetry is sent. In `--json` mode the system SHALL NOT display the notice on that run and SHALL leave `noticeSeen` unset, deferring the disclosure to the first later non-JSON run. | ||
|
|
||
| #### Scenario: First command execution | ||
| - **WHEN** a user runs their first openspec command without `--json` | ||
| - **AND** telemetry is enabled | ||
| - **THEN** the system displays: "Note: OpenSpec collects anonymous usage stats. Opt out: OPENSPEC_TELEMETRY=0" | ||
|
|
||
| #### Scenario: Subsequent command execution | ||
| - **WHEN** a user has already seen the notice (noticeSeen: true in config) | ||
| - **THEN** the system does not display the notice | ||
|
|
||
| #### Scenario: Notice before telemetry | ||
| - **WHEN** displaying the first-run notice | ||
| - **THEN** the notice appears before any telemetry event is sent | ||
|
|
||
| #### Scenario: First command execution in JSON mode | ||
| - **WHEN** a user's first openspec command passes `--json` | ||
| - **AND** telemetry is enabled | ||
| - **THEN** the system displays no notice on stdout | ||
| - **AND** `noticeSeen` remains unset | ||
|
|
||
| #### Scenario: Disclosure deferred, not skipped | ||
| - **WHEN** a user's first run was in `--json` mode and displayed no notice | ||
| - **AND** the user later runs a command without `--json` | ||
| - **THEN** the system displays the disclosure notice on that later run | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # Tasks | ||
|
|
||
| ## 1. Suppress notice in JSON mode | ||
| - [x] 1.1 Add a `silent` option to `maybeShowTelemetryNotice()` that skips the notice and leaves `noticeSeen` unset | ||
| - [x] 1.2 Read `actionCommand.opts().json` in the `preAction` hook and pass `silent` accordingly | ||
|
|
||
| ## 2. Tests | ||
| - [x] 2.1 Assert a first-run `--json` (silent) call prints nothing and does not mark the notice seen | ||
| - [x] 2.2 Assert the disclosure still appears on the first later non-silent run |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -115,6 +115,27 @@ export function getCommandPath(command: Command): string { | |
| return names.join(':') || 'openspec'; | ||
| } | ||
|
|
||
| /** | ||
| * True when the executing command asked for JSON output — used to suppress the | ||
| * first-run telemetry notice so stdout stays a single valid JSON document. | ||
| * | ||
| * `--json` reaches commands three ways, so a single parsed option is not enough: | ||
| * - declared on the leaf (`openspec status --json`) → `opts().json` | ||
| * - declared on a parent group and read via globals (`openspec workset --json list`) | ||
| * → `optsWithGlobals().json` | ||
| * - a residual arg on a permissive group that never declares the option | ||
| * (`openspec store --json`, which detects it from `command.args`) → `args` | ||
| * | ||
| * Suppressing is always safe: the disclosure is only deferred to the next | ||
| * non-JSON run, never lost, whereas printing it on a JSON run corrupts stdout. | ||
| */ | ||
| export function isJsonRun(command: Command): boolean { | ||
| return ( | ||
| command.optsWithGlobals().json === true || | ||
| command.args.includes('--json') | ||
| ); | ||
|
Comment on lines
+132
to
+136
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
rg -n -C 4 'allowUnknownOption|allowExcessArguments|--json|isJsonRun' src testRepository: Fission-AI/OpenSpec Length of output: 50376 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Inspect the CLI helper and surrounding tests, and list dependency versions/config snippets.
sed -n '120,170p' src/cli/index.ts
printf '\n--- relevant telemetry helper/tests ---\n'
sed -n '1,230p' test/telemetry/index.test.ts
printf '\n--- package commander version refs ---\n'
rg -n '"commander"|commander' package.json pnpm-lock.yaml package-lock.json yarn.lock 2>/dev/null | head -80Repository: Fission-AI/OpenSpec Length of output: 9706 🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate definitions and usages that make subcommands accept residual --json.
rg -n "allowUnknownOption|allowExcessArguments|command\.args|function createProgram|program|subcommand|hooks" src test/telemetry/index.test.ts | head -200
printf '\n--- candidate source files ---\n'
git ls-files src | rg 'cli|command|index' | head -100Repository: Fission-AI/OpenSpec Length of output: 15953 🌐 Web query:
💡 Result: In Commander.js, the double dash (--) serves as a terminal option terminator [1][2][3]. When encountered, it instructs the parser to immediately stop processing further command-line arguments as options [2][4][3]. Any remaining arguments provided after the -- are treated strictly as positional operands (or command-arguments) and are not interpreted or parsed as flags, even if they begin with a dash (-) [1][5][6]. This behavior aligns with the standard POSIX guideline (Guideline 10), which uses -- as a delimiter to signify the end of options [6]. Common use cases include: 1. Passing arguments that start with a dash to a subcommand or script, ensuring they are not mistakenly parsed as options for your program [6]. 2. Separating your program's own flags from external arguments that might conflict with them [1]. If you want to avoid requiring your users to use --, you have a few alternatives: - Put your options before any positional arguments [1]. - Use.passThroughOptions to stop processing options automatically when a non-option argument is encountered, allowing subsequent arguments (including options meant for another program) to be passed through unprocessed [2][7][3]. - Incorporate -- into your documented usage syntax if you want to make it an explicit part of your command interface [1]. Citations:
🌐 Web query:
💡 Result: In Commander.js v14, the behavior of command-line arguments is defined by the default setting of.allowExcessArguments(false) [1][2]. 1. Excess Arguments Behavior Since version 13.0.0, Commander throws an error by default if a user provides more arguments than the command expects [2][3]. If your application receives unexpected arguments, the parser will stop and display an error message (e.g., "error: too many arguments") [1][4]. To revert to legacy behavior where excess arguments are silently ignored or collected in command.args, you must explicitly call.allowExcessArguments on your program or command instance [1][3]. 2. Parsing with -- The -- separator is a standard feature in Commander.js that signals the end of option processing [5][6]. Any arguments appearing after -- are treated as literal arguments, not as options [5][7]. Crucially, when -- is used: - Arguments following -- are still subject to the.allowExcessArguments check [4]. If you have a command that expects specific arguments and you provide more than that total (including those after --), Commander will still throw a "too many arguments" error unless.allowExcessArguments(true) is configured [4]. - These arguments are typically captured in command.args if they do not violate the excess argument limit [5][6]. - If you intend to pass arbitrary arguments through to a subprocess or another utility, using -- effectively stops the current command's option parser from misinterpreting flags that follow [8][6]. If you find yourself needing to manually parse or trim these arguments, consider using.passThroughOptions, which can sometimes obviate the need for -- by passing all subsequent arguments through to the action handler or a subcommand [9][6]. Citations:
Do not treat a post-terminator operand as JSON mode.
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| program | ||
| .name('openspec') | ||
| .description('AI-native system for spec-driven development') | ||
|
|
@@ -133,8 +154,9 @@ program.hook('preAction', async (thisCommand, actionCommand) => { | |
| process.env.NO_COLOR = '1'; | ||
| } | ||
|
|
||
| // Show first-run telemetry notice (if not seen) | ||
| await maybeShowTelemetryNotice(); | ||
| // Show first-run telemetry notice (if not seen). Suppress it whenever the run | ||
| // asked for JSON so stdout stays a single valid JSON document (see isJsonRun). | ||
| await maybeShowTelemetryNotice({ silent: isJsonRun(actionCommand) }); | ||
|
|
||
| // Track command execution (use actionCommand to get the actual subcommand) | ||
| const commandPath = getCommandPath(actionCommand); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,79 @@ | ||
| import { describe, it, expect } from 'vitest'; | ||
| import { Command, Option } from 'commander'; | ||
|
|
||
| import { isJsonRun } from '../../src/cli/index.js'; | ||
|
|
||
| /** | ||
| * Reproduce the three ways `--json` reaches a command in the real CLI, so a | ||
| * future refactor of the telemetry-notice guard can't silently reintroduce | ||
| * first-run stdout pollution for `store --json` / `workset --json <sub>`. | ||
| */ | ||
| function buildProgram(capture: (command: Command) => void): Command { | ||
| const program = new Command(); | ||
| program.name('openspec').exitOverride(); | ||
| program.configureOutput({ writeOut: () => {}, writeErr: () => {} }); | ||
| program.option('--no-color', 'Disable color output'); | ||
| program.hook('preAction', (_thisCommand, actionCommand) => { | ||
| capture(actionCommand); | ||
| }); | ||
|
|
||
| // 1. Leaf declares --json (e.g. `openspec status --json`). | ||
| program | ||
| .command('status') | ||
| .option('--json', 'Output as JSON') | ||
| .action(() => {}); | ||
|
|
||
| // 2. Permissive bare group that never declares --json and detects it from | ||
| // residual args (e.g. `openspec store --json`). | ||
| const store = program.command('store'); | ||
| store.allowExcessArguments(true); | ||
| store.allowUnknownOption(true); | ||
| store.action(() => {}); | ||
|
|
||
| // 3. Parent group declares --json (read via optsWithGlobals) with its own | ||
| // subcommands (e.g. `openspec workset --json list`). | ||
| const workset = program.command('workset'); | ||
| workset.addOption(new Option('--json', 'Output as JSON').hideHelp()); | ||
| workset | ||
| .command('list') | ||
| .option('--json', 'Output as JSON') | ||
| .action(() => {}); | ||
|
|
||
| return program; | ||
| } | ||
|
|
||
| describe('isJsonRun', () => { | ||
| async function actionCommandFor(argv: string[]): Promise<Command> { | ||
| let captured: Command | undefined; | ||
| const program = buildProgram((command) => { | ||
| captured = command; | ||
| }); | ||
| await program.parseAsync(['node', 'openspec', ...argv]); | ||
| if (!captured) throw new Error(`no action command captured for: ${argv.join(' ')}`); | ||
| return captured; | ||
| } | ||
|
|
||
| it('detects --json declared on the leaf command', async () => { | ||
| expect(isJsonRun(await actionCommandFor(['status', '--json']))).toBe(true); | ||
| }); | ||
|
|
||
| it('detects --json as a residual arg on a permissive bare group', async () => { | ||
| expect(isJsonRun(await actionCommandFor(['store', '--json']))).toBe(true); | ||
| }); | ||
|
|
||
| it('detects --json on a parent group placed before the subcommand', async () => { | ||
| expect(isJsonRun(await actionCommandFor(['workset', '--json', 'list']))).toBe(true); | ||
| }); | ||
|
|
||
| it('detects --json declared on the subcommand leaf', async () => { | ||
| expect(isJsonRun(await actionCommandFor(['workset', 'list', '--json']))).toBe(true); | ||
| }); | ||
|
|
||
| it('is false when no --json is present', async () => { | ||
| expect(isJsonRun(await actionCommandFor(['status']))).toBe(false); | ||
| }); | ||
|
|
||
| it('is false for a bare group with unrelated residual args', async () => { | ||
| expect(isJsonRun(await actionCommandFor(['store', 'bogus']))).toBe(false); | ||
| }); | ||
| }); |
Uh oh!
There was an error while loading. Please reload this page.