From 9f4e8eef4811f2a730aa08ffa1d87d6cd110a046 Mon Sep 17 00:00:00 2001 From: Shreshth Kapoor Date: Sat, 29 Aug 2026 04:38:19 +0530 Subject: [PATCH] fix: bin writers drop data on Windows paths with an apostrophe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent Windows git-bash bugs in the bin writers, both silent because callers invoke these scripts with 2>/dev/null and do not check the exit status — a hard failure was indistinguishable from success. Bug 1 — apostrophe in the checkout path breaks the bun -e program. gstack-learnings-log, gstack-question-log and gstack-telemetry-log build a bun -e program as a double-quoted shell string and interpolate SCRIPT_DIR into a single-quoted JS import specifier. A path such as C:/Users/Someone's PC/... closes the JS string literal early and Bun fails to parse ("Expected ; but found s"). Every learning write and every plan-tune question event no-oped; telemetry error redaction fell to its fail-closed null path. The #1950 cygpath -m guard did not cover this — cygpath normalises the drive form but does not remove the apostrophe. Fixed by not interpolating the path at all: cd into the module root and use a relative import specifier, which is immune to apostrophes, spaces, backslashes and MSYS paths alike. The one remaining interpolated data path in gstack-developer-profile (readFileSync of PROFILE_FILE) is passed via the environment instead, matching do_log_session in the same file. Bug 2 — gstack-developer-profile --derive fails on an MSYS-form GSTACK_HOME. GSTACK_HOME defaults to $HOME/.gstack, which under git-bash is /c/Users/..., and Bun on Windows cannot open that form (ENOENT). This script carried no cygpath guard at all. Fixed by normalising GSTACK_HOME once, before PROFILE_FILE / LEGACY_FILE / the events path are derived from it, so all three pick up the normalised value. Adds test/hostile-path-writers.test.ts, which runs the bins from a directory whose name contains an apostrophe and asserts that rows are ACTUALLY WRITTEN (not merely that the exit code is 0 — exit-code-only checks are what masked bug 1). The apostrophe repro is OS-independent: SCRIPT_DIR derives from the script's own location, so a copied checkout under a hostile directory name reproduces bug 1 on Linux/macOS CI too. Wave-amended: all four writers unified on the env-var import pattern the PR already used in gstack-developer-profile (no CWD-dependent module resolution) Wave-amended: all four writers unified on the env-var import pattern the PR already used in gstack-developer-profile (apostrophe-safe without CWD-dependent module resolution); import-shape pin updated --- bin/gstack-developer-profile | 9 ++- bin/gstack-learnings-log | 4 +- bin/gstack-question-log | 4 +- bin/gstack-telemetry-log | 4 +- test/gstack-question-log.test.ts | 8 ++- test/hostile-path-writers.test.ts | 98 +++++++++++++++++++++++++++++++ 6 files changed, 119 insertions(+), 8 deletions(-) create mode 100644 test/hostile-path-writers.test.ts diff --git a/bin/gstack-developer-profile b/bin/gstack-developer-profile index a5b1ab771..57ba9adb9 100755 --- a/bin/gstack-developer-profile +++ b/bin/gstack-developer-profile @@ -30,6 +30,13 @@ SCRIPT_DIR="$(cd "$(dirname "$0")" && pwd)" ROOT_DIR="$(cd "$SCRIPT_DIR/.." && pwd)" # GSTACK_STATE_ROOT takes precedence over GSTACK_HOME (test isolation per D16). GSTACK_HOME="${GSTACK_STATE_ROOT:-${GSTACK_HOME:-$HOME/.gstack}}" +# Windows git-bash: GSTACK_HOME resolves to an MSYS path (/c/Users/...), which +# Bun on Windows cannot open as a filesystem path (bites --derive). Normalize +# once, here, before PROFILE_FILE/LEGACY_FILE are derived from it below — +# doing it after would leave those two on the stale MSYS form. +case "$(uname -s)" in + MINGW*|MSYS*|CYGWIN*) command -v cygpath >/dev/null 2>&1 && GSTACK_HOME="$(cygpath -m "$GSTACK_HOME")" ;; +esac PROFILE_FILE="$GSTACK_HOME/developer-profile.json" LEGACY_FILE="$GSTACK_HOME/builder-profile.jsonl" eval "$("$SCRIPT_DIR/gstack-slug" 2>/dev/null || true)" @@ -115,7 +122,7 @@ do_migrate() { mv "$LEGACY_FILE" "$LEGACY_FILE.migrated-$TS" local COUNT - COUNT=$(bun -e "console.log(JSON.parse(require('fs').readFileSync('$PROFILE_FILE','utf-8')).sessions.length)" 2>/dev/null || echo "?") + COUNT=$(PROFILE_FILE_PATH="$PROFILE_FILE" bun -e "console.log(JSON.parse(require('fs').readFileSync(process.env.PROFILE_FILE_PATH,'utf-8')).sessions.length)" 2>/dev/null || echo "?") echo "MIGRATE: ok — migrated $COUNT sessions from builder-profile.jsonl" } diff --git a/bin/gstack-learnings-log b/bin/gstack-learnings-log index 8c946a2e4..3c47ebb80 100755 --- a/bin/gstack-learnings-log +++ b/bin/gstack-learnings-log @@ -25,8 +25,8 @@ INPUT="$1" TMPERR=$(mktemp) trap 'rm -f "$TMPERR"' EXIT set +e -VALIDATED=$(printf '%s' "$INPUT" | bun -e " -import { hasInjection } from '$SCRIPT_DIR/../lib/jsonl-store.ts'; +VALIDATED=$(printf '%s' "$INPUT" | GSTACK_LIB_DIR="$SCRIPT_DIR/../lib" bun -e " +const { hasInjection } = await import(process.env.GSTACK_LIB_DIR + '/jsonl-store.ts'); const raw = await Bun.stdin.text(); let j; try { j = JSON.parse(raw); } catch { process.stderr.write('gstack-learnings-log: invalid JSON, skipping\n'); process.exit(1); } diff --git a/bin/gstack-question-log b/bin/gstack-question-log index f1b6010e0..ea1afe1e3 100755 --- a/bin/gstack-question-log +++ b/bin/gstack-question-log @@ -44,8 +44,8 @@ INPUT="$1" TMPERR=$(mktemp) trap 'rm -f "$TMPERR"' EXIT set +e -VALIDATED=$(printf '%s' "$INPUT" | bun -e " -import { hasInjection } from '$SCRIPT_DIR/../lib/jsonl-store.ts'; +VALIDATED=$(printf '%s' "$INPUT" | GSTACK_LIB_DIR="$SCRIPT_DIR/../lib" bun -e " +const { hasInjection } = await import(process.env.GSTACK_LIB_DIR + '/jsonl-store.ts'); const path = require('path'); const raw = await Bun.stdin.text(); let j; diff --git a/bin/gstack-telemetry-log b/bin/gstack-telemetry-log index 20febd32e..05d9c9866 100755 --- a/bin/gstack-telemetry-log +++ b/bin/gstack-telemetry-log @@ -192,8 +192,8 @@ ERR_FIELD="null" # look like a JSON string, the whole message becomes null — never raw. ERR_MSG_FIELD="null" if [ -n "$ERROR_MESSAGE" ]; then - ERR_MSG_FIELD="$(printf '%s' "$ERROR_MESSAGE" | bun -e " -import { redactFindingSpans } from '$SCRIPT_DIR/../lib/redact-engine.ts'; + ERR_MSG_FIELD="$(printf '%s' "$ERROR_MESSAGE" | GSTACK_LIB_DIR="$SCRIPT_DIR/../lib" bun -e " +const { redactFindingSpans } = await import(process.env.GSTACK_LIB_DIR + '/redact-engine.ts'); const input = await Bun.stdin.text(); const out = redactFindingSpans(input, { repoVisibility: 'private' }); if (out === null) process.exit(1); diff --git a/test/gstack-question-log.test.ts b/test/gstack-question-log.test.ts index 2be6772bc..be25a6712 100644 --- a/test/gstack-question-log.test.ts +++ b/test/gstack-question-log.test.ts @@ -284,7 +284,13 @@ describe('gstack-question-log — injection defense', () => { describe('gstack-question-log — shared injection patterns (#1934 dedup)', () => { test('imports hasInjection from lib/jsonl-store.ts instead of a local duplicate', () => { const source = fs.readFileSync(BIN, 'utf-8'); - expect(source).toContain("import { hasInjection } from '$SCRIPT_DIR/../lib/jsonl-store.ts'"); + // #2720 absorption: the lib path travels via env var (apostrophe-safe — + // shell interpolation into a JS string literal broke on paths containing + // '), so the import is dynamic. The invariant is unchanged: the shared + // audited hasInjection from lib/jsonl-store.ts, never a local duplicate. + expect(source).toContain( + "const { hasInjection } = await import(process.env.GSTACK_LIB_DIR + '/jsonl-store.ts');", + ); expect(source).not.toContain('const INJECTION_PATTERNS'); }); }); diff --git a/test/hostile-path-writers.test.ts b/test/hostile-path-writers.test.ts new file mode 100644 index 000000000..3379b6584 --- /dev/null +++ b/test/hostile-path-writers.test.ts @@ -0,0 +1,98 @@ +import { describe, test, expect, beforeAll, afterAll } from 'bun:test'; +import * as fs from 'fs'; +import * as path from 'path'; +import * as os from 'os'; +import { spawnSync } from 'child_process'; + +/** + * Regression tests for the two Windows path bugs in the bin writers: + * + * 1. A checkout path containing an apostrophe used to terminate the JS + * single-quoted string literal that `bun -e` programs interpolated + * SCRIPT_DIR into (gstack-learnings-log, gstack-question-log, + * gstack-telemetry-log, gstack-developer-profile). The scripts exited 1 + * but callers invoke them with 2>/dev/null, so every learning and every + * plan-tune question event was dropped with no visible error. + * + * 2. gstack-developer-profile passed an MSYS-form GSTACK_HOME (/c/Users/...) + * to Bun, which cannot open it — --derive always failed ENOENT on + * Windows git-bash. + * + * The apostrophe repro is OS-independent: SCRIPT_DIR derives from the + * script's own location, so running the bins from a copied checkout under a + * hostile directory name reproduces bug 1 on Linux/macOS CI too. + * + * These tests assert rows are ACTUALLY WRITTEN, not merely that the exit + * code is 0 — exit-code-only assertions are exactly what masked bug 1. + */ + +const HOSTILE = path.join(os.tmpdir(), "gstack o'brien test"); +const STATE = path.join(HOSTILE, 'state'); +const REPO = path.resolve(import.meta.dir, '..'); + +function runBin(bin: string, args: string[], env: Record = {}) { + // Invoke through bash explicitly: the bins are shell scripts, and Windows + // cannot exec a shebang script directly (spawn would fail before the code + // under test ever ran). + const r = spawnSync('bash', [path.join(HOSTILE, 'bin', bin), ...args], { + encoding: 'utf-8', + env: { ...process.env, GSTACK_HOME: STATE, GSTACK_STATE_ROOT: '', ...env }, + shell: false, + }); + return { status: r.status, stdout: r.stdout ?? '', stderr: r.stderr ?? '' }; +} + +beforeAll(() => { + fs.rmSync(HOSTILE, { recursive: true, force: true }); + fs.mkdirSync(STATE, { recursive: true }); + // The bins resolve SCRIPT_DIR from their own location and import ../lib and + // ../scripts relative to it, so copy all three alongside each other. + for (const dir of ['bin', 'lib', 'scripts']) { + fs.cpSync(path.join(REPO, dir), path.join(HOSTILE, dir), { recursive: true }); + } +}); + +afterAll(() => { + fs.rmSync(HOSTILE, { recursive: true, force: true }); +}); + +describe('bin writers under a path containing an apostrophe', () => { + test('gstack-learnings-log appends a row (not just exit 0)', () => { + const r = runBin('gstack-learnings-log', [ + JSON.stringify({ + skill: 't', type: 'tool', key: 'hostile-path-probe', + insight: 'row must land even under a hostile checkout path', + confidence: 5, source: 'observed', + }), + ]); + expect(r.status).toBe(0); + expect(r.stderr ?? '').not.toContain('Expected ";"'); + + const projects = path.join(STATE, 'projects'); + const rows: string[] = []; + for (const slug of fs.readdirSync(projects)) { + const f = path.join(projects, slug, 'learnings.jsonl'); + if (fs.existsSync(f)) rows.push(...fs.readFileSync(f, 'utf-8').trim().split('\n')); + } + const parsed = rows.map((l) => JSON.parse(l)); + expect(parsed.some((j) => j.key === 'hostile-path-probe')).toBe(true); + }); + + test('gstack-question-log gets past module resolution to its own validation', () => { + // An intentionally incomplete event: reaching the field-validation error + // proves the bun -e program parsed and ran, which is the regression under + // test. (A full happy-path event would couple this test to the question + // registry's required fields.) + const r = runBin('gstack-question-log', [ + JSON.stringify({ skill: 't', question_id: 'hostile-path-probe', user_choice: 'a' }), + ]); + expect(r.stderr ?? '').not.toContain('Expected ";"'); + expect(r.stderr ?? '').not.toContain('Cannot find module'); + }); + + test('gstack-developer-profile --derive resolves GSTACK_HOME for Bun', () => { + const r = runBin('gstack-developer-profile', ['--derive']); + expect(r.stdout + r.stderr).not.toContain('ENOENT'); + expect(r.stdout).toContain('DERIVE: ok'); + }); +});