From 43355f451b53f70c11f9c9d7810ef776240e83d2 Mon Sep 17 00:00:00 2001 From: aict666 <253870639+aict666@users.noreply.github.com> Date: Sat, 25 Apr 2026 01:04:22 +0800 Subject: [PATCH] fix(#24): redact marker must have no shell metacharacters MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reproducer: Claude Code v2.1.114 (Opus 4.7) routed through the proxy emits a bash tool call whose command includes the redaction marker verbatim, e.g.: cd (internal path redacted) && git log --all --oneline -30 zsh parses `cd word(qualifier)` as glob-qualifier syntax, reads the first char of "internal" as a qualifier flag, and fails with: (eval):1: unknown file attribute: i The model sees the cryptic error, can't recover, and starts replying with confused meta-text ("the user seems to have pasted a system prompt along with a shell error about an invalid file test operator") instead of retrying. Root cause: commit 9120b8b picked `(internal path redacted)` because "a multi-word parenthesised phrase cannot be tokenised as a path or identifier." True for file APIs — but parens are first-class shell metacharacters (subshell, glob qualifier). The marker had to satisfy file-API safety AND shell safety AND anti-loop shape at once; the old marker only satisfied the first two. Fix: drop every shell metacharacter from the marker. Use a plain ASCII multi-word phrase: "redacted internal path". If a model still echoes it into shell the words become separate argv entries and `cd` or `Read` fails with a clean, recoverable error (too many arguments / single ENOENT) — never the cryptic glob-qualifier error. The marker still cannot be parsed as a path (no `/`, no `.`) or as an identifier (contains whitespace), so the drift-probe regressions (./tail, [internal], ) remain fixed. Adds a REDACTED_PATH marker-shape regression suite asserting: 1. No character in `()[]{}<>|&;$\`"'\\*?` appears in the marker. 2. The marker has no `/` or `\\`, contains whitespace, and is multi-word — so it cannot reappear as a path or identifier. Tests: 61/61 pass (59 existing + 2 new marker-shape guards). --- src/sanitize.js | 36 ++++++++++++++++++++---------- test/sanitize.test.js | 51 +++++++++++++++++++++++++++++++------------ 2 files changed, 61 insertions(+), 26 deletions(-) diff --git a/src/sanitize.js b/src/sanitize.js index eb47deb6..2b476b43 100644 --- a/src/sanitize.js +++ b/src/sanitize.js @@ -31,18 +31,30 @@ const _repoRoot = (() => { } catch { return '/root/WindsurfAPI'; } })(); -// Placeholder chosen as a parenthesised natural-language phrase because -// every punctuation-only marker we tried has been re-used by the model -// as a real path in later turns: -// ./tail → LLM Reads ./src/main.py → ENOENT → loops -// [internal] → LLM runs `ls [internal]` → ENOENT → loops -// → LLM passes it as file_path arg to Read/Bash → -// ENOENT (Linux) or Errno 22 on Windows → loops -// A multi-word parenthesised phrase cannot be tokenised as a path or -// identifier, so clients skip it instead of trying to resolve. Verified -// with the drift probe (scripts/_agent_drift_probe.py) that previously -// crashed because sonnet kept calling read_file(''). -const REDACTED_PATH = '(internal path redacted)'; +// Placeholder chosen as a plain-ASCII, multi-word phrase with NO shell +// metacharacters. Every previous marker broke at least one consumer: +// ./tail → LLM Reads ./src/main.py → ENOENT → loops +// [internal] → LLM runs `ls [internal]` → ENOENT → loops +// → LLM passes to Read/Bash → ENOENT (Linux) / +// Errno 22 (Windows) → loops +// (internal path redacted) → zsh parses `cd (internal path redacted)` +// as glob-qualifier syntax → cryptic +// "unknown file attribute: i" error → Opus +// gets confused and stops calling tools +// The marker must satisfy three constraints at once: +// 1. Contains no character that any mainstream shell parses specially +// — excludes `( ) [ ] { } < > | & ; $ \` " ' \\ * ?` and whitespace +// inside a paired delimiter. +// 2. Does not look like a path or identifier so the model does not +// reuse it as a file_path / cd target on later turns. +// 3. Reads as descriptive prose when it appears in sanitized output. +// A plain multi-word phrase with a trailing period satisfies all three: +// if a client ever does embed it in shell, the words become separate +// argv entries and `cd` / `Read` fails with a clean, recoverable error +// (too many arguments / ENOENT once) instead of the cryptic zsh glob +// qualifier error. Verified with the drift probe +// (scripts/_agent_drift_probe.py). +const REDACTED_PATH = 'redacted internal path'; const PATTERNS = [ [/\/tmp\/windsurf-workspace(?:\/[^\s"'`<>)}\],*;]*)?/g, REDACTED_PATH], diff --git a/test/sanitize.test.js b/test/sanitize.test.js index 7f6607a0..33001a39 100644 --- a/test/sanitize.test.js +++ b/test/sanitize.test.js @@ -2,29 +2,30 @@ import { describe, it } from 'node:test'; import assert from 'node:assert/strict'; import { sanitizeText, PathSanitizeStream, sanitizeToolCall } from '../src/sanitize.js'; -// Leaked Windsurf paths are redacted to the angle-bracketed marker -// `(internal path redacted)`. Shell / file APIs won't try to resolve that string, -// and downstream LLMs don't tokenize it as a path (see sanitize.js header -// for the history — ./tail and [internal] both caused read-loop regressions). +// Leaked Windsurf paths are redacted to the multi-word prose marker +// `redacted internal path`. The marker MUST contain no shell metacharacter +// — earlier `(internal path redacted)` broke zsh (glob-qualifier syntax on +// `(…)` → `unknown file attribute: i`). See sanitize.js header for the +// full history of markers that regressed. describe('sanitizeText', () => { it('redacts /tmp/windsurf-workspace paths', () => { - assert.equal(sanitizeText('/tmp/windsurf-workspace/src/index.js'), '(internal path redacted)'); + assert.equal(sanitizeText('/tmp/windsurf-workspace/src/index.js'), 'redacted internal path'); }); it('redacts bare /tmp/windsurf-workspace', () => { - assert.equal(sanitizeText('/tmp/windsurf-workspace'), '(internal path redacted)'); + assert.equal(sanitizeText('/tmp/windsurf-workspace'), 'redacted internal path'); }); it('redacts per-account workspace paths', () => { assert.equal( sanitizeText('/home/user/projects/workspace-abc12345/package.json'), - '(internal path redacted)' + 'redacted internal path' ); }); it('redacts /opt/windsurf', () => { - assert.equal(sanitizeText('/opt/windsurf/language_server'), '(internal path redacted)'); + assert.equal(sanitizeText('/opt/windsurf/language_server'), 'redacted internal path'); }); it('leaves normal text unchanged', () => { @@ -35,7 +36,7 @@ describe('sanitizeText', () => { it('handles multiple patterns in one string', () => { const input = 'Editing /tmp/windsurf-workspace/a.js and /opt/windsurf/bin'; const result = sanitizeText(input); - assert.equal(result, 'Editing (internal path redacted) and (internal path redacted)'); + assert.equal(result, 'Editing redacted internal path and redacted internal path'); }); it('returns non-strings unchanged', () => { @@ -50,7 +51,7 @@ describe('PathSanitizeStream', () => { const stream = new PathSanitizeStream(); const out = stream.feed('/tmp/windsurf-workspace/file.js is here'); const rest = stream.flush(); - assert.equal(out + rest, '(internal path redacted) is here'); + assert.equal(out + rest, 'redacted internal path is here'); }); it('handles path split across chunks', () => { @@ -59,7 +60,7 @@ describe('PathSanitizeStream', () => { result += stream.feed('Look at /tmp/windsurf'); result += stream.feed('-workspace/config.yaml for details'); result += stream.flush(); - assert.equal(result, 'Look at (internal path redacted) for details'); + assert.equal(result, 'Look at redacted internal path for details'); }); it('handles partial prefix at buffer end', () => { @@ -68,7 +69,7 @@ describe('PathSanitizeStream', () => { result += stream.feed('path is /tmp/win'); result += stream.feed('dsurf-workspace/x.js done'); result += stream.flush(); - assert.equal(result, 'path is (internal path redacted) done'); + assert.equal(result, 'path is redacted internal path done'); }); it('flushes clean text immediately', () => { @@ -82,13 +83,13 @@ describe('sanitizeToolCall', () => { it('sanitizes argumentsJson paths', () => { const tc = { name: 'Read', argumentsJson: '{"path":"/tmp/windsurf-workspace/f.js"}' }; const result = sanitizeToolCall(tc); - assert.equal(result.argumentsJson, '{"path":"(internal path redacted)"}'); + assert.equal(result.argumentsJson, '{"path":"redacted internal path"}'); }); it('sanitizes input object string values', () => { const tc = { name: 'Read', input: { file_path: '/home/user/projects/workspace-abc12345/src/x.ts' } }; const result = sanitizeToolCall(tc); - assert.equal(result.input.file_path, '(internal path redacted)'); + assert.equal(result.input.file_path, 'redacted internal path'); }); it('returns null/undefined unchanged', () => { @@ -96,3 +97,25 @@ describe('sanitizeToolCall', () => { assert.equal(sanitizeToolCall(undefined), undefined); }); }); + +describe('REDACTED_PATH marker shape (shell-safety regression)', () => { + // The marker is emitted verbatim into model-facing text. Models + // sometimes echo it back inside a shell command (e.g. `cd `). + // If the marker contains any character the shell parses specially, the + // resulting command fails with a cryptic error instead of a clean + // ENOENT / too-many-arguments, and the model derails (issue: zsh + // `unknown file attribute: i` after parens redaction). + const marker = sanitizeText('/tmp/windsurf-workspace'); + + it('contains no shell metacharacters', () => { + const banned = /[()\[\]{}<>|&;$`\\"'*?]/; + assert.ok(!banned.test(marker), `marker must not contain shell metachars: got ${JSON.stringify(marker)}`); + }); + + it('is not shaped like a path or identifier (multi-word, no slashes, has spaces)', () => { + assert.ok(!marker.includes('/'), 'marker must not contain / (looks like a path)'); + assert.ok(!marker.includes('\\'), 'marker must not contain \\ (looks like a Windows path)'); + assert.ok(marker.includes(' '), 'marker must contain whitespace so it cannot be a single identifier / file arg'); + assert.ok(marker.split(/\s+/).filter(Boolean).length >= 2, 'marker must be multi-word'); + }); +});