From 281227262dd7f11a0208bb0f47feb02cd708475e Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 12 Aug 2026 11:01:38 -0700 Subject: [PATCH] feat(evals): seedSkills opt-in for PTY slash-command tests + tripwire MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Wire ClaudePtyOptions.seedSkills through launchClaudePty: when set (and hermetic, and no per-test CLAUDE_CONFIG_DIR override), the child gets hermeticSkillsConfigDir() so typed /skill slash commands resolve instead of dying as Unknown command before any model turn. Opted in at the three runPlanSkill* helpers and the four direct-launch slash-command tests (plan-design-with-ui, plan-ceo-mode-routing, autoplan-chain, ship-idempotency). New static tripwire (test/pty-skill-seeding-wiring.test.ts): any test file that sends a slash command over the PTY must route through a runPlanSkill* helper or pass seedSkills: true — an unseeded slash-command test spends money and measures nothing. hermetic-wiring.test.ts now blesses the repo-tree seeding path explicitly (config dir under runRoot, symlinks into the repo checkout, never operator ~/.claude). The CI "Register gstack skills for PTY smoke" step keeps a keep-me note: container cross-mount symlinks defeat the TUI scanner and HOME is not hermeticized, so the real-file copies there must survive this change. Co-Authored-By: Claude Fable 5 (cherry picked from commit 63c52269daaffb833b3105ea9b4b99be6df8fec7) --- .github/workflows/evals.yml | 8 ++ test/helpers/claude-pty-runner.ts | 13 +++- test/hermetic-wiring.test.ts | 21 ++++++ test/pty-skill-seeding-wiring.test.ts | 77 ++++++++++++++++++++ test/skill-e2e-autoplan-chain.test.ts | 1 + test/skill-e2e-plan-ceo-mode-routing.test.ts | 1 + test/skill-e2e-plan-design-with-ui.test.ts | 1 + test/skill-e2e-ship-idempotency.test.ts | 1 + 8 files changed, 122 insertions(+), 1 deletion(-) create mode 100644 test/pty-skill-seeding-wiring.test.ts diff --git a/.github/workflows/evals.yml b/.github/workflows/evals.yml index 3bb61f84b..fa911176d 100644 --- a/.github/workflows/evals.yml +++ b/.github/workflows/evals.yml @@ -183,6 +183,14 @@ jobs: # bin/ + sections/ are committed). $HOME is /github/home here; the spawned # claude inherits it (this runner adds no HOME/CLAUDE_CONFIG_DIR override, # no hermetic mode) and the Seed step already proved claude reads $HOME. + # + # KEEP THIS STEP even though seedSkills/hermeticSkillsConfigDir() now + # registers skills for hermetic PTY children: that registry is SYMLINKS + # into the repo checkout, and this container's cross-mount symlinks + # defeat the TUI skill scanner (see the note inside the step below) — + # the real-file copies here are what the TUI actually reads. HOME is + # also not hermeticized, so the absolute ~/.claude/skills/gstack/... + # preamble paths resolve through the gstack root symlink this step makes. - name: Register gstack skills for PTY smoke if: matrix.suite.name == 'e2e-pty-plan-smoke' run: | diff --git a/test/helpers/claude-pty-runner.ts b/test/helpers/claude-pty-runner.ts index 516840126..28e987397 100644 --- a/test/helpers/claude-pty-runner.ts +++ b/test/helpers/claude-pty-runner.ts @@ -24,7 +24,7 @@ import * as fs from 'fs'; import * as os from 'os'; import * as path from 'path'; -import { hermeticChildEnv, isHermeticEnabled } from './hermetic-env'; +import { hermeticChildEnv, hermeticSkillsConfigDir, isHermeticEnabled } from './hermetic-env'; /** Strip ANSI escapes for pattern-matching against visible text. */ export function stripAnsi(s: string): string { @@ -61,6 +61,11 @@ export function resolveClaudeBinary(): string | null { } export interface ClaudePtyOptions { + /** Register the repo's shipped skills in the child's user scope via + * hermeticSkillsConfigDir(). Required by any test that types a /skill + * slash command; without it hermetic claude rejects the command as + * Unknown before any model turn. No effect when EVALS_HERMETIC=0. */ + seedSkills?: boolean; /** * Permission mode for the session. * - 'plan' (default) — launches with --permission-mode plan @@ -1285,6 +1290,9 @@ export async function launchClaudePty( // Hermetic by default (test/helpers/hermetic-env.ts): operator session // context never reaches the child; per-test opts.env merges last. const childEnv = hermeticChildEnv(opts.env); + if (opts.seedSkills && hermetic && !opts.env?.CLAUDE_CONFIG_DIR) { + childEnv.CLAUDE_CONFIG_DIR = hermeticSkillsConfigDir(); + } // eslint-disable-next-line @typescript-eslint/no-explicit-any const proc = (Bun as any).spawn([claudePath, ...args], { @@ -1670,6 +1678,7 @@ export async function runPlanSkillObservation(opts: { extraArgs: opts.extraArgs, env: opts.env, model: opts.model, + seedSkills: true, }); try { @@ -1966,6 +1975,7 @@ export async function runPlanSkillCounting(opts: { timeoutMs: timeoutMs + 60_000, env: opts.env, model: opts.model, + seedSkills: true, }); const fingerprints: AskUserQuestionFingerprint[] = []; @@ -2199,6 +2209,7 @@ export async function runPlanSkillFloorCheck(opts: { timeoutMs: timeoutMs + 60_000, env: opts.env, model: opts.model, + seedSkills: true, }); try { diff --git a/test/hermetic-wiring.test.ts b/test/hermetic-wiring.test.ts index 08528586d..6ebbccbce 100644 --- a/test/hermetic-wiring.test.ts +++ b/test/hermetic-wiring.test.ts @@ -16,6 +16,8 @@ import { describe, test, expect } from 'bun:test'; import * as fs from 'fs'; import * as path from 'path'; +import * as os from 'os'; +import { getHermeticDirs, hermeticSkillsConfigDir } from './helpers/hermetic-env'; const ROOT = path.resolve(new URL(import.meta.url).pathname, '..', '..'); @@ -110,4 +112,23 @@ describe('hermetic wiring tripwire', () => { offenders.join(', '), ).toEqual([]); }); + + test('skill seeding stays under runRoot and reads the live repo tree, never operator ~/.claude', () => { + // hermeticSkillsConfigDir() is a BLESSED non-hermetic edge: it registers + // the LIVE repo tree's skills (the skills are the subject under test). + // What it must never do is hand children the operator's ~/.claude — the + // seeded CLAUDE_CONFIG_DIR lives under the hermetic runRoot, and every + // registered symlink resolves into the repo checkout. + const configDir = hermeticSkillsConfigDir(); + const { runRoot } = getHermeticDirs(); + const operatorClaude = path.join(os.homedir(), '.claude') + path.sep; + expect(configDir.startsWith(runRoot + path.sep)).toBe(true); + expect(configDir.startsWith(operatorClaude)).toBe(false); + const skillsDir = path.join(configDir, 'skills'); + for (const entry of fs.readdirSync(skillsDir)) { + const target = fs.readlinkSync(path.join(skillsDir, entry, 'SKILL.md')); + expect(target.startsWith(operatorClaude), `${entry}: symlink escapes to ${target}`).toBe(false); + expect(fs.realpathSync(target).startsWith(fs.realpathSync(ROOT) + path.sep), `${entry}: symlink outside repo: ${target}`).toBe(true); + } + }); }); diff --git a/test/pty-skill-seeding-wiring.test.ts b/test/pty-skill-seeding-wiring.test.ts new file mode 100644 index 000000000..01f5999ce --- /dev/null +++ b/test/pty-skill-seeding-wiring.test.ts @@ -0,0 +1,77 @@ +/** + * Static-grep tripwire for PTY slash-command skill seeding. Free tier — no API. + * + * The default hermetic config dir registers NO skills, so a PTY test that + * TYPES a /skill slash command against it gets "Unknown command" before any + * model turn — the test still runs, still spends money, and measures nothing. + * Every test file that sends a slash command over the PTY must therefore + * either route through a runPlanSkill* helper (which opts in for you) or pass + * `seedSkills: true` in its own launchClaudePty options. + * + * Pattern mirrors test/hermetic-wiring.test.ts: read sources as text, assert + * invariants on their contents. Brittle by design — changing the seeding + * wiring must force the author to look here. + */ + +import { describe, test, expect } from 'bun:test'; +import * as fs from 'fs'; +import * as path from 'path'; + +const ROOT = path.resolve(new URL(import.meta.url).pathname, '..', '..'); + +/** A PTY send whose payload starts with a slash command (`/name`, optionally + * followed by `\r`, whitespace, or the closing quote). A second slash right + * after the name (a file path like '/tmp/x') does NOT match. */ +const SLASH_SEND = /\.send\(\s*(['"`])\/[a-z][a-z0-9-]*(\\r|\s|\1)/; + +const RUN_PLAN_HELPER = /\brunPlanSkill(Observation|Counting|FloorCheck)\s*\(/; + +function testFiles(dir: string): string[] { + const out: string[] = []; + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) out.push(...testFiles(full)); + else if (entry.name.endsWith('.test.ts')) out.push(full); + } + return out; +} + +describe('PTY skill-seeding tripwire', () => { + test('every slash-command PTY test seeds skills (helper or seedSkills: true)', () => { + const offenders: string[] = []; + for (const full of testFiles(path.join(ROOT, 'test'))) { + if (path.basename(full) === 'pty-skill-seeding-wiring.test.ts') continue; + const src = fs.readFileSync(full, 'utf-8'); + const lines = src.split('\n'); + const sendLine = lines.findIndex((l) => SLASH_SEND.test(l)); + if (sendLine === -1) continue; + if (RUN_PLAN_HELPER.test(src)) continue; + if (src.includes('seedSkills: true')) continue; + offenders.push(`${path.relative(ROOT, full)}:${sendLine + 1}`); + } + expect( + offenders, + 'These tests type a /skill slash command into a hermetic PTY child that has ' + + 'no skills registered — claude rejects it as Unknown command and the test ' + + 'measures nothing. Pass seedSkills: true to launchClaudePty (or route ' + + 'through a runPlanSkill* helper): ' + offenders.join(', '), + ).toEqual([]); + }); + + test('the runPlanSkill* helpers all opt in via seedSkills: true', () => { + // The helper family types slash commands on behalf of ~20 test files; + // dropping the opt-in there silently un-measures all of them at once. + const src = fs.readFileSync(path.join(ROOT, 'test/helpers/claude-pty-runner.ts'), 'utf-8'); + const optIns = src.match(/seedSkills: true/g) ?? []; + expect(optIns.length).toBeGreaterThanOrEqual(3); + }); + + test('launchClaudePty wires seedSkills to hermeticSkillsConfigDir()', () => { + const src = fs.readFileSync(path.join(ROOT, 'test/helpers/claude-pty-runner.ts'), 'utf-8'); + // Gated on hermetic (EVALS_HERMETIC=0 must keep the operator config) and + // on the per-test env override (explicit CLAUDE_CONFIG_DIR wins). + const wired = + /if\s*\(opts\.seedSkills && hermetic && !opts\.env\?\.CLAUDE_CONFIG_DIR\)\s*\{\s*\n\s*childEnv\.CLAUDE_CONFIG_DIR = hermeticSkillsConfigDir\(\);/.test(src); + expect(wired, 'launchClaudePty must set CLAUDE_CONFIG_DIR from hermeticSkillsConfigDir() when seedSkills && hermetic && no per-test override').toBe(true); + }); +}); diff --git a/test/skill-e2e-autoplan-chain.test.ts b/test/skill-e2e-autoplan-chain.test.ts index b5e3ce740..645e55bf9 100644 --- a/test/skill-e2e-autoplan-chain.test.ts +++ b/test/skill-e2e-autoplan-chain.test.ts @@ -71,6 +71,7 @@ describeE2E('/autoplan chain ordering (periodic)', () => { permissionMode: 'plan', cwd: tempDir, timeoutMs: 1_080_000, // 18 min, slightly above test budget + seedSkills: true, }); const hits: PhaseHit[] = []; diff --git a/test/skill-e2e-plan-ceo-mode-routing.test.ts b/test/skill-e2e-plan-ceo-mode-routing.test.ts index 0199413b8..0772e7a11 100644 --- a/test/skill-e2e-plan-ceo-mode-routing.test.ts +++ b/test/skill-e2e-plan-ceo-mode-routing.test.ts @@ -152,6 +152,7 @@ describeE2E('/plan-ceo-review mode routing (gate)', () => { const session = await launchClaudePty({ permissionMode: 'plan', timeoutMs: 540_000, + seedSkills: true, }); try { await Bun.sleep(8000); diff --git a/test/skill-e2e-plan-design-with-ui.test.ts b/test/skill-e2e-plan-design-with-ui.test.ts index 622bd9382..28f60f75b 100644 --- a/test/skill-e2e-plan-design-with-ui.test.ts +++ b/test/skill-e2e-plan-design-with-ui.test.ts @@ -45,6 +45,7 @@ describeE2E('/plan-design-review with UI scope (gate)', () => { permissionMode: 'plan', cwd: ROOT, timeoutMs: 480_000, + seedSkills: true, }); let outcome: 'real_question' | 'plan_ready' | 'timeout' | 'exited' = 'timeout'; diff --git a/test/skill-e2e-ship-idempotency.test.ts b/test/skill-e2e-ship-idempotency.test.ts index daed1f1d7..e4388a7d0 100644 --- a/test/skill-e2e-ship-idempotency.test.ts +++ b/test/skill-e2e-ship-idempotency.test.ts @@ -161,6 +161,7 @@ describeE2E('/ship idempotency E2E (periodic, real-PTY)', () => { timeoutMs: 720_000, // Disable network-y pieces so the agent can't reach actual github. env: { GH_TOKEN: 'mock-not-real', NO_COLOR: '1' }, + seedSkills: true, }); let outcome: 'detected' | 'plan_ready' | 'attempted_mutation' | 'timeout' | 'exited' = 'timeout';