The postmortem step's PowerShell regex was missing its closing quote, so
the step failed to parse and turned the whole chroma-windows job red even
though every actual gate passed. Close the string and add
continue-on-error so the diagnostic can never fail the job, as its own
comment already promises.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JT1VTKoaTf7VfePb7nVfwz
Four fixes from the megafix review cycle:
- mirror-dir.cjs: refuse identical or ancestor/descendant source and
destination roots before any filesystem mutation. The --delete pass
enumerated the destination first, so a source nested inside the
destination was seen as stale and recursively deleted (Greptile P1).
- worker-logs.cjs: track file identity (dev/ino) in the --follow loop.
A rename-and-recreate rotation whose replacement matched the old
offset's byte size was skipped forever by the size-only check
(Greptile P2).
- windows-git-bash-preflight: derive bin\bash.exe from every `git`
PATH hit instead of only the first. Git for Windows puts both
mingw64\bin\git.exe and cmd\git.exe on PATH; when mingw64 sorts
first the derivation missed the real bash and doctor reported a
false negative (tester report, D:\JavaTool\GIT layout).
- doctor: a missing .install-version marker with node_modules present
is now a warn, not a required failure. The marker is written only by
the npx installer — marketplace-flow and dev build-and-sync installs
never have one, so the check always failed for them and its repair
suggestion would overwrite the checkout under test.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JT1VTKoaTf7VfePb7nVfwz
* feat(cowork): add claude-mem-cowork plugin — memory for Claude app cloud sessions
Cowork/Claude-app cloud sessions run in ephemeral containers where the
local worker can't live. This plugin replaces it with thin HTTP hook
shims: PostToolUse streams raw tool-use fragments to POST /api/hooks/ingest
(spool + batch-flush on failure), SessionStart injects compiled context
from GET /api/hooks/context with a project-scoped fallback to the live
/api/mcp memory_search, and PreToolUse on Task/Agent prepends relevant
observations into every spawned agent's prompt via updatedInput.
- project naming is always automatic (cmem_work_root / cmem_work_<folder>),
deliberately not a setting
- empty projects inject a 'Claude-Mem is taking notes' notice with the
local worker viewer link (port from ~/.claude-mem/settings.json)
- unpaired installs are fully inert; every hook exits 0 unconditionally
- bundled skills: mem-search (progressive search) and mem-setup (pairing)
- PRO-ENDPOINT-SPEC.md documents the two server endpoints; ingest maps 1:1
onto the existing worker fragment queue
- 32-test mock-server suite in cowork/test/run-tests.mjs
- registered in .claude-plugin/marketplace.json
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012mXp3icorS7jwf42fFv9Wt
* fix(cowork): read real claude-mem settings keys in the ~/.claude-mem fallback
The compat fallback looked for syncToken/userId/syncHubUrl/workerPort, but a
local claude-mem install writes CLAUDE_MEM_CLOUD_SYNC_TOKEN / _USER_ID /
_HUB_URL / CLAUDE_MEM_WORKER_PORT (SettingsDefaultsManager). Honor those
first (old short names still accepted), update mem-setup + README, and make
the fallback test use the real key names.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JAjZBD6Qi5A8EuFmbcycDH
* fix(cowork): include cwd in the session-end envelope so the summary lands on the session's project
Verified live: a session-end sent from a different working directory filed
its summary under the hook process's project instead of the session's.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JAjZBD6Qi5A8EuFmbcycDH
* fix(cowork): redact secrets before envelope, per-user 0600 spool, merge-don't-clobber spool recovery
Addresses greptile P1 findings on PR #3707:
- observation/prompt payloads pass a secret-redaction pass (PEM blocks, auth
headers, JWTs, vendor token prefixes, key=value assignments) before the
envelope is built, so neither ingest nor the retry spool sees raw credentials
- spool moved from world-readable /tmp to ~/.claude-mem/cowork-spool.jsonl,
created 0600
- failed spool flush now merges the claim back via append instead of
renameSync over a replacement spool created by a concurrent hook, so
concurrent events are no longer dropped
- test harness: hermetic $HOME, redaction + spool-permission checks (38 checks)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JAjZBD6Qi5A8EuFmbcycDH
* fix(cowork): strip <private>-tagged regions before observation envelopes
Ports the local plugin's tag-stripping convention (src/utils/tag-stripping.ts)
into the cowork shim: <private>, <claude-mem-context>, <system-instruction>,
<persisted-output>, and <system-reminder> regions are removed from tool
payloads and session-init prompts before the envelope exists, so explicitly
non-storable content reaches neither ingest nor the retry spool. Stripping
runs before truncation so a cut-off closing tag can never leak a partial
private region. 4 new harness checks (42 total).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JAjZBD6Qi5A8EuFmbcycDH
* fix(cowork): redact connection-string credentials (scheme://user:pass@)
URI userinfo with a password now redacts to scheme://[cmem-redacted]@host —
covers postgres/mysql/redis/amqp/mongodb-style DATABASE_URLs that matched
neither the vendor-token patterns nor KEYVALUE_RE. Bare user@ URIs
(ssh://git@github.com) stay intact — that's signal, not secret. Mirrors the
userinfo rule in src/services/telemetry/error-scrub.ts. 3 new harness checks
(45 total).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JAjZBD6Qi5A8EuFmbcycDH
* fix(cowork): full URI userinfo redaction, Cookie header redaction, oldest-first spool recovery
Round 3 of greptile P1s:
- URI credential redaction now covers passwords containing literal '/', ':'
or '@' — userinfo is everything up to the LAST '@' in the URI token, so
postgresql://admin:pa/ss@host and admin:pa@ss@host both fully redact
(bare user@ like ssh://git@github.com still passes through as signal)
- Cookie:/Set-Cookie: header values redact wholesale regardless of cookie name
- failed-flush recovery drains a concurrent replacement spool onto the claim's
tail (old→new) and restores via wx (no clobber, append fallback); flushSpool
additionally stable-sorts the batch by ts so replay is always oldest-first
- 6 new harness checks (50 total)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JAjZBD6Qi5A8EuFmbcycDH
* fix(cowork): overflow spool re-spools the remainder instead of dropping oldest events
flushSpool sent only the newest SPOOL_MAX lines but deleted the whole claim on
success, permanently losing anything older once the spool exceeded 200 events.
It now sends the OLDEST 200 and re-spools the remainder for the next flush —
bounded batches, zero silent drops. On failure the untouched claim still
restores in full. 4 new harness checks incl. a 201-event drain (54 total).
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JAjZBD6Qi5A8EuFmbcycDH
* fix(cowork): claim spool before reading it; redact any Authorization scheme
- flushSpool now renames (claims) the spool FIRST and reads the claim, so an
event appended between read and rename can no longer travel into the claim
unread and be deleted on success — later appenders always write a fresh spool
- Authorization/Proxy-Authorization header values redact wholesale regardless
of scheme (Token, ApiKey, custom), and the inline scheme pattern gains Token
- 2 new harness checks: Token credential absent from request body and from the
failed-delivery spool (56 total)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JAjZBD6Qi5A8EuFmbcycDH
* docs: add Memory Prize scorecard and slides (hackathon 05)
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(cowork): redact short and whitespace-bearing credential values; align marketplace version
- KEYVALUE_RE no longer requires 6+ non-whitespace chars: sensitive-key
values are redacted in full through the line or record delimiter, so
short passwords and passphrases containing spaces never reach the
ingest payload or spool (Greptile P1)
- marketplace.json bumped to 0.1.3 to match plugin.json and the
X-CMEM-Plugin runtime header (Greptile P2)
- regression tests for short, whitespace-bearing, and short query-string
credential values
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Both OpenRouter providers carried their own hardcoded copy of the attribution
headers. They are one app on the public leaderboard, so drift between the
copies splits the ranking. Moves the values into src/shared/openrouter-attribution.ts,
beside the existing openrouter-base-url.ts.
Renames the app to Claude-Mem. Safe because OpenRouter keys app identity on
HTTP-Referer, not on the title — the display name changes and the accumulated
ranking does not. The referer is unchanged and a test pins it, so a future
tidy-up to the marketing domain fails CI instead of silently resetting the
leaderboard to zero.
Migrates X-Title to X-OpenRouter-Title, its canonical name, and claims
cli-agent,creative-writing — the only categories with a visible surface on the
/apps marketplace. Media-generation and roleplay categories are excluded and a
test asserts they can never appear.
CLAUDE_MEM_OPENROUTER_SITE_URL / _APP_NAME still override both, so forks and
self-hosted gateways keep attributing their own traffic.
Pairs with claude-mem-pro#112, which sends identical values when a Pro
subscriber's traffic is proxied through cmem.ai.
The five Windows branches each carried their own build of
plugin/scripts/*.cjs; merging them conflicted on those artifacts. Rebuilt
once from the merged src/ so the shipped scripts contain all five fixes.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WVK8iVC2dnrqE1fV4ogp2a
Addresses two P1s from review.
1. The size+mtime quick check returned before synchronizing mode bits, so a
source that changed permissions without changing size or mtime left the
destination stale. The direction that bites: a file whose execute bit was
revoked stayed executable in the marketplace. The quick check exists to
skip copying *content*, not to skip reconciling *metadata*.
Re-auditing the full `-a` (-rlptgoD) set rather than only the reported bit
found a second, larger gap: directory permissions and directory mtimes were
never applied at all. The A/B against real rsync missed it because the
snapshot recorded directories as the literal string 'dir'. On the repo tree
that was 205 diverging entries on every sync.
Now reconciled: permissions (-p, full 0o7777 so setuid/setgid/sticky are
included) and mtimes (-t) on files, directories and symlinks. Directories
are reconciled after their children, since writing children bumps the
destination mtime and tightening permissions first would lock the mirror out
of its own target. Deliberately not reconciled: owner/group (-o/-g, which
rsync itself can only apply as root) and devices/specials (-D, root-only and
absent from a source checkout). -a implies none of -H/-A/-X, so hardlinks,
ACLs and xattrs are out of scope for both tools.
2. worker-logs read, decoded and split the entire daily log before keeping its
last 50 lines. Against a 64 MiB log under a 16 MiB old-space limit that
aborted with exit 134 and printed nothing - failing exactly when someone is
debugging a busy worker. It now walks backwards in 64 KiB chunks until it has
one newline more than it needs, which also guarantees the chunk boundary is
discarded along with the partial line in front of it, so a multi-byte
character split across chunks cannot reach the output. --follow is unchanged
and now seeks from the size it already stat'd.
Tests: the A/B snapshot now covers directory mode+mtime and symlink mtime, and
runs metadata-only passes (chmod with size and mtime held constant, mtime-only,
directory-only). Against the pre-fix mirror it reports PARITY BROKEN on all five
passes; against the fix, all five are identical to rsync. The log tail gains a
64 MiB case run under --max-old-space-size=16, which spawns node rather than
process.execPath - under `bun test` execPath is bun, which ignores the flag and
would have made the case prove nothing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Windows users cannot build and test claude-mem from source. `npm install`
and `npm run build` already work on native Windows (windows.yml proves it
on every PR), but every step after them is bash-only:
- sync-marketplace.cjs shelled out to the rsync binary, which does not
exist on Windows and is not installed by anything in this repo. Git for
Windows does not bundle it.
- build-and-sync used `cd ~/.claude/plugins/...`, and the sync script's
own execSync calls passed `~` paths to the shell. `~` is bash-only home
expansion.
- worker:logs / worker:tail used `tail` and `date +%Y-%m-%d`.
Replaces rsync with scripts/mirror-dir.cjs, a stdlib-only mirror that
reproduces `rsync -a --delete --exclude=...`: the same pattern syntax
(anchored `/x`, dir-only `x/`, basename-at-any-depth, `*` / `**` / `?`),
the same size+mtime quick check, and crucially the same receiver-side
protection, so `--delete` still refuses to remove excluded paths like the
marketplace's .git and node_modules. Verified byte-identical against real
rsync over the whole repo tree (1125 entries: paths, types, sizes, modes,
mtimes, content hashes, symlink targets) and again after planting stale
and protected files in both destinations.
Home-relative paths now go through os.homedir() and cwd instead of the
shell. worker:logs becomes scripts/worker-logs.cjs, which also fixes the
pre-existing `tail -f 50` typo in worker:tail.
Windows CI now runs the real sync script against a scratch USERPROFILE
and asserts the --delete semantics, runs the mirror tests, and exercises
the log tail - the sync path was previously untested on Windows, which is
how this shipped in the first place.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two exposures I flagged in the round-7 report, closed rather than left
listed.
(1) Coverage. My claim that "the reuse branches are exercised by no CI on
any platform" was WRONG and overstated: ci.yml's build job runs
`bun test tests` on ubuntu, which picks up the whole reuse and identity
suites, and on Linux every describe executes. The real gap was narrower
and Windows-only — the Windows job ran the identity file but not the
reuse suite, and the reuse suite is describe.if(isPosix) throughout, so
adding it there would have skipped rather than covered.
That left three Windows mechanisms with no executing coverage anywhere,
all of them things we cannot verify locally: the CIM process-table read,
taskkill exit-code classification, and the root identity gate
short-circuiting before `taskkill /T /F`. A difference in any of those
would silently skip every descendant as "reused" and bring back #2313
while the code still looked guarded.
Adds tests/shared/kill-process-tree-cross-platform.test.ts — the same
guarantees expressed with a platform-appropriate fixture (cmd.exe/ping on
Windows, sh/sleep on POSIX) so both implementations actually execute,
wired into BOTH jobs so the platforms are held to one contract. Verified
as real gates: removing the root gate fails the no-op case; breaking
descendant enumeration fails four of five.
(2) Test seams. Assessed individually rather than removed reflexively.
- waitForUnexpectedCloseCleanupForTesting REMOVED from
ChromaMcpManager. It was a public method existing only for one
assertion, and the assertion survives unchanged by polling the
observable side effect (the recorded killProcessTree call) with the
suite's existing waitForCondition helper. Zero coverage lost, one
method off the production surface.
- __identityProbeCountForTesting KEPT, now documented as a deliberate
seam. It is a monotonic read-only counter with no way to mutate
state or influence a kill decision, nothing in src/ imports it, and
it is the only way to observe "isSameProcess re-reads the OS on
every authorization" — the property that stops a cached verdict
certifying a reused PID. Removing it would delete a real gate to
save no risk.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Greptile P1, and an instance of the limitation documented in round 4:
self-capture detects reuse DURING killProcessTree's awaits, but only a
caller-supplied token detects reuse that happened BEFORE entry.
The onclose handler is the case where self-capture is guaranteed too
late. It fires BECAUSE the child died, so by the time cleanup runs the
process is gone by definition; self-capture then reads whatever now owns
that PID and validates the replacement against itself — the same
"certifies the wrong process" shape as rounds 5 and 6.
Made structural rather than patched per report. The rule is: any site
retaining a PID from a live ChildProcess and killing it later must
capture the start token WHILE that child is alive. TrackedChild pairs
the two at spawn so they travel together, and no cleanup path in this
class can carry one without the other.
Three sites changed, all in this class, all sharing that root cause:
- onclose cleanup (reported) — token now captured where the handler is
installed, while the child is attached and alive;
- prewarm failure/timeout — often reached BECAUSE the child exited;
- disposeActivePrewarm — the handle may already have exited.
Six sites left alone, each verified against the rule rather than assumed:
- process-registry sigtermDuplicateSdkProcess: killProcessTree's
self-capture runs synchronously at entry, immediately after
isPidAlive confirmed liveness;
- shutdown.ts signalProcess (both arms): reached only after liveness
was checked and the root token captured, or gated by rootIsSame
using a token captured while alive;
- worker-utils and ServerService: verifyPidFileOwnership runs
immediately prior;
- ensureSdkProcessExit: already passes an explicit token (round 4).
Deliberately NOT extended to ManagedProcessInfo/supervisor.json. Every
registry cleanup already captures its token at a moment the process is
provably alive, so persisting one would be schema churn with no defect
to fix.
Tests, both halves and both levels. The mechanism (a caller token
overriding self-capture for a dead PID) is gated in the reuse suite. The
WIRING is gated separately in the chroma suite, because reverting only
the call site left the integration suite green — a real PID-reuse race
cannot be driven against live chroma, so that test asserts the onclose
cleanup passes an expectedStartToken at all. Verified red-without-fix at
both levels.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Greptile P1 (security). The Windows start-token cache did not merely
weaken identity revalidation — it made it inert.
captureProcessStartToken caches per-pid for 5s. A snapshot capture
populates that entry and the revalidation reads the SAME entry back, so
inside the TTL isSameProcess compared a value against itself and
returned true 100% of the time. Not a race: a tautology. A reused PID
was therefore certified as the original and handed to
`taskkill /PID <pid> /T /F`, force-terminating an unrelated process AND
its entire descendant tree.
This is round 5's defect on the platform this PR is named after: a guard
that validates the wrong thing is worse than no guard. Three previous
round-ups called it "best-effort on Windows", which framed an incorrect
result as a weaker one. That framing was wrong.
True surface, checked before writing code:
- the cache is Windows-ONLY; POSIX always read fresh, so POSIX was
never exposed;
- round 4's atomic enumeration already fixed descendant CAPTURE (it
comes from the CIM table read, not a cached probe), so the exposure
was the revalidation side — total for the root, partial for a
descendant revalidated twice inside one TTL.
Fix: isSameProcess now reads uncached. Chosen over cache invalidation
because invalidation is action-at-a-distance — correct only if every
present and future call site remembers to invalidate first, which is
precisely the opt-in shape that produced rounds 3, 4 and 5. Reading
uncached is correct by construction at the one place that authorizes a
kill.
Blast radius contained as previously flagged: captureProcessStartToken
keeps its cache for verifyPidFileOwnership (the repeated-validation case
the cache exists for), and the uncached reader is NOT exported — the
only way to obtain a bypassing read is through the predicate that
authorizes kills, so it cannot leak to callers that want the cache. All
nine isSameProcess call sites were audited first: every one authorizes
an irreversible kill, so the change has no non-kill blast radius.
Measured cost: 2.27ms per call on macOS (~9ms for a typical chroma
teardown of one root plus three descendants). POSIX is unchanged in
behaviour since it never cached. Windows pays a CIM query per
authorization (~100-300ms); teardown is not a hot path and the
alternative is force-killing an unrelated process tree.
Tested on every platform, not just Windows: a probe counter asserts
isSameProcess performs a fresh read on every call, so the assertion is
not vacuous off-Windows, and a Windows-gated case asserts the cached
accessor really does still serve from cache (without it, the bypass
could pass on a build where caching silently stopped working). Verified
red-without-fix by restoring a cached read.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Greptile P1 (A). Descendant PIDs were enumerated first and each one's
start token probed afterwards. That is worse than having no check: if a
discovered PID exits and the number is reissued in between, the probe
captures the REPLACEMENT's token, and the later isSameProcess() compares
that replacement against itself, matches, and certifies an unrelated
process as a legitimate kill target. The guard did not merely miss a
reuse — it blessed the wrong process.
Discovery and identity now come from one process-table read per
platform:
- Linux: /proc/<pid>/stat gives ppid (field 4) and starttime (field 22)
in a single read — atomic per process, and field 22 is exactly what
captureProcessStartToken() returns here.
- macOS/BSD: one `ps -eo pid=,ppid=,lstart=` snapshot. Its lstart is
byte-identical to `ps -p <pid> -o lstart=`, which is the probe form.
LC_ALL/LANG pinned as they are there.
- Windows: the CIM query already returned the parent link, so
CreationDate now comes from the same row instead of N follow-up
PowerShell probes — which were both the race and, at ~100-300ms
each, the dominant cost of enumeration.
So POSIX could be made atomic after all, and the result is cheaper than
the pgrep walk it replaces (one observation instead of N spawns).
The same enumerate-then-probe pattern existed in two more places
Greptile did not flag — ChromaMcpManager.snapshotDescendantIdentities
and the shutdown cascade's per-record snapshot. Both now call the shared
collectDescendantIdentities, so there is one implementation rather than
three copies of the hole.
RESIDUAL, stated rather than papered over: this closes the enumerate/
capture gap but not PID reuse in general. Identity still rests on a
token that a later revalidation re-reads, and on Windows
captureProcessStartToken caches per-pid for 5s, so a revalidation inside
that TTL re-reads its own cached value. Fully effective on POSIX,
best-effort on Windows.
The new failure mode this design could have is silent and severe: if the
table read and the probe ever disagree on token FORMAT, every comparison
fails, every descendant is skipped as "reused", and the orphan bug
(#2313) returns while the code still looks guarded. That agreement is
now asserted by test on every platform, and the test is wired into both
CI jobs — including the Windows one, since the CIM format is the piece
that cannot be verified off-Windows.
Greptile's second finding (session-reap initial teardown missing
expectedStartToken) was already fixed in e2421eb1a and is present in the
pushed branch; no change was made for it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Greptile P1 #5/#6. The findings are two more instances, but the shape is
the bug: root identity was opt-in via expectedStartToken, so any call
site that omitted the option silently reopened the reuse hole. Eleven of
the thirteen real call sites omit it. That is why each review round found
another one, and patching these two would have set up a fifth.
killProcessTree now captures the root's start token ITSELF at entry when
the caller did not supply one, and revalidates before every signal it
sends — the initial root signal, the post-settle re-enumeration, and the
final SIGKILL. Omitting the option is safe; a call site can now only
weaken this by passing an explicitly wrong token, not by forgetting an
argument.
expectedStartToken remains, because it is a strictly stronger guarantee
and not redundant:
- self-captured (default) detects reuse during THIS function's awaits;
- caller-supplied additionally detects reuse that happened BEFORE
entry, which self-capture cannot see — by then it would be reading
the replacement's token. ChromaMcpManager needs that: its PID is
captured before `await transport.close()`.
A reused root skips the root signal but does NOT abort the call: the
descendants enumerated earlier were the real root's children and still
have to be reaped. Re-enumeration after the settle IS skipped, since
walking a reused PID would list the replacement's children.
P1 #6 (session-reap initial teardown) now passes its earlier-captured
token too. That is belt-and-braces given the new default, but the
reaper captured the token before the loop's awaits, so it is the
stronger of the two guarantees.
Also fixes a genuinely timing-dependent fixture this exposed. The #3378
control asserted graceful mode delivers SIGTERM to the root, using a
`sleep 300 & wait` root — which exits on its own the moment the
descendant pass kills its sleep. The added revalidation (a `ps` spawn on
macOS) widened that window enough for the root to be gone before the
root signal landed, so the trap never fired. The fixture now self-loops
and outlives its child; it was measuring fixture timing, not delivery.
Cost note: revalidation adds a start-token read per signal point —
effectively free on Linux (/proc) and Windows (5s TTL cache), ~10-30ms
per read on macOS (`ps` spawn). Teardown is not a hot path and the
correctness is worth it.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Greptile's P1 #3 and #4, plus two more sites the audit turned up. Same
defect class as the previous round: a PID captured before a wait is not a
stable handle.
Rather than patch the two reported spots, every place this PR signals a
PID it captured earlier was audited and the rule made uniform.
killProcessTree now owns both halves of its own contract:
- the INITIAL signal loop revalidates each descendant (P1 #3). The
tokens are captured moments earlier, but capturing is not free — a
`ps` spawn per PID on macOS, a CIM query on Windows — so a
multi-level chain leaves real time between enumerating the first
descendant and signalling it, in which that PID can exit and be
reissued. Graceful mode would SIGTERM the replacement; immediate
mode would SIGKILL it.
- a new `expectedStartToken` option gates the ROOT. On mismatch the
call is a complete no-op: it does not signal the root, and it does
not enumerate descendants from it either, because those children
belong to the replacement and on Windows `taskkill /T` would take
that whole subtree down.
Callers that hold a PID across an await now pass that token:
- ChromaMcpManager.disposeCurrentSubprocess (P1 #4) captures the root
token BEFORE close(); the post-close re-scan is likewise skipped
when the root's identity changed. The identity-validated descendant
reap still runs, which is the part that matters once the real root
is gone.
- ProcessRegistry.reapSession — NOT reported, found by the audit. Its
SIGKILL phase held record PIDs across a 5s waitForExit.
- ensureSdkProcessExit — NOT reported, found by the audit. Held its
PID across a race of up to timeoutMs (5s default).
Sites deliberately left unguarded, because the PID is signalled with no
intervening await and a token there would cost a probe to compare a
value against itself: reapSession's SIGTERM phase and
sigtermDuplicateSdkProcess (both immediately after isPidAlive), the
prewarm/unexpected-close paths (PID read straight off a live
ChildProcess), and worker-utils / ServerService (verifyPidFileOwnership,
which already does a token check, runs immediately before).
Fallback semantics unchanged: no token capturable → proceed; token read
and DIFFERS → skip. Refusing must stay strictly narrower than killing or
an unreadable token would strand a live orphan (#2313).
Tests cover both halves in both modes — unchanged identity is still
signalled, changed identity is skipped — plus the root gate. Verified as
real gates: removing the initial-signal guard fails both SKIP tests
(graceful and immediate), removing the root gate fails the no-op test.
The Windows-specific wiring in ChromaMcpManager (mustEscalate always
true on win32) is not reachable off-Windows; the mechanism it depends on
is covered at the killProcessTree level instead.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
claude-mem's hooks all declare "shell": "bash". On Windows, Claude Code
resolves bash through a closed chain (CLAUDE_CODE_GIT_BASH_PATH env var
-> standard Git for Windows install paths -> `git` on PATH -> null, no
WSL fallback) and throws an opaque, unbranded error the first time a
hook runs if nothing resolves. That hits any Windows user who satisfied
Claude Code's own requirements via PowerShell without installing Git
for Windows.
Add a preflight (src/npx-cli/utils/windows-git-bash-preflight.ts) that
replicates the same resolution chain and reports one actionable,
claude-mem-branded message telling the user to install Git for Windows
or set CLAUDE_CODE_GIT_BASH_PATH. Wired into:
- `doctor`: a new required check, following the existing check/report
format exactly.
- `install`: a non-blocking warning surfaced right after the version
banner, so a Windows install without Git Bash doesn't silently ship
broken hooks.
No-op on macOS/Linux (the probe is never invoked off win32). Does not
touch plugin/hooks/hooks.json or src/build/hook-shell-template.ts —
this is the "fail loudly" slice only; it does not fix the underlying
Bash dependency.
Tests inject the filesystem probe rather than mutating process.platform,
per tests/services/integrations/spawn-contract-windows.test.ts.
tree-sitter-cli installs tree-sitter.exe on win32, but getTreeSitterBin()
in parser.ts joined the package dir with the POSIX binary name only. The
existsSync() check always missed on Windows, silently falling through to
a bare `tree-sitter` that isn't guaranteed to be on the worker's PATH —
smart file parsing / structural search then returned empty results with
no error.
Extracted the resolution logic into resolveTreeSitterBinPath(platform),
following the same platform-suffix idiom already used by
ChromaMcpManager.resolveUvxCommand() and setup-runtime.ts's *_COMMON_PATHS
tables. Checked resolveGrammarPath() and the GRAMMAR_PACKAGES lookups
nearby in the same file — those resolve grammar directories, not
executables, so they aren't affected by this defect.
Found during the #3644 Windows portability audit.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Two P1s from Greptile on #3644, both the same defect class: a PID held
across a wait is not a stable handle, because the OS reissues numbers.
killProcessTree's graceful union (P1 #1) kept pre-TERM descendant PIDs
as bare numbers across the 500ms settle, then SIGKILLed them
unconditionally. Members present only in the pre-TERM scan are missing
from the post-wait scan for one of two reasons — they re-parented after
the root died (still alive, and exactly what the union exists to reap),
or they exited during the window (number possibly reissued). Without a
token those cases are indistinguishable and the second one gets an
unrelated process killed. Descendants now carry a start token from the
scan that found them and are revalidated immediately before escalation.
shutdown.ts's force phase (P1 #2) tree-killed FROM the root. A live
snapshotted descendant keeps a record in `survivors` after its root has
exited, so a reused root PID meant starting a tree-kill at an unrelated
process and taking its children with it. The root's token is now
captured alongside its descendants; when it no longer matches, the
tree-kill is skipped and only the identity-validated descendant reap
runs.
captureProcessStartToken moved to src/shared/process-identity.ts (with a
new isSameProcess helper) because kill-process-tree needs it and
process-registry already imports kill-process-tree — importing it back
would close a cycle. process-registry re-exports it, so no caller
changes. The three ad-hoc identity checks added earlier in this series
now share that one helper.
Fallback semantics are unchanged and deliberately asymmetric: an
uncapturable token on either side means proceed. Refusing to kill must
stay strictly narrower than killing, or an unreadable token would strand
a live orphan and resurrect #2313. Only a token read successfully AND
differing proves reuse.
Provenance: the union is inherited from PR #2282 and was already on main
inside ChromaMcpManager, so P1 #1 is pre-existing — but this series
spread that helper from one call site to five, which grows the blast
radius. P1 #2 is ours: it only exists because we added killProcessTree
to signalProcess and widened `survivors` to include dead-root records.
Windows caveat, unchanged: captureProcessStartToken caches per-pid for
5s, so a revalidation inside that TTL re-reads its own cached value and
cannot detect reuse there. Fully effective on POSIX (/proc and ps are
uncached), best-effort on Windows, and no worse than the unchecked
behavior it replaces.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both workflows failed to compile, so neither the Windows job nor the
Linux gate ever ran and PR #3644 had no verification signal at all.
`${{ runner.temp }}` was used in `jobs.<id>.env`. The runner context is
only available at STEP level; the job-level allowlist is github, needs,
strategy, matrix, vars, secrets and inputs. Actions rejects the whole
file with "Unrecognized named-value: 'runner'" before scheduling
anything — a startup_failure with an empty jobs array, no logs, and the
run labelled with the file path because `name:` never parsed either.
Plain YAML parsers accept the file, which is why this got through.
Moved CLAUDE_MEM_DATA_DIR to the two test steps in each workflow, where
`runner` IS valid, and left the static vars at job level. Chosen over
${{ github.workspace }} (valid at job level, but it would write the
sqlite db, chroma dir and pid files into the repo checkout) and over
exporting via $GITHUB_ENV (an extra step plus bash/pwsh divergence for
no gain). Step-level env keeps the value visible at its point of use and
still reaches the process environment, which is required: paths.ts
freezes DATA_DIR at import, so a later assignment is a silent no-op.
Isolation is unchanged. tests/preload.ts pins a temp data dir only when
the env var is unset ("env wins"), so the explicit per-step value is
honoured — verified locally: all artifacts landed in the given dir and
zero fallback dirs were created.
Also fixes the shellcheck SC2009 actionlint flagged in the Linux
diagnostic step (ps|grep -> pgrep -a -f).
Validated with actionlint 1.7.12 across all 8 workflows: it reproduces
the exact error before the fix and reports clean (exit 0) after.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Why these bugs kept shipping: windows.yml was build-only (install,
build, one Bun resolver test) and never spawned Chroma, so #3482, #3540
and #3552 were all invisible to CI.
Adds a job to each workflow. Linux carries the same gate because the
orphan reproduces there too, which means the regression is proven on the
runner every PR already uses rather than resting on a Windows job.
- 'tests/**' added to the Windows path filter; without it a PR touching
only these tests would not run them.
- astral-sh/setup-uv with enable-cache/cache-python, or a cold chromadb
resolve costs ~10 minutes per run and overruns the timeout.
- CLAUDE_MEM_CHROMA_PREWARM_TIMEOUT_MS=600000 (production default is
120s; 600s is the accepted maximum) and bun --timeout 600000 (bun's
per-test default is 5s).
- CLAUDE_MEM_DATA_DIR under runner.temp. It must be set in the job env:
DATA_DIR is resolved into a module-level const at import time, so
assigning it from inside a test is a silent no-op that falls back to
the real profile directory.
- No retries. A retry would paper over exactly the orphan race these
jobs exist to catch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A ChromaMcpManager-only round trip cannot prove this fix: stop() already
tree-kills on main, so create/add/query/stop passes there and proves
nothing. The single-PID kill that causes #3482 lives in the worker
RECYCLE path, so that is what the gate drives.
worker-recycle-orphans.test.ts spawns a fixture child that owns a real
chroma-mcp tree, serves /api/health with a stale version and writes an
owned worker PID file, then calls the production ensureWorkerRunning().
Verified by execution, both directions, on macOS:
main's single-PID SIGKILL:
orphaned descendants survived the worker recycle:
.local/bin/uv(pid=52908), .../bin/python(pid=52909)
0 pass 1 fail
with the fix:
1 pass 0 fail
Descendants are snapshotted BEFORE teardown — after the root dies its
children re-parent and a post-mortem walk finds nothing, which would be
a false PASS — and matched on pid AND start token so a recycled PID
cannot fake success. The re-check biases toward "still alive" when a
token cannot be re-read: treating a transient null as "gone" would empty
the survivor list and end the primary gate green over real orphans.
chroma-windows-lifecycle.test.ts additionally covers the manager
teardown, which nothing could previously see. It asserts the PRODUCTION
collectDescendantPids returns a non-empty chain — the exact assertion
that fails when enumeration is blind, as it was on Windows — then that
no identity-matched descendant survives stop(). Confirmed to fail when
that regression is simulated. Its polluted-env case is a real
cross-platform gate for #3552: it fails with the strip disabled.
Both suites are opt-in via CLAUDE_MEM_TEST_CHROMA and refuse to run
without CLAUDE_MEM_DATA_DIR set, since the recycle path kills whatever
owns the real worker port.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Teardown ordering was the #3540 root cause: disposeCurrentSubprocess
tree-killed BEFORE closing, so `uv` was always SIGKILLed mid-build and
never unlinked its builds-v0/.tmp* scratch dir (144.21 GB / 696 dirs
measured on one machine). StdioClientTransport.close() already
implements the escalation this needs — stdin EOF, 2s, SIGTERM, 2s,
SIGKILL — so closing first IS the fix; no new timer scheme.
Reordering has two failure modes and both are handled:
- Escalating too eagerly re-creates #3540. close() can resolve before
Node processes the child's 'exit', so exitCode is briefly still null
for a process that already died; a bounded wait for the event avoids
escalating on that false negative.
- Escalating too reluctantly orphans the chain (#3482). On Windows this
dominates and the exitCode check cannot help: close() ends in
TerminateProcess against one pid, so uvx.exe dies while uv -> python
-> chroma-mcp keep running, and reading exitCode there would skip the
taskkill /T /F that is the only thing able to reach them. win32
therefore always escalates.
The #2313 singleton invariant needs the descendant set captured BEFORE
the close (children re-parent once the root is gone) and unioned with a
post-close re-scan, so a descendant must be invisible at both sample
points to escape. Reaping verifies start tokens so a recycled PID is
never killed.
#3552: uvx --python builds its own environment, but CPython still
honours ambient interpreter variables, so a worker started from an
activated venv or conda shell handed chroma-mcp a foreign prefix and it
died on a numpy ABI clash during chromadb import — silently, before the
MCP handshake. VIRTUAL_ENV/PYTHONHOME/PYTHONPATH/CONDA_PREFIX/
CONDA_DEFAULT_ENV are stripped at the point the child env is built, and
case-insensitively on Windows, where `PythonPath` IS `PYTHONPATH` to the
OS and an exact-uppercase delete would miss it entirely.
The two uvx env builders were NOT unified: getUvxPreflightEnv is the
spawn env, while dependency-preflight's effectiveUvxEnv is only read for
its PATH key to stat the filesystem and never spawns anything. Sharing
the rule keeps them from drifting without asserting an equivalence that
does not hold.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`server stop` cleared the PID file and printed "Server stopped"
regardless of what the kill actually did, which is how the next `start`
silently races a live daemon.
- win32 tree-kills (the daemon's children were previously stranded) and
a ProcessTreeKillError now aborts the stop, leaves the PID file in
place and exits 1.
- Even a successful signal is not proof of exit, so the process is
re-checked before claiming success.
- The exit deadline was 5s while runShutdownCascade budgets 5s after
SIGTERM plus 1s after SIGKILL, so a legitimate but slow stop reported
failure every time. Raised to 15s to exceed the cascade it waits on.
POSIX still sends a single SIGTERM here: the handler runs the DB/queue/
HTTP cleanup and needs its full window, which a tree-kill would cut
short. That asymmetry is deliberate and is NOT POSIX parity.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Alex Newman