fix: pre-landing review fixes, cycle 2 (#3415)

- timeline-log and developer-profile validators print their verdict; any
  non-zero Bun exit is a reported failure (a Bun crash on Windows exits 3).
- timeline-log keeps the original event when ts stamping fails.
- The definition ratchet scopes a local GSTACK_BUN_CONFIG to its function.
- The shell-bins fixture clears its preload marker per test.
- CHANGELOG states the callers that discard the report and the Bun 1.3.x case.

Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>

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=<anything> -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

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

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"

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();

test/bun-entry-isolation.test.ts

@@ -76,15 +76,19 @@ const SHELL_EXEMPT: Record<string, string> = {
 /** 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']);