mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-10 06:58:59 +02:00
feat(plan-eng/design-review): auto-select B in plan mode at the scope gate
In plan mode the scope gate's "What should I review? A/B/C" question is pure
friction: there is no branch diff and the target is the plan being drafted.
Both gates gain an ordered exceptions block, checked BEFORE asking:
1. Plan mode → auto-select B: review the active plan (in context or pasted),
announce it in one line ("Scope gate: plan mode — auto-selected B
(reviewing <target>)") so the user can interrupt; an explicitly different
user-named target still wins; no plan drafted yet → ask as normal.
2. User-named target (outside plan mode): explicit-only — a path, a pasted
doc, or the literal words "branch diff". A passing mention is not naming;
when in doubt, ask.
Outside plan mode with no explicitly-named target, nothing changes. Plan-mode
is checked FIRST because the PTY harness seeds drafts as pasted user messages
(claude-pty-runner.ts:1600) — ordering makes the seeded smokes deterministic.
Pinning: seeded plan-mode smokes assert no gate render + announcement rendered
(eng test 2; new design seeded test); plan-mode-no-op extends to eng/design
(bypass must not misfire outside plan mode; first question must be the gate)
plus a named-target case proving the pasted target is consumed; a drift-guard
asserts the two hand-duplicated exceptions blocks stay identical modulo the
two variant slots and carry the announcement string the detectors pin.
Skeleton ceilings ratcheted with comments (eng 68k, design 89k; eng union
ratio 1.08→1.09) — measured 67,006 B / 88,226 B after regen.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
44cd3037f0
commit
2009f89283
@@ -3244,6 +3244,54 @@ describe('EXIT PLAN MODE GATE placement', () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe('scope-gate exceptions drift-guard', () => {
|
||||
// The plan-mode auto-select-B exceptions block is hand-duplicated in the
|
||||
// plan-eng-review and plan-design-review templates (matching the gate
|
||||
// around it, which predates this block). The two copies must stay
|
||||
// byte-identical modulo exactly two known variant slots:
|
||||
// 1. the plan-mode bullet's action tail (Design Doc Check vs pre-review
|
||||
// audit + mockups),
|
||||
// 2. the named-target vocabulary ("a path, a doc" vs "a path, a page, a doc").
|
||||
// A future edit to one copy that silently misses the other fails here
|
||||
// instead of drifting. The real fix (shared {{SCOPE_GATE}} resolver) is a
|
||||
// filed TODO — this guard is the stopgap that makes the duplication safe.
|
||||
const START_MARKER = '**Exceptions — check in this order, BEFORE asking:**';
|
||||
const END_MARKER = 'nothing changes: this gate is a hard STOP.';
|
||||
|
||||
function extractExceptionsBlock(skill: string): string {
|
||||
const md = fs.readFileSync(path.join(ROOT, skill, 'SKILL.md'), 'utf-8');
|
||||
const start = md.indexOf(START_MARKER);
|
||||
expect(start, `${skill}/SKILL.md: exceptions block start marker present`).toBeGreaterThan(-1);
|
||||
const end = md.indexOf(END_MARKER, start);
|
||||
expect(end, `${skill}/SKILL.md: exceptions block end marker present`).toBeGreaterThan(start);
|
||||
return md.slice(start, end + END_MARKER.length);
|
||||
}
|
||||
|
||||
const normalizeVariantSlots = (block: string) =>
|
||||
block
|
||||
.replace('Then run the Design Doc Check and Step 0 against that plan.', '<ACTION_TAIL>')
|
||||
.replace('Then run the pre-review audit, mockups, and Step 0 against that plan.', '<ACTION_TAIL>')
|
||||
.replace('a path, a page, a doc they pasted,', 'a path, a doc they pasted,');
|
||||
|
||||
test('eng and design exceptions blocks are identical modulo the two variant slots', () => {
|
||||
const eng = normalizeVariantSlots(extractExceptionsBlock('plan-eng-review'));
|
||||
const design = normalizeVariantSlots(extractExceptionsBlock('plan-design-review'));
|
||||
expect(eng).toBe(design);
|
||||
// The action tail must actually have been normalized in both (guards
|
||||
// against a rewording that bypasses the normalizer and vacuously passes).
|
||||
expect(eng).toContain('<ACTION_TAIL>');
|
||||
});
|
||||
|
||||
test('exceptions block carries the announcement string the PTY detectors pin', () => {
|
||||
for (const skill of ['plan-eng-review', 'plan-design-review']) {
|
||||
const block = extractExceptionsBlock(skill);
|
||||
expect(block, `${skill}: verbatim announcement`).toContain(
|
||||
'Scope gate: plan mode — auto-selected B (reviewing <target>).',
|
||||
);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('GSTACK REVIEW REPORT mandatory unresolved-decisions status', () => {
|
||||
// Report text rides in PLAN_FILE_REVIEW_REPORT → every report consumer gets it.
|
||||
// devex-review is a report consumer but NOT a gate consumer, so the two target
|
||||
|
||||
@@ -164,7 +164,8 @@ export const CARVE_GUARDS: Record<string, CarveGuard> = {
|
||||
},
|
||||
behavioral: 'plan',
|
||||
// v1.2.0 activation lift (shared first-run-guidance preamble) + #2077 ask-first scope gate.
|
||||
maxSkeletonBytes: 67_000,
|
||||
// +~1 KB: plan-mode auto-select-B scope-gate exceptions (2026-08).
|
||||
maxSkeletonBytes: 68_000,
|
||||
minUnionBytes: 70_000,
|
||||
mustContain: ['Architecture', 'Code Quality', 'Test', 'Performance'],
|
||||
// Cross-cutting preamble growth (v1.57.2.0 AUQ-failure prose fallback + the
|
||||
@@ -172,7 +173,9 @@ export const CARVE_GUARDS: Record<string, CarveGuard> = {
|
||||
// default-on Codex outside-voice (codexPreflight block + CODEX_MODE branch
|
||||
// prose, replacing the smaller opt-in question) land this at ~6.6% over the
|
||||
// v1.53.0.0 baseline. Headroom for those intentional additions.
|
||||
maxSizeRatio: 1.08,
|
||||
// 1.08 → 1.09: the scope-gate exceptions block put the union ~95 B over
|
||||
// the 1.08 line (123,440 / 114,209 baseline = 1.0808).
|
||||
maxSizeRatio: 1.09,
|
||||
},
|
||||
'plan-design-review': {
|
||||
skill: 'plan-design-review',
|
||||
@@ -189,7 +192,8 @@ export const CARVE_GUARDS: Record<string, CarveGuard> = {
|
||||
// +Conductor AUQ-default-prose rule + one-way/continuation safety in the
|
||||
// always-loaded AskUserQuestion Format section.
|
||||
// v1.2.0 activation lift (shared first-run-guidance preamble) + #2077 ask-first scope gate.
|
||||
maxSkeletonBytes: 88_000,
|
||||
// +~1.3 KB: plan-mode auto-select-B scope-gate exceptions (2026-08).
|
||||
maxSkeletonBytes: 89_000,
|
||||
minUnionBytes: 70_000,
|
||||
mustContain: ['design', 'visual'],
|
||||
maxSizeRatio: 1.07,
|
||||
|
||||
@@ -102,7 +102,10 @@ export const E2E_TOUCHFILES: Record<string, string[]> = {
|
||||
'plan-eng-review-plan-mode': ['plan-eng-review/**', 'scripts/resolvers/preamble/generate-completion-status.ts', 'scripts/resolvers/question-tuning.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/review.ts', 'test/helpers/claude-pty-runner.ts', 'test/skill-e2e-plan-eng-plan-mode.test.ts'],
|
||||
'plan-design-review-plan-mode': ['plan-design-review/**', 'scripts/resolvers/preamble/generate-completion-status.ts', 'scripts/resolvers/question-tuning.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/review.ts', 'test/helpers/claude-pty-runner.ts', 'test/skill-e2e-plan-design-plan-mode.test.ts'],
|
||||
'plan-devex-review-plan-mode': ['plan-devex-review/**', 'scripts/resolvers/preamble/generate-completion-status.ts', 'scripts/resolvers/question-tuning.ts', 'scripts/resolvers/preamble/generate-ask-user-format.ts', 'scripts/resolvers/preamble.ts', 'scripts/resolvers/review.ts', 'test/helpers/claude-pty-runner.ts'],
|
||||
'plan-mode-no-op': ['plan-ceo-review/**', 'scripts/resolvers/preamble/generate-completion-status.ts', 'scripts/resolvers/preamble.ts', 'test/helpers/claude-pty-runner.ts'],
|
||||
// Covers ceo (preamble misfire) + eng/design (scope-gate bypass must not
|
||||
// fire outside plan mode) + the named-target exception case. 4 sequential
|
||||
// PTY runs (~+10 min, ~+$2 vs the pre-bypass single run).
|
||||
'plan-mode-no-op': ['plan-ceo-review/**', 'plan-eng-review/**', 'plan-design-review/**', 'scripts/resolvers/preamble/generate-completion-status.ts', 'scripts/resolvers/preamble.ts', 'test/helpers/claude-pty-runner.ts', 'test/skill-e2e-plan-mode-no-op.test.ts'],
|
||||
|
||||
// v1.21+ AskUserQuestion-blocked regression tests — Conductor launches
|
||||
// claude with `--disallowedTools AskUserQuestion --permission-mode default`
|
||||
|
||||
@@ -18,6 +18,29 @@ import {
|
||||
const shouldRun = !!process.env.EVALS && process.env.EVALS_TIER === 'periodic';
|
||||
const describeE2E = shouldRun ? describe : describe.skip;
|
||||
|
||||
// UI-heavy seed with guaranteed design gaps (center-aligned everything, no
|
||||
// empty states, no responsive intent) so the review has real findings to
|
||||
// surface. Inline twin of the eng smoke's SEED_PLAN_FORCING_FINDINGS —
|
||||
// FORCING_FLOOR_DESIGN from forcing-finding-seeds.ts is NOT reusable here:
|
||||
// it embeds a write-to-/tmp instruction shaped for the floor check's
|
||||
// followUpPrompt, which would trip strictPlanWrites as a silent_write.
|
||||
const SEED_PLAN_UI_HEAVY = `
|
||||
# Plan: Marketing landing page
|
||||
|
||||
## Layout
|
||||
All headings, taglines, and body copy will be center-aligned for a
|
||||
"clean modern look." The hero h1 sits 8px above the subhead; the CTA
|
||||
button has the same visual weight as the "Learn more" link beside it.
|
||||
|
||||
## Pages
|
||||
- / (hero, 3-column features grid, testimonials carousel, footer)
|
||||
- /pricing (3 tier cards)
|
||||
|
||||
## States
|
||||
Only the happy path is designed. No empty states, no error states,
|
||||
no loading states. Mobile: "stacks on mobile."
|
||||
`;
|
||||
|
||||
describeE2E('plan-design-review plan-mode smoke (periodic)', () => {
|
||||
test('reaches a terminal outcome (asked or plan_ready) without silent writes', async () => {
|
||||
const obs = await runPlanSkillObservation({
|
||||
@@ -37,4 +60,40 @@ describeE2E('plan-design-review plan-mode smoke (periodic)', () => {
|
||||
expect(['asked', 'plan_ready']).toContain(obs.outcome);
|
||||
assertReportAtBottomIfPlanWritten(obs);
|
||||
}, 360_000);
|
||||
|
||||
// Plan-mode scope-gate bypass: with a seeded UI-heavy plan in plan mode,
|
||||
// the gate must NOT render its "What should I review?" menu — it
|
||||
// auto-selects B and announces it, then proceeds to the pre-review audit
|
||||
// and mockups. Mirrors the eng smoke's seeded STOP-gate test, without
|
||||
// --disallowedTools (native AUQ available is the common path here).
|
||||
test('scope gate auto-selects B when a plan is seeded in plan mode', async () => {
|
||||
const obs = await runPlanSkillObservation({
|
||||
skillName: 'plan-design-review',
|
||||
inPlanMode: true,
|
||||
initialPlanContent: SEED_PLAN_UI_HEAVY,
|
||||
timeoutMs: 300_000,
|
||||
});
|
||||
|
||||
if (
|
||||
obs.outcome === 'wrote_findings_before_asking' ||
|
||||
obs.outcome === 'auto_decided' ||
|
||||
obs.outcome === 'silent_write' ||
|
||||
obs.outcome === 'exited' ||
|
||||
obs.outcome === 'timeout'
|
||||
) {
|
||||
throw new Error(
|
||||
`plan-design plan-mode bypass FAILED: outcome=${obs.outcome}\n` +
|
||||
`summary: ${obs.summary}\nelapsed: ${obs.elapsedMs}ms\n` +
|
||||
`--- evidence (last 2KB) ---\n${obs.evidence}`,
|
||||
);
|
||||
}
|
||||
|
||||
expect(['asked', 'plan_ready']).toContain(obs.outcome);
|
||||
assertReportAtBottomIfPlanWritten(obs);
|
||||
|
||||
// The bypass contract (exception ordering makes this deterministic even
|
||||
// though the seed arrives as a pasted user message).
|
||||
expect(obs.scopeGateQuestionObserved ?? false).toBe(false);
|
||||
expect(obs.scopeGateAutoSelectObserved ?? false).toBe(true);
|
||||
}, 360_000);
|
||||
});
|
||||
|
||||
@@ -108,5 +108,15 @@ describeE2E('plan-eng-review plan-mode smoke (periodic)', () => {
|
||||
|
||||
expect(['asked', 'plan_ready']).toContain(obs.outcome);
|
||||
assertReportAtBottomIfPlanWritten(obs);
|
||||
|
||||
// Plan-mode scope-gate bypass: with a seeded plan in plan mode, the gate
|
||||
// must NOT render its "What should I review?" menu — it auto-selects B
|
||||
// and announces it. Exception ordering in the template (plan-mode branch
|
||||
// first) makes this deterministic even though the seed arrives as a
|
||||
// pasted user message. Unseeded test 1 keeps its lenient contract: with
|
||||
// no plan drafted, the "ask as normal" fallback legitimately renders the
|
||||
// question.
|
||||
expect(obs.scopeGateQuestionObserved ?? false).toBe(false);
|
||||
expect(obs.scopeGateAutoSelectObserved ?? false).toBe(true);
|
||||
}, 360_000);
|
||||
});
|
||||
|
||||
@@ -1,48 +1,138 @@
|
||||
/**
|
||||
* Plan-mode-info no-op regression (gate tier, paid, real-PTY).
|
||||
*
|
||||
* Asserts: when /plan-ceo-review is invoked OUTSIDE plan mode (no
|
||||
* Asserts: when a plan-review skill is invoked OUTSIDE plan mode (no
|
||||
* --permission-mode plan flag, no plan-mode reminder injected), the skill
|
||||
* still reaches a terminal outcome ('asked' or 'plan_ready'). This is the
|
||||
* negative coverage to the per-skill plan-mode smokes — if the
|
||||
* plan-mode-info preamble section ever starts misfiring for non-plan-mode
|
||||
* sessions (e.g., gating questions on a phrase that isn't there), this
|
||||
* test catches it.
|
||||
* negative coverage to the per-skill plan-mode smokes — if plan-mode-keyed
|
||||
* behavior ever starts misfiring for non-plan-mode sessions (e.g., gating
|
||||
* questions on a phrase that isn't there, or the plan-eng/plan-design
|
||||
* scope-gate auto-select-B bypass firing without plan mode), this test
|
||||
* catches it.
|
||||
*
|
||||
* Why this matters: outside plan mode, claude doesn't render a native
|
||||
* confirmation UI. The skill must drive its own AskUserQuestion. Same
|
||||
* runner, same outcome contract — just `inPlanMode: false`.
|
||||
*
|
||||
* Coverage grew with the scope-gate bypass (plan-mode auto-select B):
|
||||
* - plan-ceo-review: original preamble-misfire regression.
|
||||
* - plan-eng-review / plan-design-review: the bypass must NOT fire outside
|
||||
* plan mode (scopeGateAutoSelectObserved stays false), and when the run
|
||||
* ends in 'asked', the question that fired must be the scope gate itself
|
||||
* (outside plan mode with no named target, the gate is the FIRST
|
||||
* question by contract).
|
||||
* - named-target case: a pasted draft (initialPlanContent) IS an
|
||||
* explicitly-named target, so the gate must NOT ask — and the review
|
||||
* must actually consume the pasted content.
|
||||
*
|
||||
* Cost note: 4 sequential PTY runs (~3-5 min each) in the gate lane, up
|
||||
* from 1 pre-bypass. Selected only when plan-ceo/eng/design or the runner
|
||||
* change (see 'plan-mode-no-op' in touchfiles.ts).
|
||||
*/
|
||||
|
||||
import { describe, test, expect } from 'bun:test';
|
||||
import { existsSync, readFileSync } from 'node:fs';
|
||||
import { runPlanSkillObservation } from './helpers/claude-pty-runner';
|
||||
|
||||
const shouldRun = !!process.env.EVALS && process.env.EVALS_TIER === 'gate';
|
||||
const describeE2E = shouldRun ? describe : describe.skip;
|
||||
|
||||
const PLAN_MODE_REMINDER =
|
||||
'Plan mode is active. The user indicated that they do not want you to execute yet';
|
||||
|
||||
// Distinctive token proves the pasted target was consumed by the review —
|
||||
// not just that no question fired. Nonsense-unique so it can't appear by
|
||||
// coincidence in skill output.
|
||||
const SEED_TOKEN = 'ZephyrLedgerWidget';
|
||||
const NAMED_TARGET_SEED = `
|
||||
# Plan: ${SEED_TOKEN} settings panel
|
||||
|
||||
## Scope
|
||||
Add a ${SEED_TOKEN} settings panel with a single toggle that enables
|
||||
weekly export emails. One new component, one route, one test file.
|
||||
|
||||
## Files
|
||||
- src/components/${SEED_TOKEN}.tsx (new)
|
||||
- src/routes/settings.tsx (add panel)
|
||||
- test/${SEED_TOKEN}.test.tsx (new)
|
||||
`;
|
||||
|
||||
describeE2E('plan-mode-info no-op outside plan mode (gate regression)', () => {
|
||||
test('skill reaches a terminal outcome outside plan mode', async () => {
|
||||
for (const skillName of ['plan-ceo-review', 'plan-eng-review', 'plan-design-review'] as const) {
|
||||
test(`${skillName} reaches a terminal outcome outside plan mode`, async () => {
|
||||
const obs = await runPlanSkillObservation({
|
||||
skillName,
|
||||
inPlanMode: false,
|
||||
timeoutMs: 300_000,
|
||||
});
|
||||
|
||||
if (obs.outcome === 'silent_write' || obs.outcome === 'exited' || obs.outcome === 'timeout') {
|
||||
throw new Error(
|
||||
`plan-mode no-op regression FAILED (${skillName}): outcome=${obs.outcome}\n` +
|
||||
`summary: ${obs.summary}\n` +
|
||||
`elapsed: ${obs.elapsedMs}ms\n` +
|
||||
`--- evidence (last 2KB visible) ---\n${obs.evidence}`,
|
||||
);
|
||||
}
|
||||
expect(['asked', 'plan_ready']).toContain(obs.outcome);
|
||||
|
||||
// Negative regression: the rendered output must NOT echo the plan-mode
|
||||
// distinctive reminder phrase. If it does, the plan-mode preamble
|
||||
// section is leaking outside plan mode.
|
||||
expect(obs.evidence).not.toContain(PLAN_MODE_REMINDER);
|
||||
|
||||
if (skillName !== 'plan-ceo-review') {
|
||||
// Scope-gate bypass must not misfire: no auto-select announcement
|
||||
// outside plan mode.
|
||||
expect(obs.scopeGateAutoSelectObserved ?? false).toBe(false);
|
||||
// And when a question fired, it must have been the scope gate —
|
||||
// outside plan mode with no named target, the gate is the FIRST
|
||||
// question by contract (hard STOP before any tool call).
|
||||
if (obs.outcome === 'asked') {
|
||||
expect(obs.scopeGateQuestionObserved ?? false).toBe(true);
|
||||
}
|
||||
}
|
||||
}, 360_000);
|
||||
}
|
||||
|
||||
// Named-target exception (outside plan mode): a pasted draft IS an
|
||||
// explicitly-named target, so the scope gate must NOT ask — and the
|
||||
// review must consume the pasted content (seed token visible in the
|
||||
// review output), proving the target was used rather than the question
|
||||
// merely skipped. Also the over-trigger guard for the tightened
|
||||
// "explicit-only" exception wording.
|
||||
test('plan-eng-review skips the scope gate for an explicitly-pasted target', async () => {
|
||||
const obs = await runPlanSkillObservation({
|
||||
skillName: 'plan-ceo-review',
|
||||
skillName: 'plan-eng-review',
|
||||
inPlanMode: false,
|
||||
initialPlanContent: NAMED_TARGET_SEED,
|
||||
timeoutMs: 300_000,
|
||||
});
|
||||
|
||||
if (obs.outcome === 'silent_write' || obs.outcome === 'exited' || obs.outcome === 'timeout') {
|
||||
throw new Error(
|
||||
`plan-mode no-op regression FAILED: outcome=${obs.outcome}\n` +
|
||||
`named-target no-op FAILED: outcome=${obs.outcome}\n` +
|
||||
`summary: ${obs.summary}\n` +
|
||||
`elapsed: ${obs.elapsedMs}ms\n` +
|
||||
`--- evidence (last 2KB visible) ---\n${obs.evidence}`,
|
||||
);
|
||||
}
|
||||
expect(['asked', 'plan_ready']).toContain(obs.outcome);
|
||||
|
||||
// Negative regression: the rendered output must NOT echo the plan-mode
|
||||
// distinctive reminder phrase. If it does, the plan-mode preamble
|
||||
// section is leaking outside plan mode.
|
||||
const PLAN_MODE_REMINDER =
|
||||
'Plan mode is active. The user indicated that they do not want you to execute yet';
|
||||
expect(obs.evidence).not.toContain(PLAN_MODE_REMINDER);
|
||||
|
||||
// The pasted doc is the named target: gate question must not render,
|
||||
// no plan-mode announcement either (we are NOT in plan mode).
|
||||
expect(obs.scopeGateQuestionObserved ?? false).toBe(false);
|
||||
expect(obs.scopeGateAutoSelectObserved ?? false).toBe(false);
|
||||
|
||||
// Target consumption: the review must reference the seeded feature.
|
||||
// evidence is the lossy 2KB tail, so also accept the token appearing in
|
||||
// the plan file the review wrote — the token names the component, route,
|
||||
// and test file every finding has to discuss, so absence from BOTH means
|
||||
// the pasted target was not actually reviewed.
|
||||
const inEvidence = obs.evidence.includes(SEED_TOKEN);
|
||||
const inPlanFile =
|
||||
!!obs.planFile && existsSync(obs.planFile) && readFileSync(obs.planFile, 'utf-8').includes(SEED_TOKEN);
|
||||
expect(inEvidence || inPlanFile).toBe(true);
|
||||
}, 360_000);
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user