mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 14:38:59 +02:00
fix: bin writers drop data on Windows paths with an apostrophe
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
This commit is contained in:
committed by
Garry Tan
parent
b8f21ff713
commit
9f4e8eef48
@@ -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"
|
||||
}
|
||||
|
||||
|
||||
@@ -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); }
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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);
|
||||
|
||||
@@ -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');
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string, string> = {}) {
|
||||
// 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');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user