Two review findings from @karthik-indla on this PR.
_drain returned (0, True) on any read failure, so a claim nothing was posted
from counted as fully delivered. flush() then carried on to the next claim as
though this one had arrived, and the single signal that says the run went badly
never fired. The two cases are now separated: undecodable content is still
quarantined and reported delivered, because there is nothing left to send and
the rest of the run should continue, while an OSError leaves the file exactly
where it is and reports undelivered. Quarantining there would discard events
over a transient filesystem error, and nothing ever re-globs .corrupt.
Retries had no time backoff. _release_claim backdated straight to
immediately-reclaimable, so two senders meeting one momentary failure could walk
a batch from attempt 0 to the limit within seconds and discard it, when a retry
a minute later would have delivered. Releases now carry a cooldown that grows
with the attempts already spent, clamped so the mtime never lands in the future
and reads as a live lease.
Four tests: an unreadable batch is neither delivered nor quarantined,
undecodable content still is quarantined so one torn file cannot block every
later claim, and attempts cannot be burned without waiting. The expiry test now
ages the file between flushes, which is the wall time a real retry waits.
Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
Review finding from @kartik-mem0 on this PR, and the most serious one: it loses
events, which is what this PR exists to prevent.
_claim_parked judged exhaustion before liveness. Claiming a parked file bumps
its attempt count and refreshes its mtime, so the moment a sender takes the
final attempt the file looks exhausted to every other sender while its owner is
actively draining it. The second sender unlinked it, and everything in that
batch was gone.
The liveness check now runs first, so a batch under a live lease is skipped
whatever its attempt count. The cleanup is deferred, not cancelled: once the
lease lapses, the same exhausted file is reaped on a later run.
Two tests. The first walks a batch to the final attempt and asserts a second
sender neither takes it nor deletes it, and that the events are still in it. The
second asserts an abandoned exhausted batch is still discarded once its lease
lapses, which is the over-correction to guard against. Confirmed the first fails
against the previous ordering.
288 passed, 8 skipped.
Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
Review point. The portable bundle is built with host "coding-agent", and the
build wrote that straight into PLATFORM_APPLICATION, so every portable install
sent X-Application: coding-agent.
That value is not in the platform's allowlist, so it was already being dropped
server-side. The effect was the worst of both: the wire claimed we knew the
editor, the stored event recorded that we did not, and nothing said which was
right. An absent header says the same thing honestly and costs a lookup.
HARNESS_ID stays "coding-agent". It is the PostHog-side label, it is true, and
grouping portable installs together there is useful.
Native bundles are unchanged apart from the regenerated comment. Covered by two
new build tests: portable declares no application, and each native names the
host it was generated for.
Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
CI caught what I could not check locally: `pnpm exec tsc --noEmit` fails with
error TS2353: Object literal may only specify known properties,
and 'source' does not exist in type 'SearchMemoryOptions'.
Adding `source` to SearchMemoryOptions in mem0-ts does not help here. pi-agent
resolves `mem0ai` from npm, so it typechecks against the published 3.1.8 types,
not this repo's source. The declaration still belongs in mem0-ts for the next
release; this call site needs to compile today.
Widened by exactly that one property rather than restoring `as never`, which
was the original objection: a blanket cast also disabled checking of filters,
threshold, topK and rerank on the same literal. `source` reaches the wire
through the SDK's camelToSnakeKeys spread either way.
Verified by installing the package deps and running the real gates: tsc clean,
build clean. vercel-ai-sdk typechecks clean too. Also carries the spool-test
environment fix that had not been committed in this worktree.
Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
CI runs agent-plugin-core/tests and claude-code-plugin/tests in one pytest
process. Two things only show up in that combined run, so the suites passed
locally and failed on every push.
claude-code-plugin/tests/conftest.py sets MEM0_TELEMETRY=false at import, which
is process-wide. record() then returns early and every assertion in
test_spool_delivery.py saw an empty spool — nine failures, all reported as
"recorded nothing" rather than as a disabled feature. The fixture now pins
MEM0_TELEMETRY rather than trusting whatever collected first.
The fixture also dropped telemetry/memory_core/_harness_id from sys.modules on
teardown. That conftest imports memory_core once at collection and calls
configure_harness() on it, so a later re-import got a fresh module with default
harness config and test_memory_core failed depending on collection order. The
fixture now saves and restores those entries instead of deleting them.
Verified with CI's exact command rather than the narrower path I had been
running: 266 passed, 8 skipped.
Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
The keep-both conflict resolution split a function body. Rebuilt from both
merge parents so the header-contract test and the session-start tests are each
intact.
Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
Review found the contract documented but not implemented, and one client path
missed entirely.
AsyncMemoryClient's custom-client branch still carried the old literal header
dict, so `AsyncMemoryClient(client=...)` sent no surface identity at all — the
exact asymmetry this work set out to remove.
Both custom-client branches also used a blanket headers.update(), which
overwrites. That is the one code path where an outer layer's identity can
physically be present, and it was the one path that erased it. They now
check-then-set the identity headers and append to an existing client stack,
which is what set-once and append-only were supposed to mean.
AGENTS.md claimed a plugin calling the Python SDK produces
`mem0-plugin/0.3.1, mem0-python/2.0.19`. Nothing in the repo sets the env vars
that would make that happen, so the concatenation was unreachable. Replaced with
the three ways an integration can actually declare itself, in preference order.
memory_core's comment said the backend reads X-Mem0-Source. That is only true
from the platform release shipping alongside this, and a reader would otherwise
trust it and build header-only attribution that silently does nothing — which is
how vercel-ai-sdk was written in the first cut. Corrected in all seven copies,
and the body value is what makes attribution work against either backend.
mem0-ts hardcoded SDK_VERSION = "3.1.8" while the repo already injects
__MEM0_SDK_VERSION__ via tsup, the same mechanism telemetry.ts uses. The
hardcode was correct only until the next release bump.
Dropped both `as never` casts in pi-agent. They suppressed an excess-property
error but also disabled checking of every other option at those call sites, so a
typo in filters or threshold would have compiled. SearchMemoryOptions now
declares `source` instead.
Stack truncation cut mid-identifier, leaving a fragment that parses as a real
client name. It now drops whole entries.
Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
Review found the headline fix inverted: code.install could never fire, so every
fresh install reported an upgrade and the two cohorts became indistinguishable —
strictly worse than the bug being fixed.
hook_runner reaches claim_install() only after cache_plugin_api_key() has
written `api-key` and EvidenceStore() has created `evidence.sqlite3` and its WAL
files. Asking "is the data directory empty" at that point always saw content.
The caller now snapshots emptiness at the top of the run, before anything
writes, and passes it in.
Also caught by review, all in the same file:
- claim_version_change was an unsynchronized read-modify-write, so several
concurrently starting sessions each observed the old version and each recorded
an upgrade. The first session after a version bump is exactly when a user's
open agent windows all restart together. The transition is now claimed with an
exclusive per-version sentinel.
- A crash between O_EXCL and the write left an empty marker, which disabled
every future upgrade event on that machine: claim_install saw the file and
claim_version_change could not parse it. An unparseable marker is now
repaired.
- claim_install consumed the one-shot claim even under MEM0_TELEMETRY=false, so
a user who opted out for their first sessions would never report install after
opting in.
- Existing users have an email but no key fingerprint, so the fast path always
missed and every flush paid an uncached /v1/ping/ — a 5s timeout each time for
the offline users this stack keeps citing. Legacy rows now adopt the current
key's fingerprint instead of re-resolving.
- A key that will not resolve (revoked, offline) kept attributing to the
previous account's email, which is the bug this was meant to fix. It now falls
back to the anonymous id.
- The anonymous id was never rotated, so once it had been merged into one
account it was still offered as the alias for the next one. An alias naming an
already-identified id is what could link two real people; it is now offered
once.
The gap that let this ship was that no test drove hook_runner's session-start
path — the decision was only ever tested by calling claim_install() directly on
a directory nothing had touched. Adds subprocess tests that run the real
entrypoint: fresh install, exactly-once, and an existing data dir.
62 core tests, 203 host tests.
Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
Review found that the first cut traded the duplicate-delivery bug for a worse
one, and disproved its own load-bearing safety claim by experiment.
Expiry was unreachable. _claim_parked touched the mtime on every re-claim and
_release_claim backdated to exactly now minus the stale threshold, so a file's
age hovered around 121 seconds and never approached the 7-day expiry. The
attempt count in the filename therefore bounded nothing: an undeliverable batch
(revoked key, proxy 403, oversized event) lived on disk forever, and because
spawn_flush starts a sender whenever a .sending file exists, it spawned a
detached Python process on every hook, MCP call and CLI invocation, forever.
The old code self-healed here, so this was a regression. Expiry now gates on the
attempt budget, which is the thing that actually accumulates; age stays only as
a backstop for files that never carried an attempt marker.
The attempt parser sniffed for a leading "a", which also matches a hex id like
a1234567, so a legacy telemetry-<pid>-<hex>.sending file parsed as attempt
1234567 and was deleted unsent on the first flush after upgrade — precisely the
population this PR is meant to protect. Anchored on field position instead.
The rewrite was not durable: no fsync before the rename, and _drain unlinked any
claim that parsed to zero events. A crash between write and rename left the
claim empty, and the next flush deleted it. Now fsynced, and a non-empty file
that parses to nothing is quarantined as .corrupt rather than destroyed.
read_text raises UnicodeDecodeError on a torn file, which `except OSError` does
not catch. flush() runs from a bare `finally:` in flush_worker, so the exception
also skipped the handoff cleanup and left it stuck in .running.
The per-batch rewrite's return value was discarded, so a failed rewrite let the
loop continue as though progress had been recorded — reintroducing the exact
duplicate delivery this PR exists to fix.
.partial files orphaned by a crash between write and rename matched no glob in
the module and were never cleaned up.
Also replaces the heartbeat test, which asserted `SEND_TIMEOUT * 4 <
CLAIM_STALE_SECONDS` — two constants, executing none of the code under test. It
now drives the real rewrite and watches the mtime move. New tests cover expiry
being reachable, legacy filename parsing, torn-claim quarantine, failed-rewrite
behaviour and debris sweeping.
64 core tests, 199 host tests.
Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
Two defects, one cause: the spool protocol infers ownership instead of holding
it, and never records progress.
Duplicate delivery after a partial failure. flush() posts the claim in batches of
100 and returns on the first failure, keeping the whole file. The retry then
posts every batch again, including the ones that already arrived — 150 recorded
events were delivered 250 times. Progress is now written back to the claim after
each successful batch, so a retry resumes where the send stopped and a crash
repeats at most one batch.
Duplicate delivery when two senders overlap. spool.replace(claim) is os.rename,
which preserves mtime, so a claim created after a quiet minute inherited the
spool's last-write time and looked abandoned the instant it existed. A second
sender starting while the first was still posting took it over and sent it too —
most likely at session end, when the MCP server's exit sender and the SessionEnd
flush worker both drain. Claims are now touched at claim time, and the per-batch
rewrite doubles as a lease heartbeat. _post makes one attempt with SEND_TIMEOUT
and no retry, so a heartbeat lands well inside the 120s lease; a test asserts
that margin so adding a retry loop to _post cannot silently break it.
Parked batches starved. _claim_spool only looked at parked .sending files when
no spool existed, and because sessions keep recording there usually was one — so
a batch parked by a failed send waited until the 7-day expiry deleted it unsent,
despite its own presence being what starts the sender in the first place.
flush() now drains the live spool and then parked claims in the same run, oldest
first, bounded. Expiry applies only after a genuine retry has failed, with the
attempt count carried in the filename.
A sender that gives up releases its lease rather than heartbeating on the way
out, so the next run picks the batch up promptly instead of waiting a full stale
window for a batch nobody is working on. A failing send stops the run, so one
broken connection cannot burn every parked batch's attempt budget at once.
Two existing tests asserted the old lifecycle and are updated in place, each
with a comment saying what changed.
Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
harness is set when an event is recorded; source was set when its batch was
sent. Both came from module globals that stay at "generic" and "MEM0_PLUGIN"
until telemetry.init() runs, and two processes in the pipeline never run it:
- `python3 telemetry.py`, the detached sender spawn_flush() starts at session
start, after every skill command, and when the MCP server exits. Everything it
delivered was labelled source=MEM0_PLUGIN. Only batches flush_worker.py
happened to drain got the real host.
- mcp_server.py, which records every manual search as harness=generic.
All six Python plugins ship the same files, so source could not tell any of them
apart and MCP searches from every plugin landed in one generic bucket. The
portable plugin is worse: it has no flush_worker at all, so its only sender is
the uninitialised one and 100% of its events were mislabelled.
Two changes. record() stamps source beside harness, so the sending process stops
mattering — flush() already spreads per-event properties last, so a per-event
source wins over any sender default. And the build generates core/_harness_id.py
per host, seeding both modules at import, so identity no longer depends on an
entrypoint remembering to call init(). The build already computed HARNESS_ID and
spent it only on skill templating, and bundle_drift already diffs core/
byte-for-byte, so --check catches drift for free.
Deliberately not adding MEM0_PLUGIN_HARNESS to the six manifests: they sit
outside the --sync and --check boundary, which is the property that caused this.
Also unifies two defaults that disagreed. configure_harness derived
`<host>_plugin` while telemetry.init derived `MEM0_<HOST>_PLUGIN`, so a third
value existed. It was unreachable only because hook_runner never calls flush();
moving source into record() would have made it live.
Events now carry a uuid so a resend can be collapsed.
The suite stayed green through all of this because the only tests live under one
host, behind a conftest that calls init() at import. New tests run in real
subprocesses with no init, and cover the portable plugin, which would pass a
native-only test vacuously.
Claude-Session: https://claude.ai/code/session_01C7tEmH86HAr7GoAAKCEHZb
Saket Aryan