Repository navigation
Conversation
AI review: no P0-P2 findingsThis PR adds opt-out CLI usage telemetry: one event per command, sent to ⚪ P3
Not checked: The AI review read the diff, the code, and the PR's text; it ran no tests or builds and read nothing outside this repository.
Human review: needed
The AI review found no P0-P2 problems at fed48e1, so the code needs no second review for bugs. Decide the points above: approve to accept them, or request changes. Suggested reviewer: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| | Claude Code | `CLAUDECODE`, `CLAUDE_CODE_ENTRYPOINT` | | ||
| | Codex | `CODEX_THREAD_ID`, `CODEX_SANDBOX`, `CODEX_CI` | | ||
| | Gemini CLI | `GEMINI_CLI` | | ||
| | Cursor | `CURSOR_EXTENSION_HOST_ROLE=agent-exec` | |
There was a problem hiding this comment.
⚪ P3: Docs list Cursor before Cursor CLI, but the code checks Cursor CLI first
The doc says "If several are set, the first row in this table wins" and lists Cursor (CURSOR_EXTENSION_HOST_ROLE=agent-exec) above Cursor CLI (CURSOR_AGENT). In internal/agent/agent.go:92-93, markers puts CURSOR_AGENT → cursor-cli first. Running cursor-agent from a Cursor agent-exec terminal sets both variables, so the CLI reports cursor-cli while the published doc says cursor. Fix: swap the two table rows so they match the code.
Found by claude-opus-5-5
There was a problem hiding this comment.
Fixed: docs/agent-detection.md now lists Cursor CLI (CURSOR_AGENT) above Cursor, matching the order of markers in internal/agent/agent.go.
| {env: "ANTIGRAVITY_AGENT", agent: "antigravity"}, | ||
| // COPILOT_GITHUB_TOKEN is left out on purpose: people export it in their | ||
| // own shell profile, so it does not show an agent ran the command. | ||
| {env: "COPILOT_MODEL", agent: "github-copilot"}, |
There was a problem hiding this comment.
⚪ P3: COPILOT_MODEL and COPILOT_ALLOW_ALL may be shell-profile settings, not proof an agent ran
The code skips COPILOT_GITHUB_TOKEN because people export it in their shell profile. As far as I know, Copilot CLI also documents COPILOT_MODEL (choose the model) and COPILOT_ALLOW_ALL (auto-approve tools) as settings users export themselves. If so, a user with export COPILOT_MODEL=... in .zshrc gets every command they type tagged github-copilot with source marker. Because step-2 markers outrank VOLCANO_AGENT, an agent that identifies itself only through VOLCANO_AGENT would also be reported as Copilot. Consider dropping these two markers, or treating them as a host-level signal, unless Copilot sets them in its own command shells.
Found by claude-opus-5-5
There was a problem hiding this comment.
Fixed: COPILOT_MODEL and COPILOT_ALLOW_ALL are now weak host markers (host: true), so they only count when nothing stronger is set and never outrank VOLCANO_AGENT. The docs describe them as settings people can export in their own shell profile.
|
|
||
| ## Agent detection | ||
|
|
||
| Once set up, the CLI reports which agent ran each command to the Volcano API. |
There was a problem hiding this comment.
⚪ P3: "Once set up" suggests reporting depends on volcano setup
The new section says "Once set up, the CLI reports which agent ran each command." Detection actually runs on every API request whatever the setup state: detectAgent in versionProtocolDoer reads only environment variables and never checks whether volcano setup ran. A user who skips setup could wrongly think nothing is reported. Fix: say the CLI reports the agent on every API request whether or not volcano setup has run.
Found by claude-opus-5-5
There was a problem hiding this comment.
Fixed: the "Once set up" wording is gone. The doc now says the CLI reports the agent with each request when a coding agent runs a command, with no tie to volcano setup. It also says the headers wait for the first-run telemetry notice and stop when telemetry is off.
| | Claude Code | `CLAUDECODE`, `CLAUDE_CODE_ENTRYPOINT` | | ||
| | Codex | `CODEX_THREAD_ID`, `CODEX_SANDBOX`, `CODEX_CI` | | ||
| | Gemini CLI | `GEMINI_CLI` | | ||
| | Cursor | `CURSOR_EXTENSION_HOST_ROLE=agent-exec` | |
There was a problem hiding this comment.
🟡 P2: Cursor marker precedence is documented in the wrong order
When both CURSOR_EXTENSION_HOST_ROLE=agent-exec and CURSOR_AGENT=1 are set without an earlier match, this table promises cursor because the first row wins. However, internal/agent/agent.go checks CURSOR_AGENT first and reports cursor-cli. Swap these rows to match the implemented precedence.
Found by gpt-6.1-sol
There was a problem hiding this comment.
Fixed: the doc table now puts Cursor CLI above Cursor, matching the code's precedence.
| | `X-Volcano-Agent-Raw` | `claude-code_2-1-299_agent` | The original value of `AI_AGENT`, `AGENT`, or `VOLCANO_AGENT`, when it differs from the name | | ||
| | `X-Volcano-Agent-Markers` | `claude-code,cursor` | Every agent whose step 2 or step 5 variables are set. More than one means one agent ran inside another, for example Claude Code run from a Cursor terminal | | ||
|
|
||
| Values are limited to letters, digits, and `._-/@+:`, and to 64 characters. |
There was a problem hiding this comment.
🟡 P2: The stated header limits do not apply to the markers header
setAgentHeaders in internal/api/version_protocol.go joins marker names with commas without sanitizing or truncating the combined value. Consequently, even the documented claude-code,cursor example contains an excluded character, and sufficiently many markers can exceed 64 characters. Scope this statement to the environment-derived name and raw fields, and describe the marker list separately.
Found by gpt-6.1-sol
There was a problem hiding this comment.
Fixed: the docs scope the 64-character and character-set limits to the name and raw values, and describe the markers header separately as a comma-separated list of names from the table.
| | Augment | `AUGMENT_AGENT` | | ||
| | Antigravity | `ANTIGRAVITY_AGENT` | | ||
| | GitHub Copilot | `COPILOT_MODEL`, `COPILOT_ALLOW_ALL` | | ||
|
|
There was a problem hiding this comment.
⚪ P3: Docs table lists Cursor before Cursor CLI, but the code checks them the other way round
The doc says "If several are set, the first row in this table wins" and puts CURSOR_EXTENSION_HOST_ROLE=agent-exec (Cursor) above CURSOR_AGENT (Cursor CLI). In internal/agent/agent.go the markers slice puts CURSOR_AGENT first. So when both are set, the CLI reports cursor-cli, while the published doc says cursor. Fix: swap the two table rows, or swap the two marker entries, so the doc and code agree.
Found by claude-opus-5-5
There was a problem hiding this comment.
Fixed: the table now lists Cursor CLI first, matching markers in internal/agent/agent.go.
tkkhq
changed the title
|
|
||
| func printNotice(w io.Writer) { | ||
| fmt.Fprintln(w, "Note: the Volcano CLI collects usage data: the command and flag names you use (never their values), CLI version, platform, and which coding agent ran it. This helps us improve the CLI.") | ||
| fmt.Fprintf(w, "Turn it off with `volcano telemetry disable` or %s=1. Details: `volcano docs get telemetry`.\n", EnvDisabled) |
There was a problem hiding this comment.
🟡 P2: First-run notice points users to volcano docs get telemetry, which cannot resolve
docs.Service.Get reads the cached snapshot path exactly as given (snap.readFile(docPath) after safeRelPath). It adds no .md extension and does no fuzzy lookup, and doc IDs look like cli/deploy.md. AGENTS.md says this repo's docs/ syncs under /cli, so the new page will be at cli/telemetry.md. Running the exact command the notice prints fails with a doc-not-found error, at the one moment a user is told where to read about data collection. Change the hint to volcano docs get cli/telemetry.md, or point to the published URL.
Found by claude-opus-5-5
There was a problem hiding this comment.
Fixed: the notice now points to volcano telemetry --help, which needs no docs cache.
| // swallowed, since telemetry must not change what the CLI does. | ||
| func (r Recorder) Record(inv Invocation) { | ||
| r = r.withDefaults() | ||
| if inv.Command != nil && inv.Command.Annotations[SkipAnnotation] != "" { |
There was a problem hiding this comment.
🟡 P2: Every shell-completion request is recorded, spawns a flush process, and can silently use up the first-run notice
Only commands annotated with SkipAnnotation are skipped. Cobra's hidden __complete command (added by ExecuteC because the root does not disable completion) is returned as cmd, so every Tab press records an event. Each one also starts a detached volcano telemetry flush process that makes an HTTP POST. Cobra's bash, zsh, and fish completion scripts run __complete with 2>/dev/null. If a user's first interaction is a Tab completion, printNotice writes to a discarded stderr, NoticeShown is saved as true, and the user never sees the notice. Skip cobra.ShellCompRequestCmd / ShellCompNoDescRequestCmd (and possibly the completion command) in Record.
Found by claude-opus-5-5
There was a problem hiding this comment.
Fixed: shell completion (__complete and volcano completion <shell>) is skipped and never recorded, so it neither spawns a flush nor uses up the notice.
| if state.DeviceID == "" { | ||
| state.DeviceID = uuid.NewString() | ||
| } | ||
| if !state.NoticeShown && inv.Stderr != nil { |
There was a problem hiding this comment.
🟡 P2: Notice is printed on every command when ~/.volcano/telemetry.json cannot be written
Record prints the notice and sets NoticeShown, then returns if State.Save fails, so the flag is never saved. When HOME is readable but not writable, every invocation prints the two-line notice to stderr again and sends no event. That covers sandboxed agents (Codex's default workspace-write sandbox, Claude Code's sandbox), read-only containers, and Nix-style builds. These are the agent runs this feature targets, and every command's output picks up the noise. Save the state before printing, and print only when the save succeeds (or skip the notice when state cannot be saved).
Found by claude-opus-5-5
There was a problem hiding this comment.
Fixed: when the config cannot be saved (read-only home, sandbox), initialize returns an error and the CLI neither prints the notice nor sends anything. docs/telemetry.md says so.
| failure (`api`, `network`, `local_not_running`, `usage`, or `other`) and, for an | ||
| API error, its HTTP status and error code. | ||
|
|
||
| The CLI never sends flag values, command arguments, file contents, project or |
There was a problem hiding this comment.
⚪ P3: Docs say environment variable values are never sent, but free-text agent values are
For an unrecognized AI_AGENT, AGENT, or VOLCANO_AGENT value, canonical returns the sanitized, lowercased value itself. That value goes out as agent in the telemetry event and as X-Volcano-Agent/X-Volcano-Agent-Raw on every API request. AGENT in particular is a generic name a user may have set for something else. The privacy statement "never sends ... environment variable values" is therefore not accurate. Qualify it (e.g. "other than the agent name read from AI_AGENT/AGENT/VOLCANO_AGENT").
Found by claude-opus-5-5
There was a problem hiding this comment.
Fixed: docs/telemetry.md now names the two exceptions: the project ID from VOLCANO_PROJECT_ID, and the agent name and raw value from AI_AGENT, AGENT, or VOLCANO_AGENT, with the limits applied to them.
| } | ||
| return | ||
| } | ||
| _ = r.Send(payload) |
There was a problem hiding this comment.
🟡 P2: Honor the docs command's offline guarantee
With telemetry enabled, volcano docs list --offline still starts a background POST to /cli/telemetry, even when the docs command fails because its cache is missing. This breaks the explicit “Never touch the network” contract in internal/cmd/docs/docs.go and docs/docs-search.md. Suppress network telemetry for invocations using this offline mode.
Found by gpt-6.1-sol
There was a problem hiding this comment.
Fixed: --offline runs are never recorded, including when flag parsing fails before cobra reads the flag (offlineArg checks the raw arguments).
| printNotice(inv.Stderr) | ||
| state.NoticeShown = true | ||
| } | ||
| if err := r.State.Save(state); err != nil { |
There was a problem hiding this comment.
🟡 P2: Prevent initialization from overwriting a concurrent opt-out
A first-run recorder can load enabled state, overlap with volcano telemetry disable saving disabled:true, then overwrite that preference with its stale state here. Subsequent invocations resume telemetry despite the successful opt-out. Atomic rename prevents partial JSON but does not protect this read-modify-write sequence; serialize initialization and preference updates with a cross-process lock.
Found by gpt-6.1-sol
There was a problem hiding this comment.
Fixed: initialize writes the device ID and notice flag under the config lock and re-reads the saved state, so a concurrent volcano telemetry disable is never overwritten.
| case "opencode": | ||
| return "opencode" | ||
| } | ||
| return "" |
There was a problem hiding this comment.
🟡 P2: Map the supported pi harness before checking plugin status
After volcano setup --agent pi installs skills into ~/.pi/agent/skills, an invocation with VOLCANO_AGENT=pi reaches this fallback and checks ~/.volcano/skills instead. It therefore reports plugin_installed:false for a valid Pi installation, or reports an unrelated manual installation. Add the pi mapping supported by internal/setup/harness.go.
Found by gpt-6.1-sol
There was a problem hiding this comment.
Fixed: harnessFor maps pi and checks ~/.pi/agent/skills, matching internal/setup/harness.go.
| // wait or the login poll, is not one; an HTTP request that times out still | ||
| // counts, since the client wraps it in *url.Error. | ||
| func isNetworkError(err error) bool { | ||
| if _, ok := errors.AsType[*url.Error](err); ok { |
There was a problem hiding this comment.
⚪ P3: isNetworkError labels every *url.Error as a network failure, including URL parse errors and Ctrl-C
*url.Error is returned by url.Parse as well as by http.Client. It also wraps context.Canceled when the user interrupts a request. So a malformed URL (for example a bad VOLCANO_API_URL) or a cancelled command is reported as error_kind: network, which inflates the network-failure count this field exists to measure. Fix: check Op (skip parse), and exclude context.Canceled, before counting a *url.Error as network.
Found by claude-opus-5-5
There was a problem hiding this comment.
Valid, fixed in 4546ada: a context.Canceled error and a *url.Error with Op == "parse" are no longer counted as network. TestErrorKind covers both.
| known agent names to analytics and reports any other as `other`. See | ||
| [agent-detection.md](agent-detection.md). | ||
|
|
||
| If you are logged in, the event is linked to your Volcano account. Otherwise it |
There was a problem hiding this comment.
⚪ P3: Docs say logged-out events link only to a random machine ID, but that ID is also sent while logged in and survives logout
Event.DeviceID is always sent, including on events that carry the bearer token. config.ClearLogin deliberately keeps the telemetry block, so the same device_id survives volcano logout. The server therefore receives (device_id, account) pairs and can tie a machine's logged-out events back to the account that last used it. The sentence "Otherwise it is linked only to the random machine ID" reads as if logged-out events are anonymous. Either say that the machine ID is sent with logged-in events and kept after logout, or have logout generate a new device_id.
Found by claude-opus-5-5
There was a problem hiding this comment.
Valid, fixed in aa93c80: docs/telemetry.md now says every event carries the machine ID, and that it is kept after volcano logout, so logged-out events can be linked to the account last used on that machine. Logout keeps the ID on purpose; regenerating it would be a separate decision.
|
The AI review at c3f0ade had a P3 it posted only in its summary: the release checks in |
| terminal, the coding agent that ran it, whether the Volcano plugin is installed | ||
| for that agent, the exit code, and the duration. When a command fails, the | ||
| event also carries the failure type, the API error status and code, and the | ||
| error message with tokens and keys removed. Events are linked to your account |
There was a problem hiding this comment.
🟡 P2: volcano telemetry --help and the first-run notice understate how events are linked and what is sent
The help text says events are "linked to your account when you are logged in, and otherwise to a random ID for this machine". The first-run notice (internal/telemetry/telemetry.go printNotice) says only that events are linked to the account when logged in. In the code, build puts DeviceID on every event, logged in or not. ClearLogin keeps that ID across volcano logout, so events sent after logout can be tied to the last account. docs/telemetry.md was updated in aa93c80 to say exactly this, but the in-binary text was not. That text is what the notice points to as "Details", and the PR says it is the disclosure users can reach because older docs caches lack cli/telemetry.md. The help also lists "the selected project ID" with no qualifier, but only volcano cloud … commands send it. Fix: use the docs wording in both places — a machine ID is sent with every event and kept after logout, and the project ID is sent for cloud commands only.
Found by claude-opus-5-5
There was a problem hiding this comment.
Valid, fixed in 76a6cc8. volcano telemetry --help now says every event carries a random machine ID that volcano logout keeps, so events sent after logout can be linked to the account last used on that machine, and that the project ID is sent for volcano cloud commands only. The first-run notice says events are linked to a random machine ID and, when logged in, to the account.
…e machine ID in help and the notice
| nothing, so you can turn telemetry off before any event is sent. | ||
|
|
||
| The CLI saves that the notice was shown in `~/.volcano/config.json`. If it | ||
| can't write there, as in a sandbox with a read-only home directory, it shows no |
There was a problem hiding this comment.
🟡 P2: Limit the read-only-home guarantee to uninitialized telemetry state
A sandbox mounting an already initialized home read-only still sends telemetry: Recorder.Reporting enables agent headers, and Recorder.Record skips initialization when notice_shown and device_id already exist. Users relying on this guarantee can therefore transmit command, resource, and error data. Qualify the statement to homes where first-run state has not been saved, and direct users to the explicit opt-out.
Found by gpt-6.1-sol
There was a problem hiding this comment.
Valid, fixed in d5cadf9. docs/telemetry.md now limits the guarantee to a read-only home that has never run the CLI, says a home where the notice was already saved keeps sending when mounted read-only, and points to VOLCANO_TELEMETRY_DISABLED=1 for that case.
|
The AI review at 76a6cc8 had a P3 it posted only in its summary: the release build job's "Smoke-test Linux binary" step in |
| jwtPattern = regexp.MustCompile(`eyJ[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]*`) | ||
| keyPattern = regexp.MustCompile(`\b(?:(?:(?:pk|pt|sk|ak)-|vbt_|vpg_)[A-Za-z0-9_-]{16,}|sbp1\.[A-Za-z0-9+/=:]{16,})`) | ||
| userPattern = regexp.MustCompile(`([a-zA-Z][a-zA-Z0-9+.-]*://)[^\s/?#]*@`) | ||
| emailPattern = regexp.MustCompile(`[A-Za-z0-9._%+-]+@[A-Za-z][A-Za-z0-9-]*(?:\.[A-Za-z0-9-]+)*\.[A-Za-z]{2,}`) |
There was a problem hiding this comment.
🟡 P2: Redact email addresses with digit-led domains
Requiring a letter immediately after @ leaves valid addresses such as owner@163.com unchanged. When volcano cloud durable approvals deny <id> encounters an already-approved approval, ApprovalConflict and DecisionSummary in internal/durable/durable.go include the decider’s email, which telemetry then sends unredacted despite the documented guarantee. Support digit-led domains and handle asset-path false positives separately, or mark email-bearing errors sensitive.
Found by gpt-6.1-sol
There was a problem hiding this comment.
Valid, and a regression from my previous push. Fixed in Kong/volcano-cli 5f726dc / Kong/volcano-hosting 6178202ee: digit-led domains match again (alice@163.com, ops@1password.com, u@123corp.example), and only a first domain label of digits then x (logo@2x.png, icon@3x.webp) is skipped. Both sides have tests for each case and the patterns stay identical.
…style asset names
| resource names, though they can appear in an error message. A name that looks | ||
| like a path or a Volcano token or key is dropped. | ||
|
|
||
| The CLI does not send file contents or environment variable values, except in |
There was a problem hiding this comment.
⚪ P3: Docs say no env var values are sent except two, but error messages carry the VOLCANO_API_URL host and path
The doc says the CLI "does not send file contents or environment variable values, except in two cases" (VOLCANO_PROJECT_ID and the agent variables). But errorMessage sends the text of a failed request as is. A Go *url.Error reads like Get "https://api.corp.internal/projects/<id>/functions": dial tcp: lookup api.corp.internal: no such host. Redaction strips only user info and query strings, so a self-hosted or staging VOLCANO_API_URL host and the request path reach analytics on every network failure. Either name this case in the doc, or reduce URLs in error messages to scheme and host (or drop them) before sending.
Found by claude-opus-5-5
There was a problem hiding this comment.
Not changing. VOLCANO_API_URL is a developer override for pointing the CLI at a local or staging API; users run against the default host, whose request paths carry only the project ID and resource names that the event already sends.
| // agentReportingOff gates the agent headers (zero value: on). The entry point | ||
| // sets it when the user opted out of telemetry, since the API forwards these | ||
| // headers to product analytics too. | ||
| var agentReportingOff atomic.Bool |
There was a problem hiding this comment.
⚪ P3: Agent headers default to on; only main turns them off before the first-run notice
agentReportingOff is an atomic.Bool whose zero value means the X-Volcano-Agent* headers are sent. The promise that nothing is sent before the first-run notice, or after an opt-out, holds only because main() calls api.SetAgentReporting(recorder.Reporting()) first. Today that is the only entry point, so nothing leaks. But any new entry point or in-process caller of internal/api that skips this call would send agent headers without consent. Defaulting to off (an agentReportingOn flag that main sets) would fail closed. The api tests that expect the headers would then need to enable it explicitly.
Found by claude-opus-5-5
There was a problem hiding this comment.
Valid, fixed in 57381c8: the gate is now agentReportingOn, whose zero value sends no agent headers; main turns it on only when telemetry is reporting. New test TestVersionProtocolDoer_SendsNoAgentHeadersUntilReportingIsOn; the header tests turn reporting on explicitly.
| jwtPattern = regexp.MustCompile(`eyJ[A-Za-z0-9_-]+\.[A-Za-z0-9_-]+\.[A-Za-z0-9_-]*`) | ||
| keyPattern = regexp.MustCompile(`\b(?:(?:(?:pk|pt|sk|ak)-|vbt_|vpg_)[A-Za-z0-9_-]{16,}|sbp1\.[A-Za-z0-9+/=:]{16,})`) | ||
| userPattern = regexp.MustCompile(`([a-zA-Z][a-zA-Z0-9+.-]*://)[^\s/?#]*@`) | ||
| emailPattern = regexp.MustCompile(`[A-Za-z0-9._%+-]+@(?:[A-Za-z][A-Za-z0-9-]*|[0-9]+(?:[A-Za-wyz-][A-Za-z0-9-]*|x[A-Za-z0-9-]+)?)(?:\.[A-Za-z0-9-]+)*\.[A-Za-z]{2,}`) |
There was a problem hiding this comment.
🟡 P2: The asset-name exception leaves valid email addresses unredacted
The pattern excludes any domain whose first label is digits followed by x, including valid addresses such as alice@2x.example.com. Attempting to approve an already denied durable approval can include its decider's address through internal/durable.ApprovalConflict and DecisionSummary, so that address reaches telemetry unchanged despite the documented redaction guarantee. Restrict the asset exception to identifiable image filenames, or mark the email sensitive at its source.
Found by gpt-6.1-sol
There was a problem hiding this comment.
Valid, fixed in 957fe7b (and Kong/volcano-hosting b2d14e61b): the asset-name carve-out is gone and the email rule is back to its original form, so ops@10x.dev and a@2x.example.com are redacted. An asset name such as logo@2x.png is now redacted too; that costs a file name, while any narrower carve-out leaves some real address unredacted.
| } | ||
| if inv.Err != nil { | ||
| event.ErrorKind = errorKind(inv.Err) | ||
| event.ErrorStatus = api.Status(inv.Err) |
There was a problem hiding this comment.
🟡 P2: Config validation failures lose their API classification and status
When volcano cloud config deploy receives HTTP 422, internal/api.ApplyProjectConfig returns *api.ProjectConfigValidationError. That type does not wrap *api.Error, so api.Status returns zero and errorKind reports other. These server rejections therefore disappear from API-failure and HTTP-status telemetry. Preserve the 422 status on this error type and recognize it when building the event.
Found by gpt-6.1-sol
There was a problem hiding this comment.
Valid, fixed in 81e23ab: api.Status now returns 422 for *api.ProjectConfigValidationError, so the event carries error_kind: api and error_status: 422. No existing caller branches on 422. TestErrorKind covers it.
| // plain errors with fixed message prefixes. | ||
| func isUsageError(err error) bool { | ||
| msg := err.Error() | ||
| for _, prefix := range []string{ |
There was a problem hiding this comment.
🟡 P2: Classify Cobra flag-group validation failures as usage errors
volcano cloud databases restore app without --backup or --to produces at least one of the flags in the group ... is required; supplying both produces an error beginning if any flags in the group. Neither prefix matches here, so errorKind reports these CLI usage failures as other. The same affects storage object visibility commands. Include Cobra's flag-group error prefixes and cover these existing command validations in the classification tests.
Found by gpt-6.1-sol
There was a problem hiding this comment.
Valid, fixed in eb4ee22: isUsageError now matches cobra's three flag-group errors (if any flags in the group, at least one of the flags in the group). TestErrorKind_CobraFlagGroups runs real cobra commands with MarkFlagsOneRequired, MarkFlagsRequiredTogether, and MarkFlagsMutuallyExclusive, so the test fails if cobra changes the wording.
| ``` | ||
|
|
||
| This deletes local credentials but does not revoke the token. Revoke it in the | ||
| This removes local credentials and the selected project but does not revoke the |
There was a problem hiding this comment.
⚪ P3: Logout doc omits that function aliases are also removed
volcano logout --help now says logout removes "local credentials, the current project, and function aliases", but the published doc says only "local credentials and the selected project". A reader could take that to mean their saved functions alias mappings survive logout, which config.ClearLogin contradicts. The sentence is also broken across lines oddly ("Revoke it in the" / "Volcano dashboard"). Mention aliases and reflow the paragraph.
Found by claude-opus-5-5
There was a problem hiding this comment.
Valid, fixed in fed48e1: the doc now says logout removes local credentials, the selected project, and function aliases (matching config.ClearLogin), and the paragraph is reflowed.
|
The AI review at eb4ee22 also had a P3 in its summary only: an older CLI rewriting |
Tracking
POST /cli/telemetryand agent properties on PostHog events)VOLCANO_AGENT)Why
Summary
internal/agent: detect the coding agent from env vars in this order: namedAI_AGENT/AGENT, harness markers (CLAUDECODE,CODEX_THREAD_ID,GEMINI_CLI,CURSOR_AGENT,OPENCODE_CLIENT, ...),VOLCANO_AGENT, bare truthyAI_AGENT/AGENT(unknown), then host-terminal markers (CURSOR_TRACE_ID,REPL_ID); exact name matches only, canonical names, values sanitized to a header-safe charset capped at 64 charsX-Volcano-Agent,-Source,-Raw,-Markerson every API request fromversionProtocolDoer, and nothing when no agent is detected, telemetry is off, or the first-run notice has not been shown yetinternal/telemetry: onecli_command_invokedevent per run with command path, names of flags set, selected project ID, resource names and IDs, CLI version, OS, arch, CPU count, CI vendor, stdin TTY, agent name/source/markers, Volcano plugin installed + version for that agent (version sent only when it is semver-shaped), exit code, duration; every failed run adds error kind, HTTP status, API error code, and the error messagevolcano cloud ...commands, since local commands run against the local sandbox<database>,<name-or-id>,<bucket>, ...) and from thename,function,database,project,topic,bucketflags, keyed by placeholder or flag name; paths, queries, payloads, and sandbox commands are never sent, and values that don't look like a name, contain/(paths), or look like a secret are dropped; a flag can opt out withsensitive.FlagAnnotation, anddurable start --name(an idempotency key) does~, strip ANSI escapes and control characters, collapse whitespace, then redact bearer tokens, JWTs, Volcano keys and tokens (pk-/pt-/sk-/ak-/vbt_/vpg_/sbp1.), URL user info, email addresses, and URL query strings, and cut to 500 runes; the API applies the same steps again, and itsdocs/internal/guides/cli-telemetry.mdis the wire contractinternal/sensitivecovers errors that quote files or application data; a secret typed where no command takes one, such asvolcano DB_PASSWORD=hunter2, is not looked for and is sent as typed;docs/telemetry.mdtells users to pass secrets only through the inputs meant for theminternal/sensitiveto mark errors whose text can carry file contents or application data; telemetry replaces that part with[redacted]while the terminal still shows it; used forvolcano-config.yamlYAML/interpolation/decode/validation errors, env file parse errors (godotenv quotes the file), PostgreSQL errors from migrations, a function's passthrough error body involcano functions invoke, and an execution name passed wherevolcano durableexpects an execution ID; an API error code inside a sensitive error is dropped toovolcano telemetry flushprocess toPOST {apiURL}/cli/telemetrywith the saved login token when present, so commands never wait on the networkvolcano telemetry --help, which ships in the binary, since docs caches synced before this release lackcli/telemetry.mdsetup.PluginStatusto answer "is the Volcano plugin installed for this harness" from local files, without the harness presence probevolcano telemetry disable|enable|statusandVOLCANO_TELEMETRY_DISABLED=1;VOLCANO_TELEMETRY_DEBUG=1prints the event instead of sending (off unless set);volcano telemetrycommands are never recorded, so an opt-out isn't itself reportedtelemetryblock of~/.volcano/config.json, updated throughconfig.Updateso a first run can't overwrite a concurrentvolcano telemetry disablevolcano logoutto clear the token, user ID, current project, and function aliases instead of deletingconfig.json, so the telemetry setting and docs source survive a logout; the file is still removed when nothing else is in it, or when it can't be read or parsed (so logout still removes the token from a damaged or unreadable file)__complete, which runs on every Tab with stderr discarded, andvolcano completion <shell>, which rc files run on every new shell) or--offlineruns, checking raw args too so a flag parse error can't bypass itrun()after the exit code is known, usingExecuteCfor the executed command; telemetry never changes the exit code or output beyond the one-time noticeCOPILOT_MODEL/COPILOT_ALLOW_ALLas weak signals, since people export them in their own profile; mappito itsvolcano setupharnessdocs/telemetry.mdanddocs/agent-detection.md, linked fromdocs/README.mdanddocs/setup.mdinternal/agentaliases and markers here,allowedAgentsin volcano-hostinginternal/productanalytics/allowlist.go, and the harness list in volcano-skillsAGENTS.md/cli/telemetry, which needs Kong/volcano-hosting#1919 deployed first; before that the flush gets a 404 in the background and nothing breaksVerification
make lintgo test ./...go test -race ./internal/agent/ ./internal/api/HOMEagainst a local sink: first run printed the notice once and sent nothing, each later run posted one event to/cli/telemetrywith the expected command, flags, agent, exit code, and error kind; a failingcloud functions get hellocarried the project ID,{"name-or-id": "hello"}, and the error message;volcano telemetry status/disablesent nothing;DO_NOT_TRACK=1no longer opts out; aftervolcano telemetry disablenothing was sent;volcano logoutkept thetelemetryblock inconfig.jsonHOME(no notice on repeated runs) and__complete(no state written)volcano projects listtwice with a freshHOMEagainst a local server: the first run sent noX-Volcano-Agentheader and no event; the second sent bothCODEX_THREAD_ID+CURSOR_TRACE_ID,VOLCANO_AGENTonly, a CRLF injection attempt inAI_AGENT, and a clean env (no headers)