fix(telemetry): suppress first-run notice in --json mode - #1609
fix(telemetry): suppress first-run notice in --json mode#1609clay-good wants to merge 3 commits into
Conversation
The first-run telemetry disclosure notice was written to stdout from the global preAction hook. On a user's first-ever command with --json this polluted stdout and could break JSON parsers. Read the executing command's --json flag (actionCommand.opts().json) and, when set, skip the notice and leave noticeSeen unset so the disclosure is deferred to the first later non-JSON run rather than lost. Spinner suppression, new-change --json output, and structured JSON errors already landed on main (Fission-AI#960, Fission-AI#1190); this closes the one remaining stdout writer in --json mode. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe CLI now suppresses the first-run telemetry notice during ChangesTelemetry notice suppression
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Telemetry
participant Config
CLI->>CLI: Detect --json from command options or residual arguments
CLI->>Telemetry: Call maybeShowTelemetryNotice({silent: true})
Telemetry->>Config: Keep noticeSeen unset
CLI->>Telemetry: Call maybeShowTelemetryNotice() on later non-JSON run
Telemetry->>Config: Persist noticeSeen after displaying the notice
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/telemetry/index.test.ts (1)
195-208: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a CLI-level regression test for the
preActionwiring.These tests call
maybeShowTelemetryNotice()directly. They do not verify that an actual--jsoncommand setsactionCommand.opts().jsonor that the complete stdout is one parseable JSON document. Add a fresh-process CLI test for a first--jsonrun, followed by a non-JSON run that verifies deferred disclosure.As per coding guidelines, run the focused test with
pnpm exec vitest run test/telemetry/index.test.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/telemetry/index.test.ts` around lines 195 - 208, Add a fresh-process CLI regression test covering the preAction wiring: execute a first command with --json, verify stdout contains only one parseable JSON document and no telemetry notice, then execute a non-JSON command and verify the deferred “OpenSpec collects anonymous usage stats” disclosure appears. Keep the existing direct maybeShowTelemetryNotice test unchanged and run the focused test with pnpm exec vitest run test/telemetry/index.test.ts.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openspec/changes/suppress-telemetry-notice-in-json/specs/telemetry/spec.md`:
- Around line 6-9: Qualify the “First command execution” scenario in the
telemetry specification as applying only to non-JSON commands, either by
renaming it to “First non-JSON command execution” or adding an explicit
condition that the command does not pass --json; preserve the existing
telemetry-enabled notice behavior.
---
Nitpick comments:
In `@test/telemetry/index.test.ts`:
- Around line 195-208: Add a fresh-process CLI regression test covering the
preAction wiring: execute a first command with --json, verify stdout contains
only one parseable JSON document and no telemetry notice, then execute a
non-JSON command and verify the deferred “OpenSpec collects anonymous usage
stats” disclosure appears. Keep the existing direct maybeShowTelemetryNotice
test unchanged and run the focused test with pnpm exec vitest run
test/telemetry/index.test.ts.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c1fda48d-c821-42f2-964c-075d0d6d0ae2
📒 Files selected for processing (8)
.changeset/suppress-telemetry-notice-json.mdopenspec/changes/suppress-telemetry-notice-in-json/.openspec.yamlopenspec/changes/suppress-telemetry-notice-in-json/proposal.mdopenspec/changes/suppress-telemetry-notice-in-json/specs/telemetry/spec.mdopenspec/changes/suppress-telemetry-notice-in-json/tasks.mdsrc/cli/index.tssrc/telemetry/index.tstest/telemetry/index.test.ts
| #### Scenario: First command execution | ||
| - **WHEN** a user runs their first openspec command | ||
| - **AND** telemetry is enabled | ||
| - **THEN** the system displays: "Note: OpenSpec collects anonymous usage stats. Opt out: OPENSPEC_TELEMETRY=0" |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Qualify the generic first-run scenario as non-JSON.
Lines 7-9 apply to every first command, but Lines 20-23 define no notice for a first --json command. The specification therefore contains contradictory acceptance criteria. Rename this scenario to First non-JSON command execution or add AND the command does not pass --json.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@openspec/changes/suppress-telemetry-notice-in-json/specs/telemetry/spec.md`
around lines 6 - 9, Qualify the “First command execution” scenario in the
telemetry specification as applying only to non-JSON commands, either by
renaming it to “First non-JSON command execution” or adding an explicit
condition that the command does not pass --json; preserve the existing
telemetry-enabled notice behavior.
The preAction guard read actionCommand.opts().json, which only sees a
declared leaf option. That missed two supported --json forms that emit a
single JSON document to stdout:
- openspec store --json (permissive group reads --json from residual args;
never declares the option, so opts().json is undefined)
- openspec workset --json <sub> (--json on the parent group, consumed
before the leaf; leaf opts().json is undefined)
Both would still print the first-run telemetry notice ahead of their JSON.
Detect --json from process.argv instead: it covers leaf, parent, and
residual-arg forms uniformly. Suppressing is always safe (the disclosure
defers to the next non-JSON run, never lost), so a broad argv check is the
correct, conservative signal.
Also add a direct assertion that noticeSeen stays unset after a silent run,
and note the pre-existing raw-stdout commands (completion generate, config
get/path, __complete) as out of scope.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Replace the process.argv check with isJsonRun(command), an exported pure helper that reads Commander's parsed state: optsWithGlobals().json (leaf and parent-group forms) OR command.args (residual --json on permissive bare groups like store). This is tied to the actually-parsed command rather than raw args, and — unlike process.argv — is unit-testable in-process. Add test/core/cli-is-json-run.test.ts: a synthetic program reproducing all three registration patterns proves isJsonRun returns true for status --json, store --json, workset --json list, and workset list --json, and false otherwise. This locks in the store/workset coverage against future regressions (an e2e test can't: telemetry is disabled under CI, so the notice never fires there). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/index.ts`:
- Around line 132-136: Update isJsonRun so --json is recognized only when parsed
as an option, not when present in command.args after the -- terminator. Preserve
the parsed option state and add a regression covering ['store', '--', '--json']
that expects false.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6eeed1b7-0835-4511-a47d-333252bd9e6d
📒 Files selected for processing (3)
openspec/changes/suppress-telemetry-notice-in-json/proposal.mdsrc/cli/index.tstest/core/cli-is-json-run.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- openspec/changes/suppress-telemetry-notice-in-json/proposal.md
| export function isJsonRun(command: Command): boolean { | ||
| return ( | ||
| command.optsWithGlobals().json === true || | ||
| command.args.includes('--json') | ||
| ); |
There was a problem hiding this comment.
🎯 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:
Commander.js terminal option terminator -- options after stop option processing args
💡 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:
- 1: https://github.com/tj/commander.js/blob/HEAD/docs/options-in-depth.md
- 2: https://github.com/tj/commander.js/
- 3: https://github.com/tj/commander.js/blob/master/Readme.md
- 4: https://github.com/tj/commander.js/blob/v15.0.0/Readme.md
- 5: https://www.npmjs.com/package/commander
- 6: How to stop parsing for options after first argument? tj/commander.js#1127
- 7: https://registry.npmjs.org/commander
🌐 Web query:
Commander.js v14 allowExcessArguments parse behavior command.args after --
💡 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:
- 1: https://github.com/tj/commander.js/blob/HEAD/CHANGELOG.md
- 2: Disallow excess arguments tj/commander.js#2223
- 3: https://github.com/tj/commander.js/releases/tag/v13.0.0
- 4: Add .allowExcessArguments() and error message tj/commander.js#1407
- 5: https://github.com/tj/commander.js/blob/master/Readme.md
- 6: https://www.npmjs.com/package/commander
- 7: https://github.com/tj/commander.js/tree/master?tab=readme-ov-file
- 8: https://github.com/tj/commander.js/blob/HEAD/docs/options-in-depth.md
- 9: Double Dash Feature tj/commander.js#2139
Do not treat a post-terminator operand as JSON mode.
command.args includes arguments after --, and Commander stops option parsing at --. An invocation such as openspec store -- --json will match command.args.includes('--json') and suppress the telemetry notice as JSON output, even though --json is an operand, not an option. Preserve whether --json was parsed as an option and add a regression for ['store', '--', '--json'] that expects false.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/cli/index.ts` around lines 132 - 136, Update isJsonRun so --json is
recognized only when parsed as an option, not when present in command.args after
the -- terminator. Preserve the parsed option state and add a regression
covering ['store', '--', '--json'] that expects false.
1 participant
Status
LGTM — merge-ready. Full regression gate is clean for this delta (build, lint,
tsc --noEmit,openspec validate --strict, changeset, and the full test suite all pass; the fix adds two new passing tests and zero new failures).Credit
This picks up the last un-shipped sliver of #742 by @CosticaPuntaru ("suppress UI spinners and telemetry notices when
--jsonis used"). Full credit to them for identifying the JSON-cleanliness problem and the telemetry-notice piece specifically. Most of #742 has since shipped independently, so rather than force-push over their branch this is a fresh, minimal PR for the one remaining gap. See the "History" section below.What was missing
openspec <cmd> --jsonis 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 remained: the first-run telemetry disclosure notice.On a user's first-ever command,
maybeShowTelemetryNotice()runs from the globalpreActionhook andconsole.logs the disclosure to stdout, before the command's JSON payload. A--jsonconsumer parsing that first run gets invalid JSON. It's first-run-only (the notice then setsnoticeSeen), but that's exactly the run an automation hits on a fresh machine or CI image.What it does
maybeShowTelemetryNotice()takes asilentoption. When silent it prints nothing and leavesnoticeSeenunset — so the disclosure is deferred, not skipped.preActionhook suppresses the notice whenever--jsonappears in the invocation, detected fromprocess.argv.Why argv, not a parsed option:
--jsonreaches commands three ways, and a single parsed option (actionCommand.opts().json) misses two of them:openspec status --json✓ (a parsed check would catch this one)optsWithGlobals—openspec workset --json list(leaf'sopts().jsonis undefined)openspec store --json(emits a single JSON document via raw-arg detection)Detecting
--jsonfrom argv covers all three uniformly. It's the conservative choice: suppressing is always safe (worst case the disclosure defers one run — never lost), while printing the notice on a JSON run corrupts stdout.Net effect: any
--jsoninvocation never emits the notice on stdout; the user still sees the disclosure on their first later non-JSON run. Telemetry stays opt-out and otherwise unchanged; no new data is collected.Out of scope: a few commands write scriptable output to stdout without a
--jsonflag (completion generate,config get,config path, the hidden__complete). Their first-run notice pollution is a separate, pre-existing issue not addressed here.This also incorporates the maintainer review feedback left on #742: the
noticeSeen-persisted-while-silent bug is fixed (deferral), and the spec is a propertelemetryMODIFIED delta rather than a newmachine-readable-outputcapability.Proof it works
test/telemetry/index.test.ts: first-run--json(silent) prints nothing and leavesnoticeSeenunset; the disclosure still appears on the first later non---jsonrun.test/core/cli-is-json-run.test.ts: a synthetic Commander program reproducing all three registration patterns provesisJsonRunreturns true forstatus --json,store --json,workset --json list, andworkset list --json, and false otherwise — a regression guard for the store/workset coverage (an e2e test can't guard it: telemetry is disabled under CI, so the notice never fires there).status --json,store --json,workset --json list, andworkset list --jsoneach produce clean, valid JSON on stdout with zero notice lines; a first-ever non-JSONstatusshows the notice (deferral intact).openspec validate suppress-telemetry-notice-in-json --strictpasses.artifact-workflow,config-profile,command-generation/adapters) fail identically on pristinemain— pre-existing and unrelated to this change.History / why this is small
The bulk of #742 shipped independently after it was opened:
status/instructions/templates/new-change→ fix: suppress ora spinner when --json is used #960 (which also closed the original issue ora spinner output mixes with --json, breaking AI agent JSON parsing #957).--jsonerrors routed cleanly to stdout/stderr → feat(stores)!: replace workspaces and initiatives with stores #1190.new change --jsonoutput → already onmain(a superset of fix: suppress UI spinners and telemetry notices when --json is used #742's version).So #957 is already resolved; this PR closes no issue. The only genuinely-remaining behavior from #742 is the telemetry-notice guard above.
Related (not closed): #1526 (spinner ANSI to non-TTY stdout) is the same failure family for
archive, handled separately by #1603.Summary by CodeRabbit
Bug Fixes
Tests