diff --git a/docs/design/report-findings-typed-contract.md b/docs/design/report-findings-typed-contract.md new file mode 100644 index 00000000000..521685b1294 --- /dev/null +++ b/docs/design/report-findings-typed-contract.md @@ -0,0 +1,74 @@ +# Report Findings Typed Contract + +## Context + +`/review` already canonicalizes its findings as data twice: `qwen review +findings` writes the typed artifact under `.qwen/tmp/`, and Step 8's +`save-artifact` + `record_artifact` publish a durable copy the Web Shell +renders (`CodeReviewArtifactDetail`). But both are files registered after the +fact. Every client rendering the session live — the terminal UI, the Web Shell +transcript, ACP hosts, the daemon TUI — receives only the Markdown +restatement of the same list, and after `--fix` (or a later `fix these +issues`) nothing in-band tells a client which findings are now closed. + +## Design + +A new core tool, `report_findings`, is the in-band half of the contract: one +call with `{level, findings[]}`, rendered by host UIs as a per-finding list. +Field names and enum spellings match the findings artifact exactly (`id`, +`severity`, `confidence`, `source`, `file`/`line`, `summary`, `shortSummary`, +`failureScenario`, `category`, `outcome`, `outcomeNote`), so the model copies +values out of the artifact instead of translating them. The tool sorts by +severity → confidence → location, derives and compresses `shortSummary` to 60 +characters, rejects control characters and duplicate ids, and — mirroring +`review findings --outcomes` — refuses a call where some findings carry an +`outcome` and others do not. It persists nothing and decides no verdict; the +result is a `findings_list` structured `returnDisplay`. + +The finding enums now live in core (`tools/report-findings.ts`); +`packages/cli/src/commands/review/findings.ts` re-exports them under its historical +names. The Web Shell renderer keeps its deliberate browser-side copy. + +The `/review` skill calls the tool once after writing the findings artifact +(Step 6; low effort reports its unverified list with `level: "low"`), and +again after `--fix` with every finding carrying its outcome — a rule that +outlives Step 6B: any later in-session disposition change records outcomes +into the artifact and re-issues the call. The call is UI delivery: a failure +is disclosed and never alters artifacts or the verdict. + +Rendering: the TUI gets a `FindingsDisplay` row list (severity color, id, +`file:line`, short summary, confidence marker, outcome badge); the daemon TUI +adapter passes `findings_list` through; history/recording compaction truncates +the free-text fields and applies an aggregate retained-display budget across +the list, keeping the most severe prefix and counting the evicted tail +(`omittedFindings`). + +"Later calls replace the list" is rendered, not just validated: every +transcript surface — live history, restored history, recording/resume, and +the daemon projection — keeps only the last delivered `findings_list` and +collapses each earlier one to a one-line replacement marker, so an initial +report and its outcome re-report never show two checklists at once. + +The outcome identity gate (`activeReportIds`) is a live-process contract: +the tool instance is cached by the registry for the session, but a cold +session resume constructs a fresh instance with no active identity, and an +outcome call is then validated on its own terms (all-or-nothing outcomes) +instead of against the pre-restart report. Persisting the identity across +restarts is deliberately out of scope; the transcript-side replacement above +does not depend on it. + +The findings command's `--input` also accepts a saved review artifact or a +prior `--out` report (any object carrying the array as `findings`), because +Step 9 cleanup deletes the `findings-in.json` side file a later-session +outcome path would otherwise need. + +## Verification + +- Core tool unit tests: sorting, shortSummary derivation/compression, empty + list, outcome counting, partial-outcome refusal, duplicate ids, control + characters, schema violations, trimming. +- Compaction test: free-text fields truncate, typed fields survive. +- `FindingsDisplay` ink render tests: rows, outcomes with skip reason, empty + state. +- Existing `findings.ts`, `save-artifact`, ToolMessage, daemon adapter, + config-registration, SKILL parity and review-digest suites stay green. diff --git a/packages/cli/src/commands/review/findings.test.ts b/packages/cli/src/commands/review/findings.test.ts index 81728cb8d18..e5dd074d0f3 100644 --- a/packages/cli/src/commands/review/findings.test.ts +++ b/packages/cli/src/commands/review/findings.test.ts @@ -205,10 +205,48 @@ describe('validateFindings', () => { ).toThrow(/location 0 has an invalid "line"/); }); - it('rejects a top-level input that is not an array', () => { - expect(() => validateFindings({ findings: [] })).toThrow( + it('rejects a top-level input that is neither an array nor a findings wrapper', () => { + expect(() => validateFindings({ findings: 'not-an-array' })).toThrow( /must be a JSON array/, ); + expect(() => validateFindings({ verdict: 'approve' })).toThrow( + /must be a JSON array/, + ); + }); + + it('accepts the saved-artifact and report wrappers the recovery path feeds it', () => { + // Step 9 cleanup deletes the findings-in.json side file a later-session + // outcome path needs; the saved artifact (Step 8) and this command's own + // report survive it, and both wrap the array. `--input` must recover + // from that surviving state instead of dying on the missing side file. + const canonical = validateFindings([ + { ...base, id: 'R1-1' }, + { ...base, id: 'R1-2', severity: 'Suggestion' }, + ]); + const report = buildReport(canonical); + const fromReport = validateFindings(report); + expect(fromReport.map((f) => f.id)).toEqual(['R1-1', 'R1-2']); + + // The ReviewArtifactV1 shape: the same array under review metadata. + const artifact = { + schemaVersion: 1, + reviewId: 'review-1', + findings: report.findings, + counts: report.counts, + }; + const fromArtifact = validateFindings(artifact); + expect(fromArtifact.map((f) => f.id)).toEqual(['R1-1', 'R1-2']); + + // The wrapper round-trips the outcome merge end to end: outcomes apply + // to the unwrapped list exactly as they would to the bare array. + const withOutcomes = applyOutcomes( + validateFindings(report), + validateOutcomes([ + { id: 'R1-1', outcome: 'fixed' }, + { id: 'R1-2', outcome: 'skipped', note: 'intended behaviour' }, + ]), + ); + expect(withOutcomes.map((f) => f.outcome)).toEqual(['fixed', 'skipped']); }); }); @@ -425,6 +463,24 @@ describe('validateOutcomes', () => { /index 0 is missing a string "id"/, ); }); + + it('rejects a skipped outcome with no note', () => { + // `skipped` keeps the finding on the reader's plate and the note is the + // reader's only handle on it — and the report_findings contract refuses + // a skipped outcome that carries none, so the ledger feeding it must not + // accept one either. + expect(() => validateOutcomes([{ id: 'f1', outcome: 'skipped' }])).toThrow( + /"skipped" with no note/, + ); + expect(() => + validateOutcomes([{ id: 'f1', outcome: 'skipped', note: ' ' }]), + ).toThrow(/"skipped" with no note/); + expect( + validateOutcomes([ + { id: 'f1', outcome: 'skipped', note: 'needs a product call' }, + ]), + ).toEqual([{ id: 'f1', outcome: 'skipped', note: 'needs a product call' }]); + }); }); describe('buildReport', () => { diff --git a/packages/cli/src/commands/review/findings.ts b/packages/cli/src/commands/review/findings.ts index ca72da12c96..ee1910478b3 100644 --- a/packages/cli/src/commands/review/findings.ts +++ b/packages/cli/src/commands/review/findings.ts @@ -42,20 +42,31 @@ import { } from 'node:fs'; import type { Stats } from 'node:fs'; import { dirname, resolve, sep } from 'node:path'; +import { + FINDING_SEVERITIES, + FINDING_CONFIDENCES, + FINDING_OUTCOMES, + FINDING_SOURCES, + compressFindingSummary, +} from '@qwen-code/qwen-code-core'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import type { AnchorRequest } from './lib/anchors.js'; import { isSameFile } from './lib/same-file.js'; -// These four lists have a second consumer: the Web Shell review renderer +// These four lists are DEFINED in core (`core/src/tools/report-findings.ts`, +// the `report_findings` tool's contract) and re-exported here under this +// module's historical names. They still have one further deliberate consumer: +// the Web Shell review renderer // (packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx) -// keeps its own copy and fails closed on any value it does not know, so a -// value added here breaks rendering of every saved artifact that carries one. -// Update the renderer copy in the same change. +// is a browser bundle that cannot import Node-side packages, keeps its own +// copy, and fails closed on any value it does not know — so a value added in +// core breaks rendering of every saved artifact that carries one. Update the +// renderer copy in the same change. /** The severity ladder, most severe first — this array IS the sort order. */ -export const SEVERITIES = ['Critical', 'Suggestion', 'Nice to have'] as const; +export const SEVERITIES = FINDING_SEVERITIES; export type Severity = (typeof SEVERITIES)[number]; -export const CONFIDENCES = ['high', 'low'] as const; +export const CONFIDENCES = FINDING_CONFIDENCES; export type Confidence = (typeof CONFIDENCES)[number]; /** @@ -67,11 +78,11 @@ export type Confidence = (typeof CONFIDENCES)[number]; * already handled" and takes it off. They are different claims about the code, * so they are different words, and the fixer has to pick one. */ -export const OUTCOMES = ['fixed', 'skipped', 'no_change_needed'] as const; +export const OUTCOMES = FINDING_OUTCOMES; export type Outcome = (typeof OUTCOMES)[number]; /** Where a finding came from — the tag that decides whether it was verified. */ -export const SOURCES = ['review', 'build', 'test', 'probe', 'lint'] as const; +export const SOURCES = FINDING_SOURCES; export type Source = (typeof SOURCES)[number]; /** One location a finding applies to. A pattern aggregate carries several. */ @@ -160,20 +171,8 @@ export interface FindingsReport { outcomesRecorded: boolean; } -/** `shortSummary`, when the caller did not supply one. */ -export function compressSummary(summary: string, max = 60): string { - // Collapse whitespace first: a summary that wrapped across lines in the source - // prose would otherwise carry its newlines into a single-line list cell. - const flat = summary.replace(/\s+/g, ' ').trim(); - if (flat.length <= max) return flat; - // Cut on a word boundary when one is reasonably near the limit, so the label - // reads as a clause rather than a severed word. `max - 1` leaves room for the - // ellipsis, which is one character (U+2026), not three dots. - const head = flat.slice(0, max - 1); - const space = head.lastIndexOf(' '); - const cut = space >= max * 0.6 ? head.slice(0, space) : head; - return `${cut.trimEnd()}…`; -} +/** `shortSummary`, when the caller did not supply one. Defined in core. */ +export const compressSummary = compressFindingSummary; function fail(index: number, message: string): never { throw new Error(`Finding at index ${index}: ${message}`); @@ -305,8 +304,23 @@ function parseLocations( * are derived or dropped, never demanded. */ export function validateFindings(raw: unknown): Finding[] { + // Step 9 cleanup deletes the side files `--input` normally receives, but + // not the saved artifact (Step 8, under .qwen/reviews/) nor a surviving + // `--out` report — and both wrap the findings array. Accept the wrapper, + // so a later outcome path can recover from the state that survives the + // cleanup instead of dying on a missing findings-in.json. + if ( + !Array.isArray(raw) && + raw !== null && + typeof raw === 'object' && + Array.isArray((raw as { findings?: unknown }).findings) + ) { + raw = (raw as { findings: unknown }).findings; + } if (!Array.isArray(raw)) { - throw new Error('Input must be a JSON array of findings.'); + throw new Error( + 'Input must be a JSON array of findings, or a saved review artifact/report object carrying one as "findings".', + ); } const findings = raw.map((r, i) => { if (r === null || typeof r !== 'object' || Array.isArray(r)) { @@ -765,6 +779,13 @@ export function validateOutcomes(raw: unknown): OutcomeEntry[] { `expected one of ${OUTCOMES.map((s) => JSON.stringify(s)).join(', ')}.`, ); } + // The report_findings contract refuses a skipped outcome the reader + // cannot inspect; the ledger feeding it must not accept one either. + if (outcome === 'skipped' && !asString(o, 'note')) { + throw new Error( + `Outcome for ${JSON.stringify(id)} is "skipped" with no note — the reader is owed the reason for work not done.`, + ); + } return { id, outcome, @@ -1012,7 +1033,8 @@ export const findingsCommand: CommandModule = { .option('input', { type: 'string', demandOption: true, - describe: 'JSON array of findings written by the review', + describe: + 'JSON array of findings written by the review (or a saved review artifact/report object carrying the array as "findings")', }) .option('out', { type: 'string', diff --git a/packages/cli/src/i18n/locales/ca.js b/packages/cli/src/i18n/locales/ca.js index 9e233adf2f7..23cc0350e0c 100644 --- a/packages/cli/src/i18n/locales/ca.js +++ b/packages/cli/src/i18n/locales/ca.js @@ -2403,6 +2403,7 @@ export default { 'toolDisplayName.Agent': 'Agent', 'toolDisplayName.Artifact': 'Artefacte', 'toolDisplayName.RecordArtifact': "Enregistra l'artefacte", + 'toolDisplayName.ReportFindings': 'Informa de les troballes', 'toolDisplayName.Skill': 'Habilitat', 'toolDisplayName.EnterPlanMode': 'Entra al mode de planificació', 'toolDisplayName.ExitPlanMode': 'Surt del mode de planificació', diff --git a/packages/cli/src/i18n/locales/en.js b/packages/cli/src/i18n/locales/en.js index 2dbc0ba92f2..f2c1b65945a 100644 --- a/packages/cli/src/i18n/locales/en.js +++ b/packages/cli/src/i18n/locales/en.js @@ -192,6 +192,7 @@ export default { 'toolDisplayName.Agent': 'toolDisplayName.Agent', 'toolDisplayName.Artifact': 'toolDisplayName.Artifact', 'toolDisplayName.RecordArtifact': 'toolDisplayName.RecordArtifact', + 'toolDisplayName.ReportFindings': 'toolDisplayName.ReportFindings', 'toolDisplayName.DisplayImage': 'toolDisplayName.DisplayImage', 'toolDisplayName.Skill': 'toolDisplayName.Skill', 'toolDisplayName.EnterPlanMode': 'toolDisplayName.EnterPlanMode', diff --git a/packages/cli/src/i18n/locales/zh-TW.js b/packages/cli/src/i18n/locales/zh-TW.js index 28c22a56e29..0d7efbe5282 100644 --- a/packages/cli/src/i18n/locales/zh-TW.js +++ b/packages/cli/src/i18n/locales/zh-TW.js @@ -183,6 +183,7 @@ export default { 'toolDisplayName.Agent': 'Agent', 'toolDisplayName.Artifact': '製品', 'toolDisplayName.RecordArtifact': '記錄製品', + 'toolDisplayName.ReportFindings': '上報評審發現', 'toolDisplayName.DisplayImage': '顯示圖片', 'toolDisplayName.Skill': '技能', 'toolDisplayName.EnterPlanMode': '進入計畫模式', diff --git a/packages/cli/src/i18n/locales/zh.js b/packages/cli/src/i18n/locales/zh.js index f0149817e8c..7342eca9144 100644 --- a/packages/cli/src/i18n/locales/zh.js +++ b/packages/cli/src/i18n/locales/zh.js @@ -184,6 +184,7 @@ export default { 'toolDisplayName.Agent': 'Agent', 'toolDisplayName.Artifact': '制品', 'toolDisplayName.RecordArtifact': '记录制品', + 'toolDisplayName.ReportFindings': '上报评审发现', 'toolDisplayName.DisplayImage': '显示图片', 'toolDisplayName.Skill': '技能', 'toolDisplayName.EnterPlanMode': '进入计划模式', diff --git a/packages/cli/src/ui/AppContainer.test.tsx b/packages/cli/src/ui/AppContainer.test.tsx index 192b96d09a9..c9002c62926 100644 --- a/packages/cli/src/ui/AppContainer.test.tsx +++ b/packages/cli/src/ui/AppContainer.test.tsx @@ -99,6 +99,7 @@ import { CONTEXT_FILES_ANNOUNCEMENT_PREFIX, isContextFilesAnnouncement, } from './utils/commandUtils.js'; +import { SUPERSEDED_FINDINGS_MESSAGE } from './utils/findings-coalescing.js'; import { ICON } from './constants.js'; import type { RestoreOption } from './components/RewindSelector.js'; import { Box, measureElement } from 'ink'; @@ -6028,6 +6029,70 @@ describe('AppContainer State Management', () => { ); }); + it('restores a superseded findings list when rewinding past its replacing call', async () => { + // The outcome re-report superseded the initial list at commit time; + // rewinding past the re-report must bring the initial checklist + // back instead of leaving the stale replacement marker. + const firstDisplay = { + type: 'findings_list', + findings: [ + { + id: 'R1-1', + severity: 'Critical', + file: 'src/foo.ts', + summary: 's', + shortSummary: 's', + failureScenario: 'f', + }, + ], + }; + const findingsGroup = ( + id: number, + callId: string, + resultDisplay: unknown, + carried?: unknown, + ): HistoryItem => + ({ + id, + type: 'tool_group', + tools: [ + { + callId, + name: 'report_findings', + description: 'Report findings', + status: ToolCallStatus.Success, + confirmationDetails: undefined, + resultDisplay, + supersededFindingsDisplay: carried, + }, + ], + }) as unknown as HistoryItem; + const history: HistoryItem[] = [ + rewindUserItem(1, 'first prompt', 'prompt-1'), + findingsGroup(2, 'call-1', SUPERSEDED_FINDINGS_MESSAGE, firstDisplay), + rewindUserItem(3, 'second prompt', 'prompt-2'), + findingsGroup(4, 'call-2', { + ...firstDisplay, + findings: [{ ...firstDisplay.findings[0], outcome: 'fixed' }], + }), + ]; + const harness = renderRewindHarness({ history }); + + await runRewind(harness.target, 'both'); + + expect(harness.loadHistory).toHaveBeenCalledTimes(1); + const loaded = harness.loadHistory.mock.calls[0][0] as HistoryItem[]; + expect(loaded).toHaveLength(2); + const surviving = loaded[1] as unknown as { + tools: Array<{ + resultDisplay: unknown; + supersededFindingsDisplay?: unknown; + }>; + }; + expect(surviving.tools[0].resultDisplay).toEqual(firstDisplay); + expect(surviving.tools[0].supersededFindingsDisplay).toBeUndefined(); + }); + it('re-arms the latch when rewinding past the context-file announcement', async () => { // Announcement sits after the rewind target, so it is filtered out of // truncatedUi; the latch re-arms and the next prompt re-announces the diff --git a/packages/cli/src/ui/AppContainer.tsx b/packages/cli/src/ui/AppContainer.tsx index edd9e5e15c1..cb4e36d5593 100644 --- a/packages/cli/src/ui/AppContainer.tsx +++ b/packages/cli/src/ui/AppContainer.tsx @@ -78,6 +78,7 @@ import { buildResumedHistoryItems, expandCollapsedHistory, } from './utils/resumeHistoryUtils.js'; +import { recoalesceFindingsHistoryItems } from './utils/findings-coalescing.js'; import { buildWakeRepaint } from './utils/terminal-resize-reflow.js'; import { loadLowlight } from './utils/lowlightLoader.js'; import { @@ -3766,8 +3767,10 @@ export const AppContainer = (props: AppContainerProps) => { // Strip suppressOnRestore flags and filter out collapse-summary items // so rewound items remain visible without stale summary text - const truncatedUi = expandCollapsedHistory( - originalHistory.filter((h) => h.id < userItem.id), + const truncatedUi = recoalesceFindingsHistoryItems( + expandCollapsedHistory( + originalHistory.filter((h) => h.id < userItem.id), + ), ); clearPendingStateRef.current(); loadHistoryWithLatchReconciliation(truncatedUi); diff --git a/packages/cli/src/ui/components/FindingsDisplay.test.tsx b/packages/cli/src/ui/components/FindingsDisplay.test.tsx new file mode 100644 index 00000000000..5ddb0315cf3 --- /dev/null +++ b/packages/cli/src/ui/components/FindingsDisplay.test.tsx @@ -0,0 +1,181 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { render } from 'ink-testing-library'; +import { describe, expect, it } from 'vitest'; +import type { FindingsResultDisplay } from '@qwen-code/qwen-code-core'; +import { FindingsDisplay } from './FindingsDisplay.js'; + +function display( + overrides: Partial = {}, +): FindingsResultDisplay { + return { + type: 'findings_list', + level: 'high', + findings: [ + { + id: 'R1-1', + severity: 'Critical', + confidence: 'high', + file: 'src/foo.ts', + line: 42, + // shortSummary deliberately differs from summary: the row must render + // the compact label, and a fixture where the two coincide would keep + // every test green if FindingRow regressed to rendering `summary`. + summary: + 'the provider returns the wrong value on every cold-cache lookup', + shortSummary: 'cold-cache wrong return', + failureScenario: 'first call after start returns undefined', + }, + { + severity: 'Suggestion', + confidence: 'low', + file: 'src/bar.ts', + summary: 'the helper is duplicated between bar.ts and baz.ts', + shortSummary: 'duplicated helper', + failureScenario: 'two copies drift', + }, + ], + ...overrides, + }; +} + +describe('', () => { + it('renders one row per finding with severity, id, location and label', () => { + const { lastFrame } = render(); + const frame = lastFrame()!; + expect(frame).toContain('Critical'); + expect(frame).toContain('R1-1'); + expect(frame).toContain('src/foo.ts:42'); + expect(frame).toContain('cold-cache wrong return'); + expect(frame).toContain('Suggestion'); + expect(frame).toContain('src/bar.ts'); + expect(frame).toContain('(low confidence)'); + // The row renders shortSummary, never the full summary. + expect(frame.replace(/\s+/g, ' ')).not.toContain( + 'wrong value on every cold-cache lookup', + ); + }); + + it('renders outcomes with the skip reason', () => { + const data = display(); + data.findings = data.findings.map((finding, index) => + index === 0 + ? { ...finding, outcome: 'fixed' as const } + : { + ...finding, + outcome: 'skipped' as const, + outcomeNote: 'fix would change intended behaviour', + }, + ); + const { lastFrame } = render(); + const frame = lastFrame()!.replace(/\s+/g, ' '); + expect(frame).toContain('(fixed)'); + expect(frame).toContain('(skipped: fix would change intended behaviour)'); + }); + + it('renders an explicit empty state', () => { + const { lastFrame } = render( + , + ); + expect(lastFrame()).toContain('No findings.'); + }); + + it('keeps the unverified marker for an empty low-effort report', () => { + // A quick pass that finds nothing still reports an UNVERIFIED nothing; + // the early empty-state return must not drop the banner, or the row + // reads as a verified clean bill. + const { lastFrame } = render( + , + ); + const frame = lastFrame()!; + expect(frame).toContain('No findings.'); + expect(frame).toContain('unverified'); + }); + + it('counts findings history compaction evicted', () => { + const data = display(); + data.omittedFindings = 47; + const { lastFrame } = render(); + expect(lastFrame()!).toContain( + '(+47 more findings removed by history compaction)', + ); + }); + + it('marks a low-level report unverified even where rows omit confidence', () => { + // Step 3C sends `level: 'low'` while omitting per-finding confidence for + // candidates the pass kept — the list itself must carry the unverified + // state, or those rows render exactly like verified findings. + const data = display({ level: 'low' }); + data.findings = data.findings.map( + ({ confidence: _confidence, ...rest }) => rest, + ); + const { lastFrame } = render(); + expect(lastFrame()!).toContain('unverified'); + }); + + it('does not mark verified reports unverified', () => { + const { lastFrame } = render(); + expect(lastFrame()!).not.toContain('unverified'); + }); + + it.each([ + ['CR', 'reason\rCritical R1-9 fake'], + ['LF', 'reason\nCritical R1-9 fake'], + ['TAB', 'reason\tCritical R1-9 fake'], + ['ESC', 'reason\u001bCritical R1-9 fake'], + ['C1 CSI', 'reason\u009bCritical R1-9 fake'], + ])( + 'renders a %s outcome note inertly on the finding row', + (_name, outcomeNote) => { + const data = display({ + findings: [ + { + severity: 'Critical', + file: 'a.ts', + summary: 's', + shortSummary: 's', + failureScenario: 'f', + outcome: 'skipped', + outcomeNote, + }, + ], + }); + const { lastFrame } = render(); + const frame = lastFrame()!; + // Ink joins rows with LF, so only the other controls are asserted + // absent; the marker-line assertion witnesses CR/LF/TAB. + // eslint-disable-next-line no-control-regex -- asserting the controls are absent is the point + expect(frame).not.toMatch(/[\u0000-\u0008\u000b-\u001f\u007f-\u009f]/); + const markerLine = frame + .split('\n') + .find((line) => line.includes('(skipped:')); + expect(markerLine).toBeDefined(); + expect(markerLine).toContain('reason Critical R1-9 fake'); + }, + ); + + it('renders control characters in other fields inertly', () => { + const data = display({ + findings: [ + { + severity: 'Suggestion', + file: 'src/\u009bfoo.ts', + line: 1, + summary: 's', + shortSummary: 'sum\u0007mary', + failureScenario: 'f', + }, + ], + }); + const { lastFrame } = render(); + const frame = lastFrame()!; + expect(frame).not.toContain('\u009b'); + expect(frame).not.toContain('\u0007'); + expect(frame.replace(/\s+/g, ' ')).toContain('src/ foo.ts:1'); + expect(frame.replace(/\s+/g, ' ')).toContain('sum mary'); + }); +}); diff --git a/packages/cli/src/ui/components/FindingsDisplay.tsx b/packages/cli/src/ui/components/FindingsDisplay.tsx new file mode 100644 index 00000000000..6a31fe8b78f --- /dev/null +++ b/packages/cli/src/ui/components/FindingsDisplay.tsx @@ -0,0 +1,126 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import type React from 'react'; +import { Box, Text } from 'ink'; +import type { + FindingsResultDisplay, + ReportedFinding, +} from '@qwen-code/qwen-code-core'; +import { Colors } from '../colors.js'; +import { ICON } from '../constants.js'; + +interface FindingsDisplayProps { + data: FindingsResultDisplay; +} + +const SEVERITY_COLORS: Record string> = { + Critical: () => Colors.AccentRed, + Suggestion: () => Colors.AccentYellow, + 'Nice to have': () => Colors.Gray, +}; + +// Every interpolated value renders through this: `outcomeNote` legitimately +// carries line whitespace, and any control character that survived the +// validator could forge or overwrite lines that read as trusted findings. +function terminalSafe(text: string): string { + /* eslint-disable no-control-regex -- C0/DEL/C1 controls are exactly what this strips */ + const withoutControls = text.replace(/[\u0000-\u001f\u007f-\u009f]/g, ' '); + /* eslint-enable no-control-regex */ + return withoutControls.replace(/\s+/g, ' ').trim(); +} + +const OUTCOME_LABELS: Record< + NonNullable, + string +> = { + fixed: 'fixed', + skipped: 'skipped', + no_change_needed: 'no change needed', +}; + +export const FindingsDisplay: React.FC = ({ data }) => ( + // The unverified banner precedes the empty-state branch: an empty list is + // still the product of the pass that reported it, and a low-effort "no + // findings" that renders without the marker reads as a verified clean bill. + + {data.level === 'low' && ( + + + (low-effort pass — findings are unverified) + + + )} + {data.findings.length === 0 ? ( + No findings. + ) : ( + data.findings.map((finding, index) => ( + + )) + )} + {data.omittedFindings !== undefined && data.omittedFindings > 0 && ( + + {`(+${data.omittedFindings} more finding${data.omittedFindings === 1 ? '' : 's'} removed by history compaction)`} + + )} + +); + +const FindingRow: React.FC<{ finding: ReportedFinding }> = ({ finding }) => { + const severityColor = SEVERITY_COLORS[finding.severity](); + const resolved = + finding.outcome === 'fixed' || finding.outcome === 'no_change_needed'; + const icon = + finding.outcome === undefined + ? ICON.CIRCLE_FILLED + : resolved + ? ICON.CHECK + : ICON.CIRCLE_EMPTY; + const where = terminalSafe( + `${finding.file}${finding.line !== undefined ? `:${finding.line}` : ''}`, + ); + + return ( + + + + {icon} + + + + + + {finding.severity} + + + {finding.id ? ` ${terminalSafe(finding.id)}` : ''}{' '} + + {where} + + {' '} + {terminalSafe(finding.shortSummary)} + + {finding.confidence === 'low' && ( + (low confidence) + )} + {finding.outcome && ( + + {' '} + ({OUTCOME_LABELS[finding.outcome]} + {finding.outcome === 'skipped' && finding.outcomeNote + ? `: ${terminalSafe(finding.outcomeNote)}` + : ''} + ) + + )} + + + + ); +}; diff --git a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx index bbc774768e8..4c0063d6876 100644 --- a/packages/cli/src/ui/components/messages/ToolMessage.test.tsx +++ b/packages/cli/src/ui/components/messages/ToolMessage.test.tsx @@ -193,6 +193,55 @@ describe('', () => { expect(output).not.toContain('MockMarkdown:Test result'); // collapsed }); + it('routes a findings_list result to the findings renderer', () => { + // Pins the ToolMessage discriminator itself: FindingsDisplay has its own + // render tests, but without this the routing branch could be removed and + // every test would stay green (the display would fall through to the + // JSON-string path, which never joins file and line as `file:line`). + const { lastFrame } = renderWithContext( + , + StreamingState.Idle, + ); + const output = lastFrame()!; + expect(output).toContain('src/foo.ts:42'); + expect(output).toContain('cold-cache wrong return'); + expect(output).toContain('(low confidence)'); + expect(output).not.toContain('"findings"'); // not the JSON fallback + expect(output.replace(/\s+/g, ' ')).not.toContain( + 'wrong value on every cold-cache lookup', + ); + }); + it('renders inline images returned by a tool', () => { const { lastFrame } = renderWithContext( = ({ {effectiveDisplayRenderer.type === 'todo' && ( )} + {effectiveDisplayRenderer.type === 'findings' && ( + + )} {effectiveDisplayRenderer.type === 'plan' && ( { @@ -162,6 +163,220 @@ describe('reduceDaemonEventToTuiUpdates', () => { ]); }); + it('preserves a findings_list result as structured output', () => { + const updates = reduceDaemonEventToTuiUpdates({ + id: 1, + v: 1, + type: 'session_update', + data: { + sessionId: 'session-1', + update: { + sessionUpdate: 'tool_call_update', + toolCallId: 'tool-findings', + kind: 'think', + title: 'ReportFindings', + status: 'completed', + rawOutput: { + type: 'findings_list', + level: 'high', + // Compaction's eviction count is part of the trusted shape and + // must pass the boundary, or a compacted re-report degrades to + // text exactly where the omission matters. + omittedFindings: 3, + findings: [ + { + id: 'R1-1', + severity: 'Critical', + confidence: 'high', + file: 'src/foo.ts', + line: 42, + summary: 'wrong return value on cold cache', + shortSummary: 'wrong return value on cold cache', + failureScenario: 'first call after start returns undefined', + }, + ], + }, + }, + }, + }); + + expect(updates).toMatchObject([ + { + type: 'tool_group_update', + item: { + tools: [ + { + resultDisplay: { + type: 'findings_list', + level: 'high', + omittedFindings: 3, + findings: [ + { + id: 'R1-1', + severity: 'Critical', + file: 'src/foo.ts', + line: 42, + shortSummary: 'wrong return value on cold cache', + }, + ], + }, + }, + ], + }, + }, + ]); + }); + + it('replaces the previous findings list in the projection when a new one arrives', () => { + // The daemon reducer holds one logical report: a delivered findings list + // supersedes the earlier call's list instead of rendering beside it. + const state = createDaemonTuiReducerState(); + const findingsEvent = (toolCallId: string, outcome?: string) => ({ + id: 1, + v: 1 as const, + type: 'session_update', + data: { + sessionId: 'session-1', + update: { + sessionUpdate: 'tool_call_update', + toolCallId, + kind: 'think', + title: 'ReportFindings', + status: 'completed', + rawOutput: { + type: 'findings_list', + level: 'high', + findings: [ + { + id: 'R1-1', + severity: 'Critical', + confidence: 'high', + file: 'src/foo.ts', + line: 42, + summary: 'wrong return value on cold cache', + shortSummary: 'wrong return value on cold cache', + failureScenario: 'first call after start returns undefined', + ...(outcome ? { outcome } : {}), + }, + ], + }, + }, + }, + }); + + reduceDaemonEventToTuiUpdates(findingsEvent('tool-report-1'), state); + const updates = reduceDaemonEventToTuiUpdates( + findingsEvent('tool-report-2', 'fixed'), + state, + ); + + const group = updates.find((u) => u.type === 'tool_group_update'); + expect(group).toBeDefined(); + if (!group || group.type !== 'tool_group_update') return; + expect(group.item.tools).toHaveLength(2); + const byId = Object.fromEntries( + group.item.tools.map((tool) => [tool.callId, tool]), + ); + expect(byId['tool-report-1'].resultDisplay).toBe( + SUPERSEDED_FINDINGS_MESSAGE, + ); + expect(byId['tool-report-2'].resultDisplay).toMatchObject({ + type: 'findings_list', + findings: [{ id: 'R1-1', outcome: 'fixed' }], + }); + }); + + it('falls back to text for findings_list payloads that fail the shape check', () => { + // The daemon boundary must not hand FindingsDisplay a payload it would + // read `findings.length` off — a discriminator-only shape used to crash + // the TUI. + const payloads: unknown[] = [ + { type: 'findings_list' }, + { type: 'findings_list', findings: [{ severity: 'Critical' }] }, + { type: 'findings_list', findings: 'not-an-array' }, + { type: 'findings_list', level: 'ultra', findings: [] }, + { + type: 'findings_list', + findings: [ + { + severity: 'Critical', + file: 'a.ts', + summary: 's', + shortSummary: 's', + failureScenario: 'f', + outcome: 'wontfix', + }, + ], + }, + ]; + for (const rawOutput of payloads) { + const updates = reduceDaemonEventToTuiUpdates({ + id: 1, + v: 1, + type: 'session_update', + data: { + sessionId: 'session-1', + update: { + sessionUpdate: 'tool_call_update', + toolCallId: 'tool-findings-bad', + kind: 'think', + title: 'ReportFindings', + status: 'completed', + rawOutput, + }, + }, + }); + const resultDisplay = ( + updates[0] as { + item: { tools: Array<{ resultDisplay: unknown }> }; + } + ).item.tools[0].resultDisplay; + expect(typeof resultDisplay).toBe('string'); + expect(resultDisplay as string).toContain('findings_list'); + } + }); + + it('falls back to text for malformed findings_list payloads even when permissive keys are present', () => { + // A `findings_list` record must be judged by its full shape BEFORE the + // permissive display arms run: `ansiOutput`/`fileDiff` keys used to + // short-circuit the OR chain and smuggle an unvalidated payload through + // as structured output, which then crashed FindingsDisplay. + const payloads: unknown[] = [ + { + type: 'findings_list', + ansiOutput: '', + findings: [{ severity: 'bogus' }], + }, + { type: 'findings_list', fileDiff: '--- a\n+++ b', findings: null }, + { type: 'findings_list', ansiOutput: 'x' }, + ]; + for (const rawOutput of payloads) { + const updates = reduceDaemonEventToTuiUpdates({ + id: 1, + v: 1, + type: 'session_update', + data: { + sessionId: 'session-1', + update: { + sessionUpdate: 'tool_call_update', + toolCallId: 'tool-findings-bypass', + kind: 'think', + title: 'ReportFindings', + status: 'completed', + rawOutput, + }, + }, + }); + const resultDisplay = ( + updates[0] as { + item: { tools: Array<{ resultDisplay: unknown }> }; + } + ).item.tools[0].resultDisplay; + expect(typeof resultDisplay).toBe('string'); + expect(resultDisplay as string).toContain('findings_list'); + } + }); + it('maps assistant, tool, model, and disconnect daemon events while suppressing thought history', () => { expect( reduceDaemonEventToTuiUpdates({ diff --git a/packages/cli/src/ui/daemon/daemon-tui-adapter.ts b/packages/cli/src/ui/daemon/daemon-tui-adapter.ts index 5aa4ee22ec3..beffc86cc00 100644 --- a/packages/cli/src/ui/daemon/daemon-tui-adapter.ts +++ b/packages/cli/src/ui/daemon/daemon-tui-adapter.ts @@ -11,7 +11,12 @@ import type { } from '@agentclientprotocol/sdk'; import { createDebugLogger, + FINDING_CONFIDENCES, + FINDING_OUTCOMES, + FINDING_SEVERITIES, + FINDING_SOURCES, isVisionBridgeNoticeDisplay, + REPORT_FINDINGS_LEVELS, } from '@qwen-code/qwen-code-core'; import { ToolCallStatus, @@ -19,6 +24,7 @@ import { type HistoryItemWithoutId, type IndividualToolCallDisplay, } from '../types.js'; +import { SUPERSEDED_FINDINGS_MESSAGE } from '../utils/findings-coalescing.js'; export interface DaemonTuiEvent { id?: number; @@ -262,6 +268,54 @@ function createSanitizedDaemonError(error: unknown): Error { return new Error(`Daemon RPC failed: ${message}`); } +function isEnumValue(value: unknown, allowed: readonly string[]): boolean { + return typeof value === 'string' && allowed.includes(value); +} + +// The trust boundary for findings_list: past this check the payload reaches +// FindingsDisplay, which reads `findings` and its fields unconditionally. +// Anything short of the full typed shape — a discriminator-only payload, a +// missing array, a bad enum — falls back to the plain-text rendering instead +// of crashing the TUI. +function isFindingsListDisplay(value: unknown): boolean { + if (!isRecord(value) || !Array.isArray(value['findings'])) { + return false; + } + if ( + value['level'] !== undefined && + !isEnumValue(value['level'], REPORT_FINDINGS_LEVELS) + ) { + return false; + } + if ( + value['omittedFindings'] !== undefined && + typeof value['omittedFindings'] !== 'number' + ) { + return false; + } + return (value['findings'] as unknown[]).every( + (entry) => + isRecord(entry) && + typeof entry['file'] === 'string' && + typeof entry['summary'] === 'string' && + typeof entry['shortSummary'] === 'string' && + typeof entry['failureScenario'] === 'string' && + isEnumValue(entry['severity'], FINDING_SEVERITIES) && + (entry['confidence'] === undefined || + isEnumValue(entry['confidence'], FINDING_CONFIDENCES)) && + (entry['source'] === undefined || + isEnumValue(entry['source'], FINDING_SOURCES)) && + (entry['outcome'] === undefined || + isEnumValue(entry['outcome'], FINDING_OUTCOMES)) && + (entry['line'] === undefined || typeof entry['line'] === 'number') && + (entry['id'] === undefined || typeof entry['id'] === 'string') && + (entry['category'] === undefined || + typeof entry['category'] === 'string') && + (entry['outcomeNote'] === undefined || + typeof entry['outcomeNote'] === 'string'), + ); +} + function formatToolResultDisplay( value: unknown, ): IndividualToolCallDisplay['resultDisplay'] { @@ -283,7 +337,17 @@ function formatToolResultDisplay( ) { return sanitizeDisplayText(value['fallbackText']); } - if ( + if (isRecord(value) && value['type'] === 'findings_list') { + // Discriminator-first rejection: a findings_list record that fails the + // full shape check falls back to the text rendering below no matter + // what other keys it carries, so `ansiOutput`/`fileDiff` cannot smuggle + // it past the guard and into FindingsDisplay. + if (isFindingsListDisplay(value)) { + return sanitizeDaemonValue( + value, + ) as IndividualToolCallDisplay['resultDisplay']; + } + } else if ( isRecord(value) && (typeof value['fileDiff'] === 'string' || 'ansiOutput' in value || @@ -391,6 +455,20 @@ function toolUpdateToHistoryItem( state.toolCallsById.delete(oldest); } } + // A delivered findings list REPLACES the session's earlier one: the + // projection keeps a single logical report, so the previous list's + // entry takes the replacement marker instead of rendering beside it. + if (isFindingsListDisplay(tool.resultDisplay)) { + for (const [id, entry] of state.toolCallsById) { + if (id === toolCallId) continue; + if (isFindingsListDisplay(entry.resultDisplay)) { + state.toolCallsById.set(id, { + ...entry, + resultDisplay: SUPERSEDED_FINDINGS_MESSAGE, + }); + } + } + } } return { type: 'tool_group', diff --git a/packages/cli/src/ui/hooks/useHistoryManager.test.ts b/packages/cli/src/ui/hooks/useHistoryManager.test.ts index b88e5738ee4..f394ae3457d 100644 --- a/packages/cli/src/ui/hooks/useHistoryManager.test.ts +++ b/packages/cli/src/ui/hooks/useHistoryManager.test.ts @@ -14,6 +14,7 @@ import { import type { UseHistoryManagerReturn } from './useHistoryManager.js'; import type { HistoryItemWithoutId, HistoryItemToolGroup } from '../types.js'; import { ToolCallStatus } from '../types.js'; +import { SUPERSEDED_FINDINGS_MESSAGE } from '../utils/findings-coalescing.js'; const { debugLoggerMock } = vi.hoisted(() => ({ debugLoggerMock: { @@ -62,6 +63,56 @@ describe('useHistoryManager', () => { expect(result.current.history[0].id).toBeGreaterThanOrEqual(timestamp); }); + it('replaces earlier findings displays when a new report_findings group commits', () => { + // A delivered findings list REPLACES the session's earlier one: the + // previous group's display collapses to the marker at commit time, so + // every re-render surface shows only the latest list. + const { result } = renderHook(() => useHistory()); + const findingsGroup = (id: string, outcome?: 'fixed') => ({ + type: 'tool_group' as const, + tools: [ + { + callId: id, + name: 'ReportFindings', + description: 'Report 1 finding', + status: ToolCallStatus.Success, + confirmationDetails: undefined, + resultDisplay: { + type: 'findings_list' as const, + findings: [ + { + id: 'R1-1', + severity: 'Critical' as const, + file: 'src/foo.ts', + summary: 's', + shortSummary: 's', + failureScenario: 'f', + ...(outcome ? { outcome } : {}), + }, + ], + }, + }, + ], + }); + + act(() => { + result.current.addItem(findingsGroup('call-1'), Date.now()); + }); + act(() => { + result.current.addItem(findingsGroup('call-2', 'fixed'), Date.now()); + }); + + expect(result.current.history).toHaveLength(2); + const [first, second] = result.current.history as HistoryItemToolGroup[]; + expect(first.tools[0].resultDisplay).toBe(SUPERSEDED_FINDINGS_MESSAGE); + const latest = second.tools[0].resultDisplay as { + type: string; + findings: Array<{ outcome?: string }>; + }; + expect(latest.type).toBe('findings_list'); + expect(latest.findings[0].outcome).toBe('fixed'); + }); + it('should generate unique IDs for items added with the same base timestamp', () => { const { result } = renderHook(() => useHistory()); const timestamp = Date.now(); @@ -374,6 +425,81 @@ describe('useHistoryManager', () => { expect(recentTool.detailedDisplay).toBe('full secret file content here'); }); + it('also drops the carried superseded findings display when compacting (Ctrl+O privacy)', () => { + const { result } = renderHook(() => useHistory()); + const ts = Date.now(); + const findingsGroup = (callId: string) => ({ + type: 'tool_group' as const, + tools: [ + { + callId, + name: 'report_findings', + description: 'Report findings', + status: ToolCallStatus.Success, + confirmationDetails: undefined, + resultDisplay: { + type: 'findings_list' as const, + findings: [ + { + id: 'R1-1', + severity: 'Critical' as const, + file: 'src/foo.ts', + summary: 's', + shortSummary: 's', + failureScenario: 'f', + }, + ], + }, + }, + ], + }); + + act(() => { + result.current.addItem(findingsGroup('call-1'), ts); + }); + act(() => { + result.current.addItem(findingsGroup('call-2'), ts + 1); + }); + // The second report superseded the first; the first tool now carries + // the marker plus the original display for rewind recovery. + const superseded = ( + result.current.history[0] as unknown as HistoryItemToolGroup + ).tools[0]; + expect(superseded.resultDisplay).toBe(SUPERSEDED_FINDINGS_MESSAGE); + expect(superseded.supersededFindingsDisplay).toBeDefined(); + + for (let i = 0; i < 24; i++) { + act(() => { + result.current.addItem( + { + type: 'tool_group', + tools: [ + { + callId: `plain-${i}`, + name: 'read_file', + description: '', + resultDisplay: `content-${i}`, + status: ToolCallStatus.Success, + confirmationDetails: undefined, + }, + ], + } as unknown as HistoryItemWithoutId, + ts + 2 + i, + ); + }); + } + + act(() => { + result.current.compactOldItems(); + }); + + const compacted = ( + result.current.history[0] as unknown as HistoryItemToolGroup + ).tools[0]; + expect(compacted.resultDisplay).toBe(UI_COMPACT_CLEARED_MESSAGE); + expect(compacted.supersededFindingsDisplay).toBeUndefined(); + }); + it('clears image payloads from old tool results', () => { const { result } = renderHook(() => useHistory()); const ts = Date.now(); diff --git a/packages/cli/src/ui/hooks/useHistoryManager.ts b/packages/cli/src/ui/hooks/useHistoryManager.ts index 9aed5ba379d..5db73b0f126 100644 --- a/packages/cli/src/ui/hooks/useHistoryManager.ts +++ b/packages/cli/src/ui/hooks/useHistoryManager.ts @@ -7,6 +7,10 @@ import { useState, useRef, useCallback, useMemo } from 'react'; import { createDebugLogger } from '@qwen-code/qwen-code-core'; import type { HistoryItem, HistoryItemWithoutId } from '../types.js'; +import { + coalesceFindingsHistoryItems, + isFindingsListDisplay, +} from '../utils/findings-coalescing.js'; import process from 'node:process'; const debugLogger = createDebugLogger('HISTORY_MANAGER'); @@ -82,6 +86,18 @@ export function useHistory(): UseHistoryManagerReturn { `historyLength=${newHistory.length}`, ); } + // A delivered report_findings list REPLACES the session's earlier + // one; collapse the superseded displays the moment the new group + // commits so live history, the Ctrl+O transcript, and every + // re-render surface show only the latest list. + if ( + newItem.type === 'tool_group' && + newItem.tools.some((tool) => + isFindingsListDisplay(tool.resultDisplay), + ) + ) { + return coalesceFindingsHistoryItems(newHistory); + } return newHistory; }); return id; // Return the generated ID (even if not added, to keep signature) @@ -264,6 +280,7 @@ export function useHistory(): UseHistoryManagerReturn { ...t, resultDisplay: UI_COMPACT_CLEARED_MESSAGE, detailedDisplay: undefined, + supersededFindingsDisplay: undefined, images: undefined, omittedImageCount: undefined, }; diff --git a/packages/cli/src/ui/types.ts b/packages/cli/src/ui/types.ts index 92d9a2b2e7c..414f4e925f8 100644 --- a/packages/cli/src/ui/types.ts +++ b/packages/cli/src/ui/types.ts @@ -7,6 +7,7 @@ import type { CompactionThresholds, CompressionStatus, + FindingsResultDisplay, MCPServerConfig, ThoughtSummary, ToolCallConfirmationDetails, @@ -84,6 +85,12 @@ export interface IndividualToolCallDisplay { * is only a count. Undefined → fall back to the summary. */ detailedDisplay?: string; + /** + * The findings display a later report_findings call replaced. Kept so a + * rewind past the replacing call can restore this report's checklist; + * dropped by history compaction together with resultDisplay. + */ + supersededFindingsDisplay?: FindingsResultDisplay; /** Inline images carried by this tool's persisted response parts. */ images?: InlineImageData[]; /** Images hidden after the per-row rendering limit. */ diff --git a/packages/cli/src/ui/utils/findings-coalescing.test.ts b/packages/cli/src/ui/utils/findings-coalescing.test.ts new file mode 100644 index 00000000000..f3ed1a0d86f --- /dev/null +++ b/packages/cli/src/ui/utils/findings-coalescing.test.ts @@ -0,0 +1,158 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import type { FindingsResultDisplay } from '@qwen-code/qwen-code-core'; +import type { + HistoryItemWithoutId, + IndividualToolCallDisplay, +} from '../types.js'; +import { ToolCallStatus } from '../types.js'; +import { + SUPERSEDED_FINDINGS_MESSAGE, + coalesceFindingsHistoryItems, + recoalesceFindingsHistoryItems, +} from './findings-coalescing.js'; + +const findingsDisplay = (findingId: string): FindingsResultDisplay => ({ + type: 'findings_list', + findings: [ + { + id: findingId, + severity: 'Critical', + file: 'src/foo.ts', + summary: 's', + shortSummary: 's', + failureScenario: 'f', + }, + ], +}); + +const findingsTool = ( + callId: string, + resultDisplay: IndividualToolCallDisplay['resultDisplay'], +): IndividualToolCallDisplay => ({ + callId, + name: 'report_findings', + description: 'Report findings', + resultDisplay, + status: ToolCallStatus.Success, + confirmationDetails: undefined, +}); + +const toolGroup = (tool: IndividualToolCallDisplay): HistoryItemWithoutId => ({ + type: 'tool_group', + tools: [tool], +}); + +const userItem = (text: string): HistoryItemWithoutId => ({ + type: 'user', + text, +}); + +describe('coalesceFindingsHistoryItems', () => { + it('keeps the superseded display on the tool so a rewind can restore it', () => { + const original = findingsDisplay('R1-1'); + const items = [ + toolGroup(findingsTool('call-1', original)), + toolGroup(findingsTool('call-2', findingsDisplay('R2-1'))), + ]; + + const coalesced = coalesceFindingsHistoryItems(items); + + const first = coalesced[0] as Extract< + HistoryItemWithoutId, + { type: 'tool_group' } + >; + expect(first.tools[0].resultDisplay).toBe(SUPERSEDED_FINDINGS_MESSAGE); + expect(first.tools[0].supersededFindingsDisplay).toBe(original); + }); + + it('keeps the original display when a third report supersedes again', () => { + const original = findingsDisplay('R1-1'); + const once = coalesceFindingsHistoryItems([ + toolGroup(findingsTool('call-1', original)), + toolGroup(findingsTool('call-2', findingsDisplay('R2-1'))), + ]); + const twice = coalesceFindingsHistoryItems([ + ...once, + toolGroup(findingsTool('call-3', findingsDisplay('R3-1'))), + ]); + + const first = twice[0] as Extract< + HistoryItemWithoutId, + { type: 'tool_group' } + >; + expect(first.tools[0].resultDisplay).toBe(SUPERSEDED_FINDINGS_MESSAGE); + expect(first.tools[0].supersededFindingsDisplay).toBe(original); + }); +}); + +describe('recoalesceFindingsHistoryItems', () => { + it('restores a superseded display whose replacing call was truncated away', () => { + const original = findingsDisplay('R1-1'); + const coalesced = coalesceFindingsHistoryItems([ + userItem('first prompt'), + toolGroup(findingsTool('call-1', original)), + userItem('second prompt'), + toolGroup(findingsTool('call-2', findingsDisplay('R2-1'))), + ]); + + // The rewind slice: everything before the second user item. + const truncated = coalesced.slice(0, 2); + const repaired = recoalesceFindingsHistoryItems(truncated); + + const restored = repaired[1] as Extract< + HistoryItemWithoutId, + { type: 'tool_group' } + >; + expect(restored.tools[0].resultDisplay).toBe(original); + expect(restored.tools[0].supersededFindingsDisplay).toBeUndefined(); + }); + + it('keeps only the last list when both reports survive the truncation', () => { + const first = findingsDisplay('R1-1'); + const second = findingsDisplay('R2-1'); + const coalesced = coalesceFindingsHistoryItems([ + toolGroup(findingsTool('call-1', first)), + userItem('prompt'), + toolGroup(findingsTool('call-2', second)), + ]); + + const repaired = recoalesceFindingsHistoryItems(coalesced); + + const repairedFirst = repaired[0] as Extract< + HistoryItemWithoutId, + { type: 'tool_group' } + >; + const repairedSecond = repaired[2] as Extract< + HistoryItemWithoutId, + { type: 'tool_group' } + >; + expect(repairedFirst.tools[0].resultDisplay).toBe( + SUPERSEDED_FINDINGS_MESSAGE, + ); + expect(repairedFirst.tools[0].supersededFindingsDisplay).toBe(first); + expect(repairedSecond.tools[0].resultDisplay).toBe(second); + }); + + it('returns the input unchanged when nothing is superseded', () => { + const items = [ + userItem('prompt'), + toolGroup(findingsTool('call-1', findingsDisplay('R1-1'))), + ]; + expect(recoalesceFindingsHistoryItems(items)).toBe(items); + }); + + it('leaves a marker without a carried display alone', () => { + const items = [ + toolGroup({ + ...findingsTool('call-1', SUPERSEDED_FINDINGS_MESSAGE), + }), + ]; + expect(recoalesceFindingsHistoryItems(items)).toBe(items); + }); +}); diff --git a/packages/cli/src/ui/utils/findings-coalescing.ts b/packages/cli/src/ui/utils/findings-coalescing.ts new file mode 100644 index 00000000000..9205ac9366f --- /dev/null +++ b/packages/cli/src/ui/utils/findings-coalescing.ts @@ -0,0 +1,114 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The report_findings contract is "a later call replaces the whole list, it +// never appends." The tool enforces that against the active report's +// identity; this module is the transcript side of the same rule. Every +// delivered report in one session is a successive state of a single logical +// report, so a rendered transcript keeps only the LAST delivered list and +// collapses every earlier one to a one-line marker — the initial report and +// its outcome re-report must not show two checklists side by side. + +import type { FindingsResultDisplay } from '@qwen-code/qwen-code-core'; +import type { + HistoryItemWithoutId, + IndividualToolCallDisplay, +} from '../types.js'; + +/** Replaces the display of a findings list a later call superseded. */ +export const SUPERSEDED_FINDINGS_MESSAGE = + '(findings replaced by a later report_findings call)'; + +export function isFindingsListDisplay( + value: unknown, +): value is FindingsResultDisplay { + return ( + typeof value === 'object' && + value !== null && + 'type' in value && + (value as { type: unknown }).type === 'findings_list' + ); +} + +/** + * Keeps only the last delivered findings list across the tool groups; every + * earlier one takes the replacement marker. The superseded display stays on + * the tool so a rewind past the replacing call can restore it. Returns the + * input array itself when there is nothing to coalesce (at most one + * delivered list). + */ +export function coalesceFindingsHistoryItems( + items: T[], +): T[] { + let lastItem = -1; + let lastTool = -1; + for (let i = items.length - 1; i >= 0 && lastItem === -1; i -= 1) { + const item = items[i]; + if (item.type !== 'tool_group') continue; + for (let j = item.tools.length - 1; j >= 0; j -= 1) { + if (isFindingsListDisplay(item.tools[j].resultDisplay)) { + lastItem = i; + lastTool = j; + break; + } + } + } + if (lastItem === -1) return items; + + let changed = false; + const next = items.map((item, i) => { + if (item.type !== 'tool_group') return item; + let toolsChanged = false; + const tools: IndividualToolCallDisplay[] = item.tools.map((tool, j) => { + if (i === lastItem && j === lastTool) return tool; + if (!isFindingsListDisplay(tool.resultDisplay)) return tool; + toolsChanged = true; + return { + ...tool, + resultDisplay: SUPERSEDED_FINDINGS_MESSAGE, + supersededFindingsDisplay: + tool.supersededFindingsDisplay ?? tool.resultDisplay, + }; + }); + if (!toolsChanged) return item; + changed = true; + return { ...item, tools } as T; + }); + return changed ? next : items; +} + +/** + * Truncation/rewind repair: restores superseded displays whose replacing + * call no longer survives, then coalesces the survivors again so the + * keep-only-the-last-list invariant holds over the truncated transcript. + */ +export function recoalesceFindingsHistoryItems( + items: T[], +): T[] { + let restored = false; + const next = items.map((item) => { + if (item.type !== 'tool_group') return item; + let toolsChanged = false; + const tools: IndividualToolCallDisplay[] = item.tools.map((tool) => { + if ( + tool.resultDisplay !== SUPERSEDED_FINDINGS_MESSAGE || + !tool.supersededFindingsDisplay + ) { + return tool; + } + toolsChanged = true; + return { + ...tool, + resultDisplay: tool.supersededFindingsDisplay, + supersededFindingsDisplay: undefined, + }; + }); + if (!toolsChanged) return item; + restored = true; + return { ...item, tools } as T; + }); + return coalesceFindingsHistoryItems(restored ? next : items); +} diff --git a/packages/cli/src/ui/utils/resumeHistoryUtils.test.ts b/packages/cli/src/ui/utils/resumeHistoryUtils.test.ts index f1e7bf7e9cd..66b69599f5f 100644 --- a/packages/cli/src/ui/utils/resumeHistoryUtils.test.ts +++ b/packages/cli/src/ui/utils/resumeHistoryUtils.test.ts @@ -12,10 +12,12 @@ import { expandCollapsedHistory, } from './resumeHistoryUtils.js'; import { MessageType, ToolCallStatus } from '../types.js'; +import { SUPERSEDED_FINDINGS_MESSAGE } from './findings-coalescing.js'; import type { AnyDeclarativeTool, Config, ConversationRecord, + FindingsResultDisplay, GoalSnapshotV2, ResumedSessionData, } from '@qwen-code/qwen-code-core'; @@ -1853,4 +1855,92 @@ describe('expandCollapsedHistory', () => { const result = expandCollapsedHistory(items); expect(result).toEqual([]); }); + + describe('report_findings replacement semantics', () => { + const reportFindingsTool = { + name: 'report_findings', + displayName: 'ReportFindings', + description: 'Report findings', + build: vi + .fn() + .mockReturnValue({ getDescription: () => 'Report 1 finding' }), + } as unknown as AnyDeclarativeTool; + + const findingsDisplay = (outcome?: 'fixed') => ({ + type: 'findings_list', + level: 'high', + findings: [ + { + id: 'R1-1', + severity: 'Critical', + confidence: 'high', + file: 'src/foo.ts', + line: 42, + summary: 'wrong return value', + shortSummary: 'wrong return', + failureScenario: 'first call returns undefined', + ...(outcome ? { outcome } : {}), + }, + ], + }); + + it('keeps only the latest delivered findings list in the restored transcript', () => { + // An initial report, fix work, then the outcome re-report: two distinct + // tool records. The restored transcript must render ONLY the latest + // list — the initial report collapses to the replacement marker. + const reportCall = (callId: string) => ({ + type: 'assistant', + message: { + parts: [ + { + functionCall: { id: callId, name: 'report_findings', args: {} }, + } as unknown as Part, + ], + }, + }); + const reportResult = (callId: string, resultDisplay: unknown) => ({ + type: 'tool_result', + toolCallResult: { callId, resultDisplay, status: 'success' }, + }); + const conversation = { + messages: [ + reportCall('call-report-1'), + reportResult('call-report-1', findingsDisplay()), + { + type: 'assistant', + message: { parts: [{ text: 'applying fixes' }] }, + }, + reportCall('call-report-2'), + reportResult('call-report-2', findingsDisplay('fixed')), + ], + } as unknown as ConversationRecord; + + const items = buildResumedHistoryItems( + { conversation } as ResumedSessionData, + makeConfig({ report_findings: reportFindingsTool }), + 500, + ); + + type ToolGroupItem = Extract; + const toolGroups = items.filter( + (i): i is ToolGroupItem => i.type === 'tool_group', + ); + expect(toolGroups).toHaveLength(2); + + const everyTool = toolGroups.flatMap((group) => group.tools); + const deliveredLists = everyTool.filter( + ( + tool, + ): tool is typeof tool & { resultDisplay: FindingsResultDisplay } => + typeof tool.resultDisplay === 'object' && + (tool.resultDisplay as FindingsResultDisplay | undefined)?.type === + 'findings_list', + ); + expect(deliveredLists).toHaveLength(1); + expect(deliveredLists[0].resultDisplay.findings[0].outcome).toBe('fixed'); + expect(toolGroups[0].tools[0].resultDisplay).toBe( + SUPERSEDED_FINDINGS_MESSAGE, + ); + }); + }); }); diff --git a/packages/cli/src/ui/utils/resumeHistoryUtils.ts b/packages/cli/src/ui/utils/resumeHistoryUtils.ts index 85f6a71c95b..cf99cabc967 100644 --- a/packages/cli/src/ui/utils/resumeHistoryUtils.ts +++ b/packages/cli/src/ui/utils/resumeHistoryUtils.ts @@ -38,6 +38,7 @@ import { formatHistoryGapNotice, indexGapsByChild, } from './history-gap-notice.js'; +import { coalesceFindingsHistoryItems } from './findings-coalescing.js'; import { shouldDisplayGoalStateCause } from './goal-runtime.js'; import { collectInlineImages, @@ -631,7 +632,10 @@ function convertToHistoryItems( }); } - return items; + // A report_findings re-report REPLACES the earlier list — restored + // transcripts collapse the superseded displays so the initial report and + // its outcome re-report do not render two checklists at once. + return coalesceFindingsHistoryItems(items); } /** diff --git a/packages/core/src/agents/runtime/agent-core.ts b/packages/core/src/agents/runtime/agent-core.ts index 1784527e582..23c503dd8d1 100644 --- a/packages/core/src/agents/runtime/agent-core.ts +++ b/packages/core/src/agents/runtime/agent-core.ts @@ -242,7 +242,8 @@ export function extractParentToolNames( new Set( ( generationConfig?.tools as - Array<{ functionDeclarations?: FunctionDeclaration[] }> | undefined + | Array<{ functionDeclarations?: FunctionDeclaration[] }> + | undefined ) ?.flatMap((tool) => tool.functionDeclarations ?? []) .map((declaration) => declaration.name) @@ -1539,7 +1540,8 @@ export class AgentCore { const registeredTool = this.runtimeContext .getToolRegistry() .getTool(toolName) as - { serverName?: unknown; serverToolName?: unknown } | undefined; + | { serverName?: unknown; serverToolName?: unknown } + | undefined; if ( typeof registeredTool?.serverName !== 'string' || typeof registeredTool.serverToolName !== 'string' diff --git a/packages/core/src/config/config.test.ts b/packages/core/src/config/config.test.ts index 3ebe8d81ac1..7284c5723fe 100644 --- a/packages/core/src/config/config.test.ts +++ b/packages/core/src/config/config.test.ts @@ -4744,6 +4744,20 @@ describe('Server Config (config.ts)', () => { expect(registeredNames).toContain(ToolNames.RECORD_ARTIFACT); }); + it('registers report_findings even in headless sessions — review run depends on it', async () => { + const config = new Config({ + ...baseParams, + interactive: false, + sdkMode: false, + }); + await config.initialize(); + + const registeredNames = ( + ToolRegistry.prototype.registerFactory as Mock + ).mock.calls.map((call) => call[0]); + expect(registeredNames).toContain(ToolNames.REPORT_FINDINGS); + }); + describe('isArtifactEnabled', () => { const originalForceEnable = process.env['QWEN_CODE_ENABLE_ARTIFACT']; const originalDisable = process.env['QWEN_CODE_DISABLE_ARTIFACT']; diff --git a/packages/core/src/config/config.ts b/packages/core/src/config/config.ts index 82e62d5a34f..7cf7bba71ca 100644 --- a/packages/core/src/config/config.ts +++ b/packages/core/src/config/config.ts @@ -8811,6 +8811,12 @@ export class Config { const { TodoWriteTool } = await import('../tools/todoWrite.js'); return new TodoWriteTool(this); }); + await registerLazy(ToolNames.REPORT_FINDINGS, async () => { + const { ReportFindingsTool } = await import( + '../tools/report-findings.js' + ); + return new ReportFindingsTool(); + }); const supportsUserInteraction = resolveInteractionMode(this) !== 'headless'; if (supportsUserInteraction) { await registerLazy(ToolNames.ASK_USER_QUESTION, async () => { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 5453a928a14..08c158d81ca 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -234,6 +234,24 @@ export { isRecordableDerivedChild, } from './tools/record-artifact.js'; export type { RecordArtifactParams } from './tools/record-artifact.js'; +export { + ReportFindingsTool, + FINDING_SEVERITIES, + FINDING_CONFIDENCES, + FINDING_OUTCOMES, + FINDING_SOURCES, + REPORT_FINDINGS_LEVELS, + compressFindingSummary, +} from './tools/report-findings.js'; +export type { + ReportFindingsParams, + ReportFindingsFindingParams, + FindingSeverity, + FindingConfidence, + FindingOutcome, + FindingSource, + ReportFindingsLevel, +} from './tools/report-findings.js'; export { CreateSubSessionTool } from './tools/create-sub-session.js'; export type { ArtifactPublisher, diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index cab01680dbd..8da0aa6eca2 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -11,6 +11,7 @@ allowedTools: - edit - glob - record_artifact + - report_findings --- # Code Review @@ -581,6 +582,7 @@ Low uses the standard finding format, including **Failure scenario**, and the re Then skip Steps 4 and 5 entirely and go to Step 6 with these adjustments: - Use Step 6's structure, but label the review **"Quick pass (effort: low) — findings are unverified"** (translated per output language) in the Summary, and skip verification stats (there was no verification). +- Still make Step 6's `report_findings` call, with `level: "low"`. No findings artifact exists at this tier, so the entries come from the pooled list you just composed — `severity`, `file`/`line`, `summary`, `shortSummary`, `failureScenario` — with `confidence: "low"` only on the candidates you kept under `Confidence: low`, omitted elsewhere: the `low` level already labels the whole list unverified, and a blanket `confidence` would erase the one distinction the pass recorded. Step 6's delivery rule applies unchanged — a failure is disclosed and moved past, never a reason to change the findings. - Emit **no verdict** — no Approve / Request changes / Comment, and skip the open-Criticals re-check (that gate defends a verdict this pass does not claim). Chunks that are uncoverable by `maxLineChars` are still listed under "Not reviewed". - Follow-up tip (translated per output language, critical rule 2 — command keywords stay verbatim): "Tip: run `/review --effort medium` for a verified balanced review, or `--effort high` for the full verified review." For a local review with findings, also offer the `fix these issues` tip. - Step 7 never runs — `--comment` forces high effort, and if the user asks to "post comments" after a quick pass, decline and point at `--effort high` (unverified findings must not be posted publicly). @@ -910,6 +912,8 @@ Write every confirmed finding — high and low confidence alike — as a JSON ar 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`, `fixWitness`, `category`, `shortSummary` and `witness` are optional; `shortSummary` is derived from `summary` when absent; `witness` is the Step 4 witness — the executed evidence, or its `not run — ` line — carried as data so the report and the comment bodies quote one recorded string instead of transcribing it twice more; `fixWitness` is the acceptance criterion the finding format asks for — the test that must go red if the suggested fix is removed, or `N/A` — carried for the same reason and read back by Step 7's comment body). 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. +**Then speak the same list to the client, in-band — one `report_findings` tool call.** The artifact is the canonical record, but it is a file on disk registered after the fact (Step 8); every client rendering this session live — the TUI, the Web Shell transcript, an ACP host — otherwise sees only the prose restatement, which is the transcription surface the artifact exists to close. Immediately after the artifact is written, call the `report_findings` tool once (load it via `tool_search` if it is not in your tool list) — each call replaces the whole list, and Step 6B re-issues it with outcomes after a fix run — with `level` set to this review's effort and one entry per finding **copied from the artifact you just wrote** — `id`, `severity`, `confidence`, `source`, `file`/`line` (a pattern aggregate passes its first location; the artifact keeps the rest), `summary`, `shortSummary`, `failureScenario`, `category` — never re-typed from the terminal prose: the artifact is the oracle, and a re-derived severity here is the same drift the marker rule below closes. A finding the convergence posture deferred is still a finding — report it under its `D-` id like any other. **The tool's contract is harder-bounded than the artifact's, and a violation refuses the whole call**: at most 50 findings, with per-field length caps the schema states. When the artifact outgrows those bounds, do not let the call die on them — pass the first 50 findings in artifact order (the artifact is already sorted most-severe-first) and say in the terminal summary how many the cap cut, and shorten an over-cap `summary`/`failureScenario` — or `outcomeNote` on the Step 6B re-report — to fit rather than dropping the entry (the artifact keeps the full-length text, so nothing is lost by a delivery-only shortening). This is the one sanctioned departure from copy-verbatim, and it is a departure of length only, never of severity, confidence, or meaning — a bounded list delivered beats a complete list refused. This call is UI delivery, not bookkeeping: it persists nothing and decides nothing, and a failure (or an environment where the tool is not registered and `tool_search` cannot find it) is disclosed and moved past — never a reason to touch the artifact, the compose state, or the verdict, exactly the rule `record_artifact` follows in Step 8. + **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; DESIGN.md — The four-round misattributed Critical (#8368)). 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. @@ -991,6 +995,8 @@ Then record what happened to **every** finding — one of `fixed`, `skipped`, or 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. +**Then re-issue the `report_findings` call, outcomes on it.** Re-report the same findings — fields copied from the rebuilt artifact, exactly as Step 6's call prescribes — each entry now carrying its `outcome`, and the ledger's note as `outcomeNote` for every `skipped`. The client's per-finding status trusts only a `report_findings` call that carries outcomes — the tool refuses a partial set for the same reason the command above refuses a partial ledger — so a tree edited without re-reporting leaves every client rendering as open the findings the tree already closed. **And this rule outlives Step 6B: any later time in this session a reported finding's disposition changes** — the user has you `fix these issues`, a finding is established to be wrong, a fix lands mid-conversation — record the outcomes into the artifact (`review findings --outcomes`) and re-issue the call with them. When Step 9 cleanup has already swept the `findings-in.json` side file, pass the saved artifact (Step 8's `save-artifact` output under `.qwen/reviews/`) as `--input` instead — the command accepts that wrapper and unwraps its `findings` array, so the outcome path recovers from the state that survives cleanup. + Report the outcome counts in the terminal summary, and list each `skipped` finding with its reason. **Do not re-run Steps 1–6** to check your own work: a re-review of a tree you just edited is a new review of different code, and its verdict is not this review's. Append a follow-up tip after the verdict (high and medium effort — only a **low** quick pass emits no verdict and uses Step 3C's tip instead; its "post comments" follow-up is declined per Step 3C). **Tip lines are user-facing terminal prose — translate them into your output language** (critical rule 2). The English templates below define the _content_ and the _command keywords_ (which stay verbatim — `post comments`, `fix these issues`, `commit` are trigger phrases the user types back); translate the surrounding sentence. With a Chinese output language, "Tip: type `post comments` to publish findings as PR inline comments." becomes "提示：输入 `post comments` 将发现作为 PR 行内评论发布。" At **medium**, also add: "Tip: run `/review --effort high` for the full verified review (adds the reverse audit, the language-pitfall and wrapper/proxy specialists, the adversarial personas, and Agent 8 — and can certify Approve)." Choose the rest based on remaining state: @@ -1001,7 +1007,7 @@ Append a follow-up tip after the verdict (high and medium effort — only a **lo - **PR review, zero findings** (only if `comment.effective` is false): "Tip: type `post comments` to approve this PR on GitHub." - **Local review, all clear** (Approve or all issues fixed): "Tip: type `commit` to commit your changes." -If the user responds with "fix these issues" (local review only), use the `edit` tool to fix each remaining finding interactively based on the suggested fixes from the review — do NOT re-run Steps 1-6. This is the same work Step 6B does; when the review has a findings artifact, record the outcomes into it the same way (`review findings --outcomes`) rather than leaving the list and the tree disagreeing about what was applied. +If the user responds with "fix these issues" (local review only), use the `edit` tool to fix each remaining finding interactively based on the suggested fixes from the review — do NOT re-run Steps 1-6. This is the same work Step 6B does; when the review has a findings artifact, record the outcomes into it the same way (`review findings --outcomes`) and re-issue the `report_findings` call with the outcomes, exactly as Step 6B prescribes, rather than leaving the list, the tree, and the client display disagreeing about what was applied. If the user responds with "post comments" (or similar intent like "yes post them", "publish comments"), proceed directly to Step 7 using the findings already collected — do NOT re-run Steps 1-6. diff --git a/packages/core/src/tools/report-findings.test.ts b/packages/core/src/tools/report-findings.test.ts new file mode 100644 index 00000000000..be5a2f5381c --- /dev/null +++ b/packages/core/src/tools/report-findings.test.ts @@ -0,0 +1,596 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, expect, it } from 'vitest'; +import { + ReportFindingsTool, + compressFindingSummary, + REPORT_FINDINGS_FILE_MAX, + REPORT_FINDINGS_MAX, + type ReportFindingsFindingParams, + type ReportFindingsParams, +} from './report-findings.js'; +import type { FindingsResultDisplay } from './tools.js'; + +function finding( + overrides: Partial = {}, +): ReportFindingsFindingParams { + return { + severity: 'Critical', + file: 'src/foo.ts', + line: 42, + summary: 'wrong return value on cold cache', + failureScenario: 'first call after start returns undefined', + ...overrides, + }; +} + +async function run(params: ReportFindingsParams) { + const tool = new ReportFindingsTool(); + const invocation = tool.build(params); + return invocation.execute(new AbortController().signal); +} + +function displayOf(result: { returnDisplay: unknown }): FindingsResultDisplay { + return result.returnDisplay as FindingsResultDisplay; +} + +describe('ReportFindingsTool', () => { + it('reports findings as a findings_list display with counts in llmContent', async () => { + const result = await run({ + level: 'high', + findings: [ + finding(), + finding({ + severity: 'Suggestion', + file: 'src/bar.ts', + summary: 'duplicated helper', + failureScenario: 'two copies drift', + }), + ], + }); + const display = displayOf(result); + expect(display.type).toBe('findings_list'); + expect(display.level).toBe('high'); + expect(display.findings).toHaveLength(2); + expect(result.llmContent).toContain('2 findings'); + expect(result.llmContent).toContain('1 Critical'); + expect(result.llmContent).toContain('1 Suggestion'); + expect(result.error).toBeUndefined(); + }); + + it('sorts severity first, then confidence, then location', async () => { + const result = await run({ + findings: [ + finding({ + severity: 'Nice to have', + file: 'a.ts', + summary: 'nit', + failureScenario: 'cost', + }), + finding({ + severity: 'Critical', + confidence: 'low', + file: 'z.ts', + summary: 'possible race', + failureScenario: 'unlikely interleaving', + }), + finding({ + severity: 'Critical', + confidence: 'high', + file: 'z.ts', + summary: 'confirmed race', + failureScenario: 'interleaving observed', + }), + finding({ + severity: 'Suggestion', + file: 'm.ts', + summary: 'clearer name', + failureScenario: 'reader cost', + }), + ], + }); + const summaries = displayOf(result).findings.map((f) => f.summary); + expect(summaries).toEqual([ + 'confirmed race', + 'possible race', + 'clearer name', + 'nit', + ]); + }); + + it('breaks location ties the way the artifact does: missing line first, then id by code units', async () => { + // The two entries on z.ts:42 arrive with the LOWER-sorting id last, so a + // dropped id tiebreak (stable sort keeps input order) flips the expected + // order; the body finding has no line and must rank before every + // line-anchored one on the same file (`?? 0`, the artifact's rule). + const result = await run({ + findings: [ + finding({ + id: 'R1-2', + file: 'z.ts', + line: 42, + summary: 'second by id', + failureScenario: 'tie', + }), + finding({ + id: 'R1-10', + file: 'z.ts', + line: 42, + summary: 'first by id', + failureScenario: 'tie', + }), + finding({ + id: 'R1-3', + file: 'z.ts', + line: undefined, + summary: 'body finding without a line', + failureScenario: 'unanchored', + }), + ], + }); + expect(displayOf(result).findings.map((f) => f.summary)).toEqual([ + 'body finding without a line', + 'first by id', + 'second by id', + ]); + }); + + it('orders the file and id axes by code units, not locale collation', async () => { + // Mixed-case names are where ICU collation and code-unit order disagree + // ('a' collates before 'B' but ranks after it by code unit); every other + // fixture in this suite is lowercase ASCII, where the two coincide. Each + // pair arrives lower-sorting first, so a dropped axis (stable sort keeps + // input order) or one reverted to localeCompare flips the expected order. + const result = await run({ + findings: [ + finding({ + id: 'a-1', + file: 'a.ts', + summary: 'lowercase file', + failureScenario: 'tie', + }), + finding({ + id: 'B-1', + file: 'B.ts', + summary: 'uppercase file', + failureScenario: 'tie', + }), + finding({ + id: 'a-2', + file: 'z.ts', + line: 7, + summary: 'lowercase id', + failureScenario: 'tie', + }), + finding({ + id: 'B-2', + file: 'z.ts', + line: 7, + summary: 'uppercase id', + failureScenario: 'tie', + }), + ], + }); + expect(displayOf(result).findings.map((f) => f.summary)).toEqual([ + 'uppercase file', + 'lowercase file', + 'uppercase id', + 'lowercase id', + ]); + }); + + it('derives shortSummary from summary, prefers a supplied one, and compresses both', async () => { + // Pins both branches of the `raw.shortSummary?.trim() || raw.summary` + // derivation by exact value: the derived label must be the compressed + // SUMMARY (word-boundary cut), the supplied label must survive as itself. + const longSummary = + 'the retry guard drops the final attempt when the backoff timer fires after the abort signal has already resolved'; + const result = await run({ + findings: [ + finding({ summary: longSummary }), + finding({ file: 'src/other.ts', shortSummary: 'supplied label' }), + finding({ + file: 'src/third.ts', + shortSummary: `supplied ${'x'.repeat(100)}`, + }), + ], + }); + const byFile = Object.fromEntries( + displayOf(result).findings.map((f) => [f.file, f]), + ); + expect(byFile['src/foo.ts'].shortSummary).toBe( + 'the retry guard drops the final attempt when the backoff…', + ); + expect(byFile['src/other.ts'].shortSummary).toBe('supplied label'); + const compressedSupplied = byFile['src/third.ts'].shortSummary; + expect(compressedSupplied.length).toBeLessThanOrEqual(60); + expect(compressedSupplied.startsWith('supplied x')).toBe(true); + expect(compressedSupplied.endsWith('…')).toBe(true); + }); + + it('accepts an empty findings list as a valid nothing-found report', async () => { + const result = await run({ findings: [] }); + expect(displayOf(result).findings).toEqual([]); + expect(result.llmContent).toContain('empty findings list'); + }); + + it('reports outcome counts when every finding carries one', async () => { + const result = await run({ + findings: [ + finding({ id: 'R1-1', outcome: 'fixed' }), + finding({ + id: 'R1-2', + file: 'src/bar.ts', + outcome: 'skipped', + outcomeNote: 'fix would change intended behaviour', + }), + ], + }); + expect(result.llmContent).toContain('1 fixed'); + expect(result.llmContent).toContain('1 skipped'); + expect(displayOf(result).findings.map((f) => f.outcome)).toEqual([ + 'skipped', + 'fixed', + ]); + }); + + it('refuses a partial outcome set', () => { + const tool = new ReportFindingsTool(); + expect(() => + tool.build({ + findings: [ + finding({ outcome: 'fixed' }), + finding({ file: 'src/bar.ts' }), + ], + }), + ).toThrow(/every finding or none/); + }); + + it('refuses duplicate ids', () => { + const tool = new ReportFindingsTool(); + expect(() => + tool.build({ + findings: [ + finding({ id: 'R1-1' }), + finding({ id: 'R1-1', file: 'src/bar.ts' }), + ], + }), + ).toThrow(/duplicate id "R1-1"/); + }); + + it.each([ + ['file', { file: 'src/foo.ts\u0007' }], + ['id', { id: 'R1\u00071' }], + ['summary', { summary: 'beep\u0007boop' }], + ['shortSummary', { shortSummary: 'short\u0007' }], + ['failureScenario', { failureScenario: 'boom\u0007' }], + ['category', { category: 'corr\u0007' }], + ['outcomeNote', { outcome: 'skipped' as const, outcomeNote: 'no\u0007te' }], + ])( + 'refuses control characters in %s', + (_field: string, overrides: Partial) => { + const tool = new ReportFindingsTool(); + expect(() => tool.build({ findings: [finding(overrides)] })).toThrow( + /control characters/, + ); + }, + ); + + it('allows line whitespace only in the prose fields', () => { + const tool = new ReportFindingsTool(); + expect(() => + tool.build({ + findings: [finding({ summary: 'line one\nline two' })], + }), + ).not.toThrow(); + expect(() => + tool.build({ + findings: [finding({ failureScenario: 'step one\nstep two' })], + }), + ).not.toThrow(); + expect(() => + tool.build({ + findings: [ + finding({ outcome: 'skipped', outcomeNote: 'reason one\ntwo' }), + ], + }), + ).not.toThrow(); + expect(() => + tool.build({ + findings: [finding({ file: 'src/\nfoo.ts' })], + }), + ).toThrow(/control characters/); + expect(() => + tool.build({ + findings: [finding({ shortSummary: 'one\ntwo' })], + }), + ).toThrow(/control characters/); + }); + + it('refuses schema violations: missing failureScenario, bad enums, over-long lists', () => { + const tool = new ReportFindingsTool(); + expect(() => + tool.build({ + findings: [ + { severity: 'Critical', file: 'a.ts', summary: 's' }, + ] as ReportFindingsFindingParams[], + }), + ).toThrow(); + expect(() => + tool.build({ + findings: [finding({ severity: 'blocker' as 'Critical' })], + }), + ).toThrow(); + expect(() => + tool.build({ + level: 'ultra' as 'high', + findings: [finding()], + }), + ).toThrow(); + expect(() => + tool.build({ + findings: Array.from({ length: REPORT_FINDINGS_MAX + 1 }, (_, i) => + finding({ file: `src/f${i}.ts` }), + ), + }), + ).toThrow(); + }); + + it('refuses blank required fields after trimming', () => { + const tool = new ReportFindingsTool(); + expect(() => tool.build({ findings: [finding({ file: ' ' })] })).toThrow( + /"file" must not be empty/, + ); + expect(() => + tool.build({ findings: [finding({ summary: ' ' })] }), + ).toThrow(/"summary" must not be empty/); + expect(() => + tool.build({ findings: [finding({ failureScenario: ' ' })] }), + ).toThrow(/"failureScenario" must not be empty/); + }); + + it('trims fields and drops empty optionals in the display', async () => { + const result = await run({ + findings: [ + finding({ + id: ' ', + file: ' src/foo.ts ', + summary: ' padded summary ', + category: '', + }), + ], + }); + const [item] = displayOf(result).findings; + expect(item.id).toBeUndefined(); + expect(item.file).toBe('src/foo.ts'); + expect(item.summary).toBe('padded summary'); + expect(item.category).toBeUndefined(); + }); + + it('passes id and line through to the display item', async () => { + const result = await run({ + findings: [finding({ id: 'R2-7', line: 314 })], + }); + const [item] = displayOf(result).findings; + expect(item.id).toBe('R2-7'); + expect(item.line).toBe(314); + }); + + it('accepts file paths the artifact preserves and refuses beyond the path domain', async () => { + // The artifact keeps repo-relative paths to the filesystem limit; a + // 513-character path it preserves must not refuse the whole in-band list. + const file = `src/${'a'.repeat(506)}.ts`; + expect(file).toHaveLength(513); + const result = await run({ findings: [finding({ file })] }); + expect(displayOf(result).findings[0].file).toBe(file); + const tool = new ReportFindingsTool(); + expect(() => + tool.build({ + findings: [finding({ file: 'a'.repeat(REPORT_FINDINGS_FILE_MAX) })], + }), + ).not.toThrow(); + expect(() => + tool.build({ + findings: [finding({ file: 'a'.repeat(REPORT_FINDINGS_FILE_MAX + 1) })], + }), + ).toThrow(); + }); + + it('accepts MAX_SAFE_INTEGER line numbers and refuses unsafe ones', async () => { + const result = await run({ + findings: [finding({ line: Number.MAX_SAFE_INTEGER })], + }); + expect(displayOf(result).findings[0].line).toBe(Number.MAX_SAFE_INTEGER); + const tool = new ReportFindingsTool(); + // MAX_SAFE_INTEGER + 1 is representable — JSON keeps it, and the + // rounding that produced it is invisible — so the schema alone cannot + // catch it. + expect(() => + tool.build({ + findings: [finding({ line: Number.MAX_SAFE_INTEGER + 1 })], + }), + ).toThrow(/safe range/); + }); + + it('requires a non-empty outcomeNote for every skipped outcome', () => { + const tool = new ReportFindingsTool(); + expect(() => + tool.build({ findings: [finding({ outcome: 'skipped' })] }), + ).toThrow(/"outcomeNote" is required/); + expect(() => + tool.build({ + findings: [finding({ outcome: 'skipped', outcomeNote: ' ' })], + }), + ).toThrow(/"outcomeNote" is required/); + expect(() => + tool.build({ + findings: [ + finding({ outcome: 'skipped', outcomeNote: 'needs a product call' }), + ], + }), + ).not.toThrow(); + }); + + it('refuses an outcome replacement that does not match the active report', async () => { + // Nine findings reported, six fixed: the other three must not silently + // disappear from the client state behind an outcome set that covers only + // what the fixer handled. + const tool = new ReportFindingsTool(); + const nine = Array.from({ length: 9 }, (_, i) => + finding({ id: `R1-${i + 1}`, file: `src/f${i}.ts` }), + ); + await tool.build({ findings: nine }).execute(new AbortController().signal); + + const subset = nine + .slice(0, 6) + .map((f) => ({ ...f, outcome: 'fixed' as const })); + expect(() => tool.build({ findings: subset })).toThrow( + /drops 3 finding\(s\) from the active report: "R1-7", "R1-8", "R1-9"/, + ); + + const ghost = [...nine, finding({ id: 'R1-10', file: 'src/ghost.ts' })].map( + (f) => ({ ...f, outcome: 'fixed' as const }), + ); + expect(() => tool.build({ findings: ghost })).toThrow(/"R1-10"/); + + const full = nine.map((f) => ({ ...f, outcome: 'fixed' as const })); + const result = await tool + .build({ findings: full }) + .execute(new AbortController().signal); + expect(displayOf(result).findings).toHaveLength(9); + + // A fresh report without outcomes replaces the active identity. + await tool + .build({ findings: [finding({ id: 'R2-1', file: 'src/new.ts' })] }) + .execute(new AbortController().signal); + await tool + .build({ + findings: [ + finding({ + id: 'R2-1', + file: 'src/new.ts', + outcome: 'no_change_needed', + }), + ], + }) + .execute(new AbortController().signal); + }); + + it('does not hold an outcome call to a report that had no identity', async () => { + // A low-effort pass reports without artifact ids, so there is no id set + // a later outcome call could be joined against; an empty report + // establishes no identity either. + const tool = new ReportFindingsTool(); + await tool + .build({ findings: [finding()] }) + .execute(new AbortController().signal); + await tool + .build({ findings: [finding({ outcome: 'fixed' })] }) + .execute(new AbortController().signal); + await tool.build({ findings: [] }).execute(new AbortController().signal); + await tool + .build({ findings: [finding({ outcome: 'fixed' })] }) + .execute(new AbortController().signal); + }); + + it('does not commit the identity at build: an undelivered report blocks nothing', async () => { + // The scheduler builds every invocation of a batch up front and can + // discard one before it executes (pre-validation cancellation, an + // aborted signal). Identity committed at build time would make the + // never-delivered round-2 report the active one, rejecting a + // legitimate outcome call for the round-1 report the client shows. + const tool = new ReportFindingsTool(); + await tool + .build({ findings: [finding({ id: 'R1-1' })] }) + .execute(new AbortController().signal); + // Built but never executed — a cancelled turn's dropped call. + tool.build({ findings: [finding({ id: 'R2-1', file: 'src/new.ts' })] }); + + const outcome = await tool + .build({ + findings: [finding({ id: 'R1-1', outcome: 'fixed' })], + }) + .execute(new AbortController().signal); + expect(displayOf(outcome).findings[0].outcome).toBe('fixed'); + }); + + it('clears the identity when a later report has none', async () => { + // The removal transition: an id-less re-report (a low-effort pass) + // replaces the active identity with none, so a following outcome call + // is accepted on its own terms instead of being held to the old ids. + const tool = new ReportFindingsTool(); + await tool + .build({ findings: [finding({ id: 'R1-1' })] }) + .execute(new AbortController().signal); + await tool + .build({ findings: [finding({ file: 'src/new.ts' })] }) + .execute(new AbortController().signal); + + const result = await tool + .build({ + findings: [finding({ id: 'R9-9', outcome: 'fixed' })], + }) + .execute(new AbortController().signal); + expect(displayOf(result).findings).toHaveLength(1); + }); + + it('documents the cold-resume limit: a fresh instance has no active identity', async () => { + // The identity gate is a live-process contract. A cold session resume + // (--resume, a restart) constructs a fresh tool instance that never saw + // the pre-restart report, so an outcome call is validated on its own + // terms — all-or-nothing outcomes — instead of against the old list. + // The same subset the original instance rejects is accepted here. + const original = new ReportFindingsTool(); + await original + .build({ + findings: [ + finding({ id: 'R1-1' }), + finding({ id: 'R1-2', file: 'src/bar.ts' }), + finding({ id: 'R1-3', file: 'src/baz.ts' }), + ], + }) + .execute(new AbortController().signal); + expect(() => + original.build({ + findings: [finding({ id: 'R1-1', outcome: 'fixed' })], + }), + ).toThrow(/drops 2 finding\(s\) from the active report/); + + const resumed = new ReportFindingsTool(); + const result = await resumed + .build({ + findings: [finding({ id: 'R1-1', outcome: 'fixed' })], + }) + .execute(new AbortController().signal); + expect(displayOf(result).findings).toHaveLength(1); + }); +}); + +describe('compressFindingSummary', () => { + it('returns short summaries unchanged, collapsed to one line', () => { + expect(compressFindingSummary('a short\nsummary')).toBe('a short summary'); + }); + + it('cuts on a word boundary with a single ellipsis character', () => { + // Exact value on purpose: a hard cut at 59 units would yield + // '…keeps on ru…', which satisfies every length/ellipsis assertion — + // only the full string pins the word-boundary logic itself. + expect( + compressFindingSummary( + 'the quick brown fox jumps over the lazy dog and keeps on running far beyond the fence', + ), + ).toBe('the quick brown fox jumps over the lazy dog and keeps on…'); + }); + + it('keeps a hard cut off the middle of a surrogate pair', () => { + // 58 filler units put the astral character across units 58-59, exactly + // where the hard cut lands; spaceless input keeps the word-boundary + // rescue out of the way. + const short = compressFindingSummary(`${'a'.repeat(58)}𝕏 tail words`); + expect(short).toBe(`${'a'.repeat(58)}…`); + }); +}); diff --git a/packages/core/src/tools/report-findings.ts b/packages/core/src/tools/report-findings.ts new file mode 100644 index 00000000000..25ac39fbeb5 --- /dev/null +++ b/packages/core/src/tools/report-findings.ts @@ -0,0 +1,467 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// `report_findings`: the review findings as a typed contract to the client, +// instead of a Markdown convention. +// +// A review's findings already exist as data once — the `qwen review findings` +// artifact — but that file lives on disk, registered after the fact via +// `record_artifact`. Every client rendering the session live (the terminal UI, +// the Web Shell transcript, ACP hosts) saw only the prose restatement, which +// is exactly the transcription surface the artifact exists to close. This tool +// is the in-band half of the same contract: one call, `{level, findings[]}`, +// values copied from the artifact, rendered by the host UI as a per-finding +// list. +// +// The second call is the reason the first is trustworthy: after fixes are +// applied, the reporter calls again with every finding carrying an `outcome`, +// and — like the artifact's own `--outcomes` merge — a PARTIAL outcome set is +// refused. 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 — so an +// outcome call is also held to the active report's identity: the same ids, +// none dropped, none added. + +import type { ToolInvocation, ToolResult, ReportedFinding } from './tools.js'; +import { BaseDeclarativeTool, BaseToolInvocation, Kind } from './tools.js'; +import { ToolDisplayNames, ToolNames } from './tool-names.js'; +import { hasControlCharacter } from './record-artifact.js'; + +// These enum spellings have two other deliberate copies: the findings +// artifact (`packages/cli/src/commands/review/findings.ts`, which re-exports these) and +// the Web Shell renderer (`CodeReviewArtifactDetail.tsx`, a browser bundle +// that must not import Node-side packages and fails closed on unknown +// values). A value added here must be added to the renderer copy in the same +// change. +/** The severity ladder, most severe first — this array IS the sort order. */ +export const FINDING_SEVERITIES = [ + 'Critical', + 'Suggestion', + 'Nice to have', +] as const; +export type FindingSeverity = (typeof FINDING_SEVERITIES)[number]; + +export const FINDING_CONFIDENCES = ['high', 'low'] as const; +export type FindingConfidence = (typeof FINDING_CONFIDENCES)[number]; + +export const FINDING_OUTCOMES = [ + 'fixed', + 'skipped', + 'no_change_needed', +] as const; +export type FindingOutcome = (typeof FINDING_OUTCOMES)[number]; + +export const FINDING_SOURCES = [ + 'review', + 'build', + 'test', + 'probe', + 'lint', +] as const; +export type FindingSource = (typeof FINDING_SOURCES)[number]; + +export const REPORT_FINDINGS_LEVELS = ['low', 'medium', 'high'] as const; +export type ReportFindingsLevel = (typeof REPORT_FINDINGS_LEVELS)[number]; + +export const REPORT_FINDINGS_MAX = 50; +export const SHORT_SUMMARY_MAX = 60; +// The artifact and the repository set the path domain: repo-relative paths +// run to the filesystem limit (PATH_MAX, 4096), and the artifact keeps any +// one of them — a narrower cap refused whole lists the artifact preserves, +// and truncating a path would identify a different location. +export const REPORT_FINDINGS_FILE_MAX = 4096; + +/** `shortSummary`, when the caller did not supply one within the cap. */ +export function compressFindingSummary( + summary: string, + max = SHORT_SUMMARY_MAX, +): string { + // Collapse whitespace first: a summary that wrapped across lines in the source + // prose would otherwise carry its newlines into a single-line list cell. + const flat = summary.replace(/\s+/g, ' ').trim(); + if (flat.length <= max) return flat; + // Cut on a word boundary when one is reasonably near the limit, so the label + // reads as a clause rather than a severed word. `max - 1` leaves room for + // the ellipsis, which is one character (U+2026), not three dots. The hard cut + // backs off one unit when it would land inside a surrogate pair — an + // unpaired high surrogate is not a character, and the codebase's other + // truncation paths (terminal sanitizing, display compaction) all cut on + // code-point boundaries. + let head = flat.slice(0, max - 1); + const lastUnit = head.charCodeAt(head.length - 1); + if (lastUnit >= 0xd800 && lastUnit <= 0xdbff) { + head = head.slice(0, -1); + } + const space = head.lastIndexOf(' '); + const cut = space >= max * 0.6 ? head.slice(0, space) : head; + return `${cut.trimEnd()}…`; +} + +export interface ReportFindingsFindingParams { + id?: string; + severity: FindingSeverity; + confidence?: FindingConfidence; + source?: FindingSource; + file: string; + line?: number; + summary: string; + shortSummary?: string; + failureScenario: string; + category?: string; + outcome?: FindingOutcome; + outcomeNote?: string; +} + +export interface ReportFindingsParams { + level?: ReportFindingsLevel; + findings: ReportFindingsFindingParams[]; +} + +const DESCRIPTION = `Reports code-review findings as typed data so clients (the terminal UI, the Web Shell, ACP hosts) can render a per-finding list. Use it only when an active review flow (such as the bundled review skill) instructs you to report findings with it; otherwise present findings as ordinary text. Call it once per report with the complete list, most severe first — a later call replaces the whole list, it never appends. When the review wrote a findings artifact, copy each field verbatim from it (id, severity, confidence, source, file/line, summary, shortSummary, failureScenario, category); do not re-derive or re-word values — the artifact is the oracle. + +After fixes are applied — at the review's own fix step, or ANY later time in the session a reported finding's disposition changes — call it again with the same findings, each carrying "outcome" ("fixed", "skipped", or "no_change_needed"; "outcomeNote" for the reason). Client per-finding status trusts only a call that carries outcomes, and a call where some findings carry an outcome and others do not is refused: account for every finding. + +This tool renders data for the client and nothing else: it persists nothing, decides no verdict, and a failure is a UI-delivery failure — disclose it and move on without changing the review's artifacts or verdict.`; + +const FINDING_ITEM_SCHEMA = { + type: 'object', + additionalProperties: false, + properties: { + id: { + type: 'string', + maxLength: 64, + description: + 'The findings artifact id (e.g. "R1-2"), when the review produced one.', + }, + severity: { + type: 'string', + enum: [...FINDING_SEVERITIES], + }, + confidence: { + type: 'string', + enum: [...FINDING_CONFIDENCES], + description: + 'Verification confidence. Omit on an unverified (low-effort) pass.', + }, + source: { + type: 'string', + enum: [...FINDING_SOURCES], + description: 'Where the finding came from. Defaults to "review".', + }, + file: { + type: 'string', + maxLength: REPORT_FINDINGS_FILE_MAX, + description: + 'Repo-relative path, or the review\'s "(body)" stand-in for an unanchored finding.', + }, + line: { + type: 'integer', + minimum: 1, + }, + summary: { + type: 'string', + maxLength: 2000, + description: 'One sentence stating the defect.', + }, + shortSummary: { + type: 'string', + description: `Compressed label for a compact list UI (<= ${SHORT_SUMMARY_MAX} characters; longer values are compressed, and it is derived from "summary" when absent).`, + }, + failureScenario: { + type: 'string', + maxLength: 4000, + description: 'The concrete trigger and wrong outcome.', + }, + category: { + type: 'string', + maxLength: 64, + description: + 'Free-form kebab-case tag ("correctness", "security", "test-coverage", …).', + }, + outcome: { + type: 'string', + enum: [...FINDING_OUTCOMES], + description: + 'Set ONLY on a re-report after fixes were applied: what happened to this finding. All findings in the call must carry one, or none.', + }, + outcomeNote: { + type: 'string', + maxLength: 1000, + description: 'The fixer\'s reason — required reading for "skipped".', + }, + }, + required: ['severity', 'file', 'summary', 'failureScenario'], +} as const; + +class ReportFindingsInvocation extends BaseToolInvocation< + ReportFindingsParams, + ToolResult +> { + constructor( + params: ReportFindingsParams, + // Identity is committed on successful delivery, not at build: the + // scheduler builds every invocation of a batch up front and can still + // discard it before execution (pre-validation cancellation, an aborted + // signal), and a built-but-never-executed report must not replace the + // identity the client actually received. + private readonly commitActiveReport: ( + identity: ReadonlySet | undefined, + ) => void, + ) { + super(params); + } + + override getDescription(): string { + const n = this.params.findings.length; + return `Report ${n} finding${n === 1 ? '' : 's'}`; + } + + async execute(_signal: AbortSignal): Promise { + const findings = sortReportedFindings( + this.params.findings.map(normalizeFinding), + ); + const display = { + type: 'findings_list' as const, + ...(this.params.level ? { level: this.params.level } : {}), + findings, + }; + + const bySeverity = FINDING_SEVERITIES.map((severity) => { + const count = findings.filter((f) => f.severity === severity).length; + return count > 0 ? `${count} ${severity}` : undefined; + }).filter((part): part is string => part !== undefined); + const withOutcomes = + findings.length > 0 && findings[0].outcome + ? ` with outcomes (${FINDING_OUTCOMES.map((outcome) => { + const count = findings.filter((f) => f.outcome === outcome).length; + return count > 0 ? `${count} ${outcome}` : undefined; + }) + .filter(Boolean) + .join(', ')})` + : ''; + const summaryLine = + findings.length === 0 + ? 'Reported an empty findings list to the client UI.' + : `Reported ${findings.length} finding${findings.length === 1 ? '' : 's'} to the client UI (${bySeverity.join(', ')})${withOutcomes}.`; + + this.commitActiveReport(reportIdentity(this.params.findings)); + + return { + llmContent: `${summaryLine} Nothing was persisted; the review's findings artifact remains the canonical record.`, + returnDisplay: display, + }; + } +} + +function normalizeFinding(raw: ReportFindingsFindingParams): ReportedFinding { + const shortSource = raw.shortSummary?.trim() || raw.summary; + return { + ...(raw.id?.trim() ? { id: raw.id.trim() } : {}), + severity: raw.severity, + ...(raw.confidence ? { confidence: raw.confidence } : {}), + ...(raw.source ? { source: raw.source } : {}), + file: raw.file.trim(), + ...(raw.line !== undefined ? { line: raw.line } : {}), + summary: raw.summary.trim(), + shortSummary: compressFindingSummary(shortSource), + failureScenario: raw.failureScenario.trim(), + ...(raw.category?.trim() ? { category: raw.category.trim() } : {}), + ...(raw.outcome ? { outcome: raw.outcome } : {}), + ...(raw.outcomeNote?.trim() ? { outcomeNote: raw.outcomeNote.trim() } : {}), + }; +} + +/** + * Severity, then confidence, then location — matching the artifact's own + * `sortFindings` (code-unit file/id comparison, a missing line ranked first), + * so the list a client renders and the artifact a reader opens agree about + * order. The one extension: the artifact requires `confidence`, this contract + * does not (an unverified low-effort pass omits it), and an absent confidence + * ranks between `high` and `low`. + */ +function sortReportedFindings( + findings: readonly ReportedFinding[], +): ReportedFinding[] { + const confidenceRank = (c: ReportedFinding['confidence']): number => + c === 'high' ? 0 : c === undefined ? 1 : 2; + return [...findings].sort((a, b) => { + const severity = + FINDING_SEVERITIES.indexOf(a.severity) - + FINDING_SEVERITIES.indexOf(b.severity); + if (severity !== 0) return severity; + const confidence = + confidenceRank(a.confidence) - confidenceRank(b.confidence); + if (confidence !== 0) return confidence; + if (a.file !== b.file) return a.file < b.file ? -1 : 1; + const line = (a.line ?? 0) - (b.line ?? 0); + if (line !== 0) return line; + const aId = a.id ?? ''; + const bId = b.id ?? ''; + return aId < bId ? -1 : aId > bId ? 1 : 0; + }); +} + +export class ReportFindingsTool extends BaseDeclarativeTool< + ReportFindingsParams, + ToolResult +> { + static readonly Name: string = ToolNames.REPORT_FINDINGS; + + // Id set of the most recent DELIVERED report whose findings all carried + // ids, committed in the invocation's execute(). An outcome call replaces + // the whole list, so it must match this identity in full; undefined when + // there is no identity to join on (no report yet, or a low-effort report + // without artifact ids). + // + // The gate is a contract about the live process, not a persisted one: the + // tool instance is cached by the registry for the session, but a cold + // session resume (--resume, a daemon or process restart) constructs a + // fresh instance with no active identity. That is the documented limit of + // the join — after a restart an outcome call is validated on its own + // terms (all-or-nothing outcomes) instead of against the pre-restart + // report — and the transcript surfaces render the same replacement (the + // last delivered list wins) independently of this gate. + private activeReportIds: ReadonlySet | undefined; + + constructor() { + super( + ReportFindingsTool.Name, + ToolDisplayNames.REPORT_FINDINGS, + DESCRIPTION, + Kind.Think, + { + type: 'object', + additionalProperties: false, + properties: { + level: { + type: 'string', + enum: [...REPORT_FINDINGS_LEVELS], + description: 'The review effort the findings came from.', + }, + findings: { + type: 'array', + maxItems: REPORT_FINDINGS_MAX, + items: FINDING_ITEM_SCHEMA, + description: + 'The complete findings list, most severe first. An empty array is a valid "nothing found" report.', + }, + }, + required: ['findings'], + }, + true, + false, + true, + false, + 'review findings report code-review severity outcome fixed', + ); + } + + protected override validateToolParamValues( + params: ReportFindingsParams, + ): string | null { + const seenIds = new Set(); + let withOutcome = 0; + for (const [index, finding] of params.findings.entries()) { + for (const [field, value] of Object.entries({ + id: finding.id, + file: finding.file, + summary: finding.summary, + shortSummary: finding.shortSummary, + failureScenario: finding.failureScenario, + category: finding.category, + outcomeNote: finding.outcomeNote, + })) { + if (value === undefined) continue; + if ( + hasControlCharacter( + value, + field === 'summary' || + field === 'failureScenario' || + field === 'outcomeNote', + ) + ) { + return `Finding at index ${index}: "${field}" contains control characters`; + } + } + if (!finding.file.trim()) { + return `Finding at index ${index}: "file" must not be empty`; + } + if (!finding.summary.trim()) { + return `Finding at index ${index}: "summary" must not be empty`; + } + if (!finding.failureScenario.trim()) { + return `Finding at index ${index}: "failureScenario" must not be empty`; + } + if (finding.line !== undefined && !Number.isSafeInteger(finding.line)) { + return `Finding at index ${index}: "line" must be an integer within JavaScript's safe range`; + } + if (finding.outcome === 'skipped' && !finding.outcomeNote?.trim()) { + return `Finding at index ${index}: "outcomeNote" is required when "outcome" is "skipped" — the reader is owed the reason for work not done`; + } + const id = finding.id?.trim(); + if (id) { + if (seenIds.has(id)) { + return `Finding at index ${index}: duplicate id "${id}"`; + } + seenIds.add(id); + } + if (finding.outcome) withOutcome++; + } + if (withOutcome > 0 && withOutcome < params.findings.length) { + return `${withOutcome} of ${params.findings.length} findings carry an "outcome". Outcomes account for every finding or none: a partial set silently shortens the list. Add the missing outcomes (or remove them all) and call again.`; + } + const activeIds = this.activeReportIds; + if ( + withOutcome === params.findings.length && + params.findings.length > 0 && + activeIds !== undefined + ) { + const newIds = new Set( + params.findings.map((finding) => finding.id?.trim() ?? ''), + ); + const missing = [...activeIds].filter((id) => !newIds.has(id)); + if (missing.length > 0) { + return `Outcome report drops ${missing.length} finding(s) from the active report: ${missing + .map((id) => JSON.stringify(id)) + .join( + ', ', + )}. An outcome call replaces the whole list — re-report every active finding with its outcome.`; + } + const unknown = [...newIds].filter( + (id) => id === '' || !activeIds.has(id), + ); + if (unknown.length > 0) { + return `Outcome report carries finding(s) the active report does not have: ${unknown + .map((id) => (id === '' ? '(missing id)' : JSON.stringify(id))) + .join(', ')}. Outcomes join back to the active report by id.`; + } + } + return null; + } + + protected createInvocation( + params: ReportFindingsParams, + ): ToolInvocation { + return new ReportFindingsInvocation(params, (identity) => { + this.activeReportIds = identity; + }); + } +} + +/** + * The identity an outcome replacement is held to: the id set of a report + * whose findings ALL carry ids. A report with any id-less finding (a + * low-effort pass has no artifact ids) has no identity to join on, so a + * later outcome call is accepted on its own terms. + */ +function reportIdentity( + findings: readonly ReportFindingsFindingParams[], +): ReadonlySet | undefined { + const ids = findings.map((finding) => finding.id?.trim() ?? ''); + if (findings.length === 0 || ids.some((id) => id === '')) { + return undefined; + } + return new Set(ids); +} diff --git a/packages/core/src/tools/tool-names.ts b/packages/core/src/tools/tool-names.ts index d52c3cc8428..a53f9902058 100644 --- a/packages/core/src/tools/tool-names.ts +++ b/packages/core/src/tools/tool-names.ts @@ -62,6 +62,7 @@ export const ToolNames = { WORKFLOW: 'workflow', ARTIFACT: 'artifact', RECORD_ARTIFACT: 'record_artifact', + REPORT_FINDINGS: 'report_findings', GET_GOAL: 'get_goal', UPDATE_GOAL: 'update_goal', DISPLAY_IMAGE: 'display_image', @@ -117,6 +118,7 @@ export const ToolDisplayNames = { WORKFLOW: 'Workflow', ARTIFACT: 'Artifact', RECORD_ARTIFACT: 'RecordArtifact', + REPORT_FINDINGS: 'ReportFindings', GET_GOAL: 'Goal', UPDATE_GOAL: 'UpdateGoal', DISPLAY_IMAGE: 'DisplayImage', diff --git a/packages/core/src/tools/tools.ts b/packages/core/src/tools/tools.ts index 6264a191a0b..7ef06653c8b 100644 --- a/packages/core/src/tools/tools.ts +++ b/packages/core/src/tools/tools.ts @@ -784,6 +784,7 @@ export type ToolResultDisplay = | AgentResultDisplay | TeamResultDisplay | TaskListResultDisplay + | FindingsResultDisplay | AnsiOutputDisplay | McpToolProgressData | McpAppResultDisplay @@ -834,6 +835,52 @@ export interface DiffStat { user_removed_chars: number; } +/** + * One review finding as the `report_findings` tool hands it to clients. + * + * Field names and enum spellings deliberately match the `qwen review + * findings` artifact (`packages/cli/src/commands/review/findings.ts`) so the model + * copies values straight out of the artifact instead of translating them — + * a translation layer between two spellings of the same list is where + * severities have historically drifted. + */ +export interface ReportedFinding { + /** The findings artifact's id (`R-` / `D-`), when one exists. */ + id?: string; + severity: 'Critical' | 'Suggestion' | 'Nice to have'; + /** Verification confidence. Absent on an unverified (low-effort) pass. */ + confidence?: 'high' | 'low'; + /** Where the finding came from. */ + source?: 'review' | 'build' | 'test' | 'probe' | 'lint'; + file: string; + line?: number; + /** One sentence stating the defect. */ + summary: string; + /** `summary` compressed to <= 60 characters, for a compact list UI. */ + shortSummary: string; + /** The concrete trigger and wrong outcome. */ + failureScenario: string; + /** Free-form kebab-case tag (`correctness`, `security`, …). */ + category?: string; + /** Set only on a re-report after fixes were applied. */ + outcome?: 'fixed' | 'skipped' | 'no_change_needed'; + /** The fixer's reason — mainly for `skipped`. */ + outcomeNote?: string; +} + +export interface FindingsResultDisplay { + type: 'findings_list'; + /** The review effort the findings came from. */ + level?: 'low' | 'medium' | 'high'; + findings: ReportedFinding[]; + /** + * Set by history/recording compaction when the retained-display budget + * evicted the least severe tail of a larger list: how many findings were + * removed. The retained prefix keeps the most severe entries. + */ + omittedFindings?: number; +} + export interface TodoResultDisplay { type: 'todo_list'; planId?: string; diff --git a/packages/core/src/utils/toolResultDisplayCompaction.test.ts b/packages/core/src/utils/toolResultDisplayCompaction.test.ts index a497dd812a8..295a70f16b9 100644 --- a/packages/core/src/utils/toolResultDisplayCompaction.test.ts +++ b/packages/core/src/utils/toolResultDisplayCompaction.test.ts @@ -9,6 +9,7 @@ import type { AgentResultDisplay, AnsiOutputDisplay, FileDiff, + FindingsResultDisplay, McpAppResultDisplay, McpToolProgressData, PlanResultDisplay, @@ -356,6 +357,88 @@ describe('toolResultDisplayCompaction', () => { expect(compactedProgress.message).toContain('truncated from'); }); + it('compacts findings displays without touching their typed fields', () => { + const display: FindingsResultDisplay = { + type: 'findings_list', + level: 'high', + findings: [ + { + id: 'R1-1', + severity: 'Critical', + confidence: 'high', + file: 'src/foo.ts', + line: 42, + summary: `summary-${'x'.repeat(MAX_RETAINED_AGENT_FIELD_CHARS)}-done`, + shortSummary: 'short', + failureScenario: `scenario-${'x'.repeat(MAX_RETAINED_AGENT_FIELD_CHARS)}-done`, + outcome: 'skipped', + outcomeNote: `note-${'x'.repeat(MAX_RETAINED_AGENT_FIELD_CHARS)}-done`, + }, + ], + }; + + const compacted = compactToolResultDisplayForHistory(display); + + expect(compacted.findings[0].summary).toContain('truncated from'); + expect(compacted.findings[0].failureScenario).toContain('truncated from'); + expect(compacted.findings[0].outcomeNote).toContain('truncated from'); + expect(compacted.findings[0].severity).toBe('Critical'); + expect(compacted.findings[0].outcome).toBe('skipped'); + expect(compacted.findings[0].shortSummary).toBe('short'); + expect(compacted.level).toBe('high'); + expect(compacted.omittedFindings).toBeUndefined(); + }); + + it('applies an aggregate budget across the list, keeping the most severe prefix', () => { + // The schema-maximal shape: 50 findings at the field maxima (summary + // 2000 / failureScenario 4000 / outcomeNote 1000). Per-field caps alone + // retained ~358 KB through compaction, bypassing the retained-display + // budget every other display type obeys. Severities are staggered so + // the sort order (most severe first) is observable in the retained + // prefix. + const severities = ['Critical', 'Suggestion', 'Nice to have'] as const; + const findings = Array.from({ length: 50 }, (_, i) => ({ + id: `R1-${i + 1}`, + severity: severities[i % 3], + confidence: 'high' as const, + file: `src/f${i}.ts`, + summary: `s${i}-${'x'.repeat(1996)}`, + shortSummary: 'short', + failureScenario: `f${i}-${'y'.repeat(3996)}`, + outcome: 'skipped' as const, + outcomeNote: `n${i}-${'z'.repeat(996)}`, + })); + const display: FindingsResultDisplay = { + type: 'findings_list', + level: 'high', + findings, + }; + + const compacted = compactToolResultDisplayForHistory(display); + + // The retained prefix starts at the most severe entry and stays within + // the general retained-display budget; the evicted tail is counted. + expect(compacted.findings[0].id).toBe('R1-1'); + expect(compacted.findings.length).toBeLessThan(50); + expect(compacted.findings.length).toBeGreaterThan(0); + expect(compacted.omittedFindings).toBe(50 - compacted.findings.length); + const retainedChars = compacted.findings.reduce( + (total, f) => + total + + f.summary.length + + f.failureScenario.length + + (f.outcomeNote?.length ?? 0), + 0, + ); + expect(retainedChars).toBeLessThanOrEqual( + MAX_RETAINED_TOOL_RESULT_DISPLAY_CHARS, + ); + // The retained prefix keeps the list's own order, unsorted. + expect(compacted.findings.map((f) => f.id)).toEqual( + findings.slice(0, compacted.findings.length).map((f) => f.id), + ); + }); + it('compacts task list and team result displays', () => { const taskDisplay: TaskListResultDisplay = { type: 'task_list', diff --git a/packages/core/src/utils/toolResultDisplayCompaction.ts b/packages/core/src/utils/toolResultDisplayCompaction.ts index c6778f87e69..04861bf2ebc 100644 --- a/packages/core/src/utils/toolResultDisplayCompaction.ts +++ b/packages/core/src/utils/toolResultDisplayCompaction.ts @@ -8,9 +8,11 @@ import type { AgentResultDisplay, AnsiOutputDisplay, FileDiff, + FindingsResultDisplay, McpAppResultDisplay, McpToolProgressData, PlanResultDisplay, + ReportedFinding, TaskListResultDisplay, TeamResultDisplay, TodoResultDisplay, @@ -382,6 +384,93 @@ function compactTodoResultDisplay( }; } +function isFindingsResultDisplay( + resultDisplay: unknown, +): resultDisplay is FindingsResultDisplay { + return ( + typeof resultDisplay === 'object' && + resultDisplay !== null && + 'type' in resultDisplay && + resultDisplay.type === 'findings_list' + ); +} + +// Deterministic size estimate of one finding as retained: every string field +// at its post-compaction length. The enum and boolean fields are bounded +// constants, so a fixed per-entry allowance covers them and the JSON shape. +function findingRetainedSize(finding: ReportedFinding): number { + return ( + (finding.id?.length ?? 0) + + finding.severity.length + + (finding.confidence?.length ?? 0) + + (finding.source?.length ?? 0) + + finding.file.length + + String(finding.line ?? '').length + + finding.summary.length + + finding.shortSummary.length + + finding.failureScenario.length + + (finding.category?.length ?? 0) + + (finding.outcome?.length ?? 0) + + (finding.outcomeNote?.length ?? 0) + + 20 + ); +} + +function compactFindingsResultDisplay( + display: FindingsResultDisplay, + purpose: CompactionPurpose, +): FindingsResultDisplay { + const compacted: FindingsResultDisplay = { + ...display, + findings: display.findings.map((finding) => ({ + ...finding, + summary: compactString( + finding.summary, + purpose, + MAX_RETAINED_AGENT_FIELD_CHARS, + ), + failureScenario: compactString( + finding.failureScenario, + purpose, + MAX_RETAINED_AGENT_FIELD_CHARS, + ), + ...(finding.outcomeNote !== undefined && { + outcomeNote: compactString( + finding.outcomeNote, + purpose, + MAX_RETAINED_AGENT_FIELD_CHARS, + ), + }), + })), + }; + + // Per-field caps alone leave a schema-maximal list (50 findings x the + // field maxima) at several hundred retained KB, bypassing the budget every + // other display type obeys. The list arrives sorted most-severe-first and + // compaction never reorders, so a prefix keeps the most severe entries; + // the evicted tail is counted, never dropped silently. (A single finding + // cannot outgrow the budget: the schema's field maxima bound one to a + // third of it.) + let total = 0; + let kept = 0; + for (const finding of compacted.findings) { + const size = findingRetainedSize(finding); + if (total + size > MAX_RETAINED_TOOL_RESULT_DISPLAY_CHARS) { + break; + } + total += size; + kept += 1; + } + if (kept === compacted.findings.length) { + return compacted; + } + return { + ...compacted, + findings: compacted.findings.slice(0, kept), + omittedFindings: compacted.findings.length - kept, + }; +} + function isPlanResultDisplay( resultDisplay: unknown, ): resultDisplay is PlanResultDisplay { @@ -554,6 +643,10 @@ function compactToolResultDisplay( return compactTodoResultDisplay(resultDisplay, purpose) as T; } + if (isFindingsResultDisplay(resultDisplay)) { + return compactFindingsResultDisplay(resultDisplay, purpose) as T; + } + if (isPlanResultDisplay(resultDisplay)) { return compactPlanResultDisplay(resultDisplay, purpose) as T; } diff --git a/packages/web-shell/client/components/messages/toolFormatting.ts b/packages/web-shell/client/components/messages/toolFormatting.ts index 15038647464..da341c255ee 100644 --- a/packages/web-shell/client/components/messages/toolFormatting.ts +++ b/packages/web-shell/client/components/messages/toolFormatting.ts @@ -59,6 +59,7 @@ export const TOOL_DISPLAY_NAMES: Record = { workflow: 'Workflow', artifact: 'Artifact', record_artifact: 'RecordArtifact', + report_findings: 'ReportFindings', web_search: 'WebSearch', image_gen: 'ImageGen', display_image: 'DisplayImage', diff --git a/packages/web-shell/client/i18n.tsx b/packages/web-shell/client/i18n.tsx index 5b156ad8b15..4de304b802e 100644 --- a/packages/web-shell/client/i18n.tsx +++ b/packages/web-shell/client/i18n.tsx @@ -3212,6 +3212,7 @@ const ZH: Messages = { 'toolName.read_mcp_resource': '读取 MCP 资源', 'toolName.artifact': '制品', 'toolName.record_artifact': '记录制品', + 'toolName.report_findings': '上报评审发现', 'toolName.image_gen': '生成图片', 'toolName.display_image': '显示图片', // web-shell-only wire aliases (see TOOL_DISPLAY_NAMES in toolFormatting.ts)