From 4abb8847a47880ff944e283fbabf4c93f40744ae Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 9 Sep 2026 03:19:04 +0000 Subject: [PATCH] fix(hooks): memorable hook closes the review army's gaps - Vendor failures are logged even with empty stderr (a silently hanging vendor taxed every prompt invisibly); the stderr tail is withheld when the redaction engine finds a credential or PII shape in it; hook-errors.log is created 0600. - Trust-policy veto fails closed when git cannot run or answer in time (it read as 'no remote' before); the policy script spawn is bounded by the hook's clock; a payload cwd that is not a directory falls back. - Each secret scan is admitted by the deadline clock (the engine's cost grows with match density); stdin is decoded once. - The pre-spawn gate re-check logs a config failure instead of swallowing it; an incomplete stdin read is named as such, not as 'not JSON'. - Carriage returns are stripped with the other controls. - The vendor env allowlist adds the standard proxy, TLS and XDG variables so a vendor behind a corporate proxy or private CA still reaches its service. - A stdin EPIPE on a delivered answer is recorded in the outcome, not treated as a spawn error. - Stage caps and the truncation marker are named constants; a test-only GSTACK_MEMORABLE_TEST_BUDGET_MS can shorten (never widen) the budget. Co-Authored-By: Claude Fable 5.1 --- .../hooks/memorable-user-prompt-hook.ts | 145 ++++++++++++++---- 1 file changed, 114 insertions(+), 31 deletions(-) diff --git a/hosts/claude/hooks/memorable-user-prompt-hook.ts b/hosts/claude/hooks/memorable-user-prompt-hook.ts index 442b0b697..444865e13 100644 --- a/hosts/claude/hooks/memorable-user-prompt-hook.ts +++ b/hosts/claude/hooks/memorable-user-prompt-hook.ts @@ -10,8 +10,8 @@ * and clean removal. This file is that mediation. * * stdin JSON -> cap 1 MiB -> parse -> MEMORABLE=0? -> gate memorable_recall == on? - * -> win32? -> trust policy (deny / read-only veto, by session cwd) - * -> HIGH-tier secret scan (raw bytes AND decoded string leaves) + * -> win32? -> trust policy (deny / read-only veto, by session cwd; fail-closed) + * -> HIGH-tier secret scan (raw bytes AND decoded string leaves, each clock-bounded) * -> resolve vendor -> budget >= 500 ms? -> gate re-check * -> receipt (fail-closed: no receipt, no send) * -> VENDOR SPAWN (own process group, allowlisted env, group-killed on timeout) @@ -25,15 +25,19 @@ * vendor can never block a prompt or speak as gstack: only a string * `hookSpecificOutput.additionalContext` is accepted from its output. * - One deadline clock (BUDGET_MS) undercuts Claude Code's 5 s hook kill; - * every stage, the two ledger writes included, gets min(cap, remaining). - * A receipt with no outcome means the host killed us or the clock ran out - * (reported as `unknown`), never success. + * every stage, the two ledger writes and the secret scans included, gets + * min(cap, remaining). A receipt with no outcome means the host killed us + * or the clock ran out (reported as `unknown`), never success. * - Fail-closed on the receipt: if the ledger cannot be written, recall is * skipped for that prompt. What the receipt attests is the bytes handed to * a LOCAL binary running with the user's privileges (host `local:`); * what that binary sends is the vendor's claim. * - The vendor sees an allowlisted environment (PATH, HOME, locale, TMP, - * MEMORABLE*), never Claude Code's full env (which can carry API keys). + * the standard proxy/TLS/XDG variables, MEMORABLE*), never Claude Code's + * full env (which can carry API keys). + * - The vendor's stderr reaches hook-errors.log only when the redaction + * engine finds nothing in it (a CLI that echoes its input on a parse + * error would otherwise copy the prompt into the log). * - Windows is refused here (no process groups to contain the vendor); * bin/gstack-memorable enable refuses there too. TODOS.md D21. * @@ -51,21 +55,39 @@ import { hasRepoPolicyStore, repoPolicyTier } from '../../../lib/gbrain-repo-pol export const BUDGET_MS = 4500; export const STDIN_CAP_BYTES = 1024 * 1024; export const OUTPUT_CAP_BYTES = 8192; +/** Left on the clock for post-processing, the stdout write and one ledger append after the vendor. */ export const RESERVE_MS = 300; +/** Below this many ms left before the spawn, the vendor is not started at all. */ export const MIN_SPAWN_MS = 500; +/** Cap for each pre-spawn stage (stdin read, gate, policy, each secret scan); always min(cap, remaining). */ +export const STAGE_CAP_MS = 1000; +/** Cap for the pre-spawn gate re-check. */ +export const RECHECK_CAP_MS = 500; /** Below this many ms left, the outcome append is skipped (the receipt stands, outcome reads as unknown). */ export const OUTCOME_MIN_MS = 80; +/** Kept back from the clock when an outcome append is given the rest of it. */ +export const OUTCOME_RESERVE_MS = 50; export const LOG_RATE_LIMIT_MS = 10 * 60 * 1000; export const ENVELOPE_SOURCE = 'memorable recall (third-party)'; export const SINK = 'memorable-recall'; export const CONSENT = 'memorable_recall=on'; +export const RESOLUTION_ORDER = 'GSTACK_MEMORABLE_BIN, MEMORABLE_BIN, ~/.memorable/bin/memorable, PATH'; const HOOK_NAME = 'memorable-user-prompt-hook'; +const GIT_MAX_BUFFER = 64 * 1024; +const TRUNCATION_MARKER = `[truncated by gstack at ${OUTPUT_CAP_BYTES / 1024} KiB]`; /** Milliseconds left on a deadline that started at startMs. Pure; unit-tested. */ export function budgetFor(startMs: number, nowMs: number, cap: number = BUDGET_MS): number { return Math.max(0, startMs + cap - nowMs); } +/** The deadline: BUDGET_MS, or a test-only override that can only shorten it. */ +export function budgetMs(env: Record = process.env): number { + const raw = env.GSTACK_MEMORABLE_TEST_BUDGET_MS; + const n = raw ? Number(raw) : NaN; + return Number.isFinite(n) && n > 0 ? Math.min(n, BUDGET_MS) : BUDGET_MS; +} + /** Truncate to maxBytes of UTF-8 without splitting a multibyte character. */ export function capUtf8(text: string, maxBytes: number): { text: string; truncated: boolean } { const buf = Buffer.from(text, 'utf8'); @@ -75,10 +97,12 @@ export function capUtf8(text: string, maxBytes: number): { text: string; truncat return { text: buf.subarray(0, end).toString('utf8'), truncated: true }; } -// C0 controls minus tab (9) and newline (10), plus DEL. Built from char codes -// so the source file itself carries no control bytes. +// C0 controls minus tab (9) and newline (10), plus DEL. Carriage return (13) +// is stripped too: a CR can visually overwrite earlier text in a rendering of +// the injected context while staying one line for the envelope. Built from +// char codes so the source file itself carries no control bytes. const cc = (n: number): string => String.fromCharCode(n); -const CONTROL_RE = new RegExp(`[${cc(0)}-${cc(8)}${cc(11)}${cc(12)}${cc(14)}-${cc(31)}${cc(127)}]`, 'g'); +const CONTROL_RE = new RegExp(`[${cc(0)}-${cc(8)}${cc(11)}-${cc(31)}${cc(127)}]`, 'g'); /** Strip control characters except newline and tab (the envelope handles the rest). */ export function stripControl(text: string): string { @@ -99,7 +123,16 @@ export function stringLeaves(value: unknown, maxNodes = 10_000, maxDepth = 32): return out; } -const ENV_ALLOW = new Set(['PATH', 'HOME', 'USER', 'LOGNAME', 'SHELL', 'LANG', 'TERM', 'TMPDIR', 'TEMP', 'TMP']); +// Identity, locale and temp; the standard proxy, TLS and XDG knobs the vendor +// needs to reach its own service through the user's proxy or private CA (it +// already gets them from the user's shell); plus MEMORABLE* (its own knobs, +// matched by prefix below). Never API keys, never GSTACK_* or CLAUDE_*. +const ENV_ALLOW = new Set([ + 'PATH', 'HOME', 'USER', 'LOGNAME', 'SHELL', 'LANG', 'TERM', 'TMPDIR', 'TEMP', 'TMP', + 'HTTP_PROXY', 'HTTPS_PROXY', 'NO_PROXY', 'http_proxy', 'https_proxy', 'no_proxy', + 'SSL_CERT_FILE', 'SSL_CERT_DIR', 'NODE_EXTRA_CA_CERTS', + 'XDG_CONFIG_HOME', 'XDG_DATA_HOME', 'XDG_CACHE_HOME', 'XDG_STATE_HOME', +]); /** The vendor's environment: an allowlist, never Claude Code's full env. */ export function vendorEnv(env: Record): Record { @@ -123,10 +156,25 @@ export function pickAdditionalContext(raw: string): string | null { /** Cap + envelope: the text Claude will see. */ export function renderContext(vendorText: string): string { const { text, truncated } = capUtf8(stripControl(vendorText), OUTPUT_CAP_BYTES); - const body = truncated ? `${text}\n[truncated by gstack at 8 KiB]` : text; + const body = truncated ? `${text}\n${TRUNCATION_MARKER}` : text; return wrapUntrustedTrackerContent(body, ENVELOPE_SOURCE); } +/** + * The vendor's stderr tail as it may appear in hook-errors.log: control-stripped, + * whitespace-collapsed, last 300 chars, and WITHHELD when the redaction engine + * finds a HIGH or MEDIUM shape in it (a CLI that echoes its input on a parse + * error would otherwise copy prompt text into a log the pre-scan only cleared + * of HIGH-tier shapes). + */ +export function safeStderrTail(tail: string): string { + const t = stripControl(tail).replace(/\s+/g, ' ').trim().slice(-300); + if (!t) return ''; + const r = scan(t, { repoVisibility: 'unknown' }); + const n = r.counts.HIGH + r.counts.MEDIUM; + return r.oversize || n > 0 ? `[stderr withheld: ${n} redaction finding(s)]` : t; +} + function stripQuotes(v: string): string { return v.trim().replace(/^"(.*)"$/, '$1'); } @@ -142,6 +190,10 @@ function executable(p: string): boolean { } } +function isDirectory(p: string): boolean { + try { return fs.statSync(p).isDirectory(); } catch { return false; } +} + /** * GSTACK_MEMORABLE_BIN -> MEMORABLE_BIN -> ~/.memorable/bin/memorable -> PATH. * An explicit override that does not resolve is an error (null), never a @@ -168,7 +220,8 @@ function stateRoot(): string { /** * Best-effort, rate-limited: an identical message within LOG_RATE_LIMIT_MS is * not re-logged (a vendor removed after `enable` would otherwise append on - * every prompt). The marker is per hook so hooks never contend. + * every prompt). The marker is per hook so hooks never contend. The log is + * created 0600: it can name the session's cwd and the vendor's diagnostics. */ export function logHookError(msg: string, nowMs: number = Date.now()): void { try { @@ -180,8 +233,8 @@ export function logHookError(msg: string, nowMs: number = Date.now()): void { const [prevDigest, prevTs] = fs.readFileSync(marker, 'utf8').trim().split(':'); if (prevDigest === digest && nowMs - Number(prevTs) < LOG_RATE_LIMIT_MS) return; } catch { /* no marker yet */ } - fs.writeFileSync(marker, `${digest}:${nowMs}\n`); - fs.appendFileSync(path.join(root, 'hook-errors.log'), `${new Date(nowMs).toISOString()} ${HOOK_NAME}: ${msg}\n`); + fs.writeFileSync(marker, `${digest}:${nowMs}\n`, { mode: 0o600 }); + fs.appendFileSync(path.join(root, 'hook-errors.log'), `${new Date(nowMs).toISOString()} ${HOOK_NAME}: ${msg}\n`, { mode: 0o600 }); } catch { // best-effort; never block the session because logging failed } @@ -219,15 +272,23 @@ function gateIsOn(timeoutMs: number): 'on' | 'off' | 'error' { return String(r.stdout ?? '').trim() === 'on' ? 'on' : 'off'; } -async function policyVeto(cwd: string, timeoutMs: number): Promise<'ok' | 'skip' | 'error'> { +/** + * Trust-policy veto by the session's repo (keyed by its origin remote). + * Both halves fail CLOSED once a store exists: a git that could not run or + * answer in time, or a store that could not be read, is a failed read + * (`error`), never "no remote". Only a clean non-zero git exit (no remote, + * not a repo) means nothing can be set for this directory. + */ +async function policyVeto(cwd: string, timeoutMs: number, remaining: () => number): Promise<'ok' | 'skip' | 'error'> { if (!hasRepoPolicyStore()) return 'ok'; const git = await runExternal('git', ['remote', 'get-url', 'origin'], { - cwd, timeoutMs: Math.max(1, timeoutMs), maxBuffer: 64 * 1024, env: process.env, + cwd, timeoutMs: Math.max(1, timeoutMs), maxBuffer: GIT_MAX_BUFFER, env: process.env, }); - if (git.status !== 0) return 'ok'; // no remote: the policy (keyed by remote) has nothing set for this repo + if (git.timedOut || git.error) return 'error'; + if (git.status !== 0) return 'ok'; const url = git.stdout.toString('utf8').trim(); if (!url) return 'ok'; - const res = repoPolicyTier(url); + const res = repoPolicyTier(url, process.env, Math.max(1, Math.min(STAGE_CAP_MS, remaining()))); if (res.error) return 'error'; // `deny` and `read-only` are the tiers a user picks so a repo's content // never lands in a shared store; a third-party memory service is one. @@ -240,33 +301,45 @@ function writeStdout(text: string): Promise { export async function main(): Promise { const start = Date.now(); - const remaining = (): number => budgetFor(start, Date.now()); + const cap = budgetMs(); + const remaining = (): number => budgetFor(start, Date.now(), cap); - const stdin = await readStdin(STDIN_CAP_BYTES, Math.min(1000, remaining())); + const stdin = await readStdin(STDIN_CAP_BYTES, Math.min(STAGE_CAP_MS, remaining())); if (stdin.oversize) { logHookError('oversize: stdin exceeded 1 MiB, recall skipped'); return; } const raw = stdin.buf; if (raw.length === 0) return; + const rawText = raw.toString('utf8'); let payload: unknown; - try { payload = JSON.parse(raw.toString('utf8')); } catch { logHookError('stdin was not JSON, recall skipped'); return; } + try { payload = JSON.parse(rawText); } catch { + logHookError(stdin.timedOut + ? 'stdin was not closed within the read budget (incomplete JSON), recall skipped' + : 'stdin was not JSON, recall skipped'); + return; + } if (!payload || typeof payload !== 'object') return; if (process.env.MEMORABLE === '0') return; // the vendor's own kill switch - const gate = gateIsOn(Math.min(1000, remaining())); + const gate = gateIsOn(Math.min(STAGE_CAP_MS, remaining())); if (gate === 'error') { logHookError('gstack-config get memorable_recall failed, recall skipped (fail-closed)'); return; } if (gate !== 'on') return; if (process.platform === 'win32') { logHookError('Windows is not supported by this bridge yet (TODOS.md D21), recall skipped'); return; } const payloadCwd = (payload as { cwd?: unknown }).cwd; - const cwd = typeof payloadCwd === 'string' && fs.existsSync(payloadCwd) ? payloadCwd : process.cwd(); - const veto = await policyVeto(cwd, Math.min(1000, remaining())); + const cwd = typeof payloadCwd === 'string' && isDirectory(payloadCwd) ? payloadCwd : process.cwd(); + const veto = await policyVeto(cwd, Math.min(STAGE_CAP_MS, remaining()), remaining); if (veto === 'skip') { logHookError(`trust policy for ${cwd} is deny or read-only, recall skipped`); return; } if (veto === 'error') { logHookError('trust policy store unreadable, recall skipped (fail-closed)'); return; } - const rawText = raw.toString('utf8'); + // HIGH-tier pre-scan over the raw text and the decoded string leaves (a JSON + // escape must not hide a key). The engine's cost grows with match density + // (a pasted log is dense in MEDIUM/LOW shapes), so each scan is admitted by + // the clock: a scan we cannot afford skips recall instead of letting the + // host kill us mid-scan. const leaves = stringLeaves(payload).join('\n'); for (const text of [rawText, leaves]) { + if (remaining() < MIN_SPAWN_MS) { logHookError('budget-exhausted before the secret scan, recall skipped'); return; } const result = scan(text, { repoVisibility: 'unknown' }); if (result.oversize || result.counts.HIGH > 0) { logHookError('refused:redaction-high: the prompt carries a HIGH-tier credential shape, nothing handed to the vendor'); @@ -275,10 +348,12 @@ export async function main(): Promise { } const vendor = resolveVendor(process.env, os.homedir()); - if (!vendor) { logHookError('memorable CLI not found (checked GSTACK_MEMORABLE_BIN, MEMORABLE_BIN, ~/.memorable/bin/memorable, PATH), recall skipped'); return; } + if (!vendor) { logHookError(`memorable CLI not found (checked ${RESOLUTION_ORDER}), recall skipped`); return; } if (remaining() < MIN_SPAWN_MS) { logHookError('budget-exhausted before the vendor spawn, recall skipped'); return; } - if (gateIsOn(Math.min(500, remaining())) !== 'on') return; // a disable that landed while we worked wins + const again = gateIsOn(Math.min(RECHECK_CAP_MS, remaining())); + if (again === 'error') { logHookError('gstack-config get memorable_recall failed on the pre-spawn re-check, recall skipped (fail-closed)'); return; } + if (again !== 'on') return; // a disable that landed while we worked wins let receiptId: string; try { @@ -301,7 +376,7 @@ export async function main(): Promise { } if (remaining() < MIN_SPAWN_MS) { logHookError('budget-exhausted after the receipt, recall skipped'); - try { writeOutcome({ receipt: receiptId, status: 'budget-exhausted', lockBudgetMs: Math.max(0, remaining() - 100) }); } catch { /* bookkeeping */ } + try { writeOutcome({ receipt: receiptId, status: 'budget-exhausted', lockBudgetMs: Math.max(0, remaining() - OUTCOME_RESERVE_MS) }); } catch { /* bookkeeping */ } return; } @@ -316,6 +391,7 @@ export async function main(): Promise { }); let status: string; + let delivered = false; if (r.timedOut) status = 'timeout'; else if (r.error) status = `spawn-error:${r.error}`; else if (r.status !== 0) status = `exit:${r.status} injected=no`; @@ -326,16 +402,23 @@ export async function main(): Promise { const rendered = renderContext(ctx); const out = JSON.stringify({ hookSpecificOutput: { hookEventName: 'UserPromptSubmit', additionalContext: rendered } }); await writeStdout(out); + delivered = true; status = `exit:0 output-written bytes=${Buffer.byteLength(rendered, 'utf8')} gstack_ms=${gstackMs}`; } } - if (r.stderrTail && (r.timedOut || r.error || r.status !== 0)) { - logHookError(`vendor ${status}: ${r.stderrTail.replace(/\s+/g, ' ').slice(-300)}`); + // A vendor that exited before reading its stdin (EPIPE) is advisory when it + // still answered; the outcome records it, the answer is kept. + if (r.stdinError) status += ` stdin=${r.stdinError}`; + if (!delivered && (r.timedOut || r.error || r.status !== 0)) { + // Logged whether or not the vendor said anything: a silently hanging + // vendor taxes every prompt and must show up in `gstack-memorable status`. + const tail = safeStderrTail(r.stderrTail); + logHookError(`vendor ${status}${tail ? `: ${tail}` : ''}`); } // The vendor's timeout already left RESERVE_MS on the clock for exactly // this: the stdout write above and one bounded ledger append. if (remaining() > OUTCOME_MIN_MS) { - try { writeOutcome({ receipt: receiptId, status, lockBudgetMs: Math.max(0, remaining() - 50) }); } catch { /* the receipt is the invariant; the outcome is bookkeeping */ } + try { writeOutcome({ receipt: receiptId, status, lockBudgetMs: Math.max(0, remaining() - OUTCOME_RESERVE_MS) }); } catch { /* the receipt is the invariant; the outcome is bookkeeping */ } } }