From 4e6123302113e73d5cbfa6354b24de165a1f9000 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 12 Aug 2026 09:09:32 -0700 Subject: [PATCH] fix(pty-runner): positional floor exclusion, flag builder, outcome union, token tracking MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review-army + adversarial findings on the scope-gate observability work, all verified before fixing: - Floor check: acceptance scanned the CUMULATIVE buffer while the scope-gate exclusion scanned only the 1500-byte tail, so an early gate render satisfied the floor vacuously once ~1.5KB of output accumulated (found independently by 4 review passes; predicate reproduced). Acceptance now scans only content APPENDED after the first gate render (positional anchor), and the LLM-judge 'waiting' shortcut no longer fires while the gate menu is the pending render. - High-water flags are built once and spread at every return path — the hand-spread pattern had already drifted (judge-waiting return omitted two flags), which made must-stay-false asserts vacuous on those paths. - isScopeGateAutoSelectVisible: tense-tolerant selected/selecting/selects token (must-be-TRUE asserts shouldn't fail semantically-perfect paraphrases) and quoted-occurrence rejection (a model verbatim-quoting the announcement while declining must not trip must-stay-FALSE asserts). Fixtures added for both directions. - PlanSkillObservation outcome union gains 'wrote_findings_before_asking' (returned at runtime via classifyVisible but missing from the type). - trackTokens/tokensObserved: cumulative-buffer token high-water for consumption asserts (the 2KB evidence tail is lossy and the plan-file fallback is unreachable outside plan mode). - New scope-gate-floor unit pins (from the ship coverage audit): both gate render forms trip acceptance and exclusion; a genuine finding AUQ is not excluded; tail-scoping semantics pinned. Co-Authored-By: Claude Fable 5 --- ...e-pty-runner.scope-gate-floor.unit.test.ts | 122 ++++++++++++++++++ test/helpers/claude-pty-runner.ts | 117 ++++++++++++----- test/helpers/claude-pty-runner.unit.test.ts | 27 ++++ 3 files changed, 236 insertions(+), 30 deletions(-) create mode 100644 test/helpers/claude-pty-runner.scope-gate-floor.unit.test.ts diff --git a/test/helpers/claude-pty-runner.scope-gate-floor.unit.test.ts b/test/helpers/claude-pty-runner.scope-gate-floor.unit.test.ts new file mode 100644 index 000000000..66bf75d53 --- /dev/null +++ b/test/helpers/claude-pty-runner.scope-gate-floor.unit.test.ts @@ -0,0 +1,122 @@ +/** + * Scope-gate floor-exclusion regression pins (free, static). + * + * runPlanSkillFloorCheck's acceptance condition changed with the plan-mode + * auto-select-B work: a render only satisfies the finding floor when + * + * (isNumberedOptionListVisible(visible) || isProseAUQVisible(visible)) + * && !isPermissionDialogVisible(tail) + * && !isScopeGateQuestionVisible(tail) // <- new exclusion + * + * where tail = visible.slice(-TAIL_SCAN_BYTES). The composition lives inline + * in the paid PTY loop, so these tests pin the load-bearing behavior of each + * detector on the exact render shapes the floor passes them: + * + * 1. Both scope-gate render forms (native numbered UI, prose lettered + * fallback) trip the acceptance detectors — WITHOUT the exclusion the + * gate would trivially satisfy the floor inside the 3s pre-target + * window. The exclusion must catch both forms. + * 2. A genuine finding-driven AskUserQuestion must NOT trip the exclusion, + * or the floor becomes unsatisfiable. + * 3. The exclusion is TAIL-scoped by design: an early gate render that has + * scrolled past TAIL_SCAN_BYTES must not suppress a later real finding + * AskUserQuestion. + * + * Also closes the untested OR-branch of isScopeGateAutoSelectVisible: the + * fully-collapsed hyphen-less 'autoselectedb' form. + */ + +import { describe, test, expect } from 'bun:test'; +import { + TAIL_SCAN_BYTES, + isNumberedOptionListVisible, + isProseAUQVisible, + isPermissionDialogVisible, + isScopeGateQuestionVisible, + isScopeGateAutoSelectVisible, +} from './claude-pty-runner'; + +// The gate's native AskUserQuestion render (numbered options + cursor) — +// what fires inside the floor check's 3s window before the seed arrives. +const GATE_NATIVE_RENDER = ` + What should I review? + + ❯ 1. The current branch diff — the work in progress on this branch. + 2. A plan or design doc I'll paste or point you to. + 3. A specific file, directory, or path. +`; + +// The gate's prose fallback render (lettered options under --disallowedTools). +const GATE_PROSE_RENDER = ` +What should I review? +A) The current branch diff — the work in progress on this branch. +B) A plan or design doc I'll paste or point you to. +C) A specific file, directory, or path. +Recommendation: A when a branch diff exists, otherwise B. +`; + +// A genuine finding-driven AskUserQuestion — the render the floor MEASURES. +const FINDING_AUQ_RENDER = ` +Finding 1: the plan reimplements test sharding that Bun provides natively. + + ❯ 1. Use Bun's native --shard flag (recommended) + 2. Keep the custom scheduler as planned + 3. Defer this decision to implementation +`; + +describe('floor-check scope-gate exclusion (acceptance-condition regression)', () => { + test('native gate render trips the acceptance detector — the exclusion is load-bearing', () => { + // Pre-exclusion, this render satisfied the floor by itself. + expect(isNumberedOptionListVisible(GATE_NATIVE_RENDER)).toBe(true); + expect(isPermissionDialogVisible(GATE_NATIVE_RENDER)).toBe(false); + // The new exclusion catches it. + expect(isScopeGateQuestionVisible(GATE_NATIVE_RENDER)).toBe(true); + }); + + test('prose gate render trips the prose-AUQ arm — the exclusion catches that form too', () => { + expect(isProseAUQVisible(GATE_PROSE_RENDER)).toBe(true); + expect(isPermissionDialogVisible(GATE_PROSE_RENDER)).toBe(false); + expect(isScopeGateQuestionVisible(GATE_PROSE_RENDER)).toBe(true); + }); + + test('a genuine finding AskUserQuestion is NOT excluded — the floor stays satisfiable', () => { + expect(isNumberedOptionListVisible(FINDING_AUQ_RENDER)).toBe(true); + expect(isPermissionDialogVisible(FINDING_AUQ_RENDER)).toBe(false); + expect(isScopeGateQuestionVisible(FINDING_AUQ_RENDER)).toBe(false); + }); + + test('tail-scoping: an early gate render scrolled out of the tail does not suppress a later finding AUQ', () => { + // Gate render, then >TAIL_SCAN_BYTES of review output, then the real + // finding AskUserQuestion — the shape the TAIL-scoped exclusion exists for. + const filler = 'Reading the plan and auditing the design system.\n'.repeat( + Math.ceil(TAIL_SCAN_BYTES / 48) + 4, + ); + const visible = GATE_NATIVE_RENDER + filler + FINDING_AUQ_RENDER; + const tail = visible.slice(-TAIL_SCAN_BYTES); + + // Full buffer still remembers the gate (scrollback)… + expect(isScopeGateQuestionVisible(visible)).toBe(true); + // …but the floor's exclusion looks only at the tail, which is clean: + expect(isScopeGateQuestionVisible(tail)).toBe(false); + // and the acceptance arm (full-buffer scan) sees the finding AUQ. + expect(isNumberedOptionListVisible(visible)).toBe(true); + expect(isPermissionDialogVisible(tail)).toBe(false); + }); + + test('a gate render inside the tail IS suppressed (no false floor pass)', () => { + const tail = GATE_NATIVE_RENDER.slice(-TAIL_SCAN_BYTES); + expect(isScopeGateQuestionVisible(tail)).toBe(true); + }); +}); + +describe('isScopeGateAutoSelectVisible collapsed hyphen-less branch', () => { + test("matches the fully-collapsed 'autoselectedb' form (hyphen lost in TTY reflow)", () => { + const sample = 'Scopegate:planmode—autoselectedB(reviewingPLAN.md).'; + expect(isScopeGateAutoSelectVisible(sample)).toBe(true); + }); + + test('hyphen-less token without the announcement prefix stays false', () => { + const sample = 'The agent autoselectedB from the menu without announcing a scope gate decision.'; + expect(isScopeGateAutoSelectVisible(sample)).toBe(false); + }); +}); diff --git a/test/helpers/claude-pty-runner.ts b/test/helpers/claude-pty-runner.ts index 49e7fc8cf..dd44b9671 100644 --- a/test/helpers/claude-pty-runner.ts +++ b/test/helpers/claude-pty-runner.ts @@ -644,15 +644,27 @@ export function isScopeGateQuestionVisible(visible: string): boolean { /** * True when the plan-mode auto-select announcement is rendered: * "Scope gate: plan mode — auto-selected B (reviewing )." - * Requires BOTH the announcement prefix and the selected-B token so - * narration ("in plan mode I'd auto-select B") stays false. + * Requires BOTH the announcement prefix and an auto-select-B token so + * narration ("in plan mode I'd auto-select B") stays false. The token is + * tense-tolerant (selected/selecting/selects) because the smokes assert + * must-be-TRUE on it — a semantically-perfect paraphrase must not fail a + * paid run — while the prefix stays exact so paraphrase narration without + * the announcement frame stays false. A prefix immediately preceded by a + * quote character is a QUOTATION (e.g. the model explaining why it is NOT + * announcing), not a render — the announcement line itself never renders + * quoted. */ export function isScopeGateAutoSelectVisible(visible: string): boolean { const squished = visible.replace(/\s+/g, '').toLowerCase(); - return ( - squished.includes('scopegate:planmode') && - (squished.includes('auto-selectedb') || squished.includes('autoselectedb')) - ); + const QUOTES = ['"', "'", '`', '“', '‘']; + const re = /scopegate:planmode/g; + let m: RegExpExecArray | null; + while ((m = re.exec(squished)) !== null) { + const before = m.index > 0 ? squished[m.index - 1]! : ''; + if (QUOTES.includes(before)) continue; // quoted occurrence — narration, keep scanning + if (/auto-?select(?:ed|ing|s)?b/.test(squished.slice(m.index))) return true; + } + return false; } /** @@ -1513,10 +1525,20 @@ export interface PlanSkillObservation { * "Ready to execute" confirmation * - 'silent_write' — a Write/Edit landed BEFORE any prompt, to a path * outside the sanctioned plan/project directories + * - 'wrote_findings_before_asking' — strictPlanWrites only (seeded runs): + * the plan file was rewritten with findings before any + * AskUserQuestion render (the May-2026 transcript bug) * - 'exited' — claude process died before any of the above * - 'timeout' — none of the above within budget */ - outcome: 'asked' | 'auto_decided' | 'plan_ready' | 'silent_write' | 'exited' | 'timeout'; + outcome: + | 'asked' + | 'auto_decided' + | 'plan_ready' + | 'silent_write' + | 'wrote_findings_before_asking' + | 'exited' + | 'timeout'; /** Human-readable summary. */ summary: string; /** Visible terminal text since the slash command was sent (last 2KB). */ @@ -1567,6 +1589,14 @@ export interface PlanSkillObservation { * plan-mode smokes assert true; the no-op regression asserts false. */ scopeGateAutoSelectObserved?: boolean; + /** + * High-water map for opts.trackTokens: token → did it EVER appear in the + * cumulative visible buffer? Consumption asserts (e.g. "the pasted target's + * distinctive token shows up in the review output") must not depend on the + * lossy 2KB evidence tail — plan-file fallbacks are unreachable outside + * plan mode (extractPlanFilePath only matches plan-mode save renders). + */ + tokensObserved?: Record; } /** @@ -1627,6 +1657,10 @@ export async function runPlanSkillObservation(opts: { /** Override the spawned model. Defaults via launchClaudePty's chain * (opts.model ?? EVALS_MODEL ?? 'claude-sonnet-4-6'). */ model?: string; + /** Literal tokens to track as high-water marks over the CUMULATIVE visible + * buffer (case-sensitive). Results land in obs.tokensObserved. Use for + * consumption asserts that must survive the 2KB evidence tail. */ + trackTokens?: string[]; }): Promise { const startedAt = Date.now(); const session = await launchClaudePty({ @@ -1672,6 +1706,19 @@ export async function runPlanSkillObservation(opts: { let waitingEverObserved = false; let scopeGateQuestionObserved = false; let scopeGateAutoSelectObserved = false; + const tokensObserved: Record = {}; + for (const t of opts.trackTokens ?? []) tokensObserved[t] = false; + // Single source for the high-water flags at EVERY return site. Hand- + // spreading them per-site already drifted once (the judge-waiting return + // omitted the prose/waiting flags); a site that forgets a must-stay-false + // flag makes `obs.flag ?? false` negative assertions pass vacuously. + const highWaterFlags = () => ({ + proseAUQEverObserved, + waitingEverObserved, + scopeGateQuestionObserved, + scopeGateAutoSelectObserved, + ...(opts.trackTokens?.length ? { tokensObserved } : {}), + }); const JUDGE_AFTER_MS = 60_000; const JUDGE_INTERVAL_MS = 30_000; while (Date.now() - start < budgetMs) { @@ -1684,8 +1731,7 @@ export async function runPlanSkillObservation(opts: { summary: `claude exited (code=${session.exitCode()}) before reaching a terminal outcome`, evidence: visible.slice(-2000), elapsedMs: Date.now() - startedAt, - scopeGateQuestionObserved, - scopeGateAutoSelectObserved, + ...highWaterFlags(), }; } if (visible.includes('Unknown command:')) { @@ -1694,8 +1740,7 @@ export async function runPlanSkillObservation(opts: { summary: `claude rejected /${opts.skillName} as unknown command (skill not registered in this cwd)`, evidence: visible.slice(-2000), elapsedMs: Date.now() - startedAt, - scopeGateQuestionObserved, - scopeGateAutoSelectObserved, + ...highWaterFlags(), }; } @@ -1718,6 +1763,9 @@ export async function runPlanSkillObservation(opts: { if (!scopeGateAutoSelectObserved && isScopeGateAutoSelectVisible(visible)) { scopeGateAutoSelectObserved = true; } + for (const t of opts.trackTokens ?? []) { + if (!tokensObserved[t] && visible.includes(t)) tokensObserved[t] = true; + } const classified = classifyVisible(visible, { strictPlanWrites: !!opts.initialPlanContent, @@ -1727,10 +1775,7 @@ export async function runPlanSkillObservation(opts: { ...classified, evidence: visible.slice(-2000), elapsedMs: Date.now() - startedAt, - proseAUQEverObserved, - waitingEverObserved, - scopeGateQuestionObserved, - scopeGateAutoSelectObserved, + ...highWaterFlags(), }; // Capture the plan file path on any outcome where one may have been // written. Gating only on 'plan_ready' missed two cases: (1) the @@ -1761,8 +1806,7 @@ export async function runPlanSkillObservation(opts: { summary: `LLM judge: ${lastJudgeVerdict.reasoning} (state=waiting after ${Math.round(elapsed / 1000)}s)`, evidence: visible.slice(-2000), elapsedMs: Date.now() - startedAt, - scopeGateQuestionObserved, - scopeGateAutoSelectObserved, + ...highWaterFlags(), }; } } @@ -1784,10 +1828,7 @@ export async function runPlanSkillObservation(opts: { : ''), evidence: finalVisible.slice(-2000), elapsedMs: Date.now() - startedAt, - proseAUQEverObserved, - waitingEverObserved, - scopeGateQuestionObserved, - scopeGateAutoSelectObserved, + ...highWaterFlags(), }; } return { @@ -1799,10 +1840,7 @@ export async function runPlanSkillObservation(opts: { : ''), evidence: finalVisible.slice(-2000), elapsedMs: Date.now() - startedAt, - proseAUQEverObserved, - waitingEverObserved, - scopeGateQuestionObserved, - scopeGateAutoSelectObserved, + ...highWaterFlags(), }; } finally { await session.close(); @@ -2173,11 +2211,23 @@ export async function runPlanSkillFloorCheck(opts: { const start = Date.now(); let lastJudgeAt = 0; let lastJudgeVerdict: PtyStateVerdict | null = null; + // Positional anchor for the scope-gate exclusion. The visible buffer is + // append-only (old renders never leave scrollback), so a gate question + // rendered in the 3s pre-target window would keep satisfying the + // full-buffer acceptance checks forever while a tail-only exclusion + // stops seeing it after ~TAIL_SCAN_BYTES of output — a vacuous + // auq_observed (found independently by 4 review passes). Once the gate + // render is seen, acceptance only counts AUQ renders in content APPENDED + // after that point. + let gateSeenIdx = -1; const JUDGE_AFTER_MS = 60_000; const JUDGE_INTERVAL_MS = 30_000; while (Date.now() - start < timeoutMs) { await Bun.sleep(2000); const visible = session.visibleSince(since); + if (gateSeenIdx === -1 && isScopeGateQuestionVisible(visible)) { + gateSeenIdx = visible.length; + } if (session.exited()) { return { @@ -2206,12 +2256,16 @@ export async function runPlanSkillFloorCheck(opts: { // // Scope-gate renders do NOT count: the gate's "What should I review?" // can fire inside the 3s pre-target window and would trivially satisfy - // the floor, but the floor measures FINDING-driven questions. The - // exclusion is TAIL-scoped so an early gate render that has scrolled - // out doesn't suppress a later, real finding AUQ. + // the floor, but the floor measures FINDING-driven questions. Once a + // gate render has been seen, acceptance scans only the content APPENDED + // after it (positional anchor above) — the buffer is append-only, so a + // whole-buffer acceptance would keep matching the stale gate render + // forever. The tail exclusion additionally covers the window where the + // gate menu is still the active render. const tail = visible.slice(-TAIL_SCAN_BYTES); + const acceptWindow = gateSeenIdx === -1 ? visible : visible.slice(gateSeenIdx); if ( - (isNumberedOptionListVisible(visible) || isProseAUQVisible(visible)) && + (isNumberedOptionListVisible(acceptWindow) || isProseAUQVisible(acceptWindow)) && !isPermissionDialogVisible(tail) && !isScopeGateQuestionVisible(tail) ) { @@ -2235,7 +2289,10 @@ export async function runPlanSkillFloorCheck(opts: { lastJudgeAt = Date.now(); logPtySnapshot(visible, { testName: opts.skillName, elapsedMs: elapsed, tag: 'floor-judge-tick' }); lastJudgeVerdict = judgePtyState(visible, { testName: opts.skillName }); - if (lastJudgeVerdict.state === 'waiting') { + // The judge can't tell a scope-gate question from a finding question, + // so a 'waiting' verdict while the gate menu is the pending render + // must NOT satisfy the floor — same exclusion as the regex path. + if (lastJudgeVerdict.state === 'waiting' && !isScopeGateQuestionVisible(tail)) { return { auqObserved: true, outcome: 'auq_observed', diff --git a/test/helpers/claude-pty-runner.unit.test.ts b/test/helpers/claude-pty-runner.unit.test.ts index 37f027938..b46a3faed 100644 --- a/test/helpers/claude-pty-runner.unit.test.ts +++ b/test/helpers/claude-pty-runner.unit.test.ts @@ -264,6 +264,33 @@ Recommendation: A when a branch diff exists, otherwise B. expect(isScopeGateAutoSelectVisible(sample)).toBe(false); }); + test('stays false on a VERBATIM QUOTE of the announcement (negation narration)', () => { + // The exact announcement line sits quoted in the skill context, so a + // model explaining why it is NOT firing it can reproduce it byte-exact + // inside quotes — that must not trip a must-stay-false assert. + const sample = + 'Not in plan mode, so I won\'t announce "Scope gate: plan mode — auto-selected B (reviewing )." and will ask instead.'; + expect(isScopeGateAutoSelectVisible(sample)).toBe(false); + }); + + test('a later real render still matches after an earlier quoted mention', () => { + const sample = + 'Earlier I said I would render "Scope gate: plan mode — auto-selected B (…)" and now:\n' + + 'Scope gate: plan mode — auto-selected B (reviewing PLAN.md).'; + expect(isScopeGateAutoSelectVisible(sample)).toBe(true); + }); + + test('matches tense paraphrases WITH the announcement prefix (auto-selecting / auto-selects)', () => { + expect( + isScopeGateAutoSelectVisible('Scope gate: plan mode — auto-selecting B (reviewing the drafted plan).'), + ).toBe(true); + expect(isScopeGateAutoSelectVisible('Scope gate: plan mode — auto-selects B.')).toBe(true); + }); + + test('stays false on tense paraphrases WITHOUT the announcement prefix', () => { + expect(isScopeGateAutoSelectVisible('Auto-selecting B since we are in plan mode.')).toBe(false); + }); + test('stays false on AUTO_DECIDE preamble output', () => { const sample = 'Auto-decided scope question → B (your preference). Change with /plan-tune.'; expect(isScopeGateAutoSelectVisible(sample)).toBe(false);