CalvinBackup/gstack
1
0
Fork 0
mirror of https://github.com/garrytan/gstack.git synced 2026-09-09 06:28:59 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(hooks): outside-model review fixes for the memorable hook and runExternal

Browse Source 
- Nothing in the vendor's process group outlives the call: the group is
  killed on every resolve, the clean 'close' path included (a helper the
  vendor forked with redirected stdio ran on unsupervised before).
- A child that already exited when the deadline fires keeps its result; the
  deadline only ends the drain instead of rewriting a completed exit as a
  timeout.
- The decoded-leaf scan reports when its node/depth bound cut the walk short
  and the hook refuses the hand-off as unscanned; object keys are scanned too.
- git for the trust-policy lookup runs without inherited GIT_* selectors
  (GIT_DIR, GIT_WORK_TREE, GIT_CONFIG_*), so it inspects the session's
  repository and no other; a host cancellation kills that git as well.
- An unmatched brace in a vendor banner no longer hides the answer after it.
- The stderr tail is scanned whole before it is cropped for the log, so a
  credential's prefix cannot be cropped away from its secret half.
- The vendor override reads an empty GSTACK_MEMORABLE_BIN as unset, exactly
  as bin/gstack-memorable does, so enable checks the binary the hook runs.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
2026-09-09 03:57:16 +00:00
co-authored by Claude Fable 5.1
parent 1b9d0d11f0
commit 73245d8e1d
2 changed files with 79 additions and 21 deletions
Download Patch File Download Diff File Expand all files Collapse all files
hosts/claude/hooks/memorable-user-prompt-hook.ts
+64 -20
View File
@@ -129,18 +129,32 @@ export function stripControl(text: string): string {
return text.replace(CONTROL_RE, '').replace(FORMAT_RE, '');
}
/** Every string leaf of a parsed JSON value, bounded so a hostile payload cannot monopolize the clock. */
export function stringLeaves(value: unknown, maxNodes = 10_000, maxDepth = 32): string[] {
const out: string[] = [];
/**
* Every string leaf of a parsed JSON value, bounded so a hostile payload
* cannot monopolize the clock. `exhausted` is true when the bound cut the
* walk short: the caller must then treat the payload as unscanned (and refuse
* the hand-off), never as clean.
*/
export function stringLeavesBounded(value: unknown, maxNodes = 10_000, maxDepth = 32): { leaves: string[]; exhausted: boolean } {
const leaves: string[] = [];
let nodes = 0;
let exhausted = false;
const walk = (v: unknown, depth: number): void => {
if (nodes++ > maxNodes || depth > maxDepth) return;
if (typeof v === 'string') { out.push(v); return; }
if (nodes++ > maxNodes || depth > maxDepth) { exhausted = true; return; }
if (typeof v === 'string') { leaves.push(v); return; }
if (Array.isArray(v)) { for (const item of v) walk(item, depth + 1); return; }
if (v && typeof v === 'object') { for (const item of Object.values(v as Record<string, unknown>)) walk(item, depth + 1); }
if (v && typeof v === 'object') {
// keys are forwarded bytes too; a credential can sit in one
for (const [k, item] of Object.entries(v as Record<string, unknown>)) { leaves.push(k); walk(item, depth + 1); }
}
};
walk(value, 0);
return out;
return { leaves, exhausted };
}
/** The leaves alone (see stringLeavesBounded). */
export function stringLeaves(value: unknown, maxNodes = 10_000, maxDepth = 32): string[] {
return stringLeavesBounded(value, maxNodes, maxDepth).leaves;
}
// Identity, locale and temp; the standard proxy, TLS and XDG knobs the vendor
@@ -196,10 +210,11 @@ export function* jsonObjects(raw: string): Generator<unknown> {
let start = raw.indexOf('{');
for (let tries = 0; start >= 0 && tries < JSON_CANDIDATES; tries++) {
const end = balancedObjectEnd(raw, start);
if (end < 0) return; // nothing complete remains
let parsed: unknown;
let ok = false;
try { parsed = JSON.parse(raw.slice(start, end + 1)); ok = true; } catch { /* a brace in prose */ }
// An unbalanced candidate (a lone brace in a banner) is skipped like an
// unparsable one: the complete object after it must still be found.
if (end >= 0) { try { parsed = JSON.parse(raw.slice(start, end + 1)); ok = true; } catch { /* a brace in prose */ } }
if (ok) { yield parsed; start = raw.indexOf('{', end + 1); }
else start = raw.indexOf('{', start + 1);
}
@@ -236,11 +251,13 @@ export function renderContext(vendorText: string): string {
* 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' });
// Scan everything runExternal kept, THEN crop for the log: cropping first
// could cut a credential's identifying prefix off and log its secret half.
const whole = stripControl(tail).replace(/\s+/g, ' ').trim();
if (!whole) return '';
const r = scan(whole, { repoVisibility: 'unknown' });
const n = r.counts.HIGH + r.counts.MEDIUM;
return r.oversize || n > 0 ? `[stderr withheld: ${n} redaction finding(s)]` : t;
return r.oversize || n > 0 ? `[stderr withheld: ${n} redaction finding(s)]` : whole.slice(-300);
}
function stripQuotes(v: string): string {
@@ -268,8 +285,10 @@ function isDirectory(p: string): boolean {
* fall-through to something else (lib/claude-bin.ts contract).
*/
export function resolveVendor(env: Record<string, string | undefined>, homeDir: string): string | null {
const override = env.GSTACK_MEMORABLE_BIN ?? env.MEMORABLE_BIN;
if (override && override.trim()) {
// Empty means unset, exactly as bash's ${GSTACK_MEMORABLE_BIN:-${MEMORABLE_BIN:-}} reads it
// in bin/gstack-memorable: the binary enable checked is the binary the hook runs.
const override = (env.GSTACK_MEMORABLE_BIN ?? '').trim() || (env.MEMORABLE_BIN ?? '').trim();
if (override) {
const o = stripQuotes(override);
const resolved = path.isAbsolute(o) ? o : (Bun.which(o) ?? null);
return resolved && executable(resolved) ? resolved : null;
@@ -361,7 +380,26 @@ function gateIsOn(timeoutMs: number): 'on' | 'off' | 'error' {
const GIT_NO_REMOTE = 2;
const GIT_FATAL = 128;
const NOT_A_REPO_RE = /^fatal: not a git repository\b/;
const GIT_ENV = { ...process.env, LC_ALL: 'C', LANGUAGE: '', LC_MESSAGES: 'C' };
/** Kills the in-flight detached child (git for the policy lookup, then the vendor); read by the signal handlers. */
let killInflight: (() => void) | null = null;
/**
* git's environment for the policy lookup: English messages, and NO inherited
* GIT_* repository selectors (GIT_DIR, GIT_WORK_TREE, GIT_COMMON_DIR,
* GIT_CONFIG_*, ...): cwd does not override them, so an inherited GIT_DIR
* would make the veto inspect a different repository than the one the
* session works in. Exported for tests.
*/
export function gitEnv(env: Record<string, string | undefined>): Record<string, string> {
const out: Record<string, string> = {};
for (const [k, v] of Object.entries(env)) {
if (v == null || k.startsWith('GIT_')) continue;
out[k] = v;
}
out.LC_ALL = 'C'; out.LANGUAGE = ''; out.LC_MESSAGES = 'C';
return out;
}
/**
* Trust-policy veto by the session's repo (keyed by its origin remote).
@@ -375,8 +413,10 @@ const GIT_ENV = { ...process.env, LC_ALL: 'C', LANGUAGE: '', LC_MESSAGES: 'C' };
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: GIT_MAX_BUFFER, env: GIT_ENV,
cwd, timeoutMs: Math.max(1, timeoutMs), maxBuffer: GIT_MAX_BUFFER, env: gitEnv(process.env),
onSpawn: (kill) => { killInflight = kill; }, // a host cancellation must not leak this git either
});
killInflight = null;
if (git.timedOut || git.error) return 'error';
if (git.status === GIT_NO_REMOTE) return 'ok';
if (git.status === GIT_FATAL && NOT_A_REPO_RE.test(git.stderrTail.trim())) return 'ok';
@@ -394,8 +434,6 @@ function writeStdout(text: string): Promise<void> {
return new Promise((resolve) => { process.stdout.write(text, () => resolve()); });
}
/** Kills the in-flight vendor group; set for the duration of the vendor spawn, read by the signal handlers. */
let killInflight: (() => void) | null = null;
/** Best-effort: the ledger's size warning goes to stderr, which the host discards for an exit-0 hook; log it where `status` looks. */
function noteLedgerSize(): void {
@@ -444,7 +482,13 @@ export async function main(): Promise<void> {
// 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');
const walked = stringLeavesBounded(payload);
if (walked.exhausted) {
// A payload too deep or too wide to walk is unscanned, not clean.
logHookError('refused:payload-too-complex: the prompt JSON exceeded the scan walk bounds, nothing handed to the vendor');
return;
}
const leaves = walked.leaves.join('\n');
for (const text of [rawText, leaves]) {
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; }
hosts/claude/hooks/spawn-bin.ts
+15 -1
View File
@@ -96,6 +96,14 @@ export interface RunExternalResult {
* only on 'close': a child that exits 0 but leaves a background process
* holding its pipes gets its output delivered and the straggler group-
* killed, instead of being reported as a timeout with its answer dropped.
* - NOTHING in the group outlives the call: the group is killed on every
* resolve, including a clean 'close' (a helper the child forked with its
* stdio redirected would otherwise run on unsupervised). A child that
* must leave a daemon behind has to setsid it; that is the child's
* explicit choice, visible in its own code, not an accident of ours.
* - a child that has already exited when the deadline fires keeps its
* result: the deadline then ends the drain, it does not rewrite a
* completed exit as a timeout.
* - stderr is drained continuously (an undrained pipe blocks a noisy child
* before it writes stdout) and only its tail is kept, never forwarded.
* - stdin gets an error listener, so a child that exits before reading a
@@ -132,6 +140,7 @@ export function runExternal(exe: string, args: string[], opts: RunExternalOption
let stdinError: string | undefined;
let timedOut = false;
let done = false;
let exited: { code: number | null; signal: NodeJS.Signals | null } | null = null;
let graceTimer: ReturnType<typeof setTimeout> | undefined;
let drainTimer: ReturnType<typeof setTimeout> | undefined;
const killGroup = (): void => {
@@ -144,12 +153,16 @@ export function runExternal(exe: string, args: string[], opts: RunExternalOption
clearTimeout(timer);
if (graceTimer) clearTimeout(graceTimer);
if (drainTimer) clearTimeout(drainTimer);
// A straggler holding our pipes must not pin this process either.
// Nothing in the group outlives the call; a straggler holding our pipes
// must not pin this process either.
killGroup();
for (const s of [child.stdout, child.stderr, child.stdin]) { try { s?.destroy(); } catch { /* closed */ } }
try { child.unref(); } catch { /* fine */ }
resolve({ status, signal, stdout: Buffer.concat(chunks), stderrTail, ...(stdinError ? { stdinError } : {}), error, timedOut });
};
const timer = setTimeout(() => {
// The child already answered and exited; the deadline only ends the drain.
if (exited) { finish(exited.code, exited.signal); return; }
timedOut = true;
error = error ?? 'ETIMEDOUT';
killGroup();
@@ -167,6 +180,7 @@ export function runExternal(exe: string, args: string[], opts: RunExternalOption
});
child.stderr?.on('data', (d: Buffer) => { stderrTail = (stderrTail + d.toString('utf8')).slice(-STDERR_TAIL_BYTES); });
child.on('exit', (code, signal) => {
exited = { code, signal };
if (done) return;
// A killed child (timeout, ENOBUFS) has nothing worth draining: resolve
// now so the caller keeps its post-spawn reserve.