fix(telemetry): suppress first-run notice in --json mode - #1609
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>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe CLI 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
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
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.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The implementation is sound, but the living spec contradicts itself: the generic first-command scenario says every first command shows the notice while the JSON scenario says it does not. Qualify the generic case as non-JSON so the required behavior is unambiguous.
The generic 'First command execution' scenario asserted the notice displays on every first command, contradicting the JSON scenario that says it does not. Qualify it as 'without --json' so the required behavior is unambiguous. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@alfred-openspec addressed: qualified the generic First command execution scenario as without |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The living spec now makes the first-command and JSON scenarios mutually exclusive. Fresh build, 43 focused CLI and telemetry tests, strict change validation, and the exact-head hosted matrix pass.
Both conventions exist in this repo's history, but the two most recent behavior fixes (#1609, #1616) carry an `openspec/changes/` delta rather than editing the main spec in place, which is also the workflow this project asks of everyone else. The delta reproduces the whole Capability Retirement requirement, so archiving it drops no scenario. Verified by archiving into a scratch copy of `openspec/`: the merged main spec differs from today's by exactly the three added bullets.
* fix(archive): never dead-end a capability retirement A change whose delta removes the last requirement a capability has rebuilds the main spec empty, which can never validate. Archive already knows retiring is the fix and names the `retire_capabilities: true` marker that authorises deleting the spec - but only when the marker is the single thing missing. If the spec also holds a line the merge cannot account for (a `## Notes` section, a comment under a requirement - both ordinary), that hint was suppressed, and the hint that names such lines only spoke to authors who had already set the marker. Neither fired, so the archive aborted on "Spec must have at least one requirement" with no guidance at all: the exact dead end the marker exists to close. Archive now names the blocking content in that case. It deliberately does not name the marker there - adding it would not have let this run through, and the marker is only ever named when it really is the one thing missing. Once the content is resolved, the rerun names the marker. Closes Fission-AI#1696 * fix(archive): harden the blocked-retirement abort Three follow-ups to the same message. The blocking lines are authored spec content printed verbatim to a terminal, so they now get the treatment `describeChangeName` already gives a change directory name: control characters replaced, since a raw CR could forge a line of its own and an ESC could redraw the screen. Each line is bounded too - one very long line would push the way out of the abort off the reader's screen - and the cut counts code points so it can never leave half a surrogate pair. Both the declared and undeclared branches share the helper, so the marker-declared abort that shipped with Fission-AI#1484 is hardened with it. The wording no longer claims retiring is "the way through". It is not, in the one case this fires on that has a live requirement hiding in a second `## Requirements` section: merging the sections fixes that spec without deleting anything. `openspec/specs/cli-archive/spec.md` records the behavior change - the blocking lines are named whether or not the marker was declared, and the marker is still named only when adding it would let the archive through. * refactor(archive): drop a helper the revised wording made single-use The marker sentence is said in one place again, so it goes back inline rather than through a function that now has one caller. Also corrects the comment above `emptiedByThisRun`: retiring is not the only fix in every case it covers, which is exactly why the message stopped saying so. * docs(openspec): record the change as a delta, not a direct spec edit Both conventions exist in this repo's history, but the two most recent behavior fixes (Fission-AI#1609, Fission-AI#1616) carry an `openspec/changes/` delta rather than editing the main spec in place, which is also the workflow this project asks of everyone else. The delta reproduces the whole Capability Retirement requirement, so archiving it drops no scenario. Verified by archiving into a scratch copy of `openspec/`: the merged main spec differs from today's by exactly the three added bullets. * fix(archive): report an unhonorable marker alongside the blocking content An author who set `retire_capabilities: yes-please` believes they have authorised the deletion. Clearing the blocking content first, only to then learn the marker was never read, is two aborts for one mistake. The abort still never invites the marker to be added while content blocks the retirement - it only reports the one already there. The spec delta records that distinction, which the old bullet ("say nothing about the marker") did not draw. * style(archive): use one sentence for an unhonorable marker in both aborts * fix(metadata): strip control characters from an unhonorable marker reason Every reason a boolean change-metadata marker gives quotes something the author wrote - a schema name, a parser message carrying one, a filesystem error carrying a path - and two commands print it straight to a terminal. A schema name carrying a raw ESC, with the marker set, put that ESC on screen through `openspec archive`; `openspec validate` prints the same reason. Fixed at the source in `readBooleanMarker` rather than at either call site, so no consumer has to remember. The reason still quotes the name recognisably; only control characters are replaced. Reported by CodeRabbit on Fission-AI#1699. Pre-existing on main, and this PR would have added a second place it reaches the terminal. * test(archive): fix a comment left behind by the reworded abort
2 participants
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