I1: plan-eng-review-artifact runs the real interactive review in a PTY, replacing the composing fixture

The claude -p case told the model to skip the preamble and questions and go
straight to the review. In 37151477069, 37158847998, 37162480720 and a local
CI-image run it wrote the whole review into one file without executing the
workflow, so it measured composing, not /plan-eng-review. The interactive
batching PTY run in 37158847998 did write the artifact during Test review, so
there is no product defect.

New test/skill-e2e-plan-eng-artifact.test.ts keeps the case id, tier
(periodic), kind (rule) and budget (CAPTURE_LONG_MS): it drives the real
/plan-eng-review over the same dashboard plan and sources, answers each
question with its first option, stops once the QA test plan exists or the
report completes, and asserts the same requirement (exactly one new
eng-review test plan in the run's own state root, about the dashboard change).
The composing case is retired from skill-e2e-plan.test.ts in this commit.
runPlanSkillCounting reports engTestPlans (read before fixture cleanup) and
passes them to isCollectionComplete. One paid run in the CI image (Claude Code
2.1.284): pass, collection_complete at 498 s with one test plan.

The new file takes its own periodic runner (seeded at 498 s), so the periodic
census plans 26 slices: the planner's --max-parallel and both executors' caps
move from 25 to 26 so every slice still starts at once; the paid-file census
pin moves from 106 to 107.
This commit is contained in:
garrytan committed 2026-10-04 00:22:21 +00:00
1 parent 9244437ab2
commit dc09764b84
8 files changed
+142 -150

No files matched your search

+3 -3
View File
@@ -114,7 +114,7 @@ jobs:
- name: Emit run manifest (ALL periodic tests minus reasoned excludes)
env:
EVALS_ALL: "1"
run: EVALS_TIER=periodic bun --no-install run scripts/test-paid-shards.ts --tier periodic --emit-plan /tmp/paid-plan/manifest.json --slice-budget 540 --jobs 2 --max-parallel 25
run: EVALS_TIER=periodic bun --no-install run scripts/test-paid-shards.ts --tier periodic --emit-plan /tmp/paid-plan/manifest.json --slice-budget 540 --jobs 2 --max-parallel 26
# Codex-only slices run on the host-run eval-codex-slices job; the
# container executor runs every other slice.
@@ -172,7 +172,7 @@ jobs:
fail-fast: false
# Every planned slice starts at once; test/evals-workflow-wiring.test.ts
# fails when the live plan outgrows this cap.
max-parallel: 25
max-parallel: 26
matrix:
slice: ${{ fromJSON(needs.plan-slices.outputs.periodic_slices) }}
steps:
@@ -308,7 +308,7 @@ jobs:
packages: read
strategy:
fail-fast: false
max-parallel: 25
max-parallel: 26
matrix:
slice: ${{ fromJSON(needs.plan-slices.outputs.periodic_codex_slices) }}
steps:
+1 -1
View File
@@ -165,6 +165,7 @@
"test/skill-e2e-plan-design-finding-floor.test.ts": 155538,
"test/skill-e2e-plan-design-plan-mode.test.ts": 106608,
"test/skill-e2e-plan-devex-peer-comparison-classification.test.ts": 79786,
"test/skill-e2e-plan-eng-artifact.test.ts": 498474,
"test/skill-e2e-plan-eng-finding-floor.test.ts": 388666,
"test/skill-e2e-plan-eng-multi-finding-batching.test.ts": 597500,
"test/skill-e2e-plan-eng-plan-mode.test.ts": 229226,
@@ -176,7 +177,6 @@
"test/skill-e2e-plan.test.ts#plan-ceo-review-expansion-energy": 91984,
"test/skill-e2e-plan.test.ts#plan-ceo-review-selective": 267212,
"test/skill-e2e-plan.test.ts#plan-eng-review": 128506,
"test/skill-e2e-plan.test.ts#plan-eng-review-artifact": 164137,
"test/skill-e2e-qa-bugs.test.ts": 333847,
"test/skill-e2e-qa-workflow.test.ts": 403953,
"test/skill-e2e-retro.test.ts": 166711,
+1 -1
View File
@@ -139,7 +139,7 @@ test('live periodic census fits the declared CI wall including setup', () => {
test('registered allocation is deterministic and preserves every discovered file', () => {
const files = collectPaidTestFiles();
expect(files).toHaveLength(106);
expect(files).toHaveLength(107);
expect(files).toContain('test/skill-e2e-ship-skip.test.ts');
const m = livePlan(files);
expect(livePlan([...files].reverse())).toEqual(m);
@@ -10,7 +10,7 @@ import { describe, expect, test } from 'bun:test';
import * as fs from 'node:fs';
import * as os from 'node:os';
import * as path from 'node:path';
import { runPlanSkillObservation, runPlanSkillCounting, runPlanSkillFloorCheck } from './claude-pty-runner';
import { readEngTestPlans, runPlanSkillObservation, runPlanSkillCounting, runPlanSkillFloorCheck } from './claude-pty-runner';
import { createFakePtyDriver, type FakeFrame, type FakeSessionContext } from './pty/fake-session';
import { FORCING_FLOOR_DEVEX } from '../fixtures/forcing-finding-seeds';
@@ -228,3 +228,21 @@ describe('runPlanSkillFloorCheck through the fake driver', () => {
expect(obs.summary).toBe('agent reached plan_ready without a qualifying finding question');
});
});
describe('readEngTestPlans', () => {
test('returns only /plan-eng-review QA test plans under projects/<slug>/ of the owned state root', () => {
const root = fs.mkdtempSync(path.join(os.tmpdir(), 'eng-test-plans-'));
try {
expect(readEngTestPlans(root)).toEqual([]);
expect(readEngTestPlans(undefined)).toEqual([]);
fs.mkdirSync(path.join(root, 'projects', 'fixture-slug'), { recursive: true });
fs.writeFileSync(path.join(root, 'projects', 'fixture-slug', 'runner-main-eng-review-test-plan-20261004-010203.md'), '# Test Plan\nDashboard');
fs.writeFileSync(path.join(root, 'projects', 'fixture-slug', 'runner-main-ship-test-plan-20261004-010203.md'), '# Ship plan');
fs.writeFileSync(path.join(root, 'projects', 'fixture-slug', 'tasks-eng-review-20261004-010203.jsonl'), '{}');
fs.writeFileSync(path.join(root, 'eng-review-test-plan-outside-projects.md'), 'stray');
expect(readEngTestPlans(root)).toEqual([{ file: path.join('projects', 'fixture-slug', 'runner-main-eng-review-test-plan-20261004-010203.md'), content: '# Test Plan\nDashboard' }]);
} finally {
fs.rmSync(root, { recursive: true, force: true });
}
});
});
+2 -2
View File
@@ -24,7 +24,7 @@ export type { ReviewReportAtBottomResult, NativePlanTerminalReview, NativePlanTe
export { ceoStep0Boundary, engSetupAUQ, engFirstReviewAUQ, engStep0Boundary, pickDesignFocusAll } from './pty/boundaries';
export { runPlanSkillObservation } from './pty/runners/observation';
export type { PlanSkillObservation, PlanSkillObservationOptions } from './pty/runners/observation';
export { runPlanSkillCounting, countingCapture, isNativeCompletionSummary } from './pty/runners/counting';
export type { PlanSkillCountObservation, PlanSkillCountingOptions, CountingRun } from './pty/runners/counting';
export { runPlanSkillCounting, countingCapture, isNativeCompletionSummary, readEngTestPlans } from './pty/runners/counting';
export type { EngTestPlan, PlanSkillCountObservation, PlanSkillCountingOptions, CountingRun } from './pty/runners/counting';
export { planFloorDXPane, planFloorDXReplyInput, runPlanSkillFloorCheck } from './pty/runners/floor';
export type { PlanSkillFloorObservation, PlanFloorDXReply } from './pty/runners/floor';
+21 -3
View File
@@ -2,6 +2,7 @@
* runPlanSkillCounting. Moved from claude-pty-runner.ts.
* Import through test/helpers/claude-pty-runner.ts from tests; pty/ modules import siblings directly.
*/
import * as fs from 'node:fs';
import * as path from 'node:path';
import { createPlanCountFixture } from '../../plan-count-fixture';
import { createPlanCountSnapshotWriter } from '../../plan-count-artifacts';
@@ -62,6 +63,21 @@ export interface PlanSkillCountObservation {
reviewCount: number;
/** Answered administrative handoffs and artifact rendering, preserved separately. */
administrativeCount: number;
/** QA test plans (`projects/<slug>/*-eng-review-test-plan-*.md`) in the run's owned state root at the outcome. */
engTestPlans: EngTestPlan[];
}
export interface EngTestPlan { file: string; content: string }
/** The /plan-eng-review QA test-plan artifacts under one state root, read before fixture cleanup. */
export function readEngTestPlans(stateRoot: string | undefined): EngTestPlan[] {
const projects = stateRoot ? path.join(stateRoot, 'projects') : '';
if (!projects || !fs.existsSync(projects)) return [];
return fs.readdirSync(projects, { withFileTypes: true }).filter(slug => slug.isDirectory()).flatMap(slug =>
fs.readdirSync(path.join(projects, slug.name)).filter(name => /-eng-review-test-plan-.*\.md$/.test(name)).sort().map(name => {
const file = path.join('projects', slug.name, name);
return { file, content: fs.readFileSync(path.join(stateRoot!, file), 'utf8') };
}));
}
/** Options for runPlanSkillCounting. */
@@ -94,7 +110,8 @@ export interface PlanSkillCountingOptions {
isReviewAUQ?: (fp: AskUserQuestionFingerprint, priorCalls?: readonly NativePlanQuestionCall[]) => boolean;
/** Stop a collection-only fixture once its acknowledged inputs are complete.
* This is not review completion or a passing verdict; the caller still validates them. */
isCollectionComplete?: (transcript: PlanCountTranscript, fingerprints: readonly AskUserQuestionFingerprint[]) => boolean;
isCollectionComplete?: (transcript: PlanCountTranscript, fingerprints: readonly AskUserQuestionFingerprint[],
engTestPlans: readonly EngTestPlan[]) => boolean;
/** Narrow caller-specific selection; null retains the normal answer policy.
* The first argument retains full pending metadata for existing callers.
* Native-bound selection uses activeCapture, whose metadata is present only
@@ -272,7 +289,7 @@ export interface CountingRun {
cleanupReserveMs: number;
/** Monotonic work cutoff: budget start + timeout - cleanup reserve. */
workDeadline: number;
fixture: { cwd: string };
fixture: { cwd: string; env: Record<string, string> };
defaultPick: number;
pickerContext: Readonly<{ cwd: string; deadlineAt: number }>;
saveSnapshot: ReturnType<typeof createPlanCountSnapshotWriter>;
@@ -353,6 +370,7 @@ function countingSnapshot(
step0Count: run.step0Count,
reviewCount: run.reviewCount,
administrativeCount: run.administrativeCount,
engTestPlans: readEngTestPlans(run.fixture.env.GSTACK_STATE_ROOT),
};
const artifacts = countingCapture(run, session, observation);
Object.assign(observation, artifacts);
@@ -418,7 +436,7 @@ async function countingTick(run: CountingRun, session: ClaudePtySession): Promis
if (opts.isCollectionComplete && !pending && transcript.status === 'ready' &&
transcript.calls.length > 0 && transcript.calls.every(call => call.answered && !call.failed) &&
!unresolvedPlanQuestionCalls(transcript.calls).length && remainingWork(run) > 0 &&
opts.isCollectionComplete(transcript, run.fingerprints) && remainingWork(run) > 0) {
opts.isCollectionComplete(transcript, run.fingerprints, readEngTestPlans(run.fixture.env.GSTACK_STATE_ROOT)) && remainingWork(run) > 0) {
return { done: countingSnapshot(run, session, 'collection_complete', 'Caller-defined native collection is complete; final validation remains required', visible) };
}
const terminal = await countingTerminal(run, visible, pending);
+94
View File
@@ -0,0 +1,94 @@
/**
* /plan-eng-review writes its QA test-plan artifact when run the way users run
* it (periodic, paid, real-PTY).
*
* Replaces the composing `plan-eng-review-artifact` case in skill-e2e-plan.test.ts.
* That case told a `claude -p` session to skip the preamble and questions and
* "go straight to the review"; in runs 37151477069, 37158847998 and 37162480720
* (and one local CI-image run) the model wrote the whole review into one file
* without executing the workflow, so it measured composing, not the skill. A
* full interactive run does write the artifact (37158847998's batching PTY run
* wrote eng-review-test-plan-20261003-224617.md during Test review).
*
* This case drives the real interactive /plan-eng-review over the same
* dashboard plan and sources, answers each question with its first option, and
* stops once the artifact exists or the report completes. The requirement is
* unchanged: exactly one new QA test plan under the run's own state root, about
* the reviewed dashboard change.
*/
import { expect, test } from 'bun:test';
import * as fs from 'node:fs';
import * as os from 'node:os';
import * as path from 'node:path';
import { CAPTURE_LONG_MS } from './helpers/eval-budgets';
import { describeE2ETier } from './helpers/e2e-gate';
import { engFirstReviewAUQ, engSetupAUQ, engStep0Boundary, hasCompletePlanReport, runPlanSkillCounting } from './helpers/claude-pty-runner';
const describeE2E = describeE2ETier('periodic');
/** Working-plan path in the prompt; rewritten per run to a private temporary path. */
const FIXTURE_PLAN_PATH = '/tmp/gstack-test-plan-eng-artifact.md';
const DASHBOARD_PLAN = [
'Proceed directly to the requested engineering review; skip the optional /office-hours prerequisite.',
`Please review this plan thoroughly. As you go, write your plan-mode plan to ${FIXTURE_PLAN_PATH} (use Edit/Write to that exact path).`,
'',
'# Plan: Add Dashboard',
'',
'## Changes',
'1. New `dashboard.ts` with Dashboard component and fetchStats API call',
'2. Updated `app.ts` to import and use Dashboard',
'',
'## Architecture',
'- Dashboard fetches from `/api/stats` endpoint',
'- Returns user count and revenue metrics',
].join('\n');
const SOURCES = {
'app.ts': 'import { Dashboard } from "./dashboard";\nexport function greet() { return "hello"; }\nexport function main() { return Dashboard(); }\n',
'dashboard.ts': `export function Dashboard() {
const data = fetchStats();
return { users: data.users, revenue: data.revenue };
}
function fetchStats() {
return fetch('/api/stats').then(r => r.json());
}
`,
};
describeE2E('/plan-eng-review QA test-plan artifact (periodic)', () => {
test('an interactive review writes one QA test plan about the reviewed change', async () => {
const startedAt = Date.now();
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-e2e-plan-eng-artifact-'));
const planPath = path.join(tmpDir, 'gstack-test-plan-eng-artifact.md');
try {
const obs = await runPlanSkillCounting({
skillName: 'plan-eng-review',
slashCommand: '/plan-eng-review',
followUpPrompt: DASHBOARD_PLAN.replaceAll(FIXTURE_PLAN_PATH, planPath),
fixtureFiles: SOURCES,
permissionPlanPath: planPath,
isLastStep0AUQ: engStep0Boundary,
isSetupAUQ: engSetupAUQ,
isFirstReviewAUQ: engFirstReviewAUQ,
// The artifact is the only verdict; stop once it exists or the report is complete.
isCollectionComplete: (_transcript, _fingerprints, testPlans) =>
testPlans.length > 0 || hasCompletePlanReport(planPath, startedAt, Date.now()),
reviewCountCeiling: Infinity,
preconfiguredReviewActor: true,
timeoutMs: CAPTURE_LONG_MS,
env: { QUESTION_TUNING: 'false', EXPLAIN_LEVEL: 'default' },
});
const plans = obs.engTestPlans;
console.log(`plan-eng-review-artifact: outcome=${obs.outcome} elapsed=${obs.elapsedMs}ms review=${obs.reviewCount} test plans=${plans.map(plan => plan.file).join(', ') || 'none'}`);
if (!['plan_ready', 'completion_summary', 'collection_complete'].includes(obs.outcome)) {
throw new Error(`plan-eng-review-artifact: outcome=${obs.outcome} (${obs.summary})\n--- evidence ---\n${obs.evidence}`);
}
expect(plans, `expected one new eng-review test plan; outcome=${obs.outcome}`).toHaveLength(1);
expect(/dashboard|fetchStats|\/api\/stats/i.test(plans[0]!.content), 'the test plan covers the reviewed dashboard change').toBe(true);
} finally {
fs.rmSync(tmpDir, { recursive: true, force: true });
}
}, CAPTURE_LONG_MS + 30_000);
});
+1 -139
View File
@@ -1,13 +1,12 @@
import { describe, test, expect, beforeAll, afterAll } from 'bun:test';
import { JUDGE_MS, CAPTURE_MS, CAPTURE_LONG_MS, PTY_MS } from './helpers/eval-budgets';
import { runSkillTest } from './helpers/session-runner';
import { getHermeticDirs, isHermeticEnabled, seedHermeticGstackHome } from './helpers/hermetic-env';
import { EvalCollector } from './helpers/eval-store';
import { OFFICE_HOURS_BUN_GRACE_MS, runRecordedOfficeHoursAttempt } from './helpers/office-hours-attempt';
import {
ROOT, browseBin, runId, evalsEnabled,
describeIfSelected, testConcurrentIfSelected,
copyDirSync, setupBrowseShims, logCost, recordE2E,
copyDirSync, logCost, recordE2E,
finalizeEvalCollector,
} from './helpers/e2e-helpers';
import { judgePosture } from './helpers/llm-judge';
@@ -373,143 +372,6 @@ Focus on architecture, code quality, tests, and performance sections.`,
}, CAPTURE_LONG_MS);
});
// --- Plan-Eng-Review Test-Plan Artifact E2E ---
describeIfSelected('Plan-Eng-Review Test-Plan Artifact E2E', ['plan-eng-review-artifact'], () => {
let planDir: string;
let gstackHome: string;
let ownsGstackHome = false;
let slug: string;
let projectDir: string;
beforeAll(() => {
planDir = fs.mkdtempSync(path.join(os.tmpdir(), 'skill-e2e-plan-artifact-'));
const run = (cmd: string, args: string[]) =>
spawnSync(cmd, args, { cwd: planDir, stdio: 'pipe', timeout: 5000 });
run('git', ['init', '-b', 'main']);
run('git', ['config', 'user.email', 'test@test.com']);
run('git', ['config', 'user.name', 'Test']);
// Create base commit on main
fs.writeFileSync(path.join(planDir, 'app.ts'), 'export function greet() { return "hello"; }\n');
run('git', ['add', '.']);
run('git', ['commit', '-m', 'initial']);
// Create feature branch with changes
run('git', ['checkout', '-b', 'feature/add-dashboard']);
fs.writeFileSync(path.join(planDir, 'dashboard.ts'), `export function Dashboard() {
const data = fetchStats();
return { users: data.users, revenue: data.revenue };
}
function fetchStats() {
return fetch('/api/stats').then(r => r.json());
}
`);
fs.writeFileSync(path.join(planDir, 'app.ts'), `import { Dashboard } from "./dashboard";
export function greet() { return "hello"; }
export function main() { return Dashboard(); }
`);
run('git', ['add', '.']);
run('git', ['commit', '-m', 'feat: add dashboard']);
// Plan document
fs.writeFileSync(path.join(planDir, 'plan.md'), `# Plan: Add Dashboard
## Changes
1. New \`dashboard.ts\` with Dashboard component and fetchStats API call
2. Updated \`app.ts\` to import and use Dashboard
## Architecture
- Dashboard fetches from \`/api/stats\` endpoint
- Returns user count and revenue metrics
`);
run('git', ['add', 'plan.md']);
run('git', ['commit', '-m', 'add plan']);
// Copy plan-eng-review skill
fs.mkdirSync(path.join(planDir, 'plan-eng-review'), { recursive: true });
fs.copyFileSync(
path.join(ROOT, 'plan-eng-review', 'SKILL.md'),
path.join(planDir, 'plan-eng-review', 'SKILL.md'),
);
// Carved skills (v2 plan T9): copy sections/ so the review workflow + report template are present.
{ const _sec = path.join(ROOT, 'plan-eng-review', 'sections'); if (fs.existsSync(_sec)) fs.cpSync(_sec, path.join(planDir, 'plan-eng-review', 'sections'), { recursive: true }); }
// Set up remote-slug shim and browse shims (plan-eng-review uses remote-slug for artifact path)
setupBrowseShims(planDir);
// The actor, this observer and cleanup share one isolated state root: the
// hermetic run's GSTACK_HOME (or a seeded temporary one when hermetic mode
// is off), and the slug gstack-slug resolves for this fixture. Nothing is
// read or deleted under the operator's real home.
if (isHermeticEnabled()) gstackHome = getHermeticDirs().gstackHome;
else {
gstackHome = fs.mkdtempSync(path.join(os.tmpdir(), 'skill-e2e-plan-artifact-home-'));
seedHermeticGstackHome(gstackHome);
ownsGstackHome = true;
}
const slugOut = spawnSync(path.join(ROOT, 'bin', 'gstack-slug'), [], {
cwd: planDir, stdio: 'pipe', timeout: 5000, env: { ...process.env, GSTACK_HOME: gstackHome },
}).stdout.toString();
slug = /^SLUG=(.+)$/m.exec(slugOut)?.[1]?.trim() ?? '';
if (!slug) throw new Error(`plan-eng-review-artifact: gstack-slug resolved no slug: ${slugOut}`);
projectDir = path.join(gstackHome, 'projects', slug);
const realHome = path.resolve(os.homedir());
if (projectDir === realHome || projectDir.startsWith(realHome + path.sep) && !projectDir.startsWith(path.resolve(os.tmpdir()) + path.sep)) {
throw new Error(`plan-eng-review-artifact: state root ${projectDir} is under the real home directory`);
}
});
afterAll(() => {
try { fs.rmSync(planDir, { recursive: true, force: true }); } catch {}
// Remove only this fixture's project directory inside the isolated state root.
try { fs.rmSync(ownsGstackHome ? gstackHome : projectDir, { recursive: true, force: true }); } catch {}
});
testConcurrentIfSelected('plan-eng-review-artifact', async () => {
const testPlans = () => fs.existsSync(projectDir)
? fs.readdirSync(projectDir).filter(f => /eng-review-test-plan-.*\.md$/.test(f)) : [];
const beforeFiles = testPlans();
const result = await runSkillTest({
prompt: `Read plan-eng-review/SKILL.md for the review workflow.
Skip the preamble bash block, lake intro, telemetry, and contributor mode sections — go straight to the review.
Read plan.md — that's the plan to review. This is a standalone plan with source code in app.ts and dashboard.ts.
Proceed directly to the full review. Skip any AskUserQuestion calls — this is non-interactive.
In this fixture the gstack state root is GSTACK_HOME=${gstackHome} and the project slug is ${slug}.
Use ${planDir}/review-output.md as the selected report file. Every other output keeps the destination the skill gives it.`,
workingDirectory: planDir,
env: { GSTACK_HOME: gstackHome },
maxTurns: 25,
allowedTools: ['Bash', 'Read', 'Write', 'Glob', 'Grep'],
timeout: CAPTURE_LONG_MS,
testName: 'plan-eng-review-artifact',
runId,
model: 'claude-opus-4-7',
});
logCost('/plan-eng-review artifact', result);
// The QA test plan is a required discovery artifact of the review: exactly
// one new file under this fixture's project directory, about this plan.
const newFiles = testPlans().filter(f => !beforeFiles.includes(f));
const content = newFiles.length === 1 ? fs.readFileSync(path.join(projectDir, newFiles[0]!), 'utf-8') : '';
const aboutPlan = /dashboard|fetchStats|\/api\/stats/i.test(content);
console.log(`Test-plan artifacts in ${projectDir}: ${newFiles.length} new${newFiles.length ? ` (${newFiles[0]}, ${content.length} chars)` : ''}`);
recordE2E(evalCollector, '/plan-eng-review test-plan artifact', 'Plan-Eng-Review Test-Plan Artifact E2E', result, {
passed: ['success', 'error_max_turns'].includes(result.exitReason) && newFiles.length === 1 && aboutPlan,
});
expect(['success', 'error_max_turns']).toContain(result.exitReason);
expect(newFiles, `expected one new eng-review test plan in ${projectDir}`).toHaveLength(1);
expect(aboutPlan, 'the test plan covers the reviewed dashboard change').toBe(true);
}, CAPTURE_LONG_MS);
});
// --- Plan Review Report E2E ---
// Verifies that plan-eng-review writes a "## GSTACK REVIEW REPORT" section
// to the bottom of the plan file (the living review status footer).