From 3b4f955181097a9f77c3ef21bb7c41217e38d778 Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Wed, 9 Sep 2026 03:33:11 +0000 Subject: [PATCH] test: second-pass coverage for the memorable bridge Policy lookup outcomes (plain dir, repo without policy, corrupt .git/config fails closed); repoPolicyTier timeoutMs; line/col at line starts, after blank lines, CRLF and first char; tolerant first-JSON-object parsing; keyed rate limit; uninstall never creates a config just to say off and flips consent in a GSTACK_STATE_ROOT outside the removed state dir. Co-Authored-By: Claude Fable 5.1 --- test/gbrain-repo-policy-client.test.ts | 12 +++- test/memorable-user-prompt-hook.test.ts | 74 ++++++++++++++++++++++--- test/redact-engine.test.ts | 14 +++++ test/uninstall.test.ts | 34 ++++++++++++ 4 files changed, 126 insertions(+), 8 deletions(-) diff --git a/test/gbrain-repo-policy-client.test.ts b/test/gbrain-repo-policy-client.test.ts index de130e29c..89a6b1247 100644 --- a/test/gbrain-repo-policy-client.test.ts +++ b/test/gbrain-repo-policy-client.test.ts @@ -19,7 +19,7 @@ import * as path from "path"; import * as os from "os"; import { spawnSync } from "child_process"; -import { repoPolicyTierBatch } from "../lib/gbrain-repo-policy-client"; +import { repoPolicyTier, repoPolicyTierBatch } from "../lib/gbrain-repo-policy-client"; import { canonicalizeRemote } from "../lib/gstack-memory-helpers"; const ROOT = path.resolve(import.meta.dir, ".."); @@ -209,3 +209,13 @@ describe("normalize parity: bash normalize() ↔ canonicalizeRemote (edge URL sh expect(verdicts.get(canon)).toEqual({ tier: "deny" }); }); }); + +describe("repoPolicyTier timeoutMs (hook deadline seam)", () => { + test("a spawn that cannot finish inside timeoutMs classifies as unreadable; the default still reads the tier", () => { + const url = "https://github.com/example/timed.git"; + expect(run(["set", url, "deny"]).status).toBe(0); + expect(repoPolicyTier(url, env())).toEqual({ tier: "deny" }); + // 1 ms cannot cover a bash+jq spawn; the caller's polarity decides what unreadable means + expect(repoPolicyTier(url, env(), 1)).toEqual({ tier: "none", error: "unreadable" }); + }); +}); diff --git a/test/memorable-user-prompt-hook.test.ts b/test/memorable-user-prompt-hook.test.ts index 8e9492bea..7d5148a3b 100644 --- a/test/memorable-user-prompt-hook.test.ts +++ b/test/memorable-user-prompt-hook.test.ts @@ -31,7 +31,7 @@ import * as os from 'os'; import * as path from 'path'; import { listReceipts, sha256Hex, verifyLedger } from '../lib/egress-receipt'; import { - budgetFor, budgetMs, capUtf8, logHookError, pickAdditionalContext, renderContext, resolveVendor, safeStderrTail, + budgetFor, budgetMs, capUtf8, firstJsonObject, logHookError, pickAdditionalContext, renderContext, resolveVendor, safeStderrTail, stringLeaves, stripControl, vendorEnv, BUDGET_MS, LOG_RATE_LIMIT_MS, OUTPUT_CAP_BYTES, ENVELOPE_SOURCE, } from '../hosts/claude/hooks/memorable-user-prompt-hook.ts'; @@ -210,11 +210,9 @@ describe('gate on: the mediated hand-off', () => { } }); - test('a disable that lands mid-flight wins: the gate is re-checked right before the spawn', () => { - // Simulate with a config that reads `on` for the first check and `off` for the second: - // impossible to interleave deterministically from outside, so drive the pure ordering - // through the config store itself — flip the key off right before the hook runs but after - // a warm run proved the on-path works. The observable contract is "off wins": no spawn. + test('gate flipped off between two runs: the second run spawns nothing (the mid-flight re-check itself cannot be interleaved from outside)', () => { + // The pre-spawn re-check reads the same store; a deterministic mid-flight flip would need a + // seam inside main(). This pins the observable contract only: once off, no spawn. gateOn(); expect(runHook(PROMPT).stdout).toContain('remembered'); spawnSync('bash', [CONFIG, 'set', 'memorable_recall', 'off'], { env, encoding: 'utf8', timeout: 20_000 }); @@ -467,6 +465,16 @@ describe('pure helpers', () => { expect(pickAdditionalContext(JSON.stringify({ decision: 'block' }))).toBeNull(); expect(pickAdditionalContext('nope')).toBeNull(); }); + test('pickAdditionalContext keeps the answer when a background helper appends a line to stdout, or a banner precedes it', () => { + const answer = JSON.stringify({ hookSpecificOutput: { additionalContext: 'kept {"}"} braces in strings' } }); + expect(pickAdditionalContext(`${answer}\nhelper: flushed 3 events\n`)).toBe('kept {"}"} braces in strings'); + expect(pickAdditionalContext(`memorable v0.5.18\n${answer}`)).toBe('kept {"}"} braces in strings'); + expect(pickAdditionalContext(`{\n "hookSpecificOutput": {\n "additionalContext": "pretty"\n }\n}\n`)).toBe('pretty'); + expect(firstJsonObject('{"a": {"b": 1}} trailing')).toEqual({ a: { b: 1 } }); + expect(firstJsonObject('{"unterminated": ')).toBeNull(); + expect(firstJsonObject('no braces here')).toBeNull(); + expect(firstJsonObject('{"s": "\\"}"}')).toEqual({ s: '"}' }); + }); test('stripControl drops C0 controls, CR and DEL but keeps tab and newline', () => { const input = 'a' + String.fromCharCode(0) + 'b' + String.fromCharCode(27) + '\tc\nd' + String.fromCharCode(127) + 'e\rf\r\ng'; expect(stripControl(input)).toBe('ab\tc\ndef\ng'); @@ -496,6 +504,10 @@ describe('pure helpers', () => { expect(lines()).toHaveLength(2); logHookError('A', t0 + LOG_RATE_LIMIT_MS + 1); expect(lines()).toHaveLength(3); + // a caller-supplied key rate-limits messages whose text varies (a vendor's timestamped stderr) + logHookError('vendor timeout: at 12:00:01', t0 + LOG_RATE_LIMIT_MS + 2, 'vendor timeout'); + logHookError('vendor timeout: at 12:00:02', t0 + LOG_RATE_LIMIT_MS + 3, 'vendor timeout'); + expect(lines()).toHaveLength(4); if (process.platform !== 'win32') expect(fs.statSync(path.join(home, '.gstack', 'hook-errors.log')).mode & 0o077).toBe(0); } finally { if (prev === undefined) delete process.env.GSTACK_STATE_ROOT; else process.env.GSTACK_STATE_ROOT = prev; @@ -611,7 +623,7 @@ describe('deadline and policy failure paths (review coverage)', () => { expect(r).toEqual({ status: 0, stdout: '', stderr: '' }); expect(calls()).toBe(''); expect(fs.existsSync(ledger())).toBe(false); - expect(errLog()).toContain('trust policy store unreadable'); + expect(errLog()).toContain('trust policy lookup failed'); } finally { fs.rmSync(repo, { recursive: true, force: true }); } @@ -625,3 +637,51 @@ describe('deadline and policy failure paths (review coverage)', () => { expect(r.stdout).toContain('remembered'); }); }); + +describe('trust-policy lookup outcomes (review coverage, second pass)', () => { + function withStore(): void { + // any policy for any url creates the store; the cwd under test has a different or no remote + const set = spawnSync('bash', [POLICY, 'set', 'https://github.com/example/unrelated.git', 'deny'], { env, encoding: 'utf8', timeout: 20_000 }); + expect(set.status).toBe(0); + } + test('store present, cwd is a plain directory (not a repo): recall proceeds', () => { + gateOn(); + withStore(); + const plain = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-memo-plain-')); + try { + const r = runHook(JSON.stringify({ prompt: 'hello', cwd: plain }), {}, plain); + expect(r.stdout).toContain('remembered'); + expect(receipts()).toHaveLength(1); + } finally { + fs.rmSync(plain, { recursive: true, force: true }); + } + }); + test('store present, repo with an origin but no policy for it: recall proceeds', () => { + gateOn(); + withStore(); + const repo = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-memo-repo-')); + try { + spawnSync('git', ['init', '-q'], { cwd: repo, timeout: 10_000 }); + spawnSync('git', ['remote', 'add', 'origin', 'https://github.com/example/other.git'], { cwd: repo, timeout: 10_000 }); + expect(runHook(JSON.stringify({ prompt: 'hello', cwd: repo }), {}, repo).stdout).toContain('remembered'); + } finally { + fs.rmSync(repo, { recursive: true, force: true }); + } + }); + test('store present, repository git cannot read (corrupt .git/config): fails closed, nothing spawned', () => { + gateOn(); + withStore(); + const repo = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-memo-repo-')); + try { + spawnSync('git', ['init', '-q'], { cwd: repo, timeout: 10_000 }); + fs.writeFileSync(path.join(repo, '.git', 'config'), '[core\nbroken = '); + const r = runHook(JSON.stringify({ prompt: 'hello', cwd: repo }), {}, repo); + expect(r).toEqual({ status: 0, stdout: '', stderr: '' }); + expect(calls()).toBe(''); + expect(fs.existsSync(ledger())).toBe(false); + expect(errLog()).toContain('trust policy lookup failed'); + } finally { + fs.rmSync(repo, { recursive: true, force: true }); + } + }); +}); diff --git a/test/redact-engine.test.ts b/test/redact-engine.test.ts index 67598e79f..48ee9316f 100644 --- a/test/redact-engine.test.ts +++ b/test/redact-engine.test.ts @@ -589,6 +589,20 @@ describe("redactFindingSpans — machine-egress masking (#1947)", () => { } }); + test("line/col at boundaries: line start, after blank lines, first char, last unterminated line", () => { + const token = "ghp_" + "1234567890abcdefghijklmnopqrstuvwxyz"; + const at = (text: string) => { + const f = scan(text, { repoVisibility: "private" }).findings.find((x) => x.id === "github.pat"); + expect(f).toBeDefined(); + return [f!.line, f!.col]; + }; + expect(at(`a\nb\n${token} x`)).toEqual([3, 1]); + expect(at(`a\n\n\n ${token}`)).toEqual([4, 3]); + expect(at(token)).toEqual([1, 1]); + expect(at(`one\r\ntwo ${token}`)).toEqual([2, 5]); + expect(redactFindingSpans(`a\nb\n${token} x`, { repoVisibility: "private" })).toBe("a\nb\n x"); + }); + test("multiline input redacts a finding past the first line (locateSpan line/col path)", () => { const token = "ghp_" + "1234567890abcdefghijklmnopqrstuvwxyz"; const out = redactFindingSpans(`line one\nline two has ${token}\nline three`, { diff --git a/test/uninstall.test.ts b/test/uninstall.test.ts index 7542b741c..317e9a439 100644 --- a/test/uninstall.test.ts +++ b/test/uninstall.test.ts @@ -376,6 +376,8 @@ describe('the Memorable arm stays quiet when nothing of its is registered', () = expect(result.stdout).not.toContain('Memorable UserPromptSubmit hook'); const s = JSON.parse(fs.readFileSync(settingsFile, 'utf-8')); expect(s.hooks.UserPromptSubmit[0].hooks[0].command).toBe('/Users/me/my-own-hook'); + // the consent flip only runs when the key reads on: no config file is created just to say off + expect(fs.existsSync(path.join(mockHome, '.gstack', 'config.yaml'))).toBe(false); } finally { fs.rmSync(tmp, { recursive: true, force: true }); } @@ -436,3 +438,35 @@ describe('hook cleanup under lock contention is loud, never silent (review-army) // per-test budget is too tight on a busy box. }, 30000); }); + +describe('the consent key never outlives the hook, even when the config lives outside the removed state dir', () => { + test('full uninstall (no --keep-state) with GSTACK_STATE_ROOT elsewhere: memorable_recall flips off there', () => { + const tmp = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-uninstall-memo-root-')); + try { + const mockHome = path.join(tmp, 'home'); + const otherRoot = path.join(tmp, 'elsewhere'); + const installRoot = path.join(mockHome, '.claude', 'skills', 'gstack'); + const installBin = path.join(installRoot, 'bin'); + fs.mkdirSync(installBin, { recursive: true }); + fs.mkdirSync(otherRoot, { recursive: true }); + for (const b of ['gstack-uninstall', 'gstack-settings-hook', 'gstack-session-update', 'gstack-config']) { + const dst = path.join(installBin, b); + fs.copyFileSync(path.join(ROOT, 'bin', b), dst); + fs.chmodSync(dst, 0o755); + } + const settingsFile = path.join(mockHome, '.claude', 'settings.json'); + fs.writeFileSync(settingsFile, JSON.stringify({ hooks: {} })); + fs.mkdirSync(path.join(mockHome, '.gstack'), { recursive: true }); + const env = { ...process.env, HOME: mockHome, GSTACK_SETTINGS_FILE: settingsFile, GSTACK_STATE_ROOT: otherRoot }; + expect(spawnSync('bash', [path.join(installBin, 'gstack-config'), 'set', 'memorable_recall', 'on'], { env, timeout: 20_000 }).status).toBe(0); + const result = spawnSync('bash', [path.join(installBin, 'gstack-uninstall'), '--force'], { + stdio: 'pipe', timeout: 30_000, encoding: 'utf-8', cwd: tmp, env, + }); + expect(result.status).toBe(0); + expect(fs.existsSync(path.join(mockHome, '.gstack'))).toBe(false); // the default state dir went + expect(fs.readFileSync(path.join(otherRoot, 'config.yaml'), 'utf-8')).toMatch(/memorable_recall: off/); // the real config did not keep consent + } finally { + fs.rmSync(tmp, { recursive: true, force: true }); + } + }); +});