From 2396ecf1db79954e29a06c31506a63ae45561854 Mon Sep 17 00:00:00 2001 From: Jeremy Date: Sat, 4 Apr 2026 14:18:13 -0500 Subject: [PATCH] =?UTF-8?q?fix(gsd):=20harden=20audit=20log=20persistence?= =?UTF-8?q?=20=E2=80=94=20errors-only,=20sanitized,=20demote=20probe=20war?= =?UTF-8?q?nings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Only persist error-severity entries to audit-log.jsonl (warnings stay ephemeral in stderr + buffer). Sanitize persisted entries with message truncation and context field allowlisting. Demote expected main/master branch probe failures to silent control flow. Remove JSON.stringify of diagnostic objects embedding cwd/paths in warning messages. Addresses Codex adversarial review findings on workflow-logger migration. --- src/resources/extensions/gsd/auto-recovery.ts | 14 +++---- .../extensions/gsd/bootstrap/dynamic-tools.ts | 10 +---- .../extensions/gsd/workflow-logger.ts | 42 +++++++++++++++++-- 3 files changed, 47 insertions(+), 19 deletions(-) diff --git a/src/resources/extensions/gsd/auto-recovery.ts b/src/resources/extensions/gsd/auto-recovery.ts index 15f3e32c4..5720d1b1f 100644 --- a/src/resources/extensions/gsd/auto-recovery.ts +++ b/src/resources/extensions/gsd/auto-recovery.ts @@ -112,9 +112,8 @@ function detectMainBranch(basePath: string): string { encoding: "utf-8", }); if (result.trim()) return "main"; - } catch (err) { - // main doesn't exist - logWarning("recovery", `main branch not found: ${err instanceof Error ? err.message : String(err)}`); + } catch { + // Expected — main doesn't exist, try master next } try { const result = execFileSync("git", ["rev-parse", "--verify", "master"], { @@ -123,11 +122,12 @@ function detectMainBranch(basePath: string): string { encoding: "utf-8", }); if (result.trim()) return "master"; - } catch (err) { - // master doesn't exist either - logWarning("recovery", `master branch not found: ${err instanceof Error ? err.message : String(err)}`); + } catch { + // Expected — master doesn't exist either } - return "main"; // default fallback + // Neither main nor master found — warn and fall back + logWarning("recovery", "neither main nor master branch found, defaulting to main"); + return "main"; } /** diff --git a/src/resources/extensions/gsd/bootstrap/dynamic-tools.ts b/src/resources/extensions/gsd/bootstrap/dynamic-tools.ts index ec7cfba47..1cfe2c82b 100644 --- a/src/resources/extensions/gsd/bootstrap/dynamic-tools.ts +++ b/src/resources/extensions/gsd/bootstrap/dynamic-tools.ts @@ -104,16 +104,10 @@ export async function ensureDbOpen(): Promise { return opened; } - logWarning("bootstrap", `ensureDbOpen failed — no .gsd directory found (resolvedPath=${resolveProjectRootDbPath(basePath)}, cwd=${basePath})`); + logWarning("bootstrap", "ensureDbOpen failed — no .gsd directory found"); return false; } catch (err) { - const basePath = process.cwd(); - const diagnostic = { - resolvedPath: resolveProjectRootDbPath(basePath), - cwd: basePath, - error: (err as Error).message ?? String(err), - }; - logWarning("bootstrap", `ensureDbOpen failed — ${JSON.stringify(diagnostic)}`); + logWarning("bootstrap", `ensureDbOpen failed: ${(err as Error).message ?? String(err)}`); return false; } } diff --git a/src/resources/extensions/gsd/workflow-logger.ts b/src/resources/extensions/gsd/workflow-logger.ts index fa61b403d..f31ef0bfa 100644 --- a/src/resources/extensions/gsd/workflow-logger.ts +++ b/src/resources/extensions/gsd/workflow-logger.ts @@ -2,7 +2,9 @@ // Centralized warning/error accumulator for the workflow engine pipeline. // Captures structured entries that the auto-loop can drain after each unit // to surface root causes for stuck loops, silent degradation, and blocked writes. -// All entries are also persisted to .gsd/audit-log.jsonl for post-mortem analysis. +// Error-severity entries are persisted to .gsd/audit-log.jsonl (sanitized) for +// post-mortem analysis. Warnings are ephemeral (stderr + buffer only) to avoid +// log amplification from expected-control-flow catch paths. // // Stderr policy: every logWarning/logError call writes immediately to stderr // for terminal visibility. This is intentional — unlike debug-logger (which is @@ -243,15 +245,47 @@ function _push( _buffer.shift(); } - // Persist to .gsd/audit-log.jsonl so entries survive context resets - if (_auditBasePath) { + // Persist errors to .gsd/audit-log.jsonl so they survive context resets. + // Only error-severity entries are persisted — warnings are ephemeral (stderr + buffer) + // to avoid log amplification from expected-control-flow catch paths. + if (_auditBasePath && severity === "error") { try { const auditDir = join(_auditBasePath, ".gsd"); mkdirSync(auditDir, { recursive: true }); - appendFileSync(join(auditDir, "audit-log.jsonl"), JSON.stringify(entry) + "\n", "utf-8"); + const sanitized = _sanitizeForAudit(entry); + appendFileSync(join(auditDir, "audit-log.jsonl"), JSON.stringify(sanitized) + "\n", "utf-8"); } catch (auditErr) { // Best-effort — never let audit write failures bubble up process.stderr.write(`[gsd:audit] failed to persist log entry: ${(auditErr as Error).message}\n`); } } } + +/** + * Sanitize a log entry before persisting to the audit JSONL file. + * Strips potentially sensitive context (raw paths, cwd, full error text) + * to avoid leaking local environment details into durable telemetry. + */ +function _sanitizeForAudit(entry: LogEntry): LogEntry { + const sanitized: LogEntry = { + ts: entry.ts, + severity: entry.severity, + component: entry.component, + // Truncate message to avoid persisting oversized raw error dumps + message: entry.message.length > 200 ? entry.message.slice(0, 200) + "…[truncated]" : entry.message, + }; + if (entry.context) { + // Allowlist: only persist known-safe structured keys + const SAFE_KEYS = new Set(["fn", "tool", "mid", "sid", "tid", "worktree"]); + const filtered: Record = {}; + for (const [k, v] of Object.entries(entry.context)) { + if (SAFE_KEYS.has(k)) { + filtered[k] = v; + } + } + if (Object.keys(filtered).length > 0) { + sanitized.context = filtered; + } + } + return sanitized; +}