fix(review): refuse a separators-only run target; anchor the composed-name oracle (#9128)

* 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.

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

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.

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