From af72e3d4ffcb237c897bc1701014fb82c164ecec Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 9 Sep 2026 03:33:11 +0000 Subject: [PATCH] fix(hooks): memorable hook second-pass review fixes - Trust-policy lookup fails closed on any git failure that is not 'no such remote' or 'not a git repository' (a corrupt or unreadable .git/config and dubious ownership exit 128 and used to read as 'no remote'). - pickAdditionalContext takes the first complete top-level JSON object, so a vendor whose background helper appends a line to stdout (or prints a banner first) does not lose its answer. - The hook-errors.log rate limiter keys on a stable string (a vendor's timestamped stderr no longer defeats it); the log is chmod 0600 on every append because sibling hooks create the same file without a mode. - Scan admission is sized by payload bytes (scan() is uninterruptible). - The receipt payload class is a stable token; the prose moved to the docs. - Header, constants and comments match the behaviour (silent skips vs logged refusals; HIGH/MEDIUM withholding; STAGE_CAP_MS scope; runExternal in the spawn-bin header; the ledger lock budget doc). Co-Authored-By: Claude Fable 5.1 --- .../hooks/memorable-user-prompt-hook.ts | 111 +++++++++++++----- hosts/claude/hooks/spawn-bin.ts | 4 + lib/egress-receipt.ts | 6 +- 3 files changed, 89 insertions(+), 32 deletions(-) diff --git a/hosts/claude/hooks/memorable-user-prompt-hook.ts b/hosts/claude/hooks/memorable-user-prompt-hook.ts index 444865e13..4ef469d4e 100644 --- a/hosts/claude/hooks/memorable-user-prompt-hook.ts +++ b/hosts/claude/hooks/memorable-user-prompt-hook.ts @@ -11,14 +11,15 @@ * * stdin JSON -> cap 1 MiB -> parse -> MEMORABLE=0? -> gate memorable_recall == on? * -> 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) + * -> HIGH-tier secret scan (raw bytes AND decoded string leaves, each admitted by the clock) * -> 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) * -> parse vendor JSON -> additionalContext only -> control-strip * -> 8 KiB cap (UTF-8 boundary) -> trust envelope -> stdout (awaited) * -> outcome (bounded by the same clock) -> exit 0 - * every early exit above is: one rate-limited line in hook-errors.log, empty stdout, exit 0. + * every REFUSAL above is: one rate-limited line in hook-errors.log, empty stdout, exit 0 + * (gate off, MEMORABLE=0, empty or non-object stdin are silent: nothing was refused). * * CONTRACT * - ALWAYS exits 0 with either one hookSpecificOutput JSON or nothing. The @@ -36,10 +37,11 @@ * 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). + * engine finds no HIGH- or MEDIUM-tier shape in it (a CLI that echoes its + * input on a parse error would otherwise copy the prompt into the log). + * The log is chmod 0600 on every append (sibling hooks share the file). * - Windows is refused here (no process groups to contain the vendor); - * bin/gstack-memorable enable refuses there too. TODOS.md D21. + * bin/gstack-memorable enable refuses there too. TODOS.md: Windows support (D21). * * Pure helpers are exported for unit tests; main() runs only under import.meta.main. */ @@ -59,7 +61,7 @@ export const OUTPUT_CAP_BYTES = 8192; 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). */ +/** Cap for each pre-spawn subprocess stage (stdin read, gate, git, policy); always min(cap, remaining). */ export const STAGE_CAP_MS = 1000; /** Cap for the pre-spawn gate re-check. */ export const RECHECK_CAP_MS = 500; @@ -72,7 +74,11 @@ 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'; +/** Receipt payload class: a stable token (the prose lives in docs/memorable-workflow-memory.md), so a per-prompt sink does not repeat a sentence per line. */ +export const PAYLOAD_CLASS = 'claude-user-prompt-json->local-vendor-cli'; const HOOK_NAME = 'memorable-user-prompt-hook'; +/** Per-KiB allowance added to the scan admission check: ~1.5x the measured worst case of scan(). */ +const SCAN_MS_PER_KIB = 1; const GIT_MAX_BUFFER = 64 * 1024; const TRUNCATION_MARKER = `[truncated by gstack at ${OUTPUT_CAP_BYTES / 1024} KiB]`; @@ -144,10 +150,41 @@ export function vendorEnv(env: Record): Record 0 ? ctx : null; @@ -218,23 +255,27 @@ 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. The log is - * created 0600: it can name the session's cwd and the vendor's diagnostics. + * Best-effort, rate-limited: a message with the same `key` (default: the + * message itself) within LOG_RATE_LIMIT_MS is not re-logged, so a vendor that + * fails on every prompt with a different timestamp in its stderr still costs + * one line per ten minutes. The marker is per hook so hooks never contend. + * The log is chmod 0600 on every append: sibling hooks create the same file + * without a mode, and it can name the session's cwd and vendor diagnostics. */ -export function logHookError(msg: string, nowMs: number = Date.now()): void { +export function logHookError(msg: string, nowMs: number = Date.now(), key: string = msg): void { try { const root = stateRoot(); fs.mkdirSync(root, { recursive: true }); const marker = path.join(root, `hook-errors.${HOOK_NAME}.last`); - const digest = sha256Hex(msg).slice(0, 16); + const digest = sha256Hex(key).slice(0, 16); try { 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`, { mode: 0o600 }); - fs.appendFileSync(path.join(root, 'hook-errors.log'), `${new Date(nowMs).toISOString()} ${HOOK_NAME}: ${msg}\n`, { mode: 0o600 }); + const log = path.join(root, 'hook-errors.log'); + fs.appendFileSync(log, `${new Date(nowMs).toISOString()} ${HOOK_NAME}: ${msg}\n`, { mode: 0o600 }); + if (process.platform !== 'win32') { try { fs.chmodSync(log, 0o600); } catch { /* not ours to tighten */ } } } catch { // best-effort; never block the session because logging failed } @@ -272,12 +313,19 @@ function gateIsOn(timeoutMs: number): 'on' | 'off' | 'error' { return String(r.stdout ?? '').trim() === 'on' ? 'on' : 'off'; } +/** git exit 2 = no such remote; exit 128 + this text = not inside a repository. Anything else non-zero is a failed read. */ +const GIT_NO_REMOTE = 2; +const GIT_FATAL = 128; +const NOT_A_REPO_RE = /not a git repository/i; + /** * 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. + * Every half fails CLOSED once a store exists: a git that could not run or + * answer in time, a git that could not read the repository (corrupt or + * unreadable config, dubious ownership: exit 128 without "not a git + * repository"), or a store that could not be read is a failed lookup + * (`error`), never "no remote". Only "no such remote" and "not a repository" + * mean nothing can be set for this directory. */ async function policyVeto(cwd: string, timeoutMs: number, remaining: () => number): Promise<'ok' | 'skip' | 'error'> { if (!hasRepoPolicyStore()) return 'ok'; @@ -285,7 +333,9 @@ async function policyVeto(cwd: string, timeoutMs: number, remaining: () => numbe cwd, timeoutMs: Math.max(1, timeoutMs), maxBuffer: GIT_MAX_BUFFER, env: process.env, }); if (git.timedOut || git.error) return 'error'; - if (git.status !== 0) return 'ok'; + if (git.status === GIT_NO_REMOTE) return 'ok'; + if (git.status === GIT_FATAL && NOT_A_REPO_RE.test(git.stderrTail)) return 'ok'; + if (git.status !== 0) return 'error'; const url = git.stdout.toString('utf8').trim(); if (!url) return 'ok'; const res = repoPolicyTier(url, process.env, Math.max(1, Math.min(STAGE_CAP_MS, remaining()))); @@ -305,7 +355,7 @@ export async function main(): Promise { const remaining = (): number => budgetFor(start, Date.now(), cap); 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; } + if (stdin.oversize) { logHookError(`oversize: stdin exceeded ${STDIN_CAP_BYTES / (1024 * 1024)} MiB, recall skipped`); return; } const raw = stdin.buf; if (raw.length === 0) return; const rawText = raw.toString('utf8'); @@ -329,17 +379,18 @@ export async function main(): Promise { const payloadCwd = (payload as { cwd?: unknown }).cwd; 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; } + if (veto === 'skip') { logHookError(`trust policy for ${cwd} is deny or read-only, recall skipped`, Date.now(), 'trust policy skip'); return; } + if (veto === 'error') { logHookError('trust policy lookup failed (store or repository unreadable), recall skipped (fail-closed)'); return; } // 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. + // escape must not hide a key). scan() is synchronous and uninterruptible and + // its cost is roughly linear in bytes, so each scan is admitted by the clock + // with a size-derived allowance: 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 allowance = MIN_SPAWN_MS + Math.ceil(Buffer.byteLength(text, 'utf8') / 1024) * SCAN_MS_PER_KIB; + if (remaining() < allowance) { 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'); @@ -360,7 +411,7 @@ export async function main(): Promise { const { id } = writeReceipt({ sink: SINK, host: `local:${vendor}`, - payloadClass: 'claude-user-prompt-json handed to the local vendor CLI; network destination unknown to gstack (vendor states: memorable.sh embed API on a local recall miss)', + payloadClass: PAYLOAD_CLASS, bytes: raw.length, sha256: sha256Hex(raw), consent: CONSENT, @@ -413,7 +464,7 @@ export async function main(): Promise { // 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}` : ''}`); + logHookError(`vendor ${status}${tail ? `: ${tail}` : ''}`, Date.now(), `vendor ${status}`); } // The vendor's timeout already left RESERVE_MS on the clock for exactly // this: the stdout write above and one bounded ledger append. diff --git a/hosts/claude/hooks/spawn-bin.ts b/hosts/claude/hooks/spawn-bin.ts index d250fb030..c2835cc11 100644 --- a/hosts/claude/hooks/spawn-bin.ts +++ b/hosts/claude/hooks/spawn-bin.ts @@ -8,6 +8,10 @@ * is the correct conversion. (ENOENT before the bin ever ran.) * 2. `bin/gstack-*` are extensionless bash scripts. Windows has no shebang * support, so they must be handed to bash explicitly. + * + * Also home to runExternal: the contained runner for EXTERNAL executables + * (third-party binaries a hook hands data to; see its doc comment). Unlike + * runBin it refuses win32, because its guarantee is process-group containment. */ import * as fs from 'fs'; import * as path from 'path'; diff --git a/lib/egress-receipt.ts b/lib/egress-receipt.ts index 3cbb071dc..01882ebcf 100644 --- a/lib/egress-receipt.ts +++ b/lib/egress-receipt.ts @@ -151,8 +151,10 @@ function requireString(value: unknown, name: string): string { } /** - * mkdir spin lock, ~2.5s budget. Egress events are rare (minutes apart); the - * lock only protects the read-last-line → append window. + * mkdir spin lock; the budget defaults to LEDGER_LOCK_BUDGET_MS (2.5 s) and + * callers on their own deadline pass less. Egress events are usually rare + * (minutes apart; the memorable hook is the per-prompt exception); the lock + * only protects the read-last-line → append window. * * Stale-lock reclaim: a crashed writer strands the lock dir. Once the spin * budget is exhausted, a lock dir whose mtime is >10s old is stale by