diff --git a/CHANGELOG.md b/CHANGELOG.md index 38e22797d..1e33c04f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,8 +5,8 @@ **Windows: gstack's record bins stop losing review, question, learning and timeline records when Git Bash path conversion is off (#3415).** Since GSTA-56 every shell bin started Bun with `--config=/dev/null`. Native `bun.exe` cannot open `/dev/null`: it exits 1 with `EINVAL` on Bun 1.3.10, 1.4.2 and 1.4.4 (measured on windows-latest). Git Bash hides this by rewriting the argument to `nul`, but only while MSYS path conversion is on. With it off (`MSYS_NO_PATHCONV` set to any value, even empty; `MSYS2_ARG_CONV_EXCL=*`; or Cygwin), `gstack-review-log`, `gstack-question-log` and `gstack-learnings-log` exited 1 with nothing written, and `gstack-timeline-log` exited 0 with nothing written. ### Fixed -- **40 shipped shell files** (the bash bins, the Claude hook wrappers, `setup`, `scripts/build-cso.sh` and one migration) define `GSTACK_BUN_CONFIG` once and start Bun with `--config="${GSTACK_BUN_CONFIG:-/dev/null}"`. The value is `NUL` only when `OSTYPE` is `msys` or `cygwin` (Git Bash, MSYS2 and Cygwin, the check `gstack-doctor` already uses) and `cygpath` is on `PATH`; everywhere else it is `/dev/null`. The `cygpath` check is a builtin `command -v`, so it forks nothing. It means an `OSTYPE=msys` inherited by a Linux or macOS bash keeps `/dev/null`, instead of passing a relative `NUL` that Bun there resolves to a file in the repository. Bun 1.4.2 and later, gstack's minimum, read `NUL` as an empty config whether or not conversion is on. The `:-/dev/null` default covers a call that loses its definition: it still passes `/dev/null`, so POSIX is unchanged and Windows fails loudly as before, and never reads the repository's `bunfig.toml`. `bin/gstack-codex-probe` is sourced into the caller's shell, so it defines the variable inside the one function that starts Bun and still runs nothing at source time. -- **`bin/gstack-timeline-log`** and **`gstack-developer-profile --log-session`**: invalid input still exits 0 with nothing logged (the hook-path contract). Their validators now exit 3 for invalid input. Any other non-zero exit, which is Bun itself failing, is reported on stderr and returned instead of being skipped as invalid. +- **40 shipped shell files** (the bash bins, the Claude hook wrappers, `setup`, `scripts/build-cso.sh` and one migration) define `GSTACK_BUN_CONFIG` once and start Bun with `--config="${GSTACK_BUN_CONFIG:-/dev/null}"`. The value is `NUL` only when `OSTYPE` is `msys` or `cygwin` (Git Bash, MSYS2 and Cygwin, the check `gstack-doctor` already uses) and `cygpath` is on `PATH`; everywhere else it is `/dev/null`. The `cygpath` check is a builtin `command -v`, so it forks nothing. It means an `OSTYPE=msys` inherited by a Linux or macOS bash keeps `/dev/null`, instead of passing a relative `NUL` that Bun there resolves to a file in the repository. Bun 1.4.2 and later, the version gstack tests against, read `NUL` as an empty config whether or not conversion is on. Bun 1.3.10 on Windows exits 1 on `--config=NUL` and on the lowercase `nul` Git Bash used to produce alike (measured), so 1.3.x installs, which setup and `gstack-doctor` only warn about, fail the same way they did before. The `:-/dev/null` default covers a call that loses its definition: it still passes `/dev/null`, so POSIX is unchanged and Windows fails loudly as before, and never reads the repository's `bunfig.toml`. `bin/gstack-codex-probe` is sourced into the caller's shell, so it defines the variable inside the one function that starts Bun and still runs nothing at source time. +- **`bin/gstack-timeline-log`** and **`gstack-developer-profile --log-session`**: invalid input still exits 0 with nothing logged (the hook-path contract). Their validators now print `valid` or `invalid`, and any non-zero exit, which is Bun itself failing, is reported on stderr and returned instead of being skipped as invalid. The verdict is not an exit code because a Bun crash on Windows exits 3. When stamping a missing `ts` fails, `gstack-timeline-log` keeps the original event instead of appending an empty line. Their production callers (`gstack-skill-start`, `gstack-skill-end` and the `/office-hours` templates) still discard stderr and the exit status, so the report shows only when the bin is run directly. - Unchanged: bun shebangs and skill prose keep `--config=/dev/null`, because a shebang cannot name a per-platform path. TypeScript spawns keep `bunIsolationArgs()`, and `scripts/ubicloud/` keeps `/dev/null` because it runs on Linux. - Not covered: callers that hand `bun.exe` a POSIX script path built from `pwd` or `$HOME` (the Claude hook wrappers, `autoplan/bin/phase-publication-hook`, `plan-ceo-review/bin/mode-handoff-hook`, and some `gstack-doctor`, `gstack-codex-probe`, `setup` and migration calls) only got the config spelling. With path conversion off they may still fail on the `/c/...` script path itself. The record bins convert their own paths with `cygpath -m`; the others were not verified on Windows with conversion off. @@ -16,15 +16,16 @@ - On POSIX hosts a simulated Windows bash (`OSTYPE=msys`, a `cygpath`, and a `bun` stand-in that rejects `/dev/null` with EINVAL as `bun.exe` does) reproduces the bug: on main the four bins write nothing and the timeline control exits 0 (5 failures); with the fix they pass. The controls, without `OSTYPE=msys`, show each bin exits non-zero with nothing written. - An inherited `OSTYPE=msys` with no `cygpath`, in a repository that commits a file named `NUL` with a preload, leaves each bin on `/dev/null`: the record is written and the preload never runs. This fails on the first version of this fix (4 failures). - `gstack-developer-profile --log-session` reports a rejected config on stderr and exits non-zero, and still skips invalid input silently. This fails on main. + - A `bun` that exits 3 with no output, the way a Bun crash exits on Windows, makes both bins report and exit non-zero instead of skipping the event as invalid input. This fails with an exit-code verdict. - `test/bun-isolation-launch.test.ts`: the shell-file spelling stays isolated with conversion off, and a win32-only control shows that the literal `--config=/dev/null` fails there with EINVAL. -- `test/bun-entry-isolation.test.ts`: shipped shell files must use the fail-closed spelling and define `GSTACK_BUN_CONFIG` with the canonical line before first use. Skill sources keep the literal spelling. +- `test/bun-entry-isolation.test.ts`: shipped shell files must use the fail-closed spelling and define `GSTACK_BUN_CONFIG` with the canonical line before first use. A `local` definition covers only the rest of its function. Skill sources keep the literal spelling. - `test/learnings-injection.test.ts`, `browse/test/learnings-injection.test.ts` and `test/setup-hook-canonical-paths.test.ts` now anchor on the new spelling. The two learnings-injection files also check that the anchored `bun -e` block exists. Before, a missing anchor sliced the script's last character and the negative assertions passed without checking anything. ### Gate integrity - `test/bun-entry-isolation.test.ts` is strengthened. On POSIX the required shell spelling still resolves to `/dev/null`. - Now offenders: the literal `--config=/dev/null` in a shipped shell file, and an undefined or redefined `GSTACK_BUN_CONFIG`. - The shell-caller detector also skips a `--config` value before deciding whether a call starts a program, so `bun --no-env-file --config= -e` no longer passes unchecked. The tree has no such call. - - Negative controls, each of which fails the check: the old spelling, a variable without the default, an empty `--config=`, a missing, late or altered definition, and a reverted bin. + - Negative controls, each of which fails the check: the old spelling, a variable without the default, an empty `--config=`, a missing, late or altered definition, a `local` definition used from another function, and a reverted bin. - No threshold, budget, timeout, trial count or case kind changed. ## [1.91.223.0] - 2026-10-25 diff --git a/bin/gstack-developer-profile b/bin/gstack-developer-profile index 2380beb44..5e004d453 100755 --- a/bin/gstack-developer-profile +++ b/bin/gstack-developer-profile @@ -186,17 +186,22 @@ do_log_session() { fi # Validate: input must be parseable JSON with required fields (date, mode). - # Invalid input exits 3; any other non-zero exit means Bun itself failed. - local VALIDATE_RC=0 - printf '%s' "$INPUT" | bun --no-env-file --config="${GSTACK_BUN_CONFIG:-/dev/null}" -e " + # The validator prints its verdict; a non-zero exit means Bun itself failed. + local VALIDATE_RC=0 VALIDATE_OUT + VALIDATE_OUT=$(printf '%s' "$INPUT" | bun --no-env-file --config="${GSTACK_BUN_CONFIG:-/dev/null}" -e " + const text = await Bun.stdin.text(); let j; - try { j = JSON.parse(await Bun.stdin.text()); } catch { process.exit(3); } - if (!j || !j.date || !j.mode) process.exit(3); - " || VALIDATE_RC=$? - case "$VALIDATE_RC" in - 0) ;; - 3) return 0 ;; - *) echo "gstack-developer-profile: bun exited $VALIDATE_RC before the session was validated; nothing was logged" >&2; return "$VALIDATE_RC" ;; + try { j = JSON.parse(text); } catch {} + console.log(j && j.date && j.mode ? 'valid' : 'invalid'); + ") || VALIDATE_RC=$? + if [ "$VALIDATE_RC" -ne 0 ]; then + echo "gstack-developer-profile: bun exited $VALIDATE_RC before the session was validated; nothing was logged" >&2 + return "$VALIDATE_RC" + fi + case "$VALIDATE_OUT" in + valid) ;; + invalid) return 0 ;; + *) echo "gstack-developer-profile: bun printed no verdict while validating the session; nothing was logged" >&2; return 1 ;; esac ensure_profile diff --git a/bin/gstack-timeline-log b/bin/gstack-timeline-log index 894c18082..ab6eabff1 100755 --- a/bin/gstack-timeline-log +++ b/bin/gstack-timeline-log @@ -20,27 +20,33 @@ mkdir -p "$GSTACK_HOME/projects/$SLUG" INPUT="$1" -# Validate: input must be parseable JSON with required fields. Invalid input -# exits 3; any other non-zero exit means Bun itself failed (#3415). +# Validate: input must be parseable JSON with required fields. The validator +# prints its verdict; a non-zero exit means Bun itself failed (#3415). An exit +# code cannot carry the verdict: a Bun crash on Windows exits 3. VALIDATE_RC=0 -printf '%s' "$INPUT" | bun --no-env-file --config="${GSTACK_BUN_CONFIG:-/dev/null}" -e " +VALIDATE_OUT=$(printf '%s' "$INPUT" | bun --no-env-file --config="${GSTACK_BUN_CONFIG:-/dev/null}" -e " + const text = await Bun.stdin.text(); let j; - try { j = JSON.parse(await Bun.stdin.text()); } catch { process.exit(3); } - if (!j || !j.skill || !j.event) process.exit(3); -" || VALIDATE_RC=$? -case "$VALIDATE_RC" in - 0) ;; - 3) exit 0 ;; # skip silently, non-blocking - *) echo "gstack-timeline-log: bun exited $VALIDATE_RC before the event was validated; nothing was logged" >&2; exit "$VALIDATE_RC" ;; + try { j = JSON.parse(text); } catch {} + console.log(j && j.skill && j.event ? 'valid' : 'invalid'); +") || VALIDATE_RC=$? +if [ "$VALIDATE_RC" -ne 0 ]; then + echo "gstack-timeline-log: bun exited $VALIDATE_RC before the event was validated; nothing was logged" >&2 + exit "$VALIDATE_RC" +fi +case "$VALIDATE_OUT" in + valid) ;; + invalid) exit 0 ;; # skip silently, non-blocking + *) echo "gstack-timeline-log: bun printed no verdict while validating the event; nothing was logged" >&2; exit 1 ;; esac # Inject timestamp if not present if ! printf '%s' "$INPUT" | bun --no-env-file --config="${GSTACK_BUN_CONFIG:-/dev/null}" -e "const j=JSON.parse(await Bun.stdin.text()); if(!j.ts) process.exit(1)" 2>/dev/null; then - INPUT=$(printf '%s' "$INPUT" | bun --no-env-file --config="${GSTACK_BUN_CONFIG:-/dev/null}" -e " + STAMPED=$(printf '%s' "$INPUT" | bun --no-env-file --config="${GSTACK_BUN_CONFIG:-/dev/null}" -e " const j = JSON.parse(await Bun.stdin.text()); j.ts = new Date().toISOString(); console.log(JSON.stringify(j)); - " 2>/dev/null) || true + " 2>/dev/null) && [ -n "$STAMPED" ] && INPUT=$STAMPED || true fi echo "$INPUT" >> "$GSTACK_HOME/projects/$SLUG/timeline.jsonl" diff --git a/test/bun-config-shell-bins.test.ts b/test/bun-config-shell-bins.test.ts index a8d60f577..9eefc12eb 100644 --- a/test/bun-config-shell-bins.test.ts +++ b/test/bun-config-shell-bins.test.ts @@ -33,6 +33,7 @@ let stateRoot = ''; let evilRoot = ''; let preloadMarker = ''; let fakeBun = ''; +let crashBun = ''; const BINS: Array<{ bin: string; input: string; log: string }> = [ { bin: 'gstack-review-log', log: 'main-reviews.jsonl', input: JSON.stringify({ skill: 'plan-eng-review', timestamp: '2026-10-25T00:00:00Z', status: 'clean', unresolved: 0, critical_gaps: 0, issues_found: 0, mode: 'FULL_REVIEW', commit: 'abc1234' }) }, @@ -64,6 +65,7 @@ function records(log: string): string[] { function resetState(): void { fs.rmSync(stateRoot, { recursive: true, force: true }); + fs.rmSync(preloadMarker, { force: true }); fs.mkdirSync(stateRoot, { recursive: true }); } @@ -101,6 +103,9 @@ beforeAll(() => { `exec ${JSON.stringify(process.execPath)} "\${a[@]}"`, '', ].join('\n')); + crashBun = path.join(tmp, 'crash-bun'); + fs.mkdirSync(crashBun); + writeExec(path.join(crashBun, 'bun'), '#!/usr/bin/env bash\nexit 3\n'); writeExec(path.join(fakeBun, 'cygpath'), '#!/usr/bin/env bash\nprintf \'%s\\n\' "${@: -1}"\n'); }); @@ -152,6 +157,17 @@ describe.skipIf(IS_WINDOWS)('#3415: simulated Windows bash (POSIX hosts)', () => } }); + test('a Bun that dies with exit 3 (how a Bun crash exits on Windows) is reported, never read as invalid input', () => { + resetState(); + const timeline = run('gstack-timeline-log', JSON.stringify({ skill: 'review', event: 'started' }), crashBun); + expect(timeline.status).not.toBe(0); + expect(timeline.stderr).toContain('bun exited 3'); + expect(records('timeline.jsonl')).toEqual([]); + const profile = run('gstack-developer-profile', '--log-session', crashBun, {}, [JSON.stringify({ date: '2026-10-25', mode: 'startup' })]); + expect(profile.status).not.toBe(0); + expect(profile.stderr).toContain('bun exited 3'); + }); + for (const { bin, input, log } of BINS) { test(`${bin} passes NUL and writes its record`, () => { resetState(); diff --git a/test/bun-entry-isolation.test.ts b/test/bun-entry-isolation.test.ts index 64b35ce4a..5b53d30e9 100644 --- a/test/bun-entry-isolation.test.ts +++ b/test/bun-entry-isolation.test.ts @@ -76,15 +76,19 @@ const SHELL_EXEMPT: Record = { /** Run only on Linux (the Ubicloud VM or its Linux-only local fallback), where /dev/null is the spelling; remote command strings expand there. */ const SHELL_LINUX_ONLY = (f: string) => f.startsWith('scripts/ubicloud/'); -/** Lines of shell text that use GSTACK_BUN_CONFIG with no CONFIG_DEF line before them, or define it any other way. */ +/** Lines of shell text that use GSTACK_BUN_CONFIG with no CONFIG_DEF line before them, or define it any other way. A `local` definition covers only the rest of its function. */ function configDefinitionGaps(text: string): string[] { const gaps: string[] = []; let defined = false; + let definedInFunction = false; text.split('\n').forEach((line, i) => { if (/^\s*(?:local\s+)?GSTACK_BUN_CONFIG=/.test(line)) { - if (line.trim().replace(/^local\s+/, '') === CONFIG_DEF) defined = true; - else gaps.push(`${i + 1}: redefined: ${line.trim()}`); - } else if (!defined && line.includes('${GSTACK_BUN_CONFIG')) gaps.push(`${i + 1}: used before CONFIG_DEF`); + const local = /^\s*local\s/.test(line); + if (line.trim().replace(/^local\s+/, '') !== CONFIG_DEF) gaps.push(`${i + 1}: redefined: ${line.trim()}`); + else if (local) definedInFunction = true; + else defined = true; + } else if (/^}/.test(line)) definedInFunction = false; + else if (!defined && !definedInFunction && line.includes('${GSTACK_BUN_CONFIG')) gaps.push(`${i + 1}: used before CONFIG_DEF`); }); return gaps; } @@ -154,6 +158,7 @@ describe('GSTA-56: gstack never takes configuration from the audited repository' const use = `bun ${SHELL_FLAGS} -e 1`; expect(configDefinitionGaps(`#!/usr/bin/env bash\n${CONFIG_DEF}\n${use}\n`)).toEqual([]); expect(configDefinitionGaps(`f() {\n local ${CONFIG_DEF}\n ${use}\n}\n`)).toEqual([]); + expect(configDefinitionGaps(`f() {\n local ${CONFIG_DEF}\n}\ng() {\n ${use}\n}\n`)).toEqual(['5: used before CONFIG_DEF']); expect(configDefinitionGaps(`#!/usr/bin/env bash\n${use}\n`)).toEqual(['2: used before CONFIG_DEF']); expect(configDefinitionGaps(`${use}\n${CONFIG_DEF}\n`)).toEqual(['1: used before CONFIG_DEF']); expect(configDefinitionGaps(`GSTACK_BUN_CONFIG=./bunfig.toml\n${use}\n`)).toEqual(['1: redefined: GSTACK_BUN_CONFIG=./bunfig.toml', '2: used before CONFIG_DEF']);