From 3ddbee5952915840d2fa253f11ab70ea882fe719 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 12 Aug 2026 09:09:48 -0700 Subject: [PATCH] test(evals): harden no-op asserts, close tier-invariant fail-open holes, pin gate question strings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - no-op regression: gate-must-ask is now UNCONDITIONAL for eng/design (the outcome==='asked' conditional let a silent-bypass plan_ready run sail through); eng/design cases force --disallowedTools so the pinned prose shape is contractual rather than hoping native AUQ renders match; the named-target case uses trackTokens for consumption and lists wrote_findings_before_asking in its diagnostic throw branch. - tier-alignment invariant: both quote styles matched; zero-self-gate, mixed-tier, and owning-keys-without-E2E_TIERS-entries are all REPORTED instead of silently skipped (the fail-open holes three reviewers found). - drift-guard: the generated gate menus must carry the exact question/option strings the PTY question detector anchors on — free CI fails before the paid smokes can go vacuous on a menu reword. - touchfiles: corrected the no-op cost note for CI concurrency + retry semantics. Co-Authored-By: Claude Fable 5 --- test/e2e-tier-alignment.test.ts | 42 +++++++++++++++++------- test/gen-skill-docs.test.ts | 14 +++++++- test/helpers/touchfiles.ts | 7 ++-- test/skill-e2e-plan-mode-no-op.test.ts | 45 ++++++++++++++++---------- 4 files changed, 76 insertions(+), 32 deletions(-) diff --git a/test/e2e-tier-alignment.test.ts b/test/e2e-tier-alignment.test.ts index 681a8ce94..34451a0d4 100644 --- a/test/e2e-tier-alignment.test.ts +++ b/test/e2e-tier-alignment.test.ts @@ -22,7 +22,10 @@ import * as path from 'path'; import { E2E_TOUCHFILES, E2E_TIERS, LLM_JUDGE_TOUCHFILES } from './helpers/touchfiles'; const TEST_DIR = import.meta.dir; -const SELF_GATE_RE = /EVALS_TIER\s*===\s*'(gate|periodic)'/g; +// Both quote styles — a mechanical refactor to double quotes must not +// silently drop a file from the invariant (fail-open is the defect class +// this test exists to kill). +const SELF_GATE_RE = /EVALS_TIER\s*===\s*['"](gate|periodic)['"]/g; describe('E2E tier alignment (touchfiles declaration vs test self-gate)', () => { const testFiles = readdirSync(TEST_DIR) @@ -33,24 +36,40 @@ describe('E2E tier alignment (touchfiles declaration vs test self-gate)', () => test('every self-gated test file named in a dep list matches its declared tier', () => { const misaligned: string[] = []; - const unmapped: string[] = []; + const reported: string[] = []; for (const file of testFiles) { const content = readFileSync(path.join(TEST_DIR, file), 'utf-8'); const tiers = new Set(); for (const m of content.matchAll(SELF_GATE_RE)) tiers.add(m[1]); - if (tiers.size !== 1) continue; // no self-gate, or mixed-tier file — out of scope + const repoPath = `test/${file}`; + if (tiers.size === 0) { + // Every skill-e2e file is expected to self-gate; zero matches means + // either a genuinely ungated file or a gate shape the regex can't + // see — both worth a visible report, never a silent skip. + reported.push(`${repoPath}: no detectable EVALS_TIER self-gate`); + continue; + } + if (tiers.size > 1) { + reported.push(`${repoPath}: mixed-tier self-gates (${[...tiers].join(', ')}) — not tier-checked`); + continue; + } const selfTier = [...tiers][0]; - const repoPath = `test/${file}`; const owningKeys = Object.keys(allDeps).filter((k) => allDeps[k].includes(repoPath)); if (owningKeys.length === 0) { - unmapped.push(`${repoPath} (self-gates '${selfTier}')`); + reported.push(`${repoPath} (self-gates '${selfTier}'): not named in any touchfiles dep list`); continue; } for (const k of owningKeys) { const declared = E2E_TIERS[k]; - if (declared && declared !== selfTier) { + if (!declared) { + // A dep-list key with no E2E_TIERS entry (e.g. an LLM-judge key) + // can't tier-check this file — report instead of silently passing. + reported.push(`${repoPath}: matched key '${k}' which has no E2E_TIERS entry`); + continue; + } + if (declared !== selfTier) { misaligned.push( `${repoPath}: self-gates on '${selfTier}' but E2E_TIERS['${k}'] declares '${declared}' — the declaration is inert`, ); @@ -58,13 +77,12 @@ describe('E2E tier alignment (touchfiles declaration vs test self-gate)', () => } } - // Reported, not asserted: these files' evals can't be tier-checked until - // their dep lists name the test file. Add `test/` to the eval's - // touchfiles entry to bring them under the invariant. - if (unmapped.length > 0) { + // Reported, not asserted: coverage holes the invariant can see but not + // arbitrate. Add the test file to its eval's dep list (or a tier entry + // for the key) to bring it under the invariant. + if (reported.length > 0) { console.warn( - `[tier-alignment] ${unmapped.length} self-gated test file(s) not named in any touchfiles dep list:\n ` + - unmapped.join('\n '), + `[tier-alignment] ${reported.length} file(s) outside the invariant:\n ` + reported.join('\n '), ); } diff --git a/test/gen-skill-docs.test.ts b/test/gen-skill-docs.test.ts index d1c83642d..3174164bd 100644 --- a/test/gen-skill-docs.test.ts +++ b/test/gen-skill-docs.test.ts @@ -3256,7 +3256,7 @@ describe('scope-gate exceptions drift-guard', () => { // 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.'; + const END_MARKER = 'in any mode — it is a hard STOP.'; function extractExceptionsBlock(skill: string): string { const md = fs.readFileSync(path.join(ROOT, skill, 'SKILL.md'), 'utf-8'); @@ -3290,6 +3290,18 @@ describe('scope-gate exceptions drift-guard', () => { ); } }); + + test('gate menu carries the question strings the PTY question detector pins', () => { + // isScopeGateQuestionVisible (claude-pty-runner.ts) anchors on the + // question text + option A's body. If the menu is reworded without + // updating the detector, the paid smokes' must-stay-false assertions go + // vacuous — this free pin fails first. + for (const skill of ['plan-eng-review', 'plan-design-review']) { + const md = fs.readFileSync(path.join(ROOT, skill, 'SKILL.md'), 'utf-8'); + expect(md, `${skill}: gate question text`).toContain('What should I review?'); + expect(md, `${skill}: option A body text`).toContain('The current branch diff'); + } + }); }); describe('GSTACK REVIEW REPORT mandatory unresolved-decisions status', () => { diff --git a/test/helpers/touchfiles.ts b/test/helpers/touchfiles.ts index d538f3ec8..489b2a915 100644 --- a/test/helpers/touchfiles.ts +++ b/test/helpers/touchfiles.ts @@ -103,8 +103,11 @@ export const E2E_TOUCHFILES: Record = { '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'], // 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). + // fire outside plan mode) + the named-target exception case. 4 PTY runs; + // in CI these run CONCURRENT with the rest of the pty-plan-smoke suite + // (--max-concurrency + --retry 2), so worst-case cost is ~3x a single + // pass of each, sharing the API budget with sibling tests — not the + // sequential ~+10min a local read suggests. '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 diff --git a/test/skill-e2e-plan-mode-no-op.test.ts b/test/skill-e2e-plan-mode-no-op.test.ts index 7f1939ca3..ae1311156 100644 --- a/test/skill-e2e-plan-mode-no-op.test.ts +++ b/test/skill-e2e-plan-mode-no-op.test.ts @@ -31,7 +31,6 @@ */ 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'; @@ -64,6 +63,15 @@ describeE2E('plan-mode-info no-op outside plan mode (gate regression)', () => { skillName, inPlanMode: false, timeoutMs: 300_000, + // eng/design: force the prose-fallback path. The unconditional + // gate-must-ask assert below pins the render shape the detector + // anchors on, and only the --disallowedTools prose fallback makes + // that shape CONTRACTUAL ("use exactly this shape" in the template); + // native AskUserQuestion could render terse option labels that a + // correct run would fail on (red-team finding). + ...(skillName === 'plan-ceo-review' + ? {} + : { extraArgs: ['--disallowedTools', 'AskUserQuestion'] }), }); if (obs.outcome === 'silent_write' || obs.outcome === 'exited' || obs.outcome === 'timeout') { @@ -85,12 +93,13 @@ describeE2E('plan-mode-info no-op outside plan mode (gate regression)', () => { // 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); - } + // UNCONDITIONAL: outside plan mode with no named target, the gate is + // a hard STOP before any tool call, so the gate question must have + // rendered no matter which terminal outcome fired. Gating this on + // outcome === 'asked' would let a silent-bypass run that reaches + // plan_ready (isPlanReadyVisible also matches common prose) sail + // through — the exact regression this test exists to catch. + expect(obs.scopeGateQuestionObserved ?? false).toBe(true); } }, 360_000); } @@ -106,10 +115,16 @@ describeE2E('plan-mode-info no-op outside plan mode (gate regression)', () => { skillName: 'plan-eng-review', inPlanMode: false, initialPlanContent: NAMED_TARGET_SEED, + trackTokens: [SEED_TOKEN], timeoutMs: 300_000, }); - if (obs.outcome === 'silent_write' || obs.outcome === 'exited' || obs.outcome === 'timeout') { + if ( + obs.outcome === 'wrote_findings_before_asking' || + obs.outcome === 'silent_write' || + obs.outcome === 'exited' || + obs.outcome === 'timeout' + ) { throw new Error( `named-target no-op FAILED: outcome=${obs.outcome}\n` + `summary: ${obs.summary}\n` + @@ -125,14 +140,10 @@ describeE2E('plan-mode-info no-op outside plan mode (gate regression)', () => { 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); + // Target consumption via high-water token tracking over the CUMULATIVE + // buffer — the 2KB evidence tail is lossy and the plan-file fallback is + // unreachable outside plan mode (extractPlanFilePath only matches + // plan-mode save renders). + expect(obs.tokensObserved?.[SEED_TOKEN] ?? false).toBe(true); }, 360_000); });