mirror of https://github.com/garrytan/gstack.git
feat(evals): seedSkills opt-in for PTY slash-command tests + tripwire
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 <noreply@anthropic.com> (cherry picked from commit 63c52269daaffb833b3105ea9b4b99be6df8fec7)
This commit is contained in:
parent
5819203905
commit
281227262d
|
|
@ -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: |
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
|
|
@ -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[] = [];
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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';
|
||||
|
|
|
|||
|
|
@ -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';
|
||||
|
|
|
|||
Loading…
Reference in New Issue