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'); + }); +});