From d52ede2dff01ed6c220e5dec7913215a4ceddd33 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Fri, 14 Aug 2026 07:03:31 +0000 Subject: [PATCH] fix(review): refuse a separators-only run target; anchor the composed-name oracle (#9128) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(review): refuse a separators-only target; anchor the composed-name oracle Round-7 review feedback: - run.ts: buildReviewPrompt now refuses a separators-only target. passed every existing guard, survived basename extraction as the empty string, and pinned the unmatchable qwen-review--composed.json — so a whole child review ran before the parent reported 'no composed verdict was produced', violating the invariant the adjacent comment states. - run-skill-parity.test.ts: anchor the SKILL.md --out capture at the end of the filename. Unanchored, a skill edit that APPENDS to the artifact name (…composed.json.tmp for an atomic write-then-rename) matched as a prefix and left the oracle green while the pin drifted — one direction of the drift this test exists to catch. Both mutation-verified: dropping the guard, and applying the .tmp drift to SKILL.md, each fail their test. * fix(review): refuse an empty run target; widen the oracle anchor; pin the guard's acceptance side Review feedback on this PR: - run.ts: validate on target PRESENCE, not truthiness. An empty target is a target the caller named and got wrong (`qwen review run "$TARGET"` with TARGET unset); read as 'no target given' it silently launched a full local review at the 120-minute default and real model spend, where its siblings /, //, \ now error. - run-skill-parity.test.ts: require the artifact filename to END its token (whitespace-or-end) instead of rejecting an enumerated character class. The (?![\w.]) form still prefix-matched a hyphen rename (…composed.json-v2), leaving the oracle green while the pin drifted; a positive whitespace anchor needs no future suffix character foreseen. - run.test.ts: pin the guard's acceptance side. Over-rejection mutants were surviving — dropping the $ from /^[\\/]+$/ kills every absolute path, dropping the ^ kills every tab-completed trailing slash, and both keep the rejection cases throwing. Four mutations verified, each failing exactly one test: the two over-rejection forms, the removed empty-target guard, and a hyphen-suffix drift applied to SKILL.md. --- .../commands/review/run-skill-parity.test.ts | 8 ++++- packages/cli/src/commands/review/run.test.ts | 30 +++++++++++++++++++ packages/cli/src/commands/review/run.ts | 15 ++++++++-- 3 files changed, 50 insertions(+), 3 deletions(-) diff --git a/packages/cli/src/commands/review/run-skill-parity.test.ts b/packages/cli/src/commands/review/run-skill-parity.test.ts index 5502592c90..aa3bcbd4b3 100644 --- a/packages/cli/src/commands/review/run-skill-parity.test.ts +++ b/packages/cli/src/commands/review/run-skill-parity.test.ts @@ -58,8 +58,14 @@ describe('run pins match the bundled skill templates', () => { itWithSkill('composedNameFor renders Step 6’s --out template', () => { // The template as the skill writes it, e.g. // --out .qwen/tmp/qwen-review-{target}-composed.json + // The filename must END the token: an unanchored capture matches a skill + // edit that APPENDS to the artifact name (`…composed.json.tmp` for an + // atomic write-then-rename, `…json-v2` for a rename) as a prefix, + // leaving this oracle green while the pin drifts — one direction of the + // drift it exists to catch. Whitespace-or-end, not a rejected character + // class, so no future suffix character has to be foreseen. const m = - /--out\s+\.qwen\/tmp\/(qwen-review-\{target\}-composed\.json)/.exec( + /--out\s+\.qwen\/tmp\/(qwen-review-\{target\}-composed\.json)(?=\s|$)/.exec( skill as string, ); // A null here means SKILL.md no longer writes that `--out` line: update diff --git a/packages/cli/src/commands/review/run.test.ts b/packages/cli/src/commands/review/run.test.ts index 46d93d180d..5715402c85 100644 --- a/packages/cli/src/commands/review/run.test.ts +++ b/packages/cli/src/commands/review/run.test.ts @@ -87,6 +87,36 @@ describe('buildReviewPrompt', () => { ); }); + it('rejects a separators-only or empty target', () => { + // `/` names no file: it survives basename extraction as the empty + // string, pinning the unmatchable `qwen-review--composed.json` — a whole + // child review burned before the parent reports "no composed verdict". + for (const target of ['/', '//', '\\']) { + expect(() => buildReviewPrompt({ target })).toThrow( + /Invalid review target/, + ); + } + // An empty target is a target the caller named and got wrong (an unset + // `"$TARGET"`); read as "no target given" it silently becomes a full + // local review at the 120-minute default, not an error. + for (const target of ['', ' ']) { + expect(() => buildReviewPrompt({ target })).toThrow( + /Invalid review target/, + ); + } + }); + + it('keeps accepting separator-bearing paths — the guard is not over-broad', () => { + // The rejection cases alone leave over-rejection mutants green: dropping + // the `$` from `/^[\\/]+$/` kills every absolute path, dropping the `^` + // kills every tab-completed trailing slash. Both shapes are valid + // targets and reach the child unchanged. + expect(buildReviewPrompt({ target: '/repo/src/foo.ts' })).toBe( + '/review /repo/src/foo.ts', + ); + expect(buildReviewPrompt({ target: 'src/' })).toBe('/review src/'); + }); + it('rejects a target carrying quote characters', () => { // tokenizeArgs strips quotes, so `src/it's-a-file.ts` would re-tokenize // to `src/its-a-file.ts` — silently re-targeting a file never named. diff --git a/packages/cli/src/commands/review/run.ts b/packages/cli/src/commands/review/run.ts index bffdcf76b1..3835926829 100644 --- a/packages/cli/src/commands/review/run.ts +++ b/packages/cli/src/commands/review/run.ts @@ -217,16 +217,27 @@ export function buildReviewPrompt(args: { comment?: boolean; }): string { const parts = ['/review']; - if (args.target) { + // Presence, not truthiness: an EMPTY target is a target the caller named + // and got wrong — `qwen review run "$TARGET"` with `TARGET` unset — and + // treating it as "no target given" silently launches a full local review + // on the caller's tree, at the 120-minute default timeout and real model + // spend, instead of the error its siblings `/`, `//`, `\` now get. + if (args.target !== undefined) { // The child re-tokenizes this string; a target carrying whitespace or a // leading dash would split into extra tokens (`123 --comment` would // silently authorise posting), and a quote is stripped by the tokenizer // (`src/it's.ts` would re-target to `src/its.ts`) — refuse anything but a // single clean token. if ( + args.target.trim() === '' || /\s/.test(args.target) || args.target.startsWith('-') || - /['"]/.test(args.target) + /['"]/.test(args.target) || + // A separators-only path (`/`, `//`, `\`) names no file: it survives + // basename extraction as the empty string, which pins the unmatchable + // `qwen-review--composed.json` and burns a whole child review before + // reporting "no composed verdict". Refuse it here instead. + /^[\\/]+$/.test(args.target) ) { throw new Error( `Invalid review target ${JSON.stringify(args.target)}: expected a single PR number, PR URL, or file path`,