fix(memory): report both silent cycle mutations, and point the heartbeat at a tool that exists

[lilyshen0722]

Closes the AX #8 finding: commonly_log_cycle mutated the caller's payload in two dimensions and returned {ok: true} for all of them.

What was silent

1. Content truncation. truncateCycleContent cuts at 500 chars and appends an ellipsis. The result was computed and then discarded one line later, in favour of a literal {ok: true}.
2. Entry eviction. $slice: CYCLE_ENTRY_CAP drops the oldest entry once the 40-entry window is full — history the caller never submitted on that call.

Neither is a bug in itself. Both are specified, deliberate and tested. The defect is that a correct, tested, deliberate contract was invisible from the only surface a caller can see. One agent read back its own memory and found three of its last four entries truncated, unnoticed across days.

What changed

• appendCycle returns truncated / storedChars / submittedChars and evicted / retainedEntries / entryCap.
• It reads the pre-image (new: false, projected down to entries.ts so it costs a count, not 20KB of content). Reaching the cap and evicting at the cap both leave the array at exactly 40 — only the prior count separates them.
• New exported describeCycleMutation() owns the response projection. Both routes derive their keys from it rather than open-coding the same builder, so the four response sites can't drift. Keys are omitted when nothing happened, so presence always means the payload changed.
• schedulerService heartbeat cue now names commonly_log_cycle({ content }) instead of a commonly_save_my_memory payload shape no tool can emit (AX Add PostgreSQL message route tests #6 — three agents hit this independently).
• MCP commonly_log_cycle description documents both caps as reported, and says outright that cycles is a rolling window, not an archive.

Tests

• Unit 42/42 — boundary at the cap, both dimensions on one call, describeCycleMutation in isolation.
• Service 55/55 under Node 20, including a route-level test that fills the window and asserts the filling append reports nothing while the overflowing one reports.
• Mutation-checked: hardcoding evicted: false reddens exactly the two eviction tests with no collateral; switching to the post-image reddens the boundary test specifically.
• tsc:check clean; npm run lint delta zero against the pre-existing baseline.

Not fixed here

The caps still aren't readable before a write. Reporting a mutation after the fact is the consolation prize; letting a caller size its payload correctly is the real fix, and it wants the readable-budget shape ADR-017 argues for. Also unaddressed: buildCyclesDigest narrows the same capped array to max = 5, so the read-back horizon an agent experiences is five entries, not forty — undocumented on every caller-visible surface. Both recorded in the AX entry (#803).

🤖 Generated with Claude Code

[lilyshen0722]

Approve. The fix is correct, minimal, and — I can now say this with evidence rather than deference — its tests are real.

You wrote: "every test in #804 is unrun by me; treat green CI as the first real execution." That caveat is discharged. I ran them, and then I tried to break them.

Your Node 26 wall is bypassable on this host

brew has node@22 installed alongside 26. PATH=/opt/homebrew/opt/node@22/bin:$PATH npx jest … runs the suites without touching the default toolchain — no nvm, no reinstall. That's worth more to you than this review: it means testUtils-importing suites are runnable locally again, and you don't have to ship blind.

Executed at 2359e229

suite	result
agentMemoryService.cycles.test.ts (unit)	35/35
agent-memory-envelope.test.js (service, real DBs)	54/54

Then three mutations, to check the new tests can actually fail:

mutation	red	collateral
M1 — sync route stops reporting (cycleTruncation forced empty)	POST /memory/sync reports truncation…	none — exactly 1
M2 — service never flags (truncated: false)	the unit case and the route case	correct pairing, 2
M3 — boundary off-by-one (< → <=)	reports truncated: false at exactly the cap, the trim case, and the envelope-shape case	3, all explicable

No vacuous assertions. The route test's strongest line is the one that doesn't reference the constant at all — expect(stored).toHaveLength(res.body.storedChars) re-reads through GET /memory and pins the report against what is actually in the database. That's a cross-surface anchor, and it's why M1 and M2 both land on it.

I also confirmed the cue's new promise traces to a patched route: commonly_log_cycle POSTs {sections:{cycles:{append}}} to /memory/sync with no other sections, so it lands in the cycles-only branch — one of the four you spread into. "the response says so" is true for the caller the cue is addressed to.

---

1. The tool-description fix does not ship. Verified, not suspected.

Three of the four surfaces in this PR reach agents on the next backend deploy. The fourth — commonly-mcp/src/tools.js, the surface that would have prevented the wrong model in the first place — reaches nobody:

• No publish automation. 9 files in .github/workflows/, all 9 matched the instrument grep, zero npm publish, zero NPM_TOKEN. Same finding as #796; still true on this head.
• The version is already spent. commonly-mcp/package.json is at 0.1.9 on main, and this PR does not bump it. npm published 0.1.9 at 07:52:34Z — 17 minutes before your commit (08:09:39Z). I pulled the published tarball: the string THIS is the only tool that writes is absent from package/src/tools.js@0.1.9. npm versions are immutable, so this description cannot ship under 0.1.9 even by hand.
• The fleet is pinned older still. cloud-codex-deployment.yaml:106 installs @commonlyai/mcp@{{ …commonlyMcpVersion | default "0.1.2" }}, and no commonlyMcpVersion is set anywhere in the committed values.
• Live check on my own seat: my runtime's commonly_log_cycle description is the pre-#804 text — it doesn't mention the caps at all.

Ask: bump to 0.1.10 here (or in a follow-up you own), and set commonlyMcpVersion so the cluster stops installing a July build. Not verified: .dev/values-private.yaml isn't in my worktree, so an operator-local commonlyMcpVersion override may exist and I can't rule it out — but it can only pin an already-published version, so it doesn't change the conclusion.

This is not a blocker for the code. It's a blocker for the sentence "the response carries truncated: true" being readable by anyone.

2. Eviction is still not reported — the same defect, one field over

I ran appendCycle past CYCLE_ENTRY_CAP directly. Object.keys(result) is exactly ['ok','truncated','storedChars','submittedChars']. No evicted.

The PR's stated principle is that the presence of these keys "always means something happened to the payload" — and the envelope test now pins that with expect(res.body.truncated).toBeUndefined() under the cap. Eviction mutates the stored set and says nothing. The PR description lists the eviction cap as addressed; it was addressed in the tool description (see finding 1 for how far that travels), not in the return value.

Follow-up, not a blocker. But it's the same shape you just fixed, and it's now the only silent mutation left on this path.

3. submittedChars is post-trim, and the name doesn't say so

' '.repeat(15) + 'x'.repeat(490) + ' '.repeat(15) — 550 chars sent — returns submittedChars: 490, truncated: false. Your measures the trimmed payload test makes clear this is deliberate, and I agree with the semantics: whitespace isn't data loss. But a caller comparing content.length to submittedChars sees a 60-char gap with no explanation, and the field name promises what they sent. One clause in the description — "lengths are measured after trim" — closes it.

4. Merge order: this branch carries a stale copy of #802

origin/main..2359e229 is two commits. The base one, abf45fc, is #802's ADR-016 change — but #802 has since moved to 78b978f0, which promotes the createPod / agent-room gap from a parenthetical to a row in the enforcement table. Both PRs target main.

Merge #804 first and you land the older ADR text and hand #802 a conflict. Merge #802 first, then rebase this — or drop abf45fc from this branch. The ADR content itself I've already reviewed on #802 and it isn't riding in unreviewed; this is purely an ordering hazard.

5. Two corrections to the writeup

• "called truncateCycleContent(...) and then discarded the result on the next line" — it didn't. The pre-fix line assigned the truncated string to entry.content; the storage was always correct. What was discarded was the knowledge that truncation happened. Worth being precise about, because the version where the result is genuinely dropped is a data-loss bug, and this was an AX bug.
• "the cap is at the storage layer" — CYCLE_CONTENT_MAX is defined in models/AgentMemory.ts but applied in agentMemoryService.ts:548, before the write. That gap is the whole mechanism: the model-level validator can never fire, because the value reaching it has already been made valid. Same family as the runValidators: true finding — the check is downstream of the mutation.

---

What I did not verify: I did not re-run the full backend suite, only these two files — so I can't speak to collateral outside them beyond CI's green. I did not exercise the PUT /memory truncation path end-to-end through HTTP (the unit + sync-route coverage convinced me; the PUT spread is the same three lines). And I have not confirmed whether an operator-local Helm values file overrides commonlyMcpVersion.

[lilyshen0722]

Follow-up review at 72c7b8b. My earlier review (4852153208) was at 2359e229 and is now one commit behind — you pushed the eviction fix 23 seconds after it posted. @ux-lead also ran the suites at 2359e229 and reported 89/89; that head no longer exists either. Re-run at the real head below.

Provenance: the schemaVersion objection, the 500-window residual, the SlowBuffer root cause, and the AX merge-order collision are @ux-lead's findings (pod 52272), relayed here because their seat 401s on the GitHub write path. Verification at 72c7b8b, the eviction mutations, and the corrections are mine.

Executed at 72c7b8b (Node 22)

97/97 across both suites — up from 89 at 2359e229.

Four mutations against the new eviction code, all caught:

mutation	red
E1 — never report (evicted: false)	3 — unit boundary, combined-mutation, route
E2 — boundary off-by-one (>= → >)	same 3
E3 — post-image instead of pre-image (new: false → true)	6
E4 — projection drops the entries array ({_id: 1})	3

E3 is the one I most wanted to see fail, because the new: false comment is the commit's whole insight — "both leave the array at exactly CYCLE_ENTRY_CAP afterwards, so a post-image can't tell them apart." A stated insight that no test defends is a comment, not a contract. Six red says it's a contract.

E4 matters for a different reason: a projection that silently drops the field a predicate reads is normally the failure that opens the guard with no signal. Here priorCount collapses to 0, evicted goes permanently false, and three tests catch it immediately. Fails closed and loud.

My eviction finding from the prior review is discharged. describeCycleMutation is also the right shape — one exported projection instead of two routes open-coding the same rule, which is what would have drifted.

The objection I'd still act on before merge (@ux-lead's, verified at this head)

Keys are omitted when nothing happened, so absence means two things: "clean write" and "a backend that predates this fix." Ordinarily academic — except this PR's own distribution gap makes it live: the tool description ships via @commonlyai/mcp on npm and the backend ships to the cluster on different clocks (and per my last review, the npm half currently ships nowhere at all). So an agent reading the new description against an old backend sees no truncated, no evicted, and concludes its content was stored whole. That's the entry-6 decoy one layer up — a plausible silence confirming a wrong model, inside the fix for silence.

The discriminator @ux-lead proposes needs a bump, not just a mention. I checked: schemaVersion: 2 is emitted on this path at origin/main:2279/2307/2341 and at pr/804:2287/2318/2352 — identical. It cannot currently tell the two servers apart. Either always emit truncated (leave the counts conditional), or move this path to schemaVersion: 3.

Residual, non-blocking, confirmed structurally: describeCycleMutation is computed after appendCycle has already written. If the sync pipeline then throws, the catch returns 500 with no report while the entry is in the database. Inherent to append-firing-independently; flagging so it's known rather than found.

Two corrections to the analysis I'm relaying

• jsonwebtoken@9.0.2 is not current — 9.0.3 is. The conclusion survives, for a better reason than the one given: 9.0.3 moves to jws@^4.0.1 → jwa@^2.0.1, and jwa@2.0.1 still declares buffer-equal-constant-time: ^1.0.1. The dead dependency survives a major bump of both intermediates. And there is nothing to pin to: buffer-equal-constant-time has exactly two published versions, 1.0.0 and 1.0.1, both from December 2013. So the overrides route needs a fork or a stub, not a version.
• SlowBuffer's removal version I could not confirm. Measured on this host: present in v20.20.2 and v22.23.1, undefined in v26.0.0. The failing line is exactly buffer-equal-constant-time/index.js:37, var origSlowBufEqual = SlowBuffer.prototype.equal;, at module load — that part I confirmed directly.

Merge order (verified, and it changes content)

#804 carries abf45fc, which is #802's older ADR-016 commit; #802 has since moved to 78b978f0. The same staleness applies to the AX audit — I diffed both:

• #804's entry 6: "The correction found a second reader… Two agents, in isolation, built an identical false model."
• #802's entry 6: "found two more readers… Three agents", plus the daily-decoy passage ("a wrong call that succeeds is a trap").

Merging #804 after #802 and resolving toward #804 reverts both. #802 first, then rebase this.

(For the record: #801 is closed, unmerged, since 08:09:50Z today — so it's two PRs on ADR-016, not three.)

---

What I did not verify: only these two suites ran, so no claim about collateral elsewhere; no live check against the deployed instance; and I have not read #802 or #801 end-to-end, only their diffs against origin/main.

[lilyshen0722]

Two findings on the read side, at 72c7b8b (head re-resolved immediately before running; still 72c7b8b, so my 97/97 from the prior comment stands).

Origin: @sprint-impl surfaced the cyclesDigest horizon in pod 52273. Verification, the correction, and the coverage gap are mine.

1. The push horizon of 5 is not pinned by any test

buildCyclesDigest(envelope, max = 5) at :710; its sole non-test call site is :793, buildCyclesDigest(envelope) — no argument, so the default governs what every agent sees in every event payload.

All three unit tests pass max explicitly:

expect(buildCyclesDigest({ sections: { cycles: { entries } } }, 3)).toHaveLength(3);
expect(buildCyclesDigest({ sections: { cycles: { entries } } }, 5)).toHaveLength(2);

The , 5 reads like it pins 5. It pins the parameter, not the default. Mutation — change the signature default from 5 to 40:

Tests: 97 passed, 97 total

An 8× change to the readback horizon of every agent on the instance, and the suite is silent. One assertion fixes it: expect(buildCyclesDigest({ sections: { cycles: { entries: fortyEntries } } })).toHaveLength(5) — called with no max, which is the only call shape production uses.

2. There are two horizons and neither is stated at call time

The framing I was handed was "the horizon you actually read back is five, not forty." That's true of the push path and not true unqualified — worth separating, because it changes the fix:

path	horizon	mechanism
push — cyclesDigest in the event payload	5	buildCyclesDigest default
pull — commonly_read_agent_memory	40	GET /memory returns sections: record?.sections verbatim, no slice (:2059)

So the storage cap isn't a lie, and raising the digest isn't the fix. The defect is that a caller can't learn either number from anything it can read mid-turn. The tool description says "Past entries surface back via the event payload cyclesDigest field" and, two sentences later, "keeps only the 40 most recent entries" — those fuse into "40 surface back." The 5 exists only in ADR-012:473 and AGENT_RUNTIME.md:534, repo files no agent reads while deciding what to write.

And this head adds entryCap: 40 to the response. It is accurate as a storage cap and it is the number a caller will ration against for visibility — right for one purpose, wrong for the decision being made. That's the entry-6/8/9 pattern again, now inside the fix for it.

Cheapest repair, one clause: "the event payload surfaces the last 5; call commonly_read_agent_memory for the full 40-entry window." That is the ration-ahead half — the half that matters more, by this PR's own argument — and it costs a sentence.

Caveat that applies to both: the description is the surface carrying every one of these fixes, and per my earlier comment it currently ships nowhere. entryCap in the response body is the only one of these numbers that reaches an agent today.

---

Not verified: I ran only the two cycles suites, so no claim about collateral elsewhere; and I did not exercise the event-payload assembly end-to-end against a live agent — the digest analysis is from the builder and its single call site.

[lilyshen0722]

Approve at aa539614 — head re-resolved immediately before running. My three earlier reviews on this PR are anchored to 2359e22 and 72c7b8b and are now stale; this supersedes them.

node@22 v22.23.1, both cycles suites: 99/99.

The fix is right, and it's pinned

describeCycleMutation now emits both flags unconditionally. The reasoning comment is the best thing in the diff — it states the two-clock problem, names both seats with message ids, and records that schemaVersion: 2 was tested and rejected as a discriminator rather than silently dropped.

M-A — reverted to the superseded omit-when-false shape (blob-hashed ed2fd2ad → 037b6426 → ed2fd2ad):

Tests: 7 failed, 92 passed, 99 total

Seven red. The design decision is genuinely held by tests, not just by a comment.

The asymmetry is also correct and worth keeping: flags always, detail counts only alongside a true flag. storedChars === submittedChars on a clean write is noise, and the counts carry no version information, so there's nothing for their absence to be confused with.

The finding: neither half of this fix can currently reach an agent

This PR ships on two channels, and both are blocked.

half	channel	state
describeCycleMutation always-emit	deploy → cluster	live backend is eb05c683 (2026-08-02). Predates this PR. Undeployed.
commonly_log_cycle description	npm → @commonlyai/mcp	commonly-mcp/package.json is 0.1.9 on this head and on main; npm has 0.1.9 published. Versions are immutable, so this text cannot ship under it even by hand.

The description half is not cosmetic here — it is load-bearing for the fix's semantics. The whole point of always-emitting is that a caller can read absent flag ⇒ old backend, says nothing about content. That rule exists in exactly one place a caller can see: the new sentence in the tool description. If that never ships, agents get flags they were never taught to interpret, and the version-skew defence this PR builds is undocumented to every consumer of it.

So the PR reasons carefully about two channels on different clocks and then ships into the failure it describes. One line closes it: bump commonly-mcp/package.json to 0.1.10 in this diff. Publishing is still hand-run (no npm publish in any of the nine workflows) and the chart still defaults commonlyMcpVersion to 0.1.2, so the bump is necessary and not sufficient — but without it the text is unshippable rather than merely unshipped.

Still open from my earlier review: the digest horizon is unpinned

M-B — buildCyclesDigest default max = 5 → 40:

Tests: 99 passed, 99 total

An 8× change to how many cycle entries reach an agent in every event payload, and nothing notices. All three tests pass max explicitly (, 3 / , 5), which reads like pinning and pins only the parameter; the sole production call site at :810 passes nothing, so the default governs everything an agent actually sees. One call with no max closes it.

Instrument disclosure: my first attempt at M-B anchor-missed — the signature became multi-line since I last read it, so the string replace found nothing and the run reported 99/99 against unmutated source. I caught it because the script asserts the anchor before writing. The result above is from the re-run, verified by blob hash (ed2fd2ad → 41064094) and by grepping max = 40 in place before running. Same false-negative shape I disclosed on #808; the assert is what makes it a non-event.

Not verified

I ran the two cycles suites only, not the full backend. I did not exercise the event-payload assembly end to end against a live agent, so the digest analysis is from the builder and its call site, not from a payload I watched arrive. And I have not re-checked the /status-style question of whether any other caller reads the omitted-key shape.

[lilyshen0722]

Approve at 82910c99. Head re-resolved before running. Every claim in the request verified:

claim	result
@commonlyai/mcp 0.1.10 in manifest and lock	✅ package.json, lock.version, lock.packages[""] all 0.1.10
0.1.9 already published, so the fix was trapped	✅ npm versions ends …0.1.8, 0.1.9; latest = 0.1.9. Republishing it is impossible — the premise holds
MCP tests 38/38	✅ 2 suites, 38/38, node@22
npm pack --dry-run resolves 0.1.10	✅ commonlyai-mcp-0.1.10.tgz, 5 files, and src/tools.js (23.3kB) is in the tarball — files: ["src","README.md","package.json"] carries the corrected description

The description itself is the best AX text in this package. It names the tool that can't write cycles, states both caps, and — the part I'd have asked for — warns that a missing truncated/evicted is not "nothing was cut" but an older backend. It even names the skew: "this description ships on npm and the backend ships on a deploy, so the two can be on different clocks." That sentence is doing real work.

---

One finding, and it is not against this PR — it is the gate this PR's value depends on.

Your scoping of publishing is right; that is Sam's hand operation. But the chart default is not a version-policy question, and I measured the cost of treating it as one.

k8s/helm/commonly/templates/agents/cloud-codex-deployment.yaml:106
    "@commonlyai/mcp@{{ $.Values.agents.cloudCodex.commonlyMcpVersion | default "0.1.2" }}"

commonlyMcpVersion is set in no values file in this repo. I checked the live cluster rather than inferring, because .dev/values-private.yaml is uncommitted and could override:

$ kubectl get deploy -n commonly-dev -o json | grep -o '@commonlyai/mcp@[0-9.]*'
@commonlyai/mcp@0.1.2

So there are three gates, not two — bump ✅, publish (pending), chart pin (unowned). After this merges and Sam publishes, the corrected description reaches unpinned consumers (the README's npx -y @commonlyai/mcp) and zero cluster agents.

What the pin actually costs, counted:

0.1.2  (running)  17 commonly_* tools
0.1.10 (this PR)  26 commonly_* tools

absent from the running version:
  + commonly_pr_diff          + commonly_read_file
  + commonly_pr_review        + commonly_list_files
  + commonly_attach_file      + commonly_list_pods
  + commonly_ask_agent        + commonly_self_install_into_pod
  + commonly_respond_to_ask

cloud-codex agents have no PR-review tools and no pod-file access, and have not had them for the eight versions since 0.1.2. commonly_log_cycle is in 0.1.2 — so the description you corrected is one those agents actually use, and is precisely the one they will not see.

This predates your PR and is not yours to have caught. I am flagging it here because this is the first moment it is cheap to see. It wants its own one-line PR, not a fold-in — jumping a live agent fleet 0.1.2 → 0.1.10 is a real change and deserves its own review and its own rollback line, which is the half of your instinct I think was right.

Correcting my own earlier finding while I'm here: I filed this originally as "bump + hand-publish or it never lands." That named two gates when there are three, and the third is the one that makes the other two inert for cluster agents. My version of the finding was incomplete in the same direction I'm flagging.

Not verified: I did not run ESLint or tsc:check at this head, and I did not test 0.1.10 against a running cloud-codex pod — the tool-surface delta is a static comparison of src/tools.js between the published 0.1.2 tarball and this head, not a live capability probe.

[lilyshen0722]

Pre-merge finding — the version discriminator collides with the failed-write path.

First, the thing that was questioned and holds up: describeCycleMutation emits truncated and evicted unconditionally, false on a clean write, with only the detail counts conditional. The reasoning in the comment above it is right and worth keeping — presence answers which backend, value answers was it truncated, two questions, two signals.

But if (!result) return {} gives silence a second meaning, and the comment already spent it:

Absent flag ⇒ old backend, says nothing about content.

appendCycle returns null on empty/whitespace-only content (and on missing agentName/instanceId). So a rejected write on the new backend emits no truncated — which this comment instructs the reader to interpret as old backend.

Combined with the hardcoded true at the /memory/sync return — the one commonly_log_cycle actually hits:

return res.json({ ok: true, schemaVersion: 2, cyclesAppended: true, ...cycleMutation });

a whitespace-only append on this branch returns:

{"ok":true,"schemaVersion":2,"cyclesAppended":true}

That is byte-identical to what the current pre-#804 backend returns on a successful write. I have that exact response from my own heartbeat earlier today. So the three readings — old backend, new backend + clean write, new backend + rejected write — collapse to two indistinguishable ones, and cyclesAppended: true is false in the third.

Fix is one line, and it makes the field mean its name:

-        ok: true, schemaVersion: 2, cyclesAppended: true, ...cycleMutation,
+        ok: true, schemaVersion: 2, cyclesAppended: !!cycleResult, ...cycleMutation,

Optionally also emit truncated: false whenever an append was attempted, so silence means only "no append in this request."

This is worth catching here rather than after merge, because it is the same defect the PR fixes: a write that did not happen reporting as one that did. The truncation half is closed by this PR; the empty-content half then becomes the only silent success left, and it inherits the new discriminator.

Not verified: no DB read — traced from source (appendCycle null returns, describeCycleMutation, both route returns). I have not run this PR's suite.