Telemetry proxy - agent detection#39
Conversation
- Added `getInvocationContext` function to determine if the CLI is invoked by an AI agent or a human based on environment variables. - Updated `trackEvent` to include `invocation_context` and `agent_name` in the payload sent to the telemetry service. - Expanded test coverage for `trackEvent` and `getInvocationContext` to validate new functionality.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR updates the telemetry plugin to (1) send metrics via a proxy using a detached “fire-and-forget” flush worker and a persistent on-disk retry queue, and (2) add AI agent vs human invocation detection and include that context in every event.
Changes:
- Replace direct HTTP posting from the main process with a detached subprocess flush worker (
src/flush-worker.js) and add a persistent retry queue (src/queue-store.js). - Add agent detection via environment variables (plus Copilot PATH markers) and attach
invocation_context/agent_nameattributes to emitted telemetry. - Update tests and README to reflect the new flush architecture and additional telemetry attributes; remove unused runtime deps.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/telemetry-lib.js |
Builds New Relic-style metric payloads, adds agent detection, and spawns detached flush worker instead of POSTing directly. |
src/flush-worker.js |
New detached worker that POSTs merged queued+current metrics and persists failures for retry. |
src/queue-store.js |
New persistent JSON-backed queue store under the aio config directory (XDG-aware). |
test/telemetry-lib.test.js |
Updates tests to assert spawned worker payloads, agent context attributes, env overrides, and postrun duration behavior. |
test/hooks.test.js |
Updates hook interface tests to validate flush-worker spawning instead of direct fetch usage. |
test/queue-store.test.js |
Adds unit tests for queue path resolution and queue read/write/clear behavior. |
test/flush-worker.test.js |
Adds unit tests for flush-worker merging, posting, and queue persistence on failure. |
README.md |
Documents environment opt-out, flush architecture, agent detection, and new payload shape. |
package.json |
Removes unused dependencies (os-name, splunk-logging). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const parsed = JSON.parse(process.argv[2]) | ||
| const { body, postUrl: url, headers } = parsed | ||
| if (!url || typeof url !== 'string') { | ||
| return | ||
| } | ||
| postUrl = url | ||
| if (headers && typeof headers === 'object') { | ||
| const safe = { ...headers } | ||
| delete safe['Api-Key'] | ||
| delete safe['api-key'] | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | ||
| } | ||
| currentMetrics = JSON.parse(body)[0].metrics | ||
| } catch { | ||
| // Malformed argument — nothing useful to do. | ||
| return | ||
| } | ||
|
|
||
| // Merge previously-queued metrics (if any) with the current event so they | ||
| // are all retried in a single POST. | ||
| const queuedMetrics = readQueue() | ||
| const allMetrics = [...queuedMetrics, ...currentMetrics] | ||
|
|
| try { | ||
| debug('POST %s requestHeaders=%o', postUrl, requestHeaders) | ||
| await fetch(postUrl, { | ||
| method: 'POST', | ||
| headers: requestHeaders, | ||
| body: JSON.stringify({ batches: [{ metrics: allMetrics }] }) | ||
| }) | ||
| // Successful delivery — the queue is no longer needed. | ||
| clearQueue() | ||
| } catch { | ||
| // Network failure — persist all metrics so the next invocation can retry. | ||
| writeQueue(allMetrics) | ||
| } |
| // Merge previously-queued metrics (if any) with the current event so they | ||
| // are all retried in a single POST. | ||
| const queuedMetrics = readQueue() | ||
| const allMetrics = [...queuedMetrics, ...currentMetrics] | ||
|
|
||
| try { | ||
| debug('POST %s requestHeaders=%o', postUrl, requestHeaders) | ||
| await fetch(postUrl, { | ||
| method: 'POST', | ||
| headers: requestHeaders, | ||
| body: JSON.stringify({ batches: [{ metrics: allMetrics }] }) | ||
| }) | ||
| // Successful delivery — the queue is no longer needed. | ||
| clearQueue() | ||
| } catch { | ||
| // Network failure — persist all metrics so the next invocation can retry. | ||
| writeQueue(allMetrics) | ||
| } |
| init: (versionString, binName, remoteConf = {}) => { | ||
| global.commandHookStartTime = Date.now() | ||
| rootCliVersion = versionString | ||
| if (remoteConf.fetchHeaders) { | ||
| fetchHeaders = remoteConf.fetchHeaders | ||
| } | ||
| if (remoteConf.postUrl) { | ||
| postUrl = remoteConf.postUrl | ||
| postUrl = remoteConf.postUrl || process.env.AIO_TELEMETRY_POST_URL || DEFAULT_TELEMETRY_POST_URL | ||
| fetchHeaders = { | ||
| 'Content-Type': 'application/json' | ||
| } |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 PR Reviewer
This is a well-structured refactor that replaces a synchronous Splunk/AEP telemetry POST with a fire-and-forget detached subprocess pattern, adds persistent retry queuing, and introduces agent detection. The code is clear, well-tested, and follows good practices. A few minor issues worth addressing: the header filtering in flush-worker.js has a case-sensitivity bug that could allow Api-Key through, the queue can grow unboundedly on repeated failures, and the flushPayload passes fetchHeaders to the worker which then redundantly re-merges DEFAULT_HEADERS.
📝 4 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| postUrl = url | ||
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( | ||
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') |
There was a problem hiding this comment.
The filter strips headers whose key lowercased equals 'api-key', but the DEFAULT_HEADERS are merged after, meaning any header named exactly 'Content-Type' passed in fetchHeaders will correctly override. However the intent seems to be blocking sensitive credential headers. The filter only blocks 'api-key' but not common credential headers like 'authorization', 'x-api-key', 'x-ingest-key', etc. At minimum add 'authorization' and 'x-api-key' to the blocklist, or invert to an allowlist.
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
| * @param {Array<object>} items Flat array of New Relic metric objects. | ||
| */ | ||
| function writeQueue (items) { | ||
| const file = getQueuePath() | ||
| try { | ||
| fs.mkdirSync(path.dirname(file), { recursive: true }) | ||
| fs.writeFileSync(file, JSON.stringify(items), 'utf8') | ||
| } catch { | ||
| // silently ignore — failing to persist the queue must not affect the CLI |
There was a problem hiding this comment.
There is no cap on the queue size. If the telemetry endpoint is down for an extended period, the queue will grow indefinitely, consuming disk space. Add a max-items guard before writing.
| * @param {Array<object>} items Flat array of New Relic metric objects. | |
| */ | |
| function writeQueue (items) { | |
| const file = getQueuePath() | |
| try { | |
| fs.mkdirSync(path.dirname(file), { recursive: true }) | |
| fs.writeFileSync(file, JSON.stringify(items), 'utf8') | |
| } catch { | |
| // silently ignore — failing to persist the queue must not affect the CLI | |
| function writeQueue (items) { | |
| const MAX_QUEUE_SIZE = 100 | |
| const file = getQueuePath() | |
| try { | |
| fs.mkdirSync(path.dirname(file), { recursive: true }) | |
| const capped = items.slice(-MAX_QUEUE_SIZE) | |
| fs.writeFileSync(file, JSON.stringify(capped), 'utf8') | |
| } catch { | |
| // silently ignore — failing to persist the queue must not affect the CLI | |
| } | |
| } |
| nodeVersion: process.version, | ||
| osNameVersion, | ||
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | ||
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | ||
| } |
There was a problem hiding this comment.
The flushPayload includes fetchHeaders, but flush-worker.js also defines its own DEFAULT_HEADERS and merges them again. This means Content-Type is always set twice (harmlessly), but the round-trip serialization of headers is redundant if the worker already applies defaults. More importantly, if fetchHeaders is mutated between spawning and the worker parsing (unlikely but possible in tests), behavior could be surprising. Consider only passing extra/override headers and letting the worker own DEFAULT_HEADERS entirely.
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| const flushPayload = JSON.stringify({ | |
| body: fetchConfig.body, | |
| postUrl, | |
| headers: Object.fromEntries( | |
| Object.entries(fetchHeaders).filter(([k]) => k.toLowerCase() !== 'content-type') | |
| ) | |
| }) |
| async function trackEvent (eventType, rawEventData = {}) { | ||
| // prerunEvent will be null when telemetry-prompt event fires, this happens before | ||
| // any command is actually run, so we want to ignore the command+flags in this case |
There was a problem hiding this comment.
The AIO_TELEMETRY_DISABLED check uses a truthy JS check on the env string, meaning AIO_TELEMETRY_DISABLED=0 or AIO_TELEMETRY_DISABLED=false would still disable telemetry, which contradicts the README's claim of 'any truthy value'. This is inconsistent — env vars are always strings, so '0' and 'false' are truthy in JS. Either document this clearly or normalize the check.
| async function trackEvent (eventType, rawEventData = {}) { | |
| // prerunEvent will be null when telemetry-prompt event fires, this happens before | |
| // any command is actually run, so we want to ignore the command+flags in this case | |
| const isEnvDisabled = process.env.AIO_TELEMETRY_DISABLED !== undefined && | |
| process.env.AIO_TELEMETRY_DISABLED !== '' && | |
| process.env.AIO_TELEMETRY_DISABLED !== '0' && | |
| process.env.AIO_TELEMETRY_DISABLED.toLowerCase() !== 'false' | |
| const optedOut = isDisabledForCommand || isEnvDisabled || config.get(`${configKey}.optOut`, 'global') === true |
There was a problem hiding this comment.
🤖 PR Reviewer
This is a substantial, well-structured refactor: replacing a direct HTTP call with a fire-and-forget detached subprocess, adding a persistent retry queue, and introducing agent-detection telemetry. Code quality is good overall, tests are comprehensive, and the architecture is sound. Three previously raised issues remain unaddressed (header blocklist, queue size cap, and the AIO_TELEMETRY_DISABLED truthy-string semantics), and there is one new issue with an argument injection risk when passing the flush payload as a CLI argument.
📝 4 suggestion(s) (1 new, 3 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| const safe = Object.fromEntries( | ||
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
There was a problem hiding this comment.
[Re-raised] The filter strips only 'api-key'. Common credential headers like 'authorization', 'x-api-key', and 'x-ingest-key' are not blocked. Expand the blocklist.
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
| /** | ||
| * Persists the given metrics array to the queue file, creating directories as needed. | ||
| * Silently ignores write errors — telemetry must never crash the CLI. | ||
| * @param {Array<object>} items Flat array of New Relic metric objects. |
There was a problem hiding this comment.
[Re-raised] There is no cap on the queue size. If the endpoint is unreachable for an extended period the file will grow unboundedly. Cap at a reasonable maximum.
| * @param {Array<object>} items Flat array of New Relic metric objects. | |
| function writeQueue (items) { | |
| const MAX_QUEUE_SIZE = 100 | |
| const file = getQueuePath() | |
| try { | |
| fs.mkdirSync(path.dirname(file), { recursive: true }) | |
| const capped = items.slice(-MAX_QUEUE_SIZE) | |
| fs.writeFileSync(file, JSON.stringify(capped), 'utf8') | |
| } catch { | |
| // silently ignore — failing to persist the queue must not affect the CLI | |
| } | |
| } |
| osNameVersion | ||
| } | ||
| }) | ||
| body: JSON.stringify([{ |
There was a problem hiding this comment.
[Re-raised] AIO_TELEMETRY_DISABLED uses a JS truthy check on a string. The strings '0' and 'false' are truthy in JS, so they would incorrectly disable telemetry. Normalize the check to match the documented behaviour.
| body: JSON.stringify([{ | |
| const envDisabled = process.env.AIO_TELEMETRY_DISABLED !== undefined && | |
| process.env.AIO_TELEMETRY_DISABLED !== '' && | |
| process.env.AIO_TELEMETRY_DISABLED !== '0' && | |
| process.env.AIO_TELEMETRY_DISABLED.toLowerCase() !== 'false' | |
| const optedOut = isDisabledForCommand || envDisabled || config.get(`${configKey}.optOut`, 'global') === true |
| osNameVersion, | ||
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | ||
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | ||
| } | ||
| }] | ||
| }]) | ||
| } | ||
| const flushPayload = JSON.stringify({ | ||
| body: fetchConfig.body, |
There was a problem hiding this comment.
The entire JSON-serialized flush payload is passed as a single CLI argument (process.argv[2]). On Windows, argument length is capped around 32 767 characters and can silently truncate or fail. For large queues or long eventData strings this will cause data loss. Consider writing the payload to a temp file and passing the file path instead, then deleting it in the worker after parsing.
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] | |
| }]) | |
| } | |
| const flushPayload = JSON.stringify({ | |
| body: fetchConfig.body, | |
| const tmpFile = path.join(os.tmpdir(), `aio-telemetry-${Date.now()}-${Math.random().toString(36).slice(2)}.json`) | |
| try { | |
| require('fs').writeFileSync(tmpFile, flushPayload, 'utf8') | |
| } catch (err) { | |
| debug('Failed to write telemetry payload to temp file: %O', err) | |
| return | |
| } | |
| try { | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), tmpFile], { | |
| env: { ...process.env, AIO_TELEMETRY_DISABLED: '1' }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) | |
| child.unref() | |
| } catch (err) { | |
| debug('Failed to launch telemetry flush worker: %O', err) | |
| } |
…ut only 1 server call
There was a problem hiding this comment.
🤖 PR Reviewer
The refactor is well-structured and the test coverage is thorough. Three issues from the previous review remain unaddressed (header blocklist too narrow, unbounded queue, AIO_TELEMETRY_DISABLED truthy-string bug), and the Windows argument-length concern is still present. One new minor issue exists: fetchHeaders is re-initialized to a bare Content-Type object in init, so any remoteConf.fetchHeaders passed by the host CLI is silently ignored despite being documented in the README.
📝 5 suggestion(s) (1 new, 4 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
⚠️ Inline comments could not be attached (lines not in diff). See summary above.
| init: (versionString, binName, remoteConf = {}) => { | ||
| global.commandHookStartTime = Date.now() | ||
| rootCliVersion = versionString | ||
| if (remoteConf.fetchHeaders) { | ||
| fetchHeaders = remoteConf.fetchHeaders | ||
| } | ||
| if (remoteConf.postUrl) { | ||
| postUrl = remoteConf.postUrl | ||
| postUrl = remoteConf.postUrl || process.env.AIO_TELEMETRY_POST_URL || DEFAULT_TELEMETRY_POST_URL | ||
| fetchHeaders = { | ||
| 'Content-Type': 'application/json' | ||
| } |
| // TODO: detect VSCODE run as an agent | ||
| /** | ||
| * Environment variables checked for agent detection (proposed standard first, then tool-specific). | ||
| * Used for metrics only. See aio-cli README "Agent detection" for full list. | ||
| */ | ||
| const AGENT_ENV_VARS = [ | ||
| { env: 'AGENT', name: (v) => (v && v !== '1' && v !== 'true' ? String(v).toLowerCase() : 'generic') }, | ||
| { env: 'AI_AGENT', name: (v) => (v && v !== '1' && v !== 'true' ? String(v).toLowerCase() : 'generic') }, |
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( | ||
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | ||
| } | ||
| currentMetrics = JSON.parse(body)[0].metrics | ||
| } catch { | ||
| // Malformed argument — nothing useful to do. | ||
| return | ||
| } | ||
|
|
||
| // Merge previously-queued metrics (if any) with the current event so they | ||
| // are all retried in a single POST. | ||
| const queuedMetrics = readQueue() | ||
| const allMetrics = [...queuedMetrics, ...currentMetrics] | ||
|
|
| /** | ||
| * Appends metric objects to the end of the queue (read + merge + write). | ||
| * Silently ignores invalid input or write errors. | ||
| * @param {Array<object>} newItems Flat array of New Relic metric objects. | ||
| */ | ||
| function appendToQueue (newItems) { | ||
| if (!Array.isArray(newItems) || newItems.length === 0) { | ||
| return | ||
| } | ||
| const existing = readQueue() | ||
| writeQueue([...existing, ...newItems]) | ||
| } |
| - `fetchHeaders`: Optional extra headers merged into telemetry requests (`Content-Type` is always set) | ||
| - `productPrivacyPolicyLink`: A link to display to users when prompting to opt in | ||
| - `productName`: How to refer to the CLI when the user is prompted to enable telemetry (from `displayName` or `name` in `package.json`) | ||
| - `productBin`: Shown in help text (from `bin` in `package.json`; if several bins exist, the first is used). Example: run `${productBin} telemetry on` | ||
|
|
There was a problem hiding this comment.
🤖 PR Reviewer
This is a well-structured refactor that replaces a direct Splunk/AEP HTTP POST with a fire-and-forget detached flush worker, adds a persistent retry queue, and introduces agent detection. The code is clean, well-documented, and has comprehensive test coverage. A few minor issues around security, correctness, and robustness are noted below.
📝 6 suggestion(s) - Please review inline comments below.
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| const { body, postUrl: url, headers } = parsed | ||
| if (!url || typeof url !== 'string') { | ||
| return | ||
| } | ||
| postUrl = url | ||
| if (headers && typeof headers === 'object') { |
There was a problem hiding this comment.
The header filter only blocks 'api-key' (case-insensitively), but callers could inadvertently pass 'authorization', 'x-api-key', or other sensitive headers. Since the parent already controls fetchHeaders and strips secrets before spawning, consider either allowlisting known safe headers or at minimum blocking a broader set of sensitive header names.
| const { body, postUrl: url, headers } = parsed | |
| if (!url || typeof url !== 'string') { | |
| return | |
| } | |
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
There was a problem hiding this comment.
If body is valid JSON but does not contain an array at index 0, or if metrics is missing, this will throw a TypeError that is silently swallowed. Add a guard to avoid misleading silent failures.
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| const parsed = JSON.parse(process.argv[2]) | |
| const { body, postUrl: url, headers } = parsed | |
| if (!url || typeof url !== 'string') { | |
| return | |
| } | |
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| currentMetrics = parsedBody[0].metrics |
| fs.mkdirSync(path.dirname(file), { recursive: true }) | ||
| fs.writeFileSync(file, JSON.stringify(items), 'utf8') | ||
| } catch { | ||
| // silently ignore — failing to persist the queue must not affect the CLI |
There was a problem hiding this comment.
The queue file can grow unboundedly if the telemetry endpoint is unreachable for a long time (e.g. offline laptop). Consider capping the number of retained metrics to prevent disk/memory issues on the next flush.
| fs.mkdirSync(path.dirname(file), { recursive: true }) | |
| fs.writeFileSync(file, JSON.stringify(items), 'utf8') | |
| } catch { | |
| // silently ignore — failing to persist the queue must not affect the CLI | |
| function readQueue () { | |
| const MAX_QUEUE_SIZE = 500 | |
| try { | |
| const data = fs.readFileSync(getQueuePath(), 'utf8') | |
| const parsed = JSON.parse(data) | |
| if (!Array.isArray(parsed)) return [] | |
| return parsed.slice(-MAX_QUEUE_SIZE) | |
| } catch { | |
| return [] | |
| } | |
| } |
| debug('trackEvent %s eventData=%j postUrl=%s willSend=%s', eventType, eventData, postUrl, willSend) | ||
|
|
||
| if (optedOut) { |
There was a problem hiding this comment.
process.env.AIO_TELEMETRY_DISABLED is truthy for any non-empty string (including '0' or 'false'). This is documented intentionally, but it may surprise operators. At minimum, add a comment clarifying this is intentional and consistent with standard CLI env-var conventions.
| debug('trackEvent %s eventData=%j postUrl=%s willSend=%s', eventType, eventData, postUrl, willSend) | |
| if (optedOut) { | |
| // AIO_TELEMETRY_DISABLED: any non-empty string disables telemetry (including '0'/'false'). | |
| // This is intentional — operators should set it to '1' to disable, and unset/empty to enable. | |
| const optedOut = isDisabledForCommand || process.env.AIO_TELEMETRY_DISABLED || config.get(`${configKey}.optOut`, 'global') === true |
| }] | ||
| }]) | ||
| } | ||
| const metrics = JSON.parse(fetchConfig.body)[0].metrics | ||
|
|
||
| if (eventType !== 'postrun') { | ||
| // Queue until postrun so only one detached flush worker runs per command (avoids concurrent merges). | ||
| queueStore.appendToQueue(metrics) | ||
| return | ||
| } | ||
|
|
||
| const flushPayload = JSON.stringify({ |
There was a problem hiding this comment.
The flush payload passes fetchHeaders (which includes user-supplied headers) as a CLI argument string. If fetchHeaders contains values with characters that are significant in shell contexts (quotes, newlines), and the spawned process is ever launched via shell:true, this could be a command injection vector. The current spawn uses shell:false (default) so it is safe today, but it is worth adding an explicit comment and ensuring shell is never set to true.
| }] | |
| }]) | |
| } | |
| const metrics = JSON.parse(fetchConfig.body)[0].metrics | |
| if (eventType !== 'postrun') { | |
| // Queue until postrun so only one detached flush worker runs per command (avoids concurrent merges). | |
| queueStore.appendToQueue(metrics) | |
| return | |
| } | |
| const flushPayload = JSON.stringify({ | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), flushPayload], { | |
| // shell MUST remain false (the default) — flushPayload is passed as a raw argument | |
| // to avoid any shell interpolation of header values or URLs. | |
| env: { ...process.env, AIO_TELEMETRY_DISABLED: '1' }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) |
There was a problem hiding this comment.
🤖 PR Reviewer
This is a substantial refactor that replaces a direct HTTP POST with a fire-and-forget detached subprocess queue/flush architecture. The code is well-structured with good test coverage. Several previously-raised security issues remain unaddressed (incomplete header denylist in both flush-worker.js and telemetry-lib.js, full process.env passed to child, fragile body re-parsing, non-string PATH guard, redundant mkdirSync in readQueue), plus one new structural issue in flush-worker.js where the POST body format is inconsistent with what telemetry-lib.js produces.
📝 8 suggestion(s) (1 new, 7 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| } | ||
| postUrl = url | ||
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( |
There was a problem hiding this comment.
[Re-raised] The header filter only blocks 'api-key' (case-insensitively), but callers could inadvertently pass 'authorization', 'x-api-key', or other sensitive headers. Consider blocking a broader set of sensitive header names.
| const safe = Object.fromEntries( | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | ||
| } | ||
| currentMetrics = JSON.parse(body)[0].metrics |
There was a problem hiding this comment.
[Re-raised] If body is valid JSON but does not contain an array at index 0, or if metrics is missing, this will throw a TypeError that is silently swallowed by the outer catch. Add an explicit guard.
| currentMetrics = JSON.parse(body)[0].metrics | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| currentMetrics = parsedBody[0].metrics |
| try { | ||
| debug('POST %s requestHeaders=%o', postUrl, requestHeaders) | ||
| const res = await fetch(postUrl, { | ||
| method: 'POST', |
There was a problem hiding this comment.
The flush-worker POSTs with body shape { batches: [{ metrics: allMetrics }] } but telemetry-lib.js serializes the body as [{ metrics: [...] }] (a top-level array). These formats are inconsistent — the worker re-parses the body to extract [0].metrics correctly, but then re-wraps in a different shape. Both sides should agree on the wire format sent to the proxy.
| method: 'POST', | |
| const res = await fetch(postUrl, { | |
| method: 'POST', | |
| headers: requestHeaders, | |
| body: JSON.stringify([{ metrics: allMetrics }]) | |
| }) |
| } | ||
|
|
||
| const flushPayload = JSON.stringify({ | ||
| body: fetchConfig.body, |
There was a problem hiding this comment.
[Re-raised] The flush worker subprocess inherits the full process.env (including potentially sensitive variables like AWS credentials, tokens, etc.) with only AIO_TELEMETRY_DISABLED added. Consider passing a minimal env to the worker instead of spreading all of process.env.
| body: fetchConfig.body, | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), flushPayload], { | |
| env: { | |
| PATH: process.env.PATH, | |
| HOME: process.env.HOME, | |
| XDG_CONFIG_HOME: process.env.XDG_CONFIG_HOME, | |
| NODE_PATH: process.env.NODE_PATH, | |
| AIO_TELEMETRY_DISABLED: '1' | |
| }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) |
| commandDuration: timestamp - prerunEvent.start, | ||
| commandFlags: prerunEvent.flags.toString(), | ||
| commandSuccess: eventType !== 'command-error', | ||
| nodeVersion: process.version, |
There was a problem hiding this comment.
[Re-raised] The line const metrics = JSON.parse(fetchConfig.body)[0].metrics re-parses JSON that was just serialized. The metrics array is already available locally. Build it first and reference it directly.
| nodeVersion: process.version, | |
| const metricsArray = [{ | |
| name: 'aio.cli.telemetry', | |
| type: 'gauge', | |
| value: 1, | |
| timestamp, | |
| attributes: { | |
| eventType, | |
| eventData: formatEventDataAttribute(eventData), | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] | |
| const fetchConfig = { | |
| method: 'POST', | |
| headers: fetchHeaders, | |
| body: JSON.stringify([{ metrics: metricsArray }]) | |
| } | |
| const metrics = metricsArray |
| } | ||
|
|
||
| // TODO: detect VSCODE run as an agent | ||
| /** |
There was a problem hiding this comment.
[Re-raised] detectCopilotAgent checks pathValue.includes(...) without guarding against a non-string value. On systems where PATH is set to a non-string (e.g. via test injection), this would throw a TypeError. Add a type guard.
| /** | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue || typeof pathValue !== 'string') return null | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| } | |
| return null | |
| } |
| function writeQueue (items) { | ||
| const file = getQueuePath() | ||
| const toWrite = normalizeQueueItems(items) | ||
| try { |
There was a problem hiding this comment.
[Re-raised] readQueue writes back a truncated file when it finds an oversized queue, but the mkdirSync call is unnecessary here since the file already exists (readFileSync succeeded). Remove the redundant mkdirSync in the shrink-rewrite path.
| try { | |
| if (parsed.length > normalized.length) { | |
| try { | |
| fs.writeFileSync(file, JSON.stringify(normalized), 'utf8') | |
| } catch { | |
| // same as writeQueue — never throw from telemetry persistence | |
| } | |
| } |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🤖 PR Reviewer
The diff introduces a major refactor: fire-and-forget flush worker via detached subprocess, persistent queue store, agent detection, and updated tests. Most of the code is well-structured and tested. However, there is a syntax error in telemetry-lib.js where a duplicate/orphaned Object.entries filter line was left in, and several previously-raised issues remain unaddressed (broad header denylist, env passthrough to worker, body re-parsing, wire format inconsistency, detectCopilotAgent type guard, redundant mkdirSync in readQueue shrink path).
📝 9 suggestion(s) (1 new, 8 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| init: (versionString, binName, remoteConf = {}) => { | ||
| global.commandHookStartTime = Date.now() | ||
| rootCliVersion = versionString | ||
| if (remoteConf.fetchHeaders) { | ||
| fetchHeaders = remoteConf.fetchHeaders | ||
| } | ||
| if (remoteConf.postUrl) { | ||
| postUrl = remoteConf.postUrl | ||
| } | ||
| postUrl = remoteConf.postUrl || process.env.AIO_TELEMETRY_POST_URL || DEFAULT_TELEMETRY_POST_URL | ||
| const rawExtra = remoteConf.fetchHeaders && typeof remoteConf.fetchHeaders === 'object' | ||
| ? remoteConf.fetchHeaders | ||
| : {} | ||
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | ||
| extraFetchHeaders = Object.fromEntries( | ||
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | ||
| ) | ||
| Object.entries(rawExtra).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| fetchHeaders = { ...DEFAULT_FETCH_HEADERS, ...extraFetchHeaders } | ||
| configKey = binName + '-cli-telemetry' |
There was a problem hiding this comment.
[Re-raised] The flush worker subprocess inherits the full process.env (including potentially sensitive variables like AWS credentials, tokens, etc.) with only AIO_TELEMETRY_DISABLED added. Consider passing a minimal env to the worker instead of spreading all of process.env.
| init: (versionString, binName, remoteConf = {}) => { | |
| global.commandHookStartTime = Date.now() | |
| rootCliVersion = versionString | |
| if (remoteConf.fetchHeaders) { | |
| fetchHeaders = remoteConf.fetchHeaders | |
| } | |
| if (remoteConf.postUrl) { | |
| postUrl = remoteConf.postUrl | |
| } | |
| postUrl = remoteConf.postUrl || process.env.AIO_TELEMETRY_POST_URL || DEFAULT_TELEMETRY_POST_URL | |
| const rawExtra = remoteConf.fetchHeaders && typeof remoteConf.fetchHeaders === 'object' | |
| ? remoteConf.fetchHeaders | |
| : {} | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| extraFetchHeaders = Object.fromEntries( | |
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| Object.entries(rawExtra).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| ) | |
| fetchHeaders = { ...DEFAULT_FETCH_HEADERS, ...extraFetchHeaders } | |
| configKey = binName + '-cli-telemetry' | |
| postUrl = remoteConf.postUrl || process.env.AIO_TELEMETRY_POST_URL || DEFAULT_TELEMETRY_POST_URL | |
| const rawExtra = remoteConf.fetchHeaders && typeof remoteConf.fetchHeaders === 'object' | |
| ? remoteConf.fetchHeaders | |
| : {} | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| extraFetchHeaders = Object.fromEntries( | |
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| fetchHeaders = { ...DEFAULT_FETCH_HEADERS, ...extraFetchHeaders } | |
| configKey = binName + '-cli-telemetry' |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | ||
| } | ||
| }] | ||
| }]) | ||
| } | ||
| const metrics = JSON.parse(fetchConfig.body)[0].metrics | ||
|
|
||
| if (eventType !== 'postrun') { |
There was a problem hiding this comment.
[Re-raised] The flush worker subprocess inherits the full process.env. Consider passing only a minimal environment.
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] | |
| }]) | |
| } | |
| const metrics = JSON.parse(fetchConfig.body)[0].metrics | |
| if (eventType !== 'postrun') { | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), flushPayload], { | |
| env: { | |
| PATH: process.env.PATH, | |
| HOME: process.env.HOME, | |
| XDG_CONFIG_HOME: process.env.XDG_CONFIG_HOME, | |
| NODE_PATH: process.env.NODE_PATH, | |
| AIO_TELEMETRY_DISABLED: '1' | |
| }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) |
| headers: fetchHeaders, | ||
| body: JSON.stringify({ | ||
| id: Math.floor(timestamp * Math.random()), | ||
| timestamp, | ||
| _adobeio: { | ||
| eventType, | ||
| eventData, | ||
| cliVersion: rootCliVersion, | ||
| clientId, | ||
| command: prerunEvent.command, | ||
| commandDuration: timestamp - prerunEvent.start, | ||
| commandFlags: prerunEvent.flags.toString(), | ||
| commandSuccess: eventType !== 'command-error', | ||
| nodeVersion: process.version, | ||
| osNameVersion | ||
| } | ||
| }) | ||
| body: JSON.stringify([{ | ||
| metrics: [{ | ||
| name: 'aio.cli.telemetry', |
There was a problem hiding this comment.
[Re-raised] The line const metrics = JSON.parse(fetchConfig.body)[0].metrics re-parses JSON that was just serialized. Build the metrics array first and reference it directly to avoid the redundant round-trip.
| headers: fetchHeaders, | |
| body: JSON.stringify({ | |
| id: Math.floor(timestamp * Math.random()), | |
| timestamp, | |
| _adobeio: { | |
| eventType, | |
| eventData, | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion | |
| } | |
| }) | |
| body: JSON.stringify([{ | |
| metrics: [{ | |
| name: 'aio.cli.telemetry', | |
| const metricsArray = [{ | |
| name: 'aio.cli.telemetry', | |
| type: 'gauge', | |
| value: 1, | |
| timestamp, | |
| attributes: { | |
| eventType, | |
| eventData: formatEventDataAttribute(eventData), | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] | |
| const fetchConfig = { | |
| method: 'POST', | |
| headers: fetchHeaders, | |
| body: JSON.stringify([{ metrics: metricsArray }]) | |
| } | |
| const metrics = metricsArray |
| if (!url || typeof url !== 'string') { | ||
| return | ||
| } | ||
| postUrl = url | ||
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( |
There was a problem hiding this comment.
[Re-raised] The header filter only blocks 'api-key'. Broaden to cover 'authorization', 'x-api-key', and 'x-ingest-key' for consistency with telemetry-lib.js.
| if (!url || typeof url !== 'string') { | |
| return | |
| } | |
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const safe = Object.fromEntries( | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | ||
| } | ||
| currentMetrics = JSON.parse(body)[0].metrics |
There was a problem hiding this comment.
[Re-raised] If body is valid JSON but does not contain an array at index 0, or metrics is missing, this throws a TypeError silently swallowed by the outer catch. Add an explicit guard.
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } | |
| currentMetrics = JSON.parse(body)[0].metrics | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| currentMetrics = parsedBody[0].metrics |
| try { | ||
| debug('POST %s requestHeaders=%o', postUrl, requestHeaders) | ||
| const res = await fetch(postUrl, { | ||
| method: 'POST', |
There was a problem hiding this comment.
[Re-raised] The flush-worker POSTs with body shape { batches: [{ metrics: allMetrics }] } but telemetry-lib.js serializes the body as [{ metrics: [...] }] (a top-level array). These formats are inconsistent. Both sides should agree on the wire format. Change the worker to re-use the same top-level array format.
| try { | |
| debug('POST %s requestHeaders=%o', postUrl, requestHeaders) | |
| const res = await fetch(postUrl, { | |
| method: 'POST', | |
| const res = await fetch(postUrl, { | |
| method: 'POST', | |
| headers: requestHeaders, | |
| body: JSON.stringify([{ metrics: allMetrics }]) | |
| }) |
| * @returns {string|null} Agent name when detected, otherwise null. | ||
| */ | ||
| function detectCopilotAgent (pathValue) { | ||
| if (!pathValue) return null | ||
| // Extension id appears in globalStorage paths on all platforms; do not tie to '/' (Windows uses '\'). | ||
| if (pathValue.includes('github.copilot-chat')) { |
There was a problem hiding this comment.
[Re-raised] detectCopilotAgent checks pathValue.includes(...) without guarding against a non-string value. Add a typeof guard to avoid TypeError when PATH is set to a non-string value.
| * @returns {string|null} Agent name when detected, otherwise null. | |
| */ | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue) return null | |
| // Extension id appears in globalStorage paths on all platforms; do not tie to '/' (Windows uses '\'). | |
| if (pathValue.includes('github.copilot-chat')) { | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue || typeof pathValue !== 'string') return null | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| } | |
| return null | |
| } |
| /** | ||
| * Persists the given metrics array to the queue file, creating directories as needed. | ||
| * Silently ignores write errors — telemetry must never crash the CLI. | ||
| * @param {Array<object>} items Flat array of New Relic metric objects. | ||
| */ | ||
| function writeQueue (items) { | ||
| const file = getQueuePath() | ||
| const toWrite = normalizeQueueItems(items) |
There was a problem hiding this comment.
[Re-raised] readQueue writes back a truncated file when it finds an oversized queue, but the mkdirSync call is unnecessary here since the file already exists (readFileSync succeeded). Remove the redundant mkdirSync in the shrink-rewrite path.
| /** | |
| * Persists the given metrics array to the queue file, creating directories as needed. | |
| * Silently ignores write errors — telemetry must never crash the CLI. | |
| * @param {Array<object>} items Flat array of New Relic metric objects. | |
| */ | |
| function writeQueue (items) { | |
| const file = getQueuePath() | |
| const toWrite = normalizeQueueItems(items) | |
| if (parsed.length > normalized.length) { | |
| try { | |
| fs.writeFileSync(file, JSON.stringify(normalized), 'utf8') | |
| } catch { | |
| // same as writeQueue — never throw from telemetry persistence | |
| } | |
| } | |
| return normalized |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
🤖 PR Reviewer
The diff is a substantial refactor introducing a flush-worker subprocess, persistent queue store, agent detection, and improved telemetry payload format. Most of the code is well-structured and tested. However, several previously-raised issues remain unaddressed (syntax error in init, flush worker inheriting full env, inconsistent wire format, redundant JSON round-trip, narrow header blocklist in flush-worker, missing guard in flush-worker body parse, missing typeof guard in detectCopilotAgent, redundant mkdirSync in readQueue shrink path), plus the tests assert the old batches wrapper format which will fail against the suggested fix.
📝 10 suggestion(s) (2 new, 8 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | ||
| extraFetchHeaders = Object.fromEntries( | ||
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | ||
| ) | ||
| configKey = binName + '-cli-telemetry' | ||
| }, | ||
| getClientId, |
There was a problem hiding this comment.
[Re-raised] Syntax error: duplicate/orphaned Object.fromEntries assignment — the closing parenthesis of the first call is missing, then the entire expression is repeated. This is a parse error that will prevent the module from loading.
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| extraFetchHeaders = Object.fromEntries( | |
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| configKey = binName + '-cli-telemetry' | |
| }, | |
| getClientId, | |
| extraFetchHeaders = Object.fromEntries( | |
| Object.entries(rawExtra).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| fetchHeaders = { ...DEFAULT_FETCH_HEADERS, ...extraFetchHeaders } | |
| configKey = binName + '-cli-telemetry' |
| headers: fetchHeaders, | ||
| body: JSON.stringify({ | ||
| id: Math.floor(timestamp * Math.random()), | ||
| timestamp, | ||
| _adobeio: { | ||
| eventType, | ||
| eventData, | ||
| cliVersion: rootCliVersion, | ||
| clientId, | ||
| command: prerunEvent.command, | ||
| commandDuration: timestamp - prerunEvent.start, | ||
| commandFlags: prerunEvent.flags.toString(), | ||
| commandSuccess: eventType !== 'command-error', | ||
| nodeVersion: process.version, | ||
| osNameVersion | ||
| } | ||
| }) | ||
| body: JSON.stringify([{ | ||
| metrics: [{ |
There was a problem hiding this comment.
[Re-raised] The line const metrics = JSON.parse(fetchConfig.body)[0].metrics re-parses JSON that was just serialized. Build the metrics array first and reference it directly.
| headers: fetchHeaders, | |
| body: JSON.stringify({ | |
| id: Math.floor(timestamp * Math.random()), | |
| timestamp, | |
| _adobeio: { | |
| eventType, | |
| eventData, | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion | |
| } | |
| }) | |
| body: JSON.stringify([{ | |
| metrics: [{ | |
| const metricsArray = [{ | |
| name: 'aio.cli.telemetry', | |
| type: 'gauge', | |
| value: 1, | |
| timestamp, | |
| attributes: { | |
| eventType, | |
| eventData: formatEventDataAttribute(eventData), | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] | |
| const fetchConfig = { | |
| method: 'POST', | |
| headers: fetchHeaders, | |
| body: JSON.stringify([{ metrics: metricsArray }]) | |
| } | |
| const metrics = metricsArray |
| nodeVersion: process.version, | ||
| osNameVersion, | ||
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | ||
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | ||
| } | ||
| }] | ||
| }]) | ||
| } | ||
| const metrics = JSON.parse(fetchConfig.body)[0].metrics |
There was a problem hiding this comment.
[Re-raised] The flush worker subprocess inherits the full process.env including potentially sensitive variables. Pass only a minimal environment to the worker.
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] | |
| }]) | |
| } | |
| const metrics = JSON.parse(fetchConfig.body)[0].metrics | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), flushPayload], { | |
| env: { | |
| PATH: process.env.PATH, | |
| HOME: process.env.HOME, | |
| XDG_CONFIG_HOME: process.env.XDG_CONFIG_HOME, | |
| NODE_PATH: process.env.NODE_PATH, | |
| AIO_TELEMETRY_DISABLED: '1' | |
| }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) |
| function detectCopilotAgent (pathValue) { | ||
| if (!pathValue) return null | ||
| // Extension id appears in globalStorage paths on all platforms; do not tie to '/' (Windows uses '\'). | ||
| if (pathValue.includes('github.copilot-chat')) { | ||
| return 'github-copilot' |
There was a problem hiding this comment.
[Re-raised] detectCopilotAgent does not guard against non-string values. If PATH is set to a non-string (e.g. null, a number injected via a test), pathValue.includes will throw a TypeError.
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue) return null | |
| // Extension id appears in globalStorage paths on all platforms; do not tie to '/' (Windows uses '\'). | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue || typeof pathValue !== 'string') return null | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| } | |
| return null | |
| } |
| } | ||
| postUrl = url | ||
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( | ||
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') |
There was a problem hiding this comment.
[Re-raised] The header filter only blocks 'api-key'. Broaden to cover 'authorization', 'x-api-key', and 'x-ingest-key' to be consistent with telemetry-lib.js.
| } | |
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
There was a problem hiding this comment.
[Re-raised] If the body parses but does not have an array at index 0, or metrics is absent/non-array, this throws a TypeError silently swallowed by the outer catch. Add an explicit guard.
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| currentMetrics = parsedBody[0].metrics |
| try { | ||
| debug('POST %s requestHeaders=%o', postUrl, requestHeaders) | ||
| const res = await fetch(postUrl, { | ||
| method: 'POST', | ||
| headers: requestHeaders, | ||
| body: JSON.stringify({ batches: [{ metrics: allMetrics }] }) |
There was a problem hiding this comment.
[Re-raised] The flush-worker POSTs with body shape { batches: [{ metrics: allMetrics }] } but telemetry-lib.js serializes the body as [{ metrics: [...] }] (a top-level array). Both sides must agree on the wire format. Change the worker to use the same top-level array format to ensure the proxy receives consistent payloads.
| try { | |
| debug('POST %s requestHeaders=%o', postUrl, requestHeaders) | |
| const res = await fetch(postUrl, { | |
| method: 'POST', | |
| headers: requestHeaders, | |
| body: JSON.stringify({ batches: [{ metrics: allMetrics }] }) | |
| const res = await fetch(postUrl, { | |
| method: 'POST', | |
| headers: requestHeaders, | |
| body: JSON.stringify([{ metrics: allMetrics }]) | |
| }) |
| * Persists the given metrics array to the queue file, creating directories as needed. | ||
| * Silently ignores write errors — telemetry must never crash the CLI. | ||
| * @param {Array<object>} items Flat array of New Relic metric objects. | ||
| */ | ||
| function writeQueue (items) { | ||
| const file = getQueuePath() | ||
| const toWrite = normalizeQueueItems(items) | ||
| try { |
There was a problem hiding this comment.
[Re-raised] The mkdirSync call inside the shrink-rewrite path in readQueue is unnecessary — the file already exists at this point (readFileSync succeeded). Remove it to avoid the redundant directory creation.
| * Persists the given metrics array to the queue file, creating directories as needed. | |
| * Silently ignores write errors — telemetry must never crash the CLI. | |
| * @param {Array<object>} items Flat array of New Relic metric objects. | |
| */ | |
| function writeQueue (items) { | |
| const file = getQueuePath() | |
| const toWrite = normalizeQueueItems(items) | |
| try { | |
| if (parsed.length > normalized.length) { | |
| try { | |
| fs.writeFileSync(file, JSON.stringify(normalized), 'utf8') | |
| } catch { | |
| // same as writeQueue — never throw from telemetry persistence | |
| } | |
| } | |
| return normalized |
| expect(writeQueue).not.toHaveBeenCalled() | ||
| }) | ||
|
|
||
| test('writes merged metrics to queue on fetch failure', async () => { |
There was a problem hiding this comment.
Once the wire format inconsistency in flush-worker.js is fixed (top-level array instead of batches wrapper), these assertions need to be updated to match the corrected format.
| expect(writeQueue).not.toHaveBeenCalled() | |
| }) | |
| test('writes merged metrics to queue on fetch failure', async () => { | |
| const posted = JSON.parse(opts.body) | |
| expect(posted[0].metrics).toHaveLength(2) | |
| expect(posted[0].metrics[0]).toEqual(queued[0]) | |
| expect(posted[0].metrics[1]).toEqual(METRIC) |
| await main() | ||
|
|
||
| expect(writeQueue).toHaveBeenCalledTimes(1) | ||
| expect(writeQueue).toHaveBeenCalledWith([METRIC]) |
There was a problem hiding this comment.
Update assertion to match corrected top-level array wire format after fixing the batches wrapper inconsistency.
| await main() | |
| expect(writeQueue).toHaveBeenCalledTimes(1) | |
| expect(writeQueue).toHaveBeenCalledWith([METRIC]) | |
| const posted = JSON.parse(fetch.mock.calls[0][1].body) | |
| expect(posted[0].metrics).toHaveLength(1) | |
| expect(posted[0].metrics[0]).toEqual(METRIC) | |
| expect(clearQueue).toHaveBeenCalledTimes(1) |
There was a problem hiding this comment.
🤖 PR Reviewer
The diff addresses the architectural refactor from direct HTTP POST to a detached flush-worker subprocess, adds agent detection, and improves documentation. Most previous re-raised issues remain unaddressed: flush-worker.js still uses the { batches: [...] } wrapper instead of the top-level array format that telemetry-lib.js produces, tests still assert the batches shape, the blocked-header set in flush-worker.js is still too narrow, and the detectCopilotAgent guard still doesn't check typeof, and the body is still re-parsed from JSON right after serialization. Several new minor issues are also present.
📝 8 suggestion(s) (2 new, 6 re-raised)
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| return | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
[Re-raised] The flush-worker POSTs with body shape { batches: [...] } but telemetry-lib.js serializes as [{ metrics: [...] }] (a top-level array). The wire format is inconsistent and will cause the proxy to receive malformed payloads. Change to pass the parsed array directly as the body, matching the format telemetry-lib produces.
| return | |
| } | |
| try { | |
| const res = await fetch(postUrl, { | |
| method: 'POST', | |
| headers: requestHeaders, | |
| body: JSON.stringify(batches) | |
| }) |
| const { body, postUrl: url, headers } = parsed | ||
| if (!url || typeof url !== 'string') { | ||
| return | ||
| } | ||
| postUrl = url |
There was a problem hiding this comment.
[Re-raised] The header filter only blocks 'api-key'. Broaden to cover 'authorization', 'x-api-key', and 'x-ingest-key' to be consistent with telemetry-lib.js.
| const { body, postUrl: url, headers } = parsed | |
| if (!url || typeof url !== 'string') { | |
| return | |
| } | |
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | ||
| } | ||
| const parsedBody = JSON.parse(body) | ||
| if (!Array.isArray(parsedBody)) { | ||
| return |
There was a problem hiding this comment.
[Re-raised] If the body parses but does not have an array at index 0, or metrics is absent/non-array, the batches variable will hold an unexpected shape, causing silent failures or incorrect POSTs. Add an explicit guard.
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody)) { | |
| return | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| batches = parsedBody |
| * | ||
| * @param {string|null|undefined} [pathValue] - PATH environment variable value. | ||
| * @returns {string|null} Agent name when detected, otherwise null. | ||
| */ | ||
| function detectCopilotAgent (pathValue) { | ||
| if (!pathValue) return null |
There was a problem hiding this comment.
[Re-raised] detectCopilotAgent does not guard against non-string values. If PATH is set to null or a non-string (e.g. injected via a test), pathValue.includes will throw a TypeError. Add a typeof guard.
| * | |
| * @param {string|null|undefined} [pathValue] - PATH environment variable value. | |
| * @returns {string|null} Agent name when detected, otherwise null. | |
| */ | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue) return null | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue || typeof pathValue !== 'string') return null | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| } | |
| return null | |
| } |
| commandSuccess: eventType !== 'command-error', | ||
| nodeVersion: process.version, | ||
| osNameVersion, | ||
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | ||
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' |
There was a problem hiding this comment.
[Re-raised] The line const batch = JSON.parse(fetchConfig.body) re-parses JSON that was just serialized two lines above. Build the metrics array first and reference it directly, avoiding the redundant serialization/deserialization round-trip.
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| const metricsThisEvent = [{ | |
| name: 'aio.cli.telemetry', | |
| type: 'gauge', | |
| value: 1, | |
| timestamp, | |
| attributes: { | |
| eventType, | |
| eventData: formatEventDataAttribute(eventData), | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] |
| if (optedOut) { | ||
| pendingCommandMetrics.length = 0 | ||
| debug('Telemetry is off. Not logging telemetry event', eventType) | ||
| } else { | ||
| const clientId = getClientId() | ||
| const timestamp = Date.now() | ||
| const invocationContext = getInvocationContext() | ||
| const fetchConfig = { | ||
| method: 'POST', | ||
| headers: fetchHeaders, | ||
| body: JSON.stringify({ | ||
| id: Math.floor(timestamp * Math.random()), | ||
| timestamp, | ||
| _adobeio: { | ||
| eventType, | ||
| eventData, | ||
| cliVersion: rootCliVersion, | ||
| clientId, | ||
| command: prerunEvent.command, | ||
| commandDuration: timestamp - prerunEvent.start, | ||
| commandFlags: prerunEvent.flags.toString(), | ||
| commandSuccess: eventType !== 'command-error', | ||
| nodeVersion: process.version, | ||
| osNameVersion | ||
| } | ||
| }) | ||
| body: JSON.stringify([{ | ||
| metrics: [{ | ||
| name: 'aio.cli.telemetry', | ||
| type: 'gauge', | ||
| value: 1, | ||
| // id: Math.floor(timestamp * Math.random()), | ||
| timestamp, | ||
| attributes: { | ||
| eventType, | ||
| eventData: formatEventDataAttribute(eventData), | ||
| cliVersion: rootCliVersion, | ||
| clientId, | ||
| command: prerunEvent.command, | ||
| commandDuration: timestamp - prerunEvent.start, | ||
| commandFlags: prerunEvent.flags.toString(), |
There was a problem hiding this comment.
The fetchConfig object is only used to serialize the body, then immediately re-parsed. After applying the suggestion to build metricsThisEvent directly, this entire fetchConfig block can be removed.
| if (optedOut) { | |
| pendingCommandMetrics.length = 0 | |
| debug('Telemetry is off. Not logging telemetry event', eventType) | |
| } else { | |
| const clientId = getClientId() | |
| const timestamp = Date.now() | |
| const invocationContext = getInvocationContext() | |
| const fetchConfig = { | |
| method: 'POST', | |
| headers: fetchHeaders, | |
| body: JSON.stringify({ | |
| id: Math.floor(timestamp * Math.random()), | |
| timestamp, | |
| _adobeio: { | |
| eventType, | |
| eventData, | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion | |
| } | |
| }) | |
| body: JSON.stringify([{ | |
| metrics: [{ | |
| name: 'aio.cli.telemetry', | |
| type: 'gauge', | |
| value: 1, | |
| // id: Math.floor(timestamp * Math.random()), | |
| timestamp, | |
| attributes: { | |
| eventType, | |
| eventData: formatEventDataAttribute(eventData), | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| const timestamp = Date.now() | |
| const invocationContext = getInvocationContext() |
| }) | ||
|
|
||
| test('does not throw when fetch rejects', async () => { | ||
| fetch.mockRejectedValue(new Error('network error')) | ||
|
|
There was a problem hiding this comment.
[Re-raised] Once the wire format inconsistency in flush-worker.js is fixed (top-level array instead of batches wrapper), these assertions need to be updated to match the corrected format.
| }) | |
| test('does not throw when fetch rejects', async () => { | |
| fetch.mockRejectedValue(new Error('network error')) | |
| const posted = JSON.parse(opts.body) | |
| expect(Array.isArray(posted)).toBe(true) | |
| expect(posted[0].metrics).toHaveLength(1) | |
| expect(posted[0].metrics[0]).toEqual(METRIC) |
There was a problem hiding this comment.
🤖 PR Reviewer
Most previously raised issues remain unresolved. The wire-format mismatch ({ batches } wrapper vs top-level array) still exists in both flush-worker.js and the test assertions. The header blocklist in flush-worker.js still only blocks 'api-key'. The parsedBody[0].metrics array guard is absent. The redundant JSON serialize/parse round-trip in telemetry-lib.js is still present. The spawn env still inherits all of process.env including secrets. The detectCopilotAgent non-string guard issue is partially addressed (falsy check added) but a non-string truthy value (e.g. a number or object) would still reach .includes and throw.
🔄 8 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| const parsedBody = JSON.parse(body) | ||
| if (!Array.isArray(parsedBody)) { | ||
| return | ||
| } |
There was a problem hiding this comment.
[Re-raised] Wire format mismatch: the body is a top-level array (as produced by telemetry-lib.js) but is re-wrapped in { batches } here. The proxy will receive a different shape than what telemetry-lib serializes.
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody)) { | |
| return | |
| } | |
| const res = await fetch(postUrl, { | |
| method: 'POST', | |
| headers: requestHeaders, | |
| body: JSON.stringify(batches) | |
| }) |
| if (!url || typeof url !== 'string') { | ||
| return | ||
| } | ||
| postUrl = url | ||
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( |
There was a problem hiding this comment.
[Re-raised] Header filter only blocks 'api-key'. Broaden to cover 'authorization', 'x-api-key', and 'x-ingest-key' for consistency with telemetry-lib.js.
| if (!url || typeof url !== 'string') { | |
| return | |
| } | |
| postUrl = url | |
| if (headers && typeof headers === 'object') { | |
| const safe = Object.fromEntries( | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | ||
| } | ||
| const parsedBody = JSON.parse(body) |
There was a problem hiding this comment.
[Re-raised] Missing guard: if parsedBody[0] exists but lacks a metrics array the worker silently sends a malformed payload. Add an explicit check.
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } | |
| const parsedBody = JSON.parse(body) | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| batches = parsedBody |
| debug('trackEvent %s eventData=%j postUrl=%s willSend=%s', eventType, eventData, postUrl, willSend) | ||
|
|
||
| if (isDisabledForCommand || config.get(`${configKey}.optOut`, 'global') === true) { | ||
| if (optedOut) { | ||
| pendingCommandMetrics.length = 0 | ||
| debug('Telemetry is off. Not logging telemetry event', eventType) | ||
| } else { | ||
| const clientId = getClientId() | ||
| const timestamp = Date.now() | ||
| const invocationContext = getInvocationContext() | ||
| const fetchConfig = { | ||
| method: 'POST', | ||
| headers: fetchHeaders, | ||
| body: JSON.stringify({ | ||
| id: Math.floor(timestamp * Math.random()), | ||
| timestamp, | ||
| _adobeio: { | ||
| eventType, | ||
| eventData, | ||
| cliVersion: rootCliVersion, | ||
| clientId, | ||
| command: prerunEvent.command, | ||
| commandDuration: timestamp - prerunEvent.start, | ||
| commandFlags: prerunEvent.flags.toString(), | ||
| commandSuccess: eventType !== 'command-error', | ||
| nodeVersion: process.version, | ||
| osNameVersion | ||
| } | ||
| }) | ||
| body: JSON.stringify([{ | ||
| metrics: [{ | ||
| name: 'aio.cli.telemetry', | ||
| type: 'gauge', | ||
| value: 1, | ||
| // id: Math.floor(timestamp * Math.random()), |
There was a problem hiding this comment.
[Re-raised] The fetchConfig body is serialized to JSON and then immediately re-parsed two lines later. Build the metrics array directly to avoid the redundant round-trip.
| debug('trackEvent %s eventData=%j postUrl=%s willSend=%s', eventType, eventData, postUrl, willSend) | |
| if (isDisabledForCommand || config.get(`${configKey}.optOut`, 'global') === true) { | |
| if (optedOut) { | |
| pendingCommandMetrics.length = 0 | |
| debug('Telemetry is off. Not logging telemetry event', eventType) | |
| } else { | |
| const clientId = getClientId() | |
| const timestamp = Date.now() | |
| const invocationContext = getInvocationContext() | |
| const fetchConfig = { | |
| method: 'POST', | |
| headers: fetchHeaders, | |
| body: JSON.stringify({ | |
| id: Math.floor(timestamp * Math.random()), | |
| timestamp, | |
| _adobeio: { | |
| eventType, | |
| eventData, | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion | |
| } | |
| }) | |
| body: JSON.stringify([{ | |
| metrics: [{ | |
| name: 'aio.cli.telemetry', | |
| type: 'gauge', | |
| value: 1, | |
| // id: Math.floor(timestamp * Math.random()), | |
| const timestamp = Date.now() | |
| const invocationContext = getInvocationContext() | |
| const metricsThisEvent = [{ | |
| name: 'aio.cli.telemetry', | |
| type: 'gauge', | |
| value: 1, | |
| timestamp, | |
| attributes: { | |
| eventType, | |
| eventData: formatEventDataAttribute(eventData), | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| commandSuccess: eventType !== 'command-error', | |
| nodeVersion: process.version, | |
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| }] |
| timestamp, | ||
| attributes: { |
There was a problem hiding this comment.
[Re-raised] These two lines exist only because of the redundant serialize/parse. Remove them after applying the suggestion above.
|
|
||
| process.argv = ['node', 'flush-worker.js', flushArg()] | ||
| await expect(main()).resolves.toBeUndefined() | ||
| expect(fetch).toHaveBeenCalledTimes(1) | ||
| }) |
There was a problem hiding this comment.
[Re-raised] Test assertions still expect the { batches } wrapper shape. Once the wire format is fixed to use a top-level array, update these assertions accordingly.
| process.argv = ['node', 'flush-worker.js', flushArg()] | |
| await expect(main()).resolves.toBeUndefined() | |
| expect(fetch).toHaveBeenCalledTimes(1) | |
| }) | |
| const posted = JSON.parse(opts.body) | |
| expect(Array.isArray(posted)).toBe(true) | |
| expect(posted[0].metrics).toHaveLength(1) | |
| expect(posted[0].metrics[0]).toEqual(METRIC) |
…but the parent never calls fetch
There was a problem hiding this comment.
🤖 PR Reviewer
The re-raised issues from the previous review remain unresolved: the flush worker still wraps the payload in { batches } instead of posting the top-level array, the header filter still only blocks 'api-key', the malformed-payload guard is missing the metrics-array check, detectCopilotAgent lacks a typeof string guard, and the spawned worker still inherits all of process.env. The test assertions also still expect the { batches } wrapper shape. All other new code (agent detection, batching, env-var disable, formatEventDataAttribute, resolveEventData) is well-structured and thoroughly tested.
🔄 6 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| return | ||
| } | ||
| batches = parsedBody | ||
| } catch { |
There was a problem hiding this comment.
[Re-raised] Wire format mismatch: body is a top-level array but is re-wrapped in { batches }. Post the parsed array directly.
| } catch { | |
| body: JSON.stringify(batches) |
| try { | ||
| const parsed = JSON.parse(process.argv[2]) | ||
| const { body, postUrl: url, headers } = parsed | ||
| if (!url || typeof url !== 'string') { |
There was a problem hiding this comment.
[Re-raised] Header filter only blocks 'api-key'. Broaden to cover authorization, x-api-key, and x-ingest-key.
| try { | |
| const parsed = JSON.parse(process.argv[2]) | |
| const { body, postUrl: url, headers } = parsed | |
| if (!url || typeof url !== 'string') { | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) | ||
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } |
There was a problem hiding this comment.
[Re-raised] Missing guard: if parsedBody[0] exists but lacks a metrics array the worker silently sends a malformed payload. Add an explicit check.
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| batches = parsedBody |
| return 'github-copilot' | ||
| } |
There was a problem hiding this comment.
[Re-raised] detectCopilotAgent does not guard against non-string truthy values. A non-string (e.g. number, object) would reach .includes and throw TypeError.
| return 'github-copilot' | |
| } | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue || typeof pathValue !== 'string') return null | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| } | |
| return null | |
| } |
| osNameVersion, | ||
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | ||
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | ||
| } | ||
| } | ||
|
|
||
| if (eventType !== 'postrun') { | ||
| pendingCommandMetrics.push(metric) | ||
| return |
There was a problem hiding this comment.
[Re-raised] The spawned worker inherits all of process.env including secrets and tokens. Pass only the minimal env vars needed.
| osNameVersion, | |
| invocation_context: /* istanbul ignore next */ invocationContext.isAgent ? 'agent' : 'human', | |
| agent_name: /* istanbul ignore next */ invocationContext.agentName || 'unknown' | |
| } | |
| } | |
| if (eventType !== 'postrun') { | |
| pendingCommandMetrics.push(metric) | |
| return | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), flushPayload], { | |
| env: { | |
| PATH: process.env.PATH, | |
| HOME: process.env.HOME, | |
| XDG_CONFIG_HOME: process.env.XDG_CONFIG_HOME, | |
| NODE_PATH: process.env.NODE_PATH, | |
| AIO_TELEMETRY_DISABLED: '1' | |
| }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) |
| process.argv = ['node', 'flush-worker.js', flushArg()] | ||
| await expect(main()).resolves.toBeUndefined() | ||
| expect(fetch).toHaveBeenCalledTimes(1) | ||
| }) | ||
|
|
There was a problem hiding this comment.
[Re-raised] Test assertions expect { batches } wrapper. Once wire format is fixed to top-level array, update accordingly.
| process.argv = ['node', 'flush-worker.js', flushArg()] | |
| await expect(main()).resolves.toBeUndefined() | |
| expect(fetch).toHaveBeenCalledTimes(1) | |
| }) | |
| const posted = JSON.parse(opts.body) | |
| expect(Array.isArray(posted)).toBe(true) | |
| expect(posted[0].metrics).toHaveLength(1) | |
| expect(posted[0].metrics[0]).toEqual(METRIC) |
…A-4597) Repoint DEFAULT_TELEMETRY_POST_URL from the stage namespace (53444-aioclitelemetryproxy-stage) to production (53444-aioclitelemetryproxy), now that the prod proxy is deployed and verified. Override via aioTelemetry.postUrl or AIO_TELEMETRY_POST_URL is unchanged.
Drop the explicit 'To opt out, run ...' line from the telemetry notice (per @purplecabbage on PR #40) so it does not over-nudge opt-out; the privacy link already covers how to opt out. Removes the now-unused binName argument from getNoticeMessage/notice.
feat: make telemetry opt-out with a one-time notice
There was a problem hiding this comment.
🤖 PR Reviewer
The refactor is substantial and well-structured: removing the Splunk/inquirer dependencies, introducing a detached flush worker, buffering non-postrun metrics, and adding agent detection. Tests are comprehensive. Three previously-raised issues remain unresolved: the wire format wraps the parsed array in { batches } instead of posting it directly, the header blocklist only filters api-key, and the flush-worker guard for a missing metrics array is absent. The detectCopilotAgent function also still lacks a typeof string guard, and the spawned worker inherits all of process.env.
🔄 6 re-raised suggestion(s) from previous review
💡 How to re-trigger
Comment /review or /pr-reviewer on this PR
| return | ||
| } | ||
| batches = parsedBody | ||
| } catch { |
There was a problem hiding this comment.
[Re-raised] Wire format mismatch: the parsed array is re-wrapped in { batches }. The proxy expects a bare array (same shape as mergedBody in telemetry-lib). Post it directly.
| } catch { | |
| body: JSON.stringify(batches) |
| try { | ||
| const parsed = JSON.parse(process.argv[2]) | ||
| const { body, postUrl: url, headers } = parsed | ||
| if (!url || typeof url !== 'string') { | ||
| return |
There was a problem hiding this comment.
[Re-raised] Header filter only blocks 'api-key'. Broaden the blocklist to cover common secret header names.
| try { | |
| const parsed = JSON.parse(process.argv[2]) | |
| const { body, postUrl: url, headers } = parsed | |
| if (!url || typeof url !== 'string') { | |
| return | |
| if (headers && typeof headers === 'object') { | |
| const BLOCKED_HEADERS = new Set(['api-key', 'authorization', 'x-api-key', 'x-ingest-key']) | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => !BLOCKED_HEADERS.has(key.toLowerCase())) | |
| ) | |
| requestHeaders = { ...DEFAULT_HEADERS, ...safe } | |
| } |
| if (headers && typeof headers === 'object') { | ||
| const safe = Object.fromEntries( | ||
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | ||
| ) |
There was a problem hiding this comment.
[Re-raised] Missing guard: if the parsed array exists but its first element lacks a metrics array the worker silently sends a malformed payload.
| if (headers && typeof headers === 'object') { | |
| const safe = Object.fromEntries( | |
| Object.entries(headers).filter(([key]) => key.toLowerCase() !== 'api-key') | |
| ) | |
| const parsedBody = JSON.parse(body) | |
| if (!Array.isArray(parsedBody) || !parsedBody[0] || !Array.isArray(parsedBody[0].metrics)) { | |
| return | |
| } | |
| batches = parsedBody |
| return null | ||
| } | ||
|
|
||
| // TODO: detect VSCODE run as an agent |
There was a problem hiding this comment.
[Re-raised] detectCopilotAgent does not guard against non-string truthy values. A non-string value (e.g. a number injected via a test env object) would reach .includes and throw TypeError.
| return null | |
| } | |
| // TODO: detect VSCODE run as an agent | |
| function detectCopilotAgent (pathValue) { | |
| if (!pathValue || typeof pathValue !== 'string') return null | |
| if (pathValue.includes('github.copilot-chat')) { | |
| return 'github-copilot' | |
| } | |
| return null | |
| } |
| eventType, | ||
| eventData: formatEventDataAttribute(eventData), | ||
| cliVersion: rootCliVersion, | ||
| clientId, | ||
| command: prerunEvent.command, | ||
| commandDuration: timestamp - prerunEvent.start, | ||
| commandFlags: prerunEvent.flags.toString(), |
There was a problem hiding this comment.
[Re-raised] The spawned worker inherits all of process.env, including secrets and tokens present in the CLI environment. Pass only the minimal set of vars the worker actually needs.
| eventType, | |
| eventData: formatEventDataAttribute(eventData), | |
| cliVersion: rootCliVersion, | |
| clientId, | |
| command: prerunEvent.command, | |
| commandDuration: timestamp - prerunEvent.start, | |
| commandFlags: prerunEvent.flags.toString(), | |
| const child = spawn(process.execPath, [path.join(__dirname, 'flush-worker.js'), flushPayload], { | |
| env: { | |
| PATH: process.env.PATH, | |
| HOME: process.env.HOME, | |
| XDG_CONFIG_HOME: process.env.XDG_CONFIG_HOME, | |
| NODE_PATH: process.env.NODE_PATH, | |
| AIO_TELEMETRY_DISABLED: '1' | |
| }, | |
| detached: true, | |
| stdio: 'ignore' | |
| }) |
| expect(posted.batches[0].metrics[0]).toEqual(METRIC) | ||
| }) | ||
|
|
||
| test('does not throw when fetch rejects', async () => { |
There was a problem hiding this comment.
[Re-raised] Test assertions expect the { batches } wrapper. Once the wire format is fixed to post the bare array, update these assertions accordingly.
| expect(posted.batches[0].metrics[0]).toEqual(METRIC) | |
| }) | |
| test('does not throw when fetch rejects', async () => { | |
| const posted = JSON.parse(opts.body) | |
| expect(Array.isArray(posted)).toBe(true) | |
| expect(posted[0].metrics).toHaveLength(1) | |
| expect(posted[0].metrics[0]).toEqual(METRIC) |
| if (eventType !== 'postrun') { | ||
| pendingCommandMetrics.push(metric) |
There was a problem hiding this comment.
Flagged by claude:
in oclif, when a command throws, the command_error hook fires and the process exits — the postrun hook does not run. Same for command_not_found. So under the new design, error/not-found telemetry is buffered and then silently dropped on exit. That's a functional regression: the previous code POSTed each event immediately, so failures were actually captured. With this change, error telemetry (arguably the most valuable signal) effectively disappears.
The tests mask this because they manually call trackEvent('postrun') right after the error hook to force a flush — e.g. test/hooks.test.js "command-error" — which doesn't reflect the real oclif lifecycle where no postrun follows an error.
Suggestion: flush terminal events (command-error, command-not-found) immediately rather than buffering them, or have the command_error hook trigger a flush.
There was a problem hiding this comment.
@pru55e11 I'm working on a fix in a new PR. I will post the link here shortly
Description
This PR updates the telemetry plugin to (1) send metrics via a proxy using a detached “fire-and-forget” flush worker and a persistent on-disk retry queue, and (2) add AI agent vs human invocation detection and include that context in every event.
Changes:
Replace direct HTTP posting from the main process with a detached subprocess flush worker (src/flush-worker.js) and add a persistent retry queue (src/queue-store.js).
Add agent detection via environment variables (plus Copilot PATH markers) and attach invocation_context / agent_name attributes to emitted telemetry.
Update tests and README to reflect the new flush architecture and additional telemetry attributes; remove unused runtime deps.
Related Issue
ACNA-4403ACNA-4403
Motivation and Context
We want to know who, or what is using the cli and how it is being used.
How Has This Been Tested?
Screenshots (if appropriate):
Types of changes
Checklist: