mirror of https://github.com/garrytan/gstack.git
fix(one-way-doors): unify credential noun net + wire it into the runtime (#2024)
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 <noreply@anthropic.com>
This commit is contained in:
parent
3d97863b14
commit
e0662ea7b5
|
|
@ -5,7 +5,9 @@
|
|||
# Schema: { "<question_id>": "always-ask" | "never-ask" | "ask-only-for-one-way" }
|
||||
#
|
||||
# Subcommands:
|
||||
# --check <id> → emit ASK_NORMALLY | AUTO_DECIDE | ASK_ONLY_ONE_WAY
|
||||
# --check <id> [--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 [<id>] → clear one or all preferences
|
||||
|
|
@ -44,22 +46,39 @@ ensure_file() {
|
|||
# --check <question_id>
|
||||
# -----------------------------------------------------------------------
|
||||
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');
|
||||
|
|
|
|||
|
|
@ -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<void> {
|
|||
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; }
|
||||
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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 "<id>"\`. \`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' "<question summary>" | ${bin}/gstack-question-preference --check "<id>" --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 \`<gstack-qid:{question_id}>\` 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 "<id>"\`.
|
||||
Before each AskUserQuestion, run: \`printf '%s' "<question summary>" | ${bin}/gstack-question-preference --check "<id>" --summary-stdin\`.
|
||||
\`AUTO_DECIDE\` → auto-choose recommended with inline annotation. \`ASK_NORMALLY\` → ask.`;
|
||||
}
|
||||
|
||||
|
|
|
|||
|
|
@ -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 <skill>-split-<option-slug>
|
||||
// must always ASK_NORMALLY regardless of stored preferences.
|
||||
// See scripts/resolvers/preamble/generate-ask-user-format.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);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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: '<gstack-qid:adhoc-credential-cleanup> 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: '<gstack-qid:adhoc-credential-cleanup> 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');
|
||||
});
|
||||
});
|
||||
|
||||
// ----------------------------------------------------------------------
|
||||
|
|
|
|||
Loading…
Reference in New Issue