Capture followthrough policy and test-harness isolation - #31
Merged
Conversation
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.
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.
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.
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.
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.
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.
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.
…ence 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.
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.
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.
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.
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.
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.
… defect 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.
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.
`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.
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.
Scoped to this branch only. The install tooling, README rewrite and git-cas fixture entries stay with the branch that carries them.
.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.
|
Warning Review limit reached
Next review available in: 34 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Summary by CodeRabbit
WalkthroughThe change centralizes capture followthrough timeout handling across CLI and MCP paths. It adds configurable deadlines and deferred results, hermetic test environments, Git environment scrubbing for hooks, acceptance fixture checks, and related documentation. ChangesCapture followthrough handling
Environment isolation
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66bb72bbba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/capture-followthrough.js`:
- Around line 57-65: Update createCaptureFollowthroughDeadline in
src/capture-followthrough.js to use a monotonic time source for startedAt,
remainingMs, and expired so backward wall-clock changes cannot extend the
clamped budget. Add coverage in test/ports/capture-followthrough.test.js
covering backward clock movement and asserting that remainingMs does not
increase.
- Around line 35-46: The timeout resolver in
resolveCaptureFollowthroughTimeoutMs already falls back for values above
Number.MAX_SAFE_INTEGER; add an over-range assertion for this deterministic
behavior. In test/ports/capture-followthrough.test.js lines 43-55, extend the
“very large budget” test with an unsafe-decimal input and assert it returns
DEFAULT_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MS; no direct source change is needed in
src/capture-followthrough.js.
In `@src/mcp/service.js`:
- Around line 47-54: Update buildCaptureFollowthroughDeferredWarning in
src/mcp/service.js to state that derived records may remain incomplete when the
response returns, without describing followthrough as cancelled or skipped. In
CHANGELOG.md lines 27-29, replace “skips” with non-cancellation terminology. In
TECHNICAL-TEARDOWN.md line 396, document that recovery can occur through either
late completion or a later healthy capture.
In `@test/acceptance/repair-v17-mind.test.js`:
- Around line 52-53: Update the git cat-file probe handling around the status
check so only a completed probe with exit status 0 and trimmed stdout equal to
“tree” is treated as a missing-object skip condition. For invalid object IDs,
aborted probes, repository errors, or any other non-tree result, raise an
assertion failure instead of returning false and allowing t.skip() to hide the
fixture failure.
In `@test/ports/hook-git-env-parity.test.js`:
- Around line 76-101: Update the shell-hook test around scrub_git_location_env
to keep the bash -c program static and pass SCRUB_SCRIPT as a positional
argument, then source that argument inside the script. Remove the interpolated
JSON.stringify(SCRUB_SCRIPT) from the generated command while preserving the
existing environment setup and assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c8df97f7-353a-41ff-9cf4-692065d28669
📒 Files selected for processing (17)
CHANGELOG.mdTECHNICAL-TEARDOWN.mddocs/method/backlog/bad-code/CORE_deferred-capture-corrupts-the-recent-read-model.mdscripts/hooks/lib/scrub-git-env.shscripts/hooks/pre-commitscripts/hooks/pre-pushsrc/capture-followthrough.jssrc/cli/commands/capture.jssrc/mcp/service.jstest/acceptance/mcp.test.jstest/acceptance/repair-v17-mind.test.jstest/fixtures/runtime.jstest/fixtures/think.jstest/ports/capture-followthrough.test.jstest/ports/hook-git-env-parity.test.jstest/ports/mcp-service.test.jstest/ports/test-isolation.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: test (22)
🧰 Additional context used
🪛 ast-grep (0.45.0)
test/fixtures/think.js
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawnSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
test/ports/capture-followthrough.test.js
[warning] 1-1: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
test/ports/hook-git-env-parity.test.js
[warning] 1-1: Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync, spawnSync } from 'node:child_process';
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process)
src/capture-followthrough.js
[warning] 96-96: Avoid using the initial state variable in setState
Context: setTimeout(() => resolve(CAPTURE_FOLLOWTHROUGH_DEFERRED), budgetMs)
Note: [CWE-710] Improper Adherence to Coding Standards. Security best practice.
(setstate-same-var)
🪛 LanguageTool
TECHNICAL-TEARDOWN.md
[style] ~396-~396: Consider replacing this word to strengthen your wording.
Context: ... default, because capture is a trapdoor and must not fail on a malformed knob. `TH...
(AND_THAT)
docs/method/backlog/bad-code/CORE_deferred-capture-corrupts-the-recent-read-model.md
[style] ~67-~67: Consider an alternative for the overused word “exactly”.
Context: ... 6 seconds on its first write, which is exactly what made the MCP acceptance assertion ...
(EXACTLY_PRECISELY)
🪛 OpenGrep (1.26.0)
test/ports/hook-git-env-parity.test.js
[ERROR] 61-61: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🔇 Additional comments (14)
test/fixtures/runtime.js (1)
2-2: LGTM!Also applies to: 20-118
test/fixtures/think.js (1)
4-4: LGTM!Also applies to: 37-41
test/ports/test-isolation.test.js (1)
1-204: LGTM!scripts/hooks/lib/scrub-git-env.sh (1)
1-43: LGTM!scripts/hooks/pre-commit (1)
4-6: LGTM!scripts/hooks/pre-push (1)
4-6: LGTM!test/ports/hook-git-env-parity.test.js (1)
1-75: LGTM!Also applies to: 103-136
src/capture-followthrough.js (1)
14-27: LGTM!Also applies to: 68-105
test/ports/capture-followthrough.test.js (1)
15-41: LGTM!Also applies to: 57-154, 185-196
src/cli/commands/capture.js (1)
2-7: LGTM!Also applies to: 60-76, 95-104
src/mcp/service.js (1)
1-5: LGTM!Also applies to: 62-76, 115-132
test/ports/mcp-service.test.js (1)
24-24: LGTM!Also applies to: 34-58, 101-103
CHANGELOG.md (1)
15-26: LGTM!Also applies to: 30-41
docs/method/backlog/bad-code/CORE_deferred-capture-corrupts-the-recent-read-model.md (1)
1-80: LGTM!
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.
flyingrobots
pushed a commit
that referenced
this pull request
Aug 4, 2026
#31 was split out of this branch, so main now carries reviewed versions of the files this branch still held in their pre-review form. Every code and doc conflict resolves to main: - src/capture-followthrough.js — main clamps digit-only over-range budgets and measures the deadline with performance.now() plus a high-water mark; this branch still had the Number.isSafeInteger gate and a Date.now() deadline that a backward clock jump could extend - test/ports/hook-git-env-parity.test.js — main passes the script path as a positional argument; this branch still interpolated it into the bash program, where JSON quoting does not stop backtick or ${...} expansion - test/acceptance/repair-v17-mind.test.js — main only skips for a genuinely absent object, so a malformed manifest fails instead of reporting as a missing refs/cas/* object - src/mcp/service.js, TECHNICAL-TEARDOWN.md — main says the followthrough is abandoned and may land late, not skipped Nothing unique to this branch was lost in those files; the only content on this side was the superseded original. CHANGELOG.md is a real merge: main's corrected foundation entries, then the 20 entries unique to this branch covering the install tooling, the Codex TOML hardening, the git-cas fixture and the README rewrite. The 10 entries this branch carried for foundation work were dropped in favour of main's wording, which is more accurate about deferral.
This was referenced Aug 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Split out of #30 so the fixes that repair things broken on
maintoday can land on their own, without waiting behind the MCP install tooling.Every commit here is cherry-picked from #30 with its original message intact, so each fix keeps the reproduction that found it. Verified as a clean subset: all 16 code and doc files are byte-identical to their #30 counterparts. No install tooling, no README rewrite, no git-cas fixture, no
package.jsonchange.What this fixes on
mainThe pre-push hook cannot currently be satisfied. Git exports
GIT_DIRand friends to every hook, so tests that shell out to git resolved the hook's repository instead of their own fixture —execSync('git init', { cwd: tempDir })wrote to$GIT_DIR. Six tests passed undernpm run test:fastand failed undergit push. The scrub now queriesgit rev-parse --local-env-varsrather than a hand-maintained list, in both the shell hook and the JS fixtures, case-insensitively.The acceptance suite could write into a developer's real mind. Fixtures pinned
HOMEbut inheritedTHINK_REPO_DIR, whichgetLocalRepoDirhonours ahead of$HOME/.think/repo. Reproduced: runningtest/acceptance/mcp.test.jswith it exported wrote 21 commits of test captures into a sentinel directory. Multi-agent setups export exactly that variable to namespace their minds, so this was the normal case.The followthrough deferral was not a guarantee. The timer was
unref'd, so a process with nothing else pending exited instead of deferring — reproduced standalone as exit code 13, "unsettled top-level await". Innode:testthat took a whole file down ascancelledByParent.The budget could be spent twice. The CLI awaits twice and handed each the full budget, so a slow capture took up to 2× what was configured — measured 3506ms against a 2000ms budget, now 2835ms. An exhausted budget also defers immediately rather than racing a zero-delay timer.
The 6s budget was duplicated between the CLI and MCP surfaces, which compared the deferral sentinel differently. Now one owner in
src/capture-followthrough.js, overridable viaTHINK_CAPTURE_FOLLOWTHROUGH_TIMEOUT_MSfor cold repositories, clamped to the 32-bit timer range so a large value cannot collapse to Node's 1ms substitute.A latent CI failure in the existing gemini fixture test. Its guard only checked for the git-warp migration, so a checkout without
refs/cas/*would have failed on an unreachable object id rather than skipping. It stayed hidden because CI'snpm testwas failing earlier intest:ports.Also corrected
The documented cost of a deferred capture. An earlier claim that
rememberstill finds it was drawn from a single unrepeated observation and is wrong: which surfaces can see it is nondeterministic, because the abandoned followthrough keeps running. The underlying read-model corruption is logged as a bad-code backlog item with a full reproduction, and confirmed pre-existing onmainby forcing its hardcoded budget to 1ms.Verification
npm run test:fastorigin/maintest:m1Note
One commit removes a
node_modulessymlink I committed by mistake..gitignorelistsnode_modules/— with a trailing slash that matches a directory, not a symlink of the same name, sogit add -Ain a worktree that symlinks its dependencies picks it up. Removed in a follow-up commit rather than an amend.#30 stays open with the MCP install tooling, README rewrite and git-cas fixture, currently at zero unresolved threads.