From 24ef48c0e07c0fc7c2acc4b1e537b8edecfe0c1e Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Fri, 26 Jun 2026 10:20:04 +0100 Subject: [PATCH 1/9] Performance: add dry-run mode and sanity-range assertions to CodeVitals posting CodeVitals is append-only with no self-service rollback, so a bad metric (wrong key, out-of-range value, scale error) permanently pollutes the trend graph. This adds the Phase 0 safety layer before we expand the metrics surface. - --dry-run flag: build and print the payload, skip the POST, no token required (usable as a CI smoke test). Exposed as `pnpm report:dry`. - Sanity-range assertions: each typed metric is checked against SANITY_RANGES in scenarios.js before posting. Out-of-range values are logged and skipped, and the script exits non-zero so CI surfaces them. Because run-performance-tests.js spawns this script, the gate guards both `pnpm report` and the integrated `pnpm test` path. - Documented the staging-key convention and the bad-data escalation path in the README. FORMS-713 --- tools/performance/README.md | 37 +++++++ tools/performance/package.json | 1 + .../performance/scripts/post-to-codevitals.js | 99 ++++++++++++++----- tools/performance/scripts/scenarios.js | 24 +++++ 4 files changed, 136 insertions(+), 25 deletions(-) diff --git a/tools/performance/README.md b/tools/performance/README.md index ac97901439d5..7fb02597d359 100644 --- a/tools/performance/README.md +++ b/tools/performance/README.md @@ -48,4 +48,41 @@ The test suite is designed to run in TeamCity. See `TEAMCITY-SETUP.md` for detai | `pnpm calibrate` | Run CPU throttling calibration | | `pnpm measure` | Run LCP measurement only | | `pnpm report` | Post results to CodeVitals only | +| `pnpm report:dry` | Build and print the CodeVitals payload without posting (CI smoke test) | | `pnpm test -- --skip-codevitals` | Run tests without posting to CodeVitals | + +## Safeguards + +CodeVitals is an **append-only** store with no self-service rollback. Once a bad point lands (wrong key, out-of-range value, scale error), the trend graph stays polluted until a CodeVitals admin corrects it. The safeguards below keep bad data out. + +### Dry run + +`pnpm report:dry` builds the full payload, prints it, and exits without posting. It needs no `CODEVITALS_TOKEN`, so it works as a CI smoke test. Use it to inspect a payload before a real `pnpm report`. + +### Sanity-range assertions + +`post-to-codevitals.js` checks every typed metric against `SANITY_RANGES` in `scenarios.js` before posting. A value outside its range is logged and skipped (not posted), and the script exits non-zero so CI surfaces the failure. Other valid metrics in the same run still post. + +| Metric | Min | Max | +|--------|-----|-----| +| `lcp` | 100 | 60000 | +| `ttfb` | 10 | 10000 | +| `fcp` | 50 | 30000 | +| `tbt` | 0 | 10000 | +| `cls` | 0 | 5 | + +Add a row when a new metric type starts being posted, and set `metricType` on the scenario so the check applies to it. + +### Staging keys + +Post a new metric to a `-staging` CodeVitals key first (e.g. `…-timeToFirstByte-staging`) for 2-3 builds. Inspect the values in the CodeVitals UI, then rename to the production key. This gives a safety window before a new metric reaches production. + +### If bad data lands anyway + +A bad point must be corrected by the CodeVitals admin. Steps: + +1. **Stop posting.** Pause the CodeVitals Jetpack Performance Scheduler build in TeamCity. +2. **Document the extent.** Record the affected metric keys, the commit range (monorepo hashes), the time window (build start to end), and whether the values are isolated or systematic. +3. **Request a correction.** CodeVitals runs outside this project; send the request through the team channel named in the FORMS-696 runbook. Include metric ID 113, the affected keys, the commit/timestamp range, and the root cause. +4. **Fix the root cause.** Add or tighten a sanity range or staging gate. Don't re-enable the Scheduler until the fix merges. +5. **Record the incident.** Add the failure mode, detection timing, and prevention measures to the FORMS-696 maintenance runbook. diff --git a/tools/performance/package.json b/tools/performance/package.json index 13ab3cecc321..1d6669e83efa 100644 --- a/tools/performance/package.json +++ b/tools/performance/package.json @@ -13,6 +13,7 @@ "docker:up": "docker compose -f docker/docker-compose.yml up -d", "measure": "node scripts/measure-lcp.js", "report": "node scripts/post-to-codevitals.js", + "report:dry": "node scripts/post-to-codevitals.js --dry-run", "setup:browsers": "playwright install chromium", "test": "node scripts/run-performance-tests.js", "test:quick": "ITERATIONS=2 node scripts/run-performance-tests.js" diff --git a/tools/performance/scripts/post-to-codevitals.js b/tools/performance/scripts/post-to-codevitals.js index d0e78b76f2c4..9ac4abae8c4b 100644 --- a/tools/performance/scripts/post-to-codevitals.js +++ b/tools/performance/scripts/post-to-codevitals.js @@ -2,27 +2,45 @@ import fs from 'fs'; import path from 'path'; -import { SCENARIOS } from './scenarios.js'; - -/** Extract metrics for a single scenario. */ +import { SCENARIOS, SANITY_RANGES } from './scenarios.js'; + +/** + * Extract metric entries for a single scenario. + * + * Each entry carries its CodeVitals key, value, and (optional) type. The type + * drives the sanity-range check; untyped legacy entries are posted unchecked. + */ function extractScenarioMetrics( scenario, summary ) { - const metrics = {}; - // Use explicit metricKey if defined, otherwise fall back to prefix-based keys if ( scenario.metricKey ) { // Single metric with exact key - metrics[ scenario.metricKey ] = summary.median; - } else { - // Legacy: prefix-based keys with suffixes - const prefix = scenario.metricPrefix; - metrics[ `${ prefix }_ms` ] = summary.median; - metrics[ `${ prefix }_mean_ms` ] = summary.mean; - metrics[ `${ prefix }_min_ms` ] = summary.min; - metrics[ `${ prefix }_max_ms` ] = summary.max; - metrics[ `${ prefix }_stddev_ms` ] = summary.stdDev; + return [ { key: scenario.metricKey, value: summary.median, type: scenario.metricType } ]; } - return metrics; + // Legacy: prefix-based keys with suffixes (untyped — not range-checked) + const prefix = scenario.metricPrefix; + return [ + { key: `${ prefix }_ms`, value: summary.median }, + { key: `${ prefix }_mean_ms`, value: summary.mean }, + { key: `${ prefix }_min_ms`, value: summary.min }, + { key: `${ prefix }_max_ms`, value: summary.max }, + { key: `${ prefix }_stddev_ms`, value: summary.stdDev }, + ]; +} + +/** + * Check a metric value against its sanity range. + * + * Returns true when the value is safe to post: either the type has no range + * defined, or the value falls within it. CodeVitals is append-only, so an + * out-of-range value must never reach it. + */ +function isWithinSanityRange( type, value ) { + const range = type && SANITY_RANGES[ type ]; + if ( ! range ) { + return true; + } + return value >= range.min && value <= range.max; } /** Post metrics to CodeVitals. */ @@ -34,8 +52,9 @@ async function postToCodeVitals( resultsPath, config ) { const results = JSON.parse( fs.readFileSync( resultsPath, 'utf8' ) ); - // Extract metrics from results + // Extract and sanity-check metrics from results const metrics = {}; + let validationFailed = false; // Process only scenarios marked for CodeVitals posting for ( const scenario of SCENARIOS ) { @@ -50,13 +69,26 @@ async function postToCodeVitals( resultsPath, config ) { continue; } - const scenarioMetrics = extractScenarioMetrics( scenario, measurement.summary ); - - Object.assign( metrics, scenarioMetrics ); + for ( const entry of extractScenarioMetrics( scenario, measurement.summary ) ) { + if ( ! isWithinSanityRange( entry.type, entry.value ) ) { + const range = SANITY_RANGES[ entry.type ]; + console.error( + `✗ Sanity check failed for "${ entry.key }" (${ entry.type }): ${ entry.value } ` + + `is outside [${ range.min }, ${ range.max }] — skipping this metric.` + ); + validationFailed = true; + continue; + } + metrics[ entry.key ] = entry.value; + } } - // Validate we have metrics to post + // Nothing valid left to post. if ( Object.keys( metrics ).length === 0 ) { + if ( validationFailed ) { + // Every metric was skipped by a sanity check (failures already logged). + return { posted: false, validationFailed }; + } throw new Error( 'No metrics to post - check scenario configuration and measurement results' ); } @@ -69,6 +101,13 @@ async function postToCodeVitals( resultsPath, config ) { branch: results.git?.branch || config.gitBranch || 'trunk', }; + // Dry run: show exactly what would be posted, then stop short of the POST. + if ( config.dryRun ) { + console.log( '— DRY RUN — building payload only, not posting to CodeVitals —' ); + console.log( JSON.stringify( payload, null, 2 ) ); + return { posted: false, validationFailed }; + } + console.log( 'Posting metrics to CodeVitals...' ); console.log( 'Metrics:', JSON.stringify( metrics, null, 2 ) ); @@ -105,7 +144,7 @@ async function postToCodeVitals( resultsPath, config ) { } ); } console.log( '✓ Metrics posted successfully to CodeVitals' ); - return data; + return { posted: true, data, validationFailed }; } catch ( error ) { clearTimeout( timeoutId ); const message = @@ -121,6 +160,8 @@ async function main() { console.log( 'CodeVitals Integration' ); console.log( '=====================\n' ); + const dryRun = process.argv.includes( '--dry-run' ); + // Configuration from environment const config = { codeVitalsUrl: process.env.CODEVITALS_URL || 'https://www.codevitals.run', @@ -129,15 +170,17 @@ async function main() { gitBranch: process.env.GIT_BRANCH || 'trunk', resultsPath: process.env.RESULTS_PATH || path.join( import.meta.dirname, '../results/lcp-results.json' ), + dryRun, }; - // Validate required config - if ( ! config.codeVitalsToken ) { + // A live post needs a token; a dry run does not, so CI can smoke-test it. + if ( ! dryRun && ! config.codeVitalsToken ) { console.error( 'ERROR: CODEVITALS_TOKEN environment variable is required' ); process.exit( 1 ); } console.log( 'Configuration:' ); + console.log( ` Mode: ${ dryRun ? 'DRY RUN (no POST)' : 'live post' }` ); console.log( ` CodeVitals URL: ${ config.codeVitalsUrl }` ); console.log( ` Results Path: ${ config.resultsPath }` ); console.log( ` Git Hash: ${ config.gitHash || 'unknown' }` ); @@ -145,8 +188,14 @@ async function main() { console.log( '' ); try { - await postToCodeVitals( config.resultsPath, config ); - console.log( '\n✓ All done!' ); + const result = await postToCodeVitals( config.resultsPath, config ); + if ( result.validationFailed ) { + console.error( + '\n✗ One or more metrics failed sanity checks (see above). Any valid metrics were still processed.' + ); + process.exit( 1 ); + } + console.log( dryRun ? '\n✓ Dry run complete!' : '\n✓ All done!' ); process.exit( 0 ); } catch ( error ) { console.error( '\n✗ Failed:', error.message ); diff --git a/tools/performance/scripts/scenarios.js b/tools/performance/scripts/scenarios.js index 1457cc6dce86..0814f1d7503c 100644 --- a/tools/performance/scripts/scenarios.js +++ b/tools/performance/scripts/scenarios.js @@ -23,12 +23,36 @@ export const SCENARIOS = [ defaultUrl: 'http://localhost:8083', header: 'Jetpack Connected (Simulated + 200ms Latency)', metricPrefix: 'wp_admin_lcp_jetpack_connected', + // CodeVitals key for the posted metric. When introducing a NEW metric, post + // it to a `-staging` key first (e.g. `…-timeToFirstByte-staging`) for 2-3 + // builds, inspect it in the CodeVitals UI, then rename to the production key. + // See the "Safeguards" section of README.md for the full convention. metricKey: 'wp-admin-dashboard-connection-sim-largestContentfulPaint', + // Metric type — drives the sanity-range check in post-to-codevitals.js. + metricType: 'lcp', postToCodeVitals: true, isBaseline: false, }, ]; +/** + * Sanity ranges for posted metrics, keyed by metric type. + * + * post-to-codevitals.js checks every typed metric against these bounds before + * posting. A value outside its range is logged and skipped (never posted), + * because CodeVitals is append-only and bad points cannot be rolled back. + * Add a row when a new metric type starts being posted. + * + * @type {Object} + */ +export const SANITY_RANGES = { + lcp: { min: 100, max: 60000 }, // <100ms is suspicious; >60s means the page never loaded. + ttfb: { min: 10, max: 10000 }, // <10ms is unrealistic; >10s means server failure. + fcp: { min: 50, max: 30000 }, + tbt: { min: 0, max: 10000 }, // Can legitimately be 0; >10s is catastrophic. + cls: { min: 0, max: 5 }, // >5 would mean the page is unusable. +}; + /** * Get the URL for a scenario from environment or default * From 06f0dd3ad6d443eabc17a7219592cd2ca65148e4 Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Fri, 26 Jun 2026 10:56:17 +0100 Subject: [PATCH 2/9] Performance: default CodeVitals POST to the apex host www.codevitals.run 301-redirects the API to the apex. On a 301 fetch retries a POST as a GET with no body, so a metric posted to the www default would never land. Default CODEVITALS_URL to https://codevitals.run. --- tools/performance/scripts/post-to-codevitals.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tools/performance/scripts/post-to-codevitals.js b/tools/performance/scripts/post-to-codevitals.js index 9ac4abae8c4b..acf6b0003caa 100644 --- a/tools/performance/scripts/post-to-codevitals.js +++ b/tools/performance/scripts/post-to-codevitals.js @@ -164,7 +164,9 @@ async function main() { // Configuration from environment const config = { - codeVitalsUrl: process.env.CODEVITALS_URL || 'https://www.codevitals.run', + // Default to the apex host. www.codevitals.run 301-redirects the API, and on a + // 301 fetch retries a POST as a GET with no body, so the metric never lands. + codeVitalsUrl: process.env.CODEVITALS_URL || 'https://codevitals.run', codeVitalsToken: process.env.CODEVITALS_TOKEN, gitHash: process.env.GIT_COMMIT, gitBranch: process.env.GIT_BRANCH || 'trunk', From 4587f3ea517d54d30d5c0d8bda4c16cba8cc8fc3 Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Fri, 26 Jun 2026 12:15:27 +0100 Subject: [PATCH 3/9] Performance: fail the CodeVitals sanity guard closed + add unit coverage The guard returned true for any metricType absent from SANITY_RANGES (a typo, a forgotten row, or the legacy untyped path), and coercion let null pass min-0 ranges and let numeric strings through as strings. Both post unchecked to an append-only store. checkSanityRange now rejects a typed metric with no range row and any non-finite value, and only a genuinely untyped legacy entry passes unchecked. Guard main() behind an import.meta.url check so the pure helpers can be imported, and add node:test coverage (pnpm test:unit) pinning the fail-closed contract: over-range skipped, non-finite/string/typo rejected, boundaries inclusive, untyped legacy passes. --- tools/performance/package.json | 3 +- .../performance/scripts/post-to-codevitals.js | 51 ++++++++++---- .../scripts/post-to-codevitals.test.js | 68 +++++++++++++++++++ 3 files changed, 106 insertions(+), 16 deletions(-) create mode 100644 tools/performance/scripts/post-to-codevitals.test.js diff --git a/tools/performance/package.json b/tools/performance/package.json index 1d6669e83efa..02d9a1a4ea7d 100644 --- a/tools/performance/package.json +++ b/tools/performance/package.json @@ -16,7 +16,8 @@ "report:dry": "node scripts/post-to-codevitals.js --dry-run", "setup:browsers": "playwright install chromium", "test": "node scripts/run-performance-tests.js", - "test:quick": "ITERATIONS=2 node scripts/run-performance-tests.js" + "test:quick": "ITERATIONS=2 node scripts/run-performance-tests.js", + "test:unit": "node --test" }, "dependencies": { "dotenv": "^16.3.1", diff --git a/tools/performance/scripts/post-to-codevitals.js b/tools/performance/scripts/post-to-codevitals.js index acf6b0003caa..a9580511f840 100644 --- a/tools/performance/scripts/post-to-codevitals.js +++ b/tools/performance/scripts/post-to-codevitals.js @@ -29,18 +29,37 @@ function extractScenarioMetrics( scenario, summary ) { } /** - * Check a metric value against its sanity range. + * Check whether a metric value is safe to post to CodeVitals. * - * Returns true when the value is safe to post: either the type has no range - * defined, or the value falls within it. CodeVitals is append-only, so an - * out-of-range value must never reach it. + * CodeVitals is append-only, so anything uncertain fails closed. A typed metric + * whose type has no range row (a typo, or a forgotten SANITY_RANGES entry) and a + * non-finite value (null, NaN, Infinity, a numeric string) are both rejected + * rather than posted. Only a genuinely untyped legacy entry passes unchecked. + * + * @param {string|undefined} type - Metric type, or falsy for an untyped legacy entry. + * @param {*} value - Candidate metric value. + * @return {{ ok: boolean, reason?: string }} Whether the value may be posted, and why not. */ -function isWithinSanityRange( type, value ) { - const range = type && SANITY_RANGES[ type ]; +function checkSanityRange( type, value ) { + // Genuinely untyped legacy entry: no declared type, so no range to enforce. + if ( ! type ) { + return { ok: true }; + } + + const range = SANITY_RANGES[ type ]; if ( ! range ) { - return true; + return { ok: false, reason: `no sanity range is defined for type "${ type }"` }; + } + + if ( typeof value !== 'number' || ! Number.isFinite( value ) ) { + return { ok: false, reason: `value ${ JSON.stringify( value ) } is not a finite number` }; } - return value >= range.min && value <= range.max; + + if ( value < range.min || value > range.max ) { + return { ok: false, reason: `${ value } is outside [${ range.min }, ${ range.max }]` }; + } + + return { ok: true }; } /** Post metrics to CodeVitals. */ @@ -70,11 +89,10 @@ async function postToCodeVitals( resultsPath, config ) { } for ( const entry of extractScenarioMetrics( scenario, measurement.summary ) ) { - if ( ! isWithinSanityRange( entry.type, entry.value ) ) { - const range = SANITY_RANGES[ entry.type ]; + const check = checkSanityRange( entry.type, entry.value ); + if ( ! check.ok ) { console.error( - `✗ Sanity check failed for "${ entry.key }" (${ entry.type }): ${ entry.value } ` + - `is outside [${ range.min }, ${ range.max }] — skipping this metric.` + `✗ Sanity check failed for "${ entry.key }" (${ entry.type }): ${ check.reason }. Skipping this metric.` ); validationFailed = true; continue; @@ -205,7 +223,10 @@ async function main() { } } -// Run if called directly -main(); +// Run only when executed directly, not when imported (e.g. by the unit test), +// so importing the pure helpers does not trigger main()'s env checks or exits. +if ( import.meta.url === `file://${ process.argv[ 1 ] }` ) { + main(); +} -export { postToCodeVitals }; +export { postToCodeVitals, checkSanityRange, extractScenarioMetrics }; diff --git a/tools/performance/scripts/post-to-codevitals.test.js b/tools/performance/scripts/post-to-codevitals.test.js new file mode 100644 index 000000000000..01b740cfd823 --- /dev/null +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -0,0 +1,68 @@ +/** + * Unit tests for the CodeVitals sanity guard. + * + * CodeVitals is append-only with no rollback, so these tests pin the one + * contract that keeps bad data out: checkSanityRange must fail closed on + * anything it cannot positively confirm is in range. Run with `pnpm test:unit` + * (node's built-in runner — no Docker, no token, no network). + */ + +import assert from 'node:assert/strict'; +import { test } from 'node:test'; +import { checkSanityRange, extractScenarioMetrics } from './post-to-codevitals.js'; + +test( 'in-range typed value passes', () => { + assert.equal( checkSanityRange( 'lcp', 120 ).ok, true ); +} ); + +test( 'range boundaries are inclusive', () => { + assert.equal( checkSanityRange( 'lcp', 100 ).ok, true ); // min + assert.equal( checkSanityRange( 'lcp', 60000 ).ok, true ); // max +} ); + +test( 'out-of-range value is rejected', () => { + assert.equal( checkSanityRange( 'lcp', 99 ).ok, false ); // below min + assert.equal( checkSanityRange( 'lcp', 60001 ).ok, false ); // above max +} ); + +test( 'a typed metric with no range row fails closed (typo / forgotten row)', () => { + assert.equal( checkSanityRange( 'lcpp', 120 ).ok, false ); // typo + assert.equal( checkSanityRange( 'LCP', 120 ).ok, false ); // case mismatch, lookup is exact +} ); + +test( 'non-finite values are rejected, including on min-0 ranges', () => { + assert.equal( checkSanityRange( 'tbt', null ).ok, false ); // null coerces to 0; must not pass + assert.equal( checkSanityRange( 'cls', null ).ok, false ); + assert.equal( checkSanityRange( 'lcp', NaN ).ok, false ); + assert.equal( checkSanityRange( 'lcp', Infinity ).ok, false ); + assert.equal( checkSanityRange( 'lcp', undefined ).ok, false ); +} ); + +test( 'numeric strings are rejected rather than posted as strings', () => { + assert.equal( checkSanityRange( 'lcp', '120' ).ok, false ); +} ); + +test( 'a genuinely untyped legacy entry passes unchecked', () => { + assert.equal( checkSanityRange( undefined, 999999 ).ok, true ); +} ); + +test( 'explicit metricKey yields one typed entry', () => { + const entries = extractScenarioMetrics( + { metricKey: 'wp-admin-lcp', metricType: 'lcp' }, + { median: 120 } + ); + assert.deepEqual( entries, [ { key: 'wp-admin-lcp', value: 120, type: 'lcp' } ] ); +} ); + +test( 'legacy prefix yields five untyped entries', () => { + const entries = extractScenarioMetrics( + { metricPrefix: 'bar' }, + { median: 1, mean: 2, min: 3, max: 4, stdDev: 5 } + ); + assert.equal( entries.length, 5 ); + assert.deepEqual( entries[ 0 ], { key: 'bar_ms', value: 1 } ); + assert.equal( + entries.every( e => e.type === undefined ), + true + ); +} ); From d4a0a21aa667a0da15069cf7a24c597facc0f748 Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Fri, 26 Jun 2026 12:48:09 +0100 Subject: [PATCH 4/9] Performance: fix direct-run guard, gate unit tests, broaden coverage The round-1 import guard compared import.meta.url against a raw file://${argv[1]} string. Node percent-encodes and symlink-resolves import.meta.url but argv[1] stays raw, so on a checkout whose path contains a space or non-ASCII char (or via /tmp -> /private/tmp), the match failed and main() silently never ran: the CLI exited 0 having posted nothing. Replace it with isDirectInvocation(), which compares realpath'd filesystem paths. Also: - Reject non-finite values for every entry, typed or untyped, by moving the finite check above the untyped early return (never post null/NaN). - Gate the unit tests on the integrated path: pnpm test now runs node --test before the perf runner, so the guard is enforced wherever this tool runs (tools/performance is outside the monorepo CI matrix). - Add integration coverage for postToCodeVitals (in-range posts, out-of-range skipped + validationFailed, missing file throws) and a CLI test that runs the script from a path with a space (regression guard for the bug above). Dry-run now returns the built payload so the integration test can assert it. - Point the README default and the perf runner's result link at the apex host, matching the POST default. --- tools/performance/README.md | 2 +- tools/performance/package.json | 2 +- .../performance/scripts/post-to-codevitals.js | 43 ++++-- .../scripts/post-to-codevitals.test.js | 130 +++++++++++++++++- .../scripts/run-performance-tests.js | 4 +- 5 files changed, 162 insertions(+), 19 deletions(-) diff --git a/tools/performance/README.md b/tools/performance/README.md index 7fb02597d359..f9828ef7fe7b 100644 --- a/tools/performance/README.md +++ b/tools/performance/README.md @@ -19,7 +19,7 @@ The test suite is designed to run in TeamCity. See `TEAMCITY-SETUP.md` for detai | Variable | Description | |----------|-------------| | `CODEVITALS_TOKEN` | API token for posting results to CodeVitals | -| `CODEVITALS_URL` | CodeVitals API URL (default: https://www.codevitals.run) | +| `CODEVITALS_URL` | CodeVitals API URL (default: https://codevitals.run). Use the apex host, not `www.`: the `www.` host 301-redirects the API and the redirect drops the POST body. | | `COMPOSE_PROJECT_NAME` | Unique Docker project name for build isolation | | `GIT_COMMIT` | Git commit SHA for tracking (auto-detected from plugin) | | `GIT_BRANCH` | Git branch for tracking (default: trunk) | diff --git a/tools/performance/package.json b/tools/performance/package.json index 02d9a1a4ea7d..02e716d12c18 100644 --- a/tools/performance/package.json +++ b/tools/performance/package.json @@ -15,7 +15,7 @@ "report": "node scripts/post-to-codevitals.js", "report:dry": "node scripts/post-to-codevitals.js --dry-run", "setup:browsers": "playwright install chromium", - "test": "node scripts/run-performance-tests.js", + "test": "node --test && node scripts/run-performance-tests.js", "test:quick": "ITERATIONS=2 node scripts/run-performance-tests.js", "test:unit": "node --test" }, diff --git a/tools/performance/scripts/post-to-codevitals.js b/tools/performance/scripts/post-to-codevitals.js index a9580511f840..2a5b22d92d1f 100644 --- a/tools/performance/scripts/post-to-codevitals.js +++ b/tools/performance/scripts/post-to-codevitals.js @@ -41,6 +41,13 @@ function extractScenarioMetrics( scenario, summary ) { * @return {{ ok: boolean, reason?: string }} Whether the value may be posted, and why not. */ function checkSanityRange( type, value ) { + // Never post a non-finite value, regardless of type. null, NaN, Infinity, and + // numeric strings are always wrong for an append-only store, so this check + // comes before the untyped early return below. + if ( typeof value !== 'number' || ! Number.isFinite( value ) ) { + return { ok: false, reason: `value ${ JSON.stringify( value ) } is not a finite number` }; + } + // Genuinely untyped legacy entry: no declared type, so no range to enforce. if ( ! type ) { return { ok: true }; @@ -51,10 +58,6 @@ function checkSanityRange( type, value ) { return { ok: false, reason: `no sanity range is defined for type "${ type }"` }; } - if ( typeof value !== 'number' || ! Number.isFinite( value ) ) { - return { ok: false, reason: `value ${ JSON.stringify( value ) } is not a finite number` }; - } - if ( value < range.min || value > range.max ) { return { ok: false, reason: `${ value } is outside [${ range.min }, ${ range.max }]` }; } @@ -123,7 +126,7 @@ async function postToCodeVitals( resultsPath, config ) { if ( config.dryRun ) { console.log( '— DRY RUN — building payload only, not posting to CodeVitals —' ); console.log( JSON.stringify( payload, null, 2 ) ); - return { posted: false, validationFailed }; + return { posted: false, validationFailed, payload }; } console.log( 'Posting metrics to CodeVitals...' ); @@ -223,10 +226,34 @@ async function main() { } } -// Run only when executed directly, not when imported (e.g. by the unit test), +/** + * Whether this module was run directly (`node post-to-codevitals.js`) rather than imported. + * + * Compares real filesystem paths so spaces, non-ASCII characters, and symlinks + * (e.g. /tmp → /private/tmp) cannot make the match fail and silently skip main(). + * A raw ``file://${ process.argv[1] }`` comparison breaks on all three: Node + * percent-encodes and symlink-resolves import.meta.url but argv[1] stays raw. + * + * @param {string|undefined} moduleFilename - Absolute path of this module (import.meta.filename). + * @param {string|undefined} invokedPath - The path Node was invoked with (process.argv[1]). + * @return {boolean} True when both resolve to the same file. + */ +function isDirectInvocation( moduleFilename, invokedPath ) { + if ( ! moduleFilename || ! invokedPath ) { + return false; + } + try { + return fs.realpathSync( moduleFilename ) === fs.realpathSync( invokedPath ); + } catch { + // realpathSync throws when invokedPath does not exist (e.g. `node --test`). + return false; + } +} + +// Run only when executed directly, not when imported (e.g. by the unit tests), // so importing the pure helpers does not trigger main()'s env checks or exits. -if ( import.meta.url === `file://${ process.argv[ 1 ] }` ) { +if ( isDirectInvocation( import.meta.filename, process.argv[ 1 ] ) ) { main(); } -export { postToCodeVitals, checkSanityRange, extractScenarioMetrics }; +export { postToCodeVitals, checkSanityRange, extractScenarioMetrics, isDirectInvocation }; diff --git a/tools/performance/scripts/post-to-codevitals.test.js b/tools/performance/scripts/post-to-codevitals.test.js index 01b740cfd823..577d5bb9c29a 100644 --- a/tools/performance/scripts/post-to-codevitals.test.js +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -1,15 +1,59 @@ /** - * Unit tests for the CodeVitals sanity guard. + * Unit and integration tests for the CodeVitals posting tool. * - * CodeVitals is append-only with no rollback, so these tests pin the one - * contract that keeps bad data out: checkSanityRange must fail closed on - * anything it cannot positively confirm is in range. Run with `pnpm test:unit` + * CodeVitals is append-only with no rollback, so these tests pin the contract + * that keeps bad data out: checkSanityRange must fail closed on anything it + * cannot positively confirm is in range, postToCodeVitals must not let a + * rejected value into the payload, and the CLI must actually run when invoked + * directly (even from a path with a space). Run with `pnpm test:unit` * (node's built-in runner — no Docker, no token, no network). */ import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; import { test } from 'node:test'; -import { checkSanityRange, extractScenarioMetrics } from './post-to-codevitals.js'; +import { fileURLToPath } from 'node:url'; +import { + checkSanityRange, + extractScenarioMetrics, + isDirectInvocation, + postToCodeVitals, +} from './post-to-codevitals.js'; + +const SCRIPTS_DIR = path.dirname( fileURLToPath( import.meta.url ) ); +const LCP_KEY = 'wp-admin-dashboard-connection-sim-largestContentfulPaint'; + +/** Write a results fixture with the given median LCP and return its path. */ +function writeResults( median ) { + const dir = fs.mkdtempSync( path.join( os.tmpdir(), 'cv-results-' ) ); + const file = path.join( dir, 'results.json' ); + fs.writeFileSync( + file, + JSON.stringify( { + git: { hash: 'testhash', branch: 'trunk' }, + measurements: { jetpackConnected: { summary: { median } } }, + } ) + ); + return file; +} + +/** Run a function with console output suppressed to keep test output readable. */ +async function silenced( fn ) { + const orig = { log: console.log, error: console.error, warn: console.warn }; + console.log = () => {}; + console.error = () => {}; + console.warn = () => {}; + try { + return await fn(); + } finally { + Object.assign( console, orig ); + } +} + +// --- checkSanityRange (the guard) --- test( 'in-range typed value passes', () => { assert.equal( checkSanityRange( 'lcp', 120 ).ok, true ); @@ -42,10 +86,17 @@ test( 'numeric strings are rejected rather than posted as strings', () => { assert.equal( checkSanityRange( 'lcp', '120' ).ok, false ); } ); -test( 'a genuinely untyped legacy entry passes unchecked', () => { +test( 'a finite untyped legacy entry passes unchecked', () => { assert.equal( checkSanityRange( undefined, 999999 ).ok, true ); } ); +test( 'a non-finite value is rejected even for an untyped legacy entry', () => { + assert.equal( checkSanityRange( undefined, null ).ok, false ); + assert.equal( checkSanityRange( undefined, NaN ).ok, false ); +} ); + +// --- extractScenarioMetrics --- + test( 'explicit metricKey yields one typed entry', () => { const entries = extractScenarioMetrics( { metricKey: 'wp-admin-lcp', metricType: 'lcp' }, @@ -66,3 +117,70 @@ test( 'legacy prefix yields five untyped entries', () => { true ); } ); + +// --- postToCodeVitals (the accumulation loop + exit semantics) --- + +test( 'dry-run posts an in-range metric into the payload, nothing rejected', async () => { + const file = writeResults( 120 ); + const result = await silenced( () => postToCodeVitals( file, { dryRun: true } ) ); + assert.equal( result.posted, false ); // dry run never posts + assert.equal( result.validationFailed, false ); + assert.equal( result.payload.metrics[ LCP_KEY ], 120 ); +} ); + +test( 'an out-of-range metric is skipped and flags validationFailed', async () => { + const file = writeResults( 70000 ); + const result = await silenced( () => postToCodeVitals( file, { dryRun: true } ) ); + assert.equal( result.posted, false ); + assert.equal( result.validationFailed, true ); // the only metric was rejected +} ); + +test( 'a non-finite metric is rejected through the posting path', async () => { + const file = writeResults( null ); + const result = await silenced( () => postToCodeVitals( file, { dryRun: true } ) ); + assert.equal( result.validationFailed, true ); +} ); + +test( 'a missing results file throws', async () => { + await assert.rejects( + () => silenced( () => postToCodeVitals( '/no/such/results.json', { dryRun: true } ) ), + /Results file not found/ + ); +} ); + +// --- isDirectInvocation (the run-when-direct guard) --- + +test( 'isDirectInvocation: a file matches itself', () => { + const real = fileURLToPath( import.meta.url ); + assert.equal( isDirectInvocation( real, real ), true ); +} ); + +test( 'isDirectInvocation: a non-existent invocation path is not a direct run', () => { + assert.equal( isDirectInvocation( fileURLToPath( import.meta.url ), '/no/such/path' ), false ); +} ); + +test( 'isDirectInvocation: a missing argv is not a direct run', () => { + assert.equal( isDirectInvocation( fileURLToPath( import.meta.url ), undefined ), false ); +} ); + +// --- CLI end to end (R2-C regression: must run from a path with a space) --- + +test( 'the CLI runs main() when invoked directly from a path containing a space', () => { + const dir = fs.mkdtempSync( path.join( os.tmpdir(), 'cv has space-' ) ); + fs.copyFileSync( + path.join( SCRIPTS_DIR, 'post-to-codevitals.js' ), + path.join( dir, 'post-to-codevitals.js' ) + ); + fs.copyFileSync( path.join( SCRIPTS_DIR, 'scenarios.js' ), path.join( dir, 'scenarios.js' ) ); + const results = writeResults( 120 ); + try { + const out = execFileSync( 'node', [ path.join( dir, 'post-to-codevitals.js' ), '--dry-run' ], { + encoding: 'utf8', + env: { ...process.env, RESULTS_PATH: results }, + } ); + assert.match( out, /DRY RUN/ ); // empty output would mean main() was skipped + assert.match( out, new RegExp( LCP_KEY ) ); + } finally { + fs.rmSync( dir, { recursive: true, force: true } ); + } +} ); diff --git a/tools/performance/scripts/run-performance-tests.js b/tools/performance/scripts/run-performance-tests.js index 1a0216e4b81a..b75adcf460da 100644 --- a/tools/performance/scripts/run-performance-tests.js +++ b/tools/performance/scripts/run-performance-tests.js @@ -482,9 +482,7 @@ async function main() { if ( process.env.CODEVITALS_TOKEN ) { console.log( 'View detailed results in CodeVitals:' ); - console.log( - ` ${ process.env.CODEVITALS_URL || 'https://www.codevitals.run' }/project/jetpack` - ); + console.log( ` ${ process.env.CODEVITALS_URL || 'https://codevitals.run' }/project/jetpack` ); console.log( '' ); } } From 1b6a50bfd4eb30635728c948f6dccdab302f5f9e Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Fri, 26 Jun 2026 13:16:43 +0100 Subject: [PATCH 5/9] Performance: keep the CodeVitals token out of logs and errors Build the request URL with new URL() before attaching the token, so a malformed CODEVITALS_URL throws a generic error instead of a parse error that echoes the secret, and scrub token=... from any caught error before logging or rethrowing. Add live-POST tests (fetch stubbed) covering the payload, a non-OK response, and a malformed-URL redaction regression. --- .../performance/scripts/post-to-codevitals.js | 46 ++++++++- .../scripts/post-to-codevitals.test.js | 94 +++++++++++++++++++ 2 files changed, 135 insertions(+), 5 deletions(-) diff --git a/tools/performance/scripts/post-to-codevitals.js b/tools/performance/scripts/post-to-codevitals.js index 2a5b22d92d1f..79f1b9c4ce5a 100644 --- a/tools/performance/scripts/post-to-codevitals.js +++ b/tools/performance/scripts/post-to-codevitals.js @@ -65,6 +65,25 @@ function checkSanityRange( type, value ) { return { ok: true }; } +/** + * Strip the CodeVitals token from a string so it can never reach a log or a + * re-thrown error. fetch and the URL parser echo the full request URL (token + * included) in their messages, and the token grants append access to an + * unrollback-able store, so scrub it before anything prints. + * + * @param {string} text - Text that may contain the token or a `token=...` query param. + * @param {string} [token] - The exact token value to strip, when known. + * @return {string} The text with the token replaced by `REDACTED`. + */ +function redactToken( text, token ) { + let safe = String( text ); + if ( token ) { + safe = safe.split( token ).join( 'REDACTED' ); + } + // Catch the query-param form too, in case the exact value isn't in hand here. + return safe.replace( /token=[^&\s]+/g, 'token=REDACTED' ); +} + /** Post metrics to CodeVitals. */ async function postToCodeVitals( resultsPath, config ) { // Read results file @@ -132,10 +151,18 @@ async function postToCodeVitals( resultsPath, config ) { console.log( 'Posting metrics to CodeVitals...' ); console.log( 'Metrics:', JSON.stringify( metrics, null, 2 ) ); - // Token passed as query param per CodeVitals API spec (don't log URL) - const url = `${ config.codeVitalsUrl }/api/log?token=${ config.codeVitalsToken }`; - const TIMEOUT_MS = 30000; // 30 second timeout + // Build the URL before attaching the token, so a malformed base host can't + // throw a parse error that echoes the secret. The token lives only on the URL + // object (via searchParams), never in a string we build or log. + let url; + try { + url = new URL( '/api/log', config.codeVitalsUrl ); + } catch { + throw new Error( 'Invalid CodeVitals URL; check the CODEVITALS_URL setting' ); + } + url.searchParams.set( 'token', config.codeVitalsToken ); + const TIMEOUT_MS = 30000; // 30 second timeout const controller = new AbortController(); const timeoutId = setTimeout( () => controller.abort(), TIMEOUT_MS ); @@ -168,10 +195,13 @@ async function postToCodeVitals( resultsPath, config ) { return { posted: true, data, validationFailed }; } catch ( error ) { clearTimeout( timeoutId ); + // Redact the token from any error before it reaches a log or a re-throw. + // Safe URL construction already keeps the secret out of parse errors; this + // is defense in depth so no upstream message carries it into a build log. const message = error.name === 'AbortError' ? `CodeVitals request timed out after ${ TIMEOUT_MS / 1000 }s` - : error.message; + : redactToken( error.message, config.codeVitalsToken ); console.error( '✗ Failed to post metrics to CodeVitals:', message ); throw new Error( message, { cause: error } ); } @@ -256,4 +286,10 @@ if ( isDirectInvocation( import.meta.filename, process.argv[ 1 ] ) ) { main(); } -export { postToCodeVitals, checkSanityRange, extractScenarioMetrics, isDirectInvocation }; +export { + postToCodeVitals, + checkSanityRange, + extractScenarioMetrics, + isDirectInvocation, + redactToken, +}; diff --git a/tools/performance/scripts/post-to-codevitals.test.js b/tools/performance/scripts/post-to-codevitals.test.js index 577d5bb9c29a..1004aaf12421 100644 --- a/tools/performance/scripts/post-to-codevitals.test.js +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -21,6 +21,7 @@ import { extractScenarioMetrics, isDirectInvocation, postToCodeVitals, + redactToken, } from './post-to-codevitals.js'; const SCRIPTS_DIR = path.dirname( fileURLToPath( import.meta.url ) ); @@ -118,6 +119,20 @@ test( 'legacy prefix yields five untyped entries', () => { ); } ); +// --- redactToken (keeps the token out of logs and errors) --- + +test( 'redactToken strips the exact token and any token query param', () => { + assert.equal( + redactToken( 'prefix token=abc123 suffix', 'abc123' ), + 'prefix token=REDACTED suffix' + ); + assert.equal( + redactToken( 'https://h/api/log?token=zzz&n=1' ), + 'https://h/api/log?token=REDACTED&n=1' + ); + assert.equal( redactToken( 'no secret in here', 'abc123' ), 'no secret in here' ); +} ); + // --- postToCodeVitals (the accumulation loop + exit semantics) --- test( 'dry-run posts an in-range metric into the payload, nothing rejected', async () => { @@ -148,6 +163,85 @@ test( 'a missing results file throws', async () => { ); } ); +// --- live POST branch (fetch stubbed; never touches the network) --- + +test( 'a live post sends the payload as POST and returns posted:true', async () => { + const file = writeResults( 120 ); + const origFetch = global.fetch; + let sentUrl, sentInit; + global.fetch = async ( u, init ) => { + sentUrl = u; + sentInit = init; + return { ok: true, status: 200, json: async () => ( { ok: true } ), text: async () => '' }; + }; + try { + const result = await silenced( () => + postToCodeVitals( file, { + dryRun: false, + codeVitalsUrl: 'https://codevitals.test', + codeVitalsToken: 'tok-success', + } ) + ); + assert.equal( result.posted, true ); + assert.equal( sentInit.method, 'POST' ); + assert.match( String( sentUrl ), /\/api\/log\?token=tok-success$/ ); + assert.equal( JSON.parse( sentInit.body ).metrics[ LCP_KEY ], 120 ); + } finally { + global.fetch = origFetch; + } +} ); + +test( 'a non-OK CodeVitals response throws without leaking the token', async () => { + const file = writeResults( 120 ); + const origFetch = global.fetch; + global.fetch = async () => ( { ok: false, status: 500, text: async () => 'upstream boom' } ); + try { + await assert.rejects( + () => + silenced( () => + postToCodeVitals( file, { + dryRun: false, + codeVitalsUrl: 'https://codevitals.test', + codeVitalsToken: 'tok-secret-500', + } ) + ), + err => { + assert.match( err.message, /CodeVitals API error \(500\)/ ); + assert.ok( ! err.message.includes( 'tok-secret-500' ) ); + return true; + } + ); + } finally { + global.fetch = origFetch; + } +} ); + +test( 'a malformed CODEVITALS_URL leaks the token into neither the error nor the logs', async () => { + const file = writeResults( 120 ); + const captured = []; + const orig = { log: console.log, error: console.error, warn: console.warn }; + console.log = () => {}; + console.warn = () => {}; + console.error = ( ...a ) => captured.push( a.join( ' ' ) ); + let thrown = ''; + try { + await postToCodeVitals( file, { + dryRun: false, + codeVitalsUrl: 'http://[::1', // malformed on purpose + codeVitalsToken: 'tok-must-not-leak', + } ); + } catch ( e ) { + thrown = e.message; + } finally { + Object.assign( console, orig ); + } + assert.ok( ! thrown.includes( 'tok-must-not-leak' ), 'token must not be in the thrown message' ); + assert.ok( + ! captured.join( '\n' ).includes( 'tok-must-not-leak' ), + 'token must not be in console output' + ); +} ); + // --- isDirectInvocation (the run-when-direct guard) --- test( 'isDirectInvocation: a file matches itself', () => { From bf53477652e2fb4ebf65f36a507a0c6a9e48e4dc Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Fri, 26 Jun 2026 15:01:50 +0100 Subject: [PATCH 6/9] Performance: scrub the CodeVitals token from error cause chains Redacting only the top-level error.message left the token in err.cause and util.inspect(err) when an upstream fetch error echoed the URL. Walk the caught error's whole cause chain and scrub it in place before logging or rethrowing, so the full error object is token-free. Also make the CLI test fixture explicitly ESM so it runs across the supported Node range, and document that CODEVITALS_URL must be origin-only. --- tools/performance/README.md | 44 +++++++++---------- .../performance/scripts/post-to-codevitals.js | 36 +++++++++++++-- .../scripts/post-to-codevitals.test.js | 41 +++++++++++++++++ 3 files changed, 95 insertions(+), 26 deletions(-) diff --git a/tools/performance/README.md b/tools/performance/README.md index f9828ef7fe7b..71eeb30146d6 100644 --- a/tools/performance/README.md +++ b/tools/performance/README.md @@ -16,16 +16,16 @@ The test suite is designed to run in TeamCity. See `TEAMCITY-SETUP.md` for detai ### Environment Variables -| Variable | Description | -|----------|-------------| -| `CODEVITALS_TOKEN` | API token for posting results to CodeVitals | -| `CODEVITALS_URL` | CodeVitals API URL (default: https://codevitals.run). Use the apex host, not `www.`: the `www.` host 301-redirects the API and the redirect drops the POST body. | -| `COMPOSE_PROJECT_NAME` | Unique Docker project name for build isolation | -| `GIT_COMMIT` | Git commit SHA for tracking (auto-detected from plugin) | -| `GIT_BRANCH` | Git branch for tracking (default: trunk) | -| `ITERATIONS` | Number of measurement iterations (default: 5) | -| `WP_ADMIN_USER` | WordPress admin username (default: admin) | -| `WP_ADMIN_PASS` | WordPress admin password (default: password) | +| Variable | Description | +| ---------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `CODEVITALS_TOKEN` | API token for posting results to CodeVitals | +| `CODEVITALS_URL` | CodeVitals API URL (default: https://codevitals.run). Use the apex host, not `www.`: the `www.` host 301-redirects the API and the redirect drops the POST body. Set an origin-only URL (scheme + host); the API path is appended, so any path prefix on this value is not preserved. | +| `COMPOSE_PROJECT_NAME` | Unique Docker project name for build isolation | +| `GIT_COMMIT` | Git commit SHA for tracking (auto-detected from plugin) | +| `GIT_BRANCH` | Git branch for tracking (default: trunk) | +| `ITERATIONS` | Number of measurement iterations (default: 5) | +| `WP_ADMIN_USER` | WordPress admin username (default: admin) | +| `WP_ADMIN_PASS` | WordPress admin password (default: password) | ## Metric @@ -41,15 +41,15 @@ The test suite is designed to run in TeamCity. See `TEAMCITY-SETUP.md` for detai ## Scripts -| Script | Description | -|--------|-------------| -| `pnpm test` | Run full test suite (auto-clones plugin if needed) | -| `pnpm test:quick` | Quick test with 2 iterations | -| `pnpm calibrate` | Run CPU throttling calibration | -| `pnpm measure` | Run LCP measurement only | -| `pnpm report` | Post results to CodeVitals only | -| `pnpm report:dry` | Build and print the CodeVitals payload without posting (CI smoke test) | -| `pnpm test -- --skip-codevitals` | Run tests without posting to CodeVitals | +| Script | Description | +| -------------------------------- | ---------------------------------------------------------------------- | +| `pnpm test` | Run full test suite (auto-clones plugin if needed) | +| `pnpm test:quick` | Quick test with 2 iterations | +| `pnpm calibrate` | Run CPU throttling calibration | +| `pnpm measure` | Run LCP measurement only | +| `pnpm report` | Post results to CodeVitals only | +| `pnpm report:dry` | Build and print the CodeVitals payload without posting (CI smoke test) | +| `pnpm test -- --skip-codevitals` | Run tests without posting to CodeVitals | ## Safeguards @@ -63,13 +63,13 @@ CodeVitals is an **append-only** store with no self-service rollback. Once a bad `post-to-codevitals.js` checks every typed metric against `SANITY_RANGES` in `scenarios.js` before posting. A value outside its range is logged and skipped (not posted), and the script exits non-zero so CI surfaces the failure. Other valid metrics in the same run still post. -| Metric | Min | Max | -|--------|-----|-----| +| Metric | Min | Max | +| ------ | --- | ----- | | `lcp` | 100 | 60000 | | `ttfb` | 10 | 10000 | | `fcp` | 50 | 30000 | | `tbt` | 0 | 10000 | -| `cls` | 0 | 5 | +| `cls` | 0 | 5 | Add a row when a new metric type starts being posted, and set `metricType` on the scenario so the check applies to it. diff --git a/tools/performance/scripts/post-to-codevitals.js b/tools/performance/scripts/post-to-codevitals.js index 79f1b9c4ce5a..18d6a6723c37 100644 --- a/tools/performance/scripts/post-to-codevitals.js +++ b/tools/performance/scripts/post-to-codevitals.js @@ -84,6 +84,31 @@ function redactToken( text, token ) { return safe.replace( /token=[^&\s]+/g, 'token=REDACTED' ); } +/** + * Redact the token from an error and every error in its `cause` chain, in place. + * + * A re-thrown error keeps its caught cause for debugging, but that cause (and any + * nested cause) can carry the token in its `message` or `stack`. Walking the + * whole chain scrubs each one so logging the full error never leaks the secret. + * + * @param {*} error - The caught error (or any thrown value). + * @param {string} token - The token value to strip from messages and stacks. + */ +function sanitizeErrorChain( error, token ) { + const seen = new Set(); + let current = error; + while ( current && typeof current === 'object' && ! seen.has( current ) ) { + seen.add( current ); + if ( typeof current.message === 'string' ) { + current.message = redactToken( current.message, token ); + } + if ( typeof current.stack === 'string' ) { + current.stack = redactToken( current.stack, token ); + } + current = current.cause; + } +} + /** Post metrics to CodeVitals. */ async function postToCodeVitals( resultsPath, config ) { // Read results file @@ -195,13 +220,16 @@ async function postToCodeVitals( resultsPath, config ) { return { posted: true, data, validationFailed }; } catch ( error ) { clearTimeout( timeoutId ); - // Redact the token from any error before it reaches a log or a re-throw. - // Safe URL construction already keeps the secret out of parse errors; this - // is defense in depth so no upstream message carries it into a build log. + // Scrub the token from the caught error and its whole cause chain before we + // log it or re-use it as `cause`. fetch/undici can echo the token-bearing + // URL in a message or stack, so a caller that logs err.cause or + // util.inspect( err ) would otherwise re-expose the secret. This is defense + // in depth: safe URL construction already keeps it out of parse errors. + sanitizeErrorChain( error, config.codeVitalsToken ); const message = error.name === 'AbortError' ? `CodeVitals request timed out after ${ TIMEOUT_MS / 1000 }s` - : redactToken( error.message, config.codeVitalsToken ); + : error.message; console.error( '✗ Failed to post metrics to CodeVitals:', message ); throw new Error( message, { cause: error } ); } diff --git a/tools/performance/scripts/post-to-codevitals.test.js b/tools/performance/scripts/post-to-codevitals.test.js index 1004aaf12421..9e33ff0841e2 100644 --- a/tools/performance/scripts/post-to-codevitals.test.js +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -16,6 +16,7 @@ import os from 'node:os'; import path from 'node:path'; import { test } from 'node:test'; import { fileURLToPath } from 'node:url'; +import { inspect } from 'node:util'; import { checkSanityRange, extractScenarioMetrics, @@ -242,6 +243,41 @@ test( 'a malformed CODEVITALS_URL leaks the token into neither the error nor the ); } ); +test( 'a token-bearing upstream error is redacted in the message, the cause, and util.inspect', async () => { + const file = writeResults( 120 ); + const origFetch = global.fetch; + // Simulate an upstream (e.g. undici) error that echoes the full request URL, + // token included, in its message AND in a nested cause. + global.fetch = async u => { + const inner = new Error( `connect to ${ String( u ) } refused` ); + throw new Error( `request to ${ String( u ) } failed`, { cause: inner } ); + }; + let err; + try { + await silenced( () => + postToCodeVitals( file, { + dryRun: false, + codeVitalsUrl: 'https://codevitals.test', + codeVitalsToken: 'tok-cause-leak', + } ) + ); + } catch ( e ) { + err = e; + } finally { + global.fetch = origFetch; + } + assert.ok( err, 'the live post should reject' ); + assert.ok( ! err.message.includes( 'tok-cause-leak' ), 'token must not be in the message' ); + assert.ok( + ! ( err.cause?.message ?? '' ).includes( 'tok-cause-leak' ), + 'token must not be in the cause message' + ); + assert.ok( + ! inspect( err, { depth: 5 } ).includes( 'tok-cause-leak' ), + 'token must not survive in util.inspect of the whole error' + ); +} ); + // --- isDirectInvocation (the run-when-direct guard) --- test( 'isDirectInvocation: a file matches itself', () => { @@ -266,6 +302,11 @@ test( 'the CLI runs main() when invoked directly from a path containing a space' path.join( dir, 'post-to-codevitals.js' ) ); fs.copyFileSync( path.join( SCRIPTS_DIR, 'scenarios.js' ), path.join( dir, 'scenarios.js' ) ); + // Declare the temp copy as ESM. Without this, a `.js` file with `import` only + // runs as a module on Node versions where syntax detection is on by default + // (22.7+); on the lower half of this package's >=20.11 range it throws + // "Cannot use import statement outside a module" and the test fails spuriously. + fs.writeFileSync( path.join( dir, 'package.json' ), '{ "type": "module" }' ); const results = writeResults( 120 ); try { const out = execFileSync( 'node', [ path.join( dir, 'post-to-codevitals.js' ), '--dry-run' ], { From d6c8e8b1ee21d9d44b9e8e2d4182acca44a987d3 Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Fri, 26 Jun 2026 17:18:02 +0100 Subject: [PATCH 7/9] Performance: fail closed when a keyed CodeVitals metric omits its type extractScenarioMetrics now throws when a scenario sets metricKey but no metricType, instead of emitting an untyped entry that checkSanityRange would pass unchecked. That closed the one path the fail-closed guard is meant to protect: a future keyed metric posting any finite value to the append-only store. The current lcp scenario is unaffected. Also harden two tests: a dry run with a poisoned fetch proves it never posts, and the non-OK path now puts the token in the response body to prove the whole error (message, cause, util.inspect) is scrubbed. --- .../performance/scripts/post-to-codevitals.js | 12 +++++ .../scripts/post-to-codevitals.test.js | 51 +++++++++++++++++-- 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/tools/performance/scripts/post-to-codevitals.js b/tools/performance/scripts/post-to-codevitals.js index 18d6a6723c37..be8f215904d9 100644 --- a/tools/performance/scripts/post-to-codevitals.js +++ b/tools/performance/scripts/post-to-codevitals.js @@ -13,6 +13,18 @@ import { SCENARIOS, SANITY_RANGES } from './scenarios.js'; function extractScenarioMetrics( scenario, summary ) { // Use explicit metricKey if defined, otherwise fall back to prefix-based keys if ( scenario.metricKey ) { + // A posted exact-key metric must declare a metricType so checkSanityRange can + // range-check it. Without one it would fall through as an untyped "legacy" + // entry and post any finite value unchecked — the exact hole this fail-closed + // guard exists to close. A missing type is a scenario misconfiguration, so + // fail loud here (the dry-run CI smoke test catches it before any post). + if ( ! scenario.metricType ) { + throw new Error( + `Scenario "${ + scenario.key ?? scenario.metricKey + }" sets metricKey but no metricType; refusing to post an unchecked keyed metric` + ); + } // Single metric with exact key return [ { key: scenario.metricKey, value: summary.median, type: scenario.metricType } ]; } diff --git a/tools/performance/scripts/post-to-codevitals.test.js b/tools/performance/scripts/post-to-codevitals.test.js index 9e33ff0841e2..7b763450f363 100644 --- a/tools/performance/scripts/post-to-codevitals.test.js +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -120,6 +120,15 @@ test( 'legacy prefix yields five untyped entries', () => { ); } ); +test( 'a keyed scenario without a metricType is rejected, not posted unchecked', () => { + // Without this guard the entry would emit type:undefined and pass the range + // check as a "legacy" metric — posting any finite value to an append-only store. + assert.throws( + () => extractScenarioMetrics( { key: 'future', metricKey: 'future-key' }, { median: 999999 } ), + /metricKey but no metricType/ + ); +} ); + // --- redactToken (keeps the token out of logs and errors) --- test( 'redactToken strips the exact token and any token query param', () => { @@ -144,6 +153,28 @@ test( 'dry-run posts an in-range metric into the payload, nothing rejected', asy assert.equal( result.payload.metrics[ LCP_KEY ], 120 ); } ); +test( 'a dry run never calls fetch, even with a malformed URL and a token set', async () => { + const file = writeResults( 120 ); + const origFetch = global.fetch; + // Poison fetch: a dry run must short-circuit before any network call. + global.fetch = async () => { + throw new Error( 'fetch must not be called during a dry run' ); + }; + try { + const result = await silenced( () => + postToCodeVitals( file, { + dryRun: true, + codeVitalsUrl: 'http://[::1', // malformed; must not even be constructed + codeVitalsToken: 'tok-dry', + } ) + ); + assert.equal( result.posted, false ); + assert.equal( result.payload.metrics[ LCP_KEY ], 120 ); + } finally { + global.fetch = origFetch; + } +} ); + test( 'an out-of-range metric is skipped and flags validationFailed', async () => { const file = writeResults( 70000 ); const result = await silenced( () => postToCodeVitals( file, { dryRun: true } ) ); @@ -192,10 +223,16 @@ test( 'a live post sends the payload as POST and returns posted:true', async () } } ); -test( 'a non-OK CodeVitals response throws without leaking the token', async () => { +test( 'a non-OK CodeVitals response throws without leaking the token, even from the body', async () => { const file = writeResults( 120 ); const origFetch = global.fetch; - global.fetch = async () => ( { ok: false, status: 500, text: async () => 'upstream boom' } ); + // A hostile/echoing error body that includes the token query param must still + // be scrubbed everywhere, not just in the top-level message. + global.fetch = async () => ( { + ok: false, + status: 500, + text: async () => 'boom for token=tok-secret-500', + } ); try { await assert.rejects( () => @@ -208,7 +245,15 @@ test( 'a non-OK CodeVitals response throws without leaking the token', async () ), err => { assert.match( err.message, /CodeVitals API error \(500\)/ ); - assert.ok( ! err.message.includes( 'tok-secret-500' ) ); + assert.ok( ! err.message.includes( 'tok-secret-500' ), 'message must be scrubbed' ); + assert.ok( + ! ( err.cause?.message ?? '' ).includes( 'tok-secret-500' ), + 'cause must be scrubbed' + ); + assert.ok( + ! inspect( err, { depth: 5 } ).includes( 'tok-secret-500' ), + 'util.inspect must be scrubbed' + ); return true; } ); From 37c235cb9b48ed22e1ca1a90db2ca76867f4606f Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Fri, 26 Jun 2026 17:48:26 +0100 Subject: [PATCH 8/9] Performance: make local validation failures always fail the CodeVitals build A sanity-check failure and a CodeVitals network outage both exited the poster with code 1, so --allow-codevitals-failure (meant to tolerate outages) also silently tolerated bad local data. Give validation failures a distinct exit code (2) that run-performance-tests.js never suppresses, and add a CLI test asserting an out-of-range dry run exits with it. Also extend sanitizeErrorChain to scrub the token from custom enumerable string error properties, not just message/stack/cause. Native fetch never populates these; this is belt-and-suspenders for a non-native HTTP client. --- .../performance/scripts/post-to-codevitals.js | 31 ++++++++-- .../scripts/post-to-codevitals.test.js | 56 +++++++++++++++++++ .../scripts/run-performance-tests.js | 22 +++++--- 3 files changed, 98 insertions(+), 11 deletions(-) diff --git a/tools/performance/scripts/post-to-codevitals.js b/tools/performance/scripts/post-to-codevitals.js index be8f215904d9..c46631187af5 100644 --- a/tools/performance/scripts/post-to-codevitals.js +++ b/tools/performance/scripts/post-to-codevitals.js @@ -4,6 +4,15 @@ import fs from 'fs'; import path from 'path'; import { SCENARIOS, SANITY_RANGES } from './scenarios.js'; +/** + * Exit code for a LOCAL data-integrity failure: a metric failed a sanity check or + * a scenario is misconfigured. This is distinct from exit 1 (a remote CodeVitals + * transport/API failure). run-performance-tests.js treats this code as always + * fatal and never suppresses it with `--allow-codevitals-failure`, which exists + * only to tolerate CodeVitals network outages, not bad local data. + */ +const VALIDATION_FAILED_EXIT_CODE = 2; + /** * Extract metric entries for a single scenario. * @@ -100,23 +109,34 @@ function redactToken( text, token ) { * Redact the token from an error and every error in its `cause` chain, in place. * * A re-thrown error keeps its caught cause for debugging, but that cause (and any - * nested cause) can carry the token in its `message` or `stack`. Walking the - * whole chain scrubs each one so logging the full error never leaks the secret. + * nested cause) can carry the token in its `message`, `stack`, or a custom string + * field a client stashed a URL in (e.g. `requestUrl`). Walking the whole chain + * scrubs each one so logging the full error never leaks the secret. Native fetch + * never populates such fields, so the own-property pass is belt-and-suspenders for + * a non-native HTTP client; it stays at string fields rather than recursing into + * arbitrary nested objects. * * @param {*} error - The caught error (or any thrown value). - * @param {string} token - The token value to strip from messages and stacks. + * @param {string} token - The token value to strip from messages, stacks, and string fields. */ function sanitizeErrorChain( error, token ) { const seen = new Set(); let current = error; while ( current && typeof current === 'object' && ! seen.has( current ) ) { seen.add( current ); + // message and stack are non-enumerable own props, so handle them explicitly. if ( typeof current.message === 'string' ) { current.message = redactToken( current.message, token ); } if ( typeof current.stack === 'string' ) { current.stack = redactToken( current.stack, token ); } + // Then scrub any enumerable string field (cause is an object, walked below). + for ( const key of Object.keys( current ) ) { + if ( typeof current[ key ] === 'string' ) { + current[ key ] = redactToken( current[ key ], token ); + } + } current = current.cause; } } @@ -286,7 +306,9 @@ async function main() { console.error( '\n✗ One or more metrics failed sanity checks (see above). Any valid metrics were still processed.' ); - process.exit( 1 ); + // Exit with the data-integrity code so the runner always fails the build + // here, even under --allow-codevitals-failure (that flag is for outages). + process.exit( VALIDATION_FAILED_EXIT_CODE ); } console.log( dryRun ? '\n✓ Dry run complete!' : '\n✓ All done!' ); process.exit( 0 ); @@ -332,4 +354,5 @@ export { extractScenarioMetrics, isDirectInvocation, redactToken, + VALIDATION_FAILED_EXIT_CODE, }; diff --git a/tools/performance/scripts/post-to-codevitals.test.js b/tools/performance/scripts/post-to-codevitals.test.js index 7b763450f363..f6f7f1069768 100644 --- a/tools/performance/scripts/post-to-codevitals.test.js +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -23,6 +23,7 @@ import { isDirectInvocation, postToCodeVitals, redactToken, + VALIDATION_FAILED_EXIT_CODE, } from './post-to-codevitals.js'; const SCRIPTS_DIR = path.dirname( fileURLToPath( import.meta.url ) ); @@ -323,6 +324,39 @@ test( 'a token-bearing upstream error is redacted in the message, the cause, and ); } ); +test( 'a token on a custom error property (and nested cause) is scrubbed too', async () => { + const file = writeResults( 120 ); + const origFetch = global.fetch; + // Native fetch never does this, but a non-native client might stash the + // token-bearing URL on a custom field instead of (or besides) the message. + global.fetch = async u => { + const inner = new Error( 'socket closed' ); + inner.url = String( u ); // token-bearing, on a nested cause + const outer = new Error( 'request failed', { cause: inner } ); + outer.requestUrl = String( u ); // token-bearing, on the top-level error + throw outer; + }; + let err; + try { + await silenced( () => + postToCodeVitals( file, { + dryRun: false, + codeVitalsUrl: 'https://codevitals.test', + codeVitalsToken: 'tok-on-prop', + } ) + ); + } catch ( e ) { + err = e; + } finally { + global.fetch = origFetch; + } + assert.ok( err, 'the live post should reject' ); + assert.ok( + ! inspect( err, { depth: 5 } ).includes( 'tok-on-prop' ), + 'token must not survive on any custom property across the cause chain' + ); +} ); + // --- isDirectInvocation (the run-when-direct guard) --- test( 'isDirectInvocation: a file matches itself', () => { @@ -364,3 +398,25 @@ test( 'the CLI runs main() when invoked directly from a path containing a space' fs.rmSync( dir, { recursive: true, force: true } ); } } ); + +test( 'the CLI exits with the validation code on an out-of-range dry run', () => { + const results = writeResults( 70000 ); // LCP far outside [100, 60000] + const env = { ...process.env, RESULTS_PATH: results }; + delete env.CODEVITALS_TOKEN; // a dry run needs no token; prove it still fails closed + let status, output; + try { + execFileSync( 'node', [ path.join( SCRIPTS_DIR, 'post-to-codevitals.js' ), '--dry-run' ], { + encoding: 'utf8', + env, + stdio: [ 'ignore', 'pipe', 'pipe' ], + } ); + status = 0; + } catch ( err ) { + status = err.status; + output = `${ err.stdout ?? '' }${ err.stderr ?? '' }`; + } + // CI keys on the exit code; an out-of-range metric must use the data-integrity + // code (not a generic 1) so the runner can never suppress it. + assert.equal( status, VALIDATION_FAILED_EXIT_CODE ); + assert.match( output, /Sanity check failed/ ); +} ); diff --git a/tools/performance/scripts/run-performance-tests.js b/tools/performance/scripts/run-performance-tests.js index b75adcf460da..59022a3e6812 100644 --- a/tools/performance/scripts/run-performance-tests.js +++ b/tools/performance/scripts/run-performance-tests.js @@ -8,6 +8,7 @@ import { execFileSync } from 'child_process'; import fs from 'fs'; import path from 'path'; import { config as dotenvConfig } from 'dotenv'; +import { VALIDATION_FAILED_EXIT_CODE } from './post-to-codevitals.js'; import { SCENARIOS, getScenarioUrl } from './scenarios.js'; // Load .env file from the performance directory if it exists @@ -452,16 +453,23 @@ async function main() { try { execFile( 'node', [ path.join( __dirname, 'post-to-codevitals.js' ) ] ); - } catch { - // When CODEVITALS_TOKEN is explicitly set, posting failures should fail the build - // to ensure CI doesn't silently drop metrics (unless --allow-codevitals-failure is set) + } catch ( err ) { + // A local data-integrity failure (the child exits VALIDATION_FAILED_EXIT_CODE + // when a metric is rejected or a scenario is misconfigured) must always fail + // the build. --allow-codevitals-failure is for tolerating CodeVitals network + // outages, not for shipping a build that silently skipped bad local data. + const isValidationFailure = err?.status === VALIDATION_FAILED_EXIT_CODE; console.error( '\n✗ Failed to post to CodeVitals' ); - if ( options.allowCodeVitalsFailure ) { + if ( options.allowCodeVitalsFailure && ! isValidationFailure ) { console.warn( ' Continuing despite failure (--allow-codevitals-failure set)' ); } else { - console.error( ' Since CODEVITALS_TOKEN is set, this is treated as a build failure.' ); - console.error( ' Use --skip-codevitals to run without posting metrics.' ); - console.error( ' Use --allow-codevitals-failure to continue on posting failures.' ); + if ( isValidationFailure ) { + console.error( ' A metric failed local validation; this always fails the build.' ); + } else { + console.error( ' Since CODEVITALS_TOKEN is set, this is treated as a build failure.' ); + console.error( ' Use --skip-codevitals to run without posting metrics.' ); + console.error( ' Use --allow-codevitals-failure to continue on posting failures.' ); + } process.exit( 1 ); } } From 648b6302ceded0b64d4b88d428a128144b769cbd Mon Sep 17 00:00:00 2001 From: Liam Sarsfield <43409125+LiamSarsfield@users.noreply.github.com> Date: Fri, 26 Jun 2026 18:52:24 +0100 Subject: [PATCH 9/9] Performance: make scenario misconfig fail closed and scrub string error causes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes two gaps the round-6 hardening left open: - A keyed scenario missing its metricType threw a plain Error, which main() mapped to exit 1 — suppressible under --allow-codevitals-failure, despite being local bad data exactly like an out-of-range metric. It now throws a ValidationError that exitCodeForError maps to VALIDATION_FAILED_EXIT_CODE (2), so the runner always fails the build on it. - sanitizeErrorChain walked the cause chain but never redacted a primitive string cause (new Error(m, { cause: someUrl })); cause is non-enumerable, so the own-property pass missed it too, leaking the token into util.inspect. It is now redacted in place before the walk advances. Also makes run-performance-tests.js import-safe (guards main() with isDirectInvocation, mirroring post-to-codevitals.js) and extracts the build-fail decision into shouldFailBuildOnPostError, so the cross-file validation/outage contract now has committed regression coverage. Tests 27 -> 31, all green. --- .../performance/scripts/post-to-codevitals.js | 48 ++++++++++++-- .../scripts/post-to-codevitals.test.js | 66 +++++++++++++++++++ .../scripts/run-performance-tests.js | 37 +++++++++-- 3 files changed, 140 insertions(+), 11 deletions(-) diff --git a/tools/performance/scripts/post-to-codevitals.js b/tools/performance/scripts/post-to-codevitals.js index c46631187af5..784cdf845c0c 100644 --- a/tools/performance/scripts/post-to-codevitals.js +++ b/tools/performance/scripts/post-to-codevitals.js @@ -13,6 +13,32 @@ import { SCENARIOS, SANITY_RANGES } from './scenarios.js'; */ const VALIDATION_FAILED_EXIT_CODE = 2; +/** + * A local data-integrity or scenario-configuration failure. A thrown error of this + * type must always fail the build (exit VALIDATION_FAILED_EXIT_CODE), so the runner + * can never suppress it under `--allow-codevitals-failure`. It is distinct from a + * CodeVitals transport/API error, which exits 1 and that flag may tolerate. + */ +class ValidationError extends Error { + constructor( message ) { + super( message ); + this.name = 'ValidationError'; + } +} + +/** + * Map a thrown error to a process exit code. A local data-integrity failure + * (ValidationError, e.g. a misconfigured scenario) exits VALIDATION_FAILED_EXIT_CODE + * so the runner always fails the build on it; any other error (a CodeVitals + * transport/API failure) exits 1, which `--allow-codevitals-failure` may tolerate. + * + * @param {*} error - The caught error. + * @return {number} The exit code to use. + */ +function exitCodeForError( error ) { + return error instanceof ValidationError ? VALIDATION_FAILED_EXIT_CODE : 1; +} + /** * Extract metric entries for a single scenario. * @@ -28,7 +54,7 @@ function extractScenarioMetrics( scenario, summary ) { // guard exists to close. A missing type is a scenario misconfiguration, so // fail loud here (the dry-run CI smoke test catches it before any post). if ( ! scenario.metricType ) { - throw new Error( + throw new ValidationError( `Scenario "${ scenario.key ?? scenario.metricKey }" sets metricKey but no metricType; refusing to post an unchecked keyed metric` @@ -110,7 +136,8 @@ function redactToken( text, token ) { * * A re-thrown error keeps its caught cause for debugging, but that cause (and any * nested cause) can carry the token in its `message`, `stack`, or a custom string - * field a client stashed a URL in (e.g. `requestUrl`). Walking the whole chain + * field a client stashed a URL in (e.g. `requestUrl`), or a primitive string + * `cause`. Walking the whole chain * scrubs each one so logging the full error never leaks the secret. Native fetch * never populates such fields, so the own-property pass is belt-and-suspenders for * a non-native HTTP client; it stays at string fields rather than recursing into @@ -131,12 +158,20 @@ function sanitizeErrorChain( error, token ) { if ( typeof current.stack === 'string' ) { current.stack = redactToken( current.stack, token ); } - // Then scrub any enumerable string field (cause is an object, walked below). + // Then scrub any enumerable string field (an object `cause` is non-enumerable + // and is walked on the next iteration; a string `cause` is handled just below). for ( const key of Object.keys( current ) ) { if ( typeof current[ key ] === 'string' ) { current[ key ] = redactToken( current[ key ], token ); } } + // `cause` is non-enumerable on Error, so the pass above can't reach a primitive + // string cause (e.g. `new Error( m, { cause: someUrl } )`); it would otherwise + // survive untouched and leak the token. Redact it in place before advancing; an + // object cause is scrubbed when the loop walks into it next. + if ( typeof current.cause === 'string' ) { + current.cause = redactToken( current.cause, token ); + } current = current.cause; } } @@ -314,7 +349,10 @@ async function main() { process.exit( 0 ); } catch ( error ) { console.error( '\n✗ Failed:', error.message ); - process.exit( 1 ); + // A scenario misconfiguration (ValidationError) is local bad data and must + // always fail the build, exactly like an out-of-range metric — not a + // suppressible exit 1. exitCodeForError encodes that split. + process.exit( exitCodeForError( error ) ); } } @@ -352,7 +390,9 @@ export { postToCodeVitals, checkSanityRange, extractScenarioMetrics, + exitCodeForError, isDirectInvocation, redactToken, + ValidationError, VALIDATION_FAILED_EXIT_CODE, }; diff --git a/tools/performance/scripts/post-to-codevitals.test.js b/tools/performance/scripts/post-to-codevitals.test.js index f6f7f1069768..8dfe203b3277 100644 --- a/tools/performance/scripts/post-to-codevitals.test.js +++ b/tools/performance/scripts/post-to-codevitals.test.js @@ -19,12 +19,14 @@ import { fileURLToPath } from 'node:url'; import { inspect } from 'node:util'; import { checkSanityRange, + exitCodeForError, extractScenarioMetrics, isDirectInvocation, postToCodeVitals, redactToken, VALIDATION_FAILED_EXIT_CODE, } from './post-to-codevitals.js'; +import { shouldFailBuildOnPostError } from './run-performance-tests.js'; const SCRIPTS_DIR = path.dirname( fileURLToPath( import.meta.url ) ); const LCP_KEY = 'wp-admin-dashboard-connection-sim-largestContentfulPaint'; @@ -130,6 +132,21 @@ test( 'a keyed scenario without a metricType is rejected, not posted unchecked', ); } ); +test( 'a scenario misconfiguration maps to the validation exit code, a transport error to 1', () => { + let configError; + try { + extractScenarioMetrics( { key: 'future', metricKey: 'future-key' }, { median: 1 } ); + } catch ( e ) { + configError = e; + } + // A keyed-without-metricType config error is local bad data: it must always fail + // the build (the validation code), exactly like an out-of-range metric — never a + // suppressible transport exit 1. Otherwise --allow-codevitals-failure could mask it. + assert.equal( configError?.name, 'ValidationError' ); + assert.equal( exitCodeForError( configError ), VALIDATION_FAILED_EXIT_CODE ); + assert.equal( exitCodeForError( new Error( 'CodeVitals API error (500)' ) ), 1 ); +} ); + // --- redactToken (keeps the token out of logs and errors) --- test( 'redactToken strips the exact token and any token query param', () => { @@ -357,6 +374,37 @@ test( 'a token on a custom error property (and nested cause) is scrubbed too', a ); } ); +test( 'a token-bearing primitive string cause is scrubbed too', async () => { + const file = writeResults( 120 ); + const origFetch = global.fetch; + // `Error.cause` accepts any value. A non-native client may throw + // `new Error( msg, { cause: someUrl } )` where the cause is a plain string, not an + // Error. cause is non-enumerable, so this must be caught by the chain walk, not the + // own-property pass — the gap this regression test pins. + global.fetch = async u => { + throw new Error( 'request failed', { cause: `connect to ${ String( u ) } refused` } ); + }; + let err; + try { + await silenced( () => + postToCodeVitals( file, { + dryRun: false, + codeVitalsUrl: 'https://codevitals.test', + codeVitalsToken: 'tok-string-cause', + } ) + ); + } catch ( e ) { + err = e; + } finally { + global.fetch = origFetch; + } + assert.ok( err, 'the live post should reject' ); + assert.ok( + ! inspect( err, { depth: 5 } ).includes( 'tok-string-cause' ), + 'token must not survive in a primitive string cause' + ); +} ); + // --- isDirectInvocation (the run-when-direct guard) --- test( 'isDirectInvocation: a file matches itself', () => { @@ -420,3 +468,21 @@ test( 'the CLI exits with the validation code on an out-of-range dry run', () => assert.equal( status, VALIDATION_FAILED_EXIT_CODE ); assert.match( output, /Sanity check failed/ ); } ); + +// --- run-performance-tests.js: the build-fail decision (cross-file contract) --- +// The poster sets the exit code; the runner decides whether it fails the build. +// These pin that the validation/outage split survives a future runner refactor. + +test( 'shouldFailBuildOnPostError: a validation failure (exit 2) is always fatal', () => { + const validation = { status: VALIDATION_FAILED_EXIT_CODE }; + // Even with --allow-codevitals-failure set, local bad data must fail the build. + assert.equal( shouldFailBuildOnPostError( validation, true ), true ); + assert.equal( shouldFailBuildOnPostError( validation, false ), true ); +} ); + +test( 'shouldFailBuildOnPostError: a transport failure is suppressible only with the flag', () => { + assert.equal( shouldFailBuildOnPostError( { status: 1 }, true ), false ); // outage tolerated + assert.equal( shouldFailBuildOnPostError( { status: 1 }, false ), true ); // fatal by default + assert.equal( shouldFailBuildOnPostError( undefined, true ), false ); // unknown exit, flag set + assert.equal( shouldFailBuildOnPostError( undefined, false ), true ); +} ); diff --git a/tools/performance/scripts/run-performance-tests.js b/tools/performance/scripts/run-performance-tests.js index 59022a3e6812..9c168397cdbe 100644 --- a/tools/performance/scripts/run-performance-tests.js +++ b/tools/performance/scripts/run-performance-tests.js @@ -8,7 +8,7 @@ import { execFileSync } from 'child_process'; import fs from 'fs'; import path from 'path'; import { config as dotenvConfig } from 'dotenv'; -import { VALIDATION_FAILED_EXIT_CODE } from './post-to-codevitals.js'; +import { isDirectInvocation, VALIDATION_FAILED_EXIT_CODE } from './post-to-codevitals.js'; import { SCENARIOS, getScenarioUrl } from './scenarios.js'; // Load .env file from the performance directory if it exists @@ -45,6 +45,23 @@ function execFile( cmd, args = [], options = {} ) { } } +/** + * Whether a failed CodeVitals post must fail the build. + * + * A local data-integrity failure — the poster exits VALIDATION_FAILED_EXIT_CODE when + * a metric is rejected or a scenario is misconfigured — is always fatal. + * `--allow-codevitals-failure` exists only to tolerate a CodeVitals network outage + * (any other non-zero exit), never to ship a build that skipped bad local data. + * + * @param {{status?: number}|undefined} err - The error the poster child threw (carries its exit code on `.status`). + * @param {boolean} allowCodeVitalsFailure - Whether `--allow-codevitals-failure` was passed. + * @return {boolean} True if the build must fail. + */ +function shouldFailBuildOnPostError( err, allowCodeVitalsFailure ) { + const isValidationFailure = err?.status === VALIDATION_FAILED_EXIT_CODE; + return isValidationFailure || ! allowCodeVitalsFailure; +} + /** Execute a docker compose command. */ function dockerCompose( args, options = {} ) { const baseArgs = [ @@ -460,7 +477,7 @@ async function main() { // outages, not for shipping a build that silently skipped bad local data. const isValidationFailure = err?.status === VALIDATION_FAILED_EXIT_CODE; console.error( '\n✗ Failed to post to CodeVitals' ); - if ( options.allowCodeVitalsFailure && ! isValidationFailure ) { + if ( ! shouldFailBuildOnPostError( err, options.allowCodeVitalsFailure ) ) { console.warn( ' Continuing despite failure (--allow-codevitals-failure set)' ); } else { if ( isValidationFailure ) { @@ -495,8 +512,14 @@ async function main() { } } -// Run -main().catch( error => { - console.error( 'Fatal error:', error ); - process.exit( 1 ); -} ); +// Run only when invoked directly, not when imported (e.g. by the unit test that +// exercises shouldFailBuildOnPostError), so importing this module never spins up +// Docker or kicks off a full perf run. +if ( isDirectInvocation( import.meta.filename, process.argv[ 1 ] ) ) { + main().catch( error => { + console.error( 'Fatal error:', error ); + process.exit( 1 ); + } ); +} + +export { shouldFailBuildOnPostError };