Repository navigation
feat(heartbeat): the heartbeat moves into the framework - #827
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated review
Recommendation: merge and fast-follow. 8 medium severity findings can be fixed after the merge.
Reviewed commit 3cc01fa with claude-opus-5-5, by reading the code. It did not run the code or look at the checks.
Does this change belong in the project?
It fits the project. This fits. The heartbeat is an opt-in, framework-owned wake source, built like the EventGate and the existing silent-heartbeat and skip_reply wake paths, and the reply judge needs to know about it.
6 reasons with sources
The PR follows the project's rules on stacked and companion PRs, changelog fragments and stating what was not verified.
- It extends a heartbeat concept the core already owns, rather than adding a new one. Before this change, the framework already had special handling for silent heartbeat ticks (silent-heartbeat.ts, #216) and its own self-wake timer for skip_reply. The new Heartbeat sends its wakes through those same paths (suppressProse, silentHeartbeat), so it does not add a second delivery mechanism. (src/silent-heartbeat.ts; src/mcpl/skip-reply-wake.ts)
- It is laid out like the existing core subsystems: its own src/ directory, an optional FrameworkConfig key, and a state file under {storePath}/config. The gate is configured by FrameworkConfig.gate, lives in src/gate/, stores {storePath}/config/gate.json, and has tool routing and lifecycle hooks in framework.ts. The heartbeat copies that layout: src/heartbeat/, FrameworkConfig.heartbeat, config/heartbeat.json, a routing branch next to wake_add_rule, and start/stop next to resumeSleep/dispose. It also reuses the gate's checkToolInput to read tool arguments. (src/framework.ts (EventGate setup, config/gate.json); src/gate/)
- It is opt-in, and heartbeat-mcpl keeps working for hosts that don't turn it on. The README already counts both MCPL servers and timers as event sources. With
heartbeatleft unset nothing changes. Running both at once is refused when the framework is created, which keeps two heartbeats from running side by side. (README.md (Overview: external events include MCPL servers and timers)) - It serves the reply judge, which can only count wakes the framework knows about. upcomingWakesFor already lists framework-known wakes such as provider cooldowns. A heartbeat kept in an external server can't add to that list, and that is the stated reason for the move. (src/framework.ts (upcomingWakesFor))
- The PR follows the contribution conventions: companion and stacked PRs declared with merge order, a changelog fragment, a test count, and a Not verified section. The PR says it is stacked on #801 and names a companion PR in connectome-host that is safe to merge in either order. It adds changelog.d/core-heartbeat.added.md, reports 393 pass / 0 fail, and lists the checks it did not run. (CONTRIBUTING.md (How changes land; What a PR should contain; Changelog))
- It adds new public exports and a config surface the maintainers will have to keep working, and the README isn't updated to describe it. The package root now exports Heartbeat, DEFAULT_HEARTBEAT, HEARTBEAT_TOOL_PREFIX and the option types. This matches how the gate and judge are exported, so it is not a scope problem, but the README's Core Concepts section does not mention it. (README.md; src/index.ts)
Findings
8 findings of medium or high severity, see the inline comments:
- Medium severity. Conversation forks and ephemeral runs can reconfigure the resident's heartbeat and set reminders that never wake them.
- Medium severity. A scheduled tick whose delivery throws is dropped, not retried.
- Medium severity. A retried reminder loses its original due time, so its "due" header and repeat rhythm are wrong.
- Medium severity. The double-heartbeat check matches only the server id, not the tool prefix that produces
heartbeat--names. - Medium severity.
unsaveableis never cleared, so a state file read successfully on a later start is still never written. - Medium severity. A failed import of heartbeat-mcpl's schedule is never retried, though the log says the file was left for later.
- Medium severity, test coverage. The framework tests start the heartbeat by hand, so the start()/stop() wiring is never tested.
- Medium severity, test coverage. No test for a delivery failure: reminder retry and trigger refusal are unexercised.
3 more findings, less severe
- Low severity, test coverage,
test/heartbeat.test.ts:214-221. The state file's unsaveable paths and field validation are untested. Only a torn JSON file is tested. Nothing covers a file that can't be read or can't be renamed (the unsaveable branch and its "Not saved" note in tool replies), or readState/readSettings rejecting bad fields such as a sub-60 interval or a bad deliveryMode. A file with v ≠ 1 being moved aside is also untested. - Low severity, test coverage,
test/heartbeat.test.ts:296-300. Excluding conversation forks and the subconscious from the heartbeat is untested. heartbeatTargets() decides who gets the ticks and reminders, and whose judge list shows them through upcomingWakesFor. The tests use only one resident agent, so a regression that wakes forks or the subconscious, or lists the heartbeat in their judge prompt, would not be caught. - Low severity, documentation,
src/heartbeat/heartbeat.ts:12-15. The module comment points to EventGate.upcomingWakes, but the judge is fed by AgentFramework.upcomingWakesFor. The header says the reply judge is told about the heartbeat throughEventGate.upcomingWakes. The heartbeat's wakes are actually added in AgentFramework.upcomingWakesFor (src/framework.ts), and EventGate knows nothing of the heartbeat. A maintainer following the comment will look in the wrong place.
For a deeper review by several readers and a judge, which takes longer, a maintainer can comment /review deep.
| /** Handle a call to one of the heartbeat's tools. */ | ||
| private dispatchHeartbeatToolCall(agentName: string, call: ToolCall): void { | ||
| this.emitTrace({ type: 'tool:started', module: 'heartbeat', tool: call.name, callId: call.id, input: call.input }); | ||
| let result: ToolResult; | ||
| try { | ||
| result = this.heartbeat!.handleToolCall(call.name, call.input); | ||
| } catch (err) { | ||
| result = { success: false, error: `${call.name} failed: ${errorMessage(err)}`, isError: true }; | ||
| } | ||
| this.emitTrace({ | ||
| type: result.isError ? 'tool:failed' : 'tool:completed', | ||
| module: 'heartbeat', tool: call.name, callId: call.id, durationMs: 0, | ||
| ...(result.isError ? { error: result.error } : {}), | ||
| }); | ||
| this.pushEvent({ type: 'tool-result', callId: call.id, toolName: call.name, agentName, moduleName: 'heartbeat', result }); | ||
| } |
There was a problem hiding this comment.
Medium severity. Conversation forks and ephemeral runs can reconfigure the resident's heartbeat and set reminders that never wake them.
dispatchHeartbeatToolCall runs any agent's call, and getAllTools offers the six tools to every agent. Yet heartbeatTargets() delivers only to residents, leaving out forks and the subconscious. A conversation fork, which is driven by a channel, can pause or retime the agent's shared heartbeat. A fork or ephemeral run that calls reminder_add sets a reminder that wakes the residents, not itself. The nearest existing framework-owned schedule, the sleep tool, has the same problem and solves it: dispatchSleepToolCall (around line 31269) refuses forks and ephemeral runs, because "a sleep is the agent's". The heartbeat should refuse these callers in the same way, or route their wakes to them.
| if (this.state.reminders.length === before) { | ||
| return this.refuse('reminder_cancel', [`No reminder has the id "${id}". If it was a one-off that has fired, or it was cancelled already, there's nothing more to do; otherwise reminder_list shows the pending ids.`]); | ||
| } | ||
| this.save(); | ||
| this.armReminders(); | ||
| log(`cancelled reminder ${id}`); | ||
| return { text: `Cancelled ${id}. ${this.state.reminders.length} reminder(s) remain.${this.saveNote()}` }; | ||
| } | ||
|
|
||
| // =========================================================================== | ||
| // Timers and delivery | ||
| // =========================================================================== | ||
|
|
||
| private armTick(): void { | ||
| this.cancelTick?.(); | ||
| this.cancelTick = null; | ||
| const at = this.state.nextFireAt; | ||
| if (!this.started || this.state.settings.paused || at === null) return; | ||
| this.cancelTick = callAt(Math.max(at, this.notBefore), this.now, () => { | ||
| this.cancelTick = null; | ||
| this.fireTick('schedule'); | ||
| this.state.nextFireAt = this.now() + this.state.settings.intervalSeconds * 1000; | ||
| this.save(); | ||
| this.armTick(); | ||
| }); | ||
| } | ||
|
|
||
| /** Deliver one tick; false when delivery threw. */ | ||
| private fireTick(reason: 'schedule' | 'manual'): boolean { | ||
| const now = this.now(); | ||
| const s = this.state.settings; | ||
| const silent = s.deliveryMode === 'silent'; | ||
| try { |
There was a problem hiding this comment.
Medium severity. A scheduled tick whose delivery throws is dropped, not retried.
In armTick's callback, fireTick('schedule') returns false when deliver throws. Its result is ignored: nextFireAt still moves a full interval ahead (4 h by default) and that tick is lost. Reminders handle the same failure by retrying after RETRY_MS (fireDueReminders), and the RETRY_MS comment says "A delivery that throws is tried again this much later." A transient failure in deliverHeartbeat, such as addMessage throwing, therefore costs the agent a whole heartbeat period with only a log line. Retry the tick after RETRY_MS as reminders do, or narrow the comment.
| private armReminders(): void { | ||
| this.cancelReminders?.(); | ||
| this.cancelReminders = null; | ||
| if (!this.started || this.state.reminders.length === 0) return; | ||
| const due = Math.min(...this.state.reminders.map((r) => r.fireAt)); | ||
| this.cancelReminders = callAt(Math.max(due, this.notBefore), this.now, () => { | ||
| this.cancelReminders = null; | ||
| this.fireDueReminders(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
Medium severity. A retried reminder loses its original due time, so its "due" header and repeat rhythm are wrong.
When delivery throws, the reminder is rewritten with fireAt: now + RETRY_MS. On the next attempt, reminderText checks lateness against this new fireAt, so it leaves out the , due … note or gives the retry time instead of the time the agent asked for. For a repeating reminder, the next period is then computed from the retry time (rem.fireAt + everySeconds), so its rhythm shifts by the retry delay. Keep the original due time separate from the retry time.
| if (config.heartbeat !== undefined && config.heartbeat !== false) { | ||
| if ((config.mcplServers ?? []).some((s) => s.id === 'heartbeat')) { | ||
| throw new Error( | ||
| 'FrameworkConfig.heartbeat is on beside an MCPL server with the id "heartbeat" (heartbeat-mcpl): both would wake ' | ||
| + 'the agent, and their tools share the names heartbeat--…. Remove the server to use the framework\'s heartbeat ' | ||
| + '(heartbeat.importFrom can bring its schedule and reminders along), or turn heartbeat off to keep the server.', | ||
| ); | ||
| } |
There was a problem hiding this comment.
Medium severity. The double-heartbeat check matches only the server id, not the tool prefix that produces heartbeat-- names.
The tool listing (around line 29867) names MCPL tools ${config.toolPrefix ?? mcpl--${server.id}}--${tool}. A server only produces heartbeat--… names when its toolPrefix is heartbeat, and the server id plays no part in that. The new create-time check refuses only s.id === 'heartbeat'. Two cases get past it. heartbeat-mcpl registered under another id with toolPrefix: 'heartbeat' passes the check; its tools are then shadowed by the framework's routing at line 24260 while its pushes still wake the agent, so the agent is woken twice. heartbeat-mcpl registered under id 'heartbeat' without a toolPrefix is refused, even though its tools (mcpl--heartbeat--…) don't collide. The check should look at the effective prefix as well as the id. The claim that agents keep the tool names they knew also holds only for hosts that set this prefix.
| log(`fired reminder ${rem.id} (one-shot)`); | ||
| continue; | ||
| } | ||
| // A repeating reminder fires once for periods it missed, then keeps its rhythm from now. | ||
| let next = rem.fireAt + rem.everySeconds * 1000; | ||
| if (next <= now) next = now + rem.everySeconds * 1000; | ||
| if (canHoldTime(next)) { | ||
| this.state.reminders[i] = { ...rem, fireAt: next }; | ||
| } else { | ||
| this.state.reminders.splice(i, 1); | ||
| log(`ended repeating reminder ${rem.id}: its next time is past ${LAST_REMINDER_TIME}`); | ||
| } | ||
| log(`fired reminder ${rem.id} (repeating)`); | ||
| } | ||
| this.save(); | ||
| this.armReminders(); | ||
| } |
There was a problem hiding this comment.
Medium severity. unsaveable is never cleared, so a state file read successfully on a later start is still never written.
load() sets this.unsaveable when the state file can't be read or can't be moved aside, but nothing resets it to null. If a framework is stopped and started again in the same process (start() calls load() again), a read that now succeeds still leaves unsaveable set. save() then returns early forever, and every change the agent makes is silently lost on restart. The message itself says "nothing is written over it until a start reads it", so load() should reset unsaveable (and lastSaveError) when it begins.
|
|
||
| /** The state a file holds, or undefined when it isn't state this framework writes. */ | ||
| private readState(text: string): HeartbeatState | undefined { | ||
| let raw: unknown; | ||
| try { | ||
| raw = JSON.parse(text.charCodeAt(0) === 0xfeff ? text.slice(1) : text); | ||
| } catch { | ||
| return undefined; | ||
| } | ||
| if (!isObject(raw) || raw.v !== 1 || !isObject(raw.settings) || !Array.isArray(raw.reminders)) return undefined; | ||
| const settings = { ...this.defaults, ...readSettings(raw.settings, this.defaults, (note) => log(`${this.statePath}: ${note}`)) }; | ||
| const nextFireAt = typeof raw.nextFireAt === 'number' && canHoldTime(raw.nextFireAt) ? raw.nextFireAt : null; | ||
| return { v: 1, settings, nextFireAt, reminders: readReminders(raw.reminders, (note) => log(`${this.statePath}: ${note}`)) }; | ||
| } | ||
|
|
||
| /** heartbeat-mcpl's files, read once when there's no state file and the host names them. */ | ||
| private imported(): HeartbeatState | undefined { | ||
| const from = this.importFrom; | ||
| if (!from?.configPath && !from?.remindersPath) return undefined; | ||
| const readJson = (path: string | undefined, what: string): unknown => { | ||
| if (!path || !existsSync(path)) return undefined; | ||
| try { | ||
| const text = readFileSync(path, 'utf-8'); | ||
| return JSON.parse(text.charCodeAt(0) === 0xfeff ? text.slice(1) : text); | ||
| } catch (err) { | ||
| log(`couldn't import heartbeat-mcpl's ${what} from ${path} (${(err as Error).message}); left as it is`); | ||
| return undefined; | ||
| } | ||
| }; |
There was a problem hiding this comment.
Medium severity. A failed import of heartbeat-mcpl's schedule is never retried, though the log says the file was left for later.
imported() goes ahead when only one of the two files parses. For example, if the config file is corrupt but the reminders file is fine, it returns a state built on the defaults. start() then saves that state, so the state file exists and imported() never runs again. The agent's interval, message, mode and paused setting from heartbeat-mcpl are lost for good. Meanwhile the log says "couldn't import … left as it is", which implies a later import. Either refuse the partial import and write no state, or say plainly that the schedule falls back to the defaults.
| const own = mkdtempSync(join(tmpdir(), 'heartbeat-fw-')); | ||
| const framework = await AgentFramework.create({ | ||
| storePath: join(own, 'store'), | ||
| membrane: membrane.asMembrane(), | ||
| agents: [{ name: 'scout', model: 'test-model', systemPrompt: 'You are scout.', proseRouting: 'disabled' }], |
There was a problem hiding this comment.
Medium severity, test coverage. The framework tests start the heartbeat by hand, so the start()/stop() wiring is never tested.
Both framework tests call heartbeat.start() through a cast instead of framework.start(), and nothing checks that framework.stop() clears the timers. Removing this.heartbeat?.start() from AgentFramework.start or the step(() => this.heartbeat?.stop()) from stop would not fail any test. In that case no tick ever fires in production, or a stopped framework keeps delivering wakes. Add a test that goes through framework.start/stop and sees a tick fire, then stop firing.
| hb.stop(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('heartbeat: reminders', () => { | ||
| it('a one-off fires once with its header, then is gone', () => { | ||
| const { hb, wakes } = heartbeat(); | ||
| hb.start(); | ||
| const r = call(hb, 'reminder_add', { message: 'water the tomatoes', inMinutes: 5 }); | ||
| assert.equal(r.success, true); | ||
| const id = /Reminder (rem_\S+) set/.exec(r.data!)![1]!; | ||
| mock.timers.tick(5 * 60_000); | ||
| assert.equal(wakes.length, 1); | ||
| const w = wakes[0]!; | ||
| assert.ok(w.kind === 'reminder' && w.reminderId === id && !w.recurring); | ||
| assert.equal(w.text, `[reminder ${id}, set 2026-10-26T12:00:00+00:00 [UTC] · current time: 2026-10-26T12:05:00+00:00 [UTC]] water the tomatoes`); | ||
| assert.deepEqual(saved().reminders, []); | ||
| assert.match(call(hb, 'reminder_list').data!, /No reminders set/); | ||
| hb.stop(); | ||
| }); | ||
|
|
||
| it('a repeating one fires each interval and stays; cancel ends it', () => { | ||
| const { hb, wakes } = heartbeat(); | ||
| hb.start(); | ||
| const id = /Reminder (rem_\S+) set/.exec(call(hb, 'reminder_add', { message: 'stretch', everyMinutes: 30 }).data!)![1]!; | ||
| mock.timers.tick(30 * 60_000); | ||
| mock.timers.tick(30 * 60_000); | ||
| assert.equal(wakes.filter((w) => w.kind === 'reminder').length, 2); | ||
| assert.match(wakes[0]!.text, /repeats every 1800s/); | ||
| assert.match(call(hb, 'reminder_list').data!, new RegExp(`1 reminder\\(s\\):\\n${id} \\| due .* \\| repeats every 1800s \\| "stretch"`)); | ||
| assert.equal(call(hb, 'reminder_cancel', { id }).success, true); | ||
| mock.timers.tick(60 * 60_000); | ||
| assert.equal(wakes.filter((w) => w.kind === 'reminder').length, 2); | ||
| assert.equal(call(hb, 'reminder_cancel', { id }).success, false); | ||
| hb.stop(); | ||
| }); | ||
|
|
||
| it('reads atIso in the agent\'s zone and refuses what can\'t be scheduled', () => { | ||
| const { hb, wakes } = heartbeat({ timeZone: 'America/New_York' }); | ||
| hb.start(); | ||
| assert.equal(call(hb, 'reminder_add', { message: 'call', atIso: '2026-10-26T09:30' }).success, true); // 13:30Z | ||
| mock.timers.tick(90 * 60_000 - 1); | ||
| assert.equal(wakes.length, 0); | ||
| mock.timers.tick(1); | ||
| assert.equal(wakes.length, 1); | ||
| for (const [input, says] of [ | ||
| [{ message: 'x' }, /No time was given/], | ||
| [{ message: 'x', inMinutes: 5, atIso: '2026-10-27T09:00' }, /"atIso" and a delay both say when it first fires/], | ||
| [{ message: 'x', everySeconds: 2 }, /under the 5-second floor/], | ||
| [{ message: '', inMinutes: 1 }, /"message" is missing; give the text to deliver when it fires\. heartbeat--reminder_add has "message" as "", so it's left out/], | ||
| [{ message: 'x', atIso: '2026-03-08T02:30' }, /doesn't exist in America\/New_York/], | ||
| ] as const) { | ||
| const refused = call(hb, 'reminder_add', input); | ||
| assert.equal(refused.success, false, JSON.stringify(input)); | ||
| assert.match(refused.error!, says); | ||
| } | ||
| hb.stop(); | ||
| }); | ||
| }); | ||
|
|
||
| describe('heartbeat: state across restarts', () => { |
There was a problem hiding this comment.
Medium severity, test coverage. No test for a delivery failure: reminder retry and trigger refusal are unexercised.
Several paths have no test: fireDueReminders' catch branch (reschedule after RETRY_MS), trigger() returning an error when deliver throws, and a scheduled tick whose delivery throws. Each is new behaviour described in the code. A test should give a deliver that throws once and check the retry time, the text of the second delivery, and the refusal returned by heartbeat_trigger.
From the automated review of #827: - Residents know the tools as mcpl--heartbeat--… (heartbeat-mcpl under the default prefix); calls by those names reach the same tools, and say the new name. - A conversation fork, an ephemeral run or the subconscious may show the schedule but not change it (as for sleep). - A tick whose delivery throws is tried again in a minute, not an interval later; a reminder's retry keeps its due time (retryAt), for its header's "due" and its repeat's rhythm. - heartbeat-mcpl is refused beside it by id, by a heartbeat tool prefix, or by the command that runs it. - A later start that reads the state file saves again; an import that can't read one of heartbeat-mcpl's files saves nothing, so the next start finishes it. - Tests: framework.start()/stop() arm and clear the tick; a fork's change refused while status answers; each delivery failure; old names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
/review deep |
|
@antra-tess One point about delivery, and two notes on merging with #805 and #819. Ticks and the gate's rules. The changelog says the heartbeat's wakes are delivered "as the server's were: past the gate's rules and through a sleep". Before this pull request, on We ran it at We think ticks and reminders should keep going through the rules. The framework could ask Merging.
Written by Crispin-2713. — Fiona-2757 |
There was a problem hiding this comment.
Automated review
Recommendation: fix here or file an issue for each, then merge. 13 should-fix issues should be fixed in this pull request or filed as an issue before merging.
Reviewed commit 879da7c by reading the code, in four stages: a plan by claude-opus-5-5; two readers on every task of it, gpt-6.1-sol and claude-haiku-5-5; their findings grouped into issues; and a judge, claude-opus-5-5, that tried to refute each issue. Nothing was run, so each issue is a lead from reading, and each blocker or should-fix says the run that would confirm it.
Between them, the reads of the readers that answered returned every changed line of the change (1485 in all). A line counts when a read by a reader that answered returned it whole: an added line from the diff or from its file, a removed line from the diff. A search that matched it doesn't count. The head has no lockfile at its root, so its dependencies were installed from package.json's version ranges, and a dependency the readers read may be a later version than the author ran.
Should fix (13)
- Should fix. Heartbeat alias canonicalization runs after exact-name allowedTools permission and tool-surface filtering, so legacy and new names don't share authority.
src/heartbeat/heartbeat.ts:291,src/framework.ts:6410,src/framework.ts:7097and 17 more places - Should fix. heartbeatTargets copies the broadcast filter that includes ephemeral runs, while the resident check and description treat ephemeral runs as non-residents.
src/framework.ts:5050,src/framework.ts:5066,src/framework.ts:5081and 7 more places - Should fix. Heartbeat text is written once to the primary's window, and readsDeferredFor marks only a write deferred for that same agent, so other residents' wakes aren't tied to the text.
src/framework.ts:5079,src/framework.ts:5084,src/framework.ts:15542and 9 more places - Should fix, text. Changelog, description and comments say bypassing gate rules is delivery 'as before', but heartbeat-mcpl pushes were still evaluated by gate.js/gate.json.
changelog.d/core-heartbeat.added.md:1,src/framework.ts:5055,src/heartbeat/heartbeat.ts:16and 8 more places - Should fix, test. The fork-refusal test uses an unregistered caller and checks only the error text, so the conversation-fork exclusion isn't pinned.
test/heartbeat.test.ts:448,test/heartbeat.test.ts:449,src/framework.ts:5051and 2 more places - Should fix. The heartbeat is routed only in dispatchToolCall, not in executeToolCall, puppetToolCall or module callTool.
src/framework.ts:24282,src/framework.ts:22206,src/framework.ts:22322and 5 more places - Should fix. The heartbeat-mcpl coexistence refusal exists only in create(), not in runtime connect/restart collision validation.
src/framework.ts:3925,src/framework.ts:28655,src/framework.ts:28665and 8 more places - Should fix. The subconscious's fixed tool surface and allowedTools omit the promised read-only heartbeat tools.
src/framework.ts:5091,src/framework.ts:7061,src/framework.ts:12621and 2 more places - Should fix, test. The silent-tick test checks only request flags, never that no prompt was stored or kept in the request content.
test/heartbeat.test.ts:377,test/heartbeat.test.ts:395,test/heartbeat.test.ts:397and 3 more places - Should fix. The heartbeat config switch bypasses runtime validation and treats any non-false value as enabled.
src/framework.ts:1550,src/framework.ts:3921,src/framework.ts:3936and 1 more place - Should fix, text. The wake-rule probe still claims to preview heartbeat wakes, even though framework heartbeats bypass the rules.
src/gate/event-gate.ts:3395,src/gate/event-gate.ts:3422,src/framework.ts:31125and 2 more places With FrameworkConfig.heartbeat on, wake_add_rule's probe table still shows a 'heartbeat' row (src/gate/event-gate.ts:3395, reported at src/framework.ts:3113x). A skip rule scoped to heartbeat shows that row as wouldWake:false. But deliverHeartbeat (src/framework.ts:5062) skips the gate's rules, so ticks still wake the agent. The preview is false, and the probe's doc at src/gate/event-gate.ts:3422 still claims it. To confirm: With heartbeat: true, call wake_add_rule with a skip rule whose serverId is 'heartbeat'. Check that the heartbeat probe reports wouldWake:false, then call heartbeat_trigger and check that a wake is still queued. - Should fix, test. The 'later start saves again' test deletes the unreadable path without restoring a valid file, so it covers only the missing-file branch.
test/heartbeat.test.ts:282,test/heartbeat.test.ts:289,src/heartbeat/heartbeat.ts:608The test 'a later start that reads the state file saves again' (test/heartbeat.test.ts:282) deletes the directory at the state path (line 289) and never writes valid state there. The second start therefore takes load()'s missing-file branch (src/heartbeat/heartbeat.ts:620), not readState (line 608). By reading, nothing pins that loading a repaired existing file makes saving work again. To confirm: Change the test to write a valid v1 state file at the path after removing the directory. Then check that a configure after the second start is saved, and that the test fails if unsaveable is cleared only on the missing-file path. - Should fix, test. The import test neither restarts nor compares source bytes, so it doesn't pin one-time import or source preservation.
test/heartbeat.test.ts:183,test/heartbeat.test.ts:193,test/heartbeat.test.ts:197By reading, the import test (test/heartbeat.test.ts:183) runs a single start and checks only that the source files still exist (line 193). It pins neither 'once' (no second start after changing the sources) nor 'leaving them as they are' (no comparison of bytes). Re-importing on every start, or rewriting the sources, would pass it. To confirm: Extend the test: keep the source bytes, compare them after start, then change heartbeat-mcpl's files, restart, and check that the saved state still holds the first import. Then check that the test fails if load() imports on every start.
Minor (22)
22 minor issues, by file
- The one-minute retry promise is unqualified, but a failed manual heartbeat_trigger is never retried.
changelog.d/core-heartbeat.added.md:7,src/heartbeat/heartbeat.ts:414,src/heartbeat/heartbeat.ts:511and 2 more places The changelog (changelog.d/core-heartbeat.added.md:7) and the description say a tick whose delivery throws "is tried again a minute later". By reading, that holds only for scheduled ticks (src/heartbeat/heartbeat.ts:512). A failed heartbeat_trigger (heartbeat.ts:415) only returns an error and arms no retry. The text should say the retry applies to scheduled ticks. - Heartbeat tools are usable before start() loads persisted state, so a save writes constructor defaults over the file.
src/framework.ts:4236,src/framework.ts:6410,src/framework.ts:12479and 6 more places Heartbeat state stays at constructor defaults until start() calls load() (src/heartbeat/heartbeat.ts:254, 263), but save() (679) has no started/loaded guard. A reminder_add or heartbeat_configure call driven via runUntilIdle() without framework.start() (src/framework.ts:12479) would write defaults plus the change over an existing heartbeat.json. Only the test helper reaches this path. - The coexistence guard checks only exact heartbeat prefixes and a command substring, while MCPL resolution runs first, so an ancestor prefix like 'mcpl' steals legacy heartbeat calls.
src/framework.ts:3925,src/framework.ts:24075,src/framework.ts:24197and 14 more places The create check at src/framework.ts:3925-3927 rejects only exact heartbeat prefixes. resolveMcplTool (src/framework.ts:24075-24083) runs at :24197, before the heartbeat route at :24282, and matches any prefix + '--'. So an MCPL server with toolPrefix 'mcpl' is accepted and takes mcpl--heartbeat--* calls, so the description's "Old names still work" fails. The legacy-name test calls handleToolCall directly, so it never goes through dispatch. - Heartbeat timers stay armed through stop()'s asynchronous waits, and delivery reports success after admission closes.
src/framework.ts:4469,src/framework.ts:4473,src/framework.ts:4586and 7 more places heartbeat.stop() runs only after stop()'s asynchronous waits (src/framework.ts:4586-4632, :4729), and deliverHeartbeat (:5062) doesn't refuse once admission is closed (:4473). A tick or one-off reminder that falls due during teardown queues an in-memory wake no turn will run. The heartbeat then saves the next tick time, or removes the reminder (src/heartbeat/heartbeat.ts:512, :571), so the wake is lost across a restart. - The description claims tool names are kept, but the advertised names change from mcpl--heartbeat-- to heartbeat--, and the resulting cache miss isn't mentioned.
src/framework.ts:6410The description says 'The six tools keep the names agents know them by: heartbeat--…', and changelog.d/core-heartbeat.added.md:1 says the same. Yet residents under heartbeat-mcpl's default prefix know them as mcpl--heartbeat--… (src/heartbeat/heartbeat.ts:42), and the description's own 'Old names still work' paragraph says so. The advertised names do change for those agents. The claim should say heartbeat--… is the new canonical name. - save() renames the temp file without fsyncing the file or its directory.
src/heartbeat/heartbeat.ts:684,src/heartbeat/heartbeat.ts:685,src/heartbeat/heartbeat.ts:683and 4 more places save() at src/heartbeat/heartbeat.ts:684-685 renames the temp file over heartbeat.json without fsyncing it or its directory, unlike writeRecoveryFile (src/framework.ts:7962, 7978). After a power loss the file can be empty, and load() (610-618) would then move it aside and start with no reminders and the default schedule. - The state file's existence is the only import-completion marker, so lost state triggers reimport of stale legacy reminders.
src/heartbeat/heartbeat.ts:599,src/heartbeat/heartbeat.ts:620,src/heartbeat/heartbeat.ts:674and 1 more place Only the state file's existence marks the import as done (src/heartbeat/heartbeat.ts:599, 620). If a damaged file is moved aside (612) and the save that follows fails, or heartbeat.json is otherwise lost, the next start re-imports heartbeat-mcpl's untouched files. Reminders that already fired or were cancelled are then re-armed and fire again, and the schedule reverts. - Using existsSync to tell missing from unreadable state/import files bypasses the no-overwrite and unfinished-import protections on permission errors.
src/heartbeat/heartbeat.ts:599,src/heartbeat/heartbeat.ts:646,src/heartbeat/heartbeat.ts:679existsSync (src/heartbeat/heartbeat.ts:599, 646) reports false on EACCES as well as ENOENT. A state file under an unsearchable directory therefore loads defaults without setting unsaveable, and a save after permissions are repaired (e.g. the tick at 513) would overwrite it. An unsearchable import source counts as absent, so a partial or empty import is saved as finished. - An unfinished import still arms and consumes imported reminders while saving is disabled.
src/heartbeat/heartbeat.ts:658,src/heartbeat/heartbeat.ts:676,src/heartbeat/heartbeat.ts:272and 3 more places During an unfinished import (src/heartbeat/heartbeat.ts:658) the timers are still armed (271-272). Imported reminders fire and are removed in memory (571), but save() is a no-op (680). The next start re-imports the untouched legacy files, so already-delivered one-shot reminders fire again and cancellations made meanwhile are lost. - Delivery isn't replay-safe: the prompt is committed before consumption is checkpointed, and retries use a new eventId while treating any throw as non-delivery.
src/heartbeat/heartbeat.ts:416,src/heartbeat/heartbeat.ts:511,src/heartbeat/heartbeat.ts:513and 6 more places By reading, the prompt is stored at src/framework.ts:5079 before the state is saved (src/heartbeat/heartbeat.ts:513, :587), and every attempt mints a new random eventId (:529, :562). A kill in that window, or a throw after context-manager 0.22.1's append has committed (message-store.js:406), leads the retry or restart to store the prompt again. heartbeat_trigger would also report "no heartbeat was sent" (:416). The window is narrow. - The judge's empty-list text still says a server's heartbeat may not be listed, though the framework now lists its own heartbeat.
src/heartbeat/heartbeat.ts:296,src/gate/reply-judge.ts:195,test/heartbeat.test.ts:230and 2 more places When the framework heartbeat is on but paused and no reminders are pending, upcomingWakesFor (src/framework.ts:5045) lists nothing. ifYouWait (src/gate/reply-judge.ts:195) then still tells the judge that a heartbeat "may not be listed", although the host owns the heartbeat and knows none is due. This may tilt the judge toward WAIT. By reading, the paused-heartbeat test doesn't check this wording. - The startup grace notBefore clamps every tick and reminder deadline, not just ones overdue at startup.
src/heartbeat/heartbeat.ts:262,src/heartbeat/heartbeat.ts:508,src/heartbeat/heartbeat.ts:544and 6 more places The 10 s start grace clamps every deadline, not only those overdue at start (src/heartbeat/heartbeat.ts:262, :508, :544). A saved tick due 5 s after start fires at start+10 s, and later ticks shift with it. A reminder added just after start with a short delay or a past atIso waits for the grace, despite 'A past time fires at once' (:150). The judge list (:301, :309) shows the clamped time. - Agent-facing failure replies show host paths/OS errors or point to the log, and don't say what's effective, whether to retry, or who must fix it.
src/heartbeat/heartbeat.ts:374,src/heartbeat/heartbeat.ts:375,src/heartbeat/heartbeat.ts:416and 5 more places The agent's replies point at things only the operator can reach. heartbeat_trigger's failure says delivery failed 'as the log says' (src/heartbeat/heartbeat.ts:416), and the 'Not saved' notes quote the host's statePath and OS error and say nothing is saved 'until a start reads it' (src/heartbeat/heartbeat.ts:375, :604, :658). None says whether calling again helps or that the host operator must fix it. - A missing or wrong-shape named import source isn't treated as a failure, so the incomplete import is saved as finished.
src/heartbeat/heartbeat.ts:646,src/heartbeat/heartbeat.ts:665,src/heartbeat/heartbeat.ts:674and 3 more places By reading, a heartbeat-mcpl reminders file that parses but is not an array is taken as zero reminders without enteringfailed(src/heartbeat/heartbeat.ts:674). Because unsaveable stays unset, the state is saved and the import counts as finished. Its reminders are never imported, and the only note is a "0 reminder(s)" log line (heartbeat.ts:675). - readReminders accepts an out-of-range createdAt that the formatter can't handle.
src/heartbeat/heartbeat.ts:727,src/heartbeat/heartbeat.ts:204,src/heartbeat/heartbeat.ts:561and 1 more place readReminders (src/heartbeat/heartbeat.ts:727) checks createdAt only with typeof, not canHoldTime as it does for fireAt. A one-shot row with createdAt 1e20 loads. Then formatHeartbeatTime throws inside reminderText (line 204, called at 561), and the catch at 567 reschedules it every minute forever. The trigger is a damaged or hand-edited file. - Non-object tool input is silently replaced with an empty object.
src/heartbeat/heartbeat.ts:323,src/heartbeat/heartbeat.ts:334,src/heartbeat/heartbeat.ts:342By reading, src/heartbeat/heartbeat.ts:334 swaps any non-object input (an array, or a double-encoded JSON string) for{}. Soheartbeat_triggerwith['x']fires a tick, andheartbeat_configurewith the string '{"paused":true}' answers 'No changes.' instead of refusing. Module tools refuse non-object input (src/tool-args.ts:172), but the heartbeat route (src/framework.ts:5109) checks nothing first. - Default and file-loaded intervals lack configure's representable-due-time check, so they can overflow nextFireAt to Infinity.
src/heartbeat/heartbeat.ts:252,src/heartbeat/heartbeat.ts:268,src/heartbeat/heartbeat.ts:383and 6 more places By reading, readSettings (src/heartbeat/heartbeat.ts:695-707) accepts any finite interval, without configure's canHoldTime check (line 383). For defaults or an imported intervalSeconds such as Number.MAX_VALUE, start (line 268) sets nextFireAt to Infinity. That is saved as null, and the judge is told of a certain wake 'in Infinity h' (src/gate/reply-judge.ts:173-178). An out-of-range finite due time is also dropped on reload (line 633). - The heartbeat file header names EventGate.upcomingWakes, but heartbeat wakes come from AgentFramework.upcomingWakesFor.
src/heartbeat/heartbeat.ts:13,src/gate/event-gate.ts:3836,src/framework.ts:5045The header at src/heartbeat/heartbeat.ts:13-14 says the judge learns of the heartbeat through EventGate.upcomingWakes. That method (src/gate/event-gate.ts:3836) lists only the gate's own timers. The heartbeat's wakes are added by AgentFramework.upcomingWakesFor (src/framework.ts:5045), so the comment points maintainers to the wrong place. - Unreadable-state backup names use only the current millisecond, so renameSync can overwrite an earlier backup.
src/heartbeat/heartbeat.ts:610,src/heartbeat/heartbeat.ts:612The backup for an unreadable state file is named only by the start's millisecond (src/heartbeat/heartbeat.ts:610), and renameSync (line 612) replaces an existing file. Two quarantines at the samenow, possible with the injectable clock or a clock stepped back, would destroy the earlier backup. - The renamed-call note isn't added when a legacy-name call fails checkToolInput.
src/heartbeat/heartbeat.ts:328,src/heartbeat/heartbeat.ts:334When a call by the legacy mcpl--heartbeat--… name fails checkToolInput (a slip or misplaced field), the catch at src/heartbeat/heartbeat.ts:336 returns the error without the rename note that line 348 adds on other paths. The agent then reads an error naming heartbeat--… (src/gate/event-gate.ts:737), a tool it never called, with nothing explaining the rename. - formatHeartbeatTime uses Intl's era year without era, sign or padding, so accepted ISO years are shown wrongly.
src/heartbeat/reminder-time.ts:41,src/heartbeat/heartbeat.ts:181,src/heartbeat/heartbeat.ts:188and 4 more places Reading the code, formatHeartbeatTime (src/heartbeat/heartbeat.ts:181,188) prints Intl's unpadded year-of-era and requests no era. A reminder that parseReminderTime accepts (src/heartbeat/reminder-time.ts:41), such as atIso '0000-01-01T00:00Z' or '-000001-…', would be listed (heartbeat.ts:474) and headed with year '1' or a positive year. Years below 1000 would lose their four-digit padding. Only extreme dates are affected. - parseReminderTime range-checks the temporary UTC wall time before applying the offset.
src/heartbeat/reminder-time.ts:67,src/heartbeat/reminder-time.ts:68,src/heartbeat/reminder-time.ts:70By reading, parseReminderTime checks the Date range on the UTC-read wall time (src/heartbeat/reminder-time.ts:67-68) before it subtracts the given offset (line 71). So '+275760-09-13T01:00:00+01:00' is refused as out of range, though its instant is exactly LAST_REMINDER_TIME. '-271821-04-19T23:00:00-01:00' is refused the same way at the lower bound.
In existing code (1)
1 issue in code that was there before this change, which doesn't weigh in the recommendation
- Minor. Parked-request collapse keys message and silent heartbeat ticks identically, so a newer silent tick can replace a message tick.
src/framework.ts:15675,src/framework.ts:15682,src/framework.ts:15899and 2 more places By reading, while the host is quiesced (not under a provider cooldown, whose holdRequest only dedups identical requests), src/framework.ts:15675 gives a parked message-mode tick (src/framework.ts:5083) and a later silent tick (src/framework.ts:5068) one key. The silent request is kept, so the resumed turn suppresses prose for the message tick's stored prompt. Base push events collapse the same way under 'mcpl:push-event' (src/framework.ts:15381).
| } | ||
|
|
||
| /** Whether `name` is one of the heartbeat's tools, by its name or heartbeat-mcpl's. */ | ||
| ownsTool(name: string): boolean { |
There was a problem hiding this comment.
Should fix. Heartbeat alias canonicalization runs after exact-name allowedTools permission and tool-surface filtering, so legacy and new names don't share authority.
allowedTools is matched exactly (src/agent.ts:496), both for the tools offered to the model (src/agent.ts:1569) and for dispatch permission (src/framework.ts:24166), and both run before heartbeat aliasing (src/heartbeat/heartbeat.ts:292). An agent allowed only mcpl--heartbeat--* names is offered no heartbeat tool and is refused on the new names. One allowed only heartbeat--* names is refused on legacy calls. This contradicts the description's "Those calls reach the same tools".
To confirm: Configure an agent with allowedTools ['heartbeat--heartbeat_status'] and dispatch a call to mcpl--heartbeat--heartbeat_status; expect the 'not permitted by this agent's allowedTools' refusal. Separately, check the tool list sent to the provider for an agent that allows only the legacy names.
| } | ||
|
|
||
| /** The agents the heartbeat wakes: every resident but conversation forks and the subconscious, as a push event's default broadcast. */ | ||
| private heartbeatTargets(): string[] { |
There was a problem hiding this comment.
Should fix. heartbeatTargets copies the broadcast filter that includes ephemeral runs, while the resident check and description treat ephemeral runs as non-residents.
heartbeatTargets (src/framework.ts:5051) keeps ephemeral runs, which are registered in this.agents (src/framework.ts:9541). So every tick, reminder and silent tick queues a turn for a live ephemeral run (src/framework.ts:5067, :5082). Meanwhile src/framework.ts:5096 treats such runs as non-residents, and the description says those reminders "would wake the residents, not them". Either exclude ephemeralRuns from the targets or correct the text.
To confirm: Start an ephemeral run, fire a reminder via mock timers, and check whether pendingRequests holds an entry for the run's agentName with reason 'reminder'.
| const metadata = wake.kind === 'reminder' | ||
| ? { source: 'reminder', serverId: 'heartbeat', reminderId: wake.reminderId, recurring: wake.recurring, eventId: wake.eventId } | ||
| : { source: 'heartbeat', serverId: 'heartbeat', reason: wake.reason, eventId: wake.eventId }; | ||
| const id = this.addMessage('user', [{ type: 'text', text: wake.text }], metadata as MessageMetadata, { placement }); |
There was a problem hiding this comment.
Should fix. Heartbeat text is written once to the primary's window, and readsDeferredFor marks only a write deferred for that same agent, so other residents' wakes aren't tied to the text.
A heartbeat wake is tied to its text only when the write was held for that same resident (src/framework.ts:5084, :26010). Stored at once, each resident's wake survives after another request answered the text and starts an extra turn. Held for the primary's turn, a co-resident's wake runs before the text lands and isn't replaced. The description's per-resident drop claim (src/framework.ts:5059) overstates this.
To confirm: With two residents, fire a message-mode tick while the primary's turn is in a tool call. Check whether the helper's request carries the tick text and whether it is woken again after the text lands. Separately, fire a tick while a request is being assembled and count the turns that follow.
| @@ -0,0 +1,8 @@ | |||
| - The heartbeat moves into the framework (`FrameworkConfig.heartbeat`, src/heartbeat/heartbeat.ts). It was the heartbeat-mcpl server: the agent's own periodic check-in, in message or silent mode, and its one-off and repeating reminders. The tools keep the names agents know: `heartbeat--heartbeat_status`, `heartbeat--heartbeat_configure`, `heartbeat--heartbeat_trigger`, `heartbeat--reminder_add`, `heartbeat--reminder_list` and `heartbeat--reminder_cancel`. Wakes are delivered as the server's were: past the gate's rules and through a sleep. A message-mode tick and a reminder store their timestamped text, as before; a silent tick is a silent-heartbeat turn with prose suppressed. | |||
There was a problem hiding this comment.
Should fix, text. Changelog, description and comments say bypassing gate rules is delivery 'as before', but heartbeat-mcpl pushes were still evaluated by gate.js/gate.json.
The changelog (changelog.d/core-heartbeat.added.md:1), the module doc (src/heartbeat/heartbeat.ts:16-17), the deliverHeartbeat comment (src/framework.ts:5055) and the description's 'Delivery, as before' all say heartbeat wakes skip the gate's rules as heartbeat-mcpl's did. heartbeat-mcpl's pushes were exempt only from the sleep and still ran through gate.js and gate.json (src/gate/event-gate.ts:3068-3080). An operator's heartbeat rule now silently stops applying.
To confirm: In a base build with a gate.json policy that skips serverId 'heartbeat', push a heartbeat-mcpl event and check that it is skipped. At head, enable heartbeat: true with the same policy and check that the tick still queues a wake.
| const results: Array<{ isError?: boolean; error?: string; data?: unknown }> = []; | ||
| const push = internals.pushEvent.bind(framework); | ||
| internals.pushEvent = (event) => { if (event.type === 'tool-result') results.push(event.result!); else push(event); }; | ||
| internals.conversationAgentHomes.set('scout-conv-1', {}); |
There was a problem hiding this comment.
Should fix, test. The fork-refusal test uses an unregistered caller and checks only the error text, so the conversation-fork exclusion isn't pinned.
The fork-refusal test (test/heartbeat.test.ts:448-453) uses a name added only to conversationAgentHomes, never to agents. heartbeatTargets() (src/framework.ts:5051) never includes it, so the refusal comes from the not-a-resident check. By reading, removing the fork exclusion would leave the test passing, and nothing checks that the reminder wasn't saved.
To confirm: Remove !this.conversationAgentHomes.has(n) && from heartbeatTargets and run test/heartbeat.test.ts. The fork test is expected to still pass. Registering a real fork in agents should make it fail.
| } | ||
|
|
||
| // Route the heartbeat's tools (heartbeat--…) | ||
| if (this.heartbeat?.ownsTool(enrichedCall.name)) { |
There was a problem hiding this comment.
Should fix. The heartbeat is routed only in dispatchToolCall, not in executeToolCall, puppetToolCall or module callTool.
By reading, only dispatchToolCall routes the heartbeat tools (src/framework.ts:24282). executeToolCallFrom has no heartbeat branch and falls through to moduleRegistry.handleToolCall (src/framework.ts:22480). So executeToolCall, puppetToolCall and a module's callTool answer 'Module not found: heartbeat, so nothing was done' (src/module-registry.ts:342). Under heartbeat-mcpl these calls reached the tools through the MCPL route.
To confirm: With heartbeat:true and a started framework, call framework.executeToolCall({id:'h',name:'heartbeat--heartbeat_status',input:{},callerAgentName:resident}) and puppetToolCall(resident,'heartbeat--heartbeat_status',{}), then check whether the result is 'Module not found: heartbeat'.
| // heartbeat-mcpl by its id, by a tool prefix that names its tools as | ||
| // the framework's (or as the old names the framework still answers), or | ||
| // by the command that runs it under any id. | ||
| const beside = (config.mcplServers ?? []).find((s) => s.id === 'heartbeat' |
There was a problem hiding this comment.
Should fix. The heartbeat-mcpl coexistence refusal exists only in create(), not in runtime connect/restart collision validation.
Only create() refuses heartbeat-mcpl beside the heartbeat (src/framework.ts:3925). A runtime connect or a restart validates with mcplPrefixCollision (src/framework.ts:28665, :28879, :28980), which never checks the heartbeat. A heartbeat-mcpl deployed at runtime is accepted and wakes the agent a second time. With toolPrefix 'heartbeat' its tools take heartbeat--* calls ahead of the built-in route (src/framework.ts:24197 before :24282).
To confirm: Create a framework with heartbeat:true and no MCPL servers, then call connectMcplServer({id:'heartbeat', command:'heartbeat-mcpl', ...}) or a server with toolPrefix:'heartbeat'. Check whether it is refused as create() refuses it.
|
|
||
| /** Handle a call to one of the heartbeat's tools. */ | ||
| private dispatchHeartbeatToolCall(agentName: string, call: ToolCall): void { | ||
| // The heartbeat is the agent's, and wakes its residents: a conversation |
There was a problem hiding this comment.
Should fix. The subconscious's fixed tool surface and allowedTools omit the promised read-only heartbeat tools.
The changelog says the subconscious 'can show the schedule and reminders' (changelog.d/core-heartbeat.added.md:6), but it can't. Its tool surface omits the heartbeat tools (src/framework.ts:7071), and its hard-coded allowedTools leaves out heartbeat_status and reminder_list (src/framework.ts:12621). Any such call is refused as not permitted at src/framework.ts:24166, before the heartbeat route.
To confirm: Enable subconscious and heartbeat, list getToolsForAgent(subconsciousName), and dispatch heartbeat--heartbeat_status as the subconscious. Check whether the tool is missing and the call comes back 'not permitted by this agent's allowedTools'.
| assert.ok(upcoming.some((w) => w.what === 'its heartbeat (a check-in prompt, every 60 min)'), JSON.stringify(upcoming)); | ||
| }); | ||
|
|
||
| it('delivers a silent tick as a silent-heartbeat wake with prose suppressed, storing no prompt', async (t) => { |
There was a problem hiding this comment.
Should fix, test. The silent-tick test checks only request flags, never that no prompt was stored or kept in the request content.
The test at test/heartbeat.test.ts:377 is named "storing no prompt", but by reading it checks only the queued request's suppressProse and silentHeartbeat fields (lines 397-399). It never compares the store before and after the tick. A regression where the silent branch (src/framework.ts:5064) also stored wake.text would pass it.
To confirm: Add an addMessage call with wake.text in deliverHeartbeat's silent branch and run test/heartbeat.test.ts; the silent-tick test should still pass, which shows the gap.
| // The heartbeat, when the host turns it on: the agent's own schedule of | ||
| // wakes. heartbeat-mcpl beside it would wake the agent twice and offer | ||
| // its tools under the same names, so that's refused. | ||
| if (config.heartbeat !== undefined && config.heartbeat !== false) { |
There was a problem hiding this comment.
Should fix. The heartbeat config switch bypasses runtime validation and treats any non-false value as enabled.
By reading, nothing checks FrameworkConfig.heartbeat at runtime. src/framework.ts:3921 treats any value except undefined and false as on. A YAML/JSON value of 'false' or 0 therefore starts a default four-hour heartbeat, and null throws a TypeError on opts.statePath (src/framework.ts:3938). The neighbouring switches are refused cleanly by assertFrameworkSwitches (src/framework.ts:1550).
To confirm: Call AgentFramework.create with heartbeat: 'false' (and again with null), and check whether it starts a heartbeat or throws a TypeError, instead of refusing the value with a message.
|
Replying to this comment. @antra-tess Please ignore our earlier comment, the one this replies to. We communicated parts of it poorly, and this comment replaces it. In our runs with this pull request, a heartbeat tick woke the agent under a Ticks and the gate's rules. The changelog says the heartbeat's wakes are delivered "as the server's were: past the gate's rules and through a sleep". Without this pull request, on We checked this on Linux with Node 24.22.1, at We think a tick's wakes should go through the gate's rules, as pushes from a server registered under the id How this interacts with #805 and #819.
Crispin-2713 found these points, wrote the gate probes, and ran the push probe at — Fiona-2757 |
heartbeat-mcpl's ambient heartbeat (message or silent) and reminders now
live in the framework, behind FrameworkConfig.heartbeat, so the framework
knows the agent's next scheduled wakes (the reply judge is told them) and
no server, policy exchange or push delivery is needed to keep them.
- src/heartbeat/heartbeat.ts: the schedule, ticks, reminders and the six
tools under their old names (heartbeat--…), arguments read as wake_add_rule
reads its own; reminder-time.ts ported as it was. Texts keep
heartbeat-mcpl's format to the second.
- State in {storePath}/config/heartbeat.json, written atomically; the next
tick's time survives restarts; overdue ticks and reminders fire once after
a 10 s grace; an unreadable file is moved aside. importFrom reads
heartbeat-mcpl's two files on a first start.
- Delivery as before: past the gate's rules and through a sleep; a silent
tick goes through the existing silent-heartbeat path.
- An MCPL server with the id "heartbeat" beside it is refused at create.
- upcomingWakesFor adds the next tick and pending reminders for the judge.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ackage Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From the automated review of #827: - Residents know the tools as mcpl--heartbeat--… (heartbeat-mcpl under the default prefix); calls by those names reach the same tools, and say the new name. - A conversation fork, an ephemeral run or the subconscious may show the schedule but not change it (as for sleep). - A tick whose delivery throws is tried again in a minute, not an interval later; a reminder's retry keeps its due time (retryAt), for its header's "due" and its repeat's rhythm. - heartbeat-mcpl is refused beside it by id, by a heartbeat tool prefix, or by the command that runs it. - A later start that reads the state file saves again; an import that can't read one of heartbeat-mcpl's files saves nothing, so the next start finishes it. - Tests: framework.start()/stop() arm and clear the tick; a fork's change refused while status answers; each delivery failure; old names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
879da7c to
82c0e01
Compare
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eFor main (afcd501) renamed the helper that ties a wake to the write it's for, and gave it stored messages too, so a heartbeat tick's or reminder's wake starts no turn once a request has read and answered its text. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
From nissa-seru's review: heartbeat-mcpl's pushes went through the gate's rules (only a sleep let the server id `heartbeat` through), so a rule matching heartbeat pushes, or a `defer` default, governed the heartbeat. deliverHeartbeat now asks the gate about each tick and reminder as the push it was (heartbeatGateEvent: mcpl:push-event from `heartbeat`) and queues the wake only when the gate triggers one; a message-mode tick's or reminder's text is stored either way, as a push's is. A sleep still lets it through, and the probe table's heartbeat row now describes it. From the automated re-review: - An agent's allowedTools naming a heartbeat tool under either prefix (heartbeat--, mcpl--heartbeat--) allows it under both (canUseTool). - Ephemeral runs are no heartbeat targets. - The tools answer executeToolCall, a puppet and a module's callTool (executeToolCallFrom), modules trusted as for utils. - heartbeat-mcpl is refused at a runtime connect or restart too (mcplPrefixCollision), by the shared isHeartbeatMcplServer. - FrameworkConfig.heartbeat is checked: true, false, null or an object with path-text fields; "false" or 0 is refused, null leaves it off. - The judge's list marks a tick or reminder the rules would hold back as "maybe" (gate probe). - Texts: delivery described as through the gate's rules; the subconscious has no heartbeat tools; a co-resident's wake isn't tied to the text. - Tests: the gate's rules, a defer default and a sleep over ticks; a registered fork refused with nothing saved; a silent tick storing nothing; a repaired state file read by the next start; the import kept byte for byte and not redone; call paths, ephemeral runs, allowedTools aliases, runtime refusal and the config check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Thanks. You were right about delivery, and fa86f47 takes your suggestion. Ticks and the gate's rules.
The automated re-review's 13 items, all in fa86f47:
🤖 Generated with Claude Code |
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
110 commits from main, 20 pull requests: #852 (supplied text can't read as the framework's own), #853 (every check of whether an agent sees a row asks its view), #827 (the heartbeat moves into the framework), #846 (exhausted turns re-armed), #830 (chat:system notices), #801 (the reply judge), #840, #843, #839, #791, #845, #847, #849, #822, #854, #836, #855, #857, #858, #859 and the 0.28.0 release. package.json takes main's version and ranges (context-manager ^0.23.1, membrane ^0.8.0). Conflicts: - src/framework.ts, TurnRouting: both fields kept, the branch's addressedElsewhere and main's heard (and both in freshTurnRouting). - src/framework.ts, createAgent: main's `sees` predicate, recorded in storeSees, is the branch's composition: heldFromResidents (the RESIDENT_VIEW_FILTER rule) and !isAnotherAgentsInjection; the view filter reads it. - src/framework.ts, tool-boundary drain: the branch's releaseLandedMail and main's noteSpokenTo both run; the injection skip asks agentSees (main) where the branch asked heldFromResidents, which it subsumes. - src/framework.ts, the fork wake and the routed fork wake: the branch's tag-only isAddressedMessage, with main's systemNoticeMark added. - src/framework.ts, a coalesced batch's wake: tag-only addressed, with main's note on why it carries no systemNotice mark. - src/framework.ts, isUnreadStoredMessage: main's form (the slot's readers each asked their own view) over the registry's sharedSlotReaders. - src/framework.ts, noteArrival: main's agentSees and heard, the branch's arrival with `addressed`. - src/framework.ts, noteForOtherResidents: the registry's readers, with main's noteSpokenTo. - src/framework.ts, the channel tool dispatch: the branch's publishInputWithTurnLocus, with main's earlierWake on the origin. - src/framework.ts, tune_out repeated on a tuned-out channel: the branch's awaited form, the name quoted by channelRegistry.quotedShown (#852). - src/mcpl/channel-registry.ts, markAccepted: the branch's (admission's noteDelivered sets the locus); main's chat:system is a marker (isMarkerTagged), so noteDelivered's rule covers it, and its comment says so. - src/mcpl/channel-registry.ts, tuneOutEndText: the branch's serverId form for a channel no name picks alone, a name written by quotedValue. - src/tune-out/coordinator.ts: the branch's readOptions, all-except refusal, renderHeldLine/historyTool, reader-fork invocation and cancel_tuneout scope logic kept; main's quotedShown, NOTICE_TAG texts and suppliedValue author adopted in each. - test/batched-wake-locus: main's new two-closed-channels test is placed in the branch's rig (framework read from rig.framework()); the branch's ambient-only tests kept. - test/channel-registry-routing: the branch's noteDelivered comment, with main's chat:system added to the marker loop. Main's changes integrated into the branch's structures: - heartbeatTargets reads registry.untargetedRecipients() (an untargeted push's recipients) and heartbeatRefusal registry.homeChannel, in place of conversationAgentHomes. - releaseAgentRuntime also drops #846's re-arm count and exhausting re-arm and #840's exhausted-turn record, and ends the run of stated waits (endStatedWaitRun). - storeSees is recorded for a derived manager (its source's predicate, which context-manager's derive keeps as its view filter) and for a context reopened on the shared slot (the residents' view). - The closed-channel invitation for a deliverTo correction reads the recipient admission chose, where main rewrites event.targetAgents. - #852 on the branch's own texts: authorLine/presentAuthor write the author through suppliedValue; the addressed-elsewhere notice's author too; `Focus` and `Agent lifecycle` are NOTICE_TAG entries and their notices use them; tune-out's branch-only notices use NOTICE_TAG and name channels by shown/quoted names. Tests read onto this branch, each named in a comment: - heartbeat (two cases): the fake fork is a registry record (conversationForkSpec), not a conversationAgentHomes entry. - prose-drafts (stated provider wait, two cases): scout is made a conversation agent through its registry record (homeChannel, no provider scheduling), as main does through conversationAgentHomes. - channel-registry-routing (chat:system locus): each message is delivered with noteDelivered, where admission records the locus here. - supplied-text-in-context (three cases): a channel message is stored and sent under its author's name (presentAuthor); the delivered body's digest is compared with the marked body before that decoration. - tune-out-reader-queued: its fake registry gains quotedShown. Tests: built with zero errors; 29 stale compiled tests removed. The targeted run (dendrite-*, tune-out-*, batched-wake-locus, channel-registry-routing, compression-after-release, stored-injections and main's added or changed framework-area tests, 63 files) gave 2507 tests, 2491 pass, 14 fail, 2 skipped before the readings above; after them, 73 files (those 63 and all-except-unit, all-except-e2e, author-presentation, module-rows-held-from-residents, refusals-are-error-results, state-read-and-write-errors, inbound-source, reinit-after-branch-move, channel-agent-words, channel-lifecycle-order) gave 2860 tests, 2858 pass, 0 fail, 2 skipped (main's runtime-conditional skips). The full suite was not run. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Problem
The heartbeat (the agent's periodic check-in and its reminders) lives in heartbeat-mcpl, an MCPL server. The framework never knows when the agent's next scheduled wake is. The reply judge (#801) is told what will wake the agent if it lets a batch wait, and today it can't count the heartbeat or a reminder due in five minutes. Keeping the heartbeat in a server also costs a process, a policy exchange and push delivery, with their own failure modes (pre-0.5 hosts, refused pushes, held reminders), for what is the agent's own timer.
Changes
src/heartbeat/heartbeat.ts: the heartbeat in the framework, behindFrameworkConfig.heartbeat(true, or{ statePath?, defaults?, importFrom? }).paused,message,deliveryModemessageorsilent) and reminders (one-off by delay oratIso, repeating every 5 s or more). The six tools keep the names agents know them by:heartbeat--heartbeat_status,--heartbeat_configure,--heartbeat_trigger,--reminder_add,--reminder_listand--reminder_cancel. Their arguments are read aswake_add_rule's are (checkToolInput: slips and misplaced fields refused, unknown names noted).reminder-time.ts(atIsoin the agent's zone, DST gaps and folds refused, naming offsets) is ported as it was. Texts keep heartbeat-mcpl's format to the second ([current time: …],[reminder <id>, set … · current time: …]).heartbeatthrough. So each tick and reminder goes to the gate as that push (heartbeatGateEvent: anmcpl:push-eventfromheartbeat), and its wake is queued only when the gate triggers one.source: "heartbeat"and the gate's default govern it as they governed the server's pushes, a sleep still lets it through, and the probe table's heartbeat row describes it.readsWriteFor, main's rename ofreadsDeferredFor); a co-resident's isn't tied to it.suppressProse,silentHeartbeatwithserverId: 'heartbeat', no stored prompt).{storePath}/config/heartbeat.json, written atomically..unreadable-<ms>; one that can't be read at all isn't written over.importFrom: { configPath, remindersPath }reads heartbeat-mcpl's two files on a first start (no state file yet), its savednullinterval loaded paused as the server did, and leaves them as they are.AgentFramework.createand at a runtime connect or restart (mcplPrefixCollision). It's recognized by an MCPL server with the idheartbeat, a tool prefix ofheartbeatormcpl--heartbeat, or a command that runs heartbeat-mcpl (isHeartbeatMcplServer).heartbeatmust betrue,false, null or an object whose paths are text."false"or0is refused rather than read as on, and null leaves it off.mcpl--heartbeat--…, heartbeat-mcpl under the defaultmcpl--<id>prefix, and their contexts hold calls by those names. Those calls reach the same tools, and the result names the new one. An agent'sallowedToolsthat names a heartbeat tool under either prefix allows it under both (Agent.canUseTool).executeToolCall, a puppet, and a module'scallTool(executeToolCallFrom; a module is trusted, as forutils).heartbeat_statusandreminder_list, but changing the schedule or setting a reminder is refused, as theirsleepis. Those reminders would wake the residents, not them. The subconscious's fixed tool surface doesn't include the heartbeat's tools.retryAt) keeps its due time for its header's "due" and its repeat's rhythm.upcomingWakesForadds the next tick ("its heartbeat (a check-in prompt, every 4 h)") and each pending reminder, with its message, to the<if_you_wait>list. One the agent's rules would hold back (by the gate's probe) is listed as only a "maybe".Heartbeat,DEFAULT_HEARTBEAT,HEARTBEAT_TOOL_PREFIXand the option types from the package root.What it drops from the server, since the framework delivers directly: the MCPL policy handshake,
delivery=BLOCKEDstates, refused or held pushes, and the silent-to-message fallback. heartbeat-mcpl stays usable on hosts that don't turn this on.Companion PRs / merge order
feat/reply-judge), forUpcomingWake. Merge feat(gate): judge wake rules — ask a small model before waking #801 first; this PR's base then moves tomain.modules.heartbeatin recipes, refused besidemcpServers.heartbeat, typechecked against this branch. Safe in either order: the host passes it by spread, and an agent-framework without this PR ignores it.Tests
test/heartbeat.test.ts(26 tests, with mocked timers and clock):heartbeat_trigger.intervalMinute) refused; an unknown name noted.atIsoread in America/New_York; refusals (no time, delay and atIso both, under 5 s, blank message, a DST gap).heartbeat_triggersaying it failed; a reminder's retry keeping its due time and rhythm; a later start saving again; a partial import saving nothing until a start finishes it.skiprule forsource: "heartbeat"holds it back while its text is stored, the probe's heartbeat row agrees, and the judge hears "maybe"; a sleep lets it through; adeferdefault holds it back.executeToolCallreaches the tools; an ephemeral run isn't a target; an agent allowedmcpl--heartbeat--heartbeat_statusmay callheartbeat--heartbeat_statusand not--reminder_add; heartbeat-mcpl refused at a runtime connect;"false",0and a non-textstatePathrefused, null leaving it off; a registered fork refused with nothing saved; a silent tick storing nothing; a repaired state file read by the next start; the import kept byte for byte and not redone when the old files change.heartbeat_statusrouted through an agent's turn; a tick stores its prompt, queues a wake and the agent reads it; a silent tick queues a silent-heartbeat request with prose suppressed; the judge's list includes the tick; heartbeat-mcpl refused by id, prefix or command;framework.start()arms andstop()clears the tick; a fork's change refused while its status call answers.npm run buildclean. On #801's head (with #805 andmainmerged into it). At fa86f47, everyevent-gate*andskip-reply-wake*file plusreply-judge,heartbeat,gate-recipe-reconcile,provider-acceleration-cooldownandprovider-waits, plusagent-allowed-tools, with #801's MCPL end-to-end test skipped (this environment can't reach channel registration): 598 pass / 0 fail. CI runs the full suite.Not verified
delivery=field, and different "not saved" notes and refusal wording. An agent that parsed them may notice.59c0ced3and onmain).🤖 Generated with Claude Code