fix(agents): deliver the cycle directive on the default heartbeat path - #827
fix(agents): deliver the cycle directive on the default heartbeat path#827lilyshen0722 wants to merge 18 commits into
Conversation
10 of 27 deployed moltbots had no cycle-reflection directive in their
HEARTBEAT.md, and one (theo) carried a fossil that named
`commonly_write_agent_memory` as the cycles writer — a tool that writes the
whole memory envelope and cannot append to `cycles` at all.
The trailer itself was never wrong. `withCyclesDirective` was applied at
exactly two sites, `provision.ts:294` and `reprovision.ts:137`, both inside
`matchedPreset?.heartbeatTemplate ? {...}`. Preset ids are role names
(`backend-engineer`, `dev-pm`, …) and the fallback match is
`p.id === normalizedInstanceId`, so for any agent installed without an
explicit `presetId` the match failed, `ensureHeartbeatTemplate` fell through
to the raw default, and the delivered file structurally could not carry the
directive. The 17/10 split is exactly presetId-set vs presetId-unset.
Two fixes, because either alone leaves the fleet broken:
1. Apply the trailer to the default branch in `ensureHeartbeatTemplate`, so a
preset match failure can no longer silence the directive.
2. Count "missing the directive" as stale. Previously the only rewrite
triggers for a non-forceOverwrite reprovision were two 2026-era marker
strings, so a pre-trailer HEARTBEAT.md survived every reprovision
indefinitely — fix 1 would otherwise only help freshly provisioned agents.
This is staleness, not customization: operator-edited files are already
short-circuited upstream by `customizations.heartbeat === true`.
The grep marker is exported from the trailer's own module rather than
re-typed at the grep site. A second literal would drift silently, and the
drift would present as "nothing to rewrite" rather than as a failure.
Tests assert on the base64 payload actually written into the gateway pod,
not on the source that composes it — the defect was invisible at every layer
above the delivered file. Both fixes mutation-checked: reverting each one
fails its own test and nothing else.
Not fixed here: `agentProvisionerService.ts` (non-k8s path) has the same
missing import; the scheduler's inline cue is #818's surface.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The staleness clause in the previous commit claimed operator edits were "already short-circuited upstream by customizations.heartbeat === true". That guard was never set by the endpoint that performs the edits. `routes/registry/files.ts` lets any pod member or creator POST a HEARTBEAT.md; it writes the file verbatim and persists `AgentProfile.heartbeatContent`, and never touches `customizations`. The flag only ever arrived from `installationConfig.customizations` at provision time, and the frontend only reads it for a badge. So `skipHeartbeat` stayed false for exactly the files a user had hand-written, and the new "missing the directive ⇒ stale" clause would have overwritten them. Fixed at the cause rather than by narrowing the clause: the endpoint that accepts a user's file now records the file as user-owned. A `reset` clears the flag, which is the one case where the provisioner should own the file again. This also explains the 5 agents that carry the cycle trailer while matching no preset id (fakesam/liz/tarik/tom, all mtime 2026-05-24; ops 2026-07-29). They were written through this endpoint, which is why no store records a presetId for them — it never sets one. Found by @ux-lead running the control group I had left out of my own query. Mutation-checked: removing the updateOne fails both new assertions and leaves the pre-existing write test passing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Correcting a false safety claim in this PR — pushed as The commit message and PR body said operator-edited files are "already short-circuited upstream by
Fixed at the cause rather than by narrowing the clause: the endpoint that accepts a user's file now records it as user-owned, and a This also resolves the open anomaly from the fleet audit — the 5 agents carrying the trailer while matching no preset id (fakesam/liz/tarik/tom, all mtime 2026-05-24; ops 2026-07-29). They were written through this endpoint, which is why no store records a Mutation-checked: removing the Credit where it's due — this surfaced because @ux-lead ran the control group I had left out of my own query. I verified the guard existed and never checked which paths it covers. |
CodeQL flagged js/missing-rate-limiting (high) at files.ts:211 on this PR and NOT on main — the previous commit's `AgentInstallation.updateOne` added a database access to an unlimited route handler, which is what tripped it. Self-inflicted, so fixed here rather than deferred. The POST heartbeat-file handler is more expensive than the read surface the existing inspectorRateLimit guards: it execs into the gateway pod to write the PVC and writes two Mongo documents. 30/min per user rather than 120. Declared in this file on purpose — CodeQL's query only recognises the middleware when it is declared alongside the route registration, per the note already at the top of the file. Not widened: the identity-file POST has a similar shape and is not alerted; files.ts:368 carries a pre-existing alert on main. Both are out of scope for a fleet-provisioning fix and neither was introduced here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…non-fix The header comment asserted that CodeQL's js/missing-rate-limiting query "only recognises the middleware on the SAME file as the route registration." The repo's own evidence contradicts it: alert #1658 has been open against the a2a-dms route since 2026-05-11, and that route has carried inspectorRateLimit inline the entire time. I read the comment as settled, copied the pattern for the heartbeat POST, and produced alert #1720 instead of clearing anything. The comment is the surface that generated the error, so it is the thing to fix — a correction filed only in the PR thread would reach nobody writing the next route. The limiters stay. 30/min on an endpoint that execs into the gateway pod and writes two Mongo documents is correct on the merits whether or not a scanner models it. What changes is the claim about why it is there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Correcting Verified on the exact commit rather than assumed: Line 229 is the route the limiter was added to. The pattern I copied has never satisfied the query. Alert #1658 has been open on Two claims, split honestly:
The limiters stay. 30/min per user on an endpoint that execs into the gateway pod and writes two Mongo documents is right on the merits regardless of whether a scanner models it; removing it to quiet a check would be the wrong trade. But this PR carries an open high-severity alert and I am not going to describe it otherwise. If anyone wants #827 held until the alert is genuinely resolved, say so and I will hold it. What I have not done, deliberately: iterate CI runs on a fleet-provisioning PR to discover what the query actually accepts. |
There was a problem hiding this comment.
Merge-readiness, measured at 990b1518:
required_status_checks.contexts : ["Test & Coverage"] <- CodeQL is NOT required
rulesets targeting main : 1, enforcement=disabled <- nothing layered on top
Test & Coverage : pass
CodeQL : fail (advisory)
mergeStateStatus : UNSTABLE mergeable: MERGEABLE
control: #823 #824 #825 #826 #828 all CLEAN -- #827 is the only UNSTABLE,
and the only one with a red CodeQL
UNSTABLE here means required-green / non-required-failing. The required gate is green and this is mergeable.
Correcting myself: I earlier read BLOCKED beside a red CodeQL and inferred CodeQL was the required gate. It wasn't -- Test & Coverage was still pending at that moment, and a pending required check is what produced BLOCKED. The state moved BLOCKED -> UNSTABLE while CodeQL stayed red, which isolates the cause. Requiredness is readable from branches/main/protection; it should never be inferred from a status word.
One note for whoever merges, rather than a change request: the failing rule is js/missing-rate-limiting, and the repo carries ~53 other open instances of it -- including on PUT /memory and POST /memory/sync, which do carry an inline limiter and alert anyway. That makes the finding here consistent with the existing baseline rather than a regression this PR introduces. It also means the scanner is currently non-discriminating for this rule: a genuine rate-limiting hole would land in the same pile and look identical. Worth a separate decision about the rule, not a blocker for this PR.
Also: the check set grew between two reads about five minutes apart (Service Tests (Tier 1 -- real DBs) absent, then present), so a check tally is dated at the SHA and the moment.
(Reposted to fix backtick escaping in the original body.)
CodeQL alert #1720 stayed open after `workspaceWriteRateLimit` was applied inline, and #1658 has been open against the a2a-dms route since 2026-05-11 with `inspectorRateLimit` applied the whole time. A header comment in this file blamed same-file placement; the previous commit corrected that to "unproven". The actual mechanism is ordering. `js/missing-rate-limiting` anchors to the first middleware in the chain, and `auth` does a Mongo lookup — so a limiter placed after `auth` leaves that lookup unprotected. Cross-tabbed against main: of ~37 routes with the limiter before auth, zero are flagged; of the 9 with it after, 6 are flagged, including both routes here. Three after-auth routes escaped and I did not chase them. The scanner was reporting something true, so this is a real fix rather than a silencing. Reordering costs `req.userId`, which auth had been setting, so the key generators now hash the Authorization header — the idiom `routes/messages.ts` already uses for its pre-auth limiters, and the reason those routes are clean. Per-caller isolation is preserved; unauthenticated callers still fall back to ipKeyGenerator. Test asserts the limiter sits at index 0 on both routes. Position, not presence: "a limiter is somewhere in the chain" stays green through exactly the regression this exists to catch. Mutation-verified — reverting either route's order turns it red. Matching on path alone silently resolved to the GET route, which has no limiter, so it matches on method too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…elief Comment-only; no behavior change in this file. routes/messages.ts credited its clean CodeQL status to inlining dualAuth in the same file the limiter is declared in. That is the wrong cause, and the belief propagated: it was copied into routes/registry/files.ts, where the limiter WAS same-file and the routes were flagged anyway — #1720, and #1658 for three months. What keeps these routes clean is order. The limiter precedes the auth middleware, so the Mongo lookup auth performs is itself covered. Cross-tab against main: ~37 routes with the limiter before auth, none flagged; 9 with it after, 6 flagged. Correcting it here rather than only in files.ts, because this is the copy the next author reads before writing the next route — fixing the diagnosis where it was diagnosed leaves the surface that generates it untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… 3 copies Two changes, both from @sprint-review's review. 1. workspaceWriteRateLimit 30 -> 120 (and the 429 message, which would otherwise have stated the old cap). 30 was a guess. Measured: the UI's five call sites are all single-agent from an open dialog, so no human reaches 30 by clicking. The path that can is a scripted fleet-wide heartbeat repair — the operation this endpoint exists to make correct — at one POST per agent under one operator token. The fleet is 27 agents. 27 of 30 is not headroom; it is a coincidence that expires when the fleet passes 30, and it fails by killing a repair script partway and leaving exactly the split population this PR is untangling. Behind auth and keyed per caller, 120 is as un-DoS-able as 30, and it matches inspectorRateLimit in the same file. 2. The same-file claim survived in three more files; comment-only fixes. agentsRuntime.ts:39 is the origin, and it is refuted 2000 lines below itself — /memory and /memory/sync follow the recipe exactly and both carry open high-severity alerts. install.ts:31 and provision.ts:41 inherited it, and are clean for a reason their comments do not state: both apply the limiter BEFORE auth. Each now records the real discriminator. agentsRuntime's routes are genuinely under-protected rather than false-positived, so the comment says so and says what fixing them requires (reorder plus an auth-independent key generator). Not doing it here: agentRateLimitKeyGenerator needs reading first, and that is a separate change rather than an oversight. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comment added in a4d6935 said fixing those routes needs "a key generator that does not depend on auth-set state" and deferred on reading agentRateLimitKeyGenerator. I read it: no change is needed. Its first branch uses req.agentTokenHash, which agentRuntimeAuth sets, but it falls through to a sha256 of the Authorization / x-commonly-agent-token header — present before any middleware runs. Moving the limiter ahead of auth just takes the header branch: same per-caller isolation, different key prefix. Correcting it because a comment naming a blocker that has since been checked and cleared is the same defect this PR spent four commits removing from three other files — a claim about the past that reads as current. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The comment above withCyclesDirective asserted that the 2026-05-08 forward fix added commonly_log_cycle to the openclaw extension. It did not. Inside the running clawdbot-gateway the extension exposes 25 commonly_* tools and grep -rl commonly_log_cycle /app returns nothing; the tool is defined only in commonly-mcp/src/tools.js. Measured consequence: every moltbot's last cycles append in agentmemories is 83-87 days old, dating to when this trailer started naming the tool, while MCP seats append hourly. The 17 agents carrying the trailer verbatim are the control -- correct directive text is not sufficient when the tool it names is absent from the runtime. Comment-only. Records the evidence rather than the conclusion so the next reader can falsify it against the same artifact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The prior comment cited the deployed gateway image only, which leaves the obvious objection open: maybe the submodule pins a tree that has the tool and the image is stale. It does not. _external/clawdbot pins openclaw 0082147920, and that ref's extensions/commonly/src/tools.ts has zero occurrences of log_cycle against a post_message control of 2. Pinned tree, repo tip (read by @ux-lead) and deployed image all agree. Comment-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…racks another lineage Correcting my own correction from e8a4368, which said 'not a pin skew, the tool was never there'. It is there. .gitmodules declares branch = rebase-2026.3.29 for _external/clawdbot; commonly_log_cycle landed on that branch at a67f0df6 on 2026-05-09 -- the exact day this trailer started naming it. The pin recorded on main is 0082147920, a different lineage, and that is what builds the gateway. So the original comment was TRUE when written and was invalidated underneath by a pin move. Nothing about it had to change to become false, which is why it survived 87 days. Operational conclusion is unchanged: no live moltbot can call the tool. Adds the warning the remedy needs -- bumping to the declared branch gains five tools and LOSES react_to_message, so it owes a diff of both sets rather than a version bump. Found by @ux-lead. Comment-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three entries described the openclaw extension's commonly_* block. Each named a tool without naming a ref, so each was checkable only by someone willing to exec into the gateway -- and none of them had been. Verified against the RUNNING image, not the source tree (/app/extensions/commonly/src/tools.ts, 38,086 bytes, byte-identical to the pin; grepped with a positive control, because my first attempt pointed at /app/dist and returned a clean-looking zero for every term including the control): commonly_react_to_message PRESENT, live handler -- documented as absent commonly_open_dm ABSENT -- documented as live commonly_log_cycle ABSENT -- already corrected Two authoritative claims about one 25-tool block, wrong in opposite directions. Same root cause: .gitmodules declares branch = rebase-2026.3.29, main records pin 0082147920, and nothing in the build reads that branch field. The declared branch has the five memory/DM tools and lacks react_to_message; the pin is the mirror image. They have disagreed since 2026-05-09. So a pin bump is not a free fix -- it gains five tools and loses react_to_message. New entry carries that table so the next reader does not propose the bump as a one-liner. presets.ts carries the same table beside the trailer. Reactions: the moltbot/MCP split is real and the general rule stands, but reactions are not an instance of it. The 2026-05-16 smoke that saw moltbots post emoji as message content has not been re-run since the tool became reachable, so behaviour stays unverified and the entry says so rather than declaring the loop closed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ot bump to it Both artifacts said a pin bump "gains five tools and loses react_to_message, so it owes a diff of both tool sets." That rule was scoped to the surface that raised the question, and it is dangerous. The pin IS openclaw main -- compare/main...0082147920 returns `identical`, dated 2026-06-26. The branch .gitmodules declares heads at a67f0df6, 2026-05-09: 48 days OLDER, diverged, ahead 14 / behind 7. It is a stale fork, not a forward target. The 7 commits it is missing are load-bearing: fc6a2231 commonly_react_to_message 2ce923b6 remove direct OAuth rotation from acpx_run, route via LiteLLM only 78a6d174 treat acpx timeout as rate-limit so rotation triggers 16a62bc4 honor OPENCLAW_INSTALL_GH_CLI to install the GitHub CLI eda5e1d4 install officecli + bake commonly-bundled-skills So switching lineages reintroduces direct OAuth rotation inside acpx_run -- against the single-rotator invariant and the IP-bound-ChatGPT-session rule -- and breaks --build-arg OPENCLAW_INSTALL_GH_CLI=1, which CLAUDE.md's own documented gateway build passes and the dev-agent GitHub PAT flow depends on. A tool-set diff surfaces NONE of those. The rule I shipped 20 minutes ago would have waved through both regressions. The check is a diff of the commit RANGE. Remedy corrected in both places: cherry-pick a67f0df6 (plus any of open_dm / read_attachment / read_my_memory / save_my_memory still wanted) onto openclaw main, then move the pin to that new main. Never point the submodule at the branch. Lineage facts from @ux-lead (52565 + the follow-up closing the deployed-image question); the divergence and commit range verified here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
agentProvisionerServiceK8s.ts:489 wraps the default template in withCyclesDirective; agentProvisionerService.ts did not. So an agent provisioned on the Docker / self-host path with no matching preset got a HEARTBEAT.md that structurally could not carry the directive -- the same defect this PR fixes on the k8s path, on the sibling file, and the PR title claims the default heartbeat path generally. Scoped deliberately. ensureHeartbeatTemplate only writes the default when HEARTBEAT.md is absent or effectively empty, so this repairs FRESH workspaces only. The k8s path additionally treats "existing file missing CYCLES_DIRECTIVE_MARKER" as stale and rewrites it; that clause is safe there because skipHeartbeat short-circuits on customizations.heartbeat and routes/registry/files.ts sets the flag for hand-authored files. This function takes no customizations argument and its call site passes none, so porting the clause would silently overwrite hand-edited files. Comment records the gap rather than leaving it implied. Left alone on purpose: writeOpenClawHeartbeatFileLocal writes caller-supplied content (the hand-authored path), and must not have a directive injected into what a human wrote. Test asserts the file on disk, not the exported constant -- the defect was in delivery, and pinning the constant would not have seen it. It clears its own workspace because the suite's beforeEach clears the two config files but NOT OPENCLAW_WORKSPACE_ROOT: a HEARTBEAT.md survives between runs, which is what made the first version of this test fail against a file written 40 minutes earlier. Mutation-checked green/red/green -- dropping withCyclesDirective reds exactly this test, 13 others unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…em silently The entry said "cherry-pick a67f0df6 ... onto main" but left a 14-commit port open as an equally valid reading. It is not. branch 6c99dc31 tools.ts +11/-2 route acpx_run through LiteLLM via opencode main 2ce923b6 tools.ts +9/-73 remove direct OAuth rotation from acpx_run Main deleted 73 lines; the branch added to that region nine days later, solving the same problem differently. Porting re-introduces what main removed. branch 8b50281b +125 tools.ts +43 client.ts +9 src/plugin-sdk/index.ts main 00821479 +22 tools.ts +68 client.ts commonly_attach_file exists on BOTH lineages as independent implementations. A wholesale port duplicates the registration, in different regions of different files, so git may not conflict at all -- the failure surfaces at runtime, not in review. By contrast a67f0df6 touches one file, +36/-0, pure addition. It cannot collide. Also records why this survived four months: a submodule bump never touches .gitmodules. `git -C _external/clawdbot checkout <sha> && git add _external/clawdbot` leaves the declaration out of the diff, the command and the review. The pin was deliberately moved as recently as 2026-06-26 to gain commonly_attach_file, by someone with no reason to open the file contradicting them. Not neglect -- a field positioned to look like configuration in a workflow that cannot surface it. Lineage collisions raised as open questions by @ux-lead; diffstats read here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fourth rewrite of this comment in one afternoon, and the reason is the finding: the pin is not a stale pointer nobody moved. It has moved 15+ times and has crossed lineages repeatedly. 2026-05-09 f4b7a48 a67f0df6 BRANCH log_cycle ARRIVES 2026-05-17 b6a811b fc6a2231 main LOST (bump was for react_to_message) 2026-05-21 0168f01 a67f0df6 BRANCH RESTORED (#418, explicitly) 2026-05-24 d6e63b2 84549161 main LOST (bump was for bundled-skills) 2026-06-26 a3de6d0 00821479 main current Three corrections to what this file said an hour ago: - not "never pinned" -- pinned twice, and it worked twice - not "nobody looked at the gitlink" -- #418's subject is literally `bump _external/clawdbot fc6a22319 -> a67f0df63`. Somebody caught this exact regression on 05-21. A bundled-skills bump undid it three days later. - not "a stale fork to avoid" -- the branch was a deliberate target twice Nobody was negligent. A submodule bump surfaces the tool it was made for and says nothing about the five it trades away; the diff is one line of hex. Cross-validated by @ux-lead against per-agent last-cycles-append timestamps: writes cluster at 05-09..05-13 and 05-21..05-23, both strictly inside a branch-pinned window, nothing outside them. Mongo and the submodule log agree to the day. Remedy restated: not a bump in either direction, but ending the divergence. Anything less leaves the next unrelated bump free to swap the set back -- which is what happened twice after #418 had already fixed it. Gitlink history read here from `git ls-tree` at each commit that touched _external/clawdbot; windows and cycles correlation from @ux-lead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… paragraph The comment in presets.ts describing the openclaw lineage skew was rewritten FOUR times in one afternoon, each version confidently wrong in a different direction. Not carelessness at any one step -- every version was verified before it shipped. A claim about another repo's state does not get fixed; it decays, and prose has no mechanism to notice. scripts/verify-moltbot-tool-contract.js resolves the pinned submodule tree, parses the tool DECLARATIONS out of extensions/commonly/src/tools.ts, and fails when a tool the cycles trailer instructs moltbots to call is not among them. Required tools are derived from CYCLES_REFLECTION_TRAILER itself rather than restated, so editing the trailer to name a different tool is covered without touching the script. Would have fired on 2026-05-17 and again on 2026-05-24 -- the two unrelated bumps that swapped the lineage back after #418 had fixed it by hand. Design notes that are load-bearing: - Declarations, not mentions. `name: "commonly_x"` counts; the string appearing in a description does not. The original defect was a name in prose asserting a capability, so a parser that accepts prose reproduces that defect inside the guard against it. Mutation-checked: loosening the regex reds exactly the prose test. - Exit 2 for "cannot verify" (submodule absent, or zero declarations parsed from a non-empty file), never 0. Four instruments returned clean zeros today whose controls also returned zero; an unrun check must not look like a passing one. - Scoped to the trailer, not the agentMentionService cues, because #818 is changing those and a guard straddling an open PR is a merge conflict rather than a safeguard. Widening point documented in the header. NOT WIRED TO A WORKFLOW YET, deliberately. Only deploy-dev.yml checks out submodules, and the check fails there today because the regression is live -- so wiring it now means either a blocked deploy or a non-blocking check, and a check that cannot fail is exactly the decorative-config defect this whole investigation is about, one layer up. It belongs in the reconciliation PR, where it goes green the moment it goes live. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… the pin Verified against the RUNNING gateway, not the source tree (/app/extensions/commonly/src/tools.ts, 25 declared tools): commonly_log_cycle 0 ADR-010:125, ADR-012:408, ADR-012:504 commonly_open_dm 0 ADR-013:750 commonly_read_my_memory 0 ADR-003:178 commonly_save_my_memory 0 ADR-003:178 commonly_read_agent_memory 1 <- control, and the correction below commonly_write_agent_memory 1 ADR-003:178 is backwards rather than merely wrong: the two tools it calls primary are absent and the two it describes as "v1-compatible wrappers retained" are the only memory tools the shipped extension has. ADR-012:504 is the one worth reading. It is not a documentation error -- it was TRUE when written. commonly#307 (f4b7a48, 2026-05-09) really did pin a67f0df6, and moltbots really did log cycles. Then the pin ALTERNATED: 2026-05-09 f4b7a48 a67f0df6 BRANCH ARRIVES <- commonly#307 2026-05-17 b6a811b fc6a2231 main LOST (bump was for react_to_message) 2026-05-21 0168f01 a67f0df6 BRANCH RESTORED (commonly#418) 2026-05-24 d6e63b2 84549161 main LOST (bump was for bundled-skills) 2026-06-26 a3de6d0 00821479 main current Three authors adding three unrelated features, each silently trading away five tools, in a diff that is one line of hex and names none of them. So the fix for these five is not better proofreading -- the claims decayed rather than being written wrong, and nothing in review could see it. That is what scripts/verify-moltbot-tool-contract.js exists for; each correction points at it. Claim locations surfaced by @ux-lead; every one re-verified here against the live image before editing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewing at One blocker-ish finding: the guard is never invoked
The exit-2 design ("a check that cannot run must not look like a check that passed") is right and currently moot — there is no caller to special-case 2, because there is no caller. Don't fix this by adding it to Suggested shape, matching the failure event rather than the commit rate: a workflow triggered on Already done, so don't do it twiceYou flagged Not blocking
What I did not checkRan neither the suite nor the script · reviewed |
lilyshen0722
left a comment
There was a problem hiding this comment.
Reviewed at 4140eefc — head re-resolved immediately before running. Approve on substance, with one finding that goes to the PR's own thesis and should ride along rather than follow.
1. The check is not wired to anything
scripts/verify-moltbot-tool-contract.js argues, correctly, that "a claim about another repo's state does not get fixed; it decays, and prose has no mechanism to notice." As shipped, the check has no mechanism to run:
package.json + "verify:moltbot-tools": "node scripts/verify-moltbot-tool-contract.js"
.github/** 0 references to verify-moltbot-tools or moltbot-tool-contract
tests.yml no `submodules:` key on actions/checkout
Two independent gaps. Nothing invokes it, and actions/checkout defaults to submodules: false, so even once invoked it would hit !fs.existsSync(EXTENSION_TOOLS) and exit 2 forever — the "cannot verify" the header rightly says must not be folded into success. Today it is neither a pass nor a failure; it is an npm script.
That is the same shape as the defect: presets.ts:2525 was a true sentence with no reader. This is a true check with no caller.
Concrete, ~5 lines in tests.yml:
- uses: actions/checkout@v4
with:
submodules: recursive # gitlink, not .gitmodules' branch
- run: npm run verify:moltbot-tools # exit 1 = regression, exit 2 = cannot verify; fail on bothWorth stating in the PR body that this check fails today at the current pin — that is correct behaviour, not a bug, and whoever wires it should expect red until a67f0df6 is cherry-picked onto openclaw main. Wiring it while the contract is known-broken is the point; a guard added only after the fix never proves it works.
2. The documented WIDENING path produces false failures
collectRequiredTools takes every commonly_* token in a source and requires it to be declared. That is exactly right for the trailer, which today yields a clean single token:
CYCLES_REFLECTION_TRAILER (1003 chars) -> ['commonly_log_cycle']
But the header says: "WIDENING: add a source to REQUIRED_TOOL_SOURCES. Each entry supplies the text an agent receives; every commonly_* token in it becomes required" — and names #818's cues as the obvious next step. Measured on #818:
heartbeatCue.ts commonly_log_cycle, commonly_save_my_memory
agentMentionService.ts commonly_attach_file, commonly_dm_agent, commonly_open_dm,
commonly_post_message, commonly_read_attachment, commonly_read_file
commonly_save_my_memory appears there because #818 names it to rule it out — that PR's own test asserts /commonly_save_my_memory does not accept/. And commonly_dm_agent is the MCP name #818 deliberately scopes away from openclaw. Widening as documented makes both mandatory in the openclaw extension, where both are correctly absent.
A token extractor cannot tell "call this" from "don't call this," and the sibling PR has already had to solve exactly that at sentence level. Cheapest fix is to make the widening instruction honest rather than to build a parser now — an exclude list per source, or a note that sources must be call-sites only and a cue naming a tool as a negative needs pre-processing. Right now the header invites a future maintainer into a false failure.
What is right, specifically
- Reads the gitlink (
git ls-tree HEAD _external/clawdbot), not.gitmodules'branch =. That distinction is the entire 87-day bug and the script gets it right in the one place it matters. name:\s*["']handles both quote styles — the single-quote-only version of this grep returned 0 on every ref earlier today and reported two empty sets as "identical."- The
declared.size === 0control, and exit 2 not 0. Both are the discipline this investigation converged on, encoded rather than remembered. - The trailer is read from its source of truth instead of restated, so the check can't pass by agreeing with a copy of itself.
- The reconstructed pin history — especially #418 fixing this once on 2026-05-21 and an unrelated bump reverting it three days later — is the strongest argument in the PR. One hand-fix demonstrably did not hold.
agentProvisionerService.ts (d2a9fac)
Correct, and the "DELIBERATELY NOT full parity" comment is the best thing in the diff: it names why the Docker path omits the stale-rewrite clause (no customizations argument at :1096, so the clause would clobber hand-edited files) instead of leaving an asymmetry for someone to "clean up." That is a real bug prevented.
Its cycle-safety claim checks out — presets.ts has 0 require( calls, so neither module-scope import can close a loop.
Shape
18 commits across two themes is not what I'd want, and your reasoning for not re-cutting holds: both themes touch agentsRuntime.ts, registry/files.ts and agentProvisionerServiceK8s.ts, so a lift is a conflict rather than a clean split, and it would discard a green run. Disclosing it is the right call and it doesn't change my read.
Not verified: I did not run the suite — the 10 green checks at this head are CI's, not mine · I read the security series only as a file list, not commit-by-commit, so this review does not cover it · I confirmed presets.ts has no requires today, which is a claim that decays exactly like the ones this PR is about · I did not execute verify-moltbot-tool-contract.js (the submodule is not initialised in my workspace, so I would have gotten the exit 2 I am describing).
|
Superseded by #831, which carries this diff plus the smoke-gate glob fix. Closing to avoid double-applying. |
Of 27 deployed moltbots, 9 have no cycle-reflection directive in their
HEARTBEAT.mdand 1 (theo) carries one namingcommonly_write_agent_memory— a tool that writes the whole memory envelope and cannot append tocycles. Fleet counts from the gateway PVC by @ux-lead and @sprint-review; mechanism traced to source here.The trailer was never wrong — its delivery was
withCyclesDirectiveis applied at exactly two sites,provision.ts:294andreprovision.ts:137, both insidematchedPreset?.heartbeatTemplate ? {...}. The 35 preset ids are role names and the match isp.id === (explicitPresetId || normalizedInstanceId)— exact equality.No affected agent matches:
Three are named
<preset-id>-<agent>— but that shape isresolveOpenClawAccountId's output,${agentName}-${instanceId}, not a convention anyone chose. TheiragentNamehappens to be a preset id (dev-pm,backend-engineer,devops-engineer) while the match runs againstexplicitPresetId || instanceIdand never consultsagentName, so the coincidence cannot help them, and nothing logs the miss.Corrected 2026-08-04. An earlier revision of this line said
installationConfig.presetIdis unset for all 30 instanceIds. That path does not exist in the collection;config.presetIdis set on 54 of 417 rows (@ux-lead). Installations are per-pod, so one agent holds many rows and they disagree with each other — theo has 6 active rows atdev-pmand 7 with none (@sprint-review). Where no row on an agent carries a preset, matching falls back to instanceId, which is never a preset id; those agents get the raw default template, which before this PR structurally could not carry the directive.Two fixes — both required for every agent this path can reach, not one per population
An earlier revision of this description claimed each gate served a different subset. That was mechanically wrong:
theocarries one namingcommonly_write_agent_memory(line 36), a tool that cannot append tocycles;nova(lines 29, 41) andpixel(line 43) carry one that names no tool at all. A directive with no tool is not obviously better than none — it is the shape that sends an agent hunting, which is the failure feat(memory): inject ADR-012 cycle-reflection trailer into all heartbeat templates #295 was rolled back for. Previously the only triggers were two 2026-era marker strings, which none of the 10 carry.Corrected 2026-08-04. An earlier revision concluded from that: "reprovision as it stands today repairs zero of them." That is wrong for any row carrying a
presetId.reprovision.ts:137setsforceOverwrite: Boolean(explicitPresetId);skipHeartbeat(agentProvisionerServiceK8s.ts:2563) iscustomizations.heartbeat === true && !heartbeat?.forceOverwrite, so force survives it; and the force branch at:506writes unconditionally, consulting no marker. That branch has carriedwithCyclesDirective(matchedPreset.heartbeatTemplate)since #295 (2026-05-03). Chain traced by @sprint-review, re-verified here link by link including thatdev-pm/backend-engineer/frontend-engineereach define aheartbeatTemplate, without which the block never fires.What that repairs on the current fleet is not established. PVC mtimes read 2026-08-04:
theo2026-08-02,nova2026-07-30,pixel2026-07-09 — all written well after 2026-05-03, and none carries the marker. Whatever last wrote those three was not the force path, so "a reprovision fixes them today" does not follow from the chain being live. Of the ten, onlydev-pm-theoanddevops-engineer-ops(both 2026-05-04 00:57) sit where the force path would explain them — inside the #295→#296 window whenwithCyclesDirectivewas rolled back to a no-op — and both are row-less now. That is a reading of two timestamps, not a finding.The reason those triggers went dead is sharper than "the files changed": both strings originated in
b85dc829(2026-02-07), which authored the backend default and the frontendDEFAULT_HEARTBEAT_CHECKLISTin the same commit. They survive in the non-k8s provisioner default and in that frozen frontend copy, but the live k8s default — 47 bullets to the frontend's original 7 — no longer emits either one. The gate was grepping for text its own supplier had stopped writing. Traced by @sprint-review from the Reset handler.2. Trailer on the default branch of
ensureHeartbeatTemplate— the payload. Since no preset matches, the content written is the default template, and without this it goes out untrailered.Gate 1 without Gate 2 only helps freshly provisioned agents. Gate 2 without Gate 1 rewrites the files with content that still has no directive.
The grep marker is exported from the trailer's own module rather than re-typed at the grep site. A second literal would drift silently, and the drift would present as "nothing to rewrite" rather than as a failure.
Second commit: the guard I cited didn't exist for the path that mattered
The staleness clause was justified by
customizations.heartbeat === true. That flag was never set by the endpoint that performs hand edits (routes/registry/files.ts, which writes HEARTBEAT.md verbatim and never touchescustomizations), so the clause would have clobbered hand-authored files. Fixed at the cause: the endpoint now records the file as user-owned, andresetclears it.The
resethalf is load-bearing, not symmetry.skipHeartbeatiscustomizations.heartbeat === true && !heartbeat?.forceOverwrite, andforceOverwriteisBoolean(explicitPresetId). Corrected 2026-08-04: an earlier revision said that never fires in this fleet, resting on the retracted "presetId unset for all 30" — it does fire, for every row that carries a preset. It does not fire for agents whose rows carry none (aria,nova-demo,pixel-demo) or for the four with no row at all, and for thoseresetis the only path back to provisioner ownership once the flag is set. Setting it unconditionally on write would have made the reset button restore the content while leaving the file permanently outside the provisioner, including for future trailer changes. Credit @ux-lead for tracing the escape hatch; the test on the reset path guards the sole exit for the agents the force path cannot reach.This also explains the 5 agents carrying the trailer while matching no preset id (fakesam/liz/tarik/tom, mtime 2026-05-24; ops 2026-07-29) — written through that endpoint, which is why no store records a
presetIdfor them.What this does NOT do: restore cycle logging
Added 2026-08-04 after @ux-lead read
agentmemories. Every moltbot's most recentcyclesappend is 83-87 days old, dating to when this trailer started namingcommonly_log_cycle; MCP seats append hourly. That includes the 17 agents whose HEARTBEAT.md already carries the correct trailer verbatim —tarik,x-curatorandlizheartbeated 90 seconds before that reading and last wrote a cycle 87 days ago.Confirmed here against the deployed artifact rather than the openclaw repo, which is not readable from this tree (
_external/clawdbotis an uninitialised submodule): inside the runningclawdbot-gateway, the extension exposes 25commonly_*tools andgrep -rl commonly_log_cycle /appreturns nothing. The tool exists only incommonly-mcp/src/tools.js:337.presets.tsasserted the opposite in a comment; corrected in0f20dd68, same file, evidence rather than verdict.So the premise this PR was built on — missing or wrong directive text → no cycles — has a control group that falsifies it as a sufficient condition. Correct text is necessary and this PR delivers it; it will not by itself make any moltbot write a cycle. The remaining work is in
Team-Commonly/openclawand is not in this diff.Coverage: this repairs at most 6 of the 10, not all 10
Corrected 2026-08-04 after @ux-lead read the live
agentinstallationscollection. Both gates live inensureHeartbeatTemplate, which has exactly one call site —agentProvisionerServiceK8s.ts:2569, inside the per-installation provisioning path.reprovision-all(routes/registry/admin.ts:164) iteratesAgentInstallation.find({ status: 'active' }).So neither gate can reach a workspace that has no active installation row. Of the 27 PVC workspace dirs, 15 have no installation row at all, and four of those fifteen are in the ten needing repair:
backend-engineer-nova,dev-pm-theo,devops-engineer-ops,newshound-aiyo. This PR plus a reprovision does not fix those four.Why those workspaces outlive their rows, and what the four actually are (traced by @ux-lead, verified here 2026-08-04). The gateway's index is
/state/moltbot.jsonagents.list— 28 entries, each storing itsworkspacepath explicitly. It is appended to atagentProvisionerServiceK8s.ts:2507on first provision and never rebuilt from the DB. A remove path does exist (:2450-2453), but it is keyed onremovedAccountIds, which:2317-2326populates only from duplicate account detection during a provision of the same identity — not from uninstall. An agent whose install row is gone is never provisioned again, so it is never a duplicate, so it is never pruned: the orphan state is self-sustaining.Of those four, exactly one is actually running. Gateway logs, 2026-08-04:
backend-engineer-nova,dev-pm-theoanddevops-engineer-opshave zero log lines in 24h and no live session files (last session state 2026-05-04 / 05-19) — cold shells.newshound-aiyois ticking every ~23 min, and every turn fails:FailoverError: 401 Missing Authentication header, followed bymessage posted … postedId=n/a. So the fleet-state class is real but it is one live agent, not four — and its harm is larger than a stale HEARTBEAT.md, because nothing reprovisions it and nothing reissues whatever credential it is missing. That is a separate piece of work from this PR and wants a sweep keyed onagents.list, not on installations.What writes and sustains the row-less workspaces is not established — 11 of the 15 carry the correct trailer, so the surface is reachable by something, but nobody has shown by what. That question is open and this PR does not answer it.
Earlier claims in this description were wrong and are corrected inline above, at each site rather than only here — the first pass corrected this summary and left three live copies of the retracted null at lines 18, 24 and 35, which is how a retraction reads as confirmation. They were: that
config.heartbeat.customContentmight shadow Gate 1 (it is set on 0 of 417 installations, so the hole is empty), and thatpresetIdis unset fleet-wide (it is set on 54; the earlier null came from queryinginstallationConfig.presetId, a path that does not exist in the collection).Known residual risks — stated, not hidden
The customization flag is not retroactive. It protects edits made from now on; any hand-authored
HEARTBEAT.mdalready on disk is unflagged, so Gate 2 will rewrite it if it lacks the trailer. The 5 above are safe by content (the grep won't fire), not by flag.AgentProfile.heartbeatContentkeeps a recoverable copy, but this is a behavior change and should be a decision, not a surprise.CodeQL: resolved, and the fix was real rather than a silencing.
AgentInstallation.updateOneadded a DB write to an unlimited handler, producing high-severity alert #1720 (js/missing-rate-limiting). Adding a limiter inline did not clear it, and an earlier revision of this description concluded the query "has never worked" — citing #1658, open since 2026-05-11 against a route that hadinspectorRateLimitapplied the whole time. That conclusion was wrong, and it was wrong for a reason worth recording: it was drawn from two failures with zero successes sampled.The cross-tab against main settles it:
The query anchors to the first middleware in the chain, and
authperforms a Mongo lookup — so a limiter placed afterauthgenuinely leaves that lookup unprotected. The scanner was reporting a real gap.2c21f0f8moves both limiters in this file ahead ofauth; becausereq.userIdis not set that early, the key generators now hash theAuthorizationheader, the idiomroutes/messages.tsalready uses for its pre-auth limiters and the reason those routes are clean. Per-caller isolation is preserved and theipKeyGeneratorfallback for unauthenticated callers is unchanged.Result:
CodeQLfail → pass; zero rate-limiting alerts onrefs/pull/827/headfor this file. #1658 closes when this merges — the a2a-dms route is reordered in the same commit.A later commit raises
workspaceWriteRateLimitfrom 30 to 120/min (and the 429 message with it). 30 was a guess; @sprint-review measured the actual batch size. The UI's five call sites are all single-agent from an open dialog, so no human reaches 30 by clicking — but a scripted fleet-wide heartbeat repair, the operation this PR exists to make correct, is one POST per agent under one operator token, and the fleet is 27. 27 of 30 is a coincidence with an expiry date, and it fails by killing a repair script partway into exactly the split population this PR is untangling. 120 matchesinspectorRateLimitin the same file.The same wrong same-file claim survived in three more files —
agentsRuntime.ts:39(the origin, refuted by its own/memoryroutes 2000 lines below),registry/install.ts:31andregistry/provision.ts:41. All three corrected, comment-only. install and provision turned out to be clean for a reason neither comment stated: both apply the limiter beforeauth. Three of the nine after-auth routes are unflagged and I cannot explain them; the eightagentsRuntime.tsroutes share the flagged shape butagentRateLimitKeyGeneratormay depend on auth-set state, so they are deliberately untouched here.Verification
Tests assert on the base64 payload actually written into the gateway pod, not on the source that composes it.
writes the cycle directive even when no preset matched✕, others passtreats a file missing the directive as stale✕, others passupdateOneauthputs its limiter at position 0✕, others pass18/18 across the affected suites ·
npm run tsc:checkclean.The
never names a tool that cannot append to cyclestest passes with and without the fix — a standing regression guard, not evidence for this change.Scope
reprovision-allafter merge. Before merge the marker gate does not exist, so the only pre-merge repair path is the preset force-write, which reaches rows carrying apresetIdand no others — see the corrected note under Gate 1.agentProvisionerService.ts(non-k8s) has the same missing import. Separate PR.appendCycleempty-content silent success folds into fix(memory): report both silent cycle mutations, and reduce the cue's two moltbot failures to one cause #804.commonly_write_agent_memory(). It writes the memory envelope, which is that tool's actual contract — and it is the line pixel-demo was mis-bucketed on.🤖 Generated with Claude Code