mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-11-07 06:19:43 -10:00
feat(review): Add structured Web Shell review results (#8402)
* feat(review): add Web Shell review artifacts Co-authored-by: Qwen-Coder <[email protected]> * test(web-shell): add code review artifact visual scenario (#8402) * fix(review): address Web Shell review artifact feedback (#8402) * save-artifact: document why paths resolve against the daemon workspace root (QWEN_CODE_PROJECT_DIR) instead of cwd, and cover the relative-path form the skill documents with a test where the two roots differ. * CLI/renderer contract: the renderer hand-duplicates the findings vocabulary and fails closed on unknown values, so name the renderer as a second consumer beside the CLI's lists and check in a contract fixture generated through the real pipeline (validateFindings -> buildReport -> save-artifact) that exercises every source, severity, confidence and outcome. Exporting the vocabulary through the SDK stays deferred: it is a public cross-package API change beyond this PR's seam. * resolve-anchors now validates `line` exactly like `findings` does (positive safe integer); the two validators in one pipeline no longer disagree. Note: an in-flight `.qwen/tmp` findings file carrying `line: 0` fails where it previously did not. * The renderer validates markdownReportPath (relative, no ".." segments, .md suffix) before it becomes a readWorkspaceFile call, resets the severity/confidence filters when switching artifacts, and surfaces heldByMeasurement so a nonzero Held count is attributable. * save-artifact refuses low effort structurally (choices and library guard) instead of by prose, stats the Markdown report before reading it so a directory reports "not a file", and the component no longer shadows the DOM `document` global. * The case-insensitive alias test now skips visibly on case-sensitive filesystems instead of passing vacuously. * Comment the kept `turnOutputs.review` key and document the JSON companion in the user docs. Co-authored-by: Qwen-Coder <[email protected]> * fix(review): address second Web Shell review artifact feedback round (#8402) --------- Co-authored-by: Qwen-Coder <[email protected]> Co-authored-by: qwen-code-dev-bot <[email protected]>
This commit is contained in:
23 files changed
+3405
-28
No files matched your search
@@ -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.
|
||||
@@ -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`.
|
||||
|
||||
@@ -63,6 +63,7 @@ describe('reviewCommand', () => {
|
||||
'findings',
|
||||
'publish-assets',
|
||||
'compose-review',
|
||||
'save-artifact',
|
||||
'submit',
|
||||
'cleanup',
|
||||
]);
|
||||
|
||||
@@ -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: () => {
|
||||
|
||||
@@ -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', () => {
|
||||
|
||||
@@ -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<string, unknown>;
|
||||
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 [
|
||||
{
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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',
|
||||
});
|
||||
});
|
||||
});
|
||||
@@ -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<string, unknown> {
|
||||
if (value === null || typeof value !== 'object' || Array.isArray(value)) {
|
||||
throw new Error(`${label} must be an object.`);
|
||||
}
|
||||
return value as Record<string, unknown>;
|
||||
}
|
||||
|
||||
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<string, number>)['agents']!,
|
||||
srcDiffLines: (lowSignal as Record<string, number>)[
|
||||
'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}`);
|
||||
},
|
||||
};
|
||||
@@ -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-<target>-findings.json \
|
||||
--composed .qwen/tmp/qwen-review-<target>-composed.json \
|
||||
--report .qwen/reviews/<report>.md \
|
||||
--target <target> \
|
||||
--effort <effort> \
|
||||
--out .qwen/reviews/<report>.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": "<absolute path>", "workspacePath": "<path relative to the main project directory>"}`. 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/<report>.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.
|
||||
|
||||
@@ -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<HTMLButtonElement>('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<HTMLButtonElement>(
|
||||
'[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<HTMLButtonElement>(
|
||||
'[data-testid="split-report-changed-artifact"]',
|
||||
)
|
||||
?.click();
|
||||
await Promise.resolve();
|
||||
});
|
||||
|
||||
expect(document.body.textContent).toContain('changed');
|
||||
|
||||
await act(async () => {
|
||||
container
|
||||
.querySelector<HTMLButtonElement>(
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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> = {},
|
||||
): 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 (
|
||||
<I18nProvider language="en">
|
||||
<ArtifactPanel
|
||||
artifacts={[artifact]}
|
||||
tabs={[
|
||||
{
|
||||
id: 'artifact:review-artifact',
|
||||
kind: 'artifact',
|
||||
title: artifact.title,
|
||||
artifactId: artifact.id,
|
||||
},
|
||||
]}
|
||||
activeTabId="artifact:review-artifact"
|
||||
reviewChanges={[]}
|
||||
selectedReviewPath={null}
|
||||
onSelectTab={() => {}}
|
||||
onCloseTab={() => {}}
|
||||
onOpenFilePreview={() => {}}
|
||||
onClose={() => {}}
|
||||
/>
|
||||
</I18nProvider>
|
||||
);
|
||||
}
|
||||
|
||||
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<HTMLButtonElement>(
|
||||
'[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<HTMLButtonElement>(
|
||||
'[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');
|
||||
});
|
||||
|
||||
@@ -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 <CodeReviewUnavailable status={artifact.status} />;
|
||||
}
|
||||
if (!canPreviewWorkspaceFile || !artifact.workspacePath) {
|
||||
return <CodeReviewWorkspaceRequired />;
|
||||
}
|
||||
return (
|
||||
<CodeReviewArtifactDetail
|
||||
workspacePath={artifact.workspacePath}
|
||||
artifactVersion={`${artifact.status}:${artifact.updatedAt}`}
|
||||
workspaceActions={workspaceActions}
|
||||
/>
|
||||
);
|
||||
}
|
||||
|
||||
if (canPreviewWorkspaceFile && artifact.workspacePath) {
|
||||
return (
|
||||
<WorkspaceFilePreview
|
||||
@@ -2206,6 +2224,24 @@ function ArtifactDetail({
|
||||
);
|
||||
}
|
||||
|
||||
function CodeReviewUnavailable({ status }: { status: string }) {
|
||||
const { t } = useI18n();
|
||||
return (
|
||||
<div className={styles.previewError} role="alert">
|
||||
{t('codeReview.unavailable', { status })}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
function CodeReviewWorkspaceRequired() {
|
||||
const { t } = useI18n();
|
||||
return (
|
||||
<div className={styles.previewError} role="alert">
|
||||
{t('codeReview.workspaceRequired')}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
function isHtmlArtifact(artifact: DaemonSessionArtifact) {
|
||||
const path = artifact.workspacePath?.toLowerCase() ?? '';
|
||||
const mimeType = artifact.mimeType?.toLowerCase() ?? '';
|
||||
|
||||
@@ -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;
|
||||
}
|
||||
}
|
||||
@@ -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(
|
||||
<I18nProvider language={language}>
|
||||
<CodeReviewArtifactDetail
|
||||
workspacePath=".qwen/reviews/pr-123.json"
|
||||
workspaceActions={actions}
|
||||
/>
|
||||
</I18nProvider>,
|
||||
);
|
||||
});
|
||||
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<HTMLSelectElement>(
|
||||
'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<HTMLSelectElement>(
|
||||
'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<HTMLAnchorElement>(
|
||||
'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(
|
||||
<I18nProvider language="en">
|
||||
<CodeReviewArtifactDetail
|
||||
workspacePath=".qwen/reviews/pr-123.json"
|
||||
artifactVersion="1"
|
||||
workspaceActions={actions}
|
||||
/>
|
||||
</I18nProvider>,
|
||||
);
|
||||
});
|
||||
await flush();
|
||||
act(() => {
|
||||
Array.from(container.querySelectorAll('button'))
|
||||
.find((item) => item.textContent === 'Open Markdown report')
|
||||
?.click();
|
||||
});
|
||||
act(() => {
|
||||
root.render(
|
||||
<I18nProvider language="en">
|
||||
<CodeReviewArtifactDetail
|
||||
workspacePath=".qwen/reviews/pr-456.json"
|
||||
artifactVersion="2"
|
||||
workspaceActions={actions}
|
||||
/>
|
||||
</I18nProvider>,
|
||||
);
|
||||
});
|
||||
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(
|
||||
<I18nProvider language="en">
|
||||
<CodeReviewArtifactDetail
|
||||
workspacePath=".qwen/reviews/pr-123.json"
|
||||
artifactVersion="1"
|
||||
workspaceActions={actions}
|
||||
/>
|
||||
</I18nProvider>,
|
||||
);
|
||||
});
|
||||
await flush();
|
||||
const severity = container.querySelector<HTMLSelectElement>(
|
||||
'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(
|
||||
<I18nProvider language="en">
|
||||
<CodeReviewArtifactDetail
|
||||
workspacePath=".qwen/reviews/pr-456.json"
|
||||
artifactVersion="2"
|
||||
workspaceActions={actions}
|
||||
/>
|
||||
</I18nProvider>,
|
||||
);
|
||||
});
|
||||
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<HTMLSelectElement>(
|
||||
'select[aria-label="Severity"]',
|
||||
)?.value,
|
||||
).toBe('all');
|
||||
expect(
|
||||
container.querySelector<HTMLSelectElement>(
|
||||
'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(
|
||||
<I18nProvider language="en">
|
||||
<CodeReviewArtifactDetail
|
||||
workspacePath=".qwen/reviews/pr-123.json"
|
||||
workspaceActions={actions}
|
||||
/>
|
||||
</I18nProvider>,
|
||||
);
|
||||
});
|
||||
await flush();
|
||||
|
||||
const image = container.querySelector<HTMLImageElement>(
|
||||
'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(
|
||||
<I18nProvider language="en">
|
||||
<CodeReviewArtifactDetail
|
||||
workspacePath=".qwen/reviews/pr-123.json"
|
||||
workspaceActions={actions}
|
||||
/>
|
||||
</I18nProvider>,
|
||||
);
|
||||
});
|
||||
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(
|
||||
<I18nProvider language={language}>
|
||||
<CodeReviewArtifactDetail
|
||||
workspacePath=".qwen/reviews/pr-123.json"
|
||||
workspaceActions={actions}
|
||||
/>
|
||||
</I18nProvider>,
|
||||
);
|
||||
});
|
||||
|
||||
renderAt('en');
|
||||
await flush();
|
||||
const severity = container.querySelector<HTMLSelectElement>(
|
||||
'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<HTMLSelectElement>(
|
||||
'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(
|
||||
<I18nProvider language="en">
|
||||
<CodeReviewArtifactDetail
|
||||
workspacePath=".qwen/reviews/pr-123.json"
|
||||
workspaceActions={actions}
|
||||
/>
|
||||
</I18nProvider>,
|
||||
);
|
||||
});
|
||||
await flush();
|
||||
|
||||
expect(container.querySelector('[role="alert"]')?.textContent).toContain(
|
||||
'truncated',
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -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<Severity, number>;
|
||||
byConfidence: Record<Confidence, number>;
|
||||
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<string, unknown> {
|
||||
if (value === null || typeof value !== 'object' || Array.isArray(value)) {
|
||||
throw new Error(`${label} must be an object.`);
|
||||
}
|
||||
return value as Record<string, unknown>;
|
||||
}
|
||||
|
||||
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<T extends string>(
|
||||
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<T extends string>(
|
||||
value: unknown,
|
||||
keys: readonly T[],
|
||||
label: string,
|
||||
): Record<T, number> {
|
||||
const source = object(value, label);
|
||||
return Object.fromEntries(
|
||||
keys.map((key) => [key, integer(source[key], `${label}.${key}`)]),
|
||||
) as Record<T, number>;
|
||||
}
|
||||
|
||||
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<string | null>(null);
|
||||
const [loadError, setLoadError] = useState<string | null>(null);
|
||||
const [artifactTruncated, setArtifactTruncated] = useState(false);
|
||||
const [severity, setSeverity] = useState<'all' | Severity>('all');
|
||||
const [confidence, setConfidence] = useState<'all' | Confidence>('all');
|
||||
const [reportPath, setReportPath] = useState<string | null>(null);
|
||||
const [reportContent, setReportContent] = useState<string | null>(null);
|
||||
const [reportError, setReportError] = useState<string | null>(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 (
|
||||
<div className={styles.report}>
|
||||
<button
|
||||
type="button"
|
||||
className={styles.secondaryButton}
|
||||
onClick={() => {
|
||||
reportRequest.current += 1;
|
||||
setReportPath(null);
|
||||
setReportContent(null);
|
||||
setReportError(null);
|
||||
setReportLoading(false);
|
||||
}}
|
||||
>
|
||||
{t('codeReview.back')}
|
||||
</button>
|
||||
<div className={styles.path}>{reportPath}</div>
|
||||
{reportError ? (
|
||||
<div className={styles.error} role="alert">
|
||||
{reportError}
|
||||
</div>
|
||||
) : (
|
||||
<Markdown content={reportContent ?? ''} />
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
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 (
|
||||
<div className={styles.errorFallback}>
|
||||
<div className={styles.error} role="alert">
|
||||
<strong>{t('codeReview.loadErrorTitle')}</strong>
|
||||
<span>{loadFailure ?? parsed?.error}</span>
|
||||
</div>
|
||||
{fallbackReport && (
|
||||
<button
|
||||
type="button"
|
||||
className={styles.secondaryButton}
|
||||
onClick={() => openReport(fallbackReport)}
|
||||
disabled={reportLoading}
|
||||
>
|
||||
{reportLoading
|
||||
? t('codeReview.loadingReport')
|
||||
: t('codeReview.openReport')}
|
||||
</button>
|
||||
)}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
if (!parsed?.document) {
|
||||
return <div className={styles.empty}>{t('codeReview.loading')}</div>;
|
||||
}
|
||||
|
||||
const reviewDocument = parsed.document;
|
||||
const findings = reviewDocument.findings.filter(
|
||||
(finding) =>
|
||||
(severity === 'all' || finding.severity === severity) &&
|
||||
(confidence === 'all' || finding.confidence === confidence),
|
||||
);
|
||||
|
||||
return (
|
||||
<div className={styles.root}>
|
||||
<section className={styles.summary}>
|
||||
<div>
|
||||
<div className={styles.eyebrow}>
|
||||
{t('codeReview.authoritativeVerdict')}
|
||||
</div>
|
||||
<h2 className={styles.verdict}>
|
||||
{reviewDocument.verdict.verdictLine}
|
||||
</h2>
|
||||
<div className={styles.meta}>
|
||||
{t('codeReview.targetEffort', {
|
||||
target: reviewDocument.target,
|
||||
effort: reviewDocument.effort,
|
||||
})}
|
||||
</div>
|
||||
</div>
|
||||
<button
|
||||
type="button"
|
||||
className={styles.primaryButton}
|
||||
onClick={() => openReport(reviewDocument.markdownReportPath)}
|
||||
disabled={reportLoading}
|
||||
>
|
||||
{reportLoading
|
||||
? t('codeReview.loadingReport')
|
||||
: t('codeReview.openReport')}
|
||||
</button>
|
||||
</section>
|
||||
|
||||
<section
|
||||
className={styles.metrics}
|
||||
aria-label={t('codeReview.reviewCounts')}
|
||||
>
|
||||
<Metric
|
||||
label={t('codeReview.total')}
|
||||
value={reviewDocument.counts.total}
|
||||
/>
|
||||
{SEVERITIES.map((value) => (
|
||||
<Metric
|
||||
key={value}
|
||||
label={value}
|
||||
value={reviewDocument.counts.bySeverity[value]}
|
||||
/>
|
||||
))}
|
||||
{CONFIDENCES.map((value) => (
|
||||
<Metric
|
||||
key={value}
|
||||
label={t('codeReview.confidence', { value })}
|
||||
value={reviewDocument.counts.byConfidence[value]}
|
||||
/>
|
||||
))}
|
||||
<Metric
|
||||
label={t('codeReview.held')}
|
||||
value={reviewDocument.counts.held}
|
||||
/>
|
||||
</section>
|
||||
|
||||
<section className={styles.caps}>
|
||||
<span className={styles.eyebrow}>{t('codeReview.caps')}</span>
|
||||
<span>
|
||||
{reviewDocument.verdict.cappedBy.length > 0
|
||||
? reviewDocument.verdict.cappedBy.join(', ')
|
||||
: t('codeReview.none')}
|
||||
</span>
|
||||
</section>
|
||||
|
||||
<div className={styles.filters}>
|
||||
<label>
|
||||
{t('codeReview.severity')}
|
||||
<select
|
||||
aria-label={t('codeReview.severity')}
|
||||
value={severity}
|
||||
onChange={(event) =>
|
||||
setSeverity(event.target.value as 'all' | Severity)
|
||||
}
|
||||
>
|
||||
<option value="all">{t('codeReview.all')}</option>
|
||||
{SEVERITIES.map((value) => (
|
||||
<option key={value} value={value}>
|
||||
{value} ({reviewDocument.counts.bySeverity[value]})
|
||||
</option>
|
||||
))}
|
||||
</select>
|
||||
</label>
|
||||
<label>
|
||||
{t('codeReview.confidenceLabel')}
|
||||
<select
|
||||
aria-label={t('codeReview.confidenceLabel')}
|
||||
value={confidence}
|
||||
onChange={(event) =>
|
||||
setConfidence(event.target.value as 'all' | Confidence)
|
||||
}
|
||||
>
|
||||
<option value="all">{t('codeReview.all')}</option>
|
||||
{CONFIDENCES.map((value) => (
|
||||
<option key={value} value={value}>
|
||||
{value} ({reviewDocument.counts.byConfidence[value]})
|
||||
</option>
|
||||
))}
|
||||
</select>
|
||||
</label>
|
||||
</div>
|
||||
|
||||
<div className={styles.findings}>
|
||||
{findings.length === 0 ? (
|
||||
<div className={styles.empty}>{t('codeReview.noMatches')}</div>
|
||||
) : (
|
||||
findings.map((finding) => (
|
||||
<article key={finding.id} className={styles.finding}>
|
||||
<div className={styles.findingHeader}>
|
||||
<span className={styles.severity}>{finding.severity}</span>
|
||||
<span>
|
||||
{t('codeReview.confidence', {
|
||||
value: finding.confidence,
|
||||
})}
|
||||
</span>
|
||||
<span>{t('codeReview.source', { value: finding.source })}</span>
|
||||
{finding.category && <span>{finding.category}</span>}
|
||||
</div>
|
||||
<h3>{finding.summary}</h3>
|
||||
<Detail
|
||||
label={t('codeReview.failureScenario')}
|
||||
value={finding.failureScenario}
|
||||
/>
|
||||
{finding.suggestedFix && (
|
||||
<Detail
|
||||
label={t('codeReview.suggestedFix')}
|
||||
value={finding.suggestedFix}
|
||||
/>
|
||||
)}
|
||||
{finding.outcome && (
|
||||
<Detail
|
||||
label={t('codeReview.outcome')}
|
||||
value={`${finding.outcome}${finding.outcomeNote ? ` — ${finding.outcomeNote}` : ''}`}
|
||||
/>
|
||||
)}
|
||||
{finding.heldByMeasurement && (
|
||||
<Detail
|
||||
label={t('codeReview.heldByMeasurement')}
|
||||
value={finding.heldByMeasurement.file}
|
||||
/>
|
||||
)}
|
||||
<div className={styles.detailBlock}>
|
||||
<strong>{t('codeReview.locations')}</strong>
|
||||
<ul>
|
||||
{finding.locations.map((location, index) => (
|
||||
<li
|
||||
key={`${location.file}:${location.line ?? ''}:${index}`}
|
||||
>
|
||||
<code>
|
||||
{location.file}
|
||||
{location.line === undefined ? '' : `:${location.line}`}
|
||||
</code>
|
||||
{location.anchor && <span> — {location.anchor}</span>}
|
||||
</li>
|
||||
))}
|
||||
</ul>
|
||||
</div>
|
||||
{((finding.assets && finding.assets.length > 0) ||
|
||||
(finding.assetFiles && finding.assetFiles.length > 0)) && (
|
||||
<div className={styles.detailBlock}>
|
||||
<strong>{t('codeReview.evidence')}</strong>
|
||||
<ul>
|
||||
{(finding.assets ?? []).map((asset, index) => (
|
||||
<li key={`${asset}-${index}`}>
|
||||
{isSafeHref(asset) ? (
|
||||
<a
|
||||
href={asset}
|
||||
target="_blank"
|
||||
rel="noopener noreferrer"
|
||||
>
|
||||
{asset}
|
||||
</a>
|
||||
) : (
|
||||
<span className={styles.notLinked}>
|
||||
{asset} ({t('codeReview.notLinked')})
|
||||
</span>
|
||||
)}
|
||||
</li>
|
||||
))}
|
||||
{(finding.assetFiles ?? []).map((file, index) => (
|
||||
<EvidenceFile
|
||||
key={`${file}-${index}`}
|
||||
path={file}
|
||||
workspaceActions={workspaceActions}
|
||||
/>
|
||||
))}
|
||||
</ul>
|
||||
</div>
|
||||
)}
|
||||
</article>
|
||||
))
|
||||
)}
|
||||
</div>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
// 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<string | null>(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 (
|
||||
<li>
|
||||
{src && <img className={styles.evidenceImage} src={src} alt={path} />}
|
||||
<code>{path}</code>
|
||||
</li>
|
||||
);
|
||||
}
|
||||
|
||||
function Metric({ label, value }: { label: string; value: number }) {
|
||||
return (
|
||||
<div className={styles.metric}>
|
||||
<strong>{value}</strong>
|
||||
<span>{label}</span>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
|
||||
function Detail({ label, value }: { label: string; value: string }) {
|
||||
return (
|
||||
<div className={styles.detailBlock}>
|
||||
<strong>{label}</strong>
|
||||
<p>{value}</p>
|
||||
</div>
|
||||
);
|
||||
}
|
||||
+142
@@ -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"
|
||||
}
|
||||
@@ -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.',
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -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<string, string>;
|
||||
/**
|
||||
* 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<void> {
|
||||
await route.fulfill({
|
||||
status,
|
||||
|
||||
@@ -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}`);
|
||||
});
|
||||
});
|
||||
}
|
||||
@@ -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': '新增',
|
||||
|
||||
Reference in new issue
Block a user