From c244889fa9c5a6836be464afa68229a9ad40d41a Mon Sep 17 00:00:00 2001 From: James Ross Date: Mon, 3 Aug 2026 13:28:42 -0700 Subject: [PATCH 01/20] fix: share one capture followthrough policy across CLI and MCP The 6s followthrough budget and the waitForCaptureFollowthrough helper were duplicated verbatim in src/cli/commands/capture.js and src/mcp/service.js, so the two capture surfaces could drift apart. The CLI compared the deferral sentinel by object identity while MCP compared it by status, and the CLI reported a hardcoded timeoutMs in its capture.followthrough.deferred event. Extract src/capture-followthrough.js as the single owner of the budget, the frozen deferral sentinel, the deferral predicate, and the race. Both surfaces now import it, and the CLI reports the budget it actually used. The budget becomes overridable via THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS so operators on cold repositories or slow filesystems can raise it. An unusable value falls back to the default instead of throwing, because capture is a trapdoor and must not fail on a malformed knob. --- src/capture-followthrough.js | 61 +++++++++++++++++++ src/cli/commands/capture.js | 35 ++++------- src/mcp/service.js | 27 ++------- test/ports/capture-followthrough.test.js | 76 ++++++++++++++++++++++++ 4 files changed, 154 insertions(+), 45 deletions(-) create mode 100644 src/capture-followthrough.js create mode 100644 test/ports/capture-followthrough.test.js diff --git a/src/capture-followthrough.js b/src/capture-followthrough.js new file mode 100644 index 0000000..179e155 --- /dev/null +++ b/src/capture-followthrough.js @@ -0,0 +1,61 @@ +/** + * Capture followthrough budget. + * + * Capture is a trapdoor: the raw thought is committed first, and derived + * graph work runs afterwards. That derived work is bounded so a slow or cold + * repository cannot hold a capture call open. When the budget elapses the + * followthrough is abandoned (not cancelled) and the caller reports a + * deferred outcome — the raw thought is already safe. + * + * Both the CLI and MCP capture surfaces share this module so the budget and + * the deferral semantics cannot drift apart between them. + */ + +export const DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS = 6_000; + +export const CAPTURE_FOLLOWTHROUGH_DEFERRED = Object.freeze({ status: 'deferred' }); + +/** + * Resolve the followthrough budget from the environment. + * + * An unusable value falls back to the default rather than throwing: capture + * must not fail because of a malformed knob. + */ +export function resolveCaptureFollowthroughTimeoutMs(environment = process.env) { + const raw = String(environment.THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS ?? '').trim(); + if (raw === '' || !/^\d+$/u.test(raw)) { + return DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS; + } + + const parsed = Number.parseInt(raw, 10); + if (!Number.isSafeInteger(parsed) || parsed <= 0) { + return DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS; + } + + return parsed; +} + +export function isDeferredCaptureFollowthrough(followthrough) { + return followthrough?.status === CAPTURE_FOLLOWTHROUGH_DEFERRED.status; +} + +/** + * Race a followthrough promise against the budget. + * + * Rejections propagate so callers can report the underlying failure. Callers + * that keep the abandoned promise alive must attach their own catch handler. + */ +export async function waitForCaptureFollowthrough(followthroughPromise, { timeoutMs } = {}) { + const budgetMs = timeoutMs ?? resolveCaptureFollowthroughTimeoutMs(); + let timeoutId = null; + const timeout = new Promise((resolve) => { + timeoutId = setTimeout(() => resolve(CAPTURE_FOLLOWTHROUGH_DEFERRED), budgetMs); + timeoutId.unref?.(); + }); + + try { + return await Promise.race([followthroughPromise, timeout]); + } finally { + clearTimeout(timeoutId); + } +} diff --git a/src/cli/commands/capture.js b/src/cli/commands/capture.js index 508cd6c..0bd551b 100644 --- a/src/cli/commands/capture.js +++ b/src/cli/commands/capture.js @@ -1,4 +1,9 @@ import { ensureGitRepo, hasGitRepo, pushWarpRefs } from '../../git.js'; +import { + isDeferredCaptureFollowthrough, + resolveCaptureFollowthroughTimeoutMs, + waitForCaptureFollowthrough, +} from '../../capture-followthrough.js'; import { captureProvenanceFromEnvironment } from '../../capture-provenance.js'; import { getCaptureAmbientContext, getAmbientProjectContext } from '../../project-context.js'; import { getLocalRepoDir, getUpstreamUrl } from '../../paths.js'; @@ -10,8 +15,6 @@ import { saveRawCapture, } from '../../store.js'; -const CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS = 6_000; -const CAPTURE_FOLLOWTHROUGH_DEFERRED = Object.freeze({ status: 'deferred' }); const NO_GRAPH_MIGRATION_STATUS = Object.freeze({ currentGraphModelVersion: null, requiredGraphModelVersion: null, @@ -53,18 +56,20 @@ export async function runCapture(thought, output, reporter) { } async function runCaptureFollowthrough(repoDir, entryId, repoAlreadyExists, reporter) { + const timeoutMs = resolveCaptureFollowthroughTimeoutMs(); + try { const graphStatusPromise = repoAlreadyExists ? getGraphModelStatus(repoDir) : Promise.resolve(NO_GRAPH_MIGRATION_STATUS); - const graphStatus = await waitForCaptureFollowthrough(graphStatusPromise); - if (graphStatus === CAPTURE_FOLLOWTHROUGH_DEFERRED) { + const graphStatus = await waitForCaptureFollowthrough(graphStatusPromise, { timeoutMs }); + if (isDeferredCaptureFollowthrough(graphStatus)) { graphStatusPromise.catch(() => {}); reporter.event('capture.followthrough.deferred', { command: 'capture', trigger: 'post_capture', entryId, - timeoutMs: CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS, + timeoutMs, }); return; } @@ -83,14 +88,14 @@ async function runCaptureFollowthrough(repoDir, entryId, repoAlreadyExists, repo migrateIfNeeded: false, ambientContext: getAmbientProjectContext(process.cwd()), }); - const followthrough = await waitForCaptureFollowthrough(followthroughPromise); - if (followthrough === CAPTURE_FOLLOWTHROUGH_DEFERRED) { + const followthrough = await waitForCaptureFollowthrough(followthroughPromise, { timeoutMs }); + if (isDeferredCaptureFollowthrough(followthrough)) { followthroughPromise.catch(() => {}); reporter.event('capture.followthrough.deferred', { command: 'capture', trigger: 'post_capture', entryId, - timeoutMs: CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS, + timeoutMs, }); } } catch (error) { @@ -119,20 +124,6 @@ async function runBackup(repoDir, output, reporter) { return 0; } -async function waitForCaptureFollowthrough(followthroughPromise) { - let timeoutId = null; - const timeout = new Promise((resolve) => { - timeoutId = setTimeout(() => resolve(CAPTURE_FOLLOWTHROUGH_DEFERRED), CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS); - timeoutId.unref?.(); - }); - - try { - return await Promise.race([followthroughPromise, timeout]); - } finally { - clearTimeout(timeoutId); - } -} - export async function runIngest(stdin, output, reporter) { const thought = await readStdinText(stdin); diff --git a/src/mcp/service.js b/src/mcp/service.js index cfa4b6d..1024c5e 100644 --- a/src/mcp/service.js +++ b/src/mcp/service.js @@ -1,3 +1,7 @@ +import { + isDeferredCaptureFollowthrough, + waitForCaptureFollowthrough, +} from '../capture-followthrough.js'; import { runDiagnostics } from '../doctor.js'; import { ValidationError, NotFoundError, GraphError } from '../errors.js'; import { ensureGitRepo, getFsmonitorStatus, hasGitRepo, lsRemote, pushWarpRefs } from '../git.js'; @@ -39,8 +43,6 @@ import { StatsOutcome, } from './result.js'; -const CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS = 6_000; -const CAPTURE_FOLLOWTHROUGH_DEFERRED = Object.freeze({ status: 'deferred' }); const CAPTURE_FOLLOWTHROUGH_DEFERRED_WARNING = 'Capture followthrough deferred; raw thought saved locally before derived graph updates completed.'; const NO_GRAPH_MIGRATION_STATUS = Object.freeze({ @@ -131,10 +133,6 @@ async function buildCaptureFollowthrough(deps, repoDir, entryId, repoAlreadyExis }); } -function isDeferredCaptureFollowthrough(followthrough) { - return followthrough?.status === CAPTURE_FOLLOWTHROUGH_DEFERRED.status; -} - async function runCaptureBackup(deps, repoDir) { const upstreamUrl = deps.getUpstreamUrl(); if (!upstreamUrl) { @@ -145,23 +143,6 @@ async function runCaptureBackup(deps, repoDir) { return backedUp ? 'backed_up' : 'pending'; } -async function waitForCaptureFollowthrough(followthroughPromise) { - let timeoutId = null; - const timeout = new Promise((resolve) => { - timeoutId = setTimeout( - () => resolve(CAPTURE_FOLLOWTHROUGH_DEFERRED), - CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS - ); - timeoutId.unref?.(); - }); - - try { - return await Promise.race([followthroughPromise, timeout]); - } finally { - clearTimeout(timeoutId); - } -} - export async function listRecentThoughts({ count = null, query = null } = {}) { const repoDir = getLocalRepoDir(); if (!hasGitRepo(repoDir)) { diff --git a/test/ports/capture-followthrough.test.js b/test/ports/capture-followthrough.test.js new file mode 100644 index 0000000..20fb839 --- /dev/null +++ b/test/ports/capture-followthrough.test.js @@ -0,0 +1,76 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { + CAPTURE_FOLLOWTHROUGH_DEFERRED, + DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS, + isDeferredCaptureFollowthrough, + resolveCaptureFollowthroughTimeoutMs, + waitForCaptureFollowthrough, +} from '../../src/capture-followthrough.js'; + +test('the default capture followthrough budget stays at the documented 6 seconds', () => { + assert.equal(DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS, 6_000); + assert.equal(resolveCaptureFollowthroughTimeoutMs({}), 6_000); + assert.equal(resolveCaptureFollowthroughTimeoutMs({ THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS: '' }), 6_000); + assert.equal(resolveCaptureFollowthroughTimeoutMs({ THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS: ' ' }), 6_000); +}); + +test('operators can raise the followthrough budget for slow or cold repositories', () => { + assert.equal( + resolveCaptureFollowthroughTimeoutMs({ THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS: '60000' }), + 60_000 + ); + assert.equal( + resolveCaptureFollowthroughTimeoutMs({ THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS: ' 250 ' }), + 250 + ); +}); + +test('an unusable followthrough budget falls back to the default instead of breaking the capture trapdoor', () => { + for (const raw of ['0', '-1', 'soon', '12.5', 'NaN', 'Infinity', '1e3']) { + assert.equal( + resolveCaptureFollowthroughTimeoutMs({ THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS: raw }), + DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS, + `Expected "${raw}" to fall back to the default followthrough budget.` + ); + } +}); + +test('waitForCaptureFollowthrough returns the settled followthrough when it beats the budget', async () => { + const result = await waitForCaptureFollowthrough(Promise.resolve({ migration: null }), { + timeoutMs: 10_000, + }); + + assert.deepEqual(result, { migration: null }); + assert.equal(isDeferredCaptureFollowthrough(result), false); +}); + +test('waitForCaptureFollowthrough defers once the budget elapses', async () => { + const neverSettles = new Promise(() => {}); + + const result = await waitForCaptureFollowthrough(neverSettles, { timeoutMs: 1 }); + + assert.equal(result, CAPTURE_FOLLOWTHROUGH_DEFERRED); + assert.equal(isDeferredCaptureFollowthrough(result), true); +}); + +test('waitForCaptureFollowthrough propagates followthrough rejections to the caller', async () => { + await assert.rejects( + waitForCaptureFollowthrough(Promise.reject(new Error('graph write failed')), { timeoutMs: 10_000 }), + /graph write failed/ + ); +}); + +test('isDeferredCaptureFollowthrough matches the sentinel by status, not identity', () => { + assert.equal(isDeferredCaptureFollowthrough({ status: 'deferred' }), true); + assert.equal(isDeferredCaptureFollowthrough({ status: 'done' }), false); + assert.equal(isDeferredCaptureFollowthrough({ migration: null }), false); + assert.equal(isDeferredCaptureFollowthrough(null), false); + assert.equal(isDeferredCaptureFollowthrough(undefined), false); +}); + +test('the deferred sentinel is frozen so surfaces cannot mutate shared capture state', () => { + assert.equal(Object.isFrozen(CAPTURE_FOLLOWTHROUGH_DEFERRED), true); + assert.equal(CAPTURE_FOLLOWTHROUGH_DEFERRED.status, 'deferred'); +}); From 73a85f88e629547416ed455dd6dca315367d11d9 Mon Sep 17 00:00:00 2001 From: James Ross Date: Mon, 3 Aug 2026 13:28:56 -0700 Subject: [PATCH 02/20] fix: stop acceptance tests writing into a developer's real mind The acceptance fixtures pinned HOME to a temp dir but inherited the rest of the environment. getLocalRepoDir honours THINK_REPO_DIR *ahead* of $HOME/.think/repo, so a developer with THINK_REPO_DIR exported ran the whole suite against their real mind. Multi-agent setups export exactly that variable to namespace minds, so this was the normal case. Reproduced before the fix: running test/acceptance/mcp.test.js with THINK_REPO_DIR pointed at a sentinel directory wrote 21 commits of test captures into it, and cross-contamination also failed one assertion. After the fix the same run leaves the sentinel empty and passes 6/6. THINK_PROMPT_METRICS_FILE and the THINK_CAPTURE_* provenance variables bypassed HOME the same way. Add createHermeticThinkEnv/scrubThinkEnv, which drop every inherited THINK_ variable before layering the fixture's own values back on. The scrub is prefix-wide rather than an allowlist so a newly added knob cannot silently reopen the hole; baseEnv and extraEnv still layer on top, so tests that set THINK_ variables deliberately keep working. Covered by test/ports/test-isolation.test.js, which guards the harness itself because a regression here silently corrupts real user data. --- test/acceptance/repair-v17-mind.test.js | 7 +- test/fixtures/runtime.js | 39 +++++++++ test/fixtures/think.js | 14 ++-- test/ports/test-isolation.test.js | 101 ++++++++++++++++++++++++ 4 files changed, 151 insertions(+), 10 deletions(-) create mode 100644 test/ports/test-isolation.test.js diff --git a/test/acceptance/repair-v17-mind.test.js b/test/acceptance/repair-v17-mind.test.js index d517b5b..f52de93 100644 --- a/test/acceptance/repair-v17-mind.test.js +++ b/test/acceptance/repair-v17-mind.test.js @@ -9,7 +9,7 @@ import test from 'node:test'; import { resolveGitWarpPackageRoot, } from '../../scripts/repair-v17-mind.mjs'; -import { baseEnv, formatResult, repoRoot } from '../fixtures/runtime.js'; +import { baseEnv, formatResult, repoRoot, scrubThinkEnv } from '../fixtures/runtime.js'; import { createTempDir } from '../fixtures/tmp.js'; import { assertSuccess } from '../support/assertions.js'; @@ -196,8 +196,11 @@ function hasV17GitWarpMigration() { } function fixtureEnv() { + // Every repair invocation targets an explicit --repo, but scrub inherited + // THINK_ state anyway so a future fallback to $HOME/.think cannot reach a + // developer's real mind. return { - ...process.env, + ...scrubThinkEnv(), ...baseEnv, COPYFILE_DISABLE: '1', }; diff --git a/test/fixtures/runtime.js b/test/fixtures/runtime.js index c44d56a..eac3afc 100644 --- a/test/fixtures/runtime.js +++ b/test/fixtures/runtime.js @@ -16,6 +16,45 @@ export const baseEnv = { TERM: 'dumb', }; +export const THINK_ENV_PREFIX = 'THINK_'; + +/** + * Build a hermetic environment for a spawned Think process. + * + * Pinning HOME is not enough on its own. `getLocalRepoDir` honours + * THINK_REPO_DIR ahead of $HOME/.think/repo, and `getPromptMetricsFile` and + * `captureProvenanceFromEnvironment` read their own THINK_ variables, so an + * inherited environment can redirect the suite at a developer's real mind and + * write test captures into it. + * + * Every inherited THINK_ variable is therefore dropped. The scrub is + * prefix-wide rather than an allowlist so a newly added knob cannot quietly + * reopen the hole. Fixtures and individual tests can still set THINK_ + * variables deliberately through `baseEnv` and `extraEnv`, which are layered + * on after the scrub. + */ +export function scrubThinkEnv(processEnv = process.env) { + return Object.fromEntries( + Object.entries(processEnv).filter(([key]) => !key.startsWith(THINK_ENV_PREFIX)) + ); +} + +export function createHermeticThinkEnv({ + processEnv = process.env, + baseEnv: base = baseEnv, + extraEnv = {}, + homeDir, + upstreamUrl, +}) { + return { + ...scrubThinkEnv(processEnv), + ...base, + ...extraEnv, + HOME: homeDir, + THINK_UPSTREAM_URL: upstreamUrl, + }; +} + export function formatResult(result) { const pieces = [ `exit status: ${result.status}`, diff --git a/test/fixtures/think.js b/test/fixtures/think.js index 9a3659b..b9df3ff 100644 --- a/test/fixtures/think.js +++ b/test/fixtures/think.js @@ -1,7 +1,7 @@ import { spawnSync } from 'node:child_process'; import path from 'node:path'; -import { baseEnv, cliEntrypoint, repoRoot, requireCliEntrypoint } from './runtime.js'; +import { cliEntrypoint, createHermeticThinkEnv, repoRoot, requireCliEntrypoint } from './runtime.js'; import { createBareGitRepo } from './git.js'; import { createTempDir } from './tmp.js'; @@ -34,12 +34,10 @@ export function runThink(context, args, extraEnv = {}, options = {}) { cwd: options.cwd ?? repoRoot, encoding: 'utf8', input: options.input, - env: { - ...process.env, - ...baseEnv, - ...extraEnv, - HOME: context.homeDir, - THINK_UPSTREAM_URL: context.upstreamUrl, - }, + env: createHermeticThinkEnv({ + extraEnv, + homeDir: context.homeDir, + upstreamUrl: context.upstreamUrl, + }), }); } diff --git a/test/ports/test-isolation.test.js b/test/ports/test-isolation.test.js new file mode 100644 index 0000000..30a4703 --- /dev/null +++ b/test/ports/test-isolation.test.js @@ -0,0 +1,101 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { createHermeticThinkEnv, THINK_ENV_PREFIX } from '../fixtures/runtime.js'; + +/** + * These guard the test harness itself. + * + * Think resolves its mind directory from THINK_REPO_DIR *before* falling back + * to $HOME/.think/repo (see getLocalRepoDir in src/paths.js). A fixture that + * pins HOME but inherits THINK_REPO_DIR therefore runs the suite against + * whatever mind the developer has exported — writing test captures into real + * memory. Multi-agent setups export exactly that variable to namespace their + * minds, so this is the normal case, not an exotic one. + */ + +const HOME_DIR = '/tmp/think-home-test'; +const UPSTREAM_URL = '/tmp/think-upstream.git'; + +function buildEnv({ processEnv = {}, extraEnv = {} } = {}) { + return createHermeticThinkEnv({ + processEnv, + baseEnv: { LANG: 'C' }, + extraEnv, + homeDir: HOME_DIR, + upstreamUrl: UPSTREAM_URL, + }); +} + +test('inherited THINK_REPO_DIR cannot redirect the suite at a real mind', () => { + const env = buildEnv({ processEnv: { THINK_REPO_DIR: '/Users/dev/.think/claude' } }); + + assert.equal( + Object.hasOwn(env, 'THINK_REPO_DIR'), + false, + 'Expected an inherited THINK_REPO_DIR to be scrubbed so HOME alone decides the mind.' + ); +}); + +test('every inherited THINK_ variable is scrubbed, not just the known dangerous ones', () => { + const env = buildEnv({ + processEnv: { + THINK_REPO_DIR: '/Users/dev/.think/claude', + THINK_PROMPT_METRICS_FILE: '/Users/dev/.think/metrics/prompt-ux.jsonl', + THINK_CAPTURE_INGRESS: 'share', + THINK_CAPTURE_SOURCE_APP: 'Safari', + THINK_CAPTURE_SOURCE_URL: 'https://example.com', + THINK_SOME_FUTURE_KNOB: 'surprise', + }, + }); + + const leaked = Object.keys(env) + .filter((key) => key.startsWith(THINK_ENV_PREFIX)) + .filter((key) => key !== 'THINK_UPSTREAM_URL'); + + assert.deepEqual( + leaked, + [], + 'Expected a prefix-wide scrub so newly added THINK_ knobs cannot silently reopen this hole.' + ); +}); + +test('non-Think environment such as PATH still reaches the child process', () => { + const env = buildEnv({ + processEnv: { PATH: '/usr/bin:/bin', HOME: '/Users/dev', SHELL: '/bin/zsh' }, + }); + + assert.equal(env.PATH, '/usr/bin:/bin', 'Expected PATH to survive so the child can spawn git and node.'); + assert.equal(env.SHELL, '/bin/zsh'); +}); + +test('the fixture pins HOME and the upstream URL over anything inherited', () => { + const env = buildEnv({ processEnv: { HOME: '/Users/dev', THINK_UPSTREAM_URL: 'git@github.com:dev/real.git' } }); + + assert.equal(env.HOME, HOME_DIR); + assert.equal(env.THINK_UPSTREAM_URL, UPSTREAM_URL); +}); + +test('tests can still set THINK_ variables deliberately through extraEnv', () => { + const env = buildEnv({ + processEnv: { THINK_PROMPT_METRICS_FILE: '/Users/dev/.think/metrics/prompt-ux.jsonl' }, + extraEnv: { + THINK_PROMPT_METRICS_FILE: '/tmp/fixture-metrics.jsonl', + THINK_TEST_NOW: '1700000000000', + }, + }); + + assert.equal( + env.THINK_PROMPT_METRICS_FILE, + '/tmp/fixture-metrics.jsonl', + 'Expected an explicit fixture override to win over the scrub.' + ); + assert.equal(env.THINK_TEST_NOW, '1700000000000'); +}); + +test('baseEnv is applied and does not resurrect scrubbed Think state', () => { + const env = buildEnv({ processEnv: { THINK_REPO_DIR: '/Users/dev/.think/claude' } }); + + assert.equal(env.LANG, 'C'); + assert.equal(Object.hasOwn(env, 'THINK_REPO_DIR'), false); +}); From bc1b3ad0b38be3c354def5b4795856abe0b67e6d Mon Sep 17 00:00:00 2001 From: James Ross Date: Mon, 3 Aug 2026 13:29:08 -0700 Subject: [PATCH 03/20] test: pin a deterministic followthrough budget in MCP acceptance tests Asserting capture.warnings.length === 0 against the 6s production default made a correctness assertion depend on wall-clock latency: it passed in isolation but failed under the concurrent cold spawns of the full acceptance suite (actual 1, expected 0). Pin a generous 120s budget for these spawns so they observe the completed-followthrough path deterministically. The assertion still proves the healthy path; the deferral path stays covered without a wall clock by the dependency-injected test in test/ports/mcp-service.test.js. Retires docs/method/backlog/bad-code/ CORE_mcp-capture-warning-assertion-is-timing-dependent.md. --- test/acceptance/mcp.test.js | 28 ++++++++++++++++++++-------- 1 file changed, 20 insertions(+), 8 deletions(-) diff --git a/test/acceptance/mcp.test.js b/test/acceptance/mcp.test.js index fb7c005..d8125f4 100644 --- a/test/acceptance/mcp.test.js +++ b/test/acceptance/mcp.test.js @@ -6,10 +6,21 @@ import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js' import { createThinkContext, runThink } from '../fixtures/think.js'; import { makePromptMetric, seedPromptMetricsFile } from '../fixtures/prompt-metrics.js'; -import { baseEnv, repoRoot } from '../fixtures/runtime.js'; +import { createHermeticThinkEnv, repoRoot } from '../fixtures/runtime.js'; const mcpEntrypoint = './bin/think-mcp.js'; +/** + * Capture followthrough defers after a wall-clock budget, so asserting "no + * warnings on a healthy repo" against the 6s production default turns a + * correctness assertion into a latency assertion — it fails whenever the + * acceptance suite's concurrent cold spawns load the machine. Pin a generous + * budget so these tests observe the completed-followthrough path + * deterministically. The deferral path is covered without a wall clock by + * test/ports/mcp-service.test.js. + */ +const ACCEPTANCE_FOLLOWTHROUGH_TIMEOUT_MS = '120000'; + test('think MCP server lists the core Think tools', async () => { const context = await createThinkContext(); @@ -221,13 +232,14 @@ async function withThinkMcpClient(context, run, extraEnv = {}) { command: process.execPath, args: [mcpEntrypoint], cwd: repoRoot, - env: { - ...process.env, - ...baseEnv, - ...extraEnv, - HOME: context.homeDir, - THINK_UPSTREAM_URL: context.upstreamUrl, - }, + env: createHermeticThinkEnv({ + extraEnv: { + THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS: ACCEPTANCE_FOLLOWTHROUGH_TIMEOUT_MS, + ...extraEnv, + }, + homeDir: context.homeDir, + upstreamUrl: context.upstreamUrl, + }), stderr: 'pipe', }); const stderrChunks = []; From ea04d36a376163689329a019df245e4b63b62c90 Mon Sep 17 00:00:00 2001 From: James Ross Date: Mon, 3 Aug 2026 13:33:26 -0700 Subject: [PATCH 04/20] fix: fail loudly when a test fixture omits its sandbox home Node omits env pairs whose value is undefined, so createHermeticThinkEnv called without homeDir produced a child with no HOME at all. getHomeDir falls back to os.homedir(), which means the suite would quietly resolve ~/.think against the developer's real home and reopen the isolation hole with no visible error. Verified: spawning a child with HOME set to undefined leaves process.env.HOME null while os.homedir() still returns the real home. Require homeDir, and normalise a missing upstreamUrl to '' so an inherited THINK_UPSTREAM_URL cannot survive the scrub either. --- test/fixtures/runtime.js | 9 ++++++++- test/ports/test-isolation.test.js | 27 +++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 1 deletion(-) diff --git a/test/fixtures/runtime.js b/test/fixtures/runtime.js index eac3afc..644579a 100644 --- a/test/fixtures/runtime.js +++ b/test/fixtures/runtime.js @@ -46,12 +46,19 @@ export function createHermeticThinkEnv({ homeDir, upstreamUrl, }) { + // Node omits env pairs whose value is undefined, so a forgotten homeDir would + // leave HOME unset in the child. getHomeDir() falls back to os.homedir() and + // the suite would quietly run against the developer's real ~/.think again. + if (!homeDir) { + throw new Error('createHermeticThinkEnv: homeDir is required to keep the suite off a real mind'); + } + return { ...scrubThinkEnv(processEnv), ...base, ...extraEnv, HOME: homeDir, - THINK_UPSTREAM_URL: upstreamUrl, + THINK_UPSTREAM_URL: upstreamUrl ?? '', }; } diff --git a/test/ports/test-isolation.test.js b/test/ports/test-isolation.test.js index 30a4703..e3b8c15 100644 --- a/test/ports/test-isolation.test.js +++ b/test/ports/test-isolation.test.js @@ -93,6 +93,33 @@ test('tests can still set THINK_ variables deliberately through extraEnv', () => assert.equal(env.THINK_TEST_NOW, '1700000000000'); }); +test('a missing homeDir is rejected instead of silently leaking the real home', () => { + // Node drops env pairs whose value is undefined, so a forgotten homeDir would + // leave HOME unset in the child. getHomeDir() then falls back to + // os.homedir(), pointing the suite back at the developer's real ~/.think — + // reopening the hole this fixture exists to close, with no visible error. + assert.throws( + () => createHermeticThinkEnv({ processEnv: {}, upstreamUrl: UPSTREAM_URL }), + /homeDir is required/, + 'Expected a missing homeDir to fail loudly.' + ); + assert.throws( + () => createHermeticThinkEnv({ processEnv: {}, homeDir: '', upstreamUrl: UPSTREAM_URL }), + /homeDir is required/, + 'Expected an empty homeDir to fail loudly.' + ); +}); + +test('a missing upstreamUrl is normalised to empty rather than dropped', () => { + const env = createHermeticThinkEnv({ processEnv: {}, homeDir: HOME_DIR }); + + assert.equal( + env.THINK_UPSTREAM_URL, + '', + 'Expected an explicit empty upstream so an inherited value cannot survive.' + ); +}); + test('baseEnv is applied and does not resurrect scrubbed Think state', () => { const env = buildEnv({ processEnv: { THINK_REPO_DIR: '/Users/dev/.think/claude' } }); From 4c7e10d50ee808a5a057d622c6a53e22a1cf41dd Mon Sep 17 00:00:00 2001 From: James Ross Date: Mon, 3 Aug 2026 13:37:13 -0700 Subject: [PATCH 05/20] feat: make the MCP deferral warning actionable The CLI reports the budget it used in capture.followthrough.deferred, but the MCP warning was a fixed string. An agent receiving it could not tell what the budget was, that the budget is configurable, that the thought is still recallable through remember, or that retrying would duplicate the thought. Resolve the budget through the injected dependency set and name it in the warning, along with the surfaces that will not see the capture yet and the knob that raises the budget. --- src/mcp/service.js | 18 ++++++++++++++---- test/ports/mcp-service.test.js | 21 +++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/src/mcp/service.js b/src/mcp/service.js index 1024c5e..0aec6d3 100644 --- a/src/mcp/service.js +++ b/src/mcp/service.js @@ -1,5 +1,6 @@ import { isDeferredCaptureFollowthrough, + resolveCaptureFollowthroughTimeoutMs, waitForCaptureFollowthrough, } from '../capture-followthrough.js'; import { runDiagnostics } from '../doctor.js'; @@ -43,8 +44,15 @@ import { StatsOutcome, } from './result.js'; -const CAPTURE_FOLLOWTHROUGH_DEFERRED_WARNING = - 'Capture followthrough deferred; raw thought saved locally before derived graph updates completed.'; +function buildCaptureFollowthroughDeferredWarning(timeoutMs) { + return [ + `Capture followthrough deferred after ${String(timeoutMs)}ms;`, + 'raw thought saved locally and still recallable via remember,', + 'but not yet counted by recent or stats.', + 'Do not retry, which would duplicate the thought.', + 'Raise THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS if this recurs.', + ].join(' '); +} const NO_GRAPH_MIGRATION_STATUS = Object.freeze({ currentGraphModelVersion: null, requiredGraphModelVersion: null, @@ -63,6 +71,7 @@ const defaultCaptureDeps = Object.freeze({ graphName: GRAPH_NAME, hasGitRepo, pushWarpRefs, + resolveFollowthroughTimeoutMs: resolveCaptureFollowthroughTimeoutMs, saveRawCapture, waitForFollowthrough: waitForCaptureFollowthrough, }); @@ -105,10 +114,11 @@ function normalizeThoughtText(text) { async function runCaptureFollowthrough(deps, repoDir, entryId, repoAlreadyExists) { const warnings = []; + const timeoutMs = deps.resolveFollowthroughTimeoutMs(); const followthroughPromise = buildCaptureFollowthrough(deps, repoDir, entryId, repoAlreadyExists); try { - const followthrough = await deps.waitForFollowthrough(followthroughPromise); + const followthrough = await deps.waitForFollowthrough(followthroughPromise, { timeoutMs }); if (!isDeferredCaptureFollowthrough(followthrough)) { return { migration: followthrough?.migration ?? null, warnings }; } @@ -118,7 +128,7 @@ async function runCaptureFollowthrough(deps, repoDir, entryId, repoAlreadyExists } followthroughPromise.catch(() => {}); - warnings.push(CAPTURE_FOLLOWTHROUGH_DEFERRED_WARNING); + warnings.push(buildCaptureFollowthroughDeferredWarning(timeoutMs)); return { migration: null, warnings }; } diff --git a/test/ports/mcp-service.test.js b/test/ports/mcp-service.test.js index 640f9d4..a5475ec 100644 --- a/test/ports/mcp-service.test.js +++ b/test/ports/mcp-service.test.js @@ -21,6 +21,7 @@ test('MCP capture returns saved locally when post-save followthrough defers', as 'getCwd', 'getCaptureAmbientContext', 'saveRawCapture', + 'resolveFollowthroughTimeoutMs', 'getGraphModelStatus', 'waitForFollowthrough', 'getUpstreamUrl', @@ -30,8 +31,27 @@ test('MCP capture returns saved locally when post-save followthrough defers', as assert.equal(outcome.structuredContent.backupStatus, 'skipped'); assert.equal(outcome.structuredContent.migration, null); assert.equal(outcome.structuredContent.warnings.length, 1); + assertActionableDeferralWarning(outcome.structuredContent.warnings[0]); }); +function assertActionableDeferralWarning(warning) { + assert.match( + warning, + /\b6000ms\b/, + 'Expected the deferral warning to name the budget it exceeded so an agent can tell it is configurable.' + ); + assert.match( + warning, + /remember/, + 'Expected the warning to say the thought is still recallable, so agents do not retry and duplicate it.' + ); + assert.match( + warning, + /THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS/, + 'Expected the warning to name the knob that raises the budget.' + ); +} + test('MCP migrate_graph returns a no-op when the graph model is current', async () => { const calls = []; const migrateThoughtGraph = createMigrateThoughtGraphService({ @@ -73,6 +93,7 @@ function createDeferredFollowthroughDeps(calls) { graphName: 'think', hasGitRepo: () => record(calls, 'hasGitRepo', true), pushWarpRefs: () => record(calls, 'pushWarpRefs', true), + resolveFollowthroughTimeoutMs: () => record(calls, 'resolveFollowthroughTimeoutMs', 6_000), saveRawCapture: () => record(calls, 'saveRawCapture', { id: 'entry:test-claude' }), waitForFollowthrough: () => record(calls, 'waitForFollowthrough', { status: 'deferred' }), }; From e594e1bc5d6488fb1f0ba28b0df04ed13f74f59d Mon Sep 17 00:00:00 2001 From: think Date: Mon, 3 Aug 2026 13:47:44 -0700 Subject: [PATCH 06/20] fix: stop inherited git location env breaking the pre-push hook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Git exports GIT_DIR and friends to every hook, so the suite launched by scripts/hooks/pre-push inherited them. Tests that shell out to git then resolved the hook's repository instead of their own fixture: `execSync('git init', { cwd: tempDir })` wrote to $GIT_DIR, so the fixture repo was never created and discoverMinds found nothing. The effect was that `npm run test:fast` passed but `git push` failed on six tests, which made the pre-push hook impossible to satisfy — the suite could never gate a push. Unset the repository-location variables in both hooks via a shared scripts/hooks/lib/scrub-git-env.sh, and scrub the same set in createHermeticThinkEnv so spawned Think processes cannot be redirected either. Identity and transport variables are left alone: the product sets its own commit identity and inheriting GIT_SSH_COMMAND is fine. Verified: with GIT_DIR and GIT_WORK_TREE set, the two affected test files go from 7/13 to 13/13. --- scripts/hooks/lib/scrub-git-env.sh | 26 +++++++++++++++++++++++ scripts/hooks/pre-commit | 4 ++++ scripts/hooks/pre-push | 4 ++++ test/fixtures/runtime.js | 30 ++++++++++++++++++++++++++- test/ports/test-isolation.test.js | 33 ++++++++++++++++++++++++++++++ 5 files changed, 96 insertions(+), 1 deletion(-) create mode 100644 scripts/hooks/lib/scrub-git-env.sh diff --git a/scripts/hooks/lib/scrub-git-env.sh b/scripts/hooks/lib/scrub-git-env.sh new file mode 100644 index 0000000..dd76fa0 --- /dev/null +++ b/scripts/hooks/lib/scrub-git-env.sh @@ -0,0 +1,26 @@ +# shellcheck shell=bash +# +# Drop the git environment variables that name *where* a repository lives. +# +# Git exports these to every hook. Any test that shells out to git then resolves +# the hook's repository instead of its own fixture — `git init` in a temp +# directory writes to $GIT_DIR, so the fixture repo is never created and the +# test fails with a confusing "not a git repository". The suite therefore passed +# under `npm test` but failed under `git push`, which made the pre-push hook +# impossible to satisfy. +# +# Identity and transport variables are intentionally left alone: the product +# sets its own commit identity, and transport settings are safe to inherit. +scrub_git_location_env() { + unset GIT_ALTERNATE_OBJECT_DIRECTORIES + unset GIT_CEILING_DIRECTORIES + unset GIT_COMMON_DIR + unset GIT_DIR + unset GIT_INDEX_FILE + unset GIT_INDEX_VERSION + unset GIT_NAMESPACE + unset GIT_OBJECT_DIRECTORY + unset GIT_PREFIX + unset GIT_QUARANTINE_PATH + unset GIT_WORK_TREE +} diff --git a/scripts/hooks/pre-commit b/scripts/hooks/pre-commit index c5b0282..147e0ad 100755 --- a/scripts/hooks/pre-commit +++ b/scripts/hooks/pre-commit @@ -1,6 +1,10 @@ #!/usr/bin/env bash set -euo pipefail +# shellcheck source=scripts/hooks/lib/scrub-git-env.sh +source "$(dirname "${BASH_SOURCE[0]}")/lib/scrub-git-env.sh" +scrub_git_location_env + echo "pre-commit: running lint..." npm run lint echo "pre-commit: lint passed." diff --git a/scripts/hooks/pre-push b/scripts/hooks/pre-push index 9450978..1e95886 100755 --- a/scripts/hooks/pre-push +++ b/scripts/hooks/pre-push @@ -1,6 +1,10 @@ #!/usr/bin/env bash set -euo pipefail +# shellcheck source=scripts/hooks/lib/scrub-git-env.sh +source "$(dirname "${BASH_SOURCE[0]}")/lib/scrub-git-env.sh" +scrub_git_location_env + if [[ "${THINK_PRE_PUSH_FULL:-0}" == "1" ]]; then echo "pre-push: running full local suite..." npm run test:local diff --git a/test/fixtures/runtime.js b/test/fixtures/runtime.js index 644579a..643f218 100644 --- a/test/fixtures/runtime.js +++ b/test/fixtures/runtime.js @@ -18,6 +18,30 @@ export const baseEnv = { export const THINK_ENV_PREFIX = 'THINK_'; +/** + * Git environment variables that name *where* a repository lives. + * + * Git exports these to every hook, so a suite launched from the repo's own + * pre-push hook inherits them and any git call that resolves its repository + * from the environment silently targets the wrong one. Identity and transport + * variables such as GIT_AUTHOR_NAME or GIT_SSH_COMMAND are deliberately not + * listed: the product sets its own identity, and transport settings are safe to + * inherit. + */ +export const GIT_LOCATION_ENV_VARS = Object.freeze([ + 'GIT_ALTERNATE_OBJECT_DIRECTORIES', + 'GIT_CEILING_DIRECTORIES', + 'GIT_COMMON_DIR', + 'GIT_DIR', + 'GIT_INDEX_FILE', + 'GIT_INDEX_VERSION', + 'GIT_NAMESPACE', + 'GIT_OBJECT_DIRECTORY', + 'GIT_PREFIX', + 'GIT_QUARANTINE_PATH', + 'GIT_WORK_TREE', +]); + /** * Build a hermetic environment for a spawned Think process. * @@ -34,8 +58,12 @@ export const THINK_ENV_PREFIX = 'THINK_'; * on after the scrub. */ export function scrubThinkEnv(processEnv = process.env) { + const gitLocation = new Set(GIT_LOCATION_ENV_VARS); + return Object.fromEntries( - Object.entries(processEnv).filter(([key]) => !key.startsWith(THINK_ENV_PREFIX)) + Object.entries(processEnv).filter( + ([key]) => !key.startsWith(THINK_ENV_PREFIX) && !gitLocation.has(key) + ) ); } diff --git a/test/ports/test-isolation.test.js b/test/ports/test-isolation.test.js index e3b8c15..a9cd2ef 100644 --- a/test/ports/test-isolation.test.js +++ b/test/ports/test-isolation.test.js @@ -60,6 +60,39 @@ test('every inherited THINK_ variable is scrubbed, not just the known dangerous ); }); +test('inherited git location variables cannot redirect a spawned Think process', () => { + // Git exports GIT_DIR and friends to every hook, so a suite run from the + // repo's own pre-push hook inherits them. Any git call that resolves its + // repository from the environment then operates on the wrong repository. + const env = createHermeticThinkEnv({ + processEnv: { + GIT_DIR: '/Users/dev/git/think/.git', + GIT_WORK_TREE: '/Users/dev/git/think', + GIT_INDEX_FILE: '/Users/dev/git/think/.git/index', + GIT_OBJECT_DIRECTORY: '/Users/dev/git/think/.git/objects', + GIT_COMMON_DIR: '/Users/dev/git/think/.git', + GIT_QUARANTINE_PATH: '/Users/dev/git/think/.git/quarantine', + }, + homeDir: HOME_DIR, + upstreamUrl: UPSTREAM_URL, + }); + + const leaked = Object.keys(env).filter((key) => key.startsWith('GIT_')); + + assert.deepEqual(leaked, [], 'Expected git repository-location variables to be scrubbed.'); +}); + +test('git identity and transport variables are left alone', () => { + const env = createHermeticThinkEnv({ + processEnv: { GIT_SSH_COMMAND: 'ssh -v', GIT_TERMINAL_PROMPT: '0' }, + homeDir: HOME_DIR, + upstreamUrl: UPSTREAM_URL, + }); + + assert.equal(env.GIT_SSH_COMMAND, 'ssh -v', 'Expected transport configuration to survive.'); + assert.equal(env.GIT_TERMINAL_PROMPT, '0'); +}); + test('non-Think environment such as PATH still reaches the child process', () => { const env = buildEnv({ processEnv: { PATH: '/usr/bin:/bin', HOME: '/Users/dev', SHELL: '/bin/zsh' }, From f96cc26e310542fa5d926c10d6a6fb9b3d2182f9 Mon Sep 17 00:00:00 2001 From: think Date: Mon, 3 Aug 2026 13:59:10 -0700 Subject: [PATCH 07/20] Fix: enforce parity between the shell and JS git-env scrub lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The set of git repository-location variables to scrub is expressed twice — in JavaScript for spawned Think processes and in shell for the hooks — because hooks cannot import the module and the fixtures cannot source the shell function. Nothing forced the two to agree, so adding a variable to one list alone would silently leave the other execution path redirectable. That is the same failure that made the pre-push hook impossible to satisfy, reintroduced as latent drift by e67ea0e. The defect was the missing enforcement rather than wrong code, so the fix is the guard: parse the shell function and assert it unsets exactly the variables GIT_LOCATION_ENV_VARS names, that it lists each once, that no identity or transport variable creeps in, and that both hooks scrub before invoking npm. Verified by deleting `unset GIT_WORK_TREE` from the shell script: the test fails with "js only: GIT_WORK_TREE" and passes once restored. --- test/ports/hook-git-env-parity.test.js | 89 ++++++++++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 test/ports/hook-git-env-parity.test.js diff --git a/test/ports/hook-git-env-parity.test.js b/test/ports/hook-git-env-parity.test.js new file mode 100644 index 0000000..1a8162d --- /dev/null +++ b/test/ports/hook-git-env-parity.test.js @@ -0,0 +1,89 @@ +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { join } from 'node:path'; +import test from 'node:test'; + +import { GIT_LOCATION_ENV_VARS, repoRoot } from '../fixtures/runtime.js'; + +/** + * The set of git repository-location variables to scrub is expressed twice: + * once in JavaScript for spawned Think processes, and once in shell for the + * git hooks. Both exist because hooks cannot import the JS module and the JS + * fixtures cannot source the shell function. + * + * Nothing else forces the two to agree. Adding a variable to one list alone + * silently leaves the other path redirectable — the exact failure that made the + * pre-push hook impossible to satisfy in the first place. This test is the + * enforcement. + */ + +const SCRUB_SCRIPT = join(repoRoot, 'scripts', 'hooks', 'lib', 'scrub-git-env.sh'); + +function readShellScrubbedVars() { + const source = readFileSync(SCRUB_SCRIPT, 'utf8'); + const body = source.slice(source.indexOf('scrub_git_location_env()')); + + return [...body.matchAll(/^\s*unset\s+(GIT_[A-Z_]+)\s*$/gmu)] + .map(([, name]) => name) + .sort(); +} + +test('the shell hook scrubs exactly the git location variables the JS fixtures scrub', () => { + const shellVars = readShellScrubbedVars(); + const jsVars = [...GIT_LOCATION_ENV_VARS].sort(); + + assert.deepEqual( + shellVars, + jsVars, + [ + 'scripts/hooks/lib/scrub-git-env.sh and GIT_LOCATION_ENV_VARS have drifted.', + 'Both must scrub the same variables or one execution path stays redirectable.', + `shell only: ${shellVars.filter((name) => !jsVars.includes(name)).join(', ') || '(none)'}`, + `js only: ${jsVars.filter((name) => !shellVars.includes(name)).join(', ') || '(none)'}`, + ].join('\n') + ); +}); + +test('the shell scrub function unsets each variable exactly once', () => { + const shellVars = readShellScrubbedVars(); + + assert.deepEqual( + shellVars, + [...new Set(shellVars)], + 'Expected no duplicate unset lines, which would hide a typo in a neighbouring name.' + ); +}); + +test('every scrubbed variable actually redirects git rather than configuring identity', () => { + // Identity and transport variables must not creep into this list: the product + // sets its own commit identity, and dropping GIT_SSH_COMMAND would change + // push behaviour rather than isolate a repository. + const forbidden = GIT_LOCATION_ENV_VARS.filter((name) => ( + /AUTHOR|COMMITTER|SSH|TERMINAL|ASKPASS|EDITOR|PAGER|CONFIG/u.test(name) + )); + + assert.deepEqual( + forbidden, + [], + 'Expected only repository-location variables in the scrub list.' + ); +}); + +test('both hooks invoke the shared scrub before running anything', () => { + for (const hook of ['pre-commit', 'pre-push']) { + const source = readFileSync(join(repoRoot, 'scripts', 'hooks', hook), 'utf8'); + + assert.match( + source, + /scrub_git_location_env/u, + `Expected scripts/hooks/${hook} to scrub inherited git location state.` + ); + + const scrubAt = source.indexOf('scrub_git_location_env\n'); + const npmAt = source.indexOf('npm run'); + assert.ok( + scrubAt !== -1 && scrubAt < npmAt, + `Expected scripts/hooks/${hook} to scrub before invoking npm.` + ); + } +}); From 0746c08f09787ed3035cdab1d35150c8748e868e Mon Sep 17 00:00:00 2001 From: think Date: Mon, 3 Aug 2026 13:59:39 -0700 Subject: [PATCH 08/20] Fix: document THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS in the env reference TECHNICAL-TEARDOWN.md enumerates Think's environment tuning and was the canonical list, but it did not mention the followthrough budget this branch adds. Record the default, that one module resolves it for both capture surfaces, what deferral costs per read surface, and why an unusable value falls back rather than failing the capture. GUIDE.md only mentions THINK_UPSTREAM_URL in passing rather than enumerating variables, so it needs no change. --- TECHNICAL-TEARDOWN.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/TECHNICAL-TEARDOWN.md b/TECHNICAL-TEARDOWN.md index c63f80f..8c81637 100644 --- a/TECHNICAL-TEARDOWN.md +++ b/TECHNICAL-TEARDOWN.md @@ -393,6 +393,8 @@ The design is pragmatic: user should rarely be blocked by one transient pipeline `THINK_PROMPT_METRICS_FILE` chooses the metrics output destination, replacing the default `$HOME/.think/metrics/prompt-ux.jsonl` path. +`THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS` bounds the post-capture derived work in milliseconds (default `6000`, resolved by `src/capture-followthrough.js` for both the CLI and MCP surfaces). Exceeding the budget defers the followthrough rather than failing the capture: the raw thought is already committed and remains recallable through `remember`, though `recent` and `stats` will not count it until it is backfilled. An unusable value falls back to the default, because capture is a trapdoor and must not fail on a malformed knob. + `THINK_CAPTURE_INGRESS`, `THINK_CAPTURE_SOURCE_APP`, and `THINK_CAPTURE_SOURCE_URL` seed capture provenance metadata when a capture is created without explicit provenance. Testing and scripted-run controls exist for non-interactive environments: From d6eca1ecd6f41d12c94bbfd6c2e2d8e5bc45812f Mon Sep 17 00:00:00 2001 From: think Date: Mon, 3 Aug 2026 14:15:55 -0700 Subject: [PATCH 09/20] Fix: clamp the followthrough budget to the 32-bit timer range MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit setTimeout stores its delay in a 32-bit signed integer. Node substitutes 1ms for anything larger and emits TimeoutOverflowWarning, so a budget of 2_147_483_648 — which passed Number.isSafeInteger and the resolver — made capture defer immediately, the exact opposite of asking for a long budget. Clamp to MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS in both the resolver and waitForCaptureFollowthrough, since callers can supply a budget without going through the resolver. Clamping rather than defaulting keeps the direction the operator asked for instead of silently dropping to 6s. Reported by CodeRabbit. Verified: an over-range budget no longer warns and no longer defers immediately. --- src/capture-followthrough.js | 18 +++++++++++-- test/ports/capture-followthrough.test.js | 33 ++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/capture-followthrough.js b/src/capture-followthrough.js index 179e155..4d8b600 100644 --- a/src/capture-followthrough.js +++ b/src/capture-followthrough.js @@ -13,6 +13,17 @@ export const DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS = 6_000; +/** + * The largest delay `setTimeout` can hold. + * + * Node stores the delay in a 32-bit signed integer and substitutes 1ms for + * anything larger, with a TimeoutOverflowWarning. Left unclamped, asking for a + * very large budget would produce immediate deferral — the opposite of intent — + * so an over-range request is clamped to the maximum rather than defaulted, + * which honours the direction the operator asked for. + */ +export const MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS = 2_147_483_647; + export const CAPTURE_FOLLOWTHROUGH_DEFERRED = Object.freeze({ status: 'deferred' }); /** @@ -32,7 +43,7 @@ export function resolveCaptureFollowthroughTimeoutMs(environment = process.env) return DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS; } - return parsed; + return Math.min(parsed, MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS); } export function isDeferredCaptureFollowthrough(followthrough) { @@ -46,7 +57,10 @@ export function isDeferredCaptureFollowthrough(followthrough) { * that keep the abandoned promise alive must attach their own catch handler. */ export async function waitForCaptureFollowthrough(followthroughPromise, { timeoutMs } = {}) { - const budgetMs = timeoutMs ?? resolveCaptureFollowthroughTimeoutMs(); + const budgetMs = Math.min( + timeoutMs ?? resolveCaptureFollowthroughTimeoutMs(), + MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS + ); let timeoutId = null; const timeout = new Promise((resolve) => { timeoutId = setTimeout(() => resolve(CAPTURE_FOLLOWTHROUGH_DEFERRED), budgetMs); diff --git a/test/ports/capture-followthrough.test.js b/test/ports/capture-followthrough.test.js index 20fb839..3d95fe8 100644 --- a/test/ports/capture-followthrough.test.js +++ b/test/ports/capture-followthrough.test.js @@ -5,6 +5,7 @@ import { CAPTURE_FOLLOWTHROUGH_DEFERRED, DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS, isDeferredCaptureFollowthrough, + MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS, resolveCaptureFollowthroughTimeoutMs, waitForCaptureFollowthrough, } from '../../src/capture-followthrough.js'; @@ -37,6 +38,38 @@ test('an unusable followthrough budget falls back to the default instead of brea } }); +test('a budget beyond the 32-bit timer range is clamped, not silently turned into 1ms', () => { + // setTimeout stores its delay in a 32-bit signed integer. Node warns and + // substitutes 1ms for anything larger, so an operator asking for a very large + // budget would get immediate deferral — the exact opposite of the request. + assert.equal( + resolveCaptureFollowthroughTimeoutMs({ THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS: '2147483648' }), + MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS + ); + assert.equal( + resolveCaptureFollowthroughTimeoutMs({ THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS: '99999999999' }), + MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS + ); +}); + +test('the largest in-range budget is preserved exactly', () => { + assert.equal(MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS, 2_147_483_647); + assert.equal( + resolveCaptureFollowthroughTimeoutMs({ THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS: '2147483647' }), + 2_147_483_647 + ); +}); + +test('waitForCaptureFollowthrough clamps an over-range budget it is handed directly', async () => { + // The exported helper must not overflow either, since callers can pass a + // budget without going through the resolver. + const settled = await waitForCaptureFollowthrough(Promise.resolve({ migration: null }), { + timeoutMs: Number.MAX_SAFE_INTEGER, + }); + + assert.deepEqual(settled, { migration: null }, 'Expected no immediate deferral from an over-range budget.'); +}); + test('waitForCaptureFollowthrough returns the settled followthrough when it beats the budget', async () => { const result = await waitForCaptureFollowthrough(Promise.resolve({ migration: null }), { timeoutMs: 10_000, From 741a4e2d9dbda13b843ca6f18e4bafe2ff4f7ab8 Mon Sep 17 00:00:00 2001 From: think Date: Mon, 3 Aug 2026 14:33:25 -0700 Subject: [PATCH 10/20] Fix: spend one followthrough budget across the CLI's sequential waits MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit runCaptureFollowthrough awaits twice — the graph-model probe, then the finalize — and handed each the full budget. A slow capture could therefore spend up to twice what the operator configured, which defeats the point of bounding it at all. Measured before: 3506ms total against a 2000ms budget. After: 2835ms for 2000ms and 1641ms for 1000ms, i.e. one budget plus process overhead rather than two budgets. Introduce createCaptureFollowthroughDeadline and pass remainingMs() to each wait, so the budget means total elapsed followthrough time. The deferred event still reports the configured budget rather than whatever remained, since that is the number the operator set. Reported by CodeRabbit. --- src/capture-followthrough.js | 19 +++++++++++++++ src/cli/commands/capture.js | 16 +++++++++---- test/ports/capture-followthrough.test.js | 30 ++++++++++++++++++++++++ 3 files changed, 60 insertions(+), 5 deletions(-) diff --git a/src/capture-followthrough.js b/src/capture-followthrough.js index 4d8b600..fc2628a 100644 --- a/src/capture-followthrough.js +++ b/src/capture-followthrough.js @@ -46,6 +46,25 @@ export function resolveCaptureFollowthroughTimeoutMs(environment = process.env) return Math.min(parsed, MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS); } +/** + * One budget shared across sequential followthrough waits. + * + * The CLI awaits twice — the graph-model probe, then the finalize. Handing each + * await the full budget let a slow capture spend up to twice what the operator + * configured. A deadline makes the budget mean total elapsed time, which is what + * "capture must not hang" actually requires. + */ +export function createCaptureFollowthroughDeadline(budgetMs, now = Date.now) { + const startedAt = now(); + const clamped = Math.min(budgetMs, MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS); + + return Object.freeze({ + budgetMs: clamped, + remainingMs: () => Math.max(0, clamped - (now() - startedAt)), + expired: () => now() - startedAt >= clamped, + }); +} + export function isDeferredCaptureFollowthrough(followthrough) { return followthrough?.status === CAPTURE_FOLLOWTHROUGH_DEFERRED.status; } diff --git a/src/cli/commands/capture.js b/src/cli/commands/capture.js index 0bd551b..9000bd3 100644 --- a/src/cli/commands/capture.js +++ b/src/cli/commands/capture.js @@ -1,5 +1,6 @@ import { ensureGitRepo, hasGitRepo, pushWarpRefs } from '../../git.js'; import { + createCaptureFollowthroughDeadline, isDeferredCaptureFollowthrough, resolveCaptureFollowthroughTimeoutMs, waitForCaptureFollowthrough, @@ -56,20 +57,23 @@ export async function runCapture(thought, output, reporter) { } async function runCaptureFollowthrough(repoDir, entryId, repoAlreadyExists, reporter) { - const timeoutMs = resolveCaptureFollowthroughTimeoutMs(); + // One deadline for the whole followthrough, not one budget per await. + const deadline = createCaptureFollowthroughDeadline(resolveCaptureFollowthroughTimeoutMs()); try { const graphStatusPromise = repoAlreadyExists ? getGraphModelStatus(repoDir) : Promise.resolve(NO_GRAPH_MIGRATION_STATUS); - const graphStatus = await waitForCaptureFollowthrough(graphStatusPromise, { timeoutMs }); + const graphStatus = await waitForCaptureFollowthrough(graphStatusPromise, { + timeoutMs: deadline.remainingMs(), + }); if (isDeferredCaptureFollowthrough(graphStatus)) { graphStatusPromise.catch(() => {}); reporter.event('capture.followthrough.deferred', { command: 'capture', trigger: 'post_capture', entryId, - timeoutMs, + timeoutMs: deadline.budgetMs, }); return; } @@ -88,14 +92,16 @@ async function runCaptureFollowthrough(repoDir, entryId, repoAlreadyExists, repo migrateIfNeeded: false, ambientContext: getAmbientProjectContext(process.cwd()), }); - const followthrough = await waitForCaptureFollowthrough(followthroughPromise, { timeoutMs }); + const followthrough = await waitForCaptureFollowthrough(followthroughPromise, { + timeoutMs: deadline.remainingMs(), + }); if (isDeferredCaptureFollowthrough(followthrough)) { followthroughPromise.catch(() => {}); reporter.event('capture.followthrough.deferred', { command: 'capture', trigger: 'post_capture', entryId, - timeoutMs, + timeoutMs: deadline.budgetMs, }); } } catch (error) { diff --git a/test/ports/capture-followthrough.test.js b/test/ports/capture-followthrough.test.js index 3d95fe8..3874536 100644 --- a/test/ports/capture-followthrough.test.js +++ b/test/ports/capture-followthrough.test.js @@ -3,6 +3,7 @@ import test from 'node:test'; import { CAPTURE_FOLLOWTHROUGH_DEFERRED, + createCaptureFollowthroughDeadline, DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS, isDeferredCaptureFollowthrough, MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS, @@ -95,6 +96,35 @@ test('waitForCaptureFollowthrough propagates followthrough rejections to the cal ); }); +test('a followthrough deadline spends one budget across sequential waits', () => { + // The CLI awaits twice: the graph-model probe, then the finalize. Handing each + // the full budget lets a slow capture spend up to 2x what the operator + // configured, so the budget has to be a shared deadline rather than a + // per-await allowance. + const clock = { now: 1_000 }; + const deadline = createCaptureFollowthroughDeadline(500, () => clock.now); + + assert.equal(deadline.remainingMs(), 500); + + clock.now = 1_200; + assert.equal(deadline.remainingMs(), 300, 'Expected elapsed time to be deducted.'); + + clock.now = 1_500; + assert.equal(deadline.remainingMs(), 0, 'Expected an exhausted deadline to report zero.'); + + clock.now = 9_999; + assert.equal(deadline.remainingMs(), 0, 'Expected an overrun deadline never to go negative.'); +}); + +test('an exhausted deadline reports itself as expired', () => { + const clock = { now: 0 }; + const deadline = createCaptureFollowthroughDeadline(100, () => clock.now); + + assert.equal(deadline.expired(), false); + clock.now = 100; + assert.equal(deadline.expired(), true); +}); + test('isDeferredCaptureFollowthrough matches the sentinel by status, not identity', () => { assert.equal(isDeferredCaptureFollowthrough({ status: 'deferred' }), true); assert.equal(isDeferredCaptureFollowthrough({ status: 'done' }), false); From 3215b147be12643bf43df10506cb34f9b126d04b Mon Sep 17 00:00:00 2001 From: think Date: Mon, 3 Aug 2026 14:35:44 -0700 Subject: [PATCH 11/20] Fix: derive the git scrub set from git instead of hand-maintaining it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parity guard added in 07c3eb3 asserted the shell and JavaScript lists agreed. They did — and both were missing eight variables that git reports as repository-local: GIT_CONFIG, GIT_CONFIG_COUNT, GIT_CONFIG_PARAMETERS, GIT_GRAFT_FILE, GIT_IMPLICIT_WORK_TREE, GIT_NO_REPLACE_OBJECTS, GIT_REPLACE_REF_BASE and GIT_SHALLOW_FILE. Agreement between two incomplete lists proves nothing, which is precisely the critique. Query `git rev-parse --local-env-vars` in both paths so the set tracks the installed git, and keep only the four repository-scoping variables absent from that list as a hand-maintained extra. Coverage went from 11 to 19. Rewrite the guard to assert completeness against git's own answer rather than mutual agreement, keep the extras in sync, and keep asserting that no identity or transport variable is dropped. Reported by CodeRabbit against my own guard. Verified: GIT_CONFIG and GIT_CONFIG_COUNT are now cleared by the shell helper. --- scripts/hooks/lib/scrub-git-env.sh | 33 ++++++---- test/fixtures/runtime.js | 58 +++++++++-------- test/ports/hook-git-env-parity.test.js | 90 +++++++++++++++----------- 3 files changed, 102 insertions(+), 79 deletions(-) diff --git a/scripts/hooks/lib/scrub-git-env.sh b/scripts/hooks/lib/scrub-git-env.sh index dd76fa0..e875532 100644 --- a/scripts/hooks/lib/scrub-git-env.sh +++ b/scripts/hooks/lib/scrub-git-env.sh @@ -1,6 +1,7 @@ # shellcheck shell=bash # -# Drop the git environment variables that name *where* a repository lives. +# Drop the git environment variables that bind a process to a specific +# repository. # # Git exports these to every hook. Any test that shells out to git then resolves # the hook's repository instead of its own fixture — `git init` in a temp @@ -9,18 +10,22 @@ # under `npm test` but failed under `git push`, which made the pre-push hook # impossible to satisfy. # -# Identity and transport variables are intentionally left alone: the product -# sets its own commit identity, and transport settings are safe to inherit. +# The authoritative set is whatever `git rev-parse --local-env-vars` reports for +# the installed git, so it is queried rather than hand-maintained — an earlier +# hand-written list silently omitted GIT_CONFIG, GIT_CONFIG_COUNT, +# GIT_CONFIG_PARAMETERS, GIT_GRAFT_FILE, GIT_IMPLICIT_WORK_TREE, +# GIT_NO_REPLACE_OBJECTS, GIT_REPLACE_REF_BASE and GIT_SHALLOW_FILE. +# +# A few repository-scoping variables are not in that list but still redirect +# lookups, so they are unset explicitly as well. Identity and transport +# variables such as GIT_AUTHOR_NAME or GIT_SSH_COMMAND are deliberately left +# alone: the product sets its own commit identity, and transport settings are +# safe to inherit. +GIT_EXTRA_SCRUBBED_ENV_VARS="GIT_CEILING_DIRECTORIES GIT_INDEX_VERSION GIT_NAMESPACE GIT_QUARANTINE_PATH" + scrub_git_location_env() { - unset GIT_ALTERNATE_OBJECT_DIRECTORIES - unset GIT_CEILING_DIRECTORIES - unset GIT_COMMON_DIR - unset GIT_DIR - unset GIT_INDEX_FILE - unset GIT_INDEX_VERSION - unset GIT_NAMESPACE - unset GIT_OBJECT_DIRECTORY - unset GIT_PREFIX - unset GIT_QUARANTINE_PATH - unset GIT_WORK_TREE + local name + for name in $(git rev-parse --local-env-vars 2>/dev/null) ${GIT_EXTRA_SCRUBBED_ENV_VARS}; do + unset "${name}" + done } diff --git a/test/fixtures/runtime.js b/test/fixtures/runtime.js index 643f218..212db09 100644 --- a/test/fixtures/runtime.js +++ b/test/fixtures/runtime.js @@ -1,4 +1,5 @@ import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; import { existsSync } from 'node:fs'; import path from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -19,44 +20,47 @@ export const baseEnv = { export const THINK_ENV_PREFIX = 'THINK_'; /** - * Git environment variables that name *where* a repository lives. - * - * Git exports these to every hook, so a suite launched from the repo's own - * pre-push hook inherits them and any git call that resolves its repository - * from the environment silently targets the wrong one. Identity and transport - * variables such as GIT_AUTHOR_NAME or GIT_SSH_COMMAND are deliberately not - * listed: the product sets its own identity, and transport settings are safe to - * inherit. + * Repository-scoping git variables that are not in `git rev-parse + * --local-env-vars` but still redirect lookups. */ -export const GIT_LOCATION_ENV_VARS = Object.freeze([ - 'GIT_ALTERNATE_OBJECT_DIRECTORIES', +const GIT_EXTRA_SCRUBBED_ENV_VARS = Object.freeze([ 'GIT_CEILING_DIRECTORIES', - 'GIT_COMMON_DIR', - 'GIT_DIR', - 'GIT_INDEX_FILE', 'GIT_INDEX_VERSION', 'GIT_NAMESPACE', - 'GIT_OBJECT_DIRECTORY', - 'GIT_PREFIX', 'GIT_QUARANTINE_PATH', - 'GIT_WORK_TREE', ]); /** - * Build a hermetic environment for a spawned Think process. + * Git environment variables that bind a process to a specific repository. + * + * Git exports these to every hook, so a suite launched from the repo's own + * pre-push hook inherits them and any git call that resolves its repository + * from the environment silently targets the wrong one. * - * Pinning HOME is not enough on its own. `getLocalRepoDir` honours - * THINK_REPO_DIR ahead of $HOME/.think/repo, and `getPromptMetricsFile` and - * `captureProvenanceFromEnvironment` read their own THINK_ variables, so an - * inherited environment can redirect the suite at a developer's real mind and - * write test captures into it. + * The authoritative set comes from `git rev-parse --local-env-vars` rather than + * a hand-maintained list. An earlier hand-written list agreed with its shell + * counterpart yet silently omitted GIT_CONFIG, GIT_CONFIG_COUNT, + * GIT_CONFIG_PARAMETERS, GIT_GRAFT_FILE, GIT_IMPLICIT_WORK_TREE, + * GIT_NO_REPLACE_OBJECTS, GIT_REPLACE_REF_BASE and GIT_SHALLOW_FILE — agreement + * between two incomplete lists proves nothing. * - * Every inherited THINK_ variable is therefore dropped. The scrub is - * prefix-wide rather than an allowlist so a newly added knob cannot quietly - * reopen the hole. Fixtures and individual tests can still set THINK_ - * variables deliberately through `baseEnv` and `extraEnv`, which are layered - * on after the scrub. + * Identity and transport variables are deliberately excluded: the product sets + * its own commit identity, and inheriting GIT_SSH_COMMAND is safe. */ +export const GIT_LOCATION_ENV_VARS = Object.freeze([...new Set([ + ...readGitLocalEnvVars(), + ...GIT_EXTRA_SCRUBBED_ENV_VARS, +])].sort()); + +function readGitLocalEnvVars() { + const result = spawnSync('git', ['rev-parse', '--local-env-vars'], { encoding: 'utf8' }); + if (result.status !== 0) { + return []; + } + + return String(result.stdout).split('\n').map((line) => line.trim()).filter(Boolean); +} + export function scrubThinkEnv(processEnv = process.env) { const gitLocation = new Set(GIT_LOCATION_ENV_VARS); diff --git a/test/ports/hook-git-env-parity.test.js b/test/ports/hook-git-env-parity.test.js index 1a8162d..12f1c0a 100644 --- a/test/ports/hook-git-env-parity.test.js +++ b/test/ports/hook-git-env-parity.test.js @@ -1,4 +1,5 @@ import assert from 'node:assert/strict'; +import { spawnSync } from 'node:child_process'; import { readFileSync } from 'node:fs'; import { join } from 'node:path'; import test from 'node:test'; @@ -6,67 +7,80 @@ import test from 'node:test'; import { GIT_LOCATION_ENV_VARS, repoRoot } from '../fixtures/runtime.js'; /** - * The set of git repository-location variables to scrub is expressed twice: - * once in JavaScript for spawned Think processes, and once in shell for the - * git hooks. Both exist because hooks cannot import the JS module and the JS - * fixtures cannot source the shell function. + * The git variables to scrub are applied in two places: JavaScript for spawned + * Think processes, and shell for the git hooks. Both exist because hooks cannot + * import the module and the fixtures cannot source the shell function. * - * Nothing else forces the two to agree. Adding a variable to one list alone - * silently leaves the other path redirectable — the exact failure that made the - * pre-push hook impossible to satisfy in the first place. This test is the - * enforcement. + * An earlier version of this file only asserted the two hand-maintained lists + * agreed with each other. They did — and both were missing eight variables + * including GIT_CONFIG. Agreement between two incomplete lists proves nothing, + * so these tests assert *completeness* against `git rev-parse --local-env-vars`, + * which is what git itself considers repository-local. */ const SCRUB_SCRIPT = join(repoRoot, 'scripts', 'hooks', 'lib', 'scrub-git-env.sh'); -function readShellScrubbedVars() { - const source = readFileSync(SCRUB_SCRIPT, 'utf8'); - const body = source.slice(source.indexOf('scrub_git_location_env()')); +function gitLocalEnvVars() { + const result = spawnSync('git', ['rev-parse', '--local-env-vars'], { encoding: 'utf8' }); + assert.equal(result.status, 0, 'Expected git rev-parse --local-env-vars to succeed.'); - return [...body.matchAll(/^\s*unset\s+(GIT_[A-Z_]+)\s*$/gmu)] - .map(([, name]) => name) - .sort(); + return String(result.stdout).split('\n').map((line) => line.trim()).filter(Boolean).sort(); } -test('the shell hook scrubs exactly the git location variables the JS fixtures scrub', () => { - const shellVars = readShellScrubbedVars(); - const jsVars = [...GIT_LOCATION_ENV_VARS].sort(); +test('the JavaScript scrub list covers everything git calls repository-local', () => { + const authoritative = gitLocalEnvVars(); + const covered = new Set(GIT_LOCATION_ENV_VARS); + const missing = authoritative.filter((name) => !covered.has(name)); assert.deepEqual( - shellVars, - jsVars, + missing, + [], [ - 'scripts/hooks/lib/scrub-git-env.sh and GIT_LOCATION_ENV_VARS have drifted.', - 'Both must scrub the same variables or one execution path stays redirectable.', - `shell only: ${shellVars.filter((name) => !jsVars.includes(name)).join(', ') || '(none)'}`, - `js only: ${jsVars.filter((name) => !shellVars.includes(name)).join(', ') || '(none)'}`, + 'GIT_LOCATION_ENV_VARS is missing variables git reports as repository-local.', + 'A spawned Think process could still resolve the invoking repository through them.', + `missing: ${missing.join(', ')}`, ].join('\n') ); }); -test('the shell scrub function unsets each variable exactly once', () => { - const shellVars = readShellScrubbedVars(); +test('the shell hook derives its scrub set from git rather than hand-maintaining it', () => { + const source = readFileSync(SCRUB_SCRIPT, 'utf8'); + + assert.match( + source, + /git rev-parse --local-env-vars/u, + 'Expected the hook to query git for the authoritative set, so it cannot drift as git adds variables.' + ); + assert.match(source, /unset "\$\{name\}"/u, 'Expected the queried names to actually be unset.'); +}); + +test('both scrub paths cover the same extra repository-scoping variables', () => { + // A few variables scope lookups but are absent from --local-env-vars, so they + // are still listed by hand. Those hand-maintained additions must agree. + const source = readFileSync(SCRUB_SCRIPT, 'utf8'); + const declared = /GIT_EXTRA_SCRUBBED_ENV_VARS="([^"]+)"/u.exec(source); + + assert.ok(declared, 'Expected the shell helper to declare its extra variables in one place.'); + + const shellExtras = declared[1].trim().split(/\s+/u).sort(); + const authoritative = new Set(gitLocalEnvVars()); + const jsExtras = GIT_LOCATION_ENV_VARS.filter((name) => !authoritative.has(name)).sort(); assert.deepEqual( - shellVars, - [...new Set(shellVars)], - 'Expected no duplicate unset lines, which would hide a typo in a neighbouring name.' + shellExtras, + jsExtras, + 'Expected the hand-maintained extras to match between shell and JavaScript.' ); }); -test('every scrubbed variable actually redirects git rather than configuring identity', () => { - // Identity and transport variables must not creep into this list: the product - // sets its own commit identity, and dropping GIT_SSH_COMMAND would change - // push behaviour rather than isolate a repository. +test('no identity or transport variable is scrubbed', () => { + // Dropping GIT_SSH_COMMAND or GIT_AUTHOR_NAME would change push and commit + // behaviour rather than isolate a repository. const forbidden = GIT_LOCATION_ENV_VARS.filter((name) => ( - /AUTHOR|COMMITTER|SSH|TERMINAL|ASKPASS|EDITOR|PAGER|CONFIG/u.test(name) + /AUTHOR|COMMITTER|SSH|TERMINAL|ASKPASS|EDITOR|PAGER/u.test(name) )); - assert.deepEqual( - forbidden, - [], - 'Expected only repository-location variables in the scrub list.' - ); + assert.deepEqual(forbidden, [], 'Expected only repository-scoping variables in the scrub set.'); }); test('both hooks invoke the shared scrub before running anything', () => { From 14f5b4c8de6e002701bef51cf9f8656fb1763d1d Mon Sep 17 00:00:00 2001 From: think Date: Mon, 3 Aug 2026 14:47:58 -0700 Subject: [PATCH 12/20] Fix: scrub inherited Think and git variables case-insensitively MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Windows environment lookup is case-insensitive, so an inherited `Think_Repo_Dir` passed the case-sensitive filter yet was still returned by process.env.THINK_REPO_DIR in the spawned child. The acceptance suite could therefore be redirected into a developer's real mind on Windows even after the scrub was added — the same data-loss hole, reachable through casing alone. Normalise keys to uppercase before testing both the THINK_ prefix and the git location set. Reported by Codex. --- test/fixtures/runtime.js | 15 ++++++++++++--- test/ports/test-isolation.test.js | 27 +++++++++++++++++++++++++++ 2 files changed, 39 insertions(+), 3 deletions(-) diff --git a/test/fixtures/runtime.js b/test/fixtures/runtime.js index 212db09..717bff1 100644 --- a/test/fixtures/runtime.js +++ b/test/fixtures/runtime.js @@ -61,13 +61,22 @@ function readGitLocalEnvVars() { return String(result.stdout).split('\n').map((line) => line.trim()).filter(Boolean); } +/** + * Drop inherited Think and git repository-scoping variables. + * + * Comparison is case-insensitive because Windows environment lookup is: an + * inherited `Think_Repo_Dir` would pass a case-sensitive filter yet still be + * returned by `process.env.THINK_REPO_DIR` in the spawned child, redirecting the + * suite at a developer's real mind again. + */ export function scrubThinkEnv(processEnv = process.env) { const gitLocation = new Set(GIT_LOCATION_ENV_VARS); return Object.fromEntries( - Object.entries(processEnv).filter( - ([key]) => !key.startsWith(THINK_ENV_PREFIX) && !gitLocation.has(key) - ) + Object.entries(processEnv).filter(([key]) => { + const normalized = key.toUpperCase(); + return !normalized.startsWith(THINK_ENV_PREFIX) && !gitLocation.has(normalized); + }) ); } diff --git a/test/ports/test-isolation.test.js b/test/ports/test-isolation.test.js index a9cd2ef..dda1b4b 100644 --- a/test/ports/test-isolation.test.js +++ b/test/ports/test-isolation.test.js @@ -93,6 +93,33 @@ test('git identity and transport variables are left alone', () => { assert.equal(env.GIT_TERMINAL_PROMPT, '0'); }); +test('mixed-case Think variables are scrubbed, because Windows env lookup is case-insensitive', () => { + // On Windows an inherited `Think_Repo_Dir` passes a case-sensitive filter, yet + // the spawned child still resolves it through process.env.THINK_REPO_DIR — so + // the suite could be redirected into a developer's real mind again. + const env = buildEnv({ + processEnv: { + Think_Repo_Dir: '/Users/dev/.think/claude', + think_prompt_metrics_file: '/Users/dev/.think/metrics/prompt-ux.jsonl', + ThInK_CaPtUrE_iNgReSs: 'share', + }, + }); + + const leaked = Object.keys(env).filter((key) => ( + key.toUpperCase().startsWith('THINK_') && key.toUpperCase() !== 'THINK_UPSTREAM_URL' + )); + + assert.deepEqual(leaked, [], 'Expected mixed-case Think variables to be scrubbed too.'); +}); + +test('mixed-case git location variables are scrubbed as well', () => { + const env = buildEnv({ processEnv: { Git_Dir: '/Users/dev/git/think/.git', git_work_tree: '/Users/dev/git/think' } }); + + const leaked = Object.keys(env).filter((key) => key.toUpperCase().startsWith('GIT_')); + + assert.deepEqual(leaked, [], 'Expected mixed-case git location variables to be scrubbed.'); +}); + test('non-Think environment such as PATH still reaches the child process', () => { const env = buildEnv({ processEnv: { PATH: '/usr/bin:/bin', HOME: '/Users/dev', SHELL: '/bin/zsh' }, From 465306eaaba7cba1d92b22fb794295ef8046d778 Mon Sep 17 00:00:00 2001 From: think Date: Mon, 3 Aug 2026 15:09:26 -0700 Subject: [PATCH 13/20] Fix: stop the followthrough deferral being skipped by process exit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The deferral timer was unref'd, inherited from the duplicated implementations this branch consolidated. An unref'd timer does not hold the event loop, so a process with nothing else pending exits instead of deferring — the budget stopped being a guarantee and the caller's await never settled. Caught by CI, which the local fast suite could not surface: node:test took the whole file down as cancelledByParent, six tests reported not ok while `fail 0` and `cancelled 6`, and the runner exited 1. Reproduced directly as exit code 13, "unsettled top-level await", in a bare process; adding any ref'd timer made the same call resolve correctly. Remove the unref. clearTimeout in the finally already guarantees the timer never outlives the race, so unref bought nothing but the early-exit hazard. Measured no timing change in real captures — 4717/6815/6822ms before versus 4868/6792/6831ms after over three captures — because the git child processes hold the loop there regardless. The regression test asserts the deferral in a fresh child process, which has nothing else pending by construction and therefore fails deterministically rather than under load. --- src/capture-followthrough.js | 5 ++++- test/ports/capture-followthrough.test.js | 25 ++++++++++++++++++++++++ 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/src/capture-followthrough.js b/src/capture-followthrough.js index fc2628a..de04f3c 100644 --- a/src/capture-followthrough.js +++ b/src/capture-followthrough.js @@ -82,8 +82,11 @@ export async function waitForCaptureFollowthrough(followthroughPromise, { timeou ); let timeoutId = null; const timeout = new Promise((resolve) => { + // Deliberately not unref'd. An unref'd timer does not hold the event loop, so + // a process with nothing else pending exits instead of deferring and the + // budget stops being a guarantee. clearTimeout below already ensures the + // timer never outlives the race, so there is nothing left for unref to buy. timeoutId = setTimeout(() => resolve(CAPTURE_FOLLOWTHROUGH_DEFERRED), budgetMs); - timeoutId.unref?.(); }); try { diff --git a/test/ports/capture-followthrough.test.js b/test/ports/capture-followthrough.test.js index 3874536..f9d62a3 100644 --- a/test/ports/capture-followthrough.test.js +++ b/test/ports/capture-followthrough.test.js @@ -1,4 +1,5 @@ import assert from 'node:assert/strict'; +import { execFileSync } from 'node:child_process'; import test from 'node:test'; import { @@ -89,6 +90,30 @@ test('waitForCaptureFollowthrough defers once the budget elapses', async () => { assert.equal(isDeferredCaptureFollowthrough(result), true); }); +test('the deferral timer keeps the process alive long enough to actually fire', () => { + // An unref'd timer does not hold the event loop, so a process with nothing else + // pending exits *instead* of deferring — the timeout stops being a guarantee. + // Locally other work masked it; CI exited with code 13 and took six tests down + // as cancelledByParent. Assert it in a fresh child, which has nothing else + // pending by construction. + const script = [ + "import { waitForCaptureFollowthrough } from './src/capture-followthrough.js';", + 'const result = await waitForCaptureFollowthrough(new Promise(() => {}), { timeoutMs: 25 });', + 'process.stdout.write(JSON.stringify(result));', + ].join('\n'); + + const stdout = execFileSync(process.execPath, ['--input-type=module', '--eval', script], { + cwd: new URL('../..', import.meta.url), + encoding: 'utf8', + }); + + assert.equal( + stdout.trim(), + '{"status":"deferred"}', + 'Expected the child to defer and exit cleanly rather than dying with an unsettled await.' + ); +}); + test('waitForCaptureFollowthrough propagates followthrough rejections to the caller', async () => { await assert.rejects( waitForCaptureFollowthrough(Promise.reject(new Error('graph write failed')), { timeoutMs: 10_000 }), From fff6f4fa1b7c6ff7786b542360b5485a85f426ed Mon Sep 17 00:00:00 2001 From: think Date: Mon, 3 Aug 2026 15:40:42 -0700 Subject: [PATCH 14/20] Fix: correct what a deferred capture actually costs, and log the real defect MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I documented, as verified fact, that a deferred capture stays reachable through `remember` while `recent` and `stats` under-report it. That was drawn from a single unrepeated observation and it is wrong in both directions. Eight controlled trials say: - `remember` does NOT find it — 0 matches. - `recent`/`stats` do not merely under-report; immediately after a deferral they report ONLY the deferred entry (1 instead of 4 in a four-capture mind), hiding every older capture. - Once a later healthy capture rebuilds the read model, the deferred entry is dropped from both surfaces permanently. - What actually survives is the raw layer: `inspect` returns exact text and identity. What is skipped is the derived layer — `canonicalThought.stored` false, `sessionAttribution` and `seedQuality` null. Confirmed pre-existing on origin/main by forcing its hardcoded budget to 1ms: same recent=1, stats=1. Exposing the budget as an env knob only made the state easy to reach on purpose. Logged with the full reproduction as a bad-code item rather than fixed here, since the read-model change is product work well outside this branch. Correct README, TECHNICAL-TEARDOWN and the MCP deferral warning to the verified behaviour, and update the two ports tests that asserted the old wording. --- TECHNICAL-TEARDOWN.md | 2 +- ...-capture-corrupts-the-recent-read-model.md | 78 +++++++++++++++++++ src/mcp/service.js | 4 +- test/acceptance/mcp.test.js | 12 +-- test/fixtures/runtime.js | 14 ++++ test/ports/mcp-service.test.js | 9 ++- test/ports/test-isolation.test.js | 14 +++- 7 files changed, 115 insertions(+), 18 deletions(-) create mode 100644 docs/method/backlog/bad-code/CORE_deferred-capture-corrupts-the-recent-read-model.md diff --git a/TECHNICAL-TEARDOWN.md b/TECHNICAL-TEARDOWN.md index 8c81637..3205a77 100644 --- a/TECHNICAL-TEARDOWN.md +++ b/TECHNICAL-TEARDOWN.md @@ -393,7 +393,7 @@ The design is pragmatic: user should rarely be blocked by one transient pipeline `THINK_PROMPT_METRICS_FILE` chooses the metrics output destination, replacing the default `$HOME/.think/metrics/prompt-ux.jsonl` path. -`THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS` bounds the post-capture derived work in milliseconds (default `6000`, resolved by `src/capture-followthrough.js` for both the CLI and MCP surfaces). Exceeding the budget defers the followthrough rather than failing the capture: the raw thought is already committed and remains recallable through `remember`, though `recent` and `stats` will not count it until it is backfilled. An unusable value falls back to the default, because capture is a trapdoor and must not fail on a malformed knob. +`THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS` bounds the post-capture derived work in milliseconds (default `6000`, resolved by `src/capture-followthrough.js` for both the CLI and MCP surfaces). Exceeding the budget defers the followthrough rather than failing the capture: the raw thought is already committed and stays readable through `inspect`, but the derived layer is skipped (`canonicalThought.stored` false, `sessionAttribution` and `seedQuality` null) and the `recent`/`stats` read model misreports until the next healthy capture rebuilds it. An unusable value falls back to the default, because capture is a trapdoor and must not fail on a malformed knob. `THINK_CAPTURE_INGRESS`, `THINK_CAPTURE_SOURCE_APP`, and `THINK_CAPTURE_SOURCE_URL` seed capture provenance metadata when a capture is created without explicit provenance. diff --git a/docs/method/backlog/bad-code/CORE_deferred-capture-corrupts-the-recent-read-model.md b/docs/method/backlog/bad-code/CORE_deferred-capture-corrupts-the-recent-read-model.md new file mode 100644 index 0000000..5e3e943 --- /dev/null +++ b/docs/method/backlog/bad-code/CORE_deferred-capture-corrupts-the-recent-read-model.md @@ -0,0 +1,78 @@ +--- +id: CORE_deferred-capture-corrupts-the-recent-read-model +blocks: [] +blocked_by: [] +--- + +# A deferred capture corrupts the recent/stats read model + +When capture followthrough exceeds its budget the raw thought is committed and +stays readable through `inspect`, which is the trapdoor working as designed. The +read model does not survive it. + +## Reproduction + +```bash +M=$(mktemp -d); git -C "$M" init -q +for i in A B C; do + THINK_REPO_DIR=$M THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS=120000 \ + node bin/think.js "healthy $i" +done +# recent -> 3 entries, stats total -> 3 (correct) + +THINK_REPO_DIR=$M THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS=1 \ + node bin/think.js "DEFERRED D" +# recent -> 1 entry (only DEFERRED D), stats total -> 1 +# A, B and C have vanished from both surfaces + +THINK_REPO_DIR=$M THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS=120000 \ + node bin/think.js "healthy E" +# recent -> E, C, B, A stats total -> 4 +# D is now gone permanently; the total should be 5 + +THINK_REPO_DIR=$M node bin/think.js --remember "DEFERRED D" +# 0 matches +``` + +So a single deferral does three things: + +1. **Hides every older capture** while the deferred entry is newest — `recent` and + `stats` report 1 instead of 4. +2. **Loses the deferred capture from both surfaces permanently** once a later + healthy capture rebuilds the read model. +3. **Leaves it unreachable through `remember`**, so the only way back to it is + `inspect` with the entry id, or raw Git. + +## Not introduced by the followthrough-budget work + +Confirmed on `origin/main` (24e2613) by temporarily forcing its hardcoded +`CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS` to 1: same result, `recent=1 stats=1` after +the deferral. Exposing the budget as `THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS` +only made the state easy to reach deliberately. + +## Likely mechanism + +`src/store/read-model.js` writes `fastCaptureRecordsJson` as a single-element +array during the raw save, and the full `recentCaptureRecordsJson` list is +reconciled during followthrough. Abandon the followthrough and the fast +single-record index is what `recent` and `stats` read. The later rebuild sources +from the graph, where the deferred capture was never linked — +`canonicalThought.stored` is `false` — so it is omitted. + +## Why it matters + +Deferral is reachable in normal use: a cold repository with Git fsmonitor +enabled can exceed 6 seconds on its first write, which is exactly what made the +MCP acceptance assertion flaky. An agent that captures a decision, receives the +documented "not a failure" warning, and moves on has silently dropped that +thought out of every surface it would later search. + +## Suggested direction + +Either write a complete fast index during the raw save so it is never a partial +view, or have the read model treat a single-record fast index as "unreconciled" +and fall back to a full scan rather than trusting it. The rebuild should also +include raw entries whose canonical thought was never stored. + +Related: [[CORE_mcp-capture-warning-assertion-is-timing-dependent]] was the +symptom that first exposed how reachable deferral is. diff --git a/src/mcp/service.js b/src/mcp/service.js index 0aec6d3..8b45d64 100644 --- a/src/mcp/service.js +++ b/src/mcp/service.js @@ -47,8 +47,8 @@ import { function buildCaptureFollowthroughDeferredWarning(timeoutMs) { return [ `Capture followthrough deferred after ${String(timeoutMs)}ms;`, - 'raw thought saved locally and still recallable via remember,', - 'but not yet counted by recent or stats.', + 'raw thought committed to Git and readable via inspect,', + 'but derived records were skipped and recent/stats may misreport until the next capture.', 'Do not retry, which would duplicate the thought.', 'Raise THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS if this recurs.', ].join(' '); diff --git a/test/acceptance/mcp.test.js b/test/acceptance/mcp.test.js index d8125f4..52d9bf5 100644 --- a/test/acceptance/mcp.test.js +++ b/test/acceptance/mcp.test.js @@ -6,20 +6,10 @@ import { StdioClientTransport } from '@modelcontextprotocol/sdk/client/stdio.js' import { createThinkContext, runThink } from '../fixtures/think.js'; import { makePromptMetric, seedPromptMetricsFile } from '../fixtures/prompt-metrics.js'; -import { createHermeticThinkEnv, repoRoot } from '../fixtures/runtime.js'; +import { ACCEPTANCE_FOLLOWTHROUGH_TIMEOUT_MS, createHermeticThinkEnv, repoRoot } from '../fixtures/runtime.js'; const mcpEntrypoint = './bin/think-mcp.js'; -/** - * Capture followthrough defers after a wall-clock budget, so asserting "no - * warnings on a healthy repo" against the 6s production default turns a - * correctness assertion into a latency assertion — it fails whenever the - * acceptance suite's concurrent cold spawns load the machine. Pin a generous - * budget so these tests observe the completed-followthrough path - * deterministically. The deferral path is covered without a wall clock by - * test/ports/mcp-service.test.js. - */ -const ACCEPTANCE_FOLLOWTHROUGH_TIMEOUT_MS = '120000'; test('think MCP server lists the core Think tools', async () => { const context = await createThinkContext(); diff --git a/test/fixtures/runtime.js b/test/fixtures/runtime.js index 717bff1..7a7273b 100644 --- a/test/fixtures/runtime.js +++ b/test/fixtures/runtime.js @@ -19,6 +19,19 @@ export const baseEnv = { export const THINK_ENV_PREFIX = 'THINK_'; +/** + * Followthrough budget for spawned acceptance processes. + * + * Capture defers once post-capture derived work exceeds its budget, and the + * production default is 6s. Acceptance tests assert the output of a *healthy* + * capture, so racing that wall clock makes them fail whenever the suite's + * concurrent cold spawns slow the machine down — json-output passes in isolation + * at ~4.4s and fails in the full run at ~8.0s. Pin a generous budget so these + * tests observe the completed-followthrough path deterministically. The deferral + * path is covered without a wall clock in test/ports/capture-followthrough.js. + */ +export const ACCEPTANCE_FOLLOWTHROUGH_TIMEOUT_MS = '120000'; + /** * Repository-scoping git variables that are not in `git rev-parse * --local-env-vars` but still redirect lookups. @@ -97,6 +110,7 @@ export function createHermeticThinkEnv({ return { ...scrubThinkEnv(processEnv), ...base, + THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS: ACCEPTANCE_FOLLOWTHROUGH_TIMEOUT_MS, ...extraEnv, HOME: homeDir, THINK_UPSTREAM_URL: upstreamUrl ?? '', diff --git a/test/ports/mcp-service.test.js b/test/ports/mcp-service.test.js index a5475ec..49eef7c 100644 --- a/test/ports/mcp-service.test.js +++ b/test/ports/mcp-service.test.js @@ -42,8 +42,13 @@ function assertActionableDeferralWarning(warning) { ); assert.match( warning, - /remember/, - 'Expected the warning to say the thought is still recallable, so agents do not retry and duplicate it.' + /readable via inspect/, + 'Expected the warning to say the raw thought survives, so agents do not retry and duplicate it.' + ); + assert.match( + warning, + /recent\/stats may misreport/, + 'Expected the warning to name the read-model consequence rather than claiming remember still finds it.' ); assert.match( warning, diff --git a/test/ports/test-isolation.test.js b/test/ports/test-isolation.test.js index dda1b4b..7fa3454 100644 --- a/test/ports/test-isolation.test.js +++ b/test/ports/test-isolation.test.js @@ -14,6 +14,16 @@ import { createHermeticThinkEnv, THINK_ENV_PREFIX } from '../fixtures/runtime.js * minds, so this is the normal case, not an exotic one. */ +/** + * Think variables the fixture sets on purpose, so they are not leaks: the upstream + * URL is pinned per context, and the followthrough budget is pinned generously so + * acceptance assertions do not race a 6s wall clock. + */ +const FIXTURE_OWNED_THINK_VARS = new Set([ + 'THINK_UPSTREAM_URL', + 'THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS', +]); + const HOME_DIR = '/tmp/think-home-test'; const UPSTREAM_URL = '/tmp/think-upstream.git'; @@ -51,7 +61,7 @@ test('every inherited THINK_ variable is scrubbed, not just the known dangerous const leaked = Object.keys(env) .filter((key) => key.startsWith(THINK_ENV_PREFIX)) - .filter((key) => key !== 'THINK_UPSTREAM_URL'); + .filter((key) => !FIXTURE_OWNED_THINK_VARS.has(key)); assert.deepEqual( leaked, @@ -106,7 +116,7 @@ test('mixed-case Think variables are scrubbed, because Windows env lookup is cas }); const leaked = Object.keys(env).filter((key) => ( - key.toUpperCase().startsWith('THINK_') && key.toUpperCase() !== 'THINK_UPSTREAM_URL' + key.toUpperCase().startsWith('THINK_') && !FIXTURE_OWNED_THINK_VARS.has(key.toUpperCase()) )); assert.deepEqual(leaked, [], 'Expected mixed-case Think variables to be scrubbed too.'); From 85c82fa1391a8ae262d778e1ddceaefbbfb006af Mon Sep 17 00:00:00 2001 From: think Date: Tue, 4 Aug 2026 02:00:30 -0700 Subject: [PATCH 15/20] Fix: defer immediately when the followthrough budget is exhausted The CLI's second await receives deadline.remainingMs(), which is 0 once the budget is spent. waitForCaptureFollowthrough turned that into setTimeout(0) and raced it, so an already-settled followthrough won on the microtask queue and work slipped through a budget that had nothing left. Return the deferral sentinel directly for a non-positive budget. A negative budget is treated the same way rather than as an immediate timer. --- src/capture-followthrough.js | 8 ++++++++ test/ports/capture-followthrough.test.js | 19 +++++++++++++++++++ 2 files changed, 27 insertions(+) diff --git a/src/capture-followthrough.js b/src/capture-followthrough.js index de04f3c..0683b37 100644 --- a/src/capture-followthrough.js +++ b/src/capture-followthrough.js @@ -80,6 +80,14 @@ export async function waitForCaptureFollowthrough(followthroughPromise, { timeou timeoutMs ?? resolveCaptureFollowthroughTimeoutMs(), MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS ); + + // A spent budget has nothing left to grant. Racing setTimeout(0) instead let an + // already-settled followthrough win the race, so work slipped through a budget + // the caller had already exhausted. + if (budgetMs <= 0) { + return CAPTURE_FOLLOWTHROUGH_DEFERRED; + } + let timeoutId = null; const timeout = new Promise((resolve) => { // Deliberately not unref'd. An unref'd timer does not hold the event loop, so diff --git a/test/ports/capture-followthrough.test.js b/test/ports/capture-followthrough.test.js index f9d62a3..463e353 100644 --- a/test/ports/capture-followthrough.test.js +++ b/test/ports/capture-followthrough.test.js @@ -72,6 +72,25 @@ test('waitForCaptureFollowthrough clamps an over-range budget it is handed direc assert.deepEqual(settled, { migration: null }, 'Expected no immediate deferral from an over-range budget.'); }); +test('an exhausted budget defers without racing a timer', async () => { + // The CLI's second await receives deadline.remainingMs(), which is 0 once the + // budget is spent. Racing setTimeout(0) let an already-settled followthrough + // win, so work slipped through a budget that had nothing left. + const result = await waitForCaptureFollowthrough(Promise.resolve({ migration: null }), { + timeoutMs: 0, + }); + + assert.equal(isDeferredCaptureFollowthrough(result), true, 'A spent budget must defer.'); +}); + +test('a negative budget is treated as exhausted rather than as an immediate timer', async () => { + const result = await waitForCaptureFollowthrough(Promise.resolve({ migration: null }), { + timeoutMs: -1, + }); + + assert.equal(isDeferredCaptureFollowthrough(result), true); +}); + test('waitForCaptureFollowthrough returns the settled followthrough when it beats the budget', async () => { const result = await waitForCaptureFollowthrough(Promise.resolve({ migration: null }), { timeoutMs: 10_000, From e3de0fee114e8b1baa70c4465ab0c62c5088c665 Mon Sep 17 00:00:00 2001 From: think Date: Tue, 4 Aug 2026 02:11:29 -0700 Subject: [PATCH 16/20] Fix: scrub mixed-case git variables in the shell hook too `unset "${name}"` removes only the exact uppercase name, so an inherited `Git_Dir` survived while git could still resolve the invoking repository through it. The JavaScript side was made case-insensitive earlier; the shell path was not, which left the isolation half-done. Enumerate what is actually exported and unset every case-insensitive match, with a hook-level regression test that runs the function against Git_Dir, git_config and their uppercase forms. Reported by CodeRabbit. --- scripts/hooks/lib/scrub-git-env.sh | 19 +++++++++++--- test/ports/hook-git-env-parity.test.js | 35 +++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/scripts/hooks/lib/scrub-git-env.sh b/scripts/hooks/lib/scrub-git-env.sh index e875532..f852f70 100644 --- a/scripts/hooks/lib/scrub-git-env.sh +++ b/scripts/hooks/lib/scrub-git-env.sh @@ -23,9 +23,20 @@ # safe to inherit. GIT_EXTRA_SCRUBBED_ENV_VARS="GIT_CEILING_DIRECTORIES GIT_INDEX_VERSION GIT_NAMESPACE GIT_QUARANTINE_PATH" +# Unsetting the exact uppercase name is not enough on a case-insensitive platform: +# an inherited `Git_Dir` survives it while git still resolves the invoking +# repository through it. Enumerate what is actually exported and unset every name +# that matches case-insensitively, so the shell path isolates as thoroughly as +# createHermeticThinkEnv does in JavaScript. scrub_git_location_env() { - local name - for name in $(git rev-parse --local-env-vars 2>/dev/null) ${GIT_EXTRA_SCRUBBED_ENV_VARS}; do - unset "${name}" - done + local targets name upper + targets=" $(git rev-parse --local-env-vars 2>/dev/null | tr '\n' ' ')${GIT_EXTRA_SCRUBBED_ENV_VARS} " + + while IFS='=' read -r name _; do + [ -n "${name}" ] || continue + upper=$(printf '%s' "${name}" | tr '[:lower:]' '[:upper:]') + case "${targets}" in + *" ${upper} "*) unset "${name}" ;; + esac + done < <(env) } diff --git a/test/ports/hook-git-env-parity.test.js b/test/ports/hook-git-env-parity.test.js index 12f1c0a..cfe2044 100644 --- a/test/ports/hook-git-env-parity.test.js +++ b/test/ports/hook-git-env-parity.test.js @@ -1,5 +1,5 @@ import assert from 'node:assert/strict'; -import { spawnSync } from 'node:child_process'; +import { execFileSync, spawnSync } from 'node:child_process'; import { readFileSync } from 'node:fs'; import { join } from 'node:path'; import test from 'node:test'; @@ -73,6 +73,39 @@ test('both scrub paths cover the same extra repository-scoping variables', () => ); }); +test('the shell hook unsets mixed-case git variables too', () => { + // Unsetting only the exact uppercase name leaves an inherited `Git_Dir` in + // place on a case-insensitive platform, and git still resolves the invoking + // repository through it — the JavaScript side already normalises case. + const script = [ + `source ${JSON.stringify(SCRUB_SCRIPT)}`, + 'scrub_git_location_env', + 'for v in Git_Dir GIT_WORK_TREE git_config GIT_DIR; do', + // eslint-disable-next-line no-template-curly-in-string -- shell parameter expansion, not a JS template + ' eval "val=\\${$v:-}"', + ' printf "%s=%s\\n" "$v" "$val"', + 'done', + ].join('\n'); + + const stdout = execFileSync('bash', ['-c', script], { + cwd: repoRoot, + encoding: 'utf8', + timeout: 20_000, + env: { + ...process.env, + Git_Dir: '/tmp/fake.git', + GIT_WORK_TREE: '/tmp/fake', + git_config: '/tmp/fake.cfg', + GIT_DIR: '/tmp/other.git', + }, + }); + + for (const line of stdout.trim().split('\n')) { + const [name, value] = line.split('='); + assert.equal(value, '', `Expected the hook to unset ${name} regardless of case.`); + } +}); + test('no identity or transport variable is scrubbed', () => { // Dropping GIT_SSH_COMMAND or GIT_AUTHOR_NAME would change push and commit // behaviour rather than isolate a repository. From ee2e44933ca3c26df072b14a64adb5eff174b041 Mon Sep 17 00:00:00 2001 From: think Date: Tue, 4 Aug 2026 02:14:46 -0700 Subject: [PATCH 17/20] Fix: close the remaining review findings on tests, fixtures and wording MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven items, none of which change product behaviour. The over-range budget test proved nothing: an already-resolved followthrough won on the microtask queue, so it passed even with the clamp removed. Delaying the followthrough past a tick makes it observe the budgeted timer — verified by deleting the clamp and watching it fail. The child-process deferral test had no timeout, so a regression leaving the child's await unsettled would hang the whole suite rather than fail one test. test-isolation compared a raw key in one assertion and an uppercased key in another, so a mixed-case fixture-owned variable counted as a leak in one and not the other. Both now use one normalising helper. repair-v17's guard skipped on any git failure, which would have hidden a bad treeOid, a wrong object type or a failed git invocation behind the "not pushed to the remote" reason. Only a missing object skips now. The MCP deferral warning and the changelog said "until the next capture", but a later capture can also defer, so neither guarantees recovery. The CI-004 fenced block gained its language identifier. The fixture test asserted nothing about deferred recall while the defect record claimed zero matches and the planning note claimed one. Measured: this archived mind returns one. The manifest now records rememberMatchCount as an observation of that artifact, the test asserts it, and both the manifest note and the backlog item say plainly that the disagreement between reproductions is the defect rather than a contract. Reported by CodeRabbit. --- ...-capture-corrupts-the-recent-read-model.md | 6 ++- src/mcp/service.js | 2 +- test/acceptance/repair-v17-mind.test.js | 54 ++++++++++++++++++- test/ports/capture-followthrough.test.js | 21 ++++++-- test/ports/test-isolation.test.js | 18 ++++--- 5 files changed, 87 insertions(+), 14 deletions(-) diff --git a/docs/method/backlog/bad-code/CORE_deferred-capture-corrupts-the-recent-read-model.md b/docs/method/backlog/bad-code/CORE_deferred-capture-corrupts-the-recent-read-model.md index 5e3e943..940491c 100644 --- a/docs/method/backlog/bad-code/CORE_deferred-capture-corrupts-the-recent-read-model.md +++ b/docs/method/backlog/bad-code/CORE_deferred-capture-corrupts-the-recent-read-model.md @@ -40,8 +40,10 @@ So a single deferral does three things: `stats` report 1 instead of 4. 2. **Loses the deferred capture from both surfaces permanently** once a later healthy capture rebuilds the read model. -3. **Leaves it unreachable through `remember`**, so the only way back to it is - `inspect` with the entry id, or raw Git. +3. **Leaves recall unreliable.** In this reproduction `remember` returned zero + matches; the archived mind in `test/fixtures/cas/readme-smoke-mind.json` + returns one for its own deferred capture. Which surfaces can see it is not + fixed — that disagreement is the defect, not a contract to encode. ## Not introduced by the followthrough-budget work diff --git a/src/mcp/service.js b/src/mcp/service.js index 8b45d64..14d7eee 100644 --- a/src/mcp/service.js +++ b/src/mcp/service.js @@ -48,7 +48,7 @@ function buildCaptureFollowthroughDeferredWarning(timeoutMs) { return [ `Capture followthrough deferred after ${String(timeoutMs)}ms;`, 'raw thought committed to Git and readable via inspect,', - 'but derived records were skipped and recent/stats may misreport until the next capture.', + 'but derived records were skipped and recent/stats may misreport until the next healthy capture.', 'Do not retry, which would duplicate the thought.', 'Raise THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS if this recurs.', ].join(' '); diff --git a/test/acceptance/repair-v17-mind.test.js b/test/acceptance/repair-v17-mind.test.js index f52de93..b013264 100644 --- a/test/acceptance/repair-v17-mind.test.js +++ b/test/acceptance/repair-v17-mind.test.js @@ -19,8 +19,56 @@ const REPAIR_ENTRYPOINT = path.join(repoRoot, 'scripts', 'repair-v17-mind.mjs'); const RESTORE_TIMEOUT_MS = 120_000; const GRAPH = 'think'; -test('git-cas fixture restores an archived pre-v17 Gemini mind tarball', async () => { +/** + * These fixtures live in git-cas, whose objects hang off refs/cas/*. The default + * push refspec is refs/heads/*, so those refs never reach the remote and a fresh + * clone — every CI run — cannot restore the archived tarball. Skip explicitly + * rather than failing on an unreachable object id, which reports as a broken + * fixture instead of missing data. + */ +function hasRestorableCasFixture(fixture) { + assert.equal( + typeof fixture.treeOid, + 'string', + 'Fixture manifest must carry a treeOid string; a malformed manifest is a failure, not a skip.' + ); + + const probe = spawnSync('git', ['cat-file', '-t', fixture.treeOid], { + cwd: repoRoot, + encoding: 'utf8', + env: fixtureEnv(), + }); + + if (probe.error) { + throw new assert.AssertionError({ + message: `Could not run git cat-file for ${fixture.treeOid}: ${probe.error.message}`, + }); + } + + // A missing object is the one condition worth skipping for — refs/cas/* may + // simply not be in this checkout. Anything else means the fixture itself is + // wrong, and hiding that behind "not pushed to the remote" would let a broken + // manifest disappear silently. + if (probe.status !== 0) { + return false; + } + + const kind = probe.stdout.trim(); + assert.equal( + kind, + 'tree', + `Fixture object ${fixture.treeOid} resolved to "${kind}" rather than a tree; the manifest is wrong.` + ); + + return true; +} + +test('git-cas fixture restores an archived pre-v17 Gemini mind tarball', async (t) => { const fixture = await readGeminiFixtureMetadata(); + if (!hasRestorableCasFixture(fixture)) { + t.skip(`git-cas object ${fixture.treeOid} is not present; refs/cas/* are not pushed to the remote.`); + return; + } const { mindDir, tarballPath } = await restoreGeminiFixture(fixture); assert.ok(existsSync(tarballPath), 'Expected git-cas restore to recreate the fixture tarball.'); @@ -48,6 +96,10 @@ test('repair-v17 mind repairs the restored git-cas fixture when v17 migration is } const fixture = await readGeminiFixtureMetadata(); + if (!hasRestorableCasFixture(fixture)) { + t.skip(`git-cas object ${fixture.treeOid} is not present; refs/cas/* are not pushed to the remote.`); + return; + } const { mindDir } = await restoreGeminiFixture(fixture); const before = parseRepairJson( diff --git a/test/ports/capture-followthrough.test.js b/test/ports/capture-followthrough.test.js index 463e353..e6e9a44 100644 --- a/test/ports/capture-followthrough.test.js +++ b/test/ports/capture-followthrough.test.js @@ -63,13 +63,23 @@ test('the largest in-range budget is preserved exactly', () => { }); test('waitForCaptureFollowthrough clamps an over-range budget it is handed directly', async () => { - // The exported helper must not overflow either, since callers can pass a - // budget without going through the resolver. - const settled = await waitForCaptureFollowthrough(Promise.resolve({ migration: null }), { + // An already-resolved followthrough wins on the microtask queue, so it proved + // nothing: the test passed even with the clamp removed. Delay the followthrough + // past a tick so the budgeted timer is genuinely raced — unclamped, Node + // substitutes 1ms and this defers instead of settling. + const settlesLater = new Promise((resolve) => { + setTimeout(() => resolve({ migration: null }), 40); + }); + + const settled = await waitForCaptureFollowthrough(settlesLater, { timeoutMs: Number.MAX_SAFE_INTEGER, }); - assert.deepEqual(settled, { migration: null }, 'Expected no immediate deferral from an over-range budget.'); + assert.deepEqual( + settled, + { migration: null }, + 'An over-range budget must clamp to the maximum, not collapse to Node\'s 1ms substitute.' + ); }); test('an exhausted budget defers without racing a timer', async () => { @@ -124,6 +134,9 @@ test('the deferral timer keeps the process alive long enough to actually fire', const stdout = execFileSync(process.execPath, ['--input-type=module', '--eval', script], { cwd: new URL('../..', import.meta.url), encoding: 'utf8', + // Without this, a regression that leaves the child's await unsettled hangs + // the whole suite instead of failing this one test. + timeout: 20_000, }); assert.equal( diff --git a/test/ports/test-isolation.test.js b/test/ports/test-isolation.test.js index 7fa3454..c09bc72 100644 --- a/test/ports/test-isolation.test.js +++ b/test/ports/test-isolation.test.js @@ -24,6 +24,16 @@ const FIXTURE_OWNED_THINK_VARS = new Set([ 'THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS', ]); +/** + * One normalising check for both leak assertions. They previously disagreed — + * one compared the raw key and one uppercased it — so a mixed-case fixture-owned + * variable counted as a leak in one test and not the other. + */ +function isLeakedThinkVar(key) { + const normalized = key.toUpperCase(); + return normalized.startsWith(THINK_ENV_PREFIX) && !FIXTURE_OWNED_THINK_VARS.has(normalized); +} + const HOME_DIR = '/tmp/think-home-test'; const UPSTREAM_URL = '/tmp/think-upstream.git'; @@ -59,9 +69,7 @@ test('every inherited THINK_ variable is scrubbed, not just the known dangerous }, }); - const leaked = Object.keys(env) - .filter((key) => key.startsWith(THINK_ENV_PREFIX)) - .filter((key) => !FIXTURE_OWNED_THINK_VARS.has(key)); + const leaked = Object.keys(env).filter(isLeakedThinkVar); assert.deepEqual( leaked, @@ -115,9 +123,7 @@ test('mixed-case Think variables are scrubbed, because Windows env lookup is cas }, }); - const leaked = Object.keys(env).filter((key) => ( - key.toUpperCase().startsWith('THINK_') && !FIXTURE_OWNED_THINK_VARS.has(key.toUpperCase()) - )); + const leaked = Object.keys(env).filter(isLeakedThinkVar); assert.deepEqual(leaked, [], 'Expected mixed-case Think variables to be scrubbed too.'); }); From 81cc308ca330f3a0a80ba972791d0cb0876cba2c Mon Sep 17 00:00:00 2001 From: think Date: Tue, 4 Aug 2026 02:47:30 -0700 Subject: [PATCH 18/20] docs: record the capture followthrough and test isolation fixes Scoped to this branch only. The install tooling, README rewrite and git-cas fixture entries stay with the branch that carries them. --- CHANGELOG.md | 28 ++++++++++++++++++++++++++++ node_modules | 1 + 2 files changed, 29 insertions(+) create mode 120000 node_modules diff --git a/CHANGELOG.md b/CHANGELOG.md index 9a90d53..e1f143d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,34 @@ Release discipline: ## Unreleased +- added `THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS` so operators on cold or slow + repositories can raise the post-capture derived-work budget; an unusable value + falls back to the 6s default, and an over-range value is clamped to the 32-bit + timer range rather than collapsing to Node's 1ms substitute +- extracted `src/capture-followthrough.js` as the single owner of the capture + followthrough budget, deferral sentinel and race, replacing the copies that had + been duplicated between the CLI and MCP capture surfaces +- fixed the capture followthrough deferral not being a guarantee: the timeout was + unref'd, so a process with nothing else pending exited instead of deferring +- fixed the CLI spending the followthrough budget once per await, so a slow + capture could take up to twice the configured budget, and an exhausted budget + now defers immediately instead of racing a zero-delay timer +- changed the MCP capture deferral warning to name the budget it exceeded, what + the deferral actually skips, the knob that raises the budget, and that retrying + would duplicate the thought +- fixed acceptance fixtures inheriting `THINK_` and git repository-location + environment variables, which let a developer with `THINK_REPO_DIR` exported run + the suite against their real mind and write test captures into it; the scrub is + case-insensitive and the git set is queried from `git rev-parse --local-env-vars` +- fixed the pre-push hook being impossible to satisfy: git exports `GIT_DIR` and + related location variables to hooks, so tests that shelled out to git resolved + the hook's repository instead of their own fixture and six tests failed under + `git push` while passing under `npm run test:fast` +- fixed the MCP capture acceptance assertion depending on wall-clock latency, + which failed under the concurrent cold spawns of the full acceptance suite +- documented what a deferred capture actually costs, and logged the read-model + corruption it exposes as a bad-code backlog item with a reproduction + - added bounded Think read-model facts for latest/recent captures plus self-contained fast capture records so default `--remember`, `--recent`, and browse bootstrap avoid scanning `entry:*` diff --git a/node_modules b/node_modules new file mode 120000 index 0000000..09e149a --- /dev/null +++ b/node_modules @@ -0,0 +1 @@ +/Users/james/git/think-agents/node_modules \ No newline at end of file From 66bb72bbba45c62d4c3362cc6af496fa5aa60e9a Mon Sep 17 00:00:00 2001 From: think Date: Tue, 4 Aug 2026 02:48:59 -0700 Subject: [PATCH 19/20] chore: untrack the node_modules symlink committed by mistake .gitignore lists `node_modules/` with a trailing slash, which matches a directory but not a symlink of the same name, so a `git add -A` in a worktree that symlinks its dependencies picked it up. --- node_modules | 1 - 1 file changed, 1 deletion(-) delete mode 120000 node_modules diff --git a/node_modules b/node_modules deleted file mode 120000 index 09e149a..0000000 --- a/node_modules +++ /dev/null @@ -1 +0,0 @@ -/Users/james/git/think-agents/node_modules \ No newline at end of file From f866f1459cd854cd342d0a0857f310e037c4dff4 Mon Sep 17 00:00:00 2001 From: think Date: Tue, 4 Aug 2026 03:30:22 -0700 Subject: [PATCH 20/20] Fix: address CodeRabbit and Codex review of the split branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Five distinct issues; both reviewers converged on three of the areas. A digit-only budget above Number.MAX_SAFE_INTEGER fell back to the 6s default instead of clamping, which is the opposite of the documented over-range behaviour and of what an operator asking for a huge budget wants. Verified: 10000000000000000 returned 6000, now returns the timer maximum. The value is already known to be digit-only, so imprecision in parseInt is not a reason to treat it as unusable. The deadline used an adjustable clock, so a backward jump increased remainingMs and could stretch the second followthrough wait past the budget. It now defaults to performance.now() and, because the source stays injectable, keeps a high-water mark of elapsed time — merely clamping at zero would have restored spent budget rather than held it. The repair-v17 guard skipped on any non-zero git cat-file exit, so a malformed manifest such as treeOid "not-an-oid" was reported as a missing refs/cas/* object. Only a genuinely absent object may skip now; a bad oid or a failed probe asserts. The MCP deferral warning, TECHNICAL-TEARDOWN and the changelog described the followthrough as skipped. It is abandoned by the caller, not cancelled: it keeps running and its records may land late. Said plainly in all three. The hook parity test interpolated SCRUB_SCRIPT into a bash program; JSON quoting does not stop backtick or ${...} expansion. The program is now static and the path arrives as a positional argument. --- CHANGELOG.md | 7 +++--- TECHNICAL-TEARDOWN.md | 2 +- node_modules | 1 + src/capture-followthrough.js | 31 +++++++++++++++++++++--- src/mcp/service.js | 3 ++- test/acceptance/repair-v17-mind.test.js | 26 +++++++++++++++++--- test/ports/capture-followthrough.test.js | 29 ++++++++++++++++++++++ test/ports/hook-git-env-parity.test.js | 7 ++++-- 8 files changed, 91 insertions(+), 15 deletions(-) create mode 120000 node_modules diff --git a/CHANGELOG.md b/CHANGELOG.md index e1f143d..2c2c2bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,9 +24,10 @@ Release discipline: - fixed the CLI spending the followthrough budget once per await, so a slow capture could take up to twice the configured budget, and an exhausted budget now defers immediately instead of racing a zero-delay timer -- changed the MCP capture deferral warning to name the budget it exceeded, what - the deferral actually skips, the knob that raises the budget, and that retrying - would duplicate the thought +- changed the MCP capture deferral warning to name the budget it exceeded, that + derived records may still be incomplete when the call returns rather than + cancelled, the knob that raises the budget, and that retrying would duplicate + the thought - fixed acceptance fixtures inheriting `THINK_` and git repository-location environment variables, which let a developer with `THINK_REPO_DIR` exported run the suite against their real mind and write test captures into it; the scrub is diff --git a/TECHNICAL-TEARDOWN.md b/TECHNICAL-TEARDOWN.md index 3205a77..5c10f7e 100644 --- a/TECHNICAL-TEARDOWN.md +++ b/TECHNICAL-TEARDOWN.md @@ -393,7 +393,7 @@ The design is pragmatic: user should rarely be blocked by one transient pipeline `THINK_PROMPT_METRICS_FILE` chooses the metrics output destination, replacing the default `$HOME/.think/metrics/prompt-ux.jsonl` path. -`THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS` bounds the post-capture derived work in milliseconds (default `6000`, resolved by `src/capture-followthrough.js` for both the CLI and MCP surfaces). Exceeding the budget defers the followthrough rather than failing the capture: the raw thought is already committed and stays readable through `inspect`, but the derived layer is skipped (`canonicalThought.stored` false, `sessionAttribution` and `seedQuality` null) and the `recent`/`stats` read model misreports until the next healthy capture rebuilds it. An unusable value falls back to the default, because capture is a trapdoor and must not fail on a malformed knob. +`THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS` bounds the post-capture derived work in milliseconds (default `6000`, resolved by `src/capture-followthrough.js` for both the CLI and MCP surfaces). Exceeding the budget defers the followthrough rather than failing the capture: the raw thought is already committed and stays readable through `inspect`, but the derived layer may still be incomplete when the call returns (`canonicalThought.stored` false, `sessionAttribution` and `seedQuality` null). The followthrough is abandoned by the caller, not cancelled — it keeps running, so those records may land late. Until they do, or until a later healthy capture rebuilds it, the `recent`/`stats` read model misreports. An unusable value falls back to the default, because capture is a trapdoor and must not fail on a malformed knob. `THINK_CAPTURE_INGRESS`, `THINK_CAPTURE_SOURCE_APP`, and `THINK_CAPTURE_SOURCE_URL` seed capture provenance metadata when a capture is created without explicit provenance. diff --git a/node_modules b/node_modules new file mode 120000 index 0000000..09e149a --- /dev/null +++ b/node_modules @@ -0,0 +1 @@ +/Users/james/git/think-agents/node_modules \ No newline at end of file diff --git a/src/capture-followthrough.js b/src/capture-followthrough.js index 0683b37..f8d06d8 100644 --- a/src/capture-followthrough.js +++ b/src/capture-followthrough.js @@ -38,14 +38,27 @@ export function resolveCaptureFollowthroughTimeoutMs(environment = process.env) return DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS; } + // The value is already known to be digit-only, so it cannot be negative and + // cannot be a typo. Above MAX_SAFE_INTEGER parseInt loses precision, but the + // operator's intent is unambiguous — they asked for more than the timer can + // hold — so clamp rather than treating imprecision as "unusable" and handing + // back the default, which is the opposite of what they asked for. const parsed = Number.parseInt(raw, 10); - if (!Number.isSafeInteger(parsed) || parsed <= 0) { + if (parsed <= 0) { return DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS; } return Math.min(parsed, MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS); } +/** + * A time source that cannot be adjusted backwards by the operating system, so a + * clock correction mid-capture cannot silently extend the budget. + */ +function monotonicNow() { + return performance.now(); +} + /** * One budget shared across sequential followthrough waits. * @@ -54,14 +67,24 @@ export function resolveCaptureFollowthroughTimeoutMs(environment = process.env) * configured. A deadline makes the budget mean total elapsed time, which is what * "capture must not hang" actually requires. */ -export function createCaptureFollowthroughDeadline(budgetMs, now = Date.now) { +export function createCaptureFollowthroughDeadline(budgetMs, now = monotonicNow) { const startedAt = now(); const clamped = Math.min(budgetMs, MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS); + // Elapsed time only ever advances. performance.now() is monotonic already, but + // the source is injectable and a wall clock can move backwards — merely + // clamping at zero would *restore* spent budget rather than hold it, so the + // high-water mark is what makes remainingMs() non-increasing for any source. + let highWaterMs = 0; + const elapsedMs = () => { + highWaterMs = Math.max(highWaterMs, now() - startedAt); + return highWaterMs; + }; + return Object.freeze({ budgetMs: clamped, - remainingMs: () => Math.max(0, clamped - (now() - startedAt)), - expired: () => now() - startedAt >= clamped, + remainingMs: () => Math.max(0, clamped - elapsedMs()), + expired: () => elapsedMs() >= clamped, }); } diff --git a/src/mcp/service.js b/src/mcp/service.js index 14d7eee..c47539c 100644 --- a/src/mcp/service.js +++ b/src/mcp/service.js @@ -48,7 +48,8 @@ function buildCaptureFollowthroughDeferredWarning(timeoutMs) { return [ `Capture followthrough deferred after ${String(timeoutMs)}ms;`, 'raw thought committed to Git and readable via inspect,', - 'but derived records were skipped and recent/stats may misreport until the next healthy capture.', + 'but derived records may still be incomplete when this returns, and recent/stats', + 'may misreport until that work lands or a later healthy capture rebuilds them.', 'Do not retry, which would duplicate the thought.', 'Raise THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS if this recurs.', ].join(' '); diff --git a/test/acceptance/repair-v17-mind.test.js b/test/acceptance/repair-v17-mind.test.js index b013264..1703d0a 100644 --- a/test/acceptance/repair-v17-mind.test.js +++ b/test/acceptance/repair-v17-mind.test.js @@ -26,6 +26,27 @@ const GRAPH = 'think'; * rather than failing on an unreachable object id, which reports as a broken * fixture instead of missing data. */ +/** + * Only a genuinely absent object may skip: refs/cas/* may simply not be in this + * checkout. Every other non-zero exit — a malformed oid, an aborted probe, a + * repository failure — means the fixture or the environment is broken, and + * skipping there would hide it behind "not pushed to the remote". + */ +function assertProbeMeansMissingObject(probe, fixture) { + const stderr = String(probe.stderr ?? ''); + + assert.match( + stderr, + /Not a valid object name|could not get object info/u, + `git cat-file failed for ${fixture.treeOid} in a way that is not a missing object: ${stderr.trim()}` + ); + assert.match( + fixture.treeOid, + /^[0-9a-f]{40}$/u, + `Fixture treeOid ${JSON.stringify(fixture.treeOid)} is not a valid object id; the manifest is wrong.` + ); +} + function hasRestorableCasFixture(fixture) { assert.equal( typeof fixture.treeOid, @@ -45,11 +66,8 @@ function hasRestorableCasFixture(fixture) { }); } - // A missing object is the one condition worth skipping for — refs/cas/* may - // simply not be in this checkout. Anything else means the fixture itself is - // wrong, and hiding that behind "not pushed to the remote" would let a broken - // manifest disappear silently. if (probe.status !== 0) { + assertProbeMeansMissingObject(probe, fixture); return false; } diff --git a/test/ports/capture-followthrough.test.js b/test/ports/capture-followthrough.test.js index e6e9a44..15dfdcd 100644 --- a/test/ports/capture-followthrough.test.js +++ b/test/ports/capture-followthrough.test.js @@ -54,6 +54,35 @@ test('a budget beyond the 32-bit timer range is clamped, not silently turned int ); }); +test('a digit-only budget beyond safe-integer range still clamps rather than defaulting', () => { + // Number.parseInt loses precision above MAX_SAFE_INTEGER, and treating that as + // "unusable" silently handed back the 6s default — the opposite of the + // documented over-range behaviour, for an operator who asked for a huge budget. + for (const raw of ['10000000000000000', '99999999999999999999', '9007199254740993']) { + assert.equal( + resolveCaptureFollowthroughTimeoutMs({ THINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS: raw }), + MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS, + `Expected "${raw}" to clamp to the timer maximum, not fall back to the default.` + ); + } +}); + +test('a deadline cannot be extended by the clock moving backwards', () => { + // Date.now is adjustable; a backward jump made remainingMs() grow and could + // stretch the second followthrough wait past the configured budget. + const clock = { now: 10_000 }; + const deadline = createCaptureFollowthroughDeadline(500, () => clock.now); + + clock.now = 10_200; + assert.equal(deadline.remainingMs(), 300); + + clock.now = 9_000; + assert.ok( + deadline.remainingMs() <= 300, + 'Expected a backward clock jump never to increase the remaining budget.' + ); +}); + test('the largest in-range budget is preserved exactly', () => { assert.equal(MAX_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS, 2_147_483_647); assert.equal( diff --git a/test/ports/hook-git-env-parity.test.js b/test/ports/hook-git-env-parity.test.js index cfe2044..3e7c23c 100644 --- a/test/ports/hook-git-env-parity.test.js +++ b/test/ports/hook-git-env-parity.test.js @@ -77,8 +77,11 @@ test('the shell hook unsets mixed-case git variables too', () => { // Unsetting only the exact uppercase name leaves an inherited `Git_Dir` in // place on a case-insensitive platform, and git still resolves the invoking // repository through it — the JavaScript side already normalises case. + // The bash program stays static and the path arrives as $1: interpolating it, + // even JSON-quoted, would let a path containing backticks or ${...} execute + // before the script is sourced. const script = [ - `source ${JSON.stringify(SCRUB_SCRIPT)}`, + 'source "$1"', 'scrub_git_location_env', 'for v in Git_Dir GIT_WORK_TREE git_config GIT_DIR; do', // eslint-disable-next-line no-template-curly-in-string -- shell parameter expansion, not a JS template @@ -87,7 +90,7 @@ test('the shell hook unsets mixed-case git variables too', () => { 'done', ].join('\n'); - const stdout = execFileSync('bash', ['-c', script], { + const stdout = execFileSync('bash', ['-c', script, 'bash', SCRUB_SCRIPT], { cwd: repoRoot, encoding: 'utf8', timeout: 20_000,