From f5c2fee3a9dca55ad1f2785a9d8c8d0780689a2c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 14 May 2026 14:02:10 -0700 Subject: [PATCH] test: wave coverage for sanitizer, link_or_copy, build script, doc drift MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four new test files (29 cases total): browse/test/server-sanitize-surrogates.test.ts: - 11 unit cases for sanitizeLoneSurrogates (passthrough, valid pair, lone high/low mid-string, trailing/leading lone, adjacent doubles, pair-then-lone, lone-then-pair, empty) - 2 bug-repro tests pinning the regression intent (UTF-8 round-trip, JSON.parse round-trip with codepoint assertion) - 4 wiring invariants asserting the architectural choke points stay intact (handleCommandInternalImpl rename, central sanitization line, sanitizeReplacer function exists, SSE producers stringify with replacer) Function extracted from server.ts via regex + eval'd in test scope so no production-code export is needed. test/setup-windows-fallback.test.ts: - Static invariant (D7): zero raw `ln` calls outside the _link_or_copy helper body and comments - Helper-existence assertions - 4-cell behavior matrix (file/dir × Windows/Unix) via awk-style helper extraction + bash -c sourcing - Windows-note printer registration check Mirrors test/setup-conductor-worktree.test.ts patterns. test/build-script-shell-compat.test.ts: - Regex assertion that package.json scripts.* contain no bash brace groups (Bun-Windows-hostile) - Subshell-precedence check for `.version` redirects Strips single-quoted strings before regexing so embedded JS code inside echo '...' doesn't false-positive. test/docs-config-keys.test.ts: - DEPRECATED_KEYS denylist scanned across docs/**/*.md - Round-trip test for `gstack-config get artifacts_sync_mode` Defends the v1.27.0.0 rename from doc drift. Updates to two existing tests: - test/setup-conductor-worktree.test.ts: expect `_link_or_copy` instead of `ln -snf` at the Conductor-worktree guard call site - test/gen-skill-docs.test.ts: same swap at three assertion sites (Codex section, Claude link_claude_skill_dirs body, Codex link_codex_skill_dirs body) Co-Authored-By: Claude Opus 4.7 --- .../test/server-sanitize-surrogates.test.ts | 129 ++++++++++++++++++ test/build-script-shell-compat.test.ts | 40 ++++++ test/docs-config-keys.test.ts | 81 +++++++++++ test/gen-skill-docs.test.ts | 7 +- test/setup-conductor-worktree.test.ts | 5 +- test/setup-windows-fallback.test.ts | 122 +++++++++++++++++ 6 files changed, 379 insertions(+), 5 deletions(-) create mode 100644 browse/test/server-sanitize-surrogates.test.ts create mode 100644 test/build-script-shell-compat.test.ts create mode 100644 test/docs-config-keys.test.ts create mode 100644 test/setup-windows-fallback.test.ts diff --git a/browse/test/server-sanitize-surrogates.test.ts b/browse/test/server-sanitize-surrogates.test.ts new file mode 100644 index 000000000..156d9a3e9 --- /dev/null +++ b/browse/test/server-sanitize-surrogates.test.ts @@ -0,0 +1,129 @@ +import { describe, test, expect } from 'bun:test'; +import * as fs from 'fs'; +import * as path from 'path'; + +// The sanitizer is module-private in server.ts. Rather than refactor it to a +// separate module just for testing, we extract its source via a regex slice and +// eval it in a fresh function scope. Keeps the production layout untouched. +const SERVER_PATH = path.resolve(import.meta.dir, '..', 'src', 'server.ts'); +const SERVER_SRC = fs.readFileSync(SERVER_PATH, 'utf-8'); + +const fnMatch = SERVER_SRC.match( + /function sanitizeLoneSurrogates\(str: string\): string \{[\s\S]*?\n\}/ +); +if (!fnMatch) throw new Error('Could not locate sanitizeLoneSurrogates in server.ts'); + +// Strip TS annotations so eval works under plain JS. +const jsSrc = fnMatch[0].replace('(str: string): string', '(str)'); +const sanitizeLoneSurrogates = new Function(`${jsSrc}\nreturn sanitizeLoneSurrogates;`)() as ( + s: string, +) => string; + +describe('sanitizeLoneSurrogates — unit cases', () => { + test('passthrough ASCII', () => { + expect(sanitizeLoneSurrogates('hello')).toBe('hello'); + }); + + test('passthrough empty string', () => { + expect(sanitizeLoneSurrogates('')).toBe(''); + }); + + test('preserves valid surrogate pair (U+1F389 🎉)', () => { + expect(sanitizeLoneSurrogates('hi 🎉')).toBe('hi 🎉'); + }); + + test('replaces lone high surrogate mid-string', () => { + expect(sanitizeLoneSurrogates('a\uD800b')).toBe('a�b'); + }); + + test('replaces lone low surrogate mid-string', () => { + expect(sanitizeLoneSurrogates('a\uDC00b')).toBe('a�b'); + }); + + test('replaces trailing lone high at end of string', () => { + expect(sanitizeLoneSurrogates('a\uD800')).toBe('a�'); + }); + + test('replaces leading lone low at start of string', () => { + expect(sanitizeLoneSurrogates('\uDC00b')).toBe('�b'); + }); + + test('replaces two adjacent lone highs', () => { + expect(sanitizeLoneSurrogates('\uD800\uD800')).toBe('��'); + }); + + test('replaces two adjacent lone lows', () => { + expect(sanitizeLoneSurrogates('\uDC00\uDC00')).toBe('��'); + }); + + test('preserves valid pair followed by lone low', () => { + // 𐀀 = U+10000 = 𐀀, then a separate lone low. + const input = '𐀀\uDC00'; + const output = sanitizeLoneSurrogates(input); + // Valid pair intact, trailing lone low replaced. + expect(output).toBe('𐀀�'); + }); + + test('preserves valid pair preceded by lone low', () => { + const input = '\uDC00𐀀'; + const output = sanitizeLoneSurrogates(input); + expect(output).toBe('�𐀀'); + }); +}); + +describe('sanitizeLoneSurrogates — bug-repro (D5)', () => { + // Pin the regression intent: a future refactor that drops sanitization + // must fail this test even if happy-path tests still pass. + test('unsanitized lone surrogate causes UTF-8 encode to substitute, sanitized version is stable', () => { + const badPayload = 'page content\uD800more content'; + + // Buffer.from(str, 'utf-8') silently substitutes invalid sequences with + // EF BF BD (U+FFFD). Round-trip is therefore lossy for lone surrogates. + const roundTrippedRaw = Buffer.from(badPayload, 'utf-8').toString('utf-8'); + expect(roundTrippedRaw).not.toBe(badPayload); // proves the bug exists pre-sanitize + + // After sanitization the round-trip is stable. + const sanitized = sanitizeLoneSurrogates(badPayload); + const roundTrippedSanitized = Buffer.from(sanitized, 'utf-8').toString('utf-8'); + expect(roundTrippedSanitized).toBe(sanitized); + }); + + test('JSON.parse(JSON.stringify(...)) round-trip is stable after sanitization', () => { + // Anthropic's API path wraps the response body in a tool_result JSON + // object. JSON.stringify CAN encode a lone surrogate (escapes it), but + // some downstream consumers reject the resulting body. + const badPayload = 'before\uD800after'; + const sanitized = sanitizeLoneSurrogates(badPayload); + const wrapped = JSON.stringify({ content: sanitized }); + const reparsed = JSON.parse(wrapped) as { content: string }; + // .toBe(sanitized) already proves the surrogate was replaced; the + // additional explicit check below documents the specific code points. + expect(reparsed.content).toBe(sanitized); + expect(reparsed.content.charCodeAt(6)).toBe(0xfffd); // � not \uD800 + }); +}); + +describe('sanitizeLoneSurrogates — wiring invariants', () => { + test('server.ts wraps every command result through handleCommandInternal', () => { + // The architectural choice is to wrap once at handleCommandInternal so + // both single-command HTTP and the batch loop inherit. If a future + // refactor moves sanitization back to handleCommand only, this test + // fails by detecting the missing wrapper. + expect(SERVER_SRC).toContain('async function handleCommandInternalImpl('); + expect(SERVER_SRC).toContain('result: sanitizeLoneSurrogates(cr.result)'); + }); + + test('SSE activity feed sanitizes outbound frames via sanitizeReplacer', () => { + // Replacer must run DURING stringify; post-stringify regex is ineffective + // because JSON.stringify converts \uD800 → "\\ud800" before our regex sees it. + expect(SERVER_SRC).toContain('JSON.stringify(entry, sanitizeReplacer)'); + }); + + test('SSE inspector stream sanitizes outbound frames via sanitizeReplacer', () => { + expect(SERVER_SRC).toContain('JSON.stringify(event, sanitizeReplacer)'); + }); + + test('sanitizeReplacer is a function defined in server.ts', () => { + expect(SERVER_SRC).toContain('function sanitizeReplacer('); + }); +}); diff --git a/test/build-script-shell-compat.test.ts b/test/build-script-shell-compat.test.ts new file mode 100644 index 000000000..ee13fb709 --- /dev/null +++ b/test/build-script-shell-compat.test.ts @@ -0,0 +1,40 @@ +import { describe, test, expect } from 'bun:test'; +import * as fs from 'fs'; +import * as path from 'path'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const PKG = JSON.parse(fs.readFileSync(path.join(ROOT, 'package.json'), 'utf-8')) as { + scripts: Record; +}; + +// Strip single-quoted strings so JS code emitted as `echo '{ ... }'` doesn't +// trip the shell-brace-group check. Conservative: only `'...'` segments. +function stripSingleQuoted(s: string): string { + return s.replace(/'[^']*'/g, "''"); +} + +describe('package.json build scripts — POSIX shell compat (D-1460)', () => { + // Bun's Windows shell parser doesn't grok bash brace groups `{ cmd; }`. + // Subshells `( cmd )` are POSIX-universal. This test prevents regression. + test('no bash brace groups in any npm script', () => { + const offending: { script: string; pattern: string }[] = []; + for (const [name, body] of Object.entries(PKG.scripts)) { + const stripped = stripSingleQuoted(body); + const match = stripped.match(/\{\s+[^}]*;\s*\}/); + if (match) { + offending.push({ script: name, pattern: match[0] }); + } + } + expect(offending).toEqual([]); + }); + + test('every `> path/.version` redirect is preceded by a subshell, not a brace group', () => { + // The original PR #1460 target: package.json line 12 had three of these. + const build = PKG.scripts.build ?? ''; + const versionRedirects = [...build.matchAll(/(\([^)]*\)|\{[^}]*\})\s*>\s*\S+\/\.version/g)]; + expect(versionRedirects.length).toBeGreaterThan(0); + for (const m of versionRedirects) { + expect(m[1].startsWith('(')).toBe(true); + } + }); +}); diff --git a/test/docs-config-keys.test.ts b/test/docs-config-keys.test.ts new file mode 100644 index 000000000..2ced29d6e --- /dev/null +++ b/test/docs-config-keys.test.ts @@ -0,0 +1,81 @@ +import { describe, test, expect } from 'bun:test'; +import { spawnSync } from 'child_process'; +import * as fs from 'fs'; +import * as path from 'path'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const CONFIG_BIN = path.join(ROOT, 'bin', 'gstack-config'); + +// gstack-config accepts arbitrary keys (free-form YAML store), so we can't +// build an authoritative set of "valid keys" from the script. Instead, defend +// the specific invariant this wave introduces: deprecated keys must not +// reappear in user-facing docs. Extend the denylist as future renames happen. +const DEPRECATED_KEYS = new Set([ + // Renamed to artifacts_sync_mode in v1.27.0.0, doc references re-deprecated + // in v1.36.0.0 alongside the same rename of *_prompted. + 'gbrain_sync_mode', + 'gbrain_sync_mode_prompted', +]); + +function scanDocsForConfigKeys(): { docPath: string; key: string; line: number }[] { + const hits: { docPath: string; key: string; line: number }[] = []; + const docsDir = path.join(ROOT, 'docs'); + // Recurse docs/ but skip dotfiles. CHANGELOG.md/TODOS.md are excluded by virtue + // of being top-level; we only scan docs/**. + const stack = [docsDir]; + while (stack.length) { + const cur = stack.pop()!; + for (const ent of fs.readdirSync(cur, { withFileTypes: true })) { + if (ent.name.startsWith('.')) continue; + const full = path.join(cur, ent.name); + if (ent.isDirectory()) { + stack.push(full); + continue; + } + if (!ent.name.endsWith('.md')) continue; + const text = fs.readFileSync(full, 'utf-8'); + const lines = text.split('\n'); + lines.forEach((line, idx) => { + // Match `gstack-config set ` or `gstack-config get `. + for (const m of line.matchAll(/gstack-config\s+(?:set|get)\s+([a-z][a-z0-9_]*)/g)) { + hits.push({ docPath: full, key: m[1], line: idx + 1 }); + } + }); + } + } + return hits; +} + +describe('docs ↔ gstack-config key drift guard', () => { + test('docs/ references at least one config key (smoke)', () => { + const hits = scanDocsForConfigKeys(); + expect(hits.length).toBeGreaterThan(0); + }); + + test('no doc references a deprecated config key', () => { + const hits = scanDocsForConfigKeys(); + const stale = hits.filter((h) => DEPRECATED_KEYS.has(h.key)); + if (stale.length > 0) { + console.error('Deprecated config keys referenced in docs:', stale); + } + expect(stale).toEqual([]); + }); + + test('`gstack-config get artifacts_sync_mode` returns a value (the rename landed)', () => { + // Run from a clean HOME so the user's local config doesn't pollute. + const tmpHome = fs.mkdtempSync(path.join(require('os').tmpdir(), 'gstack-cfg-')); + try { + const result = spawnSync(CONFIG_BIN, ['get', 'artifacts_sync_mode'], { + encoding: 'utf-8', + env: { ...process.env, HOME: tmpHome, GSTACK_HOME: tmpHome }, + timeout: 5000, + }); + expect(result.status).toBe(0); + // A known key returns its default value, not the "unknown key" error string. + expect(result.stderr).not.toContain('not recognized'); + expect(result.stdout.trim().length).toBeGreaterThan(0); + } finally { + fs.rmSync(tmpHome, { recursive: true, force: true }); + } + }); +}); diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index 4bf8abeee..309fd7e4b 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -2198,7 +2198,7 @@ describe('setup script validation', () => { expect(codexSection).toContain('create_codex_runtime_root'); expect(codexSection).toContain('link_codex_skill_dirs'); expect(codexSection).not.toContain('link_claude_skill_dirs'); - expect(codexSection).not.toContain('ln -snf "$GSTACK_DIR" "$CODEX_GSTACK"'); + expect(codexSection).not.toContain('_link_or_copy "$GSTACK_DIR" "$CODEX_GSTACK"'); }); test('Codex install prefers repo-local .agents/skills when setup runs from there', () => { @@ -2238,7 +2238,8 @@ describe('setup script validation', () => { const fnEnd = setupContent.indexOf('}', setupContent.indexOf('linked[@]}', fnStart)); const fnBody = setupContent.slice(fnStart, fnEnd); expect(fnBody).toContain('mkdir -p "$target"'); - expect(fnBody).toContain('ln -snf "$gstack_dir/$dir_name/SKILL.md" "$target/SKILL.md"'); + // v1.36.0.0: routes through _link_or_copy helper for Windows fallback (cp on MSYS2/Git Bash). + expect(fnBody).toContain('_link_or_copy "$gstack_dir/$dir_name/SKILL.md" "$target/SKILL.md"'); }); // REGRESSION: cleanup functions must handle both old symlinks AND new real-directory pattern @@ -2345,7 +2346,7 @@ describe('setup script validation', () => { expect(fnBody).toContain('design-checklist.md'); expect(fnBody).toContain('greptile-triage.md'); expect(fnBody).toContain('TODOS-format.md'); - expect(fnBody).not.toContain('ln -snf "$gstack_dir" "$codex_gstack"'); + expect(fnBody).not.toContain('_link_or_copy "$gstack_dir" "$codex_gstack"'); }); test('direct Codex installs are migrated out of ~/.codex/skills/gstack', () => { diff --git a/test/setup-conductor-worktree.test.ts b/test/setup-conductor-worktree.test.ts index 6fb675afc..29609ac8f 100644 --- a/test/setup-conductor-worktree.test.ts +++ b/test/setup-conductor-worktree.test.ts @@ -8,10 +8,11 @@ const ROOT = path.resolve(import.meta.dir, '..'); const SETUP_SCRIPT = path.join(ROOT, 'setup'); describe('setup: Conductor worktree guard', () => { - test('setup contains the real-dir guard before the ln -snf into ~/.claude/skills/', () => { + test('setup contains the real-dir guard before the symlink-or-copy into ~/.claude/skills/', () => { const content = fs.readFileSync(SETUP_SCRIPT, 'utf-8'); const guardIdx = content.indexOf('_SKIP_CLAUDE_REGISTER=0'); - const lnIdx = content.indexOf('ln -snf "$SOURCE_GSTACK_DIR" "$CLAUDE_GSTACK_LINK"'); + // v1.36.0.0: symlink work routes through _link_or_copy helper for Windows fallback. + const lnIdx = content.indexOf('_link_or_copy "$SOURCE_GSTACK_DIR" "$CLAUDE_GSTACK_LINK"'); expect(guardIdx).toBeGreaterThan(-1); expect(lnIdx).toBeGreaterThan(-1); expect(guardIdx).toBeLessThan(lnIdx); diff --git a/test/setup-windows-fallback.test.ts b/test/setup-windows-fallback.test.ts new file mode 100644 index 000000000..6c3735860 --- /dev/null +++ b/test/setup-windows-fallback.test.ts @@ -0,0 +1,122 @@ +import { describe, test, expect } from 'bun:test'; +import { spawnSync } from 'child_process'; +import * as path from 'path'; +import * as fs from 'fs'; +import * as os from 'os'; + +const ROOT = path.resolve(import.meta.dir, '..'); +const SETUP_SCRIPT = path.join(ROOT, 'setup'); +const SETUP_SRC = fs.readFileSync(SETUP_SCRIPT, 'utf-8'); + +// Slice out the _link_or_copy helper body via awk-style anchors so the test is +// resilient to line-number drift. +function extractHelper(): string { + const start = SETUP_SRC.indexOf('_link_or_copy() {'); + const end = SETUP_SRC.indexOf('\n}\n', start); + if (start < 0 || end < 0) throw new Error('Could not locate _link_or_copy() in setup'); + return SETUP_SRC.slice(start, end + 2); +} + +describe('setup: _link_or_copy invariant (D7)', () => { + test('helper function is defined near the top of setup', () => { + expect(SETUP_SRC).toContain('_link_or_copy() {'); + expect(SETUP_SRC).toContain('if [ "$IS_WINDOWS" -eq 1 ]; then'); + }); + + test('zero raw `ln` calls outside the helper body and comments', () => { + // Pull the helper body out of the source first so its internal `ln -snf` + // (the Unix branch) is exempted from the invariant. + const helper = extractHelper(); + const withoutHelper = SETUP_SRC.replace(helper, ''); + + // Strip shell comments to allow prose mentions of `ln -snf` in docstrings. + const lines = withoutHelper.split('\n'); + const offending: { lineNo: number; line: string }[] = []; + lines.forEach((line, idx) => { + const trimmed = line.trim(); + if (trimmed.startsWith('#')) return; + // Match standalone `ln ` invocations (allow `ln` as a substring in + // variable names like `linked`, `_LINK`). + if (/(^|[\s;&|`])ln\s+-/.test(line)) { + offending.push({ lineNo: idx + 1, line: line.trim() }); + } + }); + expect(offending).toEqual([]); + }); + + test('Windows-copy note message exists in setup', () => { + expect(SETUP_SRC).toContain('Windows install uses file copies'); + expect(SETUP_SRC).toContain('_print_windows_copy_note_once'); + }); + + test('link_claude_skill_dirs calls the Windows note printer', () => { + const fnStart = SETUP_SRC.indexOf('link_claude_skill_dirs() {'); + const fnEnd = SETUP_SRC.indexOf('\n}\n', fnStart); + const fnBody = SETUP_SRC.slice(fnStart, fnEnd); + expect(fnBody).toContain('_print_windows_copy_note_once'); + }); +}); + +describe('setup: _link_or_copy helper — behavior matrix', () => { + // Source the helper into a temp shell with IS_WINDOWS set and exercise + // each cell of the file/dir × Windows/Unix matrix. + function runHelper( + isWindows: '0' | '1', + srcKind: 'file' | 'dir', + ): { ok: boolean; targetIsSymlink: boolean; targetExists: boolean; stderr: string } { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-helper-')); + try { + const src = path.join(tmp, 'source'); + const dst = path.join(tmp, 'dest'); + if (srcKind === 'file') { + fs.writeFileSync(src, 'hello\n'); + } else { + fs.mkdirSync(src); + fs.writeFileSync(path.join(src, 'inner.txt'), 'hello\n'); + } + const helper = extractHelper(); + // IS_WINDOWS must exist as a shell-readable var before sourcing. + const script = `IS_WINDOWS=${isWindows}\n${helper}\n_link_or_copy "${src}" "${dst}"\n`; + const result = spawnSync('bash', ['-c', script], { + encoding: 'utf-8', + timeout: 5000, + }); + const lst = fs.lstatSync(dst, { throwIfNoEntry: false }); + return { + ok: result.status === 0, + targetIsSymlink: lst?.isSymbolicLink() ?? false, + targetExists: lst !== undefined, + stderr: result.stderr, + }; + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + } + + test('IS_WINDOWS=0 + file → symlink (existing Unix behavior)', () => { + const r = runHelper('0', 'file'); + expect(r.ok).toBe(true); + expect(r.targetExists).toBe(true); + expect(r.targetIsSymlink).toBe(true); + }); + + test('IS_WINDOWS=0 + dir → symlink', () => { + const r = runHelper('0', 'dir'); + expect(r.ok).toBe(true); + expect(r.targetIsSymlink).toBe(true); + }); + + test('IS_WINDOWS=1 + file → regular file copy (no symlink)', () => { + const r = runHelper('1', 'file'); + expect(r.ok).toBe(true); + expect(r.targetExists).toBe(true); + expect(r.targetIsSymlink).toBe(false); + }); + + test('IS_WINDOWS=1 + dir → real directory copy', () => { + const r = runHelper('1', 'dir'); + expect(r.ok).toBe(true); + expect(r.targetExists).toBe(true); + expect(r.targetIsSymlink).toBe(false); + }); +});