From 539495226d6be0c44c7629356d73ef3b699d4064 Mon Sep 17 00:00:00 2001 From: Shaojin Wen Date: Wed, 19 Aug 2026 14:32:46 +0800 Subject: [PATCH] fix(cli): make the safeTarget slug space prefix-free at the dash boundary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review's cleanup sweeps .qwen/tmp/ by qwen-review-- prefix. A slug that itself carried '-' — natively (pr-6771 vs pr) or via the truncation join — could extend a shorter slug, letting one target's cleanup delete a DISTINCT target's artifacts (R8-7). The truncation branch this PR carries newly lands deep targets inside the cap instead of dying ENAMETOOLONG, which turned the latent collision live. Drop '-' from the slug alphabet entirely (dashes flatten like separators) and join the truncation digest with '_': with '-' out of every slug, the qwen-review-- boundary is unambiguous by construction — no slug can start with another slug plus '-'. prev-ledger side files keep their hardcoded dashed name on both writer and reader, untouched by the slug. Tests pin the prefix-free property (short-vs-short, short-vs-truncated) and the fixture names follow the new slugs. Co-authored-by: Qwen-Coder --- .../commands/review/issue-9206-repro.test.ts | 14 ++++++------- .../cli/src/commands/review/lib/paths.test.ts | 4 ++-- .../cli/src/commands/review/submit.test.ts | 2 +- packages/cli/src/utils/paths.test.ts | 17 ++++++++++++++++ packages/cli/src/utils/paths.ts | 20 +++++++++++++------ 5 files changed, 41 insertions(+), 16 deletions(-) diff --git a/packages/cli/src/commands/review/issue-9206-repro.test.ts b/packages/cli/src/commands/review/issue-9206-repro.test.ts index c9e925e8b6..2d2ececa02 100644 --- a/packages/cli/src/commands/review/issue-9206-repro.test.ts +++ b/packages/cli/src/commands/review/issue-9206-repro.test.ts @@ -80,7 +80,7 @@ import { import { runCleanup } from './cleanup.js'; const PLAN = { - diffPathAbsolute: '/abs/.qwen/tmp/qwen-review-pr-9206-diff.txt', + diffPathAbsolute: '/abs/.qwen/tmp/qwen-review-pr_9206-diff.txt', chunks: [ { id: 13, @@ -502,7 +502,7 @@ describe('issue #9206 — Step 9 cleanup must not destroy a non-converged run’ dir, '.qwen', 'tmp', - 'qwen-review-pr-9206-fetch.json', + 'qwen-review-pr_9206-fetch.json', ); writeFileSync(planPath, JSON.stringify({ prNumber: '9206' })); const recordDir = promptRecordDir(planPath); @@ -533,7 +533,7 @@ describe('issue #9206 — Step 9 cleanup must not destroy a non-converged run’ dir, '.qwen', 'tmp', - 'qwen-review-pr-9206-fetch.json', + 'qwen-review-pr_9206-fetch.json', ); writeFileSync(planPath, JSON.stringify({ prNumber: '9206' })); const recordDir = promptRecordDir(planPath); @@ -564,7 +564,7 @@ describe('issue #9206 — Step 9 cleanup must not destroy a non-converged run’ dir, '.qwen', 'tmp', - 'qwen-review-pr-9206-fetch.json', + 'qwen-review-pr_9206-fetch.json', ); const recordDir = promptRecordDir(planPath); mkdirSync(recordDir, { recursive: true }); @@ -601,7 +601,7 @@ describe('issue #9206 — Step 9 cleanup must not destroy a non-converged run’ dir, '.qwen', 'tmp', - 'qwen-review-pr-9206-fetch.json', + 'qwen-review-pr_9206-fetch.json', ); const recordDir = promptRecordDir(planPath); mkdirSync(recordDir, { recursive: true }); @@ -634,7 +634,7 @@ describe('issue #9206 — Step 9 cleanup must not destroy a non-converged run’ dir, '.qwen', 'tmp', - 'qwen-review-pr-9206-fetch.json', + 'qwen-review-pr_9206-fetch.json', ); writeFileSync(planPath, JSON.stringify({ prNumber: '9206' })); const recordDir = promptRecordDir(planPath); @@ -657,7 +657,7 @@ describe('issue #9206 — Step 9 cleanup must not destroy a non-converged run’ dir, '.qwen', 'tmp', - 'qwen-review-pr-9206-fetch.json', + 'qwen-review-pr_9206-fetch.json', ); writeFileSync(planPath, JSON.stringify({ prNumber: '9206' })); const recordDir = promptRecordDir(planPath); diff --git a/packages/cli/src/commands/review/lib/paths.test.ts b/packages/cli/src/commands/review/lib/paths.test.ts index e3b428cea9..b830b80589 100644 --- a/packages/cli/src/commands/review/lib/paths.test.ts +++ b/packages/cli/src/commands/review/lib/paths.test.ts @@ -25,9 +25,9 @@ describe('PARSE_ARGS_REPORT', () => { }); describe('tmpFile — target is a single safe component', () => { - it('keeps ordinary labels intact', () => { + it('flattens ordinary labels to the prefix-free slug', () => { expect(tmpFile('pr-6771', 'diff.txt')).toContain( - 'qwen-review-pr-6771-diff.txt', + 'qwen-review-pr_6771-diff.txt', ); expect(tmpFile('local', 'plan.json')).toContain( 'qwen-review-local-plan.json', diff --git a/packages/cli/src/commands/review/submit.test.ts b/packages/cli/src/commands/review/submit.test.ts index 621a5c3b88..3d4a4dcef1 100644 --- a/packages/cli/src/commands/review/submit.test.ts +++ b/packages/cli/src/commands/review/submit.test.ts @@ -2781,7 +2781,7 @@ describe('what the reviewer caught in this change', () => { // receipt lands there. describe('submit receipt (producer half of the audit contract)', () => { const receiptPath = () => - join(dir, '.qwen', 'tmp', 'qwen-review-pr-6771-submit-receipt.json'); + join(dir, '.qwen', 'tmp', 'qwen-review-pr_6771-submit-receipt.json'); const authorizedPost = (over: Record = {}) => args({ userAuthorized: true, ...over }); diff --git a/packages/cli/src/utils/paths.test.ts b/packages/cli/src/utils/paths.test.ts index d4081be9b3..868cc611ab 100644 --- a/packages/cli/src/utils/paths.test.ts +++ b/packages/cli/src/utils/paths.test.ts @@ -38,6 +38,23 @@ describe('safeTarget', () => { expect(safeTarget('///')).toBe('target'); }); + it('keeps the slug space prefix-free at the dash boundary', () => { + // Review's cleanup sweeps `.qwen/tmp/` by `qwen-review--` prefix: + // if a slug could contain `-` — natively, or via the truncation join — + // one target's slug could extend another's, and the shorter target's + // cleanup would delete the longer target's artifacts. `-` is therefore + // out of the slug alphabet entirely. + expect(safeTarget('foo-bar')).toBe('foo_bar'); + const shortSlug = safeTarget('foo'); + expect(safeTarget('foo-bar').startsWith(`${shortSlug}-`)).toBe(false); + const shared = Array.from({ length: 30 }, (_, i) => `level${i}`).join('/'); + const deepSlug = safeTarget(`${shared}/alpha`); + expect(deepSlug).not.toContain('-'); + // The short-vs-truncated direction too: a truncated slug never starts + // with `-`, whatever the short target is. + expect(deepSlug.startsWith(`${safeTarget(shared)}-`)).toBe(false); + }); + it('bounds deep targets to the one-component cap, keeping them distinct', () => { // Deep nested paths flatten past POSIX's 255-byte filename component cap // (ENAMETOOLONG): truncate and carry a hash of the ORIGINAL target so diff --git a/packages/cli/src/utils/paths.ts b/packages/cli/src/utils/paths.ts index ce8366d1f2..16cbf365e5 100644 --- a/packages/cli/src/utils/paths.ts +++ b/packages/cli/src/utils/paths.ts @@ -29,22 +29,30 @@ const SAFE_TARGET_MAX_CHARS = 200; * packages/core (agent-transcript.ts) answers the same question for * transcript/monitor names and flattens dots instead — the two stay separate * on that deliberate difference. + * + * Dashes are flattened too, on purpose: the slugs are prefix-scanned with a + * dash boundary (`qwen-review--` in review's cleanup), and a slug that + * itself contained `-` — natively, or via the truncation join — could extend + * a shorter slug, letting one target's cleanup sweep a DISTINCT target's + * artifacts. With `-` out of the slug alphabet the space is prefix-free at + * that boundary by construction. */ export function safeTarget(target: string): string { let flat = target - .replace(/[^A-Za-z0-9._-]/g, '_') // separators and anything odd → underscore + .replace(/[^A-Za-z0-9._]/g, '_') // separators, dashes, anything odd → underscore .replace(/\.\.+/g, '_'); // no run of dots survives as a traversal token // A deep nested target flattens past the one-component byte cap // (ENAMETOOLONG): truncate and keep uniqueness with a hash of the - // ORIGINAL target, so distinct long paths stay distinct. + // ORIGINAL target, so distinct long paths stay distinct. The digest joins + // with `_` — the one dash in a filename is the prefix-scan boundary. if (flat.length > SAFE_TARGET_MAX_CHARS) { const digest = createHash('sha256') .update(target) .digest('hex') .slice(0, 12); - flat = `${flat.slice(0, SAFE_TARGET_MAX_CHARS - digest.length - 1)}-${digest}`; + flat = `${flat.slice(0, SAFE_TARGET_MAX_CHARS - digest.length - 1)}_${digest}`; } - // Leading dashes too: a dash-leading slug is parsed as short flags when a - // consumer passes it as a spaced CLI argument value. - return flat.replace(/^[._-]+/, '') || 'target'; + // Leading dots/underscores too: a dash-leading slug is no longer possible + // (dashes flatten), and a dot-leading component reads as a hidden file. + return flat.replace(/^[._]+/, '') || 'target'; }