diff --git a/packages/cli/src/commands/review/findings.test.ts b/packages/cli/src/commands/review/findings.test.ts index 1e2f54e674c..e193cd9742c 100644 --- a/packages/cli/src/commands/review/findings.test.ts +++ b/packages/cli/src/commands/review/findings.test.ts @@ -4,7 +4,7 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { mkdtempSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; @@ -19,6 +19,8 @@ import { validateOutcomes, type Finding, type FindingsReport, + holdCriticalsFailingOnBase, + sharedFailingFilesOf, } from './findings.js'; /** A minimal valid finding, spread-and-overridden per case. */ @@ -457,6 +459,248 @@ describe('findings (command boundary)', () => { return JSON.parse(readFileSync(out, 'utf8')) as FindingsReport; } + /** Run the handler and return everything it wrote to stderr. */ + function runCapturingStderr(argv: Record): string { + let out = ''; + const spy = vi + .spyOn(process.stderr, 'write') + .mockImplementation((chunk: string | Uint8Array) => { + out += + typeof chunk === 'string' ? chunk : Buffer.from(chunk).toString(); + return true; + }); + try { + (findingsCommand.handler as (a: unknown) => void)(argv); + } finally { + spy.mockRestore(); + } + return out; + } + + it('announces every hold, naming the finding and the measured file', () => { + // A severity this command lowered is a change to what the review says. Left + // unannounced it reads as the reviewer's own judgement, which is the one + // thing the measurement is not. + const input = join(dir, 'in.json'); + const out = join(dir, 'findings.json'); + const delta = join(dir, 'test-delta.json'); + writeFileSync( + input, + JSON.stringify([ + { + ...base, + id: 'R1-3', + severity: 'Critical', + failureScenario: + 'packages/cli/src/ui/auth/AuthDialog.test.tsx goes red on this change.', + }, + ]), + ); + writeFileSync( + delta, + JSON.stringify({ + entries: [ + { + command: 'npm test --workspace="packages/cli"', + netNew: [], + shared: ['src/ui/auth/AuthDialog.test.tsx'], + }, + ], + }), + ); + const stderr = runCapturingStderr({ + input, + out, + testDelta: delta, + print: false, + }); + expect(stderr).toContain('R1-3'); + expect(stderr).toContain('packages/cli/src/ui/auth/AuthDialog.test.tsx'); + expect(stderr).toContain('held back from Critical'); + }); + + it('says nothing about holds or unreadable measurements without the flag', () => { + // Distinguishes "the guard did not run" from "the guard ran and its read + // failed": both leave severities untouched, and only stderr tells them + // apart. + const input = join(dir, 'in.json'); + writeFileSync(input, JSON.stringify([{ ...base, severity: 'Critical' }])); + const stderr = runCapturingStderr({ + input, + out: join(dir, 'findings.json'), + print: false, + }); + expect(stderr).not.toContain('held back'); + expect(stderr).not.toContain('no holds applied'); + }); + + it('holds a Critical back when --test-delta measured its test as failing on base', () => { + // Through the handler, not the helper: the option has to be parsed, the + // artifact read, and the held finding written into the report. The + // test-delta shape here is the one the command emits — a top-level + // `shared` beside per-command entries. + const input = join(dir, 'in.json'); + const out = join(dir, 'findings.json'); + const delta = join(dir, 'test-delta.json'); + writeFileSync( + input, + JSON.stringify([ + { + ...base, + severity: 'Critical', + failureScenario: + 'packages/cli/src/ui/auth/AuthDialog.test.tsx goes red on this change.', + }, + ]), + ); + writeFileSync( + delta, + JSON.stringify({ + entries: [ + { + command: 'npm test --workspace="packages/cli"', + netNew: [], + shared: ['src/ui/auth/AuthDialog.test.tsx'], + }, + ], + netNew: [], + shared: ['src/ui/auth/AuthDialog.test.tsx'], + }), + ); + (findingsCommand.handler as (a: unknown) => void)({ + input, + out, + testDelta: delta, + print: false, + }); + const report = JSON.parse(readFileSync(out, 'utf8')) as FindingsReport; + expect(report.findings[0].severity).toBe('Suggestion'); + expect(report.counts.bySeverity['Critical']).toBe(0); + expect(report.findings[0].failureScenario).toContain('failed there too'); + }); + + it.each([ + ['a path that does not exist', undefined], + ['a file that is not valid JSON', '{ "shared": ['], + ])('still writes the findings when --test-delta is %s', (_name, contents) => { + // Through the command, because that is where the guarantee lives: the + // helper tolerates a wrong SHAPE, but the read itself throws on these + // two, and a cross-check that cannot read its input must not take the + // findings down with it. + const input = join(dir, 'in.json'); + const out = join(dir, 'findings.json'); + const delta = join(dir, 'missing/test-delta.json'); + if (contents !== undefined) { + writeFileSync(join(dir, 'bad.json'), contents); + } + writeFileSync(input, JSON.stringify([{ ...base, severity: 'Critical' }])); + expect(() => + (findingsCommand.handler as (a: unknown) => void)({ + input, + out, + testDelta: contents === undefined ? delta : join(dir, 'bad.json'), + print: false, + }), + ).not.toThrow(); + const report = JSON.parse(readFileSync(out, 'utf8')) as FindingsReport; + expect(report.counts.total).toBe(1); + // Unheld: an absent measurement contradicts nothing. + expect(report.findings[0].severity).toBe('Critical'); + }); + + it('says nothing when the measurement file simply is not there', () => { + // test-delta only runs when a test command failed AND a base tree built, + // so on the ordinary review the artifact does not exist. The SKILL passes + // the flag unconditionally, and a loud line here would report the normal + // path as a failure on every green run. + const input = join(dir, 'in.json'); + writeFileSync(input, JSON.stringify([{ ...base, severity: 'Critical' }])); + const stderr = runCapturingStderr({ + input, + out: join(dir, 'findings.json'), + testDelta: join(dir, 'never-written.json'), + print: false, + }); + expect(stderr).not.toContain('no holds applied'); + expect(stderr).not.toContain('ENOENT'); + }); + + it('still speaks up for a measurement that exists and will not parse', () => { + const input = join(dir, 'in.json'); + const delta = join(dir, 'broken.json'); + writeFileSync(input, JSON.stringify([{ ...base, severity: 'Critical' }])); + writeFileSync(delta, '{ "shared": ['); + const stderr = runCapturingStderr({ + input, + out: join(dir, 'findings.json'), + testDelta: delta, + print: false, + }); + expect(stderr).toContain('no holds applied'); + }); + + it('counts the holds and reaches the same severity with and without outcomes', () => { + const input = join(dir, 'in.json'); + const delta = join(dir, 'test-delta.json'); + writeFileSync( + input, + JSON.stringify([ + { + ...base, + id: 'R1-1', + severity: 'Critical', + failureScenario: + 'packages/cli/src/ui/auth/AuthDialog.test.tsx goes red on this change.', + }, + ]), + ); + writeFileSync( + delta, + JSON.stringify({ + entries: [ + { + command: 'npm test --workspace="packages/cli"', + netNew: [], + shared: ['src/ui/auth/AuthDialog.test.tsx'], + }, + ], + }), + ); + const outcomes = join(dir, 'outcomes.json'); + writeFileSync(outcomes, JSON.stringify([{ id: 'R1-1', outcome: 'fixed' }])); + + const first = join(dir, 'a.json'); + (findingsCommand.handler as (a: unknown) => void)({ + input, + out: first, + testDelta: delta, + print: false, + }); + const second = join(dir, 'b.json'); + (findingsCommand.handler as (a: unknown) => void)({ + input, + out: second, + outcomes, + testDelta: delta, + print: false, + }); + const a = JSON.parse(readFileSync(first, 'utf8')) as FindingsReport; + const b = JSON.parse(readFileSync(second, 'utf8')) as FindingsReport; + expect(a.counts.held).toBe(1); + expect(b.counts.held).toBe(1); + // The whole point: one input, one measurement, one answer. + expect(b.findings[0].severity).toBe(a.findings[0].severity); + expect(b.findings[0].heldByMeasurement).toEqual( + a.findings[0].heldByMeasurement, + ); + }); + + it('leaves severities alone when --test-delta is not passed', () => { + const report = run([{ ...base, severity: 'Critical' }]); + expect(report.findings[0].severity).toBe('Critical'); + expect(report.counts.bySeverity['Critical']).toBe(1); + }); + it('writes the artifact, creating intermediate directories', () => { const report = run([base]); expect(report.counts.total).toBe(1); @@ -512,7 +756,414 @@ describe('findings (command boundary)', () => { }); }); +describe('holdCriticalsFailingOnBase', () => { + // The shape test-delta writes: workspace-relative paths, while a finding + // names the repo-relative one. + // Repo-relative, which is what `sharedFailingFilesOf` hands the helper: it + // qualifies each entry's paths from that entry's `--workspace=`. + const shared = ['packages/cli/src/ui/auth/AuthDialog.test.tsx']; + const critical = { + id: 'f1', + severity: 'Critical' as const, + confidence: 'high' as const, + source: 'review' as const, + summary: 'Height-based pagination breaks AuthDialog.test.tsx', + shortSummary: 'pagination breaks a test', + failureScenario: + "packages/cli/src/ui/auth/AuthDialog.test.tsx > 'drives API key provider steps' expects MiniMax visible without scrolling.", + locations: [{ file: 'packages/cli/src/ui/auth/AuthDialog.tsx', line: 132 }], + }; + + it('holds a Critical that blames the PR for a test the base also fails', () => { + const { findings, held } = holdCriticalsFailingOnBase([critical], shared); + expect(findings[0].severity).toBe('Suggestion'); + expect(findings[0].failureScenario).toContain('failed there too'); + // The original scenario survives — the measurement is added to the + // evidence, it does not replace it. + expect(findings[0].failureScenario).toContain('expects MiniMax visible'); + expect(held).toEqual([ + { id: 'f1', file: 'packages/cli/src/ui/auth/AuthDialog.test.tsx' }, + ]); + }); + + it('leaves a Critical alone when the test it names is not shared', () => { + const { findings, held } = holdCriticalsFailingOnBase( + [critical], + ['packages/cli/src/other/unrelated.test.ts'], + ); + expect(findings[0].severity).toBe('Critical'); + expect(findings[0].failureScenario).toBe(critical.failureScenario); + expect(held).toEqual([]); + }); + + it('holds the same finding whether or not the fixer already applied it', () => { + // The SKILL runs this command twice over one input, the second time with + // --outcomes. An exemption for `fixed` made run 2 answer Critical where + // run 1 answered Suggestion — the same finding at two severities inside + // one review, which this file's header names as the failure it exists to + // prevent. The two statements are about different things: the measurement + // says the base was already red, the outcome says the tree was edited. + const plain = holdCriticalsFailingOnBase([critical], shared); + const fixed = holdCriticalsFailingOnBase( + [{ ...critical, outcome: 'fixed' as const }], + shared, + ); + expect(fixed.findings[0].severity).toBe(plain.findings[0].severity); + expect(fixed.held).toEqual(plain.held); + }); + + it('leaves a re-filed Critical alone once it already carries the measurement', () => { + // The escape the report offers — "say which test fails for a NEW reason" — + // names the test file, which IS the match condition, so re-applying the + // measurement would make the promised door unopenable. The ledger carries a + // held finding forward as the Suggestion it became, so Critical plus the + // marker is a deliberate act by someone who read it. + const once = holdCriticalsFailingOnBase([critical], shared).findings[0]; + expect(once.severity).toBe('Suggestion'); + + const again = holdCriticalsFailingOnBase( + [{ ...once, severity: 'Critical' as const }], + shared, + ); + expect(again.findings[0].severity).toBe('Critical'); + expect(again.held).toEqual([]); + expect(again.readjudicated).toEqual([ + { id: 'f1', file: 'packages/cli/src/ui/auth/AuthDialog.test.tsx' }, + ]); + // ...and the explanation is not written twice. + const count = (t: string) => + (t.match(/Held back from Critical by measurement:/g) ?? []).length; + expect(count(again.findings[0].failureScenario)).toBe(1); + }); + + it('records the hold as a field, not only as prose', () => { + // A later round reads the artifact. A hold discoverable only by + // substring-matching the scenario is a hold the round ledger cannot see. + const { findings } = holdCriticalsFailingOnBase([critical], shared); + expect(findings[0].heldByMeasurement).toEqual({ + file: 'packages/cli/src/ui/auth/AuthDialog.test.tsx', + }); + }); + + it('does not demote a finding whose subject IS the already-red test', () => { + // "this new assertion checks the wrong thing" names the test file as its + // location, and a PR touching an already-red test is exactly when such a + // finding gets written. The measurement says nothing about that claim. + const { findings } = holdCriticalsFailingOnBase( + [ + { + ...critical, + summary: 'The new assertion asserts the wrong property', + failureScenario: + 'It asserts `visible` where the contract is `enabled`, so a regression that flips enabled ships green.', + locations: [ + { + file: 'packages/cli/src/ui/auth/AuthDialog.test.tsx', + line: 40, + }, + ], + }, + ], + shared, + ); + expect(findings[0].severity).toBe('Critical'); + }); + + it('does not match on suggestedFix, where a test file is proposed work', () => { + // "add a case in src/…test.tsx" is not a claim that the file is red. + const { findings } = holdCriticalsFailingOnBase( + [ + { + ...critical, + summary: 'The retry counter is never reset', + failureScenario: 'Two failures then a success leaves attempts at 2.', + locations: [{ file: 'packages/cli/src/retry.ts' }], + suggestedFix: + 'Add a case in packages/cli/src/ui/auth/AuthDialog.test.tsx covering the guard.', + }, + ], + shared, + ); + expect(findings[0].severity).toBe('Critical'); + }); + + it('never touches a finding that is not Critical', () => { + const { findings, held } = holdCriticalsFailingOnBase( + [{ ...critical, severity: 'Suggestion' as const }], + shared, + ); + expect(findings[0].severity).toBe('Suggestion'); + expect(findings[0].failureScenario).toBe(critical.failureScenario); + expect(held).toEqual([]); + }); + + it('does not match a nested copy of the same tree', () => { + // `/` is not a leading boundary. The probe reaches here repo-relative, so + // anything in front of it means the match sits under another root: a + // vendored copy is not this file, and demoting a Critical about the real + // one on that basis is the cross-tree collapse in miniature. + const { findings, held } = holdCriticalsFailingOnBase( + [ + { + ...critical, + summary: 'the assertion is wrong', + failureScenario: + 'third_party/packages/cli/src/ui/auth/AuthDialog.test.tsx is red', + }, + ], + shared, + ); + expect(findings[0].severity).toBe('Critical'); + expect(held).toEqual([]); + }); + + it('matches a path that ends a sentence', () => { + // Findings are prose. Treating the full stop as part of the name would + // silently stop matching the ordinary way a file is written. + const { findings } = holdCriticalsFailingOnBase( + [ + { + ...critical, + failureScenario: + 'It goes red in packages/cli/src/ui/auth/AuthDialog.test.tsx.', + locations: [{ file: 'packages/cli/src/ui/auth/AuthDialog.tsx' }], + }, + ], + shared, + ); + expect(findings[0].severity).toBe('Suggestion'); + }); + + it('does not match a longer extension on the same stem', () => { + const { findings } = holdCriticalsFailingOnBase( + [ + { + ...critical, + failureScenario: + 'packages/cli/src/ui/auth/AuthDialog.test.tsx.snap is stale', + locations: [{ file: 'packages/cli/src/ui/auth/AuthDialog.tsx' }], + }, + ], + shared, + ); + expect(findings[0].severity).toBe('Critical'); + }); + + it('requires a boundary after the match, not only before it', () => { + // `src/a.test.ts` sits inside `src/a.test.tsx`, and the leading check + // cannot see it: both are preceded by `/`. + const { findings } = holdCriticalsFailingOnBase( + [ + { + ...critical, + failureScenario: + 'packages/cli/src/ui/auth/AuthDialog.test.tsx is red', + locations: [{ file: 'packages/cli/src/ui/auth/AuthDialog.tsx' }], + }, + ], + ['packages/cli/src/ui/auth/AuthDialog.test.ts'], + ); + expect(findings[0].severity).toBe('Critical'); + }); + + it('requires a path boundary, so a longer directory name is not a match', () => { + const { findings } = holdCriticalsFailingOnBase( + [ + { + ...critical, + failureScenario: + 'vendor/other-src/ui/auth/AuthDialog.test.tsx is red', + locations: [{ file: 'packages/cli/src/ui/auth/AuthDialog.tsx' }], + }, + ], + shared, + ); + expect(findings[0].severity).toBe('Critical'); + }); +}); + +describe('sharedFailingFilesOf — cross-workspace identity', () => { + // The artifact test-delta actually writes for this repo: one entry per + // workspace command, paths relative to that workspace. + const delta = { + entries: [ + { + command: 'npm test --workspace="packages/cli"', + netNew: [], + shared: ['src/utils/errors.test.ts'], + }, + { + command: 'npm test --workspace="packages/core"', + netNew: ['src/utils/errors.test.ts'], + shared: [], + }, + ], + netNew: ['src/utils/errors.test.ts'], + shared: ['src/utils/errors.test.ts'], + }; + + it('qualifies each entry by the workspace its command names', () => { + expect(sharedFailingFilesOf(delta).shared).toEqual([ + 'packages/cli/src/utils/errors.test.ts', + ]); + }); + + it('does not hold a Critical about the OTHER workspace of the same path', () => { + // Six test paths in this repo exist under both packages/cli/src and + // packages/core/src. A bare suffix would demote a real finding about + // core's copy because cli's copy was already red. + const { shared } = sharedFailingFilesOf(delta); + const critical = { + id: 'f1', + severity: 'Critical' as const, + confidence: 'high' as const, + source: 'review' as const, + summary: 'this PR breaks core errors', + shortSummary: 'core errors', + failureScenario: 'packages/core/src/utils/errors.test.ts goes red.', + locations: [{ file: 'packages/core/src/utils/errors.ts' }], + }; + expect( + holdCriticalsFailingOnBase([critical], shared).findings[0].severity, + ).toBe('Critical'); + // ...while the workspace it WAS measured in is still held. + const cli = { + ...critical, + failureScenario: 'packages/cli/src/utils/errors.test.ts goes red.', + }; + expect(holdCriticalsFailingOnBase([cli], shared).findings[0].severity).toBe( + 'Suggestion', + ); + }); + + it('drops a file some other command measured as net-new', () => { + expect( + sharedFailingFilesOf({ + entries: [ + { command: 'npm test', netNew: [], shared: ['src/a.test.ts'] }, + { command: 'npm test', netNew: ['src/a.test.ts'], shared: [] }, + ], + }).shared, + ).toEqual([]); + }); + + it('refuses a project-keyed path it cannot place in a workspace', () => { + // The shape the producer can actually emit: `failingFilesOf` writes + // `project::path` only when the runner prints a project tag, and a + // `--workspace=` command never does (neither vitest config here names a + // project), so a key always arrives on a bare `npm test`. Stripping it + // would leave a project-relative path that matches as a suffix of any + // directory — the cross-project collapse, from the consumer side. + const { shared, unidentifiable } = sharedFailingFilesOf({ + entries: [ + { + command: 'npm test', + netNew: [], + shared: [ + '@qwen-code/qwen-code::src/utils/errors.test.ts', + 'src/plain.test.ts', + ], + }, + ], + }); + expect(shared).toEqual(['src/plain.test.ts']); + expect(unidentifiable).toEqual([ + '@qwen-code/qwen-code::src/utils/errors.test.ts', + ]); + }); + + it('does not demote a Critical on a path it refused to place', () => { + const { shared } = sharedFailingFilesOf({ + entries: [ + { + command: 'npm test', + netNew: [], + shared: ['@qwen-code/qwen-code::src/utils/errors.test.ts'], + }, + ], + }); + const critical = { + id: 'f1', + severity: 'Critical' as const, + confidence: 'high' as const, + source: 'review' as const, + summary: 'this PR breaks core errors', + shortSummary: 'core errors', + failureScenario: 'packages/core/src/utils/errors.test.ts goes red.', + locations: [{ file: 'packages/core/src/utils/errors.ts' }], + }; + expect( + holdCriticalsFailingOnBase([critical], shared).findings[0].severity, + ).toBe('Critical'); + }); + + it('holds nothing from an artifact with no entries to qualify against', () => { + // The top-level list is the union of the entries with the workspace + // context already lost. Honouring it would put back the bare path that + // matches inside any package — the collapse, through the last open door. + expect( + sharedFailingFilesOf({ shared: ['src/utils/errors.test.ts'] }).shared, + ).toEqual([]); + }); + + it('reports only shared paths it had to set aside, not net-new ones', () => { + // A net-new file was never eligible to hold anything back, so dropping it + // set nothing aside — and on a plain `npm test` with vitest projects, most + // keyed entries are net-new. + const { unidentifiable } = sharedFailingFilesOf({ + entries: [ + { + command: 'npm test', + netNew: ['proj::src/n.test.ts'], + shared: ['proj::src/s.test.ts'], + }, + ], + }); + expect(unidentifiable).toEqual(['proj::src/s.test.ts']); + }); +}); + +describe('sharedFailingFilesOf', () => { + it('takes the shared list from the top level and from every entry', () => { + expect( + sharedFailingFilesOf({ + shared: ['src/a.test.ts'], + entries: [ + { shared: ['src/a.test.ts', 'src/b.test.ts'] }, + { shared: ['src/c.test.ts'] }, + ], + }).shared.sort(), + ).toEqual(['src/a.test.ts', 'src/b.test.ts', 'src/c.test.ts']); + }); + + it('yields none for a shape it does not recognise, rather than throwing', () => { + // An unreadable measurement must not hold a Critical back, and must not + // take the review down either. + for (const junk of [null, 42, 'shared', {}, { shared: 'nope' }, []]) { + expect(sharedFailingFilesOf(junk).shared).toEqual([]); + } + }); +}); + describe('validateFindings — the canonical artifact round-trips', () => { + it('keeps heldByMeasurement, so a hold survives being fed back', () => { + // The field exists so a LATER round can see that a measurement lowered + // this finding, and a round reads the artifact by feeding it back through + // --input. Dropped here, the hold survives exactly one command and + // counts.held returns to 0. + const [f] = validateFindings([ + { + ...base, + severity: 'Suggestion', + heldByMeasurement: { file: 'packages/cli/src/a.test.ts' }, + }, + ]); + expect(f.heldByMeasurement).toEqual({ + file: 'packages/cli/src/a.test.ts', + }); + expect(buildReport([f]).counts.held).toBe(1); + }); + it('keeps outcome and outcomeNote when an artifact is fed back through --input', () => { // `validateFindings` accepts `outcome`; dropping the note while keeping the // outcome would strip exactly the field a `skipped` finding owes the reader. diff --git a/packages/cli/src/commands/review/findings.ts b/packages/cli/src/commands/review/findings.ts index 2e73ad8ab25..2cce4eff911 100644 --- a/packages/cli/src/commands/review/findings.ts +++ b/packages/cli/src/commands/review/findings.ts @@ -32,7 +32,7 @@ // coverage is the error. import type { CommandModule } from 'yargs'; -import { readFileSync, writeFileSync, mkdirSync } from 'node:fs'; +import { existsSync, readFileSync, writeFileSync, mkdirSync } from 'node:fs'; import { dirname, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; @@ -95,6 +95,15 @@ export interface Finding { outcome?: Outcome; /** The fixer's reason, carried from the ledger — mainly for `skipped`. */ outcomeNote?: string; + /** + * Set when `--test-delta` lowered this finding's severity: the test file the + * measurement matched. Structured, not prose only — the sibling command makes + * the same argument about its own budget skips, and for the same reason. A + * later round reads the artifact, not the paragraph, so a hold discoverable + * only by substring-matching `failureScenario` is a hold the round ledger + * cannot see, and it re-files the finding at whatever severity it likes. + */ + heldByMeasurement?: { file: string }; } export interface FindingsReport { @@ -105,6 +114,9 @@ export interface FindingsReport { byConfidence: Record; /** Present only once outcomes have been recorded. */ byOutcome?: Record; + /** How many findings a measurement lowered. Counted, not inferred from + * prose, so a later round can act on it. */ + held: number; }; /** True once every finding carries an outcome. */ outcomesRecorded: boolean; @@ -311,6 +323,16 @@ export function validateFindings(raw: unknown): Finding[] { const outcomeNote = asString(o, 'outcomeNote') ?? asString(o, 'outcome_note'); + // And `heldByMeasurement`, for the same reason and with more riding on it: + // the field exists so a later round can see that a measurement lowered this + // finding, and a round reads the artifact by feeding it back through + // `--input`. Dropped here, the hold survives exactly one command. + const heldRaw = (o as { heldByMeasurement?: unknown }).heldByMeasurement; + const heldFile = + heldRaw && typeof heldRaw === 'object' + ? asString(heldRaw as Record, 'file') + : undefined; + const shortSummary = asString(o, 'shortSummary') ?? asString(o, 'short_summary'); @@ -336,6 +358,7 @@ export function validateFindings(raw: unknown): Finding[] { locations: parseLocations(o, i), ...(assetFiles ? { assetFiles } : {}), ...(assets ? { assets } : {}), + ...(heldFile ? { heldByMeasurement: { file: heldFile } } : {}), ...(outcome ? { outcome } : {}), ...(outcome && outcomeNote ? { outcomeNote } : {}), } satisfies Finding; @@ -357,6 +380,229 @@ export function validateFindings(raw: unknown): Finding[] { return findings; } +/** + * Does `text` name `probe`? Both are repo-relative by the time this runs — + * `sharedFailingFilesOf` qualifies what `test-delta` reported. Match on a name + * boundary so `src/a.test.ts` is satisfied by neither `vendor/other-src/a.test.ts` + * nor `src/a.test.tsx`. + */ +function namesPath(text: string, probe: string): boolean { + const isNameChar = (c: string | undefined) => + c !== undefined && /[A-Za-z0-9._-]/.test(c); + // `/` is NOT a leading boundary. Probes reach here repo-relative, so a `/` + // in front means the match sits under some other root — + // `third_party/packages/cli/src/a.test.ts` is a vendored copy, not this + // file, and treating the slash as a boundary demoted a Critical about the + // real one. The trailing side already required both ends; this is the same + // rule on the side that was left open. + const isBoundary = (c: string | undefined) => + c === undefined || (!isNameChar(c) && c !== '/'); + let from = 0; + for (;;) { + const at = text.indexOf(probe, from); + if (at < 0) return false; + const after = text[at + probe.length]; + // A trailing `.` is the end of a sentence far more often than the start of + // another extension — findings are prose, and "…in src/a.test.ts." is the + // ordinary way to write it. So a dot only extends the name when something + // alphanumeric follows it (`.snap`), and never at the end of the text. + const extendsName = + after !== undefined && + (/[A-Za-z0-9_-]/.test(after) || + (after === '.' && + /[A-Za-z0-9]/.test(text[at + probe.length + 1] ?? ''))); + // Both ends: the leading check alone cannot see a probe matching inside a + // longer name, where `src/a.test.ts` is satisfied by `src/a.test.tsx`. + if (isBoundary(at === 0 ? undefined : text[at - 1]) && !extendsName) { + return true; + } + from = at + 1; + } +} + +/** + * Hold a Critical that blames the PR for a test failure the base tree already + * had — the one contradiction this pipeline measures and then used to ignore. + * + * `test-delta` reruns the PR side's failed test commands on the merge base and + * splits the failures into `netNew` (the PR's own) and `shared` (failing on both + * sides, whatever files the diff touches). A Critical naming a `shared` test file + * is asserting a breakage against a file that was already red without the PR. + * + * Measured on #8368: `AuthDialog.test.tsx` was `shared` in two independent runs, + * and the base tree fails the very same test — `drives API key provider steps + * from endpoint options metadata` — at merge-base `e967cc90`. A Critical reading + * "height-based pagination breaks the pre-existing test" was carried across four + * rounds and into the composed review anyway, because nothing reconciled the two + * artifacts. The path rule this replaced was closed off in `test-delta` itself; + * the round ledger reopened it from the other side. + * + * Downgrade, never drop. The measurement contradicts the SEVERITY — the PR is + * not breaking a passing test — but the finding may still describe something + * real about a test that is red for two reasons. A Suggestion stays in front of + * a human, carrying the measurement that demoted it; a deletion would not. Only + * Critical is touched, and nothing is ever raised. + */ +export function holdCriticalsFailingOnBase( + findings: readonly Finding[], + sharedFailingFiles: readonly string[], +): { + findings: Finding[]; + held: Array<{ id: string; file: string }>; + readjudicated: Array<{ id: string; file: string }>; +} { + const held: Array<{ id: string; file: string }> = []; + const readjudicated: Array<{ id: string; file: string }> = []; + const out = findings.map((f) => { + if (f.severity !== 'Critical') return f; + // `summary` and `failureScenario` only — the two fields where a finding + // states its claim. + // + // `suggestedFix` is where it proposes work ("add a case in src/x.test.ts"), + // and `locations[].file` is its subject. Both name a test file routinely + // without asserting anything about that file being red, and the second is + // the sharper trap: for a finding ABOUT a test's content — "this new + // assertion checks the wrong thing" — the location IS the test file, and a + // PR touching an already-red test is exactly when such a finding gets + // written. Demoting it would use the measurement against a claim the + // measurement says nothing about. + const haystack = [f.summary, f.failureScenario].join('\n'); + const hit = sharedFailingFiles.find((p) => p && namesPath(haystack, p)); + if (!hit) return f; + // Already held for this file, and back at Critical anyway. The ledger + // carries a held finding forward as the Suggestion it became, so Critical + // plus this marker takes a deliberate act: someone read the measurement and + // raised it again. Re-applying the same measurement to the same finding + // adds nothing and silently overrides that decision — and the escape the + // report offers ("say which test fails for a NEW reason") names the test + // file, which is the match condition, so without this the door the + // documentation promises cannot be opened at all. + if (f.heldByMeasurement?.file === hit) { + readjudicated.push({ id: f.id, file: hit }); + return f; + } + held.push({ id: f.id, file: hit }); + return { + ...f, + severity: 'Suggestion' as Severity, + heldByMeasurement: { file: hit }, + failureScenario: `${f.failureScenario}\n\nHeld back from Critical by measurement: \`test-delta\` reran the failing test command on the merge base and ${hit} failed there too, so this is not a passing test the PR turns red. If the PR makes an already-red test fail for a NEW reason, say which test, quote both sides, and file it at Critical again: a finding that already carries this measurement and is raised anyway is left where you put it.`, + }; + }); + return { findings: out, held, readjudicated }; +} + +const WORKSPACE_IN_COMMAND_RE = /--workspace="([^"]+)"/; + +/** + * One `test-delta` path as a finding would write it: repo-relative, unkeyed. + * + * Two things are stripped away. `failingFilesOf` keys a file by its vitest + * project when the runner prints one (`@qwen-code/qwen-code::src/x.test.ts`) — + * a finding never writes that prefix, so a keyed entry could never match and the + * guard no-opped silently on the one shape a real projects run emits. And a + * per-workspace command prints paths relative to that workspace, so + * `src/utils/errors.test.ts` from the `packages/cli` command is qualified back + * to `packages/cli/src/utils/errors.test.ts`. + * + * The qualification is the part that matters. Five test paths in this repo exist + * under BOTH `packages/cli/src` and `packages/core/src` (`utils/errors.test.ts` + * among them), so a bare suffix cannot tell them apart: a Critical about core's + * copy would be held by cli's copy being red — demoting a real finding on a + * measurement that was never about it. `failingFilesOf` keeps the project token + * in its identity for exactly this reason; discarding it here would reopen from + * the consumer side what the producer closed. + */ +function repoRelative( + path: string, + workspace: string | undefined, +): string | undefined { + const at = path.indexOf('::'); + const bare = at < 0 ? path : path.slice(at + 2); + // A key with no workspace to put back cannot be re-qualified, and the bare + // remainder is project-relative: `namesPath` would then match it as a suffix + // of ANY directory, which is the cross-project collapse this whole function + // exists to prevent. The two shapes are mutually exclusive in practice — a + // project tag comes from a runner printing one, and a `--workspace=` command + // in this repo never does — so the qualifying half was never covering for the + // stripping half. Refuse: a measurement whose subject cannot be identified + // licenses nothing, and a missed hold costs less than a demoted Critical. + if (at >= 0 && !workspace) return undefined; + if (!workspace || bare.startsWith(`${workspace}/`)) return bare; + return `${workspace}/${bare}`; +} + +/** + * Every file `test-delta` measured as failing on BOTH sides, repo-relative. + * + * Read from `entries` only, because only an entry carries the `--workspace=` + * its paths are relative to. The top-level `shared` is the union of the same + * files with that context already lost, so it is never honoured — an artifact + * with no entries measured nothing this can safely act on, and a bare + * workspace-relative path matches inside any package. + * + * A file measured `netNew` anywhere in the run is dropped even if some other + * command called it `shared`: the two claims cannot both license a hold, and the + * direction that suppresses a real finding is the worse one to get wrong. + * + * A shape it does not recognise yields none: an unreadable measurement must not + * silently hold a Critical back, and must not throw either — the review has + * findings to report whether or not this file parsed. + */ +export function sharedFailingFilesOf(raw: unknown): { + shared: string[]; + unidentifiable: string[]; +} { + if (!raw || typeof raw !== 'object') + return { shared: [], unidentifiable: [] }; + const shared = new Set(); + const netNew = new Set(); + const unidentifiable = new Set(); + const take = ( + into: Set, + v: unknown, + workspace: string | undefined, + ) => { + if (!Array.isArray(v)) return; + for (const e of v) { + if (typeof e !== 'string' || !e) continue; + const qualified = repoRelative(e, workspace); + // Reported only for `shared`: a `netNew` path was never eligible to hold + // anything back, so nothing was set aside by dropping it, and saying + // otherwise would make the disclosure noise on the very shape (a plain + // `npm test` with vitest projects) that produces most of these keys. + if (qualified === undefined) { + if (into === shared) unidentifiable.add(e); + } else into.add(qualified); + } + }; + + const entries = (raw as { entries?: unknown }).entries; + const rows = Array.isArray(entries) ? entries : []; + if (rows.length > 0) { + for (const e of rows) { + if (!e || typeof e !== 'object') continue; + const command = (e as { command?: unknown }).command; + const workspace = + typeof command === 'string' + ? (WORKSPACE_IN_COMMAND_RE.exec(command)?.[1] ?? undefined) + : undefined; + take(shared, (e as { shared?: unknown }).shared, workspace); + take(netNew, (e as { netNew?: unknown }).netNew, workspace); + } + } + // No `entries` and therefore no `--workspace=` to qualify against. The + // top-level list is the union of the entries with that context already lost, + // so honouring it would put back the bare workspace-relative path that + // matches inside ANY package — the collapse `repoRelative` exists to stop, + // through the one door left open. A real artifact always has entries; + // anything else measured nothing this can safely act on. + return { + shared: [...shared].filter((f) => !netNew.has(f)), + unidentifiable: [...unidentifiable], + }; +} + /** Most severe first, then high-confidence before low, then file and line. */ export function sortFindings(findings: readonly Finding[]): Finding[] { return [...findings].sort((a, b) => { @@ -494,6 +740,7 @@ export function buildReport(findings: readonly Finding[]): FindingsReport { total: sorted.length, bySeverity, byConfidence, + held: sorted.filter((f) => f.heldByMeasurement !== undefined).length, ...(byOutcome ? { byOutcome } : {}), }, outcomesRecorded, @@ -520,6 +767,7 @@ interface FindingsArgs { out: string; outcomes: string | undefined; print: boolean | undefined; + testDelta: string | undefined; } function readJson(path: string, what: string): unknown { @@ -565,12 +813,18 @@ export const findingsCommand: CommandModule = { describe: 'JSON array of {id, outcome, note?} recording what --fix did to each finding. Must cover every finding.', }) + .option('test-delta', { + type: 'string', + describe: + 'The test-delta artifact. A Critical naming a test file that also failed on the merge base is held back to Suggestion, carrying the measurement that demoted it.', + }) .option('print', { type: 'boolean', describe: 'Also print one line per finding to stdout', }), handler: (argv) => { - const { input, out, outcomes, print } = argv as unknown as FindingsArgs; + const { input, out, outcomes, print, testDelta } = + argv as unknown as FindingsArgs; let findings = validateFindings(readJson(input, 'findings')); if (outcomes !== undefined) { @@ -579,6 +833,46 @@ export const findingsCommand: CommandModule = { validateOutcomes(readJson(outcomes, 'outcomes')), ); } + let held: Array<{ id: string; file: string }> = []; + let readjudicated: Array<{ id: string; file: string }> = []; + if (testDelta !== undefined) { + // A measurement that will not read is a measurement, absent — and an + // absent measurement holds nothing back. It must not take the review + // down with it either: the findings are the deliverable, this is a + // cross-check on them. `readJson` throws on a missing file and on + // invalid JSON, so the loud-but-fatal path is caught here and the + // shape-level tolerance in `sharedFailingFilesOf` covers the rest. + // Never silent, though — a hold that did not happen because the file was + // unreadable is exactly what a reader needs told. + // A file that is not there and a file that will not parse are different + // facts, and only the second is worth alarming about. `test-delta` runs + // only when a test command failed AND a base tree was available, so on + // the ordinary review — tests green — the artifact does not exist, and a + // loud line there would report the normal path as a failure on every run. + let measurement: unknown; + if (existsSync(resolve(testDelta))) { + try { + measurement = readJson(testDelta, 'test-delta'); + } catch (err) { + writeStderrLine( + `findings: no holds applied — ${err instanceof Error ? err.message : String(err)}`, + ); + } + } + const { shared, unidentifiable } = sharedFailingFilesOf(measurement); + // Never a silent drop: a measured file this could not place is coverage + // the command chose not to use, and saying so is the difference between + // "nothing matched" and "something was set aside". + for (const f of unidentifiable) { + writeStderrLine( + `findings: ignored the measured file ${f} — it carries a vitest project key and no workspace to resolve it against, so which project it names cannot be established`, + ); + } + ({ findings, held, readjudicated } = holdCriticalsFailingOnBase( + findings, + shared, + )); + } const report = buildReport(findings); const target = resolve(out); @@ -592,6 +886,21 @@ export const findingsCommand: CommandModule = { `${bySeverity['Nice to have']} Nice to have; ` + `${byConfidence['low']} low-confidence. Wrote ${target}`, ); + // Never silent: a severity this command lowered is a change to what the + // review says, and the reader has to be told which finding and on what + // evidence — otherwise the demotion reads as the reviewer's own judgement. + for (const h of held) { + writeStderrLine( + `findings: ${h.id} held back from Critical — test-delta measured ${h.file} as failing on the merge base too`, + ); + } + // A hold that was weighed and reversed is a decision, and a decision this + // command declined to overrule is exactly as reportable as one it made. + for (const r of readjudicated) { + writeStderrLine( + `findings: ${r.id} left at Critical — it already carries the ${r.file} measurement and was raised again anyway`, + ); + } if (report.counts.byOutcome) { const o = report.counts.byOutcome; writeStderrLine( diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index efa1c118ee1..41155bff3d2 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -731,6 +731,29 @@ Run it on a same-repo **PR** review only. A **local** or **file** review has no **None of it blocks, and none of it caps.** A Test Plan defect is not a code defect — the diff is unaffected — and the verdict is about the code. The notes are disclosed in the body on every event including Approve, the same disclosed-but-not-capping treatment a deferred checker gets, and for the same reason: an author cannot fix "you wrote a sentence I could not check", so it must never become a permanent cap. +### The findings, as data + +**Write the findings artifact before you do anything else with them.** Everything that matters in this pipeline is a computed artifact — the diff plan, the coverage report, the resolved anchors, the verdict — and the findings were the one exception: prose in a terminal, re-typed into the Step 8 report, re-typed again into the Step 7 review JSON. Three transcriptions of the same list, and this skill's history is a catalogue of what transcription costs (a Critical that changed severity between two sections of one review; an aggregate that arrived at `resolve-anchors` with its per-location anchors dropped and took the whole batch down). + +Write every confirmed finding — high and low confidence alike — as a JSON array, then: + +```bash +"${QWEN_CODE_CLI:-qwen}" review findings \ + --input .qwen/tmp/qwen-review-{target}-findings-in.json \ + --test-delta .qwen/tmp/qwen-review-{target}-test-delta.json \ + --out .qwen/tmp/qwen-review-{target}-findings.json +``` + +**Pass `--test-delta` on both invocations of this command — the block above and the `--outcomes` one in Step 6B, which already carry it.** `test-delta` runs only when a test command failed and a base tree was available, so on an ordinary green review the artifact is not there, and the command treats a file that is absent as no measurement taken and says nothing. It speaks up only for a file that exists and will not parse, which is a different fact. It holds back to Suggestion any Critical that names a test file `test-delta` measured as failing on the merge base too, and says on stderr which finding and which file. A Critical asserting "this PR breaks test X" against a test that was already red is the misattribution `test-delta` exists to prevent — and the round ledger is the other door into it: measured on #8368, exactly such a Critical was carried across four rounds and into the composed review while the run's own `test-delta` had classified that file `shared` twice. The finding is not deleted, because a test can be red for two reasons at once; it keeps its evidence, gains the measurement that demoted it, and stays in front of a human who can restore it by naming which test fails for a new reason and quoting both sides. + +**One finding, one name.** A high-effort PR review also writes the incremental cache's cross-round `findings` ledger (Step 8), whose ids are `R-` — use those same ids here: a finding that will enter the ledger gets its `R-` as the artifact `id`, and a carried-forward finding keeps the id it already has. Two id schemes for one finding is how "R1-2" in next round's report and "f7" in this round's outcome ledger turn out to be the same defect that nobody can join. + +Each entry carries `id` (unique — outcomes and resolved anchors both join on it), `severity`, `confidence`, `source`, `summary`, `failureScenario`, and either `file`/`line`/`anchor` or, for a pattern aggregate, a `locations[]` array with **one entry per location** (`suggestedFix`, `category` and `shortSummary` are optional; `shortSummary` is derived from `summary` when absent). The command validates the shape, refuses a duplicate id, refuses a finding with no failure scenario, sorts by severity → confidence → file → line → id, and writes counts nobody then recomputes by hand. Read the artifact for the numbers you quote in the Summary. This is a **canonicalization**, not a gate: it does not decide the verdict — `compose-review` does that, from the same findings — and it does not run at low effort, where the pass is unverified and emits no verdict. + +**The severities in this artifact are the canonical ones — draft the inline markers and the compose state FROM it, not from the list you typed by hand.** Ordering alone does not close the loop: `compose-review` reads `comments.json` and `compose.json`, both hand-written, so a hold that lowered a severity here still ships as `**[Critical]**` in the payload if the marker was copied from the draft instead of the artifact. Read `severity` out of `findings.json` for every marker and for the body Criticals. + +**This section sits before `### Verdict` on purpose.** `--test-delta` can lower a severity, and a Critical held back after `compose-review` has run reaches only the Step 8 report: the verdict line, the drafted `**[Critical]**` marker and the payload Step 7 recounts were all fixed before the measurement was consulted. Measured on #8368, that is the exact path the misattribution took into a composed review. If a hold does land after composing — a later round, a re-verified finding — treat it as a comment-set change: redraft the marker, update the comments file, and run `compose-review` again. + ### Verdict **You do not decide the verdict, and you do not write it. Ask for it:** @@ -760,22 +783,6 @@ The rules it applies — so you can read the line it gives you, not so you can a **The `FIX:` lines on stderr are that repair, spelled out.** For every repairable gap it capped on, `compose-review` prints one `FIX:` line naming the command — with this run's plan path already substituted. The parts that vary per agent stay as selectors: take ``, `` and `` from the labels in the same report (never paste a literal `<...>` into a shell — it parses as a redirection), and add the `--rules` file whenever Step 2 loaded one. Execute them — **one repair round, then `compose-review` again**. If the same gap survives the round, stop: the cap stands, post with it, and disclose the gap. Do not loop repairs hoping for a different verdict, and do not skip the round and post a capped verdict the FIX lines could have lifted — both are the same failure, choosing the verdict over the evidence, in opposite directions. -### The findings, as data - -**Write the findings artifact before you do anything else with them.** Everything that matters in this pipeline is a computed artifact — the diff plan, the coverage report, the resolved anchors, the verdict — and the findings were the one exception: prose in a terminal, re-typed into the Step 8 report, re-typed again into the Step 7 review JSON. Three transcriptions of the same list, and this skill's history is a catalogue of what transcription costs (a Critical that changed severity between two sections of one review; an aggregate that arrived at `resolve-anchors` with its per-location anchors dropped and took the whole batch down). - -Write every confirmed finding — high and low confidence alike — as a JSON array, then: - -```bash -"${QWEN_CODE_CLI:-qwen}" review findings \ - --input .qwen/tmp/qwen-review-{target}-findings-in.json \ - --out .qwen/tmp/qwen-review-{target}-findings.json -``` - -**One finding, one name.** A high-effort PR review also writes the incremental cache's cross-round `findings` ledger (Step 8), whose ids are `R-` — use those same ids here: a finding that will enter the ledger gets its `R-` as the artifact `id`, and a carried-forward finding keeps the id it already has. Two id schemes for one finding is how "R1-2" in next round's report and "f7" in this round's outcome ledger turn out to be the same defect that nobody can join. - -Each entry carries `id` (unique — outcomes and resolved anchors both join on it), `severity`, `confidence`, `source`, `summary`, `failureScenario`, and either `file`/`line`/`anchor` or, for a pattern aggregate, a `locations[]` array with **one entry per location** (`suggestedFix`, `category` and `shortSummary` are optional; `shortSummary` is derived from `summary` when absent). The command validates the shape, refuses a duplicate id, refuses a finding with no failure scenario, sorts by severity → confidence → file → line → id, and writes counts nobody then recomputes by hand. Read the artifact for the numbers you quote in the Summary. This is a **canonicalization**, not a gate: it does not decide the verdict — `compose-review` does that, from the same findings — and it does not run at low effort, where the pass is unverified and emits no verdict. - ### Step 6B: Apply the findings (`--fix`) **Run this only when the Step 1 verdict says `fix.effective` is true.** A requested-but-ineffective `--fix` (a PR target) has already produced its warning in Step 1; say nothing further and move on. @@ -792,10 +799,13 @@ Then record what happened to **every** finding — one of `fixed`, `skipped`, or "${QWEN_CODE_CLI:-qwen}" review findings \ --input .qwen/tmp/qwen-review-{target}-findings-in.json \ --outcomes .qwen/tmp/qwen-review-{target}-outcomes.json \ + --test-delta .qwen/tmp/qwen-review-{target}-test-delta.json \ --out .qwen/tmp/qwen-review-{target}-findings.json \ --print ``` +`--test-delta` belongs on this invocation for the same reason it belongs on the first: this run rebuilds the artifact from the same input, so leaving it off here restores every Critical the earlier run held back. + **The command refuses a ledger that does not account for every finding**, and that refusal is the whole reason it exists. A fixer that applies six of nine findings and reports six has not lied about any one of them — it has silently shortened the list, and the reader has no way to see the three that fell off. It also refuses an outcome for an id this review never produced, which is what a ledger built against the wrong list looks like. If it exits non-zero, the ledger is wrong, not the check: complete it and run it again. The three words are three different claims and are not interchangeable. `fixed` — the edit is in the tree. `skipped` — the finding is real and you did not apply it; the note says why, and the reader still owes it attention. `no_change_needed` — the finding was wrong or the code already handled it; it comes **off** the reader's plate. Collapsing `skipped` into `no_change_needed` is how a review quietly retracts a finding it could not fix.