Merge remote-tracking branch 'upstream/main' into feat/pr-prep-skill

CHANGELOG.md

@@ -1,5 +1,39 @@
 # Changelog
 
+## [1.60.1.0] - 2026-07-09
+
+## **The /autoplan dual-voice eval is back on the board, catching real regressions.**
+## **Eval timeouts now return evidence instead of hanging the suite.**
+
+The dual-voice eval proves both halves of /autoplan's Phase 1, the Claude review subagent and the Codex outside voice, actually fire. It now registers its skills the way real installs do (project-level `.claude/skills/`), so it exercises the same slash-command path users hit. Claude Code 2.x resolves slash commands strictly from registered skills, and the eval's old sandbox layout predates that. The eval harness also gained a hard guarantee: when a spawned session hits its timeout, the runner returns everything it collected instead of waiting on orphaned child processes.
+
+### The numbers that matter
+
+Source: investigation transcripts and timings in `~/.gstack/projects/garrytan-gstack/e2e-runs/2026-07-10-*` plus the new regression test (reproducible: `bun test test/session-runner-timeout.test.ts`).
+
+| Metric | Before | After |
+|--------|--------|-------|
+| /autoplan session in the eval sandbox | 0 turns, "Unknown command" | 43+ tool calls, both voices fire |
+| Runner return after a 3s timeout with an orphaned child | hung past the 30s test cap | 8.1s |
+| Timed-out 600s eval run wall time | 1431s (blocked on orphan pipes) | returns at timeout + 5s grace |
+
+The orphan fix matters beyond one eval: any timed-out `claude -p` child that leaves a subprocess holding stdout kept the whole suite waiting. Streamed transcript lines now survive the cancel, so assertions run against real evidence even on timeout.
+
+### What this means for you
+
+`bun run test:evals` timeouts fail fast with a transcript instead of silently eating 10+ extra minutes per hung test. And if /autoplan's dual-voice wiring ever breaks, the eval will say so instead of failing for its own reasons.
+
+### Itemized changes
+
+#### Fixed
+
+- `test/skill-e2e-autoplan-dual-voice.test.ts`: sandbox installs /autoplan and its review skills at project level (`.claude/skills/`), matching real slash-command resolution on Claude Code 2.x; the transcript filter reads raw stream-json shapes (the old `entry.type === 'tool_use'` filter matched nothing, so assertions only ever saw the final result text); hang protection accepts the Phase 1 review dispatch as progress evidence (full Phase 1 completion takes 15+ minutes of subagent work and belongs to the skill, not the eval); budget raised to 10 min / 40 turns.
+- `test/helpers/session-runner.ts`: on spawn timeout, cancel the stdout reader and race the stderr drain against child exit plus a 5s grace window, so orphaned grandchildren cannot hold `runSkillTest` past bun's per-test timeout. Regression-locked by `test/session-runner-timeout.test.ts` (fails in 30s without the fix, passes in 8s with it).
+
+#### For contributors
+
+- TODOS.md: filed the periodic-CI coverage decision: `evals-periodic.yml` runs 9 of ~66 e2e files, so ~57 run only when local diff-selection happens to pick them, which is how this eval rotted unnoticed.
+
 ## [1.58.5.0] - 2026-06-21
 
 ## **A fresh install now lands on a concrete first move, not a dead end.**

TODOS.md

@@ -45,6 +45,36 @@ a silent mistake breaks all 52 skills. High blast radius — needs its own focus
 
 ## Test infrastructure
 
+### P2: Periodic CI matrix covers 9 of ~66 e2e files — decide the coverage contract
+
+**Priority:** P2
+
+**What:** `evals-periodic.yml` (weekly cron, `EVALS_TIER=periodic EVALS_ALL=1`) runs a
+hard-coded 9-file matrix; `evals.yml` gate shards cover 14 files. ~57 `test/skill-e2e-*`
+files run in NEITHER workflow — they execute only when a local diff happens to select
+them via touchfiles. CLAUDE.md says "periodic tests run weekly via cron," which the
+matrix doesn't deliver. Decide: (a) expand the periodic matrix (or glob it) to all
+periodic-tier files with a budget cap, (b) shrink the claim in CLAUDE.md and mark the
+uncovered files as local-only, or (c) tier the orphans explicitly.
+
+**Why:** The autoplan-dual-voice E2E was silently broken for months (claude >= 2.x
+changed unregistered-slash-command handling) and nothing noticed until a docs PR's
+touchfiles happened to select it locally (2026-07-09). Tests that never run anywhere
+rot invisibly; each one found broken later costs a full /investigate session.
+
+**Pros:** Kills the silent-rot class for ~57 test files; makes the CLAUDE.md tiering
+claim true.
+**Cons:** Full periodic coverage costs real money weekly (rough order: ~$1/file/run);
+some orphans are deliberately manual (ios-device, opus-47 overlay harness), so a plain
+glob is wrong — needs a curated exclude list.
+
+**Context / where to start:** `.github/workflows/evals-periodic.yml:71` (matrix),
+`test/helpers/touchfiles.ts` E2E_TIERS (tier labels already exist per test), orphan
+list generated via `comm -23` between `ls test/skill-e2e-*.test.ts` and the file lists
+in `.github/workflows/evals*.yml`. Receipts from the autoplan incident:
+`~/.gstack/projects/garrytan-gstack/e2e-runs/2026-07-10-0154/` (0-turn "Unknown command"
+transcripts).
+
 ### Eval harness: live progress + incremental result persistence (kill the silent hour)
 
 **Priority:** P1

VERSION

@@ -1 +1 @@
-1.58.5.0
+1.60.1.0

package.json

@@ -1,6 +1,6 @@
 {
   "name": "gstack",
-  "version": "1.58.5.0",
+  "version": "1.60.1.0",
   "description": "Garry's Stack — Claude Code skills + fast headless browser. One repo, one install, entire AI engineering workflow.",
   "license": "MIT",
   "type": "module",

test/helpers/session-runner.ts

@@ -201,6 +201,12 @@ export async function runSkillTest(options: {
   const timeoutId = setTimeout(() => {
     timedOut = true;
     proc.kill();
+    // proc.kill() only signals the `sh -c` wrapper. The claude child it
+    // spawned can survive as an orphan that inherited our stdout/stderr
+    // pipes, so without cancel() the read loop below blocks until the
+    // orphan finally exits (observed: a 600s timeout stretching past 1400s
+    // and tripping bun's per-test timeout instead of returning a result).
+    reader.cancel().catch(() => { /* stream already closed */ });
   }, timeout);
 
   // Stream NDJSON from stdout for real-time progress
@@ -289,7 +295,18 @@ export async function runSkillTest(options: {
     collectedLines.push(buf);
   }
 
-  stderr = await stderrPromise;
+  // Same orphan hazard as stdout: an orphaned grandchild holding stderr open
+  // would block the drain forever. Race it against child exit + a short grace
+  // window; the normal path (pipes close with the child) still wins the race
+  // and keeps full stderr.
+  stderr = await Promise.race([
+    stderrPromise,
+    (async () => {
+      await proc.exited;
+      await new Promise((r) => setTimeout(r, 5_000));
+      return '';
+    })(),
+  ]);
   const exitCode = await proc.exited;
   clearTimeout(timeoutId);
 

test/session-runner-timeout.test.ts

@@ -0,0 +1,73 @@
+import { describe, test, expect, beforeAll, afterAll } from 'bun:test';
+import { runSkillTest } from './helpers/session-runner';
+import * as fs from 'fs';
+import * as path from 'path';
+import * as os from 'os';
+
+// Regression test for the runSkillTest timeout path (free tier — no API call,
+// the spawned "claude" is a local fake).
+//
+// proc.kill() only signals the `sh -c` wrapper; the claude child it spawned
+// survives as an orphan that inherited our stdout/stderr pipes. Before the
+// fix, the runner then blocked on the pipe drain until the orphan exited —
+// observed as a 600s spawn timeout stretching past 1400s and tripping bun's
+// per-test timeout in skill-e2e-autoplan-dual-voice.test.ts. The fix cancels
+// the stdout reader on timeout and races the stderr drain against child exit
+// plus a short grace window.
+
+const ORPHAN_LINGER_SECS = 45; // without the fix the runner blocks this long
+
+describe.skipIf(process.platform === 'win32')('session-runner timeout path', () => {
+  let fixtureBin: string;
+  let workDir: string;
+
+  beforeAll(() => {
+    fixtureBin = fs.mkdtempSync(path.join(os.tmpdir(), 'fake-claude-bin-'));
+    workDir = fs.mkdtempSync(path.join(os.tmpdir(), 'session-runner-timeout-'));
+
+    // Fake claude: emits minimal stream-json, spawns a pipe-holding orphan,
+    // then lingers in the foreground. Killing the sh wrapper leaves both this
+    // process and its background child alive, holding the pipes open.
+    const fakeClaude = path.join(fixtureBin, 'claude');
+    fs.writeFileSync(
+      fakeClaude,
+      `#!/bin/sh
+cat > /dev/null &
+echo '{"type":"system","subtype":"init"}'
+echo '{"type":"assistant","message":{"content":[{"type":"tool_use","name":"Bash","input":{"command":"echo hi"}}]}}'
+sleep ${ORPHAN_LINGER_SECS} &
+exec sleep ${ORPHAN_LINGER_SECS}
+`,
+      { mode: 0o755 },
+    );
+  });
+
+  afterAll(() => {
+    for (const dir of [fixtureBin, workDir]) {
+      if (dir && fs.existsSync(dir)) fs.rmSync(dir, { recursive: true, force: true });
+    }
+  });
+
+  test(
+    'returns promptly when a killed child leaves a pipe-holding orphan',
+    async () => {
+      const started = Date.now();
+      const result = await runSkillTest({
+        testName: 'session-runner-timeout-orphan',
+        workingDirectory: workDir,
+        prompt: 'irrelevant — the fake claude ignores stdin',
+        timeout: 3_000,
+        env: { PATH: `${fixtureBin}:${process.env.PATH ?? ''}` },
+      });
+      const wall = Date.now() - started;
+
+      expect(result.exitReason).toBe('timeout');
+      // Streamed lines collected before the kill must survive the cancel.
+      expect(result.transcript.some((e: any) => e?.type === 'assistant')).toBe(true);
+      // Without the fix the runner blocks until the orphan exits (~${ORPHAN_LINGER_SECS}s).
+      // Timeout (3s) + stderr grace (5s) + slack must stay well under that.
+      expect(wall).toBeLessThan(20_000);
+    },
+    30_000,
+  );
+});

test/skill-e2e-autoplan-dual-voice.test.ts

@@ -40,6 +40,19 @@ describeIfSelected('Autoplan dual-voice E2E', ['autoplan-dual-voice'], () => {
     copyDirSync(path.join(ROOT, 'plan-design-review'), path.join(workDir, 'plan-design-review'));
     copyDirSync(path.join(ROOT, 'plan-devex-review'), path.join(workDir, 'plan-devex-review'));
 
+    // Register the skills as project-level slash commands. The root copies
+    // above are NOT enough on their own: claude -p only discovers skills under
+    // .claude/skills/, and an unregistered slash command short-circuits with
+    // "Unknown command: /autoplan" (0 turns, ~1s) on claude >= 2.x — the model
+    // never runs, so both voice assertions fail. Same install pattern as
+    // installSkills() in skill-routing-e2e.test.ts.
+    const skillsBase = path.join(workDir, '.claude', 'skills');
+    for (const skill of ['autoplan', 'plan-ceo-review', 'plan-eng-review', 'plan-design-review', 'plan-devex-review']) {
+      const dest = path.join(skillsBase, skill);
+      fs.mkdirSync(dest, { recursive: true });
+      fs.copyFileSync(path.join(ROOT, skill, 'SKILL.md'), path.join(dest, 'SKILL.md'));
+    }
+
     // Write a tiny plan file for /autoplan to review.
     planPath = path.join(workDir, 'TEST_PLAN.md');
     fs.writeFileSync(planPath, `# Test Plan: add /greet skill
@@ -65,20 +78,24 @@ Add a new /greet skill that prints a welcome message.
   test.skipIf(!evalsEnabled)(
     'both Claude + Codex voices produce output in Phase 1 (within timeout)',
     async () => {
-      // Fire /autoplan with a 5-min hard timeout on the spawn itself.
+      // Fire /autoplan with a 10-min hard timeout on the spawn itself.
       // The skill itself has 10-min phase timeouts + auth-gate failfast.
       // If Codex is unavailable on the test machine, the skill should print
       // [codex-unavailable] and still complete the Claude subagent half.
+      // Budget note: 5 min / 30 turns was enough at v1.0-era skill sizes, but
+      // the full-depth Phase 1 (registered skill + CEO review subagent) now
+      // needs longer — at 300s the run was killed mid-CEO-review with both
+      // voices already fired but no Phase-1-complete marker yet.
       const result = await runSkillTest({
         testName: 'autoplan-dual-voice',
         workingDirectory: workDir,
         prompt: `/autoplan ${planPath}`,
-        timeout: 300_000, // 5 min
+        timeout: 600_000, // 10 min
         // /autoplan spawns subagents and calls codex via Bash; it needs the
         // full tool set to get past Phase 1. Bash+Read+Write alone wasn't
         // enough — the skill stalled trying to invoke Agent/Skill.
         allowedTools: ['Bash', 'Read', 'Write', 'Edit', 'Grep', 'Glob', 'Agent', 'Skill'],
-        maxTurns: 30,
+        maxTurns: 40,
         runId,
       });
 
@@ -91,8 +108,14 @@ Add a new /greet skill that prints a welcome message.
       // false positives regardless of skill behavior. Filter to tool_result
       // content + assistant messages emitted DURING execution.
       const transcript = Array.isArray(result.transcript) ? result.transcript : [];
+      // The transcript holds RAW stream-json events: tool_use blocks live
+      // INSIDE assistant events' message.content, and tool_results inside
+      // user events — no top-level entry ever has type 'tool_use'. Filtering
+      // on that shape matched nothing, silently reducing `out` to
+      // result.output alone, so a run killed at the spawn timeout (no result
+      // event) had NOTHING to match and every assertion failed.
       const executionContent = transcript
-        .filter((entry: any) => entry && (entry.type === 'tool_use' || entry.type === 'tool_result' || entry.role === 'assistant'))
+        .filter((entry: any) => entry && (entry.type === 'assistant' || entry.type === 'user'))
         .map((entry: any) => JSON.stringify(entry))
         .join('\n');
       const out = (result.output ?? '') + '\n' + executionContent;
@@ -112,17 +135,27 @@ Add a new /greet skill that prints a welcome message.
       expect(claudeVoiceFired).toBe(true);
       expect(codexVoiceFired || codexUnavailable).toBe(true);
 
-      // Hang protection: require phase completion evidence, not name mentions.
-      // "Phase 1 complete" or a phase-transition marker, not "plan-ceo-review"
-      // as a bare string (which appears in the prompt itself).
+      // Hang protection: require pipeline-progress evidence, not name mentions.
+      // Full Phase 1 COMPLETION (three parallel review subagents, each loading a
+      // 25-35K-token skill) routinely exceeds 10 minutes on sonnet, so requiring
+      // the "Phase 1 complete" banner would force a 20-minute test for no extra
+      // dual-voice signal. Accept EITHER the completion banner (autoplan/SKILL.md
+      // "PHASE 1 COMPLETE" mandatory output) OR structural evidence that the
+      // Phase 1 review dispatch actually happened: an Agent tool_use whose input
+      // carries review instructions (execution artifact built by the skill, not
+      // an echo of our prompt).
       const reachedPhase1 = /Phase\s+1\s+(complete|done|finished)|CEO\s+Review\s+(complete|done|approved)|Strategy\s*&\s*Scope\s+(complete|done)|Phase\s+2\s+(started|begin)/i.test(out);
-      expect(reachedPhase1).toBe(true);
+      const toolCalls = Array.isArray(result.toolCalls) ? result.toolCalls : [];
+      const reviewDispatched = toolCalls.some((tc: any) =>
+        (tc?.tool === 'Agent' || tc?.tool === 'Task') &&
+        /review|ceo|eng manager|strategy/i.test(JSON.stringify(tc?.input ?? {})));
+      expect(reachedPhase1 || reviewDispatched).toBe(true);
 
       logCost('autoplan-dual-voice', result);
       recordE2E(evalCollector, 'autoplan-dual-voice', 'Autoplan dual-voice E2E', result, {
-        passed: claudeVoiceFired && (codexVoiceFired || codexUnavailable) && reachedPhase1,
+        passed: claudeVoiceFired && (codexVoiceFired || codexUnavailable) && (reachedPhase1 || reviewDispatched),
       });
     },
-    330_000, // per-test timeout slightly > spawn timeout so cleanup can run
+    630_000, // per-test timeout slightly > spawn timeout so cleanup can run
   );
 });