Capture followthrough policy and test-harness isolation

CHANGELOG.md

@@ -12,6 +12,35 @@ 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, 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
+  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:*`

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 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.
 
 Testing and scripted-run controls exist for non-interactive environments:

docs/method/backlog/bad-code/CORE_deferred-capture-corrupts-the-recent-read-model.md

@@ -0,0 +1,80 @@
+---
+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 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
+
+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.

node_modules

@@ -0,0 +1 @@
+/Users/james/git/think-agents/node_modules

scripts/hooks/lib/scrub-git-env.sh

@@ -0,0 +1,42 @@
+# shellcheck shell=bash
+#
+# 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
+# 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.
+#
+# 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"
+
+# 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 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)
+}

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."

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

src/capture-followthrough.js

@@ -0,0 +1,128 @@
+/**
+ * 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;
+
+/**
+ * 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' });
+
+/**
+ * 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;
+  }
+
+  // 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 (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.
+ *
+ * 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 = 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 - elapsedMs()),
+    expired: () => elapsedMs() >= clamped,
+  });
+}
+
+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 = Math.min(
+    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
+    // 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);
+  });
+
+  try {
+    return await Promise.race([followthroughPromise, timeout]);
+  } finally {
+    clearTimeout(timeoutId);
+  }
+}