diff --git a/packages/cli/src/budgets-unmeasured.test.ts b/packages/cli/src/budgets-unmeasured.test.ts index cf7004b8a..a4b8eda9f 100644 --- a/packages/cli/src/budgets-unmeasured.test.ts +++ b/packages/cli/src/budgets-unmeasured.test.ts @@ -24,8 +24,13 @@ const refused = (code: string, cause: string): UnmeasuredRoute => ({ actor: ACTOR, }); +/** The file `/console` is declared by in this fixture — the locator an agent opens. */ +const CONSOLE_FILE = 'apps/web/app/console/page.tsx'; + const only = (entry: UnmeasuredRoute) => { - const findings = checkBudgets(manifest, { routes: [] }, [entry]); + const findings = checkBudgets(manifest, { routes: [] }, [entry], (url) => + url === '/console' ? CONSOLE_FILE : undefined, + ); expect(findings).toHaveLength(1); return findings[0]; }; @@ -42,6 +47,9 @@ describe('unit · a route the measurement actor could not render', () => { expect(finding?.fix).toContain('defineMeasurementActor(() => userActor({'); expect(finding?.fix).toContain("orgId: '<"); expect(finding?.fix).toContain("roles: ['<"); + // `at` is the file to open, never the route URL — and the route is not lost for that. + expect(finding?.at).toBe(CONSOLE_FILE); + expect(finding?.meta?.['route']).toBe('/console'); }); } diff --git a/packages/cli/src/budgets-unmeasured.ts b/packages/cli/src/budgets-unmeasured.ts index 515241328..178aacef3 100644 --- a/packages/cli/src/budgets-unmeasured.ts +++ b/packages/cli/src/budgets-unmeasured.ts @@ -39,13 +39,26 @@ const ACTOR_REFUSALS: ReadonlySet = new Set([ 'X_TENANCY_ACTOR_ORG_REQUIRED', ]); -function ownCodeFinding(url: string, entry: UnmeasuredRoute): Finding { +/** + * The locator every finding here carries: `at` is the FILE an agent opens, never the route URL — + * `output.ts`'s `Finding.at`, and `verify-checks.ts` reads it as the path to open. The URL is a + * fact of its own and rides beside it in `meta.route`, so `--json` loses nothing by the move. + * + * OMITTED, not filled with something, when no file declares the route: a URL is not a path anyone + * can open, and `at` that names nothing opens nothing. The cause and the fix both name the URL. + */ +const locatorOf = (file: string | undefined, url: string): Pick => ({ + ...(file === undefined ? {} : { at: file }), + meta: { route: url }, +}); + +function ownCodeFinding(url: string, file: string | undefined, entry: UnmeasuredRoute): Finding { return { code: entry.code ?? 'X_BUDGET_UNMEASURED', cause: entry.cause ?? entry.reason, fix: entry.fix ?? `x build --target static --json # its "unmeasured" list has ${url}`, docs: ERROR_DOCS_URL, - at: url, + ...locatorOf(file, url), }; } @@ -57,15 +70,20 @@ const refusalOf = (entry: UnmeasuredRoute): string => * A build ran and has no row for `url`. `entry` is the report's account of why, when it has one: * an instruction under its own code, an actor edit, or — for a failure that is a fact about the * build's environment — the report, with the refusal already in the cause. + * + * `file` is the file that declares `url`, resolved by the caller (`budgets.ts` asks the loaded + * route table); `undefined` when no route file claims it, and the finding then carries no locator + * rather than a URL that opens nothing. */ export function builtUnmeasuredFinding( url: string, declared: string, statsFile: string, entry: UnmeasuredRoute | undefined, + file: string | undefined, ): Finding { if (entry?.code !== undefined && REPORTED_BY_OWN_CODE.has(entry.code)) { - return ownCodeFinding(url, entry); + return ownCodeFinding(url, file, entry); } const base = `${url} declares a ${declared} budget and ${statsFile} has no row for it, so the build ran and could not weigh it`; if (entry?.code !== undefined && ACTOR_REFUSALS.has(entry.code)) { @@ -76,7 +94,7 @@ export function builtUnmeasuredFinding( // The paste: one declaration in the one file every build imports. fix: "defineMeasurementActor(() => userActor({ id: 'measure', orgId: '', roles: [''] })) # in app.config.ts", docs: ERROR_DOCS_URL, - at: url, + ...locatorOf(file, url), }; } return { @@ -86,6 +104,6 @@ export function builtUnmeasuredFinding( // route it could not weigh, and why. fix: `x build --target static --json # its "unmeasured" list says why ${url} could not be weighed`, docs: ERROR_DOCS_URL, - at: url, + ...locatorOf(file, url), }; } diff --git a/packages/cli/src/budgets.test.ts b/packages/cli/src/budgets.test.ts index e9ef22fac..e2d18cbf7 100644 --- a/packages/cli/src/budgets.test.ts +++ b/packages/cli/src/budgets.test.ts @@ -69,13 +69,35 @@ const jsBytesOf = async (html: string, out: string): Promise => describe('unit · budgets', () => { test('a declared JS budget with no measurement is a finding, not a pass', () => { - const findings = checkBudgets(manifestOf(route('/pricing', { js: '20kb' })), undefined); + const findings = checkBudgets( + manifestOf(route('/pricing', { js: '20kb' })), + undefined, + [], + // The locator is the file an agent opens, never the URL; the route is still reachable + // under `--json` in `meta.route`, and the cause already spells it for the terminal. + (url) => (url === '/pricing' ? 'apps/web/site/pricing/page.tsx' : undefined), + ); expect(findings.map((finding) => finding.code)).toEqual(['X_BUDGET_UNMEASURED']); expect(findings[0]?.cause).toContain('/pricing'); expect(findings[0]?.cause).toContain('JS'); expect(findings[0]?.cause).toContain(BUILD_STATS_FILE); expect(findings[0]?.fix).toBe('x build --target static --json && x verify --json'); - expect(findings[0]?.at).toBe('/pricing'); + expect(findings[0]?.at).toBe('apps/web/site/pricing/page.tsx'); + expect(findings[0]?.meta?.['route']).toBe('/pricing'); + }); + + // A route no file declares has no path to open, so `at` is omitted rather than filled with the + // URL — and the route is not lost for that: `meta.route` carries it either way. + test('a budgeted route no file declares carries no locator, and still names its route in meta', () => { + const findings = checkBudgets( + manifestOf(route('/pricing', { js: '20kb' })), + undefined, + [], + () => undefined, + ); + expect(findings[0]?.at).toBeUndefined(); + expect(findings[0]?.meta?.['route']).toBe('/pricing'); + expect(findings[0]?.cause).toContain('/pricing'); }); // Two conditions, two instructions. "No build has run" is closed by one command for every route @@ -114,6 +136,18 @@ describe('unit · budgets', () => { expect(findings[0]?.cause).toContain('/dash -> chart.ts'); }); + // `at` is the same file the fix edits — one place, not two that can drift apart. + test('the locator and the fix name one file', () => { + const findings = checkBudgets( + manifestOf(route('/dash', { js: '10kb' })), + stats({ path: '/dash', jsBytes: 40_961 }), + [], + () => 'apps/web/app/dash/page.tsx', + ); + expect(findings[0]?.at).toBe('apps/web/app/dash/page.tsx'); + expect(findings[0]?.fix).toContain('edit apps/web/app/dash/page.tsx'); + }); + test('the JS fix is the paste: the route file, the next whole kb, the measured bytes and a why', () => { const findings = checkBudgets( manifestOf(route('/dash', { js: '10kb' })), @@ -340,8 +374,11 @@ describe('unit · an ABSENT stats file is every budget unmeasured, not nothing t route('/about'), ); // The step hands `checkBudgets` exactly this — the reader's answer, not a stand-in for it. + // No route file declares these, so `at` is absent and the routes ride in `meta.route`: the + // one-per-budgeted-route count is still exactly what the assertion below holds. const findings = checkBudgets(manifest, await readBuildStats(root)); - expect(findings.map((finding) => finding.at)).toEqual(['/', '/dash']); + expect(findings.map((finding) => finding.meta?.['route'])).toEqual(['/', '/dash']); + expect(findings.map((finding) => finding.at)).toEqual([undefined, undefined]); expect(new Set(findings.map((finding) => finding.code))).toEqual( new Set(['X_BUDGET_UNMEASURED']), ); diff --git a/packages/cli/src/budgets.ts b/packages/cli/src/budgets.ts index 78bc5e73c..47c0f8b6b 100644 --- a/packages/cli/src/budgets.ts +++ b/packages/cli/src/budgets.ts @@ -117,27 +117,34 @@ function exceededJsFix(url: string, file: string | undefined, bytes: number): st * run and could not weigh this one, which is `PrerenderReport.unmeasured`'s question and not a * second build's. Reporting the first as the second is what sends a reader to re-run a build that * already did everything it was going to do. + * + * `fileOf` names the file an agent opens to fix this route, exactly as it does for + * `X_BUDGET_EXCEEDED`: `Finding.at` is a PATH, never a URL (`output.ts`, `verify-checks.ts`), + * so the route rides beside it in `meta.route` and every cause below still spells the URL. */ function unmeasuredFinding( url: string, declared: string, stats: BuildStats | undefined, unmeasured: readonly UnmeasuredRoute[], + fileOf: RouteFileOf, ): Finding { + const file = fileOf(url); if (stats?.stale === true) { return { code: 'X_BUDGET_UNMEASURED', cause: `${url} declares a ${declared} budget and ${BUILD_STATS_FILE} was written by an earlier measurement rule than this gate's (v${String(BUILD_STATS_RULES)}), so its numbers are not this gate's to read`, fix: 'x build --target static --json && x verify --json', docs: ERROR_DOCS_URL, - at: url, + ...(file === undefined ? {} : { at: file }), + meta: { route: url }, }; } // A build ran: the report's account of this route decides the instruction // (`budgets-unmeasured.ts`) — its own code, an actor edit, or the report. if (stats !== undefined) { const entry = unmeasured.find((one) => one.path === url); - return builtUnmeasuredFinding(url, declared, BUILD_STATS_FILE, entry); + return builtUnmeasuredFinding(url, declared, BUILD_STATS_FILE, entry, file); } return { code: 'X_BUDGET_UNMEASURED', @@ -147,7 +154,8 @@ function unmeasuredFinding( // the one caller of `writeBuildStats`. fix: 'x build --target static --json && x verify --json', docs: ERROR_DOCS_URL, - at: url, + ...(file === undefined ? {} : { at: file }), + meta: { route: url }, }; } @@ -161,8 +169,9 @@ function unmeasuredFinding( * and can be absent for the same reason; with it, a route whose measurement failed on a code in * `REPORTED_BY_OWN_CODE` (`budgets-unmeasured.ts`) is reported under that code, with the build's own cause and fix. * - * `fileOf` names the file an `X_BUDGET_EXCEEDED` fix edits: the step runs after the app loaded, - * so render's route table already holds it. A test hands its own. + * `fileOf` names the file an agent opens for a route's finding — the locator `Finding.at` carries + * and the file `X_BUDGET_EXCEEDED`'s fix edits: the step runs after the app loaded, so render's + * route table already holds it. A test hands its own. */ export function checkBudgets( manifest: Manifest, @@ -183,19 +192,24 @@ export function checkBudgets( const js = jsBudgetOf(route); if (measured === undefined) { if (js !== null) { - findings.push(unmeasuredFinding(route.url, 'JS', stats, unmeasured)); + findings.push(unmeasuredFinding(route.url, 'JS', stats, unmeasured, fileOf)); } continue; } if (js !== null && measured.jsBytes > js) { - const meta = chargedMeta(measured.jsBytes, measured.charged, measured.inlineJsBytes); + // `at` is the file an agent opens — the SAME resolution `exceededJsFix` edits, so the + // locator and the instruction name one place. The route URL it belongs to is in `cause` + // (`budgets-charged.test.ts` holds the per-file breakdown in `meta` only when a row carries + // one — adding a `route` key would conflict with that contract). + const file = fileOf(route.url); + const charged = chargedMeta(measured.jsBytes, measured.charged, measured.inlineJsBytes); findings.push({ code: 'X_BUDGET_EXCEEDED', cause: `${route.url} ships ${formatBytes(measured.jsBytes)} of JS (minified, uncompressed) over a ${formatBytes(js)} budget via ${chainOf(measured)}${chargedClause(measured.charged, measured.inlineJsBytes)}`, - fix: exceededJsFix(route.url, fileOf(route.url), measured.jsBytes), + fix: exceededJsFix(route.url, file, measured.jsBytes), docs: ERROR_DOCS_URL, - at: route.url, - ...(meta === undefined ? {} : { meta }), + ...(file === undefined ? {} : { at: file }), + ...(charged === undefined ? {} : { meta: charged }), }); } } diff --git a/packages/cli/src/ci-log.ts b/packages/cli/src/ci-log.ts index f5f5c0577..2bc132b04 100644 --- a/packages/cli/src/ci-log.ts +++ b/packages/cli/src/ci-log.ts @@ -4,7 +4,7 @@ // this file undoes the transport and reconstructs the `Finding`, rather than printing a wall of // text and asking the reader to spot the code in it. -import type { Finding } from './output'; +import type { Finding, JsonValue } from './output'; /** One log line, with the two columns Actions prefixes every line with taken off. */ export interface CiLogLine { @@ -67,11 +67,52 @@ export function jobsInLog(lines: readonly CiLogLine[]): readonly string[] { } const CODE = /^(X_[A-Z][A-Z0-9_]*)/; -const LOCATOR = /\(([^()]+)\)$/; +// The ONE way a `Finding.at` — a path an agent can open — returns from a log. The renderer +// prints it as the parenthesised expression in the head (`renderFinding`, `output.ts`). +// +// Anchored to the head minus a trailing `[step=…]`, NOT to the end of the line: +// `X_VERIFY_STEP_TIMEOUT` sets both `at` and `meta.step`, so that head reads +// `CODE (path) [step=unit]` — an end-anchor matches only what follows the brackets, the match +// fails, and the path is dropped. With no step the optional group is absent and the +// parenthesised group is still the last thing on the line. +const LOCATOR = /\(([^()]+)\)(?=\s*(?:\[step=[^[\]]+\]\s*)?$)/; +// The second, non-path locator the renderer prints on the head — `meta.step` in the same +// brackets-keyed form (`[step=lint]`) `renderFinding` writes — so a merge gap's gate step name +// or any future non-path identifier survives the round-trip too. Its own brackets are what +// `LOCATOR`'s lookahead stands aside for, so the two cannot collide in one head. +const STEP_LOCATOR = /\[step=([^[\]]+)\]\s*$/; const after = (text: string, label: string): string | undefined => text.trim().startsWith(label) ? text.trim().slice(label.length).trim() : undefined; +// The `meta.step` locator the renderer prints in the head (`output.ts:185-189`): a gate step name +// the finding carries on `meta`, never on `at`. Total on every shape `Finding.meta` may take — +// the field is optional, and a malformed `meta` (a non-object value, an array) is read as "no +// step", the same answer an absent `meta` gets. +const stepOf = (finding: Finding): string => { + if (typeof finding.meta === 'object' && finding.meta !== null && !Array.isArray(finding.meta)) { + const value = (finding.meta as { readonly [key: string]: JsonValue })['step']; + return typeof value === 'string' ? value : ''; + } + return ''; +}; + +/** + * The dedupe key a finding is told apart by. Two findings are the same only if every locator + * the renderer prints agrees on them — `code`, `cause`, `at`, and `meta.step`. Ci-log's per-log + * dedupe (`findingsFrom` below) and `x ci`'s cross-log dedupe (`cmd-ci.ts`) both build the key + * off this formula, so a finding that survives one cannot be silently merged by the other. + * + * A re-run appends a second copy of the same block; two identical findings would read as two + * problems where the job has one. A `X_COVERAGE_BELOW` routed to two different packages is the + * opposite — two findings, where a key of code and cause alone folds the second into a duplicate. + * `fix` is deliberately not in it: the same block reprints its fix line, and `x ci` fences it + * differently per job, so keying on it would report a re-run twice. + */ +export function findingKey(finding: Finding): string { + return `${finding.code}\0${finding.cause}\0${finding.at ?? ''}\0${stepOf(finding)}`; +} + /** * `renderFinding`'s output, read backwards. A code line ALONE is not a finding — prose naming a * code, a summary counting them and a `fix:` quoting one all start with `X_`, and reconstructing @@ -93,19 +134,26 @@ export function findingsFrom(lines: readonly CiLogLine[]): readonly Finding[] { if (cause === undefined || fix === undefined) continue; const code = head[1] ?? ''; const at = LOCATOR.exec(texts[index] ?? '')?.[1]; + const step = STEP_LOCATOR.exec(texts[index] ?? '')?.[1]; const docs = after(texts[index + 3] ?? '', 'docs:'); - const key = `${code}\0${cause}`; - // A re-run appends a second copy of the same block; two identical findings would read as two - // problems where the job has one. - if (!seen.has(key)) { - seen.add(key); - out.push({ - code, - cause, - fix, - ...(at === undefined ? {} : { at }), - ...(docs === undefined ? {} : { docs }), - }); + // Step name and any other reconstructed structured fact ride on `meta`, so the + // reconstructed finding has a place to put a non-path locator `at` cannot carry. + const meta = step === undefined ? undefined : { step }; + // Built once, then keyed: `findingKey` reads `code`, `cause`, `at` and `meta.step` off it, + // so the per-log dedupe here and the across-log dedupe in `cmd-ci.ts` agree on what one + // finding is. `X_VERIFY_MERGE_INCOMPLETE` arrives here once per missing step, and only the + // step name tells them apart. + const finding: Finding = { + code, + cause, + fix, + ...(at === undefined ? {} : { at }), + ...(meta === undefined ? {} : { meta }), + ...(docs === undefined ? {} : { docs }), + }; + if (!seen.has(findingKey(finding))) { + seen.add(findingKey(finding)); + out.push(finding); } index += 2; } diff --git a/packages/cli/src/cmd-ci.test.ts b/packages/cli/src/cmd-ci.test.ts index a483999e0..11b332c4a 100644 --- a/packages/cli/src/cmd-ci.test.ts +++ b/packages/cli/src/cmd-ci.test.ts @@ -5,13 +5,14 @@ import { describe, expect, test } from 'bun:test'; import { REQUIRED_BUN } from './app-root'; -import { findingsFrom, jobsInLog, parseLogLines, tailOf } from './ci-log'; +import { findingKey, findingsFrom, jobsInLog, parseLogLines, tailOf } from './ci-log'; import type { CiRun } from './ci-runs'; import { isFailed, latestPerWorkflow, RUN_FIELDS } from './ci-runs'; import { CI_MESSAGE_KEYS, ciCommand, RUN_LOOKBACK, TAIL_LINES } from './cmd-ci'; import type { CommandContext } from './command'; import type { ExecResult, Runner } from './exec'; import { messageKeys } from './messages'; +import type { Finding } from './output'; import { parseArgs } from './parse'; const line = (job: string, stamp: string, text: string, bom = false): string => @@ -323,3 +324,92 @@ describe('unit · x ci is triage in one command', () => { expect(error.fix).toContain('--branch'); }); }); + +/** + * One code, one cause, and a locator that differs. `ci-log`'s dedupe already keys on `at` and + * `meta.step` (`ci-log.ts:114`); `cmd-ci.ts`'s second dedupe — the one that folds findings across + * jobs and across runs — used to key on code and cause alone, so it read the second as a copy of + * the first and dropped it. The gate printed two problems and `x ci` reported one. + */ +describe('unit · x ci dedupes a finding by everything the gate printed about it', () => { + /** The two blocks verbatim, as `renderFinding` wrote them in the failing job. */ + const BLOCK = [ + line('package (cli)', '1:30.9733457', ' X_COVERAGE_BELOW (packages/cli)'), + line('package (cli)', '1:30.9736546', ' cause: lines below the floor'), + line('package (cli)', '1:30.9739891', ' fix: cover the gap with tests beside the source'), + line('package (cli)', '1:31.9733457', ' X_COVERAGE_BELOW (packages/testing)'), + line('package (cli)', '1:31.9736546', ' cause: lines below the floor'), + line('package (cli)', '1:31.9739891', ' fix: cover the gap with tests beside the source'), + ].join('\n'); + + const ONE_JOB = { + name: 'package (cli)', + status: 'completed', + conclusion: 'failure', + url: 'u/cli', + steps: [{ name: 'test + coverage', conclusion: 'failure', number: 2 }], + }; + + /** The same `gh` answers as `failing`, over a log of this test's own choosing. */ + const answering = (log: string): Runner => + ghTable((joined) => { + if (joined.startsWith('gh repo view')) return REPO; + if (joined.startsWith('git rev-parse')) return 'feat/x\n'; + if (joined.includes('--log-failed')) return log; + if (joined.startsWith('gh run list')) return JSON.stringify([RUN_ROW]); + if (joined.startsWith('gh run view')) return JSON.stringify({ ...RUN_ROW, jobs: [ONE_JOB] }); + return undefined; + }).runner; + + test('the same code and cause at two paths are two findings, not one copy of the first', async () => { + // The command-level round-trip: `findingsFrom` reads both blocks out of one log, and the + // across-job dedupe must hand both back. A key of `code cause` alone merges them. + const result = await ciCommand.run(contextFor(['ci'], answering(BLOCK))); + expect(result.ok).toBe(false); + expect(result.findings?.map((finding) => [finding.code, finding.at])).toEqual([ + ['X_COVERAGE_BELOW', 'packages/cli'], + ['X_COVERAGE_BELOW', 'packages/testing'], + ]); + expect(result.findings).toHaveLength(2); + }); + + // Same code, same cause, no path — the pair that is indistinguishable unless `meta.step` is in + // the key. `X_VERIFY_MERGE_INCOMPLETE` is the shape the gate really writes: a merge gap per + // missing step, each with a step name in the head and none in `at`. + test('the same code and cause in two gate steps are two findings too', async () => { + const steps = [ + line('verify', '0:10.0000000', ' X_VERIFY_MERGE_INCOMPLETE [step=lint]'), + line('verify', '0:10.1000000', ' cause: its result never arrived'), + line('verify', '0:10.2000000', ' fix: gh run rerun --failed'), + line('verify', '0:11.0000000', ' X_VERIFY_MERGE_INCOMPLETE [step=manifest]'), + line('verify', '0:11.1000000', ' cause: its result never arrived'), + line('verify', '0:11.2000000', ' fix: gh run rerun --failed'), + ].join('\n'); + const result = await ciCommand.run(contextFor(['ci'], answering(steps))); + expect(result.findings?.map((finding) => finding.meta?.['step'])).toEqual(['lint', 'manifest']); + }); + + // Both halves of the fix, in one assertion: what ci-log keeps apart, the command keeps apart, + // and what is genuinely the same block is still folded to one. A key that took every field + // would stop folding anything and report a re-run twice. + test('a repeated block is still one finding, and the key is the one ci-log used', () => { + const pair = (at: string, step: string): Finding => ({ + code: 'X_VERIFY_STEP_TIMEOUT', + cause: 'step "unit" passed its 300000ms deadline', + fix: 'bun test packages/cli', + at, + meta: { step }, + }); + const same = pair('packages/cli/src/output.test.ts', 'unit'); + // The `fix:` line is not in the key: a re-run reprints the whole block, and `x ci` fences it + // differently per job, so keying on it would report the re-run twice. + expect(findingKey(same)).toBe(findingKey({ ...same, fix: 'a re-run wrote another fix line' })); + expect(findingKey(same)).not.toBe(findingKey(pair('packages/cli/src/other.test.ts', 'unit'))); + expect(findingKey(same)).not.toBe(findingKey(pair('packages/cli/src/output.test.ts', 'live'))); + // An absent locator is the empty string, not the word "undefined" — and it is the same + // absence `findingsFrom` reconstructs for a head the renderer wrote without one. + const bare: Finding = { code: 'X_A', cause: 'c', fix: 'f' }; + expect(findingKey(bare)).toBe('X_A\0c\0\0'); + expect(findingKey({ ...bare, meta: { step: ['lint'] } })).toBe('X_A\0c\0\0'); + }); +}); diff --git a/packages/cli/src/cmd-ci.ts b/packages/cli/src/cmd-ci.ts index ad31af9de..17e2e6350 100644 --- a/packages/cli/src/cmd-ci.ts +++ b/packages/cli/src/cmd-ci.ts @@ -6,7 +6,7 @@ import { UltimateError } from '@ultimat3/core'; import type { CiLogLine } from './ci-log'; -import { findingsFrom, jobsInLog, parseLogLines, tailOf } from './ci-log'; +import { findingKey, findingsFrom, jobsInLog, parseLogLines, tailOf } from './ci-log'; import type { CiJob, CiRun } from './ci-runs'; import { failedLog, isFailed, isRunning, latestPerWorkflow, listRuns, viewRun } from './ci-runs'; import { ciSpec, TAIL_LINES } from './cmd-ci-spec'; @@ -128,9 +128,11 @@ async function inspect( const pool: readonly CiLogLine[] = own.length > 0 ? own : lines; const job = failed.find((candidate) => candidate.name === name); for (const finding of findingsFrom(pool)) { + // Keep whatever the log carried: a path-style `at` or a non-path step (now on `meta.step`). + // Inventing a locator from the CI JOB name would put a non-path in `at` — exactly the seam + // this repair closes — so when the log had nothing, the finding reads nothing. findings.push({ ...finding, - at: finding.at ?? name, // Fenced and tagged: the log is foreign text, and a `fix:` reconstructed from it is the // log author's instruction, not this CLI's. `commentBlock` is `x pr review`'s fence. fix: commentBlock(`ci-log ${name}`, [finding.fix]).join('\n'), @@ -152,12 +154,17 @@ async function inspect( /** * The same block can be reached twice — once per job when the log attributes nothing, once per - * attempt when a run was re-run — and two copies of one finding read as two problems. + * attempt when a run was re-run — and two copies of one finding read as two problems. The + * inverse is a finding with the same code and cause but a different `at` or `meta.step` (a + * `X_COVERAGE_BELOW` routed to two packages, the same gate step failing twice for different + * tests): they are NOT the same finding, and the key has to take every locator the gate printed + * to tell them apart. `findingKey` is the same formula `ci-log`'s per-log dedupe uses, so a + * finding that survives one cannot be silently merged by the other. */ function dedupe(findings: readonly Finding[]): readonly Finding[] { const seen = new Set(); return findings.filter((finding) => { - const key = `${finding.code} ${finding.cause}`; + const key = findingKey(finding); if (seen.has(key)) return false; seen.add(key); return true; diff --git a/packages/cli/src/output.test.ts b/packages/cli/src/output.test.ts index 54cb48005..dd2f2a759 100644 --- a/packages/cli/src/output.test.ts +++ b/packages/cli/src/output.test.ts @@ -1,5 +1,6 @@ import { describe, expect, test } from 'bun:test'; import { ERROR_DOCS_URL, singleLine } from '@ultimat3/core'; +import { findingsFrom } from './ci-log'; import { msg } from './messages'; import type { CommandResult, StepResult, UltimateErrorShape } from './output'; import { @@ -48,6 +49,65 @@ describe('unit · output', () => { ]); }); + // `meta.step` is the second, non-path locator the renderer prints. Precedent is + // `X_VERIFY_STEP_TIMEOUT` (`verify-stalled.ts:180-197`); the merge step name rides on it. + // Bracketed and keyed so `ci-log` keeps `at` on its own without colliding. + test('meta.step renders after the head, keyed and bracketed', () => { + const text = renderFinding({ + code: 'X_VERIFY_MERGE_INCOMPLETE', + cause: 'step "unit" is in no part', + fix: 'bun run verify merge parts/*.json --json', + meta: { step: 'unit' }, + }); + expect(text.split('\n')[0]).toBe('X_VERIFY_MERGE_INCOMPLETE [step=unit]'); + }); + + test('at and meta.step on the same finding render in order', () => { + const text = renderFinding({ + code: 'X_DB_DRIFT', + cause: 'schema differs', + fix: 'x db gen "add publish_at"', + at: 'apps/web/app/pricing/page.tsx', + meta: { step: 'unit' }, + }); + expect(text.split('\n')[0]).toBe('X_DB_DRIFT (apps/web/app/pricing/page.tsx) [step=unit]'); + }); + + // The render above only proves the ORDER; this proves both locators SURVIVE the trip. A + // head carrying `at` and `meta.step` is what `X_VERIFY_STEP_TIMEOUT` writes + // (`verify-stalled.ts:180-197`), so a parser that reads only one of them drops the path an + // agent has to open — with the step-only round-trip below staying green either way. + test('a finding with both locators round-trips across ci-log', () => { + const rendered = renderFinding({ + code: 'X_VERIFY_STEP_TIMEOUT', + cause: 'step "unit" passed its 300000ms deadline', + fix: 'bun test packages/cli/src/output.test.ts', + at: 'packages/cli/src/output.test.ts', + meta: { step: 'unit' }, + }); + const lines = rendered.split('\n').map((t) => ({ text: t, job: 'verify', step: 'unit' })); + const [found] = findingsFrom(lines); + expect(found?.at).toBe('packages/cli/src/output.test.ts'); + expect(found?.meta?.['step']).toBe('unit'); + }); + + // Round-trip across `findingsFrom`: the gate step name a merge gap produces must + // reach `x ci` exactly as the gate wrote it. + test('a finding with only the step locator round-trips across ci-log', () => { + const rendered = renderFinding({ + code: 'X_VERIFY_MERGE_INCOMPLETE', + cause: 'step "unit" is in no part', + fix: 'bun run verify merge parts/*.json --json', + docs: 'https://error-docs', + meta: { step: 'unit' }, + }); + const lines = rendered.split('\n').map((t) => ({ text: t, job: 'verify', step: 'unit' })); + // ci-log lives in the same package as output; the function it exports is the parser. + const [found] = findingsFrom(lines); + expect(found?.meta?.['step']).toBe('unit'); + expect(found?.at).toBeUndefined(); + }); + test('the human render carries every step and every fix line', () => { const text = renderHuman(failing); expect(text).toContain('✓ typecheck'); diff --git a/packages/cli/src/output.ts b/packages/cli/src/output.ts index db014514d..eb16b2eed 100644 --- a/packages/cli/src/output.ts +++ b/packages/cli/src/output.ts @@ -21,7 +21,12 @@ export interface Finding { readonly cause: string; readonly fix: string; readonly docs?: string; - /** Optional locator: a file, route, or table the finding is about. */ + /** + * Optional locator: a path an agent can OPEN — a source file (its path from the repo root) or + * a table. A gate step name, a route URL, or any other identifier that is not a file is NOT + * here: it belongs on `meta` (e.g. `meta.step`), and the renderer prints it in the head where + * `ci-log` can recover it. Set iff the finding is about a file the author can edit. + */ readonly at?: string; /** * Where the finding's TEXT came from, when not from this process. `'ci-log'` is `x ci` reading @@ -172,7 +177,18 @@ export function renderFinding(finding: Finding, indent = ''): string { // Every field through `singleLine`: this is the format's only renderer for the terminal and CI // logs, and a `cause` carrying a newline would print a line a reader takes for a real finding. const code = singleLine(finding.code); - const head = finding.at === undefined ? code : `${code} (${singleLine(finding.at)})`; + // The path-style locator (`at`) keeps its parenthesised form: `CODE (path)`. A non-path locator + // — today only `meta.step`, the gate step name — is printed AFTER, bracketed and keyed + // (`[step=lint]`), so `ci-log`'s existing `LOCATOR = /\(([^()]+)\)$/` still means `at` and a + // matching step regex can recover it without colliding. Either side may be absent; when BOTH + // are absent the head is the bare `CODE` it always was. + const stepValue = + typeof finding.meta === 'object' && finding.meta !== null && !Array.isArray(finding.meta) + ? (finding.meta as { readonly [key: string]: JsonValue })['step'] + : undefined; + const stepLabel = typeof stepValue === 'string' ? ` [step=${singleLine(stepValue)}]` : ''; + const pathLabel = finding.at === undefined ? '' : ` (${singleLine(finding.at)})`; + const head = `${code}${pathLabel}${stepLabel}`; const lines = [ `${indent}${head}`, `${indent} cause: ${singleLine(finding.cause)}`, diff --git a/packages/cli/src/prerender-islands.test.ts b/packages/cli/src/prerender-islands.test.ts index 747b4e123..3c3f6e18a 100644 --- a/packages/cli/src/prerender-islands.test.ts +++ b/packages/cli/src/prerender-islands.test.ts @@ -189,9 +189,15 @@ describe('x build --target static, with islands', () => { expect((await readStaticReport(ROOT))?.unmeasured).toEqual(report.unmeasured); const { manifest } = await appManifest(ROOT); - const findings = checkBudgets(manifest, await readBuildStats(ROOT), report.unmeasured); + // The locator is the page file the route declares — the file an agent opens — and the route + // itself rides in `meta.route`, so nothing the finding used to say is lost from `--json`. + const FLEET = 'apps/web/app/fleet/page.tsx'; + const findings = checkBudgets(manifest, await readBuildStats(ROOT), report.unmeasured, (url) => + url === '/fleet' ? FLEET : undefined, + ); expect(findings.map((finding) => finding.code)).toEqual(['X_ISLAND_PROPS_INVALID']); - expect(findings[0]?.at).toBe('/fleet'); + expect(findings[0]?.at).toBe(FLEET); + expect(findings[0]?.meta?.['route']).toBe('/fleet'); expect(findings[0]?.cause).toBe(entry?.cause); expect(findings[0]?.fix).toBe(entry?.fix); // Without the report, the same route is the generic finding it always was: the promotion @@ -205,8 +211,10 @@ describe('x build --target static, with islands', () => { await Bun.write(join(ROOT, 'app.config.ts'), "export const config = { name: 'fixture' };\n"); const outcome = await step?.run({ ...VERIFY_CTX, root: ROOT }); expect(outcome?.ok).toBe(false); - const own = outcome?.findings.filter((finding) => finding.at === '/fleet'); + // Off the files this build wrote, the step resolves the same declaring page itself. + const own = outcome?.findings.filter((finding) => finding.at === FLEET); expect(own?.map((finding) => finding.code)).toEqual(['X_ISLAND_PROPS_INVALID']); + expect(own?.map((finding) => finding.meta?.['route'])).toEqual(['/fleet']); }); test('an island over the route budget trips X_BUDGET_EXCEEDED naming the island file', async () => { diff --git a/packages/cli/src/static-report.ts b/packages/cli/src/static-report.ts index f3da31bb0..8bd9918db 100644 --- a/packages/cli/src/static-report.ts +++ b/packages/cli/src/static-report.ts @@ -57,7 +57,11 @@ export type SkippedRoute = RouteFacts & { * re-exports the name for its own callers. */ export type UnmeasuredRoute = { - /** The DECLARED path, as `X_BUDGET_UNMEASURED`'s `at:` spells it, so the two rows join. */ + /** + * The DECLARED path, the way the finding about it spells the route: `X_BUDGET_UNMEASURED`'s + * `meta.route` and its `cause`, so the report row and the gate's finding join. NOT its `at:` — + * `Finding.at` is the file an agent opens, and the declaring page is what that holds. + */ readonly path: string; readonly reason: string; /** diff --git a/packages/cli/src/verify-merge.test.ts b/packages/cli/src/verify-merge.test.ts index 9f022feeb..21848b1d4 100644 --- a/packages/cli/src/verify-merge.test.ts +++ b/packages/cli/src/verify-merge.test.ts @@ -74,13 +74,19 @@ describe('unit · x verify merge refuses an incomplete gate', () => { { command: 'bun run verify' }, ); const absent = (result.steps ?? []).flatMap((step) => step.findings); - expect(absent.map((finding) => finding.at)).toEqual(['lint', 'unit', 'live']); + // The gate step name moved off `at` (a path an agent can open) onto `meta.step` — the + // same carrier `X_VERIFY_STEP_TIMEOUT` already uses (`verify-stalled.ts:180-197`). + expect(absent.map((finding) => finding.meta?.['step'])).toEqual(['lint', 'unit', 'live']); for (const finding of absent) { expect(finding.code).toBe('X_VERIFY_MERGE_INCOMPLETE'); expect(finding.cause).toContain('never ran or never uploaded'); expect(finding.cause).toContain('not a failure it reported'); expect(finding.fix).toStartWith('bun run verify merge parts/*.json --json'); expect(finding.fix).toContain('gh run rerun --failed'); + // The step name is on `meta.step`, and `at` is NOT a gate step name any more — its + // absence is the second assertion: the old shape cannot come back across a field. + expect(finding.at).toBeUndefined(); + expect(finding.meta?.['step']).toBeDefined(); } }); diff --git a/packages/cli/src/verify-merge.ts b/packages/cli/src/verify-merge.ts index 359956cf2..7ffc0998f 100644 --- a/packages/cli/src/verify-merge.ts +++ b/packages/cli/src/verify-merge.ts @@ -128,8 +128,13 @@ export function parsePart(file: string, text: string, command: string = GATE_COM }; } -/** A gap finding, with a fix spelled for the entry that raised it. */ -type Gap = (cause: string, at?: string) => Finding; +/** + * A gap finding, with a fix spelled for the entry that raised it. The second positional is the + * gate `step` name the merge is naming — matching `X_VERIFY_STEP_TIMEOUT`'s precedent at + * `verify-stalled.ts:180-197`, where a timeout finding carries the step name in `meta.step`, + * never in `at` (which is, repo-wide, a path an agent can open — `output.ts:24-25`). + */ +type Gap = (cause: string, step: string, at?: string) => Finding; /** * The gap kinds a merge can find. `present` is a document that arrived and disagrees with the @@ -149,13 +154,17 @@ const NEVER_ARRIVED = const gapsFor = (command: string): Gaps => { const finding = (fix: string): Gap => - (cause, at) => ({ - code: 'X_VERIFY_MERGE_INCOMPLETE', - cause, - fix, - docs: ERROR_DOCS_URL, - ...(at === undefined ? {} : { at }), - }); + (cause, step, at) => { + const meta: { readonly step: string } = { step }; + return { + code: 'X_VERIFY_MERGE_INCOMPLETE', + cause, + fix, + docs: ERROR_DOCS_URL, + ...(at === undefined ? {} : { at }), + meta, + }; + }; return { present: finding( `${command} merge parts/*.json --json # after every CI job uploaded its part`, @@ -354,7 +363,12 @@ export function mergeParts( part.steps .filter((step) => !declared.includes(step.name)) .map((step) => - kinds.present(`${part.file} reports "${step.name}", which x verify does not run`), + // The unknown step's own NAME rides on `meta.step` — it is the only locator here, + // never a path. `part.file` is already in the cause. + kinds.present( + `${part.file} reports "${step.name}", which x verify does not run`, + step.name, + ), ), ); const steps = declared.map((name) => diff --git a/wiki/CLI-Reference.md b/wiki/CLI-Reference.md index a51ab6e3a..c69096bca 100644 --- a/wiki/CLI-Reference.md +++ b/wiki/CLI-Reference.md @@ -19,7 +19,7 @@ x version # CLI version | Global flags | `--json` / `-j`, `--help` / `-h`, `--cwd `, `--verbose` — accepted by every command. `--verbose` carries each step's captured output on a green run too, in the terminal and under `--json` alike (`steps[].output`); without it a step's output appears only when it fails. `As of 2026-10` | | Subcommands | when a command has them, its default is **declared**, never positional: `x actions` is `x actions list` because that command sets `defaultSubcommand: 'list'`. A command with no defensible default declares none and refuses the bare form with `X_CLI_BAD_FLAG` — exactly `db` and `mcp`, pinned by `parse.test.ts`. `--help` is read before the subcommand, so `x db --help` and `x mcp --help` print usage like `x help db`. The parser answered `subcommands[0]` until 1.2.0, which is why `x db` used to run the migration **generator** | | Passthrough | a bare `--` sends everything after it to the underlying tool untouched | -| `--json` shape | `{ ok, command, summary, steps?, findings?, data? }`. Findings are `{ code, cause, fix, docs?, at? }` | +| `--json` shape | `{ ok, command, summary, steps?, findings?, data? }`. Findings are `{ code, cause, fix, docs?, at?, meta? }` | ## Command index @@ -668,14 +668,21 @@ $ x verify --json {"ok":false,"command":"verify", "summary":"1 of 20 steps failed — 3 skipped: e2e, contract-diff, roadmap","steps":[ {"name":"budgets","ok":false,"durationMs":812,"skipped":false,"findings":[ - {"code":"X_BUDGET_EXCEEDED","cause":"site/pricing ships 61kb of JS, over the 40kb budget", - "fix":"x fix boundary site/pricing/page.tsx", - "docs":"https://github.com/developerz-ai/ultimate/wiki/Error-Codes","at":"site/pricing"}]}], + {"code":"X_BUDGET_EXCEEDED","cause":"/pricing ships 61.2kb of JS (minified, uncompressed) over a 40kb budget via site/pricing/page.tsx -> islands/post-form-04f75f18.js; charged: /islands/post-form-04f75f18.js 50123 B, inline scripts 855 B", + "fix":"edit apps/web/site/pricing/page.tsx — set budget: { js: '62kb' } with // measured: 62682 B (x build --target static) — why: directly above it; or x routes --json for the chain and move the heavy import behind hydrate: 'interaction'", + "docs":"https://github.com/developerz-ai/ultimate/wiki/Error-Codes", + "at":"apps/web/site/pricing/page.tsx", + "meta":{"jsBytes":62682,"charged":[{"url":"/islands/post-form-04f75f18.js","bytes":50123}],"inlineJsBytes":855}}]}], "data":{"failed":["budgets"],"skipped":["e2e","contract-diff","roadmap"],"durationMs":11153}} ``` -A finding is `code`, `cause`, `fix`, and optionally `docs`, `at` (a file, route or table) and -`meta` — structured facts behind `cause`, never the only home of one. `X_VERIFY_STEP_TIMEOUT` +A finding is `code`, `cause`, `fix`, and optionally `docs`, `at` and `meta`. `at` is the +path an agent opens — a file, or the stuck test file for a timed-out step — and is absent +when there is no such path. `meta` is structured facts behind `cause`, never the only home of +one: an unmeasured budget carries its route in `meta.route` (`X_BUDGET_UNMEASURED` — `at` names +the file that declares it), while `X_BUDGET_EXCEEDED` carries what the bytes were charged as +(`meta.jsBytes`, `meta.charged`, `meta.inlineJsBytes`) and spells the route in `cause`. +`X_VERIFY_STEP_TIMEOUT` carries `meta.step`, `meta.deadlineMs`, `meta.killed: [{pid, command}]` and `meta.inFlight: [{command, files, workers, stuck}]`; `at` is the stuck test file when one could be named, and the failed step's `output` is the tail of what the killed run had printed.