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

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.

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', () => {

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

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

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/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