From 5d07c30587bac8965bcdce21ae2d8bb1cf373810 Mon Sep 17 00:00:00 2001 From: Abdel Fane Date: Thu, 16 Jul 2026 16:52:44 -0600 Subject: [PATCH 1/8] fix(telemetry): fail loudly when collection stops instead of exiting 0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The collector had three separate silent `exit 0` paths — REGISTRY_URL unset, feed fetch failure, feed validation failure. Each was individually defensible (a transient feed must not fail the daily workflow, and must never write zeros over a good snapshot), but together they made an outage indistinguishable from a healthy no-op. Registry #283 moved the ingest route on 2026-06-26 and live CLI telemetry was dropped until the #299 alias on 2026-07-03 — seven days, no signal, clean logs throughout. It was also the only collector behaving this way: npm, docker, huggingface and pypi all exit 1 on missing config. The rule now, in classifyCollectionSkip (pure, no db/network/clock, so it is actually testable): unset + never collected -> warning, exit 0 (legitimately unprovisioned) unset + previously collected -> error, exit 1 (config removed; not a blip) set + never collected -> error, exit 1 (broken feed or wrong URL) set + failed <= 2 days -> warning, exit 0 (transient) set + failed > 2 days -> error, exit 1 (outage; dashboard is stale) A single bad run stays tolerated. A sustained one fails. Failures also emit GitHub Actions annotations so they surface in the run summary rather than in step output nobody opens. The workflow step is now continue-on-error. The step runs before generate-md, generate-summary and the commit step, so a hard exit 1 there would silently cost a full day of npm/pypi/docker/hf/chrome data for every other collector — the fix must make failure loud without making it destructive. No behaviour change in today's real state: REGISTRY_URL is unset and no snapshot has ever been stored, so this warns and exits 0 exactly as before, but now says so. Verified end to end: unconfigured -> exit 0 + ::warning::; configured with an unreachable feed -> exit 1 + ::error::. 39 tests pass, including a regression test for the #283 shape (a 7-day gap must exit 1). Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A --- .github/workflows/collect-stats.yml | 11 ++- lib/telemetry.js | 117 +++++++++++++++++++++++++++- scripts/collect-telemetry-stats.js | 89 +++++++++++++++++---- test/telemetry.test.js | 114 ++++++++++++++++++++++++++- 4 files changed, 313 insertions(+), 18 deletions(-) diff --git a/.github/workflows/collect-stats.yml b/.github/workflows/collect-stats.yml index 6d2fc6d9..4fe71511 100644 --- a/.github/workflows/collect-stats.yml +++ b/.github/workflows/collect-stats.yml @@ -80,11 +80,20 @@ jobs: run: npm run collect-chrome - name: Collect first-party CLI telemetry + # A sustained collection failure exits 1 so it is visible here and in the + # run summary as an annotation. continue-on-error keeps that from + # dropping the other collectors' data: this step runs before + # generate-md/generate-summary and the commit step below, so a hard + # failure would silently cost a full day of npm/pypi/docker/hf/chrome + # numbers. A transient blip warns and exits 0 — see + # classifyCollectionSkip in lib/telemetry.js. + continue-on-error: true env: # Registry base URL exposing the coarse public adoption feed # (GET /api/v1/telemetry/v1/adoption/public). Only coarse, anonymous # active-user aggregates are collected here; fine-grained detail is - # never persisted. Unset -> the collector skips gracefully. + # never persisted. Unset -> warns, and errors only if telemetry was + # previously being collected (i.e. the variable was removed). REGISTRY_URL: ${{ vars.REGISTRY_URL }} run: npm run collect-telemetry diff --git a/lib/telemetry.js b/lib/telemetry.js index aaac3203..c24e42ee 100644 --- a/lib/telemetry.js +++ b/lib/telemetry.js @@ -114,4 +114,119 @@ function normalizeAdoptionFeed(feed) { }; } -module.exports = { normalizeAdoptionFeed, toolDisplayName, toCount, TOOL_PRODUCT_NAMES }; +// Consecutive days a collection may fail before it stops being "transient" and +// becomes an outage worth failing on. Two days tolerates a single bad run plus a +// retry; anything longer is a real gap in the data. +const DEFAULT_STALE_AFTER_DAYS = 2; + +function daysBetween(fromISODate, toISODate) { + const from = Date.parse(`${fromISODate}T00:00:00Z`); + const to = Date.parse(`${toISODate}T00:00:00Z`); + if (!Number.isFinite(from) || !Number.isFinite(to)) return null; + return Math.round((to - from) / 86400000); +} + +/** + * Decide how loudly a skipped/failed telemetry collection should complain. + * + * Why this exists: the collector had three separate silent `exit 0` paths — an + * unset REGISTRY_URL, a failed fetch, and a failed validation. Each was + * individually defensible (don't fail the daily workflow; don't write zeros over + * a good snapshot), but together they made an outage look exactly like a healthy + * no-op. Registry #283 dropped live CLI telemetry for ~7 days (2026-06-26 → + * 2026-07-03) and nothing in the logs said so. Silence is not success. + * + * The rule: a single failed run is transient and warns. A sustained one is an + * outage and fails. Losing the variable after it once worked is a regression and + * fails immediately — that is a config being removed, not a flaky endpoint. + * + * Pure: no db, no network, no clock. The caller supplies `today` and the last + * known-good snapshot date so this is testable and deterministic. + * + * @param {object} o + * @param {string} o.reason human-readable cause, echoed into the message + * @param {boolean} o.registryUrlSet whether REGISTRY_URL is configured + * @param {string|null} o.lastSuccessDate YYYY-MM-DD of the newest stored + * snapshot, or null if the collector has never persisted one + * @param {string} o.today YYYY-MM-DD + * @param {number} [o.staleAfterDays] + * @returns {{level:'warning'|'error', exitCode:0|1, message:string}} + */ +function classifyCollectionSkip({ + reason, + registryUrlSet, + lastSuccessDate, + today, + staleAfterDays = DEFAULT_STALE_AFTER_DAYS, +}) { + if (!registryUrlSet) { + // Never configured: legitimately optional, and erroring every day before the + // endpoint is provisioned would train everyone to ignore the annotation. + if (!lastSuccessDate) { + return { + level: 'warning', + exitCode: 0, + message: + 'Telemetry collection is not configured: REGISTRY_URL is unset and no snapshot has ever been stored. ' + + 'Set the REGISTRY_URL repo variable to enable it.', + }; + } + // It worked before and the variable is now gone. That is someone removing + // config, not a transient blip — say so on the first run. + return { + level: 'error', + exitCode: 1, + message: + `REGISTRY_URL is unset but telemetry was collected as recently as ${lastSuccessDate}. ` + + 'The variable was removed or lost; collection has stopped.', + }; + } + + if (!lastSuccessDate) { + // Configured deliberately, yet nothing has ever landed. Broken setup. + return { + level: 'error', + exitCode: 1, + message: + `Telemetry collection failed and has NEVER succeeded: ${reason}. ` + + 'REGISTRY_URL is set, so this is a broken feed or a wrong URL, not an unprovisioned one.', + }; + } + + const age = daysBetween(lastSuccessDate, today); + if (age === null) { + return { + level: 'error', + exitCode: 1, + message: `Telemetry collection failed: ${reason}. Could not parse last-success date ${lastSuccessDate}.`, + }; + } + + if (age > staleAfterDays) { + return { + level: 'error', + exitCode: 1, + message: + `Telemetry collection has failed for ${age} days (last good snapshot ${lastSuccessDate}): ${reason}. ` + + 'This is an outage, not a blip — the dashboard is serving stale active-user numbers.', + }; + } + + return { + level: 'warning', + exitCode: 0, + message: + `Telemetry collection skipped: ${reason}. Last good snapshot ${lastSuccessDate} (${age}d ago); ` + + `tolerating up to ${staleAfterDays}d before this becomes an error.`, + }; +} + +module.exports = { + normalizeAdoptionFeed, + toolDisplayName, + toCount, + classifyCollectionSkip, + daysBetween, + DEFAULT_STALE_AFTER_DAYS, + TOOL_PRODUCT_NAMES, +}; diff --git a/scripts/collect-telemetry-stats.js b/scripts/collect-telemetry-stats.js index 3456e9b9..da9594c4 100644 --- a/scripts/collect-telemetry-stats.js +++ b/scripts/collect-telemetry-stats.js @@ -7,7 +7,7 @@ const fs = require('fs'); require('dotenv').config(); -const { normalizeAdoptionFeed } = require('../lib/telemetry'); +const { normalizeAdoptionFeed, classifyCollectionSkip, DEFAULT_STALE_AFTER_DAYS } = require('../lib/telemetry'); /* * First-party CLI telemetry collector. @@ -27,22 +27,82 @@ const { normalizeAdoptionFeed } = require('../lib/telemetry'); * dashboard and is never persisted here. * * Config (env): - * REGISTRY_URL=https://api.oa2a.org # Registry base URL (required) + * REGISTRY_URL=https://api.oa2a.org # Registry base URL (required) + * TELEMETRY_STALE_AFTER_DAYS=2 # optional; consecutive failure days + * # tolerated before this exits 1 * - * If REGISTRY_URL is unset the collector skips gracefully (exit 0), so the - * daily workflow doesn't fail before the Registry endpoint is provisioned. + * Failure policy: a single failed run warns and exits 0 (a transient feed blip + * must not fail the daily workflow, and must never write zeros over a good + * snapshot). A SUSTAINED failure exits 1 — see classifyCollectionSkip in + * lib/telemetry.js for the full rule and the incident that motivated it. + * + * The workflow marks this step continue-on-error, so a non-zero exit surfaces + * the failure without dropping the other collectors' data for the day (this + * step runs before generate-md and the commit step). */ const REGISTRY_URL = (process.env.REGISTRY_URL || '').trim().replace(/\/+$/, ''); - -if (!REGISTRY_URL) { - console.log('REGISTRY_URL not set — skipping first-party telemetry collection.'); - process.exit(0); -} +const STALE_AFTER_DAYS = Number.isFinite(Number(process.env.TELEMETRY_STALE_AFTER_DAYS)) + ? Number(process.env.TELEMETRY_STALE_AFTER_DAYS) + : DEFAULT_STALE_AFTER_DAYS; const FEED_PATH = '/api/v1/telemetry/v1/adoption/public'; const dbPath = path.join(__dirname, '..', 'data', 'analytics.db'); const today = new Date().toISOString().split('T')[0]; +/** + * Emit a GitHub Actions annotation so a failure is visible in the run summary + * and the PR/commit UI, not just buried in step output nobody opens. No-op + * locally. + */ +function annotate(level, message) { + if (process.env.GITHUB_ACTIONS) { + // Annotations are single-line; collapse any newlines. + console.log(`::${level}::${String(message).replace(/\r?\n/g, ' ')}`); + } +} + +/** + * Newest stored snapshot date, or null if the collector has never persisted one + * (including when the telemetry tables don't exist yet, or the db is missing). + * Read-only and never throws — this runs on the failure path, where a second + * error would just mask the first. + */ +function lastSuccessDate() { + let db; + try { + if (!fs.existsSync(dbPath)) return null; + db = new Database(dbPath, { readonly: true }); + const row = db.prepare('SELECT MAX(date) AS d FROM telemetry_snapshots').get(); + return row && row.d ? row.d : null; + } catch { + return null; + } finally { + try { if (db) db.close(); } catch { /* already closed */ } + } +} + +/** + * Report a skipped/failed collection at the right volume and exit. + * Never writes data — a bad run must not overwrite a good snapshot with zeros. + */ +function reportSkipAndExit(reason) { + const { level, exitCode, message } = classifyCollectionSkip({ + reason, + registryUrlSet: Boolean(REGISTRY_URL), + lastSuccessDate: lastSuccessDate(), + today, + staleAfterDays: STALE_AFTER_DAYS, + }); + annotate(level, message); + if (level === 'error') console.error(' %s', message); + else console.warn(' %s', message); + process.exit(exitCode); +} + +if (!REGISTRY_URL) { + reportSkipAndExit('REGISTRY_URL is not set'); +} + function httpGetJson(rawUrl) { return new Promise((resolve, reject) => { let url; @@ -166,18 +226,17 @@ async function main() { try { raw = await httpGetJson(REGISTRY_URL + FEED_PATH); } catch (err) { - // A transient/unavailable feed must not fail the whole daily workflow, and - // must not write zeros over yesterday's real snapshot. Skip this run. - console.error(' Feed fetch failed: %s — skipping (no data written).', err.message); - process.exit(0); + // A transient feed must not fail the workflow or write zeros over a good + // snapshot — but a sustained one is an outage, so let the classifier decide + // rather than swallowing every failure the same way. + reportSkipAndExit(`feed fetch failed: ${err.message}`); } let feed; try { feed = normalizeAdoptionFeed(raw); } catch (err) { - console.error(' Feed failed validation: %s — skipping (no data written).', err.message); - process.exit(0); + reportSkipAndExit(`feed failed validation: ${err.message}`); } const db = new Database(dbPath); diff --git a/test/telemetry.test.js b/test/telemetry.test.js index 46ecb0ba..b710a3f3 100644 --- a/test/telemetry.test.js +++ b/test/telemetry.test.js @@ -1,6 +1,12 @@ const { test } = require('node:test'); const assert = require('node:assert'); -const { normalizeAdoptionFeed, toolDisplayName, toCount } = require('../lib/telemetry'); +const { + normalizeAdoptionFeed, + toolDisplayName, + toCount, + classifyCollectionSkip, + daysBetween, +} = require('../lib/telemetry'); const validFeed = () => ({ generatedAt: '2026-07-02T12:00:00Z', @@ -101,3 +107,109 @@ test('normalizeAdoptionFeed tolerates missing optional arrays', () => { assert.deepEqual(n.byCountry, []); assert.equal(n.generatedAt, ''); }); + +// --- classifyCollectionSkip ------------------------------------------------ +// +// Regression guard for the incident this logic exists to prevent: the collector +// had three separate silent `exit 0` paths, so registry #283 dropped live CLI +// telemetry for ~7 days (2026-06-26 -> 2026-07-03) with nothing in the logs +// saying so. Silence is not success. + +const skip = (o) => classifyCollectionSkip({ reason: 'feed fetch failed: HTTP 404', today: '2026-07-16', ...o }); + +test('unconfigured and never collected: warns, does not fail the workflow', () => { + const r = skip({ registryUrlSet: false, lastSuccessDate: null }); + assert.equal(r.level, 'warning'); + assert.equal(r.exitCode, 0); + assert.match(r.message, /not configured/i); + assert.match(r.message, /REGISTRY_URL/); +}); + +test('variable removed after it once worked: errors immediately', () => { + // Config being deleted is not a transient blip — no staleness grace. + const r = skip({ registryUrlSet: false, lastSuccessDate: '2026-07-15' }); + assert.equal(r.level, 'error'); + assert.equal(r.exitCode, 1); + assert.match(r.message, /2026-07-15/); +}); + +test('configured but never succeeded: errors (broken feed, not unprovisioned)', () => { + const r = skip({ registryUrlSet: true, lastSuccessDate: null }); + assert.equal(r.level, 'error'); + assert.equal(r.exitCode, 1); + assert.match(r.message, /NEVER succeeded/i); +}); + +test('a single failed run is transient: warns, keeps yesterday data', () => { + const r = skip({ registryUrlSet: true, lastSuccessDate: '2026-07-15' }); + assert.equal(r.level, 'warning'); + assert.equal(r.exitCode, 0); + assert.match(r.message, /1d ago/); +}); + +test('at the staleness threshold: still tolerated', () => { + const r = skip({ registryUrlSet: true, lastSuccessDate: '2026-07-14', staleAfterDays: 2 }); + assert.equal(r.level, 'warning'); + assert.equal(r.exitCode, 0); +}); + +test('past the staleness threshold: errors as an outage', () => { + const r = skip({ registryUrlSet: true, lastSuccessDate: '2026-07-13', staleAfterDays: 2 }); + assert.equal(r.level, 'error'); + assert.equal(r.exitCode, 1); + assert.match(r.message, /3 days/); + assert.match(r.message, /outage/i); +}); + +test('the #283 shape: a 7-day gap fails loudly instead of exiting 0', () => { + // 2026-06-26 -> 2026-07-03 went unnoticed. It must not be possible again. + const r = classifyCollectionSkip({ + reason: 'feed fetch failed: HTTP 404', + registryUrlSet: true, + lastSuccessDate: '2026-06-26', + today: '2026-07-03', + }); + assert.equal(r.level, 'error'); + assert.equal(r.exitCode, 1); + assert.match(r.message, /7 days/); +}); + +test('the reason is always carried into the message', () => { + const r = classifyCollectionSkip({ + reason: 'feed failed validation: totalInstalls is not a number', + registryUrlSet: true, + lastSuccessDate: '2026-07-01', + today: '2026-07-16', + }); + assert.match(r.message, /totalInstalls is not a number/); +}); + +test('an unparseable last-success date errors rather than passing silently', () => { + const r = skip({ registryUrlSet: true, lastSuccessDate: 'not-a-date' }); + assert.equal(r.level, 'error'); + assert.equal(r.exitCode, 1); +}); + +test('every classification returns a usable exit code and a non-empty message', () => { + const cases = [ + { registryUrlSet: false, lastSuccessDate: null }, + { registryUrlSet: false, lastSuccessDate: '2026-07-15' }, + { registryUrlSet: true, lastSuccessDate: null }, + { registryUrlSet: true, lastSuccessDate: '2026-07-15' }, + { registryUrlSet: true, lastSuccessDate: '2026-06-01' }, + ]; + for (const c of cases) { + const r = skip(c); + assert.ok(r.exitCode === 0 || r.exitCode === 1, JSON.stringify(c)); + assert.ok(r.level === 'warning' || r.level === 'error', JSON.stringify(c)); + assert.ok(r.message.length > 0, JSON.stringify(c)); + // An error must never exit 0: that is the bug this whole change is about. + if (r.level === 'error') assert.equal(r.exitCode, 1, JSON.stringify(c)); + } +}); + +test('daysBetween handles ordinary and malformed input', () => { + assert.equal(daysBetween('2026-07-01', '2026-07-16'), 15); + assert.equal(daysBetween('2026-07-16', '2026-07-16'), 0); + assert.equal(daysBetween('garbage', '2026-07-16'), null); +}); From 9c6189f5affe49d0ebd1a2257671ff15ea5973db Mon Sep 17 00:00:00 2001 From: Abdel Fane Date: Thu, 16 Jul 2026 16:55:07 -0600 Subject: [PATCH 2/8] docs: document the telemetry collector's failure policy README:104 claimed the collector 'skips gracefully when unset', which is no longer the whole truth: it warns when unprovisioned, but errors when the variable is removed after telemetry was previously collected, and when a failure becomes sustained. Adds the escalation table, the TELEMETRY_STALE_AFTER_DAYS knob, and states the two properties the original silence existed to protect and which still hold: a failed run never writes (zeros must not overwrite a good snapshot), and a telemetry outage never costs the other collectors their data for the day. Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A --- README.md | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 80bb3962..b2796310 100644 --- a/README.md +++ b/README.md @@ -84,9 +84,10 @@ HF_AUTHOR=opena2a # auto-discovers a HF_MODELS=org/model,org/other # optional extra models HF_TOKEN=hf_... # optional, raises HF rate limits / private repos GOOGLE_APPLICATION_CREDENTIALS=/path/to/gcp-key.json # optional for BigQuery country stats -REGISTRY_URL=https://api.oa2a.org # optional; enables first-party CLI telemetry +REGISTRY_URL=https://api.oa2a.org # optional; enables first-party CLI telemetry REGISTRY_TELEMETRY_TOKEN=... # optional; SA token for the gated detail view (server-side only) ANALYTICS_TRACKER_PASSWORD=... # optional; gates the fine-grained telemetry view on THIS dashboard +TELEMETRY_STALE_AFTER_DAYS=2 # optional; consecutive failure days tolerated before the collector exits 1 ``` The collector persists only the coarse, anonymous adoption feed. The fine-grained @@ -101,7 +102,26 @@ The included workflow (`.github/workflows/collect-stats.yml`) runs daily at `6:0 1. Settings → Secrets and variables → Actions → New secret. 2. Add `GH_STATS_TOKEN` (a Personal Access Token with `repo` or `public_repo` scope). 3. Optionally set `GOOGLE_APPLICATION_CREDENTIALS_JSON` for BigQuery country stats. -4. Optionally set the `REGISTRY_URL` Actions **variable** to enable first-party CLI telemetry collection (the collector skips gracefully when unset). +4. Optionally set the `REGISTRY_URL` Actions **variable** to enable first-party CLI telemetry collection. Unset, the collector warns and skips. + +### When telemetry collection stops + +A collector that fails quietly is worse than one that fails: in June 2026 a Registry +route change dropped live CLI telemetry for seven days and nothing in the logs said +so. The telemetry collector therefore distinguishes a blip from an outage: + +| Situation | Result | +|---|---| +| `REGISTRY_URL` unset, nothing ever collected | warning, exit 0 — legitimately not provisioned | +| `REGISTRY_URL` unset, but telemetry *was* collected before | **error, exit 1** — the variable was removed | +| `REGISTRY_URL` set, nothing ever collected | **error, exit 1** — broken feed or wrong URL | +| Failing for ≤ `TELEMETRY_STALE_AFTER_DAYS` (default 2) | warning, exit 0 — transient | +| Failing for longer | **error, exit 1** — the dashboard is serving stale numbers | + +Failures emit GitHub Actions annotations so they surface in the run summary. The step +is marked `continue-on-error`, so a telemetry outage is loud but never costs the other +collectors their data for the day. A failed run never writes: zeros must not overwrite +a good snapshot. The workflow auto-discovers public repos in the orgs listed in `GITHUB_ORG`. Add a new repo to the org, the next run picks it up. No manual list maintenance. From e241dd455cc2d97b16063b60e0ceb58ca1542293 Mon Sep 17 00:00:00 2001 From: Abdel Fane Date: Thu, 16 Jul 2026 17:13:02 -0600 Subject: [PATCH 3/8] fix(telemetry): sanitize log injection, detect zero-collapse, surface outages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adversarial review found the previous two commits shipped a false premise and two real defects. Correcting all of it. THE PREMISE WAS WRONG. I justified this change with registry #283, an incident it cannot detect. #283 moved the INGEST path (CLIs POSTing heartbeats in). This collector reads the ADOPTION FEED, a different endpoint that stayed up and would have answered HTTP 200 with a valid all-zeros payload — normalizeAdoptionFeed accepts that (correctly; it is well-formed), persist() writes it, lastSuccessDate advances, and classifyCollectionSkip is never called. The collector was also created 2026-07-02, one day before #283 was fixed, so it did not exist during the gap. The claim appeared in three comments, a test name and the README. Rather than just delete the story, add the detector that actually catches that shape: classifyFeedHealth compares against the previous snapshot and errors when a live fleet reports mau=0 AND installs=0. Users do not all vanish overnight. The zeros are still recorded as reported — we do not suppress what the feed said; raising the alarm is the job. Only a total collapse of both counts triggers: a decline, or MAU-only churn, is not assumed to be an outage. HIGH — log injection. httpGetJson embeds up to 200 bytes of the registry's raw response body into the message; a JSON parse error echoes body bytes even on a 200. That message reached an ::annotation:: on stdout AND console.error on stderr, and the runner scans both. Only annotate() scrubbed, and its /\r?\n/ missed a lone \r — .NET line readers break on \r, \n and \r\n alike. Verified against a hostile server: the old code emitted three attacker-controlled commands including ::stop-commands::, letting a compromised registry suppress the outage alert this code exists to raise. Now sanitized once at the boundary (classifyCollectionSkip sanitizes its own output, so every consumer is safe by construction), covering \r, \n, U+2028/29/0085, defanging `::`, and bounding length. Re-verified: 0 injected commands, 0 surviving CRs. HIGH — the exit 1 was swallowed. continue-on-error means the job goes green, and a green scheduled run notifies nobody: the change relocated the silence rather than ending it. The telemetry step now has an id, and a post-commit step re-raises the failure so the run goes red and the notification fires — after the day's npm/pypi/docker/hf/chrome data is safely committed. MEDIUM — a FUTURE last-success date (clock skew, bad row) made age negative, which never exceeds the threshold, pinning the collector to "warning" forever and silently disabling escalation. Now errors. MEDIUM — lastSuccessDate() mapped every db error to null, so a corrupt or unreadable database asserted "has never succeeded" and pointed the operator at the feed. Now three-valued: known / never / unknown, with unknown getting its own honest message. A missing table is still legitimately 'never'. MEDIUM — Number('') === 0 and Number.isInteger(0) is true, so my first guard let an empty `${{ vars.UNSET }}` set the tolerance to zero and error on the first blip. Empties are rejected before coercion. Also: the impure glue was at 0% coverage and every real bug above lived in it. collect-telemetry-stats.js now exports its internals for test, mirroring collect-chrome-stats.js which already does this for parseListing. Also: raw U+2028 bytes pasted into source made `file` and `grep` classify lib/telemetry.js as binary data. Written as escapes now. 52 tests pass (was 39). New tests fail against the previous commit. Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A --- .github/workflows/collect-stats.yml | 29 +++- lib/telemetry.js | 237 +++++++++++++++++++++------- scripts/collect-telemetry-stats.js | 104 +++++++++--- test/telemetry.test.js | 137 +++++++++++++++- 4 files changed, 417 insertions(+), 90 deletions(-) diff --git a/.github/workflows/collect-stats.yml b/.github/workflows/collect-stats.yml index 4fe71511..9c5d26b0 100644 --- a/.github/workflows/collect-stats.yml +++ b/.github/workflows/collect-stats.yml @@ -80,13 +80,17 @@ jobs: run: npm run collect-chrome - name: Collect first-party CLI telemetry - # A sustained collection failure exits 1 so it is visible here and in the - # run summary as an annotation. continue-on-error keeps that from - # dropping the other collectors' data: this step runs before - # generate-md/generate-summary and the commit step below, so a hard - # failure would silently cost a full day of npm/pypi/docker/hf/chrome - # numbers. A transient blip warns and exits 0 — see - # classifyCollectionSkip in lib/telemetry.js. + id: telemetry + # continue-on-error keeps a telemetry outage from dropping the other + # collectors' data: this step runs before generate-md/generate-summary + # and the commit step, so a hard failure here would silently cost a full + # day of npm/pypi/docker/hf/chrome numbers. + # + # But continue-on-error alone would ALSO swallow the failure — the job + # would go green and a scheduled run that nobody watches would notify + # nobody, which is the exact silence this is meant to end. The + # "Surface telemetry outage" step at the end re-raises it once the data + # is safely committed. continue-on-error: true env: # Registry base URL exposing the coarse public adoption feed @@ -110,3 +114,14 @@ jobs: git add data/analytics.db data/badge-*.json data/stats-*.json data/npm-stats-*.json data/pypi-stats-*.json data/docker-badge-*.json data/hf-badge-*.json data/chrome-badge-*.json data/chrome-stats-*.json data/telemetry-badge.json data/summary.json ANALYTICS.md ANALYTICS_DETAILED.md git diff --staged --quiet || git commit -m "chore: update analytics data [skip ci]" git push || echo "No changes to push" + + # Re-raise a telemetry outage AFTER the day's data is safely committed, so + # the run goes red and the scheduled-workflow failure notification fires. + # Without this the telemetry step's continue-on-error would leave the job + # green and nobody would ever learn that collection stopped — which is the + # failure mode this whole thing exists to prevent. + - name: Surface telemetry outage + if: steps.telemetry.outcome == 'failure' + run: | + echo "::error::First-party telemetry collection failed — see the 'Collect first-party CLI telemetry' step above. Other collectors' data for today was committed successfully." + exit 1 diff --git a/lib/telemetry.js b/lib/telemetry.js index c24e42ee..8e39b7b6 100644 --- a/lib/telemetry.js +++ b/lib/telemetry.js @@ -126,28 +126,68 @@ function daysBetween(fromISODate, toISODate) { return Math.round((to - from) / 86400000); } +// The GitHub Actions runner scans BOTH stdout and stderr for `::command::` +// lines, so any untrusted text reaching a log line can forge or suppress +// workflow commands. +// +// A lone \r matters. .NET line readers break on \r, \n AND \r\n, so /\r?\n/ is +// not enough - a bare CR starts a new line by itself. +// +// Not hypothetical here: the HTTP error path embeds up to 200 bytes of the +// registry raw response body, and a JSON parse error echoes body bytes even on +// a 200 response. A compromised or MITM-d registry (httpGetJson permits http:) +// could otherwise emit an add-mask or stop-commands directive and suppress the +// very outage annotation this module exists to raise. +// +// Written as escapes on purpose: pasting the raw characters in makes `file` and +// `grep` classify this source as binary data. U+2028/U+2029/U+0085 are NOT .NET +// line terminators and cannot inject on their own - they are collapsed anyway +// because they render as breaks in some viewers and have no business on a log +// line. +const LOG_UNSAFE = /[\r\n\u2028\u2029\u0085]/g; + +/** + * Make untrusted text safe to place on a log line. + * Collapses every line terminator and defangs the `::` command sigil. + */ +function sanitizeForLog(value, maxLength = 300) { + const flat = String(value ?? '').replace(LOG_UNSAFE, ' '); + // A workflow command must start the line, but defanging `::` costs nothing + // and removes the class rather than reasoning about column positions. + const defanged = flat.replace(/::/g, "__"); + return defanged.length > maxLength ? `${defanged.slice(0, maxLength)}...` : defanged; +} + /** * Decide how loudly a skipped/failed telemetry collection should complain. * * Why this exists: the collector had three separate silent `exit 0` paths — an * unset REGISTRY_URL, a failed fetch, and a failed validation. Each was - * individually defensible (don't fail the daily workflow; don't write zeros over - * a good snapshot), but together they made an outage look exactly like a healthy - * no-op. Registry #283 dropped live CLI telemetry for ~7 days (2026-06-26 → - * 2026-07-03) and nothing in the logs said so. Silence is not success. + * individually defensible (don't fail the daily workflow; don't write zeros + * over a good snapshot), but together they made a broken collector look exactly + * like a healthy no-op. Silence is not success. + * + * Scope, stated honestly: this catches the collector failing to RETRIEVE the + * feed. It does NOT catch the feed being served successfully with wrong or + * collapsed numbers — that is `classifyFeedHealth` below, and it is the shape a + * broken ingest actually takes. * * The rule: a single failed run is transient and warns. A sustained one is an - * outage and fails. Losing the variable after it once worked is a regression and - * fails immediately — that is a config being removed, not a flaky endpoint. + * outage and fails. Losing the variable after it once worked is a regression + * and fails immediately — that is config being removed, not a flaky endpoint. * * Pure: no db, no network, no clock. The caller supplies `today` and the last - * known-good snapshot date so this is testable and deterministic. + * known-good snapshot so this is deterministic and testable. * * @param {object} o - * @param {string} o.reason human-readable cause, echoed into the message + * @param {string} o.reason cause, echoed into the message (untrusted: may carry + * registry response bytes, so the returned message is always sanitized) * @param {boolean} o.registryUrlSet whether REGISTRY_URL is configured - * @param {string|null} o.lastSuccessDate YYYY-MM-DD of the newest stored - * snapshot, or null if the collector has never persisted one + * @param {{state:'known'|'never'|'unknown', date?:string}|string|null} + * o.lastSuccess newest stored snapshot. `unknown` means the store could not be + * read — which is NOT the same as "never collected" and must not be reported + * as it. A bare string/null is accepted for convenience and mapped to + * known/never. * @param {string} o.today YYYY-MM-DD * @param {number} [o.staleAfterDays] * @returns {{level:'warning'|'error', exitCode:0|1, message:string}} @@ -155,77 +195,162 @@ function daysBetween(fromISODate, toISODate) { function classifyCollectionSkip({ reason, registryUrlSet, + lastSuccess, lastSuccessDate, today, staleAfterDays = DEFAULT_STALE_AFTER_DAYS, }) { + // Accept the older bare-date/null shape so callers and tests can use either. + const last = normalizeLastSuccess(lastSuccess !== undefined ? lastSuccess : lastSuccessDate); + const tolerance = normalizeStaleAfterDays(staleAfterDays); + const why = sanitizeForLog(reason); + + const out = (level, message) => ({ + level, + exitCode: level === 'error' ? 1 : 0, + message: sanitizeForLog(message, 500), + }); + + if (last.state === 'unknown') { + // The store is unreadable (corrupt / locked / permission denied). We cannot + // tell a first run from a seven-day outage, so say exactly that rather than + // asserting a specific false diagnosis. + return out( + 'error', + `Telemetry collection failed: ${why}. Additionally, the snapshot store could not be read ` + + `(${last.detail || 'unknown error'}), so how long this has been failing is UNKNOWN.` + ); + } + if (!registryUrlSet) { - // Never configured: legitimately optional, and erroring every day before the - // endpoint is provisioned would train everyone to ignore the annotation. - if (!lastSuccessDate) { - return { - level: 'warning', - exitCode: 0, - message: - 'Telemetry collection is not configured: REGISTRY_URL is unset and no snapshot has ever been stored. ' + - 'Set the REGISTRY_URL repo variable to enable it.', - }; + if (last.state === 'never') { + // Never configured: legitimately optional. Erroring every day before the + // endpoint is provisioned would train everyone to ignore the annotation. + return out( + 'warning', + 'Telemetry collection is not configured: REGISTRY_URL is unset and no snapshot has ever ' + + 'been stored. Set the REGISTRY_URL repo variable to enable it.' + ); } - // It worked before and the variable is now gone. That is someone removing - // config, not a transient blip — say so on the first run. - return { - level: 'error', - exitCode: 1, - message: - `REGISTRY_URL is unset but telemetry was collected as recently as ${lastSuccessDate}. ` + - 'The variable was removed or lost; collection has stopped.', - }; + // It worked before and the variable is gone. Config removed, not a blip. + return out( + 'error', + `REGISTRY_URL is unset but telemetry was collected as recently as ${last.date}. ` + + 'The variable was removed or lost; collection has stopped.' + ); } - if (!lastSuccessDate) { + if (last.state === 'never') { // Configured deliberately, yet nothing has ever landed. Broken setup. - return { - level: 'error', - exitCode: 1, - message: - `Telemetry collection failed and has NEVER succeeded: ${reason}. ` + - 'REGISTRY_URL is set, so this is a broken feed or a wrong URL, not an unprovisioned one.', - }; + return out( + 'error', + `Telemetry collection failed and has never succeeded: ${why}. REGISTRY_URL is set, so this ` + + 'is a broken feed or a wrong URL, not an unprovisioned one.' + ); } - const age = daysBetween(lastSuccessDate, today); + const age = daysBetween(last.date, today); if (age === null) { - return { - level: 'error', - exitCode: 1, - message: `Telemetry collection failed: ${reason}. Could not parse last-success date ${lastSuccessDate}.`, - }; + return out('error', `Telemetry collection failed: ${why}. Last-success date ${last.date} is unparseable.`); } - if (age > staleAfterDays) { - return { - level: 'error', - exitCode: 1, - message: - `Telemetry collection has failed for ${age} days (last good snapshot ${lastSuccessDate}): ${reason}. ` + - 'This is an outage, not a blip — the dashboard is serving stale active-user numbers.', - }; + if (age < 0) { + // A future snapshot date means clock skew or a bad row. Left alone this + // silently disables escalation forever (a negative age never exceeds the + // threshold), which defeats the entire point of this function. + return out( + 'error', + `Telemetry collection failed: ${why}. The last-success date ${last.date} is ${-age} day(s) in ` + + `the FUTURE relative to ${today} — clock skew or a corrupt row. Staleness cannot be judged.` + ); } + if (age > tolerance) { + return out( + 'error', + `Telemetry collection has failed for ${age} day(s) (last good snapshot ${last.date}): ${why}. ` + + 'This is an outage, not a blip — the dashboard is serving stale active-user numbers.' + ); + } + + return out( + 'warning', + `Telemetry collection skipped: ${why}. Last good snapshot ${last.date} (${age}d ago); ` + + `tolerating up to ${tolerance}d before this becomes an error.` + ); +} + +/** + * Catch the failure shape a retrieval check cannot see: the feed responds 200 + * with structurally valid numbers that have collapsed to zero. + * + * This is what a broken ingest actually looks like from here. When registry #283 + * (2026-06-26) moved the ingest route, CLIs stopped POSTing but the adoption + * feed this collector reads stayed up and simply reported zeros. A retrieval + * check sees a healthy 200 and persists the zeros; only comparing against + * yesterday reveals it. + * + * Real users do not all vanish overnight. A drop from a non-zero MAU to zero is + * a pipeline failure until proven otherwise. + * + * Note this REPORTS but never suppresses the write: the zeros are what the feed + * said, and recording them honestly is right. Raising the alarm is the job. + * + * @param {{mau:number, totalInstalls:number}} current normalized feed + * @param {{mau:number, totalInstalls:number}|null} previous newest stored + * snapshot, or null when there is nothing to compare against + * @returns {{level:'error', message:string}|null} null when healthy + */ +function classifyFeedHealth(current, previous) { + if (!previous) return null; + const prevMau = Number(previous.mau) || 0; + const prevInstalls = Number(previous.totalInstalls) || 0; + if (prevMau <= 0 && prevInstalls <= 0) return null; // nothing to fall from + + const mau = Number(current.mau) || 0; + const installs = Number(current.totalInstalls) || 0; + if (mau > 0 || installs > 0) return null; + return { - level: 'warning', - exitCode: 0, - message: - `Telemetry collection skipped: ${reason}. Last good snapshot ${lastSuccessDate} (${age}d ago); ` + - `tolerating up to ${staleAfterDays}d before this becomes an error.`, + level: 'error', + message: sanitizeForLog( + `Telemetry feed returned all zeros (mau=0, installs=0) but the previous snapshot had ` + + `mau=${prevMau}, installs=${prevInstalls}. Users do not all disappear overnight — this is ` + + 'almost certainly a broken ingest path, not real churn. The zeros were recorded as reported; ' + + 'investigate before trusting the dashboard.', + 500 + ), }; } +function normalizeLastSuccess(v) { + if (v && typeof v === 'object' && typeof v.state === 'string') return v; + if (typeof v === 'string' && v !== '') return { state: 'known', date: v }; + return { state: 'never' }; +} + +// Guard the operator-supplied tolerance. Number('') is 0, so the common Actions +// pattern `TELEMETRY_STALE_AFTER_DAYS: ${{ vars.UNSET }}` would otherwise +// silently set the tolerance to zero and error on the first transient blip. +// Negative values are worse: age is never < 0, so every run would error. +function normalizeStaleAfterDays(v) { + if (v === null || v === undefined) return DEFAULT_STALE_AFTER_DAYS; + // Number('') and Number(' ') are both 0, which Number.isInteger happily + // accepts — so an empty `${{ vars.UNSET }}` would set the tolerance to zero + // and error on the first transient blip. Reject empties before coercing. + if (typeof v === 'string' && v.trim() === '') return DEFAULT_STALE_AFTER_DAYS; + const n = Number(v); + if (!Number.isInteger(n) || n < 0) return DEFAULT_STALE_AFTER_DAYS; + return n; +} + module.exports = { normalizeAdoptionFeed, toolDisplayName, toCount, classifyCollectionSkip, + classifyFeedHealth, + sanitizeForLog, daysBetween, DEFAULT_STALE_AFTER_DAYS, TOOL_PRODUCT_NAMES, diff --git a/scripts/collect-telemetry-stats.js b/scripts/collect-telemetry-stats.js index da9594c4..2af8243e 100644 --- a/scripts/collect-telemetry-stats.js +++ b/scripts/collect-telemetry-stats.js @@ -7,7 +7,12 @@ const fs = require('fs'); require('dotenv').config(); -const { normalizeAdoptionFeed, classifyCollectionSkip, DEFAULT_STALE_AFTER_DAYS } = require('../lib/telemetry'); +const { + normalizeAdoptionFeed, + classifyCollectionSkip, + classifyFeedHealth, + sanitizeForLog, +} = require('../lib/telemetry'); /* * First-party CLI telemetry collector. @@ -41,9 +46,11 @@ const { normalizeAdoptionFeed, classifyCollectionSkip, DEFAULT_STALE_AFTER_DAYS * step runs before generate-md and the commit step). */ const REGISTRY_URL = (process.env.REGISTRY_URL || '').trim().replace(/\/+$/, ''); -const STALE_AFTER_DAYS = Number.isFinite(Number(process.env.TELEMETRY_STALE_AFTER_DAYS)) - ? Number(process.env.TELEMETRY_STALE_AFTER_DAYS) - : DEFAULT_STALE_AFTER_DAYS; +// Left unvalidated here on purpose: normalizeStaleAfterDays inside +// classifyCollectionSkip rejects anything that is not a non-negative integer and +// falls back to the default. Number('') === 0 would otherwise set the tolerance +// to zero for the standard `${{ vars.UNSET }}` Actions pattern. +const STALE_AFTER_DAYS = process.env.TELEMETRY_STALE_AFTER_DAYS; const FEED_PATH = '/api/v1/telemetry/v1/adoption/public'; const dbPath = path.join(__dirname, '..', 'data', 'analytics.db'); @@ -54,26 +61,59 @@ const today = new Date().toISOString().split('T')[0]; * and the PR/commit UI, not just buried in step output nobody opens. No-op * locally. */ +/** + * Emit a GitHub Actions annotation so a failure surfaces in the run summary and + * the commit UI, not only in step output nobody opens. + * + * The runner scans stdout AND stderr for `::command::` lines, so `message` must + * already be sanitized by the caller — sanitizing only here would leave the + * console.* sinks below as an open injection path. Sanitizing again is cheap + * and keeps this safe if a future caller forgets. + */ function annotate(level, message) { if (process.env.GITHUB_ACTIONS) { - // Annotations are single-line; collapse any newlines. - console.log(`::${level}::${String(message).replace(/\r?\n/g, ' ')}`); + console.log(`::${level}::${sanitizeForLog(message, 500)}`); } } /** - * Newest stored snapshot date, or null if the collector has never persisted one - * (including when the telemetry tables don't exist yet, or the db is missing). - * Read-only and never throws — this runs on the failure path, where a second - * error would just mask the first. + * Newest stored snapshot, three-valued. + * + * { state: 'known', date } — a snapshot exists + * { state: 'never' } — the store is readable and holds nothing + * { state: 'unknown', ... } — the store could not be read + * + * The distinction matters: collapsing 'unknown' into 'never' makes the + * collector assert "has never succeeded" when the truth is "the database is + * corrupt and 60 days of data may be sitting in it" — a confident, wrong + * diagnosis pointing the operator at the wrong system. */ -function lastSuccessDate() { +function readLastSuccess() { let db; try { - if (!fs.existsSync(dbPath)) return null; + if (!fs.existsSync(dbPath)) return { state: 'never' }; db = new Database(dbPath, { readonly: true }); const row = db.prepare('SELECT MAX(date) AS d FROM telemetry_snapshots').get(); - return row && row.d ? row.d : null; + return row && row.d ? { state: 'known', date: row.d } : { state: 'never' }; + } catch (err) { + // A missing table means the schema predates telemetry: readable, empty. + if (/no such table/i.test(err.message || '')) return { state: 'never' }; + return { state: 'unknown', detail: err.message }; + } finally { + try { if (db) db.close(); } catch { /* already closed */ } + } +} + +/** Newest stored fleet totals, for the zero-collapse check. Null if unavailable. */ +function readPreviousTotals() { + let db; + try { + if (!fs.existsSync(dbPath)) return null; + db = new Database(dbPath, { readonly: true }); + const row = db + .prepare('SELECT mau, total_installs AS totalInstalls FROM telemetry_snapshots WHERE date < ? ORDER BY date DESC LIMIT 1') + .get(today); + return row || null; } catch { return null; } finally { @@ -89,17 +129,18 @@ function reportSkipAndExit(reason) { const { level, exitCode, message } = classifyCollectionSkip({ reason, registryUrlSet: Boolean(REGISTRY_URL), - lastSuccessDate: lastSuccessDate(), + lastSuccess: readLastSuccess(), today, staleAfterDays: STALE_AFTER_DAYS, }); + // `message` is sanitized by the classifier, so both sinks are safe. annotate(level, message); if (level === 'error') console.error(' %s', message); else console.warn(' %s', message); process.exit(exitCode); } -if (!REGISTRY_URL) { +if (require.main === module && !REGISTRY_URL) { reportSkipAndExit('REGISTRY_URL is not set'); } @@ -239,6 +280,10 @@ async function main() { reportSkipAndExit(`feed failed validation: ${err.message}`); } + // Compare against yesterday BEFORE writing, so today's row can't be the thing + // we compare to. + const previous = readPreviousTotals(); + const db = new Database(dbPath); try { persist(db, feed); @@ -251,10 +296,31 @@ async function main() { ' Installs: %d | WAU: %d | MAU: %d | engaged(MAU): %d | tools: %d | countries: %d', feed.totalInstalls, feed.wau, feed.mau, feed.engagedMau, feed.tools.length, feed.byCountry.length ); + + // A retrieval check cannot see a feed that answers 200 with collapsed numbers, + // which is exactly what a broken ingest looks like from here. The zeros are + // recorded as reported — we don't suppress what the feed said — but a live + // fleet does not drop to zero overnight, so say so loudly. + const health = classifyFeedHealth(feed, previous); + if (health) { + annotate(health.level, health.message); + console.error(' %s', health.message); + process.exit(1); + } + console.log('First-party telemetry collection complete.'); } -main().catch(error => { - console.error('Fatal error: %s', error.message); - process.exit(1); -}); +if (require.main === module) { + main().catch(error => { + const msg = sanitizeForLog(error && error.message); + annotate('error', `Telemetry collector crashed: ${msg}`); + console.error('Fatal error: %s', msg); + process.exit(1); + }); +} + +// Exported for unit tests. Mirrors collect-chrome-stats.js, which exports +// parseListing for test/chrome.test.js — the impure glue is where the real bugs +// live, so it needs to be reachable from a test. +module.exports = { annotate, readLastSuccess, readPreviousTotals }; diff --git a/test/telemetry.test.js b/test/telemetry.test.js index b710a3f3..321c26fd 100644 --- a/test/telemetry.test.js +++ b/test/telemetry.test.js @@ -5,6 +5,8 @@ const { toolDisplayName, toCount, classifyCollectionSkip, + classifyFeedHealth, + sanitizeForLog, daysBetween, } = require('../lib/telemetry'); @@ -110,10 +112,10 @@ test('normalizeAdoptionFeed tolerates missing optional arrays', () => { // --- classifyCollectionSkip ------------------------------------------------ // -// Regression guard for the incident this logic exists to prevent: the collector -// had three separate silent `exit 0` paths, so registry #283 dropped live CLI -// telemetry for ~7 days (2026-06-26 -> 2026-07-03) with nothing in the logs -// saying so. Silence is not success. +// Covers the collector failing to RETRIEVE the feed. Note what this does NOT +// cover: a feed that answers 200 with collapsed numbers. That is the shape a +// broken ingest actually takes, and it is classifyFeedHealth's job — see the +// zero-collapse block further down. const skip = (o) => classifyCollectionSkip({ reason: 'feed fetch failed: HTTP 404', today: '2026-07-16', ...o }); @@ -157,12 +159,11 @@ test('past the staleness threshold: errors as an outage', () => { const r = skip({ registryUrlSet: true, lastSuccessDate: '2026-07-13', staleAfterDays: 2 }); assert.equal(r.level, 'error'); assert.equal(r.exitCode, 1); - assert.match(r.message, /3 days/); + assert.match(r.message, /3 day/); assert.match(r.message, /outage/i); }); -test('the #283 shape: a 7-day gap fails loudly instead of exiting 0', () => { - // 2026-06-26 -> 2026-07-03 went unnoticed. It must not be possible again. +test('a week-long retrieval gap fails loudly instead of exiting 0', () => { const r = classifyCollectionSkip({ reason: 'feed fetch failed: HTTP 404', registryUrlSet: true, @@ -171,7 +172,127 @@ test('the #283 shape: a 7-day gap fails loudly instead of exiting 0', () => { }); assert.equal(r.level, 'error'); assert.equal(r.exitCode, 1); - assert.match(r.message, /7 days/); + assert.match(r.message, /7 day/); +}); + +test('a FUTURE last-success date errors instead of disabling escalation forever', () => { + // age < 0 never exceeds the threshold, so a single skewed/corrupt row would + // otherwise pin this to "warning" permanently — silently undoing the whole point. + const r = skip({ registryUrlSet: true, lastSuccessDate: '2026-08-20' }); + assert.equal(r.level, 'error'); + assert.equal(r.exitCode, 1); + assert.match(r.message, /FUTURE/); + assert.doesNotMatch(r.message, /-\d+d ago/); // the old nonsense phrasing +}); + +test('an unreadable store is reported as UNKNOWN, not as "never succeeded"', () => { + // Conflating these asserts a specific false claim and points the operator at + // the feed when the fault is the database. + const r = skip({ + registryUrlSet: true, + lastSuccess: { state: 'unknown', detail: 'database disk image is malformed' }, + }); + assert.equal(r.level, 'error'); + assert.match(r.message, /UNKNOWN/); + assert.match(r.message, /malformed/); + assert.doesNotMatch(r.message, /never succeeded/i); +}); + +test('staleAfterDays rejects junk rather than silently tightening the tolerance', () => { + // Number('') === 0, so `${{ vars.UNSET }}` would otherwise error on the first blip. + for (const bad of ['', ' ', 'abc', '-1', '2.5', NaN, Infinity, null, undefined]) { + const r = skip({ registryUrlSet: true, lastSuccessDate: '2026-07-15', staleAfterDays: bad }); + assert.equal(r.level, 'warning', `staleAfterDays=${JSON.stringify(bad)} should fall back to the default`); + } + // A real value is still honored. + assert.equal(skip({ registryUrlSet: true, lastSuccessDate: '2026-07-13', staleAfterDays: 5 }).level, 'warning'); + assert.equal(skip({ registryUrlSet: true, lastSuccessDate: '2026-07-13', staleAfterDays: 1 }).level, 'error'); +}); + +// --- log-injection boundary ------------------------------------------------- +// +// The HTTP error path embeds up to 200 bytes of the registry's raw response body +// into the message, which reaches BOTH an ::annotation:: on stdout and a +// console.error on stderr. The runner scans both for workflow commands, so +// untrusted bytes there can forge or suppress the very outage signal this module +// raises. + +test('sanitizeForLog collapses every line terminator, including a lone CR', () => { + // A bare \r is the one that /\r?\n/ misses: .NET line readers break on it. + assert.equal(sanitizeForLog('a\rb'), 'a b'); + assert.equal(sanitizeForLog('a\nb'), 'a b'); + assert.equal(sanitizeForLog('a\r\nb'), 'a b'); + assert.equal(sanitizeForLog("a\u2028b"), "a b"); + assert.equal(sanitizeForLog("a\u2029b"), "a b"); + assert.equal(sanitizeForLog("a\u0085b"), "a b"); +}); + +test('sanitizeForLog defangs the workflow-command sigil', () => { + assert.doesNotMatch(sanitizeForLog('x::add-mask::secret'), /::/); +}); + +test('a hostile feed body cannot inject a workflow command into the message', () => { + const hostile = 'HTTP 500: x\r::add-mask::0\r::stop-commands::deadbeef'; + const r = classifyCollectionSkip({ + reason: `feed fetch failed: ${hostile}`, + registryUrlSet: true, + lastSuccessDate: '2026-07-15', + today: '2026-07-16', + }); + // Nothing that could start a new line, and no sigil left to start one with. + assert.ok(!/[\r\n\u2028\u2029\u0085]/.test(r.message), "message must be single-line"); + assert.doesNotMatch(r.message, /::/); + // The command names may survive as inert text; what must not survive is a + // `::` that could begin a workflow command. + assert.doesNotMatch(r.message, /::add-mask::/); + assert.doesNotMatch(r.message, /::stop-commands::/); +}); + +test('the message is bounded so a 200-byte body cannot flood the annotation', () => { + const r = classifyCollectionSkip({ + reason: `feed fetch failed: ${'A'.repeat(5000)}`, + registryUrlSet: true, + lastSuccessDate: '2026-07-15', + today: '2026-07-16', + }); + assert.ok(r.message.length <= 520, `message length ${r.message.length}`); +}); + +// --- classifyFeedHealth (the zero-collapse detector) ------------------------- +// +// This is the failure a retrieval check structurally cannot see, and the one a +// broken ingest actually produces: the adoption feed answers 200 with valid, +// all-zero numbers. normalizeAdoptionFeed accepts them (correctly — they are +// well-formed), persist() writes them, and lastSuccessDate advances, so +// classifyCollectionSkip is never even called. + +test('a live fleet collapsing to zero is reported as an outage', () => { + const r = classifyFeedHealth({ mau: 0, totalInstalls: 0 }, { mau: 200, totalInstalls: 500 }); + assert.ok(r, 'a drop from 200 MAU to 0 must not pass silently'); + assert.equal(r.level, 'error'); + assert.match(r.message, /200/); + assert.match(r.message, /do not all disappear overnight/i); +}); + +test('healthy numbers report nothing', () => { + assert.equal(classifyFeedHealth({ mau: 200, totalInstalls: 500 }, { mau: 190, totalInstalls: 480 }), null); +}); + +test('a real decline is not an outage — only a total collapse is', () => { + assert.equal(classifyFeedHealth({ mau: 1, totalInstalls: 1 }, { mau: 200, totalInstalls: 500 }), null); +}); + +test('no previous snapshot means nothing to compare against', () => { + assert.equal(classifyFeedHealth({ mau: 0, totalInstalls: 0 }, null), null); +}); + +test('zero following zero is not a collapse (nothing to fall from)', () => { + assert.equal(classifyFeedHealth({ mau: 0, totalInstalls: 0 }, { mau: 0, totalInstalls: 0 }), null); +}); + +test('installs surviving while MAU zeroes is not yet a collapse', () => { + // Only a total collapse of both is unambiguous; MAU alone could be real churn. + assert.equal(classifyFeedHealth({ mau: 0, totalInstalls: 500 }, { mau: 200, totalInstalls: 500 }), null); }); test('the reason is always carried into the message', () => { From 0738f237d81b764312e36a41f1d6b9e51950ef69 Mon Sep 17 00:00:00 2001 From: Abdel Fane Date: Thu, 16 Jul 2026 17:13:59 -0600 Subject: [PATCH 4/8] docs: correct the collector's failure-policy story MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The README justified the loud-failure work with registry #283, an incident this collector structurally cannot detect: #283 broke the INGEST path, while this reads the ADOPTION FEED — a different endpoint that stayed up and would have answered 200 with zeros. Replaced with what is actually true, and documented the zero-collapse detector that does catch that shape. Also softened the same claim in classifyFeedHealth's docstring: the collector was written 2026-07-02, after the gap closed, so it never observed the feed's behaviour during #283. Stated as the mechanism, not as an observation. Documents the log-injection boundary while here — untrusted feed text is sanitized before reaching any log line, because the runner parses ::command:: on both stdout and stderr. Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A --- README.md | 27 +++++++++++++++++++++------ lib/telemetry.js | 15 ++++++++++----- 2 files changed, 31 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index b2796310..4d95dcf1 100644 --- a/README.md +++ b/README.md @@ -106,9 +106,10 @@ The included workflow (`.github/workflows/collect-stats.yml`) runs daily at `6:0 ### When telemetry collection stops -A collector that fails quietly is worse than one that fails: in June 2026 a Registry -route change dropped live CLI telemetry for seven days and nothing in the logs said -so. The telemetry collector therefore distinguishes a blip from an outage: +A collector that fails quietly is worse than one that fails outright. Two different +failures are checked, because they look nothing alike. + +**The feed can't be retrieved.** A blip is tolerated; an outage is not: | Situation | Result | |---|---| @@ -118,11 +119,25 @@ so. The telemetry collector therefore distinguishes a blip from an outage: | Failing for ≤ `TELEMETRY_STALE_AFTER_DAYS` (default 2) | warning, exit 0 — transient | | Failing for longer | **error, exit 1** — the dashboard is serving stale numbers | -Failures emit GitHub Actions annotations so they surface in the run summary. The step -is marked `continue-on-error`, so a telemetry outage is loud but never costs the other -collectors their data for the day. A failed run never writes: zeros must not overwrite +**The feed responds fine but the numbers collapsed.** This is the shape a broken +ingest path actually takes: the adoption feed keeps answering `200` with a +structurally valid payload while every count reads zero. A retrieval check sees a +healthy response and happily persists the zeros. So the collector also compares +against the previous snapshot and errors when a live fleet reports `mau=0` **and** +`installs=0` — real users do not all vanish overnight. The zeros are still recorded +as reported (we don't suppress what the feed said); the point is to raise the alarm. +A decline, or MAU-only churn, is not treated as an outage. + +Failures emit GitHub Actions annotations so they surface in the run summary, and a +final step re-raises the failure **after** the day's data is committed — so the run +goes red and the scheduled-run notification fires, without a telemetry outage costing +the other collectors their data. A failed run never writes: zeros must not overwrite a good snapshot. +Untrusted feed text (HTTP error bodies) is sanitized before it reaches any log line — +the Actions runner parses `::command::` lines on both stdout and stderr, so an +unsanitized response body could otherwise forge or suppress these very annotations. + The workflow auto-discovers public repos in the orgs listed in `GITHUB_ORG`. Add a new repo to the org, the next run picks it up. No manual list maintenance. ## Architectural notes (so you can audit it) diff --git a/lib/telemetry.js b/lib/telemetry.js index 8e39b7b6..8d26fe7f 100644 --- a/lib/telemetry.js +++ b/lib/telemetry.js @@ -284,11 +284,16 @@ function classifyCollectionSkip({ * Catch the failure shape a retrieval check cannot see: the feed responds 200 * with structurally valid numbers that have collapsed to zero. * - * This is what a broken ingest actually looks like from here. When registry #283 - * (2026-06-26) moved the ingest route, CLIs stopped POSTing but the adoption - * feed this collector reads stayed up and simply reported zeros. A retrieval - * check sees a healthy 200 and persists the zeros; only comparing against - * yesterday reveals it. + * This is what a broken ingest looks like from here, and it is the reason the + * retrieval check above is not sufficient on its own. Registry #283 (2026-06-26) + * moved the ingest route and CLIs silently stopped POSTing for ~7 days; the + * adoption feed this collector reads is a different endpoint and was unaffected, + * so it would have kept answering 200 while the counts drained. (Stated as the + * mechanism, not an observation — this collector was written 2026-07-02, after + * the gap, so nothing here actually watched it happen.) + * + * A retrieval check sees a healthy 200 and persists the zeros; only comparing + * against the previous snapshot reveals it. * * Real users do not all vanish overnight. A drop from a non-zero MAU to zero is * a pipeline failure until proven otherwise. From becf3275bf158a40d46cf27627a634fa132728c2 Mon Sep 17 00:00:00 2001 From: Abdel Fane Date: Thu, 16 Jul 2026 17:41:23 -0600 Subject: [PATCH 5/8] fix(telemetry): detect the drain the old check waved through for 90 days MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verification of the previous commit found the detector I added to catch an ingest outage would not have caught one, and would then have alerted once. WRONG ENVELOPE. It required mau === 0 AND installs === 0. Those are different windows: total_installs is distinct installs over the 90-day retention, mau is 30 days (setup-database.js). When ingest breaks, MAU empties at T+30 while installs coasts down until T+90: T+30 mau=0 installs=333 healthy <- fleet provably dead, no alert T+60 mau=0 installs=167 healthy T+90 mau=0 installs=0 ERROR <- first alert, 90 days late Sixty days of zero monthly actives against a live install base passed as healthy — and a test of mine encoded that blind spot as intentional ("could be real churn"). Zero actives against 500 known installs is not churn. Now keyed on MAU alone; installs surviving is stated as corroboration, because it is exactly what a drained ingest looks like. SELF-SILENCING. It compared against yesterday, so day one's zeros persisted and became the baseline: every later day saw zero-following-zero and reported healthy. One red run, then green forever while serving zeros — inverting this module's own escalate-on-sustained rule. Now compares against the last snapshot that actually had users, so the alarm keeps ringing and states the growing gap. Also: - Added a feed request timeout (30s, TELEMETRY_FEED_TIMEOUT_MS). A registry that accepts the connection and never answers would otherwise hang the daily workflow until the job limit killed it — a silent failure of a different shape that also takes the other collectors' commit down with it. Found while debugging my own test deadlock. - dbPath is overridable via TELEMETRY_DB_PATH so the glue is testable. Every defect three review rounds found lived in that glue, and it had 0% coverage while the path was hardcoded. - New test/telemetry-collector.test.js drives the REAL script as a subprocess against fixture databases and a hostile HTTP server: injection on both streams, corrupt store, drained feed, sustained drain, healthy run, brand-new deployment, and the require.main guard. The previous commit's export comment claimed it existed "so it needs to be reachable from a test" — no such test had been written. Now it has. 61 tests pass (was 52). Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A --- data/telemetry-badge.json | 7 + lib/telemetry.js | 81 +++++--- scripts/collect-telemetry-stats.js | 58 ++++-- test/telemetry-collector.test.js | 293 +++++++++++++++++++++++++++++ test/telemetry.test.js | 107 +++++------ 5 files changed, 445 insertions(+), 101 deletions(-) create mode 100644 data/telemetry-badge.json create mode 100644 test/telemetry-collector.test.js diff --git a/data/telemetry-badge.json b/data/telemetry-badge.json new file mode 100644 index 00000000..f80beb9a --- /dev/null +++ b/data/telemetry-badge.json @@ -0,0 +1,7 @@ +{ + "schemaVersion": 1, + "label": "active CLI users (MAU)", + "message": "0", + "color": "brightgreen", + "style": "flat" +} \ No newline at end of file diff --git a/lib/telemetry.js b/lib/telemetry.js index 8d26fe7f..b230e174 100644 --- a/lib/telemetry.js +++ b/lib/telemetry.js @@ -281,48 +281,75 @@ function classifyCollectionSkip({ } /** - * Catch the failure shape a retrieval check cannot see: the feed responds 200 - * with structurally valid numbers that have collapsed to zero. + * Catch the failure a retrieval check structurally cannot see: the feed answers + * 200 with a well-formed payload whose active-user counts have drained to zero. * - * This is what a broken ingest looks like from here, and it is the reason the - * retrieval check above is not sufficient on its own. Registry #283 (2026-06-26) - * moved the ingest route and CLIs silently stopped POSTing for ~7 days; the - * adoption feed this collector reads is a different endpoint and was unaffected, - * so it would have kept answering 200 while the counts drained. (Stated as the - * mechanism, not an observation — this collector was written 2026-07-02, after - * the gap, so nothing here actually watched it happen.) + * This is what a broken ingest looks like from here, and it is why the retrieval + * check above is not sufficient alone. Registry #283 (2026-06-26) moved the + * ingest route and CLIs silently stopped POSTing for ~7 days; the adoption feed + * this collector reads is a different endpoint and was unaffected, so it would + * have kept answering 200 while the counts drained. (Stated as the mechanism, + * not an observation — this collector was written 2026-07-02, after the gap, so + * nothing here watched it happen.) * - * A retrieval check sees a healthy 200 and persists the zeros; only comparing - * against the previous snapshot reveals it. + * Two things this gets right that the obvious version does not: * - * Real users do not all vanish overnight. A drop from a non-zero MAU to zero is - * a pipeline failure until proven otherwise. + * 1. It keys on MAU alone, NOT on "mau AND installs are both zero". Those are + * different windows — `total_installs` is distinct installs over the 90-day + * retention, `mau` is 30 days (see setup-database.js). When ingest breaks, + * MAU hits zero at T+30 while installs bleeds down until T+90. Requiring both + * would wave through ~60 days of "zero monthly actives against a live install + * base" as healthy. Zero actives against hundreds of known installs is not + * churn; it is a dead pipeline. * - * Note this REPORTS but never suppresses the write: the zeros are what the feed - * said, and recording them honestly is right. Raising the alarm is the job. + * 2. It compares against the last snapshot that actually had users, NOT against + * yesterday. Comparing to yesterday makes the alert self-silencing: day one + * fires, its zeros persist and become "yesterday", and every later day sees + * zero-following-zero and reports healthy. That inverts this module's own + * rule — a sustained failure must escalate, not go quiet. Keying on the last + * live snapshot means the alarm keeps ringing, and gets louder, until someone + * fixes it or the fleet genuinely has no users to report. + * + * This REPORTS but never suppresses the write: the zeros are what the feed said, + * and recording them honestly is right. Raising the alarm is the job. * * @param {{mau:number, totalInstalls:number}} current normalized feed - * @param {{mau:number, totalInstalls:number}|null} previous newest stored - * snapshot, or null when there is nothing to compare against + * @param {{mau:number, totalInstalls:number, date:string}|null} lastLive the + * newest stored snapshot with mau > 0, or null if none exists (a brand-new + * deployment, or a fleet that has genuinely never reported an active user — + * neither is an outage, so both return null) + * @param {string} today YYYY-MM-DD * @returns {{level:'error', message:string}|null} null when healthy */ -function classifyFeedHealth(current, previous) { - if (!previous) return null; - const prevMau = Number(previous.mau) || 0; - const prevInstalls = Number(previous.totalInstalls) || 0; - if (prevMau <= 0 && prevInstalls <= 0) return null; // nothing to fall from +function classifyFeedHealth(current, lastLive, today) { + // Nothing has ever been alive, so nothing has died. + if (!lastLive) return null; + const lastMau = Number(lastLive.mau) || 0; + if (lastMau <= 0) return null; const mau = Number(current.mau) || 0; + if (mau > 0) return null; + const installs = Number(current.totalInstalls) || 0; - if (mau > 0 || installs > 0) return null; + const days = daysBetween(lastLive.date, today); + const forHowLong = days === null || days < 0 ? '' : ` for ${days} day(s)`; + + // The install base still reporting while actives are zero is the tell: those + // are different windows, and a real fleet cannot have installs but no actives. + const installsClause = + installs > 0 + ? `The feed still reports ${installs} install(s) over the retention window, which is exactly ` + + 'what a drained ingest looks like: the 30-day active count empties first while the 90-day ' + + 'install count is still coasting on old events. ' + : 'Installs have drained to zero as well, so the retention window has now emptied too. '; return { level: 'error', message: sanitizeForLog( - `Telemetry feed returned all zeros (mau=0, installs=0) but the previous snapshot had ` + - `mau=${prevMau}, installs=${prevInstalls}. Users do not all disappear overnight — this is ` + - 'almost certainly a broken ingest path, not real churn. The zeros were recorded as reported; ' + - 'investigate before trusting the dashboard.', + `Telemetry feed reports mau=0${forHowLong}, but the last snapshot with any activity had ` + + `mau=${lastMau} on ${lastLive.date}. ${installsClause}` + + 'Users do not all disappear overnight — treat this as a broken ingest path until proven ' + + 'otherwise. The zeros were recorded as reported; investigate before trusting the dashboard.', 500 ), }; diff --git a/scripts/collect-telemetry-stats.js b/scripts/collect-telemetry-stats.js index 2af8243e..d42b75fa 100644 --- a/scripts/collect-telemetry-stats.js +++ b/scripts/collect-telemetry-stats.js @@ -53,7 +53,13 @@ const REGISTRY_URL = (process.env.REGISTRY_URL || '').trim().replace(/\/+$/, '') const STALE_AFTER_DAYS = process.env.TELEMETRY_STALE_AFTER_DAYS; const FEED_PATH = '/api/v1/telemetry/v1/adoption/public'; -const dbPath = path.join(__dirname, '..', 'data', 'analytics.db'); +const FEED_TIMEOUT_MS = Number(process.env.TELEMETRY_FEED_TIMEOUT_MS) || 30000; +// TELEMETRY_DB_PATH exists so the tests can drive the real script against +// fixture databases (corrupt, empty, no-table, drained). Every bug the reviews +// found in this collector lived in the db/annotation glue rather than in the +// pure classifiers, and that glue was untestable while this was hardcoded. +// Unset in production, where it resolves to the committed database. +const dbPath = process.env.TELEMETRY_DB_PATH || path.join(__dirname, '..', 'data', 'analytics.db'); const today = new Date().toISOString().split('T')[0]; /** @@ -104,14 +110,24 @@ function readLastSuccess() { } } -/** Newest stored fleet totals, for the zero-collapse check. Null if unavailable. */ -function readPreviousTotals() { +/** + * Newest stored snapshot that had ANY active users, for the drain check. + * + * Deliberately not "yesterday": comparing against yesterday makes the alert + * self-silencing, because the first zero day persists and becomes the baseline + * every later day is measured against. Keying on the last LIVE snapshot keeps + * the alarm ringing until someone fixes it. + */ +function readLastLiveTotals() { let db; try { if (!fs.existsSync(dbPath)) return null; db = new Database(dbPath, { readonly: true }); const row = db - .prepare('SELECT mau, total_installs AS totalInstalls FROM telemetry_snapshots WHERE date < ? ORDER BY date DESC LIMIT 1') + .prepare( + 'SELECT date, mau, total_installs AS totalInstalls FROM telemetry_snapshots ' + + 'WHERE date < ? AND mau > 0 ORDER BY date DESC LIMIT 1' + ) .get(today); return row || null; } catch { @@ -155,7 +171,7 @@ function httpGetJson(rawUrl) { } const client = url.protocol === 'http:' ? http : https; const headers = { 'User-Agent': 'github-analytics-tracker', Accept: 'application/json' }; - client.get(url, { headers }, (res) => { + const req = client.get(url, { headers }, (res) => { let data = ''; res.on('data', chunk => (data += chunk)); res.on('end', () => { @@ -170,7 +186,15 @@ function httpGetJson(rawUrl) { } }); res.on('error', reject); - }).on('error', reject); + }); + // Without this a registry that accepts the connection and never answers + // hangs the daily workflow until the job's own limit kills it — a silent + // failure of a different shape, and one that also takes the other + // collectors' commit down with it. + req.setTimeout(FEED_TIMEOUT_MS, () => { + req.destroy(new Error(`feed request timed out after ${FEED_TIMEOUT_MS}ms`)); + }); + req.on('error', reject); }); } @@ -280,9 +304,8 @@ async function main() { reportSkipAndExit(`feed failed validation: ${err.message}`); } - // Compare against yesterday BEFORE writing, so today's row can't be the thing - // we compare to. - const previous = readPreviousTotals(); + // Read BEFORE writing, so today's row can't become the thing we compare to. + const lastLive = readLastLiveTotals(); const db = new Database(dbPath); try { @@ -297,11 +320,12 @@ async function main() { feed.totalInstalls, feed.wau, feed.mau, feed.engagedMau, feed.tools.length, feed.byCountry.length ); - // A retrieval check cannot see a feed that answers 200 with collapsed numbers, + // A retrieval check cannot see a feed that answers 200 with drained numbers, // which is exactly what a broken ingest looks like from here. The zeros are // recorded as reported — we don't suppress what the feed said — but a live - // fleet does not drop to zero overnight, so say so loudly. - const health = classifyFeedHealth(feed, previous); + // fleet does not drop to zero overnight, so say so loudly, every day, until + // it is fixed. + const health = classifyFeedHealth(feed, lastLive, today); if (health) { annotate(health.level, health.message); console.error(' %s', health.message); @@ -320,7 +344,9 @@ if (require.main === module) { }); } -// Exported for unit tests. Mirrors collect-chrome-stats.js, which exports -// parseListing for test/chrome.test.js — the impure glue is where the real bugs -// live, so it needs to be reachable from a test. -module.exports = { annotate, readLastSuccess, readPreviousTotals }; +// Exported for test/telemetry-collector.test.js, which drives the real script as +// a subprocess against fixture databases. Mirrors collect-chrome-stats.js +// exporting parseListing for test/chrome.test.js. The db readers are bound to +// the module-level dbPath, so the tests exercise them through the CLI rather +// than by calling them directly. +module.exports = { annotate, readLastSuccess, readLastLiveTotals }; diff --git a/test/telemetry-collector.test.js b/test/telemetry-collector.test.js new file mode 100644 index 00000000..f0edea6a --- /dev/null +++ b/test/telemetry-collector.test.js @@ -0,0 +1,293 @@ +const { test } = require('node:test'); +const assert = require('node:assert'); +const { spawn } = require('node:child_process'); +const { mkdtempSync, rmSync, writeFileSync } = require('node:fs'); +const { join } = require('node:path'); +const { tmpdir } = require('node:os'); +const http = require('node:http'); +const Database = require('better-sqlite3'); + +/* + * Drives the REAL collector as a subprocess against fixture databases. + * + * Why this file exists: the pure classifiers in lib/telemetry.js were well + * covered, but every defect three review rounds found lived in the impure glue + * here — the db read, the annotation sinks, the env coercion. That glue had 0% + * coverage. A test asserting `annotate('error', 'a\r::add-mask::x')` emits one + * line would have caught the log-injection bug outright. + */ + +const SCRIPT = join(__dirname, '..', 'scripts', 'collect-telemetry-stats.js'); + +function makeDb(dir, rows = []) { + const p = join(dir, 'analytics.db'); + const db = new Database(p); + db.exec(`CREATE TABLE telemetry_snapshots ( + date TEXT PRIMARY KEY, generated_at TEXT, provenance TEXT, retention_days INTEGER, + wau_window_days INTEGER, mau_window_days INTEGER, total_installs INTEGER, + wau INTEGER, mau INTEGER, engaged_mau INTEGER, engaged_min_days INTEGER, + collected_at TEXT DEFAULT CURRENT_TIMESTAMP)`); + db.exec(`CREATE TABLE telemetry_tool_snapshots (date TEXT, tool TEXT, total_installs INTEGER, + wau INTEGER, mau INTEGER, engaged_mau INTEGER, collected_at TEXT, PRIMARY KEY (date, tool))`); + db.exec(`CREATE TABLE telemetry_version_snapshots (date TEXT, tool TEXT, version TEXT, + installs INTEGER, collected_at TEXT, PRIMARY KEY (date, tool, version))`); + db.exec(`CREATE TABLE telemetry_country_snapshots (date TEXT, country_code TEXT, + installs INTEGER, collected_at TEXT, PRIMARY KEY (date, country_code))`); + const ins = db.prepare('INSERT INTO telemetry_snapshots (date, mau, total_installs) VALUES (?, ?, ?)'); + for (const r of rows) ins.run(r.date, r.mau, r.totalInstalls); + db.close(); + return p; +} + +/** + * Run the collector and collect its output. + * + * Async on purpose: spawnSync blocks this process's event loop, so the + * in-process fixture server below could never accept the child's connection — + * the child would hang until its feed timeout. Use spawn and await. + */ +function run(env, dbFile) { + return new Promise((resolve) => { + const child = spawn(process.execPath, [SCRIPT], { + env: { + ...process.env, + GITHUB_ACTIONS: '1', + TELEMETRY_DB_PATH: dbFile, + TELEMETRY_FEED_TIMEOUT_MS: '5000', + ...env, + }, + }); + let stdout = ''; + let stderr = ''; + child.stdout.on('data', (d) => (stdout += d)); + child.stderr.on('data', (d) => (stderr += d)); + child.on('close', (status) => resolve({ status, stdout, stderr })); + }); +} + +/** Lines the Actions runner would parse as a workflow command. */ +function commandLines(res) { + return `${res.stdout}\n${res.stderr}`.split(/\r|\n/).filter((l) => l.trimStart().startsWith('::')); +} + +function withServer(handler, fn) { + const server = http.createServer(handler); + return new Promise((resolve, reject) => { + server.listen(0, '127.0.0.1', async () => { + try { + resolve(await fn(`http://127.0.0.1:${server.address().port}`)); + } catch (e) { + reject(e); + } finally { + server.close(); + } + }); + }); +} + +const tmp = () => mkdtempSync(join(tmpdir(), 'telem-collector-')); + +test('unset REGISTRY_URL with an empty store: warns, exit 0', async () => { + const dir = tmp(); + try { + const res = await run({ REGISTRY_URL: '' }, makeDb(dir)); + assert.equal(res.status, 0); + assert.match(res.stdout, /::warning::/); + assert.match(res.stdout, /not configured/i); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('unset REGISTRY_URL after telemetry was collected: errors, exit 1', async () => { + const dir = tmp(); + try { + const res = await run({ REGISTRY_URL: '' }, makeDb(dir, [{ date: '2026-07-15', mau: 200, totalInstalls: 500 }])); + assert.equal(res.status, 1); + assert.match(res.stdout, /::error::/); + assert.match(res.stdout, /2026-07-15/); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a corrupt store reports UNKNOWN rather than "never succeeded"', async () => { + const dir = tmp(); + try { + const p = join(dir, 'analytics.db'); + writeFileSync(p, 'this is not a sqlite file at all'); + const res = await run({ REGISTRY_URL: 'http://127.0.0.1:1' }, p); + assert.equal(res.status, 1); + assert.match(res.stdout, /UNKNOWN/); + assert.doesNotMatch(res.stdout, /never succeeded/i); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a hostile feed body cannot inject a workflow command on either stream', async () => { + // The bug this guards: annotate() scrubbed but console.error did not, and the + // scrub missed a bare \r. The runner parses BOTH stdout and stderr. + const dir = tmp(); + try { + const payload = 'boom\r::add-mask::0\r::stop-commands::deadbeef\n::error::all good here'; + await withServer( + (req, res) => { + res.writeHead(500, { 'Content-Type': 'text/plain' }); + res.end(payload); + }, + async (url) => { + const res = await run({ REGISTRY_URL: url }, makeDb(dir)); + const cmds = commandLines(res); + assert.equal(cmds.length, 1, `expected only our own annotation, got:\n${cmds.join('\n')}`); + assert.match(cmds[0], /^::error::/); + assert.equal(cmds.filter((l) => /^::(add-mask|stop-commands)::/.test(l)).length, 0); + } + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a hostile body on a 200 (JSON parse path) also cannot inject', async () => { + const dir = tmp(); + try { + await withServer( + (req, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end('x\r::add-mask::0'); + }, + async (url) => { + const res = await run({ REGISTRY_URL: url }, makeDb(dir)); + assert.equal(commandLines(res).filter((l) => /^::add-mask::/.test(l)).length, 0); + } + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a drained feed (mau=0, installs still live) errors — the shape a broken ingest makes', async () => { + // The old check required mau AND installs to both be zero. installs is a + // 90-day window and mau is 30-day, so that waved through ~60 days of a dead + // pipeline. This is the regression guard for that. + const dir = tmp(); + try { + const dbFile = makeDb(dir, [{ date: '2026-06-16', mau: 200, totalInstalls: 500 }]); + await withServer( + (req, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ totalInstalls: 333, wau: 0, mau: 0, tools: [], byCountry: [] })); + }, + async (url) => { + const res = await run({ REGISTRY_URL: url }, dbFile); + assert.equal(res.status, 1, `expected an outage exit; stdout:\n${res.stdout}`); + assert.match(res.stdout, /::error::/); + assert.match(res.stdout, /mau=0/); + assert.match(res.stdout, /333 install/); + } + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('the drain alert keeps firing on later days instead of self-silencing', async () => { + // Day 1's zeros used to become the baseline, so day 2 saw zero-following-zero + // and reported healthy — one red run, then green forever while serving zeros. + const dir = tmp(); + try { + const dbFile = makeDb(dir, [ + { date: '2026-06-16', mau: 200, totalInstalls: 500 }, + { date: '2026-07-15', mau: 0, totalInstalls: 333 }, // yesterday was already zero + ]); + await withServer( + (req, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ totalInstalls: 300, wau: 0, mau: 0, tools: [], byCountry: [] })); + }, + async (url) => { + const res = await run({ REGISTRY_URL: url }, dbFile); + assert.equal(res.status, 1, 'a sustained outage must keep erroring, not go quiet'); + assert.match(res.stdout, /2026-06-16/); // compares to the last LIVE day + } + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a healthy feed writes and exits 0', async () => { + const dir = tmp(); + try { + const dbFile = makeDb(dir, [{ date: '2026-07-15', mau: 190, totalInstalls: 480 }]); + await withServer( + (req, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end( + JSON.stringify({ + generatedAt: '2026-07-16T00:00:00Z', + totalInstalls: 500, + wau: 69, + mau: 200, + engagedMau: 12, + tools: [], + byCountry: [], + }) + ); + }, + async (url) => { + const res = await run({ REGISTRY_URL: url }, dbFile); + assert.equal(res.status, 0, `expected success; stderr:\n${res.stderr}`); + assert.equal(commandLines(res).length, 0, 'a healthy run must emit no annotations'); + assert.match(res.stdout, /collection complete/); + } + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('a brand-new deployment reporting zero is not an outage', async () => { + const dir = tmp(); + try { + await withServer( + (req, res) => { + res.writeHead(200, { 'Content-Type': 'application/json' }); + res.end(JSON.stringify({ totalInstalls: 0, wau: 0, mau: 0, tools: [], byCountry: [] })); + }, + async (url) => { + // No prior live snapshot -> nothing has died -> not an outage. + const res = await run({ REGISTRY_URL: url }, makeDb(dir)); + assert.equal(res.status, 0, `a first run with no users must not error; stdout:\n${res.stdout}`); + } + ); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +}); + +test('importing the script does not execute it', () => { + // require.main === module guards every side effect; without it, importing for + // a unit test would run a collection and possibly process.exit the test runner. + const mod = require('../scripts/collect-telemetry-stats'); + assert.equal(typeof mod.annotate, 'function'); + assert.equal(typeof mod.readLastSuccess, 'function'); + assert.equal(typeof mod.readLastLiveTotals, 'function'); +}); + +test('annotate collapses a bare CR so it cannot open a second command', () => { + const { annotate } = require('../scripts/collect-telemetry-stats'); + const seen = []; + const orig = console.log; + console.log = (s) => seen.push(s); + try { + process.env.GITHUB_ACTIONS = '1'; + annotate('error', 'a\r::add-mask::x'); + } finally { + console.log = orig; + } + assert.equal(seen.length, 1); + assert.ok(!/\r/.test(seen[0]), 'no CR may survive into a log line'); + assert.doesNotMatch(seen[0], /::add-mask::/); +}); diff --git a/test/telemetry.test.js b/test/telemetry.test.js index 321c26fd..59ba388d 100644 --- a/test/telemetry.test.js +++ b/test/telemetry.test.js @@ -258,79 +258,70 @@ test('the message is bounded so a 200-byte body cannot flood the annotation', () assert.ok(r.message.length <= 520, `message length ${r.message.length}`); }); -// --- classifyFeedHealth (the zero-collapse detector) ------------------------- +// --- classifyFeedHealth (the drain detector) --------------------------------- // -// This is the failure a retrieval check structurally cannot see, and the one a -// broken ingest actually produces: the adoption feed answers 200 with valid, -// all-zero numbers. normalizeAdoptionFeed accepts them (correctly — they are -// well-formed), persist() writes them, and lastSuccessDate advances, so -// classifyCollectionSkip is never even called. - -test('a live fleet collapsing to zero is reported as an outage', () => { - const r = classifyFeedHealth({ mau: 0, totalInstalls: 0 }, { mau: 200, totalInstalls: 500 }); - assert.ok(r, 'a drop from 200 MAU to 0 must not pass silently'); - assert.equal(r.level, 'error'); - assert.match(r.message, /200/); - assert.match(r.message, /do not all disappear overnight/i); -}); +// The failure a retrieval check structurally cannot see, and the one a broken +// ingest actually produces: the adoption feed answers 200 with valid numbers +// whose active counts have drained. normalizeAdoptionFeed accepts them +// (correctly — they are well-formed), persist() writes them, and last-success +// advances, so classifyCollectionSkip is never even called. -test('healthy numbers report nothing', () => { - assert.equal(classifyFeedHealth({ mau: 200, totalInstalls: 500 }, { mau: 190, totalInstalls: 480 }), null); -}); +const LIVE = { mau: 200, totalInstalls: 500, date: '2026-06-16' }; -test('a real decline is not an outage — only a total collapse is', () => { - assert.equal(classifyFeedHealth({ mau: 1, totalInstalls: 1 }, { mau: 200, totalInstalls: 500 }), null); +test('a drained feed is reported as an outage', () => { + const r = classifyFeedHealth({ mau: 0, totalInstalls: 333 }, LIVE, '2026-07-16'); + assert.ok(r, 'zero actives against a live install base must not pass silently'); + assert.equal(r.level, 'error'); + assert.match(r.message, /mau=0/); + assert.match(r.message, /mau=200 on 2026-06-16/); + assert.match(r.message, /do not all disappear overnight/i); }); -test('no previous snapshot means nothing to compare against', () => { - assert.equal(classifyFeedHealth({ mau: 0, totalInstalls: 0 }, null), null); +test('MAU zero while installs survive IS the drain signal, not "just churn"', () => { + // Regression guard for a real blind spot: the first version required mau AND + // installs to BOTH be zero. total_installs is a 90-day window and mau is + // 30-day, so during an ingest outage MAU empties at T+30 while installs + // coasts to T+90 — the old check waved through ~60 days of a dead pipeline. + const r = classifyFeedHealth({ mau: 0, totalInstalls: 500 }, LIVE, '2026-07-16'); + assert.ok(r, 'mau=0 with 500 live installs is a dead pipeline, not churn'); + assert.match(r.message, /500 install/); + assert.match(r.message, /coasting on old events/); }); -test('zero following zero is not a collapse (nothing to fall from)', () => { - assert.equal(classifyFeedHealth({ mau: 0, totalInstalls: 0 }, { mau: 0, totalInstalls: 0 }), null); +test('the alert keeps firing on later days rather than self-silencing', () => { + // Comparing against YESTERDAY made this fire once: day one's zeros persisted + // and became the baseline, so every later day saw zero-following-zero and + // reported healthy. Keying on the last LIVE snapshot keeps it ringing. + const day1 = classifyFeedHealth({ mau: 0, totalInstalls: 333 }, LIVE, '2026-07-16'); + const day8 = classifyFeedHealth({ mau: 0, totalInstalls: 100 }, LIVE, '2026-07-23'); + assert.ok(day1 && day8, 'a sustained outage must keep erroring'); + // And it gets louder: the gap is stated and grows. + assert.match(day1.message, /for 30 day\(s\)/); + assert.match(day8.message, /for 37 day\(s\)/); }); -test('installs surviving while MAU zeroes is not yet a collapse', () => { - // Only a total collapse of both is unambiguous; MAU alone could be real churn. - assert.equal(classifyFeedHealth({ mau: 0, totalInstalls: 500 }, { mau: 200, totalInstalls: 500 }), null); +test('once installs drain too, it still errors and says so', () => { + const r = classifyFeedHealth({ mau: 0, totalInstalls: 0 }, LIVE, '2026-09-20'); + assert.ok(r); + assert.match(r.message, /retention window has now emptied/); }); -test('the reason is always carried into the message', () => { - const r = classifyCollectionSkip({ - reason: 'feed failed validation: totalInstalls is not a number', - registryUrlSet: true, - lastSuccessDate: '2026-07-01', - today: '2026-07-16', - }); - assert.match(r.message, /totalInstalls is not a number/); +test('healthy numbers report nothing', () => { + assert.equal(classifyFeedHealth({ mau: 200, totalInstalls: 500 }, LIVE, '2026-07-16'), null); }); -test('an unparseable last-success date errors rather than passing silently', () => { - const r = skip({ registryUrlSet: true, lastSuccessDate: 'not-a-date' }); - assert.equal(r.level, 'error'); - assert.equal(r.exitCode, 1); +test('a real decline is not an outage — only reaching zero is', () => { + assert.equal(classifyFeedHealth({ mau: 1, totalInstalls: 500 }, LIVE, '2026-07-16'), null); }); -test('every classification returns a usable exit code and a non-empty message', () => { - const cases = [ - { registryUrlSet: false, lastSuccessDate: null }, - { registryUrlSet: false, lastSuccessDate: '2026-07-15' }, - { registryUrlSet: true, lastSuccessDate: null }, - { registryUrlSet: true, lastSuccessDate: '2026-07-15' }, - { registryUrlSet: true, lastSuccessDate: '2026-06-01' }, - ]; - for (const c of cases) { - const r = skip(c); - assert.ok(r.exitCode === 0 || r.exitCode === 1, JSON.stringify(c)); - assert.ok(r.level === 'warning' || r.level === 'error', JSON.stringify(c)); - assert.ok(r.message.length > 0, JSON.stringify(c)); - // An error must never exit 0: that is the bug this whole change is about. - if (r.level === 'error') assert.equal(r.exitCode, 1, JSON.stringify(c)); - } +test('a fleet that never had users is not an outage', () => { + // Brand-new deployment: nothing has died, so nothing to report. + assert.equal(classifyFeedHealth({ mau: 0, totalInstalls: 0 }, null, '2026-07-16'), null); + assert.equal(classifyFeedHealth({ mau: 0, totalInstalls: 0 }, { mau: 0, totalInstalls: 0, date: '2026-07-15' }, '2026-07-16'), null); }); -test('daysBetween handles ordinary and malformed input', () => { - assert.equal(daysBetween('2026-07-01', '2026-07-16'), 15); - assert.equal(daysBetween('2026-07-16', '2026-07-16'), 0); - assert.equal(daysBetween('garbage', '2026-07-16'), null); +test('the drain message is sanitized and bounded like every other output', () => { + const r = classifyFeedHealth({ mau: 0, totalInstalls: 333 }, LIVE, '2026-07-16'); + assert.ok(!/[\r\n]/.test(r.message), 'must be single-line'); + assert.doesNotMatch(r.message, /::/); }); From f570a86ea47a72089cf26315e6b3559091274b0d Mon Sep 17 00:00:00 2001 From: Abdel Fane Date: Thu, 16 Jul 2026 17:42:15 -0600 Subject: [PATCH 6/8] fix(ci): stop swallowing genuine git push failures; document the drain rule MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `git push || echo "No changes to push"` reported the run green whenever the push failed for ANY reason — non-fast-forward, a rejected hook (this repo has a local pre-push hook that blocks data/analytics.db), an auth error — with the day's data never pushed. That directly undercuts the premise of the new Surface step, which re-raises a telemetry outage on the grounds that the data is "safely committed". Now distinguishes nothing-to-push from a failed push. README: describes the drain rule as it now works — keyed on MAU alone (installs is a 90-day window against MAU's 30, so requiring both hid ~60 days of a dead pipeline) and compared against the last snapshot with users (comparing to yesterday made the alert fire once and then go quiet forever). Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A --- .github/workflows/collect-stats.yml | 11 ++++++++++- README.md | 28 +++++++++++++++++++++------- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/.github/workflows/collect-stats.yml b/.github/workflows/collect-stats.yml index 9c5d26b0..ca5e0773 100644 --- a/.github/workflows/collect-stats.yml +++ b/.github/workflows/collect-stats.yml @@ -113,7 +113,16 @@ jobs: git config user.email "actions@github.com" git add data/analytics.db data/badge-*.json data/stats-*.json data/npm-stats-*.json data/pypi-stats-*.json data/docker-badge-*.json data/hf-badge-*.json data/chrome-badge-*.json data/chrome-stats-*.json data/telemetry-badge.json data/summary.json ANALYTICS.md ANALYTICS_DETAILED.md git diff --staged --quiet || git commit -m "chore: update analytics data [skip ci]" - git push || echo "No changes to push" + # `git push || echo "No changes to push"` used to swallow EVERY push + # failure — a non-fast-forward, a rejected hook, an auth error — and + # report the run green with the day's data never pushed. Distinguish + # "nothing to push" (fine) from "the push failed" (not fine): the + # Surface step below relies on the data actually being committed. + if git diff --quiet HEAD@{upstream} HEAD 2>/dev/null; then + echo "No changes to push." + else + git push + fi # Re-raise a telemetry outage AFTER the day's data is safely committed, so # the run goes red and the scheduled-workflow failure notification fires. diff --git a/README.md b/README.md index 4d95dcf1..c72ba2a6 100644 --- a/README.md +++ b/README.md @@ -119,14 +119,28 @@ failures are checked, because they look nothing alike. | Failing for ≤ `TELEMETRY_STALE_AFTER_DAYS` (default 2) | warning, exit 0 — transient | | Failing for longer | **error, exit 1** — the dashboard is serving stale numbers | -**The feed responds fine but the numbers collapsed.** This is the shape a broken +**The feed responds fine but the numbers drained.** This is the shape a broken ingest path actually takes: the adoption feed keeps answering `200` with a -structurally valid payload while every count reads zero. A retrieval check sees a -healthy response and happily persists the zeros. So the collector also compares -against the previous snapshot and errors when a live fleet reports `mau=0` **and** -`installs=0` — real users do not all vanish overnight. The zeros are still recorded -as reported (we don't suppress what the feed said); the point is to raise the alarm. -A decline, or MAU-only churn, is not treated as an outage. +structurally valid payload while the active-user counts empty out. A retrieval check +sees a healthy response and happily persists the zeros. So the collector also errors +when the feed reports `mau=0` while the last snapshot with any activity had users — +real users do not all vanish overnight. + +Two details that matter, both learned the hard way: + +- It keys on **MAU alone**, not on "MAU and installs are both zero". `total_installs` + is a 90-day window and `mau` is 30-day, so a broken ingest empties MAU at T+30 + while installs coasts to T+90. Requiring both would wave through ~60 days of a dead + pipeline. Installs still reporting while actives are zero is corroboration, not a + reason to stay quiet. +- It compares against the **last snapshot that had users**, not yesterday. Comparing + to yesterday makes the alert self-silencing: day one fires, its zeros become the + baseline, and every later day sees zero-following-zero and reports healthy. Keying + on the last live snapshot keeps the alarm ringing and states the growing gap. + +The zeros are still recorded as reported (we don't suppress what the feed said); the +point is to raise the alarm. A real decline is not an outage — only reaching zero is. +A fleet that has never reported an active user is not an outage either. Failures emit GitHub Actions annotations so they surface in the run summary, and a final step re-raises the failure **after** the day's data is committed — so the run From 5ba1926edee7f2d5ccece1de1ffb3b1da65d68a0 Mon Sep 17 00:00:00 2001 From: Abdel Fane Date: Thu, 16 Jul 2026 17:44:05 -0600 Subject: [PATCH 7/8] fix(telemetry): stop the test suite publishing a fabricated badge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The final gate caught data/telemetry-badge.json in the diff. My own integration test wrote it: I made dbPath injectable but left writeBadge's path hardcoded, so the healthy-feed case wrote a real badge into data/ — one reading "label": "active CLI users (MAU)", "message": "0" That file is committed by the daily workflow and rendered as a shield, so a test fixture would have published a fabricated "0 active users" claim as if it were measured. It was introduced by becf327 and is removed here; it does not exist on main. writeBadge now honours TELEMETRY_BADGE_PATH, and the tests point it at a temp dir. Production is unchanged — unset, it resolves to the committed path, and the workflow still commits the real badge. Verified: the full suite leaves data/ clean. Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A --- data/telemetry-badge.json | 7 ------- scripts/collect-telemetry-stats.js | 11 +++++++---- test/telemetry-collector.test.js | 3 +++ 3 files changed, 10 insertions(+), 11 deletions(-) delete mode 100644 data/telemetry-badge.json diff --git a/data/telemetry-badge.json b/data/telemetry-badge.json deleted file mode 100644 index f80beb9a..00000000 --- a/data/telemetry-badge.json +++ /dev/null @@ -1,7 +0,0 @@ -{ - "schemaVersion": 1, - "label": "active CLI users (MAU)", - "message": "0", - "color": "brightgreen", - "style": "flat" -} \ No newline at end of file diff --git a/scripts/collect-telemetry-stats.js b/scripts/collect-telemetry-stats.js index d42b75fa..cc4ae620 100644 --- a/scripts/collect-telemetry-stats.js +++ b/scripts/collect-telemetry-stats.js @@ -60,6 +60,12 @@ const FEED_TIMEOUT_MS = Number(process.env.TELEMETRY_FEED_TIMEOUT_MS) || 30000; // pure classifiers, and that glue was untestable while this was hardcoded. // Unset in production, where it resolves to the committed database. const dbPath = process.env.TELEMETRY_DB_PATH || path.join(__dirname, '..', 'data', 'analytics.db'); +// Same reason, and not optional: writeBadge previously hardcoded this path, so +// a test that exercised a healthy collection wrote a real badge into the repo — +// one claiming "0 active CLI users", which the workflow commits and a shield +// renders. A test must not be able to publish a fabricated number. +const badgePath = + process.env.TELEMETRY_BADGE_PATH || path.join(__dirname, '..', 'data', 'telemetry-badge.json'); const today = new Date().toISOString().split('T')[0]; /** @@ -276,10 +282,7 @@ function writeBadge(feed) { color: 'brightgreen', style: 'flat', }; - fs.writeFileSync( - path.join(__dirname, '..', 'data', 'telemetry-badge.json'), - JSON.stringify(badge, null, 2) - ); + fs.writeFileSync(badgePath, JSON.stringify(badge, null, 2)); } async function main() { diff --git a/test/telemetry-collector.test.js b/test/telemetry-collector.test.js index f0edea6a..717a212f 100644 --- a/test/telemetry-collector.test.js +++ b/test/telemetry-collector.test.js @@ -53,6 +53,9 @@ function run(env, dbFile) { ...process.env, GITHUB_ACTIONS: '1', TELEMETRY_DB_PATH: dbFile, + // Must point somewhere disposable: without this the healthy-feed case + // wrote a real badge into data/, claiming "0 active CLI users". + TELEMETRY_BADGE_PATH: join(dbFile, '..', 'telemetry-badge.json'), TELEMETRY_FEED_TIMEOUT_MS: '5000', ...env, }, From 1fa18b92404a903983401485a2fbc479122e3a01 Mon Sep 17 00:00:00 2001 From: Abdel Fane Date: Thu, 16 Jul 2026 17:57:58 -0600 Subject: [PATCH 8/8] ci: actually run the test suite MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Nothing ran it. The only workflow in this repo was the daily collector, so every test in test/ was decoration: it passed on whoever last remembered to run it locally, and a change that broke it would merge green. That is the same shape as the silent-collector bug the suite exists to guard — a check that never fires looks identical to a check that passes. Runs on pull_request and on push to main: npm test, then the build. Also asserts the suite leaves the working tree clean. The tests drive the real collector against fixture databases and write a badge; the badge path was hardcoded until this branch, so a fixture wrote "active CLI users (MAU): 0" into data/ — a file the daily workflow commits and a shield renders. That was caught by hand during review. This makes it fail the build instead. Claude-Session: https://claude.ai/code/session_01KdsHmRjPoLfFSjPWeC3A4A --- .github/workflows/test.yml | 51 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) create mode 100644 .github/workflows/test.yml diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 00000000..b645255e --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,51 @@ +name: Test + +# Nothing ran the test suite before this workflow existed: the only workflow was +# the daily collector, so every test in test/ was decoration — it passed on +# whoever's laptop last remembered to run it, and a change that broke it merged +# green. That is the same shape as the silent-collector bug this suite exists to +# guard against, so it gets the same treatment: run it, and let it fail loudly. +on: + pull_request: + push: + branches: [main] + +# A newer run of the same branch makes older ones irrelevant. +concurrency: + group: test-${{ github.ref }} + cancel-in-progress: true + +jobs: + test: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + + - uses: actions/setup-node@v4 + with: + # Matches collect-stats.yml. better-sqlite3 is a native module, so the + # runner needs a toolchain — ubuntu-latest has one. + node-version: '24' + cache: 'npm' + + - run: npm ci + + - name: Test + run: npm test + + - name: Build + run: npm run build + + # The suite drives the real collector against fixture databases and writes + # a badge to a temp path. If either escapes into the repo, a test could + # publish a fabricated number that the daily workflow then commits — which + # has already happened once and was caught by hand. + - name: Assert the test suite left no artifacts behind + run: | + if [ -n "$(git status --porcelain)" ]; then + echo "::error::The test suite modified tracked files. Tests must not write into the repo." + git status --porcelain + git diff + exit 1 + fi + echo "Working tree clean."