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