feat(review): report findings to clients as a typed contract

docs/design/report-findings-typed-contract.md

@@ -0,0 +1,53 @@
+# 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
+only the free-text fields.
+
+## 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.

packages/cli/src/commands/review/findings.test.ts

@@ -425,6 +425,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', () => {

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}`);
@@ -765,6 +764,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,

packages/cli/src/i18n/locales/ca.js

@@ -2402,6 +2402,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ó',

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',

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': '進入計畫模式',

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': '进入计划模式',

packages/cli/src/ui/components/FindingsDisplay.test.tsx

@@ -0,0 +1,160 @@
+/**
+ * @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> = {},
+): 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('<FindingsDisplay />', () => {
+  it('renders one row per finding with severity, id, location and label', () => {
+    const { lastFrame } = render(<FindingsDisplay data={display()} />);
+    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(<FindingsDisplay data={data} />);
+    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(
+      <FindingsDisplay data={display({ findings: [] })} />,
+    );
+    expect(lastFrame()).toContain('No findings.');
+  });
+
+  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(<FindingsDisplay data={data} />);
+    expect(lastFrame()!).toContain('unverified');
+  });
+
+  it('does not mark verified reports unverified', () => {
+    const { lastFrame } = render(<FindingsDisplay data={display()} />);
+    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(<FindingsDisplay data={data} />);
+      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(<FindingsDisplay data={data} />);
+    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');
+  });
+});