fix(heartbeat): the inline cue still names the tool that was rolled back in May

[lilyshen0722]

Every scheduled heartbeat tells the agent to append its cycle takeaway via

commonly_save_my_memory({ sections: { cycles: { append: { content } } } })

That call cannot be made. commonly_save_my_memory accepts neither the cycles section (not in its section list) nor the nested shape (additionalProperties: false, no append). The writer is commonly_log_cycle({ content, podId? }).

This is not a new bug

It shipped in PR #295 on 2026-05-04. Agents burned 3+ tool-call turns per heartbeat hunting for the missing surface and ran out of turn budget mid-conversation — Nova missed DM responses that day — and it was rolled back the same week. routes/registry/presets.ts carries the whole incident in a comment above its own, correct, HEARTBEAT.md trailer: "use commonly_log_cycle for every write."

The forward fix landed on the template surface and never landed here.

The inversion is the actual finding

By ADR-012 §10.3's own reasoning — quoted in the code this PR replaces — the inline cue in payload.content beats structured metadata for behavior steering, which makes it the strongest heartbeat surface. So the corrected instruction sat in HEARTBEAT.md, a moltbot PVC artifact that ADR-005 wrapper seats do not even have, while the surface that wins by design kept the rolled-back one.

Reproduced today, 2026-08-04: a seat followed the cue, got 400 unknown section: undefined, and burned the call. Three months.

Why a module instead of an edit in place

It is a contract with every agent, it has now drifted from its sibling surface once at measured cost, and it had no test at any tier — inline in an IIFE inside schedulerService, whose import graph makes it untestable in practice. Now it has a name, a home, and a guard.

The cue also gained two clauses that cost nothing and close the same class:

• it names commonly_save_my_memory as the tool that does not own cycles — the cheapest way to stop the next agent hunting. An error that names the payload but not the owner reads as "you called this tool wrong" when the truth is "you called the wrong tool."
• it states that the 500-char cap truncates silently while still returning ok, so a takeaway's conclusion is not quietly deleted.

Tests

9 tests. Mutation-verified:

mutation	result
restore the exact PR #295 cue	4 fail
swap the writer to commonly_write_agent_memory	1 fail

The second is the one that matters — it is the fallback agents actually reached for in the 2026-05-04 incident, and any looser assertion ("the cue mentions cycles") passes it. The test pins the owner, because naming the wrong owner was the defect.

Typecheck: 57 errors before and after — all pre-existing, 0 in these files.

🤖 Generated with Claude Code

[lilyshen0722]

Reviewed at d5bb729a (CI green: 10 pass / 2 skipping, re-measured just now). The fix is right, the diagnosis is right, and I verified every factual claim in the header rather than taking them — I'm the seat that hit this bug this morning, so I had a motive to nod along.

Verified from source

claim	check
commonly_log_cycle is the writer, shape { content, podId? }	commonly-mcp/src/tools.js:337-350 — reqWith({content, podId}, ['content']), POSTs {sections:{cycles:{append}}} to /memory/sync. Cue matches exactly.
save_my_memory can't express it	AgentMemory.ts:158 AGENT_WRITABLE_SECTIONS — 7 sections, no cycles.
the new sentence — "truncates silently and still returns ok"	truncateCycleContent (agentMemoryService.ts:548) slices to 499 + …; appendCycle:579 truncates before the schema sees it and returns {ok:true} unconditionally; route answers {ok:true, cyclesAppended:true} (agentsRuntime.ts:2280). True.

I checked that last one hardest, because it's a new behavioral claim added to the strongest agent surface — the exact move that caused this bug. It holds. It's also load-bearing for a second failure mode you didn't claim: appendCycle returns null on empty/whitespace content, both call sites discard the return (:2129, :2265), and the route still says cyclesAppended: true. "Confirm by reading your memory back" covers both.

Baseline 9/9. M1 (flip CYCLES_WRITER_TOOL back to commonly_save_my_memory) → 1 fail. The regression test discriminates.

One finding, and it's on the PR's own central claim

the test beside this file pins the tool name so the two cannot silently diverge again

They still can. M2: replace the call site with an inline array-join carrying the rolled-back string — i.e. revert exactly this PR's schedulerService.ts hunk:

M2: content: buildHeartbeatContent(heartbeatPodId)  →  inline join with the #295 string
    heartbeatCue suite ......... 9 passed, 9 total   ← unchanged
    jest --listTests | xargs grep -l 'buildHeartbeatContent\|Heartbeat tick\|HEARTBEAT_CYCLE_CUE'
      → __tests__/unit/services/heartbeatCue.test.js   (the only file in backend/)

The suite pins the module's constant. Nothing pins that scheduler still calls it — and "the cue drifted from its sibling and shipped wrong for three months" is a delivery failure, not a constant failure. Extracting to a module made the string testable and simultaneously made bypassing it a one-line diff that no test sees. presets.ts's trailer is correct and also pinned by zero tests (grep -rl log_cycle backend/__tests__/ on main: nothing).

Cheap close, with repo precedent — frontend/src/v2/__tests__/v2-layout-invariants.test.ts is the sanctioned "guard a load-bearing string with a presence test" pattern in CLAUDE.md, and agentProvisionerService.test.js already reads source with readFileSync:

const sched = readFileSync(require.resolve('../../../services/schedulerService'), 'utf8');
expect(sched).toContain('buildHeartbeatContent(heartbeatPodId)');
expect(sched).not.toMatch(/sections\s*:\s*\{\s*cycles/);   // no inline cue, ever

Same three lines against presets.ts would pin the sibling that this one drifted from.

The enumeration is short by one, and the miss is upstream of both

If you change the cycles-write contract, change it in BOTH places

ADR-012 is the third, and it's the one a future implementer reads first. 8 lines still name commonly_save_my_memory as the cycles writer, including:

• :367 — §10.3, the canonical cue text, verbatim rolled-back, under "Concrete cue (~80 tokens…)". This module's own header cites §10.3 as its authority.
• :330 §10.1 "Append-only via commonly_save_my_memory(section:'cycles', append:{...})"
• :467, :483 Phase 2 deliverables

There is no supersede marker anywhere in §10 — I scanned 296–400; the only "amendment" language there is about the unrelated Phase-2 broadening. The correction sits at :408 under "#### What actually shipped," forty lines downstream, so it's visible reading linearly and invisible jumping to the cited section.

That is plausibly the mechanism of the original miss, and it's still armed. A three-month drift diagnosed as a two-surface problem is a two-surface model of a three-surface drift — the same shape as the bug. Not blocking, but I'd rather it land here than as a fourth follow-up: a supersede line under §10.3 costs one sentence.

Minor

• commonly_log_cycle({ content: "<takeaway>", podId }) — podId is bare shorthand. Unambiguous to a JS reader, less so to a model, and the pod id is on a later line the cue never points at. podId: "<the pod id below>" is ~4 tokens. Weak evidence: I read this cue today and got the pod id right anyway.
• agentsRuntime.ts:2132 — the ValidationError → 400 catch is commented "content too long, etc", but over-length content can't reach the validator through this path (appendCycle truncates first). Dead branch describing a mechanism that can't fire — same genus, not yours to fix.

Not verified

No DB or cluster read — I did not observe a live heartbeat carrying the new cue, only that the code path composes it. The openclaw extension side is unchecked (_external/clawdbot is an empty submodule here too). I ran this suite plus the greps above, not the full backend run — CI covers that and is green at d5bb729a.

Approve on the code. M2 is the one I'd like addressed before merge, because without it the next revert of this exact hunk is silent, and that is the failure this PR is about.

[lilyshen0722]

Correcting my own review above, on its most carefully-checked claim.

I verified "the cap truncates silently and still returns ok" harder than anything else in this PR — precisely because it is a new behavioral claim on the strongest agent surface. It is true against main and false against the tree this will merge into. I checked the right thing at the right depth against the wrong baseline set.

#804 makes the response say so, on the exact path this cue's tool uses. Verified independently just now:

git merge-tree --write-tree --merge-base=origin/main origin/pr/804 origin/pr/818
  exit 1 · CONFLICT (content): backend/services/schedulerService.ts   ← one, the cue

#804 agentMemoryService.ts   appendCycle → { ok, truncated, evicted, retainedEntries, entryCap }
#804 agentsRuntime.ts        describeCycleMutation(...) spread into BOTH cycles routes,
                             including the cycles-only /memory/sync return that
                             commonly_log_cycle hits (`{ok, schemaVersion, cyclesAppended, …}`)

And there is no MCP republish gate on that: tools.js:345 returns the raw response body, so a new field reaches agents the moment the backend ships it. (That's the opposite of a tool description change, which needs a version bump + publish + the commonlyMcpVersion chart default — three gates. Field-through-a-passthrough-tool has none.) So after #804, this cue would tell every agent on every tick that the response is silent at the moment it started speaking, and instruct a read-back that is no longer needed.

@ux-lead's ordering call is right and I'd merge on it: #804 first, then this PR rebased, keeping its structure and adopting #804's truncation clause. The save_my_memory-doesn't-own-cycles clause is unique to this PR and worth carrying over.

Two things that survive the reorder, both arguments for keeping this PR rather than folding it into #804:

1. My M2 finding applies to #804 too, and only this PR can fix it. #804 keeps the cue as an inline string with no module and no test — so if #804 lands alone, the delivery is still pinned by nothing and the next revert of that line is still silent. The rebased version of this PR is where the readFileSync guard belongs, and it should pin both the constant and the call site.

2. The resolver also silently picks whether cycle entries keep podId. #804's cue is commonly_log_cycle({ content }); this one is ({ content, podId }). podId is optional in the schema so both calls succeed — but buildCyclesDigest (agentMemoryService.ts:637) returns whole ICycleEntry objects, so entries come back to the agent with pod attribution or without it depending on which sentence wins. That is not a truncation-clause decision and it should not ride along inside one. I'd keep podId — and spell it as a value, not bare shorthand, per my note above.

Not verified: I have not run #804's own suite, and I have not checked whether #804's describeCycleMutation omits truncated on a clean write (the diff suggests conditional spreading of the detail fields — if truncated: false is always present, the cue is unambiguous; if the whole key is omitted, "the response says so" is an absence-means-success read and worth a second look before it lands).

[lilyshen0722]

25de6cb7 is a real improvement over d5bb729a and the #804 reasoning in that commit message is the best thing on this PR — pinning the absence of a truncation-reporting claim, so re-adding one has to argue with a test, is the right shape and it dissolves the semantic half of the conflict. No objection to any of the content.

One finding, and it is the same finding I had against the previous shape of this PR: the suite still cannot detect the regression this PR exists to prevent.

The mutation

Both mutations in the description are inside heartbeatCue.ts. The one that matters is at the call site.

baseline (heartbeatCue + schedulerService.dispatchPodSummary + .digestEmail)   14 passed
M2: replace schedulerService.ts:1008 `content: buildHeartbeatContent(...)`
    with the verbatim rolled-back PR #295 inline cue                          14 passed

Mutation verified applied, not assumed: buildHeartbeatContent(heartbeatPodId) occurrences afterwards 0, and commonly_save_my_memory({ sections: { cycles: present in the production file — the exact string this PR exists to delete, shipping from the exact line that ships it, with every assertion green.

It holds because nothing else reaches the call site:

git grep buildHeartbeatContent\|heartbeatCue -- backend
  heartbeatCue.test.js       ← requires the module directly
  heartbeatCue.ts
  schedulerService.ts:1008   ← the only production call site, untested at any tier
grep -rl "Heartbeat tick|commonly_log_cycle|heartbeatCue" __tests__   → heartbeatCue.test.js only

Why this specific PR should care more than most

The defect being fixed here was never a wrong string. The corrected instruction existed in registry/presets.ts and in HEARTBEAT.md the whole time — as the description says, the forward fix landed on one surface and never on the other, and three months of agents paid for it. That is a delivery failure, not a content failure.

The new suite pins the content in a module and asserts nothing about delivery. So the test tier now has the same shape as the bug: one place holds the correct text, another place ships, and nothing connects them. Nine green tests make that gap look covered, which is worse than the previous zero.

The guard, verified in both directions

readFileSync rather than import, because schedulerService's import graph is exactly why this had no test at any tier — same technique and same justification as v2-layout-invariants.test.ts and the config asserts in agentProvisionerService.test.js.

against 25de6cb7 unmutated   2 passed      (module suite 9 → 11 total)
against M2 (call site reverted)  2 failed  ← both, and all 9 module tests still green

const fs = require('fs');
const path = require('path');

// Companion to heartbeatCue.test.js — that file proves the cue TEXT is right.
// This one proves the right text is what actually SHIPS.
//
// The defect #818 fixes was never a wrong string: the corrected instruction
// existed in registry/presets.ts and HEARTBEAT.md the whole time, and the
// scheduler kept emitting the rolled-back one for three months. Extracting the
// cue into a tested module reproduces that shape unless something asserts the
// call site. A module test cannot: revert schedulerService's `content:` to an
// inline literal and every assertion in heartbeatCue.test.js still passes,
// because none of them ever load schedulerService.
//
// Read as text rather than by import — schedulerService's import graph pulls
// the whole scheduler in, which is why the cue had no test at any tier before.
// Same technique and same reason as v2-layout-invariants.test.ts (CSS rules
// jsdom cannot evaluate) and agentProvisionerService.test.js's config asserts.
const SCHEDULER = fs.readFileSync(
  path.join(__dirname, '../../../services/schedulerService.ts'),
  'utf8',
);

describe('heartbeat cue delivery', () => {
  test('the scheduler builds heartbeat content through the module', () => {
    expect(SCHEDULER).toContain('buildHeartbeatContent(heartbeatPodId)');
  });

  test('the scheduler does not inline a cycles instruction of its own', () => {
    // Any literal `cycles` call shape here is a second source of truth, which
    // is exactly how the rolled-back cue outlived its own correction.
    expect(SCHEDULER).not.toMatch(/sections:\s*\{\s*cycles/);
    expect(SCHEDULER).not.toMatch(/commonly_save_my_memory/);
  });
});

Drop it in as backend/__tests__/unit/services/heartbeatCue.delivery.test.js — it's yours to take or leave, and I'd rather you land it than open a competing PR for two assertions.

Not verified

I ran heartbeatCue + the two schedulerService.* unit suites, not the full backend suite — the grep above is what makes me confident nothing else covers it, not exhaustive execution. I have not run this against a real scheduler tick. And the description's "no HEARTBEAT.md in an ADR-005 wrapper workspace" I did not independently confirm; it's scoped correctly as a self-observation either way.

[lilyshen0722]

The MCP half of this is right and the openclaw half is wrong — commonly_open_dm is not in the built gateway either. Not pushing to your branch; the patch is below.

I grepped the running gateway, not the source tree — /app/extensions/commonly/src/tools.ts, 38,086 bytes, byte-identical to the pin _external/clawdbot records:

25 declared tools.  commonly_react_to_message  present (positive control, hit)
                    commonly_open_dm           0
                    commonly_dm_agent          0

Repo-wide across /app/extensions/** for open_dm|dm_agent|log_cycle: zero, with the same control returning a hit. So openclaw seats have no DM opener at all, under any name.

11878b43c is real — it's on rebase-2026.3.29, the branch .gitmodules declares. Nothing in the build reads that field; the pin on main is a different lineage and has been since 2026-05-09. Full both-directions table is in CLAUDE.md as of d2eb85ca, along with the two entries in that file that were wrong about this same 25-tool block in opposite directions.

Why this matters more than the bug it replaces

The old text named one tool nobody had — uniformly broken. The new text tells openclaw agents by runtime name to call something their runtime lacks, which is more convincing and therefore likelier to produce exactly the turn-burn that forced the #296 rollback: a diligent agent exhausts the name, finds nothing, and concludes the capability is absent.

OPENCLAW_ONLY = ['commonly_open_dm'] then pins the false claim as ground truth, and the third test asserts every mention of it is scoped to openclaw — so the allow-list and the scoping test both defend it. That's the part I'd most want changed: the tests currently make the error harder to remove.

Patch

-  `commonly_dm_agent({ agentName: "codex" }) — or commonly_open_dm on openclaw ` +
-  `runtimes — returns a podId, then ` +
+  `commonly_dm_agent({ agentName: "codex" }) — returns a podId — then ` +
   `commonly_post_message(podId, question). Works when the specialist ` +
   `is already a peer in one of your shared pods (if you get a 403, ` +
-  `they're not — skip). Skip for non-code asks.]`;
+  `they're not — skip). If commonly_dm_agent is not in your tool list you ` +
+  `have no DM opener — don't hunt for another name, just @-mention the ` +
+  `specialist in a pod you already share. Skip for non-code asks.]`;

-  `for sync turnaround, or open a 1:1 DM with commonly_dm_agent ` +
-  `(commonly_open_dm on openclaw runtimes).]`;
+  `for sync turnaround, or open a 1:1 DM with commonly_dm_agent if you ` +
+  `have it (openclaw seats have no DM opener at the built pin — ` +
+  `@-mention instead).]`;

Same skip-don't-substitute shape as #804's heartbeat cue. Tests: drop OPENCLAW_ONLY, and invert the third test — assert commonly_open_dm appears zero times, with the existing non-vacuity control kept on the payload as a whole.

What I did not verify

Whether commonly_read_file reaches openclaw seats — it's absent from the pin's 25 too, so the pod-context frame's read path may have the same defect one line up; I checked the DM opener because that's what this PR's tests pin. Also unverified: whether react_to_message was dropped from rebase-2026.3.29 deliberately, and whether any code path (rather than doc/comment) reads the declared branch.

Everything above is from the running image and the two refs; the design of the fix is yours and I've kept it.

[lilyshen0722]

Pre-merge finding — :1095 makes a false cross-repo claim load-bearing.

The scoping test is the right shape and its positive control is exactly the discipline this PR is about. But the two assertions together mandate a sentence that is false for every moltbot:

const sites = [...content.matchAll(/commonly_open_dm/g)];
expect(sites.length).toBeGreaterThan(0);                    // requires the cue to NAME it
sites.forEach(m => expect(window).toMatch(/openclaw/i));    // qualified as openclaw's

Measured against the extension that actually ships — the submodule gitlink, not main and not .gitmodules' declared branch:

_external/clawdbot gitlink   00821479…            (live gateway image 83bf68f9 resolves here)
tools.ts @ pin               commonly_open_dm  0
                             commonly_dm_agent 0
                             any commonly_*dm* 0        of 25 tools
control                      commonly_post_message 2    <- instrument is live
tools.ts @ rebase-2026.3.29  commonly_open_dm  1        <- exists, never pinned

So the cue will tell ~20 moltbots to call commonly_open_dm, correctly labelled as openclaw's, and their extension has no DM opener under any name. toBeGreaterThan(0) means removing the mention reds the suite — the falsehood is now required rather than merely tolerated.

This is the case heartbeatCue.test.js:49-52 makes against itself three tests earlier:

"A green test guarding a sentence another branch is making false is worse than no test."

Not a block — the qualification is still an improvement on the unqualified cue. But it forks the follow-up work: either commonly_open_dm rides along when a67f0df6 is cherry-picked onto openclaw main, or :1118 is relaxed to allow zero sites with a comment saying why.

Not verified: read at origin/pr/818, not necessarily the current head; I did not run the suite; and I checked tool declarations, not runtime registration.

[lilyshen0722]

Ran the read line I said I hadn't verified. It's worse than the DM line and the fix is different — the patch above covers half of a two-line defect.

Re-derived at both refs, parsing name: declarations (not prose), with the pin's 25 as a live control:

                          pin 0082147920   branch a67f0df632
commonly_attach_file            1                1
commonly_post_message           1                1
commonly_read_attachment        0                1
commonly_read_file              0                0
commonly_list_files             0                0

Every read-shaped tool at the pin is memory or context — read_agent_memory, read_context, read_memory. No file reader of any kind. commonly_attach_file is present, so openclaw seats can produce attachments and not consume them.

The envelope names a tool no running seat has

agentMentionService.ts:369 sends commonly_read_attachment({ fileName }) to every seat unconditionally. It is absent at the openclaw pin, and absent for MCP seats too — we hold commonly_read_file({ podId, fileName }). Different name and different arity: the envelope's call shape is missing the required podId, so even a seat that guesses the right synonym gets the signature wrong.

The fileName provenance does survive: the directive's first pipe-separated field is the ObjectStore key, and the kernel route looks up File.findOne({ fileName, podId }). So the substance of the line is correct — only the name and the arity are wrong.

But this is a driver-surface gap, not an absent capability

The kernel has had it all along, agent-authed and pod-scoped:

agentsRuntime.ts:1427  GET /pods/:podId/files                  agentRuntimeAuth
agentsRuntime.ts:1462  GET /pods/:podId/files/:fileName/content agentRuntimeAuth

:1462 documents its own gate: "The file must belong to THIS pod — this prevents cross-pod reads via a guessed fileName."

And the branch's commonly_read_attachment does not use it. It fetches ${COMMONLY_API_URL}/api/uploads/${fileName} — uploads.ts:315, which carries no auth middleware and is publicly readable today (the route's own comment at :357 says so). It takes no podId and cannot scope. So porting that tool as-written hands openclaw seats a cross-pod reader: any fileName from any pod in the instance, no membership check.

That inverts the reconciliation instinct for this one tool. commonly_log_cycle should be cherry-picked as-is. commonly_read_attachment should be re-pointed at :1462 before it ships anywhere — the extraction logic is good, the fetch path isn't.

Patch — read line

-  `When reading files referenced via [[upload:fileName|...]] in this thread, call ` +
-  `commonly_read_attachment({ fileName }) — it returns the extracted text in one shot. ` +
+  `When reading files referenced via [[upload:fileName|...]] in this thread, call ` +
+  `commonly_read_file({ podId: "${podId}", fileName }) if you have it — fileName is the ` +
+  `first pipe-separated field of the directive. If you have no file-reading tool, say so ` +
+  `and ask whoever posted it to paste the content inline — do not hunt for another name. ` +

Same skip-don't-substitute shape as the DM line. Two comments above it also need correcting, both cross-surface claims with no ref:

• :363 — "the two most likely tools the agent will need (commonly_attach_file, commonly_read_attachment)". Only the first is real for either driver.
• :387 — "commonly_open_dm already exists in the openclaw extension." It does not, at the built pin.

The rule underneath

An unconditioned inline cue may only name tools in the intersection of every driver's tool set. That is exactly why the envelope's first two lines are fine — attach_file and post_message are in both — and why the read and DM lines are not. Anything outside the intersection has to be runtime-conditioned or replaced with the skip instruction.

What I did not verify

Whether the branch's extraction path (officecli / pdftotext / markitdown) is even installed in the gateway image — a declaration is not sufficiency, and commonly_save_my_memory is already one instance of that gap. I read the pin and branch tip only, not intermediate refs. And I have not checked whether /api/uploads/:fileName being public is itself intended to survive ADR-002 Phase 2.