diff --git a/docs/design/web-shell-review-result.md b/docs/design/web-shell-review-result.md new file mode 100644 index 00000000000..51165778ef5 --- /dev/null +++ b/docs/design/web-shell-review-result.md @@ -0,0 +1,103 @@ +# Web Shell Review Result + +## Problem + +`/review` produces canonical structured findings and a composed verdict, but Web Shell only receives the final conversational prose. The existing right-panel “Review” view displays files changed by a turn; it is not a code-review result. This makes findings difficult to scan, filter, and revisit, and creates an ambiguous UI label. + +## Current State + +- `qwen review findings` writes the canonical findings report under `.qwen/tmp/`. +- `qwen review compose-review` writes the authoritative composed verdict under `.qwen/tmp/`. +- Step 8 saves a durable Markdown report under `.qwen/reviews/`. +- Step 9 removes the temporary findings and composed verdict files. +- Session artifacts already support workspace files with primitive metadata. +- Web Shell already supports artifact-specific rendering selected by metadata. + +## Proposed Design + +### Durable review-result document + +Add `qwen review save-artifact`, which reads: + +- the canonical findings report; +- the composed verdict; +- the saved Markdown report path; +- the target and effort supplied by the review skill. + +It writes a versioned JSON document under `.qwen/reviews/` containing: + +- schema version; +- target and effort; +- authoritative composed verdict fields; +- canonical findings and counts; +- the workspace-relative Markdown report path. + +The command copies structured inputs without recalculating the verdict or rewriting findings. It fails closed on unreadable or malformed inputs and refuses output inside `.qwen/tmp/`. + +### Session artifact registration + +After Step 8 creates the JSON document and before Step 9 cleanup, the `/review` skill calls `record_artifact` with: + +- `kind: other`; +- `storage: workspace`; +- the JSON document path; +- `mimeType: application/vnd.qwen.code-review+json`; +- metadata `artifactType: code_review` and `schemaVersion: 1`. + +The skill owns registration because it runs in the active session and turn. The CLI command owns document construction so the model does not transcribe findings or verdict fields. + +### Web Shell renderer + +Web Shell detects `metadata.artifactType === 'code_review'` and renders a dedicated review-result view. It reads and validates the JSON document at runtime and displays: + +- verdict and caps; +- severity and confidence counts; +- filterable findings; +- finding source, failure scenario, suggested fix, outcomes, and locations; +- published evidence images as safe links and local evidence files (`assetFiles`) as inline images; +- a link to the durable Markdown report, offered from the error view too when it can be derived from the artifact path. + +The renderer never parses transcript prose and never computes a verdict. Malformed documents display an artifact error rather than falling back to a misleading source-code preview, and the error view offers the durable Markdown report derived from the artifact path so a truncated or unparsable document is not a dead end. + +### Existing file-change view + +Change only the user-visible label from “Review / 审核” to “Changes / 文件更改”. Internal `review` discriminants and public customization values remain unchanged to avoid an unrelated API migration. + +## Files Affected + +- `packages/cli/src/commands/review/save-artifact.ts` +- `packages/cli/src/commands/review/save-artifact.test.ts` +- `packages/cli/src/commands/review.ts` +- `packages/core/src/skills/bundled/review/SKILL.md` +- `packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx` +- `packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.test.tsx` +- `packages/web-shell/client/components/artifacts/ArtifactPanel.tsx` +- `packages/web-shell/client/components/artifacts/ArtifactPanel.module.css` +- `packages/web-shell/client/i18n.tsx` + +## Scope Boundaries + +Included: + +- deterministic durable review-result JSON; +- current-session artifact registration; +- dedicated Web Shell renderer; +- visible naming disambiguation. + +Excluded: + +- Web Shell review orchestration; +- a review launcher dialog; +- phase/progress events; +- verdict recomputation in the browser; +- one-click fixing; +- daemon protocol changes; +- internal rename of the existing file-change `review` types. + +## Decisions + +- The canonical findings report remains the only finding authority. +- The composed verdict remains the only verdict authority. +- The durable JSON is created before cleanup and stored outside `.qwen/tmp/`. +- `metadata.artifactType` is sufficient for renderer dispatch; no new artifact kind is introduced. +- Review-result schema changes require a version bump and backward-compatible renderer handling. diff --git a/docs/users/features/code-review.md b/docs/users/features/code-review.md index f7c746ba480..50c33bf412f 100644 --- a/docs/users/features/code-review.md +++ b/docs/users/features/code-review.md @@ -324,6 +324,8 @@ For same-repo reviews, results are saved as a Markdown file in your project's `. Reports include: timestamp, diff stats, build/test results, all findings with verification status, and the verdict. Section headings and descriptive prose follow the output language preference; technical identifiers (SHAs, file paths, gate names, finding ids) stay verbatim. +Medium- and high-effort reviews also save a structured JSON companion with the same stem (for example, `2026-04-06-143022-pr-123.json`) holding the canonical findings and the composed verdict as data. Qwen Code's Web Shell renders that document as an interactive review view with filterable findings; the Markdown report stays the human-readable archive. + The deterministic halves of the pipeline — argument parsing (`qwen review parse-args`) and the event/body decision (`qwen review compose-review`) — are tested subcommands rather than prompt text, so `--effort` grammar, `--comment` forcing, verdict caps, and downgrade behavior are pinned by unit tests and cannot drift with the model. **GitHub Enterprise:** reviewing a PR URL on a non-`github.com` host routes every GitHub call at that host — the review subcommands (`fetch-pr`, `pr-context`, `comment-status`, `presubmit`) accept `--host` and set it in code, so a forgotten host cannot silently retarget the review at `github.com`. diff --git a/packages/cli/src/commands/review.test.ts b/packages/cli/src/commands/review.test.ts index 2cb804284e0..8b560f50477 100644 --- a/packages/cli/src/commands/review.test.ts +++ b/packages/cli/src/commands/review.test.ts @@ -63,6 +63,7 @@ describe('reviewCommand', () => { 'findings', 'publish-assets', 'compose-review', + 'save-artifact', 'submit', 'cleanup', ]); diff --git a/packages/cli/src/commands/review.ts b/packages/cli/src/commands/review.ts index 68749a526b9..cb70d08d020 100644 --- a/packages/cli/src/commands/review.ts +++ b/packages/cli/src/commands/review.ts @@ -35,6 +35,7 @@ import { testEfficacyCommand } from './review/test-efficacy.js'; import { testPlanCommand } from './review/test-plan.js'; import { cleanupCommand } from './review/cleanup.js'; import { runCommand } from './review/run.js'; +import { saveArtifactCommand } from './review/save-artifact.js'; export const reviewCommand: CommandModule = { command: 'review', @@ -66,11 +67,12 @@ export const reviewCommand: CommandModule = { .command(findingsCommand) .command(publishAssetsCommand) .command(composeReviewCommand) + .command(saveArtifactCommand) .command(submitCommand) .command(cleanupCommand) .demandCommand( 1, - 'Specify a subcommand: run, parse-args, fetch-pr, capture-local, plan-diff, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, presubmit, test-efficacy, test-plan, findings, publish-assets, compose-review, submit, or cleanup.', + 'Specify a subcommand: run, parse-args, fetch-pr, capture-local, plan-diff, pr-context, comment-status, load-rules, agent-prompt, build-test, base-tree, test-delta, drive, mock-provider, extract-step, script-lint, resolve-anchors, check-coverage, presubmit, test-efficacy, test-plan, findings, publish-assets, compose-review, save-artifact, submit, or cleanup.', ) .version(false), handler: () => { diff --git a/packages/cli/src/commands/review/findings.test.ts b/packages/cli/src/commands/review/findings.test.ts index e193cd9742c..2dcbb6ca502 100644 --- a/packages/cli/src/commands/review/findings.test.ts +++ b/packages/cli/src/commands/review/findings.test.ts @@ -12,9 +12,13 @@ import { applyOutcomes, buildReport, compressSummary, + CONFIDENCES, findingsCommand, + OUTCOMES, renderFindings, + SEVERITIES, sortFindings, + SOURCES, validateFindings, validateOutcomes, type Finding, @@ -34,6 +38,21 @@ const base = { line: 42, }; +describe('review vocabulary contract', () => { + // The Web Shell renderer + // (packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx) + // keeps its own copies of these four lists and fails closed on any value it + // does not know. This snapshot makes a CLI-side addition turn red HERE — + // next to the pointer to the renderer copy — instead of surfacing later as + // saved artifacts that silently stop rendering. + it('matches the copies the Web Shell renderer duplicates', () => { + expect([...SEVERITIES]).toEqual(['Critical', 'Suggestion', 'Nice to have']); + expect([...CONFIDENCES]).toEqual(['high', 'low']); + expect([...SOURCES]).toEqual(['review', 'build', 'test', 'probe', 'lint']); + expect([...OUTCOMES]).toEqual(['fixed', 'skipped', 'no_change_needed']); + }); +}); + describe('validateFindings', () => { it('accepts the minimal shape and defaults confidence and source', () => { const [f] = validateFindings([base]); @@ -131,10 +150,25 @@ describe('validateFindings', () => { ).toThrow(/non-empty array/); }); - it('rejects a non-numeric line', () => { - expect(() => validateFindings([{ ...base, line: '42' }])).toThrow( - /non-numeric "line"/, - ); + it.each(['42', -1, 0, 1.5, Number.NaN, Number.POSITIVE_INFINITY])( + 'rejects an invalid line value %s', + (line) => { + expect(() => validateFindings([{ ...base, line }])).toThrow( + /invalid "line"/, + ); + }, + ); + + it('rejects invalid aggregate location lines', () => { + expect(() => + validateFindings([ + { + ...base, + file: undefined, + locations: [{ file: 'a.ts', line: 0 }], + }, + ]), + ).toThrow(/location 0 has an invalid "line"/); }); it('rejects a top-level input that is not an array', () => { diff --git a/packages/cli/src/commands/review/findings.ts b/packages/cli/src/commands/review/findings.ts index 2cce4eff911..068b4d846d5 100644 --- a/packages/cli/src/commands/review/findings.ts +++ b/packages/cli/src/commands/review/findings.ts @@ -36,6 +36,11 @@ import { existsSync, readFileSync, writeFileSync, mkdirSync } from 'node:fs'; import { dirname, resolve } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; +// These four lists have a second 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. /** The severity ladder, most severe first — this array IS the sort order. */ export const SEVERITIES = ['Critical', 'Suggestion', 'Nice to have'] as const; export type Severity = (typeof SEVERITIES)[number]; @@ -203,8 +208,13 @@ function parseLocations( const lo = l as Record; const file = asString(lo, 'file'); if (!file) fail(index, `location ${j} is missing a non-empty "file".`); - if (lo['line'] !== undefined && typeof lo['line'] !== 'number') { - fail(index, `location ${j} has a non-numeric "line".`); + if ( + lo['line'] !== undefined && + (typeof lo['line'] !== 'number' || + !Number.isSafeInteger(lo['line']) || + lo['line'] <= 0) + ) { + fail(index, `location ${j} has an invalid "line".`); } return { file, @@ -221,8 +231,13 @@ function parseLocations( 'needs a non-empty "file" (or a "locations" array for a pattern aggregate).', ); } - if (o['line'] !== undefined && typeof o['line'] !== 'number') { - fail(index, 'has a non-numeric "line".'); + if ( + o['line'] !== undefined && + (typeof o['line'] !== 'number' || + !Number.isSafeInteger(o['line']) || + o['line'] <= 0) + ) { + fail(index, 'has an invalid "line".'); } return [ { diff --git a/packages/cli/src/commands/review/resolve-anchors.test.ts b/packages/cli/src/commands/review/resolve-anchors.test.ts index fad8d845cdf..dee3a1d9ad4 100644 --- a/packages/cli/src/commands/review/resolve-anchors.test.ts +++ b/packages/cli/src/commands/review/resolve-anchors.test.ts @@ -38,11 +38,17 @@ describe('validateRequests', () => { expect(() => validateRequests([{ ...ok, anchor: '' }])).toThrow(/"anchor"/); }); - it('rejects a non-numeric claimed line', () => { - expect(() => validateRequests([{ ...ok, line: '42' }])).toThrow( - /non-numeric "line"/, - ); - }); + it.each(['42', -1, 0, 1.5, Number.NaN, Number.POSITIVE_INFINITY])( + 'rejects an invalid claimed line %s', + (line) => { + // The same rule the `findings` validator applies: the two validators in + // one pipeline must agree, so an entry that passes here cannot hard-fail + // there. + expect(() => validateRequests([{ ...ok, line }])).toThrow( + /invalid "line"/, + ); + }, + ); it('names the offending index for a null element instead of crashing', () => { // `typeof null === 'object'`, so indexing it threw a bare TypeError that said diff --git a/packages/cli/src/commands/review/resolve-anchors.ts b/packages/cli/src/commands/review/resolve-anchors.ts index 27b332bdde8..8a10bf5bb74 100644 --- a/packages/cli/src/commands/review/resolve-anchors.ts +++ b/packages/cli/src/commands/review/resolve-anchors.ts @@ -55,8 +55,15 @@ export function validateRequests(raw: unknown): AnchorRequest[] { ); } } - if (o['line'] !== undefined && typeof o['line'] !== 'number') { - throw new Error(`Finding "${o['id']}" has a non-numeric "line".`); + // The same rule `findings`'s parseLocations applies: two validators in one + // pipeline must not disagree, or an entry passes here and hard-fails there. + if ( + o['line'] !== undefined && + (typeof o['line'] !== 'number' || + !Number.isSafeInteger(o['line']) || + o['line'] <= 0) + ) { + throw new Error(`Finding "${o['id']}" has an invalid "line".`); } return { id: o['id'] as string, diff --git a/packages/cli/src/commands/review/save-artifact.test.ts b/packages/cli/src/commands/review/save-artifact.test.ts new file mode 100644 index 00000000000..ed51f410cc5 --- /dev/null +++ b/packages/cli/src/commands/review/save-artifact.test.ts @@ -0,0 +1,413 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { + existsSync, + mkdtempSync, + mkdirSync, + readFileSync, + readdirSync, + rmSync, + symlinkSync, + writeFileSync, +} from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { buildReport, type Finding } from './findings.js'; +import { saveReviewArtifact } from './save-artifact.js'; + +// On a case-sensitive filesystem the alias below never exists, so that test +// can only run where the filesystem folds case. Probe once, at load time, so +// the skip is visible in CI output instead of reading as a pass. +const caseInsensitiveFs = (() => { + const dir = mkdtempSync(join(tmpdir(), 'review-artifact-case-probe-')); + try { + writeFileSync(join(dir, 'case-probe'), ''); + return existsSync(join(dir, 'CASE-PROBE')); + } finally { + rmSync(dir, { recursive: true, force: true }); + } +})(); + +let root: string; +let previousProjectDir: string | undefined; + +const finding: Finding = { + id: 'R1-1', + severity: 'Critical', + confidence: 'high', + source: 'test', + summary: 'The result is lost.', + shortSummary: 'The result is lost.', + failureScenario: 'Saving after cleanup loses the result.', + suggestedFix: 'Save before cleanup.', + category: 'correctness', + locations: [{ file: 'src/review.ts', line: 12, anchor: 'save();' }], + assetFiles: ['evidence/local.png'], + assets: ['https://example.com/evidence.png'], + outcome: 'fixed', + outcomeNote: 'Saved atomically.', + heldByMeasurement: { file: 'src/review.test.ts' }, +}; + +const verdict = { + event: 'REQUEST_CHANGES', + body: 'One blocker remains.', + baseEvent: 'REQUEST_CHANGES', + cappedBy: ['coverage gap'], + downgraded: true, + downgradedFrom: 'Request changes', + remediation: ['Run verification again.'], + lowSignal: { agents: 4, srcDiffLines: 120 }, + verdictLine: 'Verdict: Comment — Request changes was downgraded', +}; + +function writeJson(path: string, value: unknown): void { + mkdirSync(join(path, '..'), { recursive: true }); + writeFileSync(path, JSON.stringify(value)); +} + +function fixture() { + const findings = join(root, '.qwen/tmp/findings.json'); + const composed = join(root, '.qwen/tmp/composed.json'); + const report = join(root, '.qwen/reviews/review.md'); + const out = join(root, '.qwen/reviews/review.json'); + writeJson(findings, buildReport([finding])); + writeJson(composed, verdict); + mkdirSync(join(root, '.qwen/reviews'), { recursive: true }); + writeFileSync(report, '# Review\n'); + return { findings, composed, report, out }; +} + +beforeEach(() => { + root = mkdtempSync(join(tmpdir(), 'review-artifact-')); + previousProjectDir = process.env['QWEN_CODE_PROJECT_DIR']; + process.env['QWEN_CODE_PROJECT_DIR'] = root; +}); + +afterEach(() => { + rmSync(root, { recursive: true, force: true }); + if (previousProjectDir === undefined) { + delete process.env['QWEN_CODE_PROJECT_DIR']; + } else { + process.env['QWEN_CODE_PROJECT_DIR'] = previousProjectDir; + } +}); + +describe('saveReviewArtifact', () => { + it('copies canonical findings and the composed verdict into schema v1', () => { + const paths = fixture(); + saveReviewArtifact({ + ...paths, + target: 'pr-123', + effort: 'high', + }); + + const document = JSON.parse(readFileSync(paths.out, 'utf8')); + expect(document).toEqual({ + schemaVersion: 1, + target: 'pr-123', + effort: 'high', + verdict, + ...buildReport([finding]), + markdownReportPath: '.qwen/reviews/review.md', + }); + expect(readFileSync(paths.out, 'utf8')).toMatch(/\n$/); + expect( + readdirSync(join(root, '.qwen/reviews')).filter((name) => + name.endsWith('.tmp'), + ), + ).toEqual([]); + }); + + it('reads PR inputs from its worktree and writes durable output to the main project', () => { + const paths = fixture(); + const worktree = join(root, '.qwen/tmp/review-pr-123'); + const findings = join(worktree, '.qwen/tmp/findings.json'); + const composed = join(worktree, '.qwen/tmp/composed.json'); + writeJson(findings, buildReport([finding])); + writeJson(composed, verdict); + + saveReviewArtifact({ + ...paths, + findings, + composed, + target: 'pr-123', + effort: 'high', + }); + + expect(JSON.parse(readFileSync(paths.out, 'utf8'))).toMatchObject({ + schemaVersion: 1, + target: 'pr-123', + verdict, + markdownReportPath: '.qwen/reviews/review.md', + }); + }); + + it('refuses outputs outside .qwen/reviews, including .qwen/tmp and outside the workspace', () => { + const paths = fixture(); + const outside = mkdtempSync(join(tmpdir(), 'review-artifact-outside-')); + try { + for (const out of [ + join(root, '.qwen/tmp/review.json'), + join(root, '.qwen/reviews-other/review.json'), + join(outside, 'review.json'), + ]) { + expect(() => + saveReviewArtifact({ + ...paths, + out, + target: 'local', + effort: 'medium', + }), + ).toThrow(/workspace|under .*\.qwen.*reviews/); + expect(existsSync(out)).toBe(false); + } + } finally { + rmSync(outside, { recursive: true, force: true }); + } + }); + + it('refuses an output path that traverses a symlink', () => { + const paths = fixture(); + const outside = mkdtempSync(join(tmpdir(), 'review-artifact-outside-')); + rmSync(join(root, '.qwen/reviews'), { recursive: true }); + symlinkSync(outside, join(root, '.qwen/reviews')); + try { + expect(() => + saveReviewArtifact({ + ...paths, + target: 'local', + effort: 'medium', + }), + ).toThrow(/symbolic link/); + expect(existsSync(join(outside, 'review.json'))).toBe(false); + } finally { + rmSync(outside, { recursive: true, force: true }); + } + }); + + it.each(['findings', 'composed', 'report'] as const)( + 'refuses a %s input path that traverses a symlink', + (input) => { + const paths = fixture(); + const outside = mkdtempSync(join(tmpdir(), 'review-artifact-input-')); + const outsideFile = join( + outside, + input === 'report' ? 'review.md' : `${input}.json`, + ); + writeFileSync( + outsideFile, + input === 'report' ? '# Outside\n' : readFileSync(paths[input], 'utf8'), + ); + rmSync(paths[input]); + symlinkSync(outsideFile, paths[input]); + try { + expect(() => + saveReviewArtifact({ + ...paths, + target: 'local', + effort: 'medium', + }), + ).toThrow(/symbolic link/); + expect(existsSync(paths.out)).toBe(false); + } finally { + rmSync(outside, { recursive: true, force: true }); + } + }, + ); + + it('fails closed on malformed findings without leaving output', () => { + const paths = fixture(); + writeFileSync(paths.findings, '{'); + + expect(() => + saveReviewArtifact({ + ...paths, + target: 'local', + effort: 'medium', + }), + ).toThrow(/not valid JSON/); + expect(existsSync(paths.out)).toBe(false); + }); + + it('rejects inconsistent canonical counts instead of recalculating them', () => { + const paths = fixture(); + const report = buildReport([finding]); + report.counts.total = 0; + writeJson(paths.findings, report); + + expect(() => + saveReviewArtifact({ + ...paths, + target: 'local', + effort: 'medium', + }), + ).toThrow(/inconsistent/); + expect(existsSync(paths.out)).toBe(false); + }); + + it('accepts an empty body from an inline-only request-changes verdict', () => { + const paths = fixture(); + writeJson(paths.composed, { ...verdict, body: '' }); + + saveReviewArtifact({ + ...paths, + target: 'local', + effort: 'medium', + }); + + expect(JSON.parse(readFileSync(paths.out, 'utf8')).verdict.body).toBe(''); + }); + + it('fails closed when the composed verdict is incomplete', () => { + const paths = fixture(); + const { verdictLine: _removed, ...incomplete } = verdict; + writeJson(paths.composed, incomplete); + + expect(() => + saveReviewArtifact({ + ...paths, + target: 'local', + effort: 'medium', + }), + ).toThrow(/verdictLine/); + expect(existsSync(paths.out)).toBe(false); + }); + + it.each(['findings', 'composed', 'report'] as const)( + 'refuses to overwrite the %s input', + (input) => { + const paths = fixture(); + const inputPath = join( + root, + '.qwen/reviews', + input === 'report' ? 'review.md' : `${input}.json`, + ); + if (input !== 'report') { + writeFileSync(inputPath, readFileSync(paths[input], 'utf8')); + } + const original = readFileSync(inputPath, 'utf8'); + + expect(() => + saveReviewArtifact({ + ...paths, + [input]: inputPath, + out: inputPath, + target: 'local', + effort: 'medium', + }), + ).toThrow(/must not overwrite/); + expect(readFileSync(inputPath, 'utf8')).toBe(original); + }, + ); + + it.skipIf(!caseInsensitiveFs)( + 'refuses a case-insensitive output alias of the Markdown report', + () => { + const paths = fixture(); + const alias = join(root, '.qwen/reviews/REVIEW.MD'); + expect(existsSync(alias)).toBe(true); + const original = readFileSync(paths.report, 'utf8'); + + expect(() => + saveReviewArtifact({ + ...paths, + out: alias, + target: 'local', + effort: 'medium', + }), + ).toThrow(/must not overwrite/); + expect(readFileSync(paths.report, 'utf8')).toBe(original); + }, + ); + + it('requires the Markdown report to be durable under .qwen/reviews', () => { + const paths = fixture(); + const temporaryReport = join(root, '.qwen/tmp/review.md'); + writeFileSync(temporaryReport, '# Temporary\n'); + + expect(() => + saveReviewArtifact({ + ...paths, + report: temporaryReport, + target: 'local', + effort: 'medium', + }), + ).toThrow(/Markdown report must be a file under/); + expect(existsSync(paths.out)).toBe(false); + }); + + it('requires the Markdown report to exist before creating output', () => { + const paths = fixture(); + rmSync(paths.report); + + expect(() => + saveReviewArtifact({ + ...paths, + target: 'local', + effort: 'medium', + }), + ).toThrow(/Could not read the Markdown report/); + expect(existsSync(paths.out)).toBe(false); + }); + + it('names a directory Markdown report as not a file instead of unreadable', () => { + const paths = fixture(); + rmSync(paths.report); + mkdirSync(paths.report); + + expect(() => + saveReviewArtifact({ + ...paths, + target: 'local', + effort: 'medium', + }), + ).toThrow(/Markdown report is not a file/); + expect(existsSync(paths.out)).toBe(false); + }); + + it('refuses a low-effort artifact', () => { + const paths = fixture(); + + expect(() => + saveReviewArtifact({ + ...paths, + target: 'local', + effort: 'low', + }), + ).toThrow(/does not support low-effort reviews/); + expect(existsSync(paths.out)).toBe(false); + }); + + it('resolves relative paths against the workspace root, not cwd', () => { + // The form SKILL.md documents. beforeEach points QWEN_CODE_PROJECT_DIR at + // the temp root while cwd stays the package directory, so the two roots + // differ and the resolution direction is observable. + fixture(); + + const saved = saveReviewArtifact({ + findings: '.qwen/tmp/findings.json', + composed: '.qwen/tmp/composed.json', + report: '.qwen/reviews/review.md', + out: '.qwen/reviews/review.json', + target: 'pr-123', + effort: 'high', + }); + + expect(saved.path).toBe(join(root, '.qwen/reviews/review.json')); + // The registration value `record_artifact` wants, printed so the skill + // copies it verbatim — including from a PR worktree run where cwd (the + // disposable review worktree) differs from the workspace root. + expect(saved.workspacePath).toBe('.qwen/reviews/review.json'); + expect(JSON.parse(readFileSync(saved.path, 'utf8'))).toMatchObject({ + schemaVersion: 1, + target: 'pr-123', + markdownReportPath: '.qwen/reviews/review.md', + }); + }); +}); diff --git a/packages/cli/src/commands/review/save-artifact.ts b/packages/cli/src/commands/review/save-artifact.ts new file mode 100644 index 00000000000..40386e9118e --- /dev/null +++ b/packages/cli/src/commands/review/save-artifact.ts @@ -0,0 +1,387 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +import { atomicWriteFileSync } from '@qwen-code/qwen-code-core'; +import { + existsSync, + lstatSync, + mkdirSync, + readFileSync, + statSync, +} from 'node:fs'; +import { + dirname, + isAbsolute, + join, + relative, + resolve, + sep, + toNamespacedPath, +} from 'node:path'; +import { isDeepStrictEqual } from 'node:util'; +import type { CommandModule } from 'yargs'; +import type { ComposeReviewResult, ReviewEvent } from './compose-review.js'; +import { + buildReport, + type FindingsReport, + validateFindings, +} from './findings.js'; +import { EFFORT_LEVELS, type ReviewEffort } from './parse-args.js'; +import { REVIEWS_DIR } from './lib/paths.js'; +import { writeStderrLine, writeStdoutLine } from '../../utils/stdioHelpers.js'; + +interface PersistedVerdict extends ComposeReviewResult { + verdictLine: string; +} + +export interface ReviewArtifactV1 { + schemaVersion: 1; + target: string; + effort: ReviewEffort; + verdict: PersistedVerdict; + findings: FindingsReport['findings']; + counts: FindingsReport['counts']; + outcomesRecorded: boolean; + markdownReportPath: string; +} + +export interface SavedReviewArtifact { + /** Absolute path of the written document. */ + path: string; + /** + * The same path relative to the workspace root — the exact value + * `record_artifact` wants as `workspacePath`, so the skill copies it + * verbatim instead of re-deriving it from the absolute path. + */ + workspacePath: string; +} + +interface SaveArtifactArgs { + findings: string; + composed: string; + report: string; + target: string; + effort: ReviewEffort; + out: string; +} + +// Every path resolves against the daemon workspace root, not cwd: in PR +// worktree mode cwd is the disposable review worktree, while the durable +// output and `markdownReportPath` must stay relative to the main project for +// Web Shell's `readWorkspaceFile` to find them. The skill threads that root +// through its subprocesses as QWEN_CODE_PROJECT_DIR. +function workspaceRoot(): string { + return resolve(process.env['QWEN_CODE_PROJECT_DIR'] ?? process.cwd()); +} + +function isWithin(parent: string, child: string): boolean { + const rel = relative(parent, child); + return ( + rel === '' || + (!isAbsolute(rel) && rel !== '..' && !rel.startsWith(`..${sep}`)) + ); +} + +function workspacePath(root: string, value: string, label: string): string { + const absolute = resolve(root, value); + if (!isWithin(root, absolute)) { + throw new Error( + `${label} must be inside the workspace: ${JSON.stringify(value)}.`, + ); + } + return absolute; +} + +function rejectSymlinkPath(root: string, path: string, label: string): void { + const rel = relative(root, path); + let current = root; + for (const segment of rel.split(sep)) { + current = join(current, segment); + if (existsSync(current) && lstatSync(current).isSymbolicLink()) { + throw new Error(`${label} must not traverse a symbolic link.`); + } + } +} + +function sameFile(left: string, right: string): boolean { + if (left === right) return true; + if (!existsSync(left) || !existsSync(right)) return false; + const leftStat = statSync(left); + const rightStat = statSync(right); + return leftStat.dev === rightStat.dev && leftStat.ino === rightStat.ino; +} + +function readText(path: string, label: string): string { + try { + return readFileSync(path, 'utf8'); + } catch (error) { + throw new Error( + `Could not read the ${label} file at ${JSON.stringify(path)}: ${error instanceof Error ? error.message : String(error)}`, + ); + } +} + +function readJson(path: string, label: string): unknown { + const text = readText(path, label); + try { + return JSON.parse(text); + } catch (error) { + throw new Error( + `The ${label} file at ${JSON.stringify(path)} is not valid JSON: ${error instanceof Error ? error.message : String(error)}`, + ); + } +} + +function object(value: unknown, label: string): Record { + if (value === null || typeof value !== 'object' || Array.isArray(value)) { + throw new Error(`${label} must be an object.`); + } + return value as Record; +} + +function string(value: unknown, label: string): string { + if (typeof value !== 'string') { + throw new Error(`${label} must be a string.`); + } + return value; +} + +function nonEmptyString(value: unknown, label: string): string { + const text = string(value, label); + if (text.trim() === '') { + throw new Error(`${label} must be a non-empty string.`); + } + return text; +} + +function stringArray(value: unknown, label: string): string[] { + if ( + !Array.isArray(value) || + value.some((entry) => typeof entry !== 'string') + ) { + throw new Error(`${label} must be an array of strings.`); + } + return value as string[]; +} + +function event(value: unknown, label: string): ReviewEvent { + if ( + value !== 'APPROVE' && + value !== 'COMMENT' && + value !== 'REQUEST_CHANGES' + ) { + throw new Error(`${label} has an unsupported value.`); + } + return value; +} + +function validateVerdict(value: unknown): PersistedVerdict { + const verdict = object(value, 'Composed verdict'); + const downgradedFrom = verdict['downgradedFrom']; + if ( + downgradedFrom !== null && + downgradedFrom !== 'Approve' && + downgradedFrom !== 'Request changes' + ) { + throw new Error( + 'Composed verdict.downgradedFrom has an unsupported value.', + ); + } + if (typeof verdict['downgraded'] !== 'boolean') { + throw new Error('Composed verdict.downgraded must be a boolean.'); + } + const lowSignal = verdict['lowSignal']; + if (lowSignal !== null) { + const signal = object(lowSignal, 'Composed verdict.lowSignal'); + for (const key of ['agents', 'srcDiffLines'] as const) { + if ( + typeof signal[key] !== 'number' || + !Number.isInteger(signal[key]) || + signal[key] < 0 + ) { + throw new Error( + `Composed verdict.lowSignal.${key} must be a non-negative integer.`, + ); + } + } + } + return { + event: event(verdict['event'], 'Composed verdict.event'), + body: string(verdict['body'], 'Composed verdict.body'), + baseEvent: event(verdict['baseEvent'], 'Composed verdict.baseEvent'), + cappedBy: stringArray(verdict['cappedBy'], 'Composed verdict.cappedBy'), + downgraded: verdict['downgraded'], + downgradedFrom, + remediation: stringArray( + verdict['remediation'], + 'Composed verdict.remediation', + ), + lowSignal: + lowSignal === null + ? null + : { + agents: (lowSignal as Record)['agents']!, + srcDiffLines: (lowSignal as Record)[ + 'srcDiffLines' + ]!, + }, + verdictLine: nonEmptyString( + verdict['verdictLine'], + 'Composed verdict.verdictLine', + ), + }; +} + +function validateFindingsReport(value: unknown): FindingsReport { + const report = object(value, 'Canonical findings report'); + if (typeof report['outcomesRecorded'] !== 'boolean') { + throw new Error( + 'Canonical findings report.outcomesRecorded must be a boolean.', + ); + } + const canonical = buildReport(validateFindings(report['findings'])); + const supplied: FindingsReport = { + findings: report['findings'] as FindingsReport['findings'], + counts: report['counts'] as FindingsReport['counts'], + outcomesRecorded: report['outcomesRecorded'], + }; + if (!isDeepStrictEqual(supplied, canonical)) { + throw new Error( + 'Canonical findings report is malformed or inconsistent with its findings.', + ); + } + return supplied; +} + +export function saveReviewArtifact( + args: SaveArtifactArgs, +): SavedReviewArtifact { + const root = workspaceRoot(); + const findingsPath = workspacePath(root, args.findings, 'Findings input'); + const composedPath = workspacePath(root, args.composed, 'Composed input'); + const reportPath = workspacePath(root, args.report, 'Markdown report'); + const outputPath = workspacePath(root, args.out, 'Output'); + const reviewsRoot = resolve(root, REVIEWS_DIR); + + if (!isWithin(reviewsRoot, reportPath) || reportPath === reviewsRoot) { + throw new Error( + `Markdown report must be a file under ${JSON.stringify(REVIEWS_DIR)}: ${JSON.stringify(args.report)}.`, + ); + } + if (!isWithin(reviewsRoot, outputPath) || outputPath === reviewsRoot) { + throw new Error( + `Output must be a file under ${JSON.stringify(REVIEWS_DIR)}: ${JSON.stringify(args.out)}.`, + ); + } + for (const [label, inputPath] of [ + ['findings input', findingsPath], + ['composed input', composedPath], + ['Markdown report', reportPath], + ] as const) { + if (sameFile(outputPath, inputPath)) { + throw new Error(`Output must not overwrite the ${label}.`); + } + } + rejectSymlinkPath(root, findingsPath, 'Findings input'); + rejectSymlinkPath(root, composedPath, 'Composed input'); + rejectSymlinkPath(root, reportPath, 'Markdown report'); + rejectSymlinkPath(root, outputPath, 'Output'); + if (!EFFORT_LEVELS.has(args.effort)) { + throw new Error( + `Unsupported review effort: ${JSON.stringify(args.effort)}.`, + ); + } + if (args.effort === 'low') { + throw new Error( + 'save-artifact does not support low-effort reviews: low has no canonical composed verdict to persist.', + ); + } + nonEmptyString(args.target, 'Target'); + + const findings = validateFindingsReport( + readJson(findingsPath, 'canonical findings'), + ); + const verdict = validateVerdict(readJson(composedPath, 'composed verdict')); + let reportIsFile: boolean | undefined; + try { + reportIsFile = statSync(reportPath).isFile(); + } catch { + // Missing or unreadable: readText below reports it with context. + } + if (reportIsFile === false) { + throw new Error( + `Markdown report is not a file: ${JSON.stringify(args.report)}.`, + ); + } + readText(reportPath, 'Markdown report'); + + const markdownReportPath = relative(root, reportPath).split(sep).join('/'); + const document: ReviewArtifactV1 = { + schemaVersion: 1, + target: args.target, + effort: args.effort, + verdict, + findings: findings.findings, + counts: findings.counts, + outcomesRecorded: findings.outcomesRecorded, + markdownReportPath, + }; + + mkdirSync(dirname(outputPath), { recursive: true }); + atomicWriteFileSync( + toNamespacedPath(outputPath), + `${JSON.stringify(document, null, 2)}\n`, + { encoding: 'utf8', noFollow: true }, + ); + return { + path: outputPath, + workspacePath: relative(root, outputPath).split(sep).join('/'), + }; +} + +export const saveArtifactCommand: CommandModule = { + command: 'save-artifact', + describe: + 'Create a durable versioned code-review JSON document from canonical review artifacts', + builder: (yargs) => + yargs + .option('findings', { + type: 'string', + demandOption: true, + describe: 'Canonical findings JSON', + }) + .option('composed', { + type: 'string', + demandOption: true, + describe: 'Composed verdict JSON', + }) + .option('report', { + type: 'string', + demandOption: true, + describe: 'Existing durable Markdown report', + }) + .option('target', { + type: 'string', + demandOption: true, + describe: 'Review target label', + }) + .option('effort', { + type: 'string', + choices: [...EFFORT_LEVELS].filter((effort) => effort !== 'low'), + demandOption: true, + describe: 'Resolved review effort', + }) + .option('out', { + type: 'string', + demandOption: true, + describe: 'Output path under .qwen/reviews/', + }), + handler: (argv) => { + const saved = saveReviewArtifact(argv as unknown as SaveArtifactArgs); + writeStdoutLine(JSON.stringify(saved)); + writeStderrLine(`save-artifact: wrote ${saved.path}`); + }, +}; diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 5eccebc2bb1..7cb12ba6c92 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -10,6 +10,7 @@ allowedTools: - write_file - edit - glob + - record_artifact --- # Code Review @@ -1109,6 +1110,36 @@ Report content should include: A run that had read `Verdict: Comment — an Approve was NOT available` wrote `**Verdict:** Approve` into its saved report minutes later. The terminal is prose and the archive is forever; this line is the one place the archive can be made to tell the truth for free. If the composed event is not the one you expected, fix the run — not the report. +After the Markdown report exists, and before cleanup, create and register the structured review artifact for **medium and high** effort (low has no canonical composed verdict and must not invent one). Use the same filename stem as the Markdown report with a `.json` extension: + +```bash +"${QWEN_CODE_CLI:-qwen}" review save-artifact \ + --findings .qwen/tmp/qwen-review--findings.json \ + --composed .qwen/tmp/qwen-review--composed.json \ + --report .qwen/reviews/.md \ + --target \ + --effort \ + --out .qwen/reviews/.json +``` + +For PR worktree mode, the findings and composed inputs were created inside `worktreePath`, while the durable report and output belong to the main project directory. Pass absolute paths for all four: resolve `--findings` and `--composed` against `worktreePath`, and resolve `--report` and `--out` against the main project directory. The worktree lives under the main project's `.qwen/tmp/`, so all four remain inside the session workspace accepted by the helper. `save-artifact` prints one JSON object on stdout — `{"path": "", "workspacePath": ""}`. Then call `record_artifact` in the current session with exactly this registration shape, copying `workspacePath` from that stdout object verbatim (do not re-derive it from the absolute path): + +```json +{ + "title": "Code review result", + "kind": "other", + "storage": "workspace", + "workspacePath": ".qwen/reviews/.json", + "mimeType": "application/vnd.qwen.code-review+json", + "metadata": { + "artifactType": "code_review", + "schemaVersion": 1 + } +} +``` + +The JSON helper is fail-closed because it carries the authoritative review result: if it fails, do not synthesize a replacement or register a partial artifact. A `record_artifact` failure is a UI-delivery failure, not a review-verdict input: disclose the failure to the user, keep the Markdown report, and do **not** change, soften, or recompute the existing composed verdict. + ### Incremental review cache If reviewing a PR **at high effort**, update the review cache for incremental review support. Low and medium reviews must NOT write it — a cache hit would make a later high-effort review of the same SHA report "No new changes since last review", silently converting a cheaper pass into a full-review verdict. diff --git a/packages/web-shell/client/App.test.tsx b/packages/web-shell/client/App.test.tsx index b69f9df2177..3321f803f0b 100644 --- a/packages/web-shell/client/App.test.tsx +++ b/packages/web-shell/client/App.test.tsx @@ -945,6 +945,10 @@ vi.doMock('./components/SplitView', async () => { updatedAt: '2026-07-10T00:01:00Z', sizeBytes: 20, }; + const changedArtifact = { + ...updatedArtifact, + status: 'changed', + }; return React.createElement( 'div', { 'data-testid': 'split-view-mock' }, @@ -999,6 +1003,20 @@ vi.doMock('./components/SplitView', async () => { }, 'updated artifact', ), + React.createElement( + 'button', + { + 'data-testid': 'split-report-changed-artifact', + type: 'button', + onClick: () => + props.onPaneArtifactsChange?.( + 'pane-session', + [changedArtifact], + paneActions, + ), + }, + 'changed artifact', + ), React.createElement( 'button', { @@ -4510,7 +4528,7 @@ describe('App session callbacks', () => { emptyActions?.querySelectorAll('button') ?? [], ); expect(actions).toHaveLength(1); - expect(actions[0]?.textContent).toContain('Review'); + expect(actions[0]?.textContent).toContain('Changes'); expect(actions[0]?.disabled).toBe(true); expect( container.querySelector('button[aria-label="Add panel"]'), @@ -4587,16 +4605,16 @@ describe('App session callbacks', () => { ) ?.click(); }); - const review = Array.from( + const changes = Array.from( container.querySelectorAll( '[data-testid="right-panel-empty-actions"] button', ), - ).find((button) => button.textContent?.startsWith('Review')); - expect(review?.disabled).toBe(false); + ).find((button) => button.textContent?.startsWith('Changes')); + expect(changes?.disabled).toBe(false); - act(() => review?.click()); + act(() => changes?.click()); - expect(container.querySelector('button[title="Review"]')).not.toBeNull(); + expect(container.querySelector('button[title="Changes"]')).not.toBeNull(); expect(container.textContent).toContain('latest.ts'); expect(container.textContent).not.toContain('first.ts'); }); @@ -9918,6 +9936,17 @@ describe('App session callbacks', () => { expect(document.body.textContent).toContain('20 B'); + await act(async () => { + container + .querySelector( + '[data-testid="split-report-changed-artifact"]', + ) + ?.click(); + await Promise.resolve(); + }); + + expect(document.body.textContent).toContain('changed'); + await act(async () => { container .querySelector( diff --git a/packages/web-shell/client/App.tsx b/packages/web-shell/client/App.tsx index dc874a278f5..0ec17b82cc1 100644 --- a/packages/web-shell/client/App.tsx +++ b/packages/web-shell/client/App.tsx @@ -2292,10 +2292,16 @@ export function App({ previous.artifacts.length === paneArtifacts.length && previous.artifacts.every((artifact, index) => { const nextArtifact = paneArtifacts[index]; + // `metadata` is deliberately not compared: artifact events carry + // freshly parsed objects, so an identity check here would defeat + // the guard without catching a realistic change. return ( nextArtifact?.id === artifact.id && + nextArtifact.status === artifact.status && nextArtifact.updatedAt === artifact.updatedAt && - nextArtifact.sizeBytes === artifact.sizeBytes + nextArtifact.sizeBytes === artifact.sizeBytes && + nextArtifact.title === artifact.title && + nextArtifact.workspacePath === artifact.workspacePath ); }); if (unchanged) return current; diff --git a/packages/web-shell/client/components/artifacts/ArtifactPanel.test.tsx b/packages/web-shell/client/components/artifacts/ArtifactPanel.test.tsx index 9875b07dede..ed1613e1738 100644 --- a/packages/web-shell/client/components/artifacts/ArtifactPanel.test.tsx +++ b/packages/web-shell/client/components/artifacts/ArtifactPanel.test.tsx @@ -3,6 +3,7 @@ import { afterEach, describe, expect, it, vi } from 'vitest'; import { act } from 'react'; import { createRoot, type Root } from 'react-dom/client'; import type { + DaemonSessionArtifact, DaemonSessionMonitorTaskStatus, DaemonSessionShellTaskStatus, } from '@qwen-code/sdk/daemon'; @@ -16,6 +17,7 @@ const { mockActions, mockWorkspaceActions } = vi.hoisted(() => ({ }, mockWorkspaceActions: { readFileBytes: vi.fn(), + readWorkspaceFile: vi.fn(), stat: vi.fn(), }, })); @@ -91,6 +93,51 @@ function shellPanel(task: DaemonSessionShellTaskStatus) { ); } +function codeReviewArtifact( + patch: Partial = {}, +): DaemonSessionArtifact { + return { + id: 'review-artifact', + kind: 'other', + storage: 'workspace', + source: 'tool', + status: 'available', + title: 'Code review result', + workspacePath: '.qwen/reviews/review.json', + metadata: { artifactType: 'code_review', schemaVersion: 1 }, + retention: 'ephemeral', + clientRetained: false, + createdAt: '2026-08-03T00:00:00.000Z', + updatedAt: '2026-08-03T00:00:00.000Z', + ...patch, + }; +} + +function artifactPanel(artifact: DaemonSessionArtifact) { + return ( + + {}} + onCloseTab={() => {}} + onOpenFilePreview={() => {}} + onClose={() => {}} + /> + + ); +} + afterEach(() => { for (const { root, container } of mounted) { act(() => root.unmount()); @@ -100,6 +147,7 @@ afterEach(() => { mockActions.cancelTask.mockReset(); mockActions.getTasks.mockReset(); mockWorkspaceActions.readFileBytes.mockReset(); + mockWorkspaceActions.readWorkspaceFile.mockReset(); mockWorkspaceActions.stat.mockReset(); }); @@ -115,6 +163,127 @@ function openAddMenu(container: HTMLElement) { return add; } +async function flush() { + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); +} + +describe('ArtifactPanel code review artifacts', () => { + it('dispatches an available workspace artifact to the dedicated renderer', async () => { + mockWorkspaceActions.readWorkspaceFile.mockResolvedValue({ + content: JSON.stringify({ + schemaVersion: 1, + target: 'local', + effort: 'high', + verdict: { + event: 'APPROVE', + verdictLine: 'Verdict: Approve', + baseEvent: 'APPROVE', + cappedBy: [], + downgraded: false, + downgradedFrom: null, + }, + findings: [], + counts: { + total: 0, + bySeverity: { + Critical: 0, + Suggestion: 0, + 'Nice to have': 0, + }, + byConfidence: { high: 0, low: 0 }, + held: 0, + }, + outcomesRecorded: false, + markdownReportPath: '.qwen/reviews/review.md', + }), + truncated: false, + }); + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + mounted.push({ root, container }); + + act(() => root.render(artifactPanel(codeReviewArtifact()))); + await flush(); + + expect(container.textContent).toContain('Authoritative verdict'); + expect(container.textContent).toContain('Verdict: Approve'); + expect(container.querySelector('.cm-editor')).toBeNull(); + expect(mockWorkspaceActions.readWorkspaceFile).toHaveBeenCalledWith( + '.qwen/reviews/review.json', + ); + }); + + it.each(['changed', 'missing'] as const)( + 'does not render a %s artifact as authoritative', + async (status) => { + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + mounted.push({ root, container }); + + act(() => root.render(artifactPanel(codeReviewArtifact({ status })))); + await flush(); + + expect(container.querySelector('[role="alert"]')?.textContent).toContain( + status, + ); + expect(container.textContent).not.toContain('Authoritative verdict'); + expect(mockWorkspaceActions.readWorkspaceFile).not.toHaveBeenCalled(); + }, + ); + + it('requires code review artifacts to use workspace storage', async () => { + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + mounted.push({ root, container }); + + act(() => + root.render( + artifactPanel( + codeReviewArtifact({ + storage: 'external_url', + workspacePath: undefined, + }), + ), + ), + ); + await flush(); + + expect(container.querySelector('[role="alert"]')?.textContent).toContain( + 'workspace files', + ); + expect(mockWorkspaceActions.readWorkspaceFile).not.toHaveBeenCalled(); + }); + + it('still sends an ordinary JSON artifact to the generic editor', async () => { + // The regression the early `return` in the dispatch can cause: an + // artifact WITHOUT the code_review metadata must keep reaching the + // generic file preview, not the dedicated renderer. + mockWorkspaceActions.readWorkspaceFile.mockResolvedValue({ + content: '{}', + truncated: false, + }); + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + mounted.push({ root, container }); + + act(() => root.render(artifactPanel(codeReviewArtifact({ metadata: {} })))); + await flush(); + + expect(container.querySelector('.cm-editor')).not.toBeNull(); + expect(container.textContent).not.toContain('Authoritative verdict'); + expect(mockWorkspaceActions.readWorkspaceFile).toHaveBeenCalledWith( + '.qwen/reviews/review.json', + ); + }); +}); + describe('ArtifactPanel add menu', () => { it('keeps the disabled review action on the empty page and hides the add button', () => { const onClose = vi.fn(); @@ -145,7 +314,7 @@ describe('ArtifactPanel add menu', () => { container.querySelectorAll( '[data-testid="right-panel-empty-actions"] button', ), - ).find((button) => button.textContent?.includes('Review')); + ).find((button) => button.textContent?.includes('Changes')); expect(review?.disabled).toBe(true); expect(review?.textContent).toContain('View recent file changes'); expect(container.textContent).not.toContain('⌘'); @@ -378,7 +547,7 @@ describe('ArtifactPanel add menu', () => { container.querySelectorAll( '[data-testid="right-panel-empty-actions"] button', ), - ).find((button) => button.textContent?.includes('Review')); + ).find((button) => button.textContent?.includes('Changes')); expect(review?.disabled).toBe(false); act(() => review?.click()); expect(onOpenLatestReview).toHaveBeenCalledOnce(); @@ -462,7 +631,7 @@ describe('ArtifactPanel add menu', () => { openAddMenu(container); const menuText = document.body.querySelector('[role="menu"]')?.textContent; - expect(menuText).toContain('Review'); + expect(menuText).toContain('Changes'); expect(menuText).toContain('New side task'); expect(menuText).not.toContain('Existing side task'); }); diff --git a/packages/web-shell/client/components/artifacts/ArtifactPanel.tsx b/packages/web-shell/client/components/artifacts/ArtifactPanel.tsx index 7366cca4af7..f2cb564ab3b 100644 --- a/packages/web-shell/client/components/artifacts/ArtifactPanel.tsx +++ b/packages/web-shell/client/components/artifacts/ArtifactPanel.tsx @@ -76,6 +76,7 @@ import { } from './TurnOutputs'; import { LineStats, sumLineStats } from './LineStats'; import styles from './ArtifactPanel.module.css'; +import { CodeReviewArtifactDetail } from './CodeReviewArtifactDetail'; import { SubagentDetail } from './SubagentDetail'; import { SideTaskPanel } from './SideTaskPanel'; import { @@ -2116,10 +2117,27 @@ function ArtifactDetail({ const safeUrl = isSafeHref(artifact.url) ? artifact.url : undefined; const isAutomationSnapshot = artifact.metadata?.['artifactType'] === 'automation_snapshot'; + const isCodeReview = artifact.metadata?.['artifactType'] === 'code_review'; const canPreviewWorkspaceFile = artifact.storage === 'workspace' && Boolean(artifact.workspacePath); const imageMimeType = getArtifactImageMimeType(artifact); + if (isCodeReview) { + if (artifact.status !== 'available') { + return ; + } + if (!canPreviewWorkspaceFile || !artifact.workspacePath) { + return ; + } + return ( + + ); + } + if (canPreviewWorkspaceFile && artifact.workspacePath) { return ( + {t('codeReview.unavailable', { status })} + + ); +} + +function CodeReviewWorkspaceRequired() { + const { t } = useI18n(); + return ( +
+ {t('codeReview.workspaceRequired')} +
+ ); +} + function isHtmlArtifact(artifact: DaemonSessionArtifact) { const path = artifact.workspacePath?.toLowerCase() ?? ''; const mimeType = artifact.mimeType?.toLowerCase() ?? ''; diff --git a/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.module.css b/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.module.css new file mode 100644 index 00000000000..ee904f598ea --- /dev/null +++ b/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.module.css @@ -0,0 +1,248 @@ +.root { + display: grid; + gap: 14px; + color: var(--foreground); +} + +.summary { + display: flex; + align-items: flex-start; + justify-content: space-between; + gap: 12px; + padding: 14px; + border: 1px solid var(--border); + border-radius: 8px; + background: color-mix(in srgb, var(--accent) 45%, transparent); +} + +.eyebrow { + color: var(--muted-foreground); + font-size: 11px; + font-weight: 600; + letter-spacing: 0.04em; + text-transform: uppercase; +} + +.verdict { + margin: 4px 0; + font-size: 18px; + overflow-wrap: anywhere; +} + +.meta, +.path { + color: var(--muted-foreground); + font-size: 12px; + overflow-wrap: anywhere; +} + +.metrics { + display: grid; + grid-template-columns: repeat(auto-fit, minmax(86px, 1fr)); + gap: 8px; +} + +.metric { + display: grid; + gap: 2px; + padding: 9px; + border: 1px solid var(--border); + border-radius: 6px; +} + +.metric strong { + font-size: 18px; +} + +.metric span { + color: var(--muted-foreground); + font-size: 11px; +} + +.caps { + display: flex; + align-items: baseline; + gap: 10px; + padding: 9px 11px; + border-left: 3px solid var(--border); + background: color-mix(in srgb, var(--muted) 65%, transparent); + font-size: 13px; + overflow-wrap: anywhere; +} + +.filters { + display: flex; + flex-wrap: wrap; + gap: 10px; +} + +.filters label { + display: grid; + gap: 4px; + color: var(--muted-foreground); + font-size: 12px; +} + +.filters select { + min-width: 150px; + padding: 7px 9px; + border: 1px solid var(--border); + border-radius: 6px; + background: var(--background); + color: var(--foreground); + font: inherit; +} + +.filters select:focus-visible, +.primaryButton:focus-visible, +.secondaryButton:focus-visible { + outline: 2px solid var(--ring); + outline-offset: 2px; +} + +.findings { + display: grid; + gap: 10px; +} + +.finding { + display: grid; + gap: 10px; + padding: 13px; + border: 1px solid var(--border); + border-radius: 8px; +} + +.finding h3, +.detailBlock p { + margin: 0; +} + +.finding h3 { + font-size: 15px; + overflow-wrap: anywhere; +} + +.findingHeader { + display: flex; + flex-wrap: wrap; + gap: 6px 10px; + color: var(--muted-foreground); + font-size: 11px; +} + +.severity { + color: var(--foreground); + font-weight: 700; +} + +.detailBlock { + display: grid; + gap: 4px; + font-size: 13px; +} + +.detailBlock strong { + color: var(--muted-foreground); + font-size: 11px; + text-transform: uppercase; +} + +.detailBlock ul { + display: grid; + gap: 3px; + margin: 0; + padding-left: 20px; +} + +.detailBlock p, +.detailBlock li, +.detailBlock a, +.notLinked { + overflow-wrap: anywhere; + white-space: pre-wrap; +} + +.detailBlock a { + color: var(--link-color, #2563eb); +} + +.notLinked { + color: var(--muted-foreground); +} + +.primaryButton, +.secondaryButton { + appearance: none; + border: 1px solid var(--border); + border-radius: 6px; + padding: 7px 10px; + color: var(--foreground); + font: inherit; + font-size: 12px; + cursor: pointer; +} + +.primaryButton { + flex: 0 0 auto; + background: var(--foreground); + color: var(--background); +} + +.secondaryButton { + justify-self: start; + background: transparent; +} + +.primaryButton:hover, +.secondaryButton:hover { + opacity: 0.82; +} + +.report { + display: grid; + gap: 12px; +} + +.evidenceImage { + display: block; + max-width: 100%; + max-height: 240px; + margin-bottom: 3px; + border: 1px solid var(--border); + border-radius: 6px; +} + +.empty { + padding: 16px 4px; + color: var(--muted-foreground); + font-size: 13px; +} + +.errorFallback { + display: grid; + gap: 8px; + justify-items: start; +} + +.error { + display: grid; + gap: 5px; + padding: 12px; + border: 1px solid color-mix(in srgb, #dc2626 60%, var(--border)); + border-radius: 8px; + color: var(--foreground); + background: color-mix(in srgb, #dc2626 8%, var(--background)); + font-size: 13px; + overflow-wrap: anywhere; +} + +@media (max-width: 900px) { + .summary { + display: grid; + } + + .primaryButton { + justify-self: start; + } +} diff --git a/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.test.tsx b/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.test.tsx new file mode 100644 index 00000000000..e7b0039d421 --- /dev/null +++ b/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.test.tsx @@ -0,0 +1,637 @@ +// @vitest-environment jsdom +import { act } from 'react'; +import { createRoot, type Root } from 'react-dom/client'; +import type { DaemonWorkspaceActions } from '@qwen-code/webui/daemon-react-sdk'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { I18nProvider } from '../../i18n'; +import { CodeReviewArtifactDetail } from './CodeReviewArtifactDetail'; + +( + globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean } +).IS_REACT_ACT_ENVIRONMENT = true; + +const mounted: Array<{ root: Root; container: HTMLElement }> = []; + +const reviewDocument = { + schemaVersion: 1, + target: 'PR #123', + effort: 'high', + verdict: { + event: 'REQUEST_CHANGES', + verdictLine: 'Verdict: Request changes — C=9 S=4', + baseEvent: 'REQUEST_CHANGES', + cappedBy: ['coverage'], + downgraded: false, + downgradedFrom: null, + }, + findings: [ + { + id: 'R1-1', + severity: 'Critical', + confidence: 'high', + source: 'test', + summary: 'The request can overwrite another workspace.', + shortSummary: 'Cross-workspace overwrite', + failureScenario: + 'Open workspace B and submit a request from workspace A.', + suggestedFix: 'Resolve the runtime before writing.', + category: 'security', + locations: [{ file: 'src/write.ts', line: 42, anchor: 'writeFile()' }], + assets: ['https://example.com/evidence.png', 'javascript:alert(1)'], + outcome: 'skipped', + outcomeNote: 'Outside the reviewed diff.', + }, + { + id: 'R1-2', + severity: 'Suggestion', + confidence: 'low', + source: 'review', + summary: 'The label is ambiguous.', + shortSummary: 'Ambiguous label', + failureScenario: 'A user cannot distinguish changes from review results.', + locations: [{ file: 'src/panel.tsx' }], + heldByMeasurement: { file: 'src/panel.test.tsx' }, + }, + { + id: 'R1-3', + severity: 'Nice to have', + confidence: 'high', + source: 'review', + summary: 'The empty state could be shorter.', + shortSummary: 'Shorten empty state', + failureScenario: 'The panel uses more vertical space than necessary.', + locations: [{ file: 'src/empty.tsx' }], + }, + ], + counts: { + total: 14, + bySeverity: { Critical: 9, Suggestion: 4, 'Nice to have': 1 }, + byConfidence: { high: 11, low: 3 }, + byOutcome: { fixed: 0, skipped: 1, no_change_needed: 0 }, + held: 2, + }, + outcomesRecorded: false, + markdownReportPath: '.qwen/reviews/pr-123.md', +}; + +function renderWith( + content: string, + report = '# Durable report', + language: 'en' | 'zh-CN' = 'en', +) { + const actions = { + readWorkspaceFile: vi.fn((path: string) => + Promise.resolve({ + content: path.endsWith('.md') ? report : content, + truncated: false, + }), + ), + } as unknown as DaemonWorkspaceActions; + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + mounted.push({ root, container }); + act(() => { + root.render( + + + , + ); + }); + return { actions, container }; +} + +async function flush() { + await act(async () => { + await Promise.resolve(); + await Promise.resolve(); + }); +} + +afterEach(() => { + for (const { root, container } of mounted) { + act(() => root.unmount()); + container.remove(); + } + mounted.length = 0; +}); + +describe('CodeReviewArtifactDetail', () => { + it('renders authoritative verdict, counts and finding details without recomputing them', async () => { + const { container } = renderWith(JSON.stringify(reviewDocument)); + await flush(); + + expect(container.textContent).toContain( + 'Verdict: Request changes — C=9 S=4', + ); + expect(container.textContent).toContain('14Total'); + expect(container.textContent).toContain('9Critical'); + expect(container.textContent).toContain('1Nice to have'); + expect(container.textContent).toContain( + 'The empty state could be shorter.', + ); + expect(container.textContent).toContain('coverage'); + expect(container.textContent).toContain('Source: test'); + expect(container.textContent).toContain('Failure scenario'); + expect(container.textContent).toContain('Suggested fix'); + expect(container.textContent).toContain( + 'skipped — Outside the reviewed diff.', + ); + expect(container.textContent).toContain('src/write.ts:42'); + expect(container.textContent).toContain( + 'Held back from Critical by measurement', + ); + expect(container.textContent).toContain('src/panel.test.tsx'); + }); + + it('localizes the review chrome without translating canonical finding data', async () => { + const { container } = renderWith( + JSON.stringify(reviewDocument), + '# Durable report', + 'zh-CN', + ); + await flush(); + + expect(container.textContent).toContain('权威裁决'); + expect(container.textContent).toContain('打开 Markdown 报告'); + expect(container.textContent).toContain('严重级别'); + expect(container.textContent).toContain('失败场景'); + expect(container.textContent).toContain( + 'The request can overwrite another workspace.', + ); + }); + + it('filters findings by severity and confidence while preserving authoritative counts', async () => { + const { container } = renderWith(JSON.stringify(reviewDocument)); + await flush(); + + const severity = container.querySelector( + 'select[aria-label="Severity"]', + ); + await act(async () => { + if (severity) { + severity.value = 'Suggestion'; + severity.dispatchEvent(new Event('change', { bubbles: true })); + } + }); + expect(container.textContent).not.toContain('Cross-workspace overwrite'); + expect(container.textContent).toContain('The label is ambiguous.'); + expect(container.textContent).toContain('9Critical'); + + const confidence = container.querySelector( + 'select[aria-label="Confidence"]', + ); + await act(async () => { + if (confidence) { + confidence.value = 'high'; + confidence.dispatchEvent(new Event('change', { bubbles: true })); + } + }); + expect(container.textContent).toContain('No findings match these filters.'); + }); + + it('allows only safe evidence links', async () => { + const { container } = renderWith(JSON.stringify(reviewDocument)); + await flush(); + + const safe = container.querySelector( + 'a[href="https://example.com/evidence.png"]', + ); + expect(safe?.target).toBe('_blank'); + expect(safe?.rel).toBe('noopener noreferrer'); + expect(container.querySelector('a[href^="javascript:"]')).toBeNull(); + expect(container.textContent).toContain('javascript:alert(1) (not linked)'); + }); + + it('opens the Markdown report through workspace actions', async () => { + const { actions, container } = renderWith(JSON.stringify(reviewDocument)); + await flush(); + + const button = Array.from(container.querySelectorAll('button')).find( + (item) => item.textContent === 'Open Markdown report', + ); + await act(async () => { + button?.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + + expect(actions.readWorkspaceFile).toHaveBeenCalledWith( + '.qwen/reviews/pr-123.md', + ); + expect(container.textContent).toContain('Durable report'); + expect( + container.querySelector('a[href=".qwen/reviews/pr-123.md"]'), + ).toBeNull(); + }); + + it('ignores a stale Markdown response after switching artifacts', async () => { + let resolveReport: + | ((value: { content: string; truncated: false }) => void) + | undefined; + const nextDocument = { + ...reviewDocument, + target: 'PR #456', + verdict: { + ...reviewDocument.verdict, + verdictLine: 'Verdict: Approve — current artifact', + }, + }; + const actions = { + readWorkspaceFile: vi.fn((path: string) => { + if (path === '.qwen/reviews/pr-123.json') { + return Promise.resolve({ + content: JSON.stringify(reviewDocument), + truncated: false, + }); + } + if (path === '.qwen/reviews/pr-123.md') { + return new Promise<{ content: string; truncated: false }>( + (resolve) => { + resolveReport = resolve; + }, + ); + } + return Promise.resolve({ + content: JSON.stringify(nextDocument), + truncated: false, + }); + }), + } as unknown as DaemonWorkspaceActions; + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + mounted.push({ root, container }); + + act(() => { + root.render( + + + , + ); + }); + await flush(); + act(() => { + Array.from(container.querySelectorAll('button')) + .find((item) => item.textContent === 'Open Markdown report') + ?.click(); + }); + act(() => { + root.render( + + + , + ); + }); + await flush(); + await act(async () => { + resolveReport?.({ content: '# Stale report', truncated: false }); + await Promise.resolve(); + }); + + expect(container.textContent).toContain( + 'Verdict: Approve — current artifact', + ); + expect(container.textContent).not.toContain('Stale report'); + }); + + it.each([ + ['malformed JSON', '{not-json', 'Malformed code review JSON'], + [ + 'unsupported version', + JSON.stringify({ ...reviewDocument, schemaVersion: 2 }), + 'Unsupported code review schemaVersion', + ], + [ + 'invalid structure', + JSON.stringify({ ...reviewDocument, counts: { total: 2 } }), + 'counts.bySeverity must be an object', + ], + [ + 'traversing report path', + JSON.stringify({ + ...reviewDocument, + markdownReportPath: '../outside.md', + }), + 'markdownReportPath must be a relative .md path', + ], + [ + 'absolute report path', + JSON.stringify({ + ...reviewDocument, + markdownReportPath: '/etc/review.md', + }), + 'markdownReportPath must be a relative .md path', + ], + [ + 'non-Markdown report path', + JSON.stringify({ + ...reviewDocument, + markdownReportPath: '.qwen/reviews/report.json', + }), + 'markdownReportPath must be a relative .md path', + ], + [ + 'report path outside the reviews directory', + JSON.stringify({ + ...reviewDocument, + markdownReportPath: 'docs/report.md', + }), + 'markdownReportPath must be a file under .qwen/reviews/', + ], + ])('shows a dedicated error for %s', async (_name, content, expected) => { + const { container } = renderWith(content); + await flush(); + + expect(container.querySelector('[role="alert"]')?.textContent).toContain( + expected, + ); + expect(container.querySelector('.cm-editor')).toBeNull(); + }); + + it('resets filters when switching artifacts', async () => { + const firstDocument = JSON.stringify(reviewDocument); + const secondDocument = JSON.stringify({ + ...reviewDocument, + target: 'PR #456', + }); + const actions = { + readWorkspaceFile: vi.fn((path: string) => + Promise.resolve({ + content: path.endsWith('pr-123.json') + ? firstDocument + : secondDocument, + truncated: false, + }), + ), + } as unknown as DaemonWorkspaceActions; + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + mounted.push({ root, container }); + + act(() => { + root.render( + + + , + ); + }); + await flush(); + const severity = container.querySelector( + 'select[aria-label="Severity"]', + ); + await act(async () => { + if (severity) { + severity.value = 'Critical'; + severity.dispatchEvent(new Event('change', { bubbles: true })); + } + }); + expect(severity?.value).toBe('Critical'); + + act(() => { + root.render( + + + , + ); + }); + await flush(); + + // A filter left over from the previous artifact would show "no matches" + // next to a nonzero total; the switch resets both filters instead. + expect(container.textContent).toContain('PR #456'); + expect( + container.querySelector( + 'select[aria-label="Severity"]', + )?.value, + ).toBe('all'); + expect( + container.querySelector( + 'select[aria-label="Confidence"]', + )?.value, + ).toBe('all'); + }); + + it('renders local assetFiles as workspace images and bare paths', async () => { + const createdUrls: string[] = []; + Object.defineProperty(URL, 'createObjectURL', { + configurable: true, + value: vi.fn(() => { + const url = `blob:evidence-${createdUrls.length}`; + createdUrls.push(url); + return url; + }), + }); + Object.defineProperty(URL, 'revokeObjectURL', { + configurable: true, + value: vi.fn(), + }); + const assetDocument = { + ...reviewDocument, + findings: [ + { + ...reviewDocument.findings[2], + assetFiles: ['.qwen/tmp/shots/evidence.png', '.qwen/tmp/notes.txt'], + }, + ], + }; + const actions = { + readWorkspaceFile: vi.fn().mockResolvedValue({ + content: JSON.stringify(assetDocument), + truncated: false, + }), + readFileBytes: vi.fn().mockResolvedValue({ + contentBase64: btoa('img'), + offset: 0, + returnedBytes: 3, + sizeBytes: 3, + }), + stat: vi.fn().mockResolvedValue({ sizeBytes: 3, modifiedMs: 1 }), + } as unknown as DaemonWorkspaceActions; + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + mounted.push({ root, container }); + act(() => { + root.render( + + + , + ); + }); + await flush(); + + const image = container.querySelector( + 'img[alt=".qwen/tmp/shots/evidence.png"]', + ); + expect(image?.src).toBe('blob:evidence-0'); + expect(actions.readFileBytes).toHaveBeenCalledWith( + '.qwen/tmp/shots/evidence.png', + expect.objectContaining({ offset: 0 }), + ); + // A non-image assetFile stays a bare path and is never fetched. + expect(container.textContent).toContain('.qwen/tmp/notes.txt'); + expect(actions.readFileBytes).toHaveBeenCalledTimes(1); + }); + + it('offers the derived Markdown report when the artifact is invalid', async () => { + const { actions, container } = renderWith('{not-json'); + await flush(); + + expect(container.querySelector('[role="alert"]')?.textContent).toContain( + 'Malformed code review JSON', + ); + const fallback = Array.from(container.querySelectorAll('button')).find( + (item) => item.textContent === 'Open Markdown report', + ); + expect(fallback).not.toBeUndefined(); + await act(async () => { + fallback?.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + + expect(actions.readWorkspaceFile).toHaveBeenCalledWith( + '.qwen/reviews/pr-123.md', + ); + expect(container.textContent).toContain('Durable report'); + }); + + it('offers the derived Markdown report when the artifact is truncated', async () => { + const actions = { + readWorkspaceFile: vi.fn((path: string) => + Promise.resolve({ + content: path.endsWith('.md') ? '# Durable report' : 'partial', + truncated: !path.endsWith('.md'), + }), + ), + } as unknown as DaemonWorkspaceActions; + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + mounted.push({ root, container }); + act(() => { + root.render( + + + , + ); + }); + await flush(); + + expect(container.querySelector('[role="alert"]')?.textContent).toContain( + 'truncated', + ); + const fallback = Array.from(container.querySelectorAll('button')).find( + (item) => item.textContent === 'Open Markdown report', + ); + await act(async () => { + fallback?.click(); + await Promise.resolve(); + await Promise.resolve(); + }); + expect(container.textContent).toContain('Durable report'); + }); + + it('does not re-read the artifact or reset filters on a language switch', async () => { + const actions = { + readWorkspaceFile: vi.fn().mockResolvedValue({ + content: JSON.stringify(reviewDocument), + truncated: false, + }), + } as unknown as DaemonWorkspaceActions; + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + mounted.push({ root, container }); + const renderAt = (language: 'en' | 'zh-CN') => + act(() => { + root.render( + + + , + ); + }); + + renderAt('en'); + await flush(); + const severity = container.querySelector( + 'select[aria-label="Severity"]', + ); + await act(async () => { + if (severity) { + severity.value = 'Critical'; + severity.dispatchEvent(new Event('change', { bubbles: true })); + } + }); + expect(severity?.value).toBe('Critical'); + + renderAt('zh-CN'); + await flush(); + + expect(actions.readWorkspaceFile).toHaveBeenCalledTimes(1); + expect(container.textContent).toContain('严重级别'); + expect( + container.querySelector( + 'select[aria-label="严重级别"]', + )?.value, + ).toBe('Critical'); + }); + + it('fails closed for truncated input', async () => { + const actions = { + readWorkspaceFile: vi.fn().mockResolvedValue({ + content: JSON.stringify(reviewDocument), + truncated: true, + }), + } as unknown as DaemonWorkspaceActions; + const container = document.createElement('div'); + document.body.appendChild(container); + const root = createRoot(container); + mounted.push({ root, container }); + act(() => { + root.render( + + + , + ); + }); + await flush(); + + expect(container.querySelector('[role="alert"]')?.textContent).toContain( + 'truncated', + ); + }); +}); diff --git a/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx b/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx new file mode 100644 index 00000000000..15c83b56f27 --- /dev/null +++ b/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx @@ -0,0 +1,728 @@ +import type { DaemonWorkspaceActions } from '@qwen-code/webui/daemon-react-sdk'; +import { useEffect, useMemo, useRef, useState } from 'react'; +import { useI18n } from '../../i18n'; +import { isSafeHref, Markdown } from '../messages/Markdown'; +import { + getImageMimeTypeFromPath, + readWorkspaceFileAsBlob, +} from './artifactUtils'; +import styles from './CodeReviewArtifactDetail.module.css'; + +// Hand-duplicated from the CLI's canonical lists in +// packages/cli/src/commands/review/findings.ts. The parser below fails closed +// on any value missing here, so when the CLI adds one, update this copy and +// the contract fixture (__fixtures__/code-review-artifact-v1.json) with it. +// The CLI-side vocabulary snapshot (packages/cli/src/commands/review/ +// findings.test.ts) turns red on that change and names this file. +const SEVERITIES = ['Critical', 'Suggestion', 'Nice to have'] as const; +const CONFIDENCES = ['high', 'low'] as const; +const SOURCES = ['review', 'build', 'test', 'probe', 'lint'] as const; +const OUTCOMES = ['fixed', 'skipped', 'no_change_needed'] as const; + +type Severity = (typeof SEVERITIES)[number]; +type Confidence = (typeof CONFIDENCES)[number]; +type Source = (typeof SOURCES)[number]; +type Outcome = (typeof OUTCOMES)[number]; + +interface FindingLocation { + file: string; + line?: number; + anchor?: string; +} + +interface ReviewFinding { + id: string; + severity: Severity; + confidence: Confidence; + source: Source; + summary: string; + shortSummary: string; + failureScenario: string; + suggestedFix?: string; + category?: string; + locations: FindingLocation[]; + /** Local evidence paths; rendered as workspace images where possible. */ + assetFiles?: string[]; + assets?: string[]; + outcome?: Outcome; + outcomeNote?: string; + heldByMeasurement?: { file: string }; +} + +interface ReviewCounts { + total: number; + bySeverity: Record; + byConfidence: Record; + held: number; +} + +// Only fields the renderer displays are validated: every required field here +// can fail the whole document, so the fail-closed surface stays no larger +// than the visible one. Fields the CLI persists but this view ignores +// (verdict.downgraded, verdict.body, remediation, outcomesRecorded, +// counts.byOutcome, ...) are deliberately absent and pass through unchecked. +interface CodeReviewDocument { + schemaVersion: 1; + target: string; + effort: string; + verdict: { + event: 'APPROVE' | 'COMMENT' | 'REQUEST_CHANGES'; + verdictLine: string; + baseEvent: 'APPROVE' | 'COMMENT' | 'REQUEST_CHANGES'; + cappedBy: string[]; + }; + findings: ReviewFinding[]; + counts: ReviewCounts; + markdownReportPath: string; +} + +function object(value: unknown, label: string): Record { + if (value === null || typeof value !== 'object' || Array.isArray(value)) { + throw new Error(`${label} must be an object.`); + } + return value as Record; +} + +function string(value: unknown, label: string): string { + if (typeof value !== 'string' || value.trim() === '') { + throw new Error(`${label} must be a non-empty string.`); + } + return value; +} + +function optionalString(value: unknown, label: string): string | undefined { + if (value === undefined) return undefined; + return string(value, label); +} + +function integer(value: unknown, label: string): number { + if (typeof value !== 'number' || !Number.isSafeInteger(value) || value < 0) { + throw new Error(`${label} must be a non-negative safe integer.`); + } + return value; +} + +function lineNumber(value: unknown, label: string): number { + const line = integer(value, label); + if (line === 0) { + throw new Error(`${label} must be a positive safe integer.`); + } + return line; +} + +// The one field that becomes a file read. The daemon enforces workspace +// containment, but the document's own contract is checked here like every +// other field: relative, no traversal, the durable report's extension, and +// the directory the writer guarantees — save-artifact confines its report to +// .qwen/reviews/, so the renderer never follows a report the CLI would not +// have written. +function markdownReportPath(value: unknown): string { + const path = string(value, 'markdownReportPath'); + if ( + path.startsWith('/') || + path.split('/').includes('..') || + !path.endsWith('.md') + ) { + throw new Error( + 'markdownReportPath must be a relative .md path without ".." segments.', + ); + } + if (!path.startsWith('.qwen/reviews/')) { + throw new Error('markdownReportPath must be a file under .qwen/reviews/.'); + } + return path; +} + +function enumValue( + value: unknown, + allowed: readonly T[], + label: string, +): T { + if (typeof value !== 'string' || !allowed.includes(value as T)) { + throw new Error(`${label} has an unsupported value.`); + } + return value as T; +} + +function stringArray(value: unknown, label: string): string[] { + if (!Array.isArray(value)) throw new Error(`${label} must be an array.`); + return value.map((entry, index) => string(entry, `${label}[${index}]`)); +} + +function countRecord( + value: unknown, + keys: readonly T[], + label: string, +): Record { + const source = object(value, label); + return Object.fromEntries( + keys.map((key) => [key, integer(source[key], `${label}.${key}`)]), + ) as Record; +} + +function parseFinding(value: unknown, index: number): ReviewFinding { + const label = `findings[${index}]`; + const source = object(value, label); + if (!Array.isArray(source['locations']) || source['locations'].length === 0) { + throw new Error(`${label}.locations must be a non-empty array.`); + } + const locations = source['locations'].map((entry, locationIndex) => { + const location = object(entry, `${label}.locations[${locationIndex}]`); + return { + file: string( + location['file'], + `${label}.locations[${locationIndex}].file`, + ), + ...(location['line'] === undefined + ? {} + : { + line: lineNumber( + location['line'], + `${label}.locations[${locationIndex}].line`, + ), + }), + ...(optionalString( + location['anchor'], + `${label}.locations[${locationIndex}].anchor`, + ) + ? { anchor: location['anchor'] as string } + : {}), + }; + }); + const outcome = + source['outcome'] === undefined + ? undefined + : enumValue(source['outcome'], OUTCOMES, `${label}.outcome`); + return { + id: string(source['id'], `${label}.id`), + severity: enumValue(source['severity'], SEVERITIES, `${label}.severity`), + confidence: enumValue( + source['confidence'], + CONFIDENCES, + `${label}.confidence`, + ), + source: enumValue(source['source'], SOURCES, `${label}.source`), + summary: string(source['summary'], `${label}.summary`), + shortSummary: string(source['shortSummary'], `${label}.shortSummary`), + failureScenario: string( + source['failureScenario'], + `${label}.failureScenario`, + ), + ...(optionalString(source['suggestedFix'], `${label}.suggestedFix`) + ? { suggestedFix: source['suggestedFix'] as string } + : {}), + ...(optionalString(source['category'], `${label}.category`) + ? { category: source['category'] as string } + : {}), + locations, + ...(source['assetFiles'] === undefined + ? {} + : { + assetFiles: stringArray(source['assetFiles'], `${label}.assetFiles`), + }), + ...(source['assets'] === undefined + ? {} + : { assets: stringArray(source['assets'], `${label}.assets`) }), + ...(outcome ? { outcome } : {}), + ...(optionalString(source['outcomeNote'], `${label}.outcomeNote`) + ? { outcomeNote: source['outcomeNote'] as string } + : {}), + ...(source['heldByMeasurement'] === undefined + ? {} + : { + heldByMeasurement: { + file: string( + object(source['heldByMeasurement'], `${label}.heldByMeasurement`)[ + 'file' + ], + `${label}.heldByMeasurement.file`, + ), + }, + }), + }; +} + +export function parseCodeReviewDocument(content: string): CodeReviewDocument { + let parsed: unknown; + try { + parsed = JSON.parse(content); + } catch (error) { + throw new Error( + `Malformed code review JSON: ${error instanceof Error ? error.message : String(error)}`, + ); + } + const root = object(parsed, 'Code review document'); + if (root['schemaVersion'] !== 1) { + throw new Error( + `Unsupported code review schemaVersion: ${JSON.stringify(root['schemaVersion'])}. Expected 1.`, + ); + } + if (!Array.isArray(root['findings'])) { + throw new Error('findings must be an array.'); + } + const verdict = object(root['verdict'], 'verdict'); + const counts = object(root['counts'], 'counts'); + return { + schemaVersion: 1, + target: string(root['target'], 'target'), + effort: string(root['effort'], 'effort'), + verdict: { + event: enumValue( + verdict['event'], + ['APPROVE', 'COMMENT', 'REQUEST_CHANGES'], + 'verdict.event', + ), + verdictLine: string(verdict['verdictLine'], 'verdict.verdictLine'), + baseEvent: enumValue( + verdict['baseEvent'], + ['APPROVE', 'COMMENT', 'REQUEST_CHANGES'], + 'verdict.baseEvent', + ), + cappedBy: stringArray(verdict['cappedBy'], 'verdict.cappedBy'), + }, + findings: root['findings'].map(parseFinding), + counts: { + total: integer(counts['total'], 'counts.total'), + bySeverity: countRecord( + counts['bySeverity'], + SEVERITIES, + 'counts.bySeverity', + ), + byConfidence: countRecord( + counts['byConfidence'], + CONFIDENCES, + 'counts.byConfidence', + ), + held: integer(counts['held'], 'counts.held'), + }, + markdownReportPath: markdownReportPath(root['markdownReportPath']), + }; +} + +export function CodeReviewArtifactDetail({ + workspacePath, + artifactVersion, + workspaceActions, +}: { + workspacePath: string; + artifactVersion?: string; + workspaceActions: DaemonWorkspaceActions; +}) { + const { t } = useI18n(); + const [content, setContent] = useState(null); + const [loadError, setLoadError] = useState(null); + const [artifactTruncated, setArtifactTruncated] = useState(false); + const [severity, setSeverity] = useState<'all' | Severity>('all'); + const [confidence, setConfidence] = useState<'all' | Confidence>('all'); + const [reportPath, setReportPath] = useState(null); + const [reportContent, setReportContent] = useState(null); + const [reportError, setReportError] = useState(null); + const [reportLoading, setReportLoading] = useState(false); + const reportRequest = useRef(0); + + // `t` is deliberately NOT a dependency: i18n memoizes it per language, and + // including it would re-read the artifact and reset every filter on a + // language switch. The truncated message is resolved at render time. + useEffect(() => { + let cancelled = false; + reportRequest.current += 1; + setContent(null); + setLoadError(null); + setArtifactTruncated(false); + setSeverity('all'); + setConfidence('all'); + setReportPath(null); + setReportContent(null); + setReportError(null); + setReportLoading(false); + workspaceActions + .readWorkspaceFile(workspacePath) + .then((file) => { + if (cancelled) return; + if (file.truncated) { + setArtifactTruncated(true); + return; + } + setContent(file.content); + }) + .catch((error: unknown) => { + if (!cancelled) { + setLoadError(error instanceof Error ? error.message : String(error)); + } + }); + return () => { + cancelled = true; + reportRequest.current += 1; + }; + }, [artifactVersion, workspaceActions, workspacePath]); + + const parsed = useMemo(() => { + if (content === null) return null; + try { + return { document: parseCodeReviewDocument(content), error: null }; + } catch (error) { + return { + document: null, + error: error instanceof Error ? error.message : String(error), + }; + } + }, [content]); + + const openReport = (path: string) => { + const request = ++reportRequest.current; + setReportPath(path); + setReportContent(null); + setReportError(null); + setReportLoading(true); + workspaceActions + .readWorkspaceFile(path) + .then((file) => { + if (request !== reportRequest.current) return; + if (file.truncated) { + setReportError(t('codeReview.reportTruncated')); + } else { + setReportContent(file.content); + } + }) + .catch((error: unknown) => { + if (request !== reportRequest.current) return; + setReportError(error instanceof Error ? error.message : String(error)); + }) + .finally(() => { + if (request === reportRequest.current) setReportLoading(false); + }); + }; + + if (reportContent !== null || reportError) { + return ( +
+ +
{reportPath}
+ {reportError ? ( +
+ {reportError} +
+ ) : ( + + )} +
+ ); + } + + const loadFailure = artifactTruncated + ? t('codeReview.artifactTruncated') + : loadError; + if (loadFailure || parsed?.error) { + // The document that names the durable report failed to load, so derive + // the report from the artifact path by the same-stem convention + // save-artifact is invoked with: a truncated or unparsable artifact must + // not become a dead end when its Markdown report is still readable. + const fallbackReport = workspacePath.endsWith('.json') + ? `${workspacePath.slice(0, -'.json'.length)}.md` + : null; + return ( +
+
+ {t('codeReview.loadErrorTitle')} + {loadFailure ?? parsed?.error} +
+ {fallbackReport && ( + + )} +
+ ); + } + if (!parsed?.document) { + return
{t('codeReview.loading')}
; + } + + const reviewDocument = parsed.document; + const findings = reviewDocument.findings.filter( + (finding) => + (severity === 'all' || finding.severity === severity) && + (confidence === 'all' || finding.confidence === confidence), + ); + + return ( +
+
+
+
+ {t('codeReview.authoritativeVerdict')} +
+

+ {reviewDocument.verdict.verdictLine} +

+
+ {t('codeReview.targetEffort', { + target: reviewDocument.target, + effort: reviewDocument.effort, + })} +
+
+ +
+ +
+ + {SEVERITIES.map((value) => ( + + ))} + {CONFIDENCES.map((value) => ( + + ))} + +
+ +
+ {t('codeReview.caps')} + + {reviewDocument.verdict.cappedBy.length > 0 + ? reviewDocument.verdict.cappedBy.join(', ') + : t('codeReview.none')} + +
+ +
+ + +
+ +
+ {findings.length === 0 ? ( +
{t('codeReview.noMatches')}
+ ) : ( + findings.map((finding) => ( +
+
+ {finding.severity} + + {t('codeReview.confidence', { + value: finding.confidence, + })} + + {t('codeReview.source', { value: finding.source })} + {finding.category && {finding.category}} +
+

{finding.summary}

+ + {finding.suggestedFix && ( + + )} + {finding.outcome && ( + + )} + {finding.heldByMeasurement && ( + + )} +
+ {t('codeReview.locations')} +
    + {finding.locations.map((location, index) => ( +
  • + + {location.file} + {location.line === undefined ? '' : `:${location.line}`} + + {location.anchor && — {location.anchor}} +
    • + ))} +
+
+ {((finding.assets && finding.assets.length > 0) || + (finding.assetFiles && finding.assetFiles.length > 0)) && ( +
+ {t('codeReview.evidence')} +
    + {(finding.assets ?? []).map((asset, index) => ( +
  • + {isSafeHref(asset) ? ( + + {asset} + + ) : ( + + {asset} ({t('codeReview.notLinked')}) + + )} +
    • + ))} + {(finding.assetFiles ?? []).map((file, index) => ( + + ))} +
+
+ )} +
+ )) + )} +
+
+ ); +} + +// A local evidence file (`assetFiles`): an inline image when the workspace +// file is readable, the bare path when it is not. Local evidence usually +// lives under `.qwen/tmp/`, which review cleanup removes, so a path that no +// longer resolves is an ordinary state — the caption stays either way. +function EvidenceFile({ + path, + workspaceActions, +}: { + path: string; + workspaceActions: DaemonWorkspaceActions; +}) { + const mimeType = getImageMimeTypeFromPath(path); + const [src, setSrc] = useState(null); + + useEffect(() => { + if (!mimeType) return; + let cancelled = false; + let objectUrl: string | undefined; + readWorkspaceFileAsBlob( + (filePath, opts) => workspaceActions.readFileBytes(filePath, opts), + path, + mimeType, + { + statFile: (filePath) => workspaceActions.stat(filePath), + isCancelled: () => cancelled, + }, + ) + .then((blob) => { + if (cancelled) return; + objectUrl = URL.createObjectURL(blob); + setSrc(objectUrl); + }) + .catch(() => {}); + return () => { + cancelled = true; + if (objectUrl) URL.revokeObjectURL(objectUrl); + }; + }, [mimeType, path, workspaceActions]); + + return ( +
  • + {src && {path}} + {path} +
    • + ); +} + +function Metric({ label, value }: { label: string; value: number }) { + return ( +
    + {value} + {label} +
    + ); +} + +function Detail({ label, value }: { label: string; value: string }) { + return ( +
    + {label} +

    {value}

    +
    + ); +} diff --git a/packages/web-shell/client/components/artifacts/__fixtures__/code-review-artifact-v1.json b/packages/web-shell/client/components/artifacts/__fixtures__/code-review-artifact-v1.json new file mode 100644 index 00000000000..6573a7726db --- /dev/null +++ b/packages/web-shell/client/components/artifacts/__fixtures__/code-review-artifact-v1.json @@ -0,0 +1,142 @@ +{ + "schemaVersion": 1, + "target": "pr-8402", + "effort": "high", + "verdict": { + "event": "COMMENT", + "body": "Two findings remain open after the fix round.", + "baseEvent": "REQUEST_CHANGES", + "cappedBy": ["no release-blocker evidence"], + "downgraded": true, + "downgradedFrom": "Request changes", + "remediation": ["Address the skipped lint finding or record why it stays."], + "lowSignal": { + "agents": 3, + "srcDiffLines": 87 + }, + "verdictLine": "Verdict: Comment — Request changes was downgraded" + }, + "findings": [ + { + "id": "f-critical-review", + "severity": "Critical", + "confidence": "high", + "source": "review", + "summary": "A workspace write can land outside the reviewed workspace.", + "shortSummary": "Cross-workspace write", + "failureScenario": "Open workspace B, then submit a write queued from workspace A; the file lands in B.", + "suggestedFix": "Resolve the owning runtime before the write.", + "category": "security", + "locations": [ + { + "file": "packages/core/src/runtime/write.ts", + "line": 128, + "anchor": "writeFile(target, body);" + } + ], + "assets": [ + "https://assets.example.com/reviews/pr-8402/f-critical-review.png" + ], + "outcome": "fixed", + "outcomeNote": "Writes now resolve the runtime first." + }, + { + "id": "f-suggestion-build", + "severity": "Suggestion", + "confidence": "high", + "source": "build", + "summary": "The bundle step emits a deprecation warning for the banner plugin.", + "shortSummary": "The bundle step emits a deprecation warning for the banner…", + "failureScenario": "npm run bundle prints the plugin deprecation on every build, masking new warnings.", + "suggestedFix": "Pin the plugin option the warning names.", + "locations": [ + { + "file": "esbuild.config.js", + "line": 31 + } + ], + "outcome": "fixed" + }, + { + "id": "f-suggestion-test-held", + "severity": "Suggestion", + "confidence": "high", + "source": "test", + "summary": "Pagination test fails on the PR build.", + "shortSummary": "Pagination test fails on the PR build.", + "failureScenario": "vitest reports the pagination spec red on the PR side; the same command is red on the merge base too.", + "locations": [ + { + "file": "packages/cli/src/ui/pagination.ts", + "line": 77 + } + ], + "heldByMeasurement": { + "file": "packages/cli/src/ui/pagination.test.ts" + }, + "outcome": "no_change_needed", + "outcomeNote": "Red on the merge base as well; not caused by this change." + }, + { + "id": "f-suggestion-lint", + "severity": "Suggestion", + "confidence": "low", + "source": "lint", + "summary": "Evidence links render the full URL and overflow narrow panels.", + "shortSummary": "Evidence links render the full URL and overflow narrow…", + "failureScenario": "A long asset URL pushes the finding card past the panel width.", + "category": "ui", + "locations": [ + { + "file": "packages/web-shell/client/components/artifacts/ArtifactPanel.tsx", + "line": 540 + }, + { + "file": "packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx", + "line": 421 + } + ], + "assets": [ + "https://assets.example.com/reviews/pr-8402/f-suggestion-lint.png" + ], + "outcome": "skipped", + "outcomeNote": "Outside the reviewed diff." + }, + { + "id": "f-nice-probe", + "severity": "Nice to have", + "confidence": "high", + "source": "probe", + "summary": "The empty-state copy could be one line shorter.", + "shortSummary": "The empty-state copy could be one line shorter.", + "failureScenario": "The panel spends two lines on a state that carries no information yet.", + "locations": [ + { + "file": "packages/web-shell/client/components/artifacts/ArtifactPanel.tsx" + } + ], + "outcome": "no_change_needed", + "outcomeNote": "Left as-is; copy review disagreed." + } + ], + "counts": { + "total": 5, + "bySeverity": { + "Critical": 1, + "Suggestion": 3, + "Nice to have": 1 + }, + "byConfidence": { + "high": 4, + "low": 1 + }, + "held": 1, + "byOutcome": { + "fixed": 2, + "skipped": 1, + "no_change_needed": 2 + } + }, + "outcomesRecorded": true, + "markdownReportPath": ".qwen/reviews/contract-v1.md" +} diff --git a/packages/web-shell/client/components/artifacts/codeReviewContract.test.ts b/packages/web-shell/client/components/artifacts/codeReviewContract.test.ts new file mode 100644 index 00000000000..aee72f6b9e4 --- /dev/null +++ b/packages/web-shell/client/components/artifacts/codeReviewContract.test.ts @@ -0,0 +1,97 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The CLI/renderer contract: the fixture is genuine `qwen review save-artifact` +// output, generated through `buildReport(validateFindings(...))` — not a +// hand-written copy. If the CLI's document shape or vocabulary changes, +// regenerate the fixture and bring this parser along. + +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { describe, expect, it } from 'vitest'; +import { parseCodeReviewDocument } from './CodeReviewArtifactDetail'; + +const fixture = readFileSync( + fileURLToPath( + new URL('./__fixtures__/code-review-artifact-v1.json', import.meta.url), + ), + 'utf8', +); + +describe('code review artifact contract', () => { + it('parses the CLI-generated document without losing fields', () => { + const reviewDocument = parseCodeReviewDocument(fixture); + + expect(reviewDocument.schemaVersion).toBe(1); + expect(reviewDocument.target).toBe('pr-8402'); + expect(reviewDocument.effort).toBe('high'); + // The parser validates only what the renderer displays; persisted-but- + // unrendered fields (downgraded, outcomesRecorded, byOutcome, ...) pass + // through unchecked and are asserted nowhere on purpose. + expect(reviewDocument.verdict).toMatchObject({ + event: 'COMMENT', + baseEvent: 'REQUEST_CHANGES', + cappedBy: ['no release-blocker evidence'], + verdictLine: 'Verdict: Comment — Request changes was downgraded', + }); + expect(reviewDocument.markdownReportPath).toBe( + '.qwen/reviews/contract-v1.md', + ); + + // The fixture exercises the whole vocabulary — every source, severity, + // confidence and outcome the CLI can canonicalize — so this parse proves + // the renderer accepts all of it. + const findings = reviewDocument.findings; + expect(findings.map((finding) => finding.id)).toEqual([ + 'f-critical-review', + 'f-suggestion-build', + 'f-suggestion-test-held', + 'f-suggestion-lint', + 'f-nice-probe', + ]); + expect(new Set(findings.map((finding) => finding.source))).toEqual( + new Set(['review', 'build', 'test', 'probe', 'lint']), + ); + expect(new Set(findings.map((finding) => finding.severity))).toEqual( + new Set(['Critical', 'Suggestion', 'Nice to have']), + ); + expect(new Set(findings.map((finding) => finding.confidence))).toEqual( + new Set(['high', 'low']), + ); + expect(new Set(findings.map((finding) => finding.outcome))).toEqual( + new Set(['fixed', 'skipped', 'no_change_needed']), + ); + + expect(reviewDocument.counts).toEqual({ + total: 5, + bySeverity: { Critical: 1, Suggestion: 3, 'Nice to have': 1 }, + byConfidence: { high: 4, low: 1 }, + held: 1, + }); + + const held = findings.find((f) => f.id === 'f-suggestion-test-held'); + expect(held?.heldByMeasurement).toEqual({ + file: 'packages/cli/src/ui/pagination.test.ts', + }); + + const aggregate = findings.find((f) => f.id === 'f-suggestion-lint'); + expect(aggregate?.locations).toHaveLength(2); + }); + + it('fails closed on a vocabulary value the renderer does not know yet', () => { + // The drift mode the contract guards: the CLI adds a value the renderer + // copy has not been taught, and documents carrying it refuse to render + // until the renderer is updated with it. + const drifted = JSON.parse(fixture) as { + findings: Array<{ source: string }>; + }; + drifted.findings[0]!.source = 'typecheck'; + + expect(() => parseCodeReviewDocument(JSON.stringify(drifted))).toThrow( + 'findings[0].source has an unsupported value.', + ); + }); +}); diff --git a/packages/web-shell/client/e2e/utils/mockDaemon.ts b/packages/web-shell/client/e2e/utils/mockDaemon.ts index 7f63408ad4c..ccb62da095b 100644 --- a/packages/web-shell/client/e2e/utils/mockDaemon.ts +++ b/packages/web-shell/client/e2e/utils/mockDaemon.ts @@ -9,11 +9,14 @@ import { type DaemonEvent, type DaemonRestoredSession, type DaemonSession, + type DaemonSessionArtifact, + type DaemonSessionArtifactsEnvelope, type DaemonSessionGroup, type DaemonSessionGroupCatalog, type DaemonSessionState, type DaemonSessionSummary, type DaemonWorkspaceExtensionsStatus, + type DaemonWorkspaceFile, type DaemonGitHubPullRequestList, type DaemonWorkspaceGitStatus, type DaemonWorkspaceMcpResourcesStatus, @@ -61,6 +64,10 @@ export interface WebShellDaemonScenario { sessionGroups: DaemonSessionGroup[]; events: DaemonEvent[]; state: DaemonSessionState; + /** Artifact list returned by `GET /session/:id/artifacts`. */ + artifacts: DaemonSessionArtifact[]; + /** File contents served by `GET /file?path=...`, keyed by requested path. */ + workspaceFiles: Record; /** * Response for `GET /workspaces/:cwd/git`. Defaults to a null-branch status * (non-git workspace), matching the real daemon's graceful degradation. @@ -343,6 +350,8 @@ export function createWebShellDaemonScenario( sessionGroups: overrides.sessionGroups ?? [], events: overrides.events ?? [], state, + artifacts: overrides.artifacts ?? [], + workspaceFiles: overrides.workspaceFiles ?? {}, gitStatus: overrides.gitStatus, gitHubPrs: overrides.gitHubPrs, gitBranches: overrides.gitBranches, @@ -564,6 +573,8 @@ function isDaemonPath(path: string): boolean { /^\/workspaces\/.+\/github\/(prs\/create|default-branch)\/?$/.test(path) || /^\/workspace\/github\/(prs\/create|default-branch)\/?$/.test(path) || path === '/session' || + /^\/file\/?$/.test(path) || + /^\/session\/[^/]+\/artifacts\/?$/.test(path) || /^\/permission\/[^/]+\/?$/.test(path) || /^\/session\/[^/]+\/pending-prompts(?:\/[^/]+)?\/?$/.test(path) || /^\/session\/[^/]+\/(load|resume|prompt|permission\/[^/]+|context|supported-commands|events|model|approval-mode|heartbeat|cancel|detach|btw)\/?$/.test( @@ -683,6 +694,10 @@ function isDaemonRoute(method: string, path: string): boolean { if (method === 'POST' && /^\/workspace\/github\/prs\/create\/?$/.test(path)) return true; if (method === 'POST' && /^\/session\/[^/]+\/btw\/?$/.test(path)) return true; + if (method === 'GET' && /^\/file\/?$/.test(path)) return true; + if (method === 'GET' && /^\/session\/[^/]+\/artifacts\/?$/.test(path)) { + return true; + } if (method === 'POST' && path === '/session') return true; if (method === 'POST' && /^\/permission\/[^/]+\/?$/.test(path)) return true; if ( @@ -784,6 +799,16 @@ async function handleDaemonRoute( await json(route, workspaceVoice(scenario)); return; } + if (method === 'GET' && /^\/file\/?$/.test(path)) { + const filePath = searchParams.get('path') ?? ''; + const content = scenario.workspaceFiles[filePath]; + if (content === undefined) { + await json(route, { error: `No such file: ${filePath}` }, 404); + return; + } + await json(route, workspaceFile(filePath, content)); + return; + } if (method === 'GET' && /^\/workspaces\/[^/]+\/voice\/?$/.test(path)) { await json(route, workspaceVoice(scenario)); return; @@ -1187,6 +1212,10 @@ async function handleDaemonRoute( await json(route, restoredSessionEnvelope(scenario, sessionId)); return; } + if (action === 'artifacts') { + await json(route, sessionArtifactsEnvelope(scenario, sessionId)); + return; + } if (action === 'prompt') { if (!isPromptRequest(body)) { await badRequest(route, 'Invalid prompt request.'); @@ -1456,6 +1485,36 @@ function workspaceVoice( return scenario.voice; } +function sessionArtifactsEnvelope( + scenario: WebShellDaemonScenario, + sessionId: string, +): DaemonSessionArtifactsEnvelope { + return { + v: 1, + sessionId, + artifacts: scenario.artifacts, + generatedAt: now, + limits: { maxArtifacts: 100 }, + }; +} + +function workspaceFile(path: string, content: string): DaemonWorkspaceFile { + const sizeBytes = new TextEncoder().encode(content).byteLength; + return { + kind: 'file', + path, + content, + encoding: 'utf-8', + bom: false, + lineEnding: 'lf', + sizeBytes, + returnedBytes: sizeBytes, + truncated: false, + matchedIgnore: null, + originalLineCount: null, + }; +} + async function json(route: Route, body: unknown, status = 200): Promise { await route.fulfill({ status, diff --git a/packages/web-shell/client/e2e/visuals/screenshots.spec.ts b/packages/web-shell/client/e2e/visuals/screenshots.spec.ts index 9b5ad189b64..98f29e1e28f 100644 --- a/packages/web-shell/client/e2e/visuals/screenshots.spec.ts +++ b/packages/web-shell/client/e2e/visuals/screenshots.spec.ts @@ -699,5 +699,162 @@ for (const theme of THEMES) { ).toBeVisible(); await captureScreenshot(page, `permission-panel-${theme}`); }); + + test(`code review artifact`, async ({ page }, testInfo) => { + // The dedicated code-review renderer is gated three ways: the + // `session_artifacts` capability, an artifact whose metadata marks it + // `code_review`, and a readable workspace file behind it. Without a + // scenario seeding all three, the whole detail view stays invisible to + // the before/after preview. + const reviewPath = '.qwen/reviews/pr-1234.json'; + const reviewDocument = { + schemaVersion: 1, + target: 'local', + effort: 'high', + verdict: { + event: 'REQUEST_CHANGES', + verdictLine: 'Verdict: Request changes (1 Critical, 1 Suggestion)', + baseEvent: 'REQUEST_CHANGES', + cappedBy: ['Critical finding f-critical is unresolved'], + downgraded: false, + downgradedFrom: null, + }, + findings: [ + { + id: 'f-critical', + severity: 'Critical', + confidence: 'high', + source: 'review', + summary: + 'Review verdict is reported even when the child process times out', + shortSummary: 'timeout treated as success', + failureScenario: + 'When `review run` times out, the CLI still prints a verdict as if the review completed.', + suggestedFix: + 'Fail closed when timedOut is true instead of reporting the verdict.', + category: 'correctness', + locations: [ + { file: 'packages/cli/src/commands/review.ts', line: 412 }, + ], + outcome: 'fixed', + outcomeNote: 'Timeouts now surface as incomplete.', + }, + { + id: 'f-suggestion', + severity: 'Suggestion', + confidence: 'low', + source: 'lint', + summary: + 'Artifact evidence links should render their file name only', + shortSummary: 'verbose evidence labels', + failureScenario: + 'Long asset URLs overflow the finding card in narrow panels.', + locations: [ + { + file: 'packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx', + line: 540, + }, + ], + assets: [ + 'https://assets.example.com/reviews/pr-1234/f-suggestion.png', + ], + }, + ], + counts: { + total: 2, + bySeverity: { Critical: 1, Suggestion: 1, 'Nice to have': 0 }, + byConfidence: { high: 1, low: 1 }, + byOutcome: { fixed: 1, skipped: 0, no_change_needed: 0 }, + held: 0, + }, + outcomesRecorded: true, + markdownReportPath: '.qwen/reviews/pr-1234.md', + }; + const reviewDocumentJson = JSON.stringify(reviewDocument); + const scenario = createWebShellDaemonScenario({ + capabilities: { + features: [ + 'session_events', + 'permission_vote', + 'session_permission_vote', + 'session_scope_override', + 'session_source_metadata', + 'workspace_settings', + 'workspace_voice', + 'session_artifacts', + ], + }, + events: [ + userTextEvent('Review my changes and save the report.', { id: 1 }), + { + id: 2, + v: 1, + type: 'session_update', + data: { + update: { + sessionUpdate: 'tool_call', + toolCallId: 'call-record-review', + toolName: 'record_artifact', + title: 'record_artifact', + kind: 'other', + status: 'completed', + rawInput: { + title: 'Code review result', + workspacePath: reviewPath, + }, + rawOutput: { recorded: true }, + }, + }, + }, + assistantTextEvent('Review saved to the workspace.', { id: 3 }), + turnCompleteEvent('prompt-review', { id: 4 }), + ], + artifacts: [ + { + id: 'artifact-code-review', + kind: 'other', + storage: 'workspace', + source: 'tool', + status: 'available', + title: 'Code review result', + workspacePath: reviewPath, + mimeType: 'application/json', + sizeBytes: reviewDocumentJson.length, + metadata: { artifactType: 'code_review', schemaVersion: 1 }, + retention: 'restorable', + clientRetained: false, + createdAt: '2026-07-03T00:00:00.000Z', + updatedAt: '2026-07-03T00:00:00.000Z', + toolCallId: 'call-record-review', + toolName: 'record_artifact', + }, + ], + workspaceFiles: { [reviewPath]: reviewDocumentJson }, + }); + const daemon = await installScenario( + page, + scenario, + resolveBaseURL(testInfo), + ); + await gotoSession(page, scenario, daemon, theme); + + // Open the artifact card the turn outputs render for the recorded + // artifact; the right panel then loads the workspace file and renders + // the dedicated detail view instead of the generic file preview. + await page + .locator('[data-web-shell-message-list]') + .getByRole('button', { name: 'Open', exact: true }) + .click(); + await expect(page.getByText('Authoritative verdict')).toBeVisible(); + await expect( + page.getByText('Verdict: Request changes (1 Critical, 1 Suggestion)'), + ).toBeVisible(); + await expect( + page.getByText( + 'Review verdict is reported even when the child process times out', + ), + ).toBeVisible(); + await captureScreenshot(page, `code-review-artifact-${theme}`); + }); }); } diff --git a/packages/web-shell/client/i18n.tsx b/packages/web-shell/client/i18n.tsx index 825dfaf179c..063848f1112 100644 --- a/packages/web-shell/client/i18n.tsx +++ b/packages/web-shell/client/i18n.tsx @@ -1047,8 +1047,43 @@ const EN: Messages = { 'scheduledTasks.condition.cardPrefix': 'If:', 'turnOutputs.filesEdited': (v) => `Edited ${v?.count ?? 0} files`, 'turnOutputs.viewChanges': 'View changes', - 'turnOutputs.review': 'Review', + // Key keeps its historical `review` name; the label moved to "Changes" when + // the code-review result view (`codeReview.*`) took over the word. + 'turnOutputs.review': 'Changes', 'turnOutputs.reviewLatest': 'View recent file changes', + 'codeReview.authoritativeVerdict': 'Authoritative verdict', + 'codeReview.targetEffort': (v) => + `Target: ${v?.target ?? ''} · Effort: ${v?.effort ?? ''}`, + 'codeReview.openReport': 'Open Markdown report', + 'codeReview.loadingReport': 'Loading Markdown report...', + 'codeReview.back': 'Back to review', + 'codeReview.reviewCounts': 'Review counts', + 'codeReview.total': 'Total', + 'codeReview.confidence': (v) => `${v?.value ?? ''} confidence`, + 'codeReview.held': 'Held', + 'codeReview.heldByMeasurement': 'Held back from Critical by measurement', + 'codeReview.caps': 'Caps', + 'codeReview.none': 'None', + 'codeReview.severity': 'Severity', + 'codeReview.confidenceLabel': 'Confidence', + 'codeReview.all': 'All', + 'codeReview.noMatches': 'No findings match these filters.', + 'codeReview.source': (v) => `Source: ${v?.value ?? ''}`, + 'codeReview.failureScenario': 'Failure scenario', + 'codeReview.suggestedFix': 'Suggested fix', + 'codeReview.outcome': 'Outcome', + 'codeReview.locations': 'Locations', + 'codeReview.evidence': 'Evidence', + 'codeReview.notLinked': 'not linked', + 'codeReview.loadErrorTitle': 'Unable to display code review', + 'codeReview.loading': 'Loading code review...', + 'codeReview.artifactTruncated': + 'Code review artifact is truncated and cannot be validated.', + 'codeReview.reportTruncated': 'Markdown report is truncated.', + 'codeReview.unavailable': (v) => + `This code review is no longer authoritative because its artifact status is ${v?.status ?? 'unknown'}. Regenerate the review to view it.`, + 'codeReview.workspaceRequired': + 'Code review artifacts must be readable workspace files.', 'sideTask.title': 'Side task', 'sideTask.description': 'View or create side tasks', 'sideTask.new': 'New', @@ -3669,8 +3704,41 @@ const ZH: Messages = { 'scheduledTasks.condition.cardPrefix': '若：', 'turnOutputs.filesEdited': (v) => `已编辑 ${v?.count ?? 0} 个文件`, 'turnOutputs.viewChanges': '查看更改', - 'turnOutputs.review': '审核', + // 与英文键同理：键名保留历史 `review`，标签改为“文件更改”， + // 代码审查结果视图使用 `codeReview.*`，请勿“修正”此不一致。 + 'turnOutputs.review': '文件更改', 'turnOutputs.reviewLatest': '查看最近文件修改', + 'codeReview.authoritativeVerdict': '权威裁决', + 'codeReview.targetEffort': (v) => + `目标：${v?.target ?? ''} · 强度：${v?.effort ?? ''}`, + 'codeReview.openReport': '打开 Markdown 报告', + 'codeReview.loadingReport': '正在加载 Markdown 报告…', + 'codeReview.back': '返回审查结果', + 'codeReview.reviewCounts': '审查计数', + 'codeReview.total': '总计', + 'codeReview.confidence': (v) => `${v?.value ?? ''} 置信度`, + 'codeReview.held': '暂缓', + 'codeReview.heldByMeasurement': '经基线测量暂缓，未保留 Critical', + 'codeReview.caps': '裁决上限', + 'codeReview.none': '无', + 'codeReview.severity': '严重级别', + 'codeReview.confidenceLabel': '置信度', + 'codeReview.all': '全部', + 'codeReview.noMatches': '没有符合当前筛选条件的发现。', + 'codeReview.source': (v) => `来源：${v?.value ?? ''}`, + 'codeReview.failureScenario': '失败场景', + 'codeReview.suggestedFix': '建议修复', + 'codeReview.outcome': '处理结果', + 'codeReview.locations': '位置', + 'codeReview.evidence': '证据', + 'codeReview.notLinked': '未链接', + 'codeReview.loadErrorTitle': '无法显示代码审查结果', + 'codeReview.loading': '正在加载代码审查结果…', + 'codeReview.artifactTruncated': '代码审查制品已截断，无法验证。', + 'codeReview.reportTruncated': 'Markdown 报告已截断。', + 'codeReview.unavailable': (v) => + `此代码审查已不再具备权威性，制品状态为 ${v?.status ?? 'unknown'}。请重新生成审查结果。`, + 'codeReview.workspaceRequired': '代码审查制品必须是可读取的工作区文件。', 'sideTask.title': '侧边任务', 'sideTask.description': '查看或新增侧边任务', 'sideTask.new': '新增',