From e0662ea7b5d96a2d7dd344361906ab690cc48c5c Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Thu, 9 Jul 2026 19:13:54 -0700 Subject: [PATCH] fix(one-way-doors): unify credential noun net + wire it into the runtime (#2024) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Library fix: revoke/reset/rotate now share ONE noun alternation (api key, token, secret, credential, access key, password) with optional plural s?. Pre-fix leaks: "reset my secret", "reset my access key", "revoke my secret" (mismatched per-verb lists) and every plural form ("rotate the credentials", "revoke all tokens" — \b(...)\b cannot match a trailing s). Runtime wiring — the regexes could never fire in production before: - gstack-question-preference --check gains --summary-stdin: the question text pipes via stdin (never argv — summaries carry quotes/newlines/shell metacharacters) and feeds isOneWayDoor alongside the id, so an ad-hoc destructive question with a stored never-ask preference now forces ASK_NORMALLY. Empty/absent stdin keeps exact id-only semantics. - question-preference-hook falls back to classifyQuestion(question text) when the registry lookup misses, so unregistered destructive questions pass through to a human instead of auto-deciding. - question-tuning resolver prose shows the piped form (SKILL.md regen lands in the wave's release commit). Tripwires (verified fail-first): full verbs x nouns x singular/plural matrix with the #2024 repro rows, benign-summary no-over-match rows, stdin transport survival (quotes/newlines), empty-stdin fail-safe, and hook fallback both directions (destructive -> pass-through, benign -> deny). Co-Authored-By: Claude Fable 5 --- bin/gstack-question-preference | 27 +++++++-- .../claude/hooks/question-preference-hook.ts | 17 +++++- scripts/one-way-doors.ts | 13 +++-- scripts/resolvers/question-tuning.ts | 8 ++- test/gstack-question-preference.test.ts | 57 +++++++++++++++++++ test/one-way-doors.test.ts | 47 +++++++++++++++ test/question-preference-hook.test.ts | 41 +++++++++++++ 7 files changed, 199 insertions(+), 11 deletions(-) diff --git a/bin/gstack-question-preference b/bin/gstack-question-preference index eb951ebd3..34271aeef 100755 --- a/bin/gstack-question-preference +++ b/bin/gstack-question-preference @@ -5,7 +5,9 @@ # Schema: { "": "always-ask" | "never-ask" | "ask-only-for-one-way" } # # Subcommands: -# --check → emit ASK_NORMALLY | AUTO_DECIDE | ASK_ONLY_ONE_WAY +# --check [--summary-stdin] → emit ASK_NORMALLY | AUTO_DECIDE | ASK_ONLY_ONE_WAY +# (--summary-stdin pipes the question text so the +# keyword net can catch ad-hoc destructive ids, #2024) # --write '{...}' → set a preference (user-origin gate enforced) # --read → dump preferences JSON # --clear [] → clear one or all preferences @@ -44,22 +46,39 @@ ensure_file() { # --check # ----------------------------------------------------------------------- do_check() { - local QID="${1:-}" + local QID="" SUMMARY_STDIN=false + while [ $# -gt 0 ]; do + case "$1" in + --summary-stdin) SUMMARY_STDIN=true; shift ;; + *) [ -z "$QID" ] && QID="$1"; shift ;; + esac + done if [ -z "$QID" ]; then echo "ASK_NORMALLY" return 0 fi + # #2024: the question text feeds the keyword classifier for unregistered + # ids. Transport is stdin (not argv) — summaries carry quotes, newlines, + # and shell metacharacters an argv tail would mangle. Handed to bun via + # env so no shell re-quoting happens. Empty summary = id-only behavior. + local QSUMMARY="" + if [ "$SUMMARY_STDIN" = true ]; then + QSUMMARY=$(cat 2>/dev/null || true) + fi ensure_file cd "$ROOT_DIR" - PREF_FILE_PATH="$PREF_FILE" QID="$QID" bun -e " + PREF_FILE_PATH="$PREF_FILE" QID="$QID" QSUMMARY="$QSUMMARY" bun -e " import('./scripts/one-way-doors.ts').then((oneway) => { const fs = require('fs'); const qid = process.env.QID; + const summary = process.env.QSUMMARY || undefined; const prefs = JSON.parse(fs.readFileSync(process.env.PREF_FILE_PATH, 'utf-8')); const pref = prefs[qid]; // Always check one-way status first — safety overrides preferences. - const oneWay = oneway.isOneWayDoor({ question_id: qid }); + // summary (when piped) lets the keyword net catch ad-hoc destructive + // questions whose id has no registry entry (#2024). + const oneWay = oneway.isOneWayDoor({ question_id: qid, summary }); if (oneWay) { console.log('ASK_NORMALLY'); diff --git a/hosts/claude/hooks/question-preference-hook.ts b/hosts/claude/hooks/question-preference-hook.ts index 559102fda..505eea382 100644 --- a/hosts/claude/hooks/question-preference-hook.ts +++ b/hosts/claude/hooks/question-preference-hook.ts @@ -45,6 +45,7 @@ import * as path from 'path'; import * as os from 'os'; import { spawnSync } from 'child_process'; import { isConductor } from '../../../lib/is-conductor'; +import { classifyQuestion } from '../../../scripts/one-way-doors'; interface HookStdin { session_id?: string; @@ -434,7 +435,21 @@ async function main(): Promise { if (!pref.preference || pref.preference === 'always-ask') { fullyAutoDecidable = false; break; } const entry = registry[questionId]; - const doorType = entry?.door_type || 'two-way'; + let doorType: string = entry?.door_type || 'two-way'; + if (!entry) { + // #2024: an unregistered id used to default straight to two-way without + // consulting the keyword net, so an ad-hoc DESTRUCTIVE question with a + // stored never-ask preference auto-decided. classifyQuestion is a pure + // regex pass over the question text; on any failure keep the default + // (enforcement still requires an explicit stored preference). + try { + if (classifyQuestion({ summary: qText.replace(MARKER_RE, '').trim() }).oneWay) { + doorType = 'one-way'; + } + } catch (e) { + logHookError(`one-way classifier failed: ${(e as Error).message}`); + } + } // Safety override — even never-ask doesn't bypass one-way doors. if (doorType === 'one-way') { fullyAutoDecidable = false; break; } diff --git a/scripts/one-way-doors.ts b/scripts/one-way-doors.ts index 6735c386d..e0e4b67b4 100644 --- a/scripts/one-way-doors.ts +++ b/scripts/one-way-doors.ts @@ -62,10 +62,15 @@ const DESTRUCTIVE_PATTERNS: RegExp[] = [ /\bterraform\s+destroy\b/i, /\brollback\b/i, - // Credentials / auth — allow filler words ("the", "my") between verb and noun - /\brevoke\s+[\w\s]*\b(api key|token|credential|access key|password)\b/i, - /\breset\s+[\w\s]*\b(api key|token|password|credential)\b/i, - /\brotate\s+[\w\s]*\b(api key|token|secret|credential|access key|password)\b/i, + // Credentials / auth — allow filler words ("the", "my") between verb and noun. + // All three verbs share ONE noun list (#2024: mismatched alternations let + // "reset my secret" / "reset my access key" / "revoke my secret" leak as + // two-way), with optional plural (`s?` — \b(...)\b alone cannot match + // "credentials"). Keep these parallel: a verb-specific noun list is how + // this class of false negative happens. + /\brevoke\s+[\w\s]*\b(api key|token|secret|credential|access key|password)s?\b/i, + /\breset\s+[\w\s]*\b(api key|token|secret|credential|access key|password)s?\b/i, + /\brotate\s+[\w\s]*\b(api key|token|secret|credential|access key|password)s?\b/i, // Scope / architecture forks (reversible with effort — still deserve confirmation) /\barchitectur(e|al)\s+(change|fork|shift|decision)\b/i, diff --git a/scripts/resolvers/question-tuning.ts b/scripts/resolvers/question-tuning.ts index d9c843a3e..67a32469f 100644 --- a/scripts/resolvers/question-tuning.ts +++ b/scripts/resolvers/question-tuning.ts @@ -23,7 +23,11 @@ export function generateQuestionTuning(ctx: TemplateContext): string { const bin = binDir(ctx); return `## Question Tuning (skip entirely if \`QUESTION_TUNING: false\`) -Before each AskUserQuestion, choose \`question_id\` from \`scripts/question-registry.ts\` or \`{skill}-{slug}\`, then run \`${bin}/gstack-question-preference --check ""\`. \`AUTO_DECIDE\` means choose the recommended option and say "Auto-decided [summary] → [option] (your preference). Change with /plan-tune." \`ASK_NORMALLY\` means ask. +Before each AskUserQuestion, choose \`question_id\` from \`scripts/question-registry.ts\` or \`{skill}-{slug}\`, then run the check with the question summary piped on stdin (the summary lets the one-way keyword net catch ad-hoc destructive questions whose id has no registry entry — #2024; stdin, never argv, so quotes/newlines survive): +\`\`\`bash +printf '%s' "" | ${bin}/gstack-question-preference --check "" --summary-stdin +\`\`\` +\`AUTO_DECIDE\` means choose the recommended option and say "Auto-decided [summary] → [option] (your preference). Change with /plan-tune." \`ASK_NORMALLY\` means ask. **Embed the question_id as a marker in the question text** so hooks can identify it deterministically (plan-tune cathedral T14 / D18 progressive markers). Append \`\` somewhere in the rendered question (the leading line or trailing line is fine; the marker doesn't render visibly to the user when wrapped in HTML-style angle brackets, but the hook strips it). Without the marker the PreToolUse enforcement hook treats the AUQ as observed-only and never auto-decides — so always include it when the question matches a registered \`question_id\`. @@ -51,7 +55,7 @@ export function generateQuestionPreferenceCheck(ctx: TemplateContext): string { const bin = binDir(ctx); return `## Question Preference Check (skip if \`QUESTION_TUNING: false\`) -Before each AskUserQuestion, run: \`${bin}/gstack-question-preference --check ""\`. +Before each AskUserQuestion, run: \`printf '%s' "" | ${bin}/gstack-question-preference --check "" --summary-stdin\`. \`AUTO_DECIDE\` → auto-choose recommended with inline annotation. \`ASK_NORMALLY\` → ask.`; } diff --git a/test/gstack-question-preference.test.ts b/test/gstack-question-preference.test.ts index 863cd9e74..8b6fcbe04 100644 --- a/test/gstack-question-preference.test.ts +++ b/test/gstack-question-preference.test.ts @@ -39,6 +39,20 @@ function run(...args: string[]): { stdout: string; stderr: string; status: numbe }; } +function runWithStdin(input: string, ...args: string[]): { stdout: string; stderr: string; status: number } { + const res = spawnSync(BIN, args, { + env: { ...process.env, GSTACK_HOME: tmpHome }, + encoding: 'utf-8', + cwd: ROOT, + input, + }); + return { + stdout: res.stdout ?? '', + stderr: res.stderr ?? '', + status: res.status ?? -1, + }; +} + // ----------------------------------------------------------------------- // --check // ----------------------------------------------------------------------- @@ -103,6 +117,49 @@ describe('--check with preferences set', () => { }); }); +// #2024: the keyword net only fires when the question TEXT reaches the +// classifier. --summary-stdin pipes it (stdin, not argv — summaries carry +// quotes/newlines/shell metacharacters). Without the summary, an unregistered +// id with never-ask auto-decides even for destructive phrasings. +describe('--check --summary-stdin (#2024 keyword net plumb-through)', () => { + function setPref(id: string, pref: string) { + return run('--write', JSON.stringify({ question_id: id, preference: pref, source: 'plan-tune' })); + } + + test('destructive summary on unregistered never-ask id → ASK_NORMALLY (keyword net fires)', () => { + setPref('adhoc-cleanup-question', 'never-ask'); + const r = runWithStdin('Should I reset my secrets now?', '--check', 'adhoc-cleanup-question', '--summary-stdin'); + expect(r.status).toBe(0); + expect(r.stdout).toContain('ASK_NORMALLY'); + expect(r.stdout).toContain('one-way door overrides'); + }); + + test('same id WITHOUT summary still AUTO_DECIDEs (id-only fallback, current semantics)', () => { + setPref('adhoc-cleanup-question', 'never-ask'); + const r = run('--check', 'adhoc-cleanup-question'); + expect(r.stdout.trim()).toContain('AUTO_DECIDE'); + }); + + test('benign summary on unregistered never-ask id → AUTO_DECIDE (no over-match)', () => { + setPref('adhoc-cleanup-question', 'never-ask'); + const r = runWithStdin('Reorganize the TODOs file?', '--check', 'adhoc-cleanup-question', '--summary-stdin'); + expect(r.stdout.trim()).toContain('AUTO_DECIDE'); + }); + + test('summary with quotes/newlines/dashes survives the stdin transport', () => { + setPref('adhoc-cleanup-question', 'never-ask'); + const summary = 'Run "cleanup" --now\nthen rotate the access keys?'; + const r = runWithStdin(summary, '--check', 'adhoc-cleanup-question', '--summary-stdin'); + expect(r.stdout).toContain('ASK_NORMALLY'); + }); + + test('empty stdin with --summary-stdin → id-only behavior (fail-safe)', () => { + setPref('adhoc-cleanup-question', 'never-ask'); + const r = runWithStdin('', '--check', 'adhoc-cleanup-question', '--summary-stdin'); + expect(r.stdout.trim()).toContain('AUTO_DECIDE'); + }); +}); + // Split-chain carve-out: question_ids matching -split- // must always ASK_NORMALLY regardless of stored preferences. // See scripts/resolvers/preamble/generate-ask-user-format.ts diff --git a/test/one-way-doors.test.ts b/test/one-way-doors.test.ts index 382200408..618fce8a1 100644 --- a/test/one-way-doors.test.ts +++ b/test/one-way-doors.test.ts @@ -30,3 +30,50 @@ describe("one-way-door credential keyword net (#1839)", () => { } }); }); + +describe("one-way-door credential keyword net (#2024)", () => { + const VERBS = ["revoke", "reset", "rotate"]; + const NOUNS = ["api key", "token", "secret", "credential", "access key", "password"]; + + // #2024 repro rows: these leaked as two-way pre-fix because the noun + // alternations were mismatched across verbs (revoke lacked secret; reset + // lacked secret AND access key). The password-parallel test above passes on + // buggy code, so THESE rows are the fails-first proof. + test('"reset my secret" / "reset my access key" / "revoke my secret" classify one-way', () => { + for (const summary of ["reset my secret", "reset my access key", "revoke my secret"]) { + const r = classifyQuestion({ summary }); + expect(r.oneWay).toBe(true); + expect(r.reason).toBe("keyword"); + } + }); + + test("full verbs x nouns matrix classifies one-way (singular and plural)", () => { + for (const verb of VERBS) { + for (const noun of NOUNS) { + for (const form of [noun, `${noun}s`]) { + const r = classifyQuestion({ summary: `${verb} the production ${form}` }); + expect(r.oneWay).toBe(true); + expect(r.reason).toBe("keyword"); + } + } + } + }); + + // Plural forms leaked before AND after the original #2024 report: \b(...)\b + // cannot match "credentials" (no word boundary between the noun and its s). + test('plurals: "rotate the credentials" / "revoke all tokens" / "reset the passwords" classify one-way', () => { + for (const summary of ["rotate the credentials", "revoke all tokens", "reset the passwords"]) { + expect(classifyQuestion({ summary }).oneWay).toBe(true); + } + }); + + test("benign summaries stay two-way (no over-match)", () => { + for (const summary of [ + "reset the flaky test runner", + "rotate the log files nightly", + "revoke the meeting invite", + ]) { + expect(classifyQuestion({ summary }).oneWay).toBe(false); + } + }); +}); diff --git a/test/question-preference-hook.test.ts b/test/question-preference-hook.test.ts index da0184f9d..d96843f4e 100644 --- a/test/question-preference-hook.test.ts +++ b/test/question-preference-hook.test.ts @@ -298,6 +298,47 @@ describe('enforces never-ask preferences', () => { }); expectPassThrough(r); }); + + // #2024: unregistered ids used to default straight to two-way without ever + // consulting the keyword classifier — an ad-hoc DESTRUCTIVE question with a + // stored never-ask preference auto-decided. The hook now falls back to + // classifyQuestion on the question text when the registry lookup misses. + test('unregistered id + never-ask + destructive text → pass-through (keyword net fires, #2024)', () => { + writeProjectPref('adhoc-credential-cleanup', 'never-ask'); + const r = runHook({ + session_id: 's-kw-1', + tool_name: 'AskUserQuestion', + tool_use_id: 'tu-kw-1', + tool_input: { + questions: [ + { + question: ' Reset my secret and proceed?', + options: ['A) Yes (recommended)', 'B) No'], + }, + ], + }, + }); + expectPassThrough(r); + }); + + test('unregistered id + never-ask + benign text → still deny (auto-decide unchanged)', () => { + writeProjectPref('adhoc-credential-cleanup', 'never-ask'); + const r = runHook({ + session_id: 's-kw-2', + tool_name: 'AskUserQuestion', + tool_use_id: 'tu-kw-2', + tool_input: { + questions: [ + { + question: ' Reorganize the TODOs file?', + options: ['A) Yes (recommended)', 'B) No'], + }, + ], + }, + }); + expect(r.parsed?.hookSpecificOutput?.permissionDecision).toBe('deny'); + expect(r.parsed?.hookSpecificOutput?.permissionDecisionReason).toContain('plan-tune auto-decide'); + }); }); // ----------------------------------------------------------------------