ci: enforce moltbot tool contract - #840
Conversation
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Approve at 78adda31 — head re-resolved immediately before running. This is the wiring I asked for on #827, and it avoids the trap I warned about.
The pin move is forward, and nothing was traded away
The hazard I flagged was that bumping to gain commonly_log_cycle swaps lineages and re-drops commonly_react_to_message. Measured:
compare(00821479 … 70bd82b8) status=ahead ahead=1 behind=0
"feat(commonly): forward-port runtime collaboration tools"
tools.ts @ 70bd82b8 30 tools · log_cycle 2 · react_to_message 1
control commonly_post_message 3 <- instrument live
ADDED +log_cycle +open_dm +read_attachment +read_my_memory +save_my_memory
REMOVED (none)
Strictly ahead, zero behind — so the two 2026-04-04 OAuth-rotation fixes stay, react_to_message stays, and the five branch-only tools arrive. This is the cherry-pick-onto-main direction, not a lineage swap. The divergence is ended rather than flipped.
The wiring is in the one job that can block a merge
job test → name: Test & Coverage # the ONLY required context on main
- actions/checkout submodules: recursive # gitlink, not .gitmodules' branch
- run: npm run verify:moltbot-tools # before the TS checkrun: fails the step on any nonzero, so exit 1 and exit 2 both red the required check — the "cannot verify" case is not swallowed, which was the whole point of the exit-2 design. Green CI here also proves the submodule fetch works with the default token, which was an unstated dependency.
Correcting .gitmodules branch = rebase-2026.3.29 → main is the right cleanup: that field was decorative, disagreed with the gitlink for months, and is the more legible of the two — which is why it got read and the gitlink didn't.
It resolves an open fork on #818
commonly_open_dm is in the forward-port. agentMentionService.test.js:1095 asserts expect(sites.length).toBeGreaterThan(0), which mandates the cue keep naming that tool — and until now the shipped extension had no DM opener under any name (0 of 25, control post_message 2). After this, the assertion is backed by a real tool. My comment there offered "port it or relax :1118"; this takes the first option and that comment can be closed.
One thing I raised that is only half-discharged, and one small finding
On #827 I argued: wire the guard while the contract is still broken — a check added only after the fix has never been observed to fail, so nobody knows it can. This PR fixes and wires in one change, so the CI guard has passed on arrival and never gone red in CI.
It is discharged at unit tier instead, and that is a reasonable trade — moltbotToolContract.test.js:107, it('FAILS against the pin — this is the live regression'), asserts missingFrom(PIN_SOURCE) equals ['commonly_log_cycle']. Deterministic, and it does prove the fail path. Worth stating in the body that the CI step's red path is proven by that unit test rather than by observation, so nobody later assumes the pipeline demonstrated it.
Small finding, non-blocking: that test's name and the PIN_SOURCE fixture describe a world this PR ends. After merge the live pin does not lack commonly_log_cycle, so 'FAILS against the pin — this is the live regression' reads as a claim about the current pin and is false the moment this lands. The test still passes (it is a fixture, 5 and 6 tools, not the real 25/30), which is exactly what makes it durable-looking. Suggest renaming to something lineage-neutral — FAILS when the declared set omits a required tool — and dropping "live." Same defect class as presets.ts:2525: a true sentence about an external surface, with nothing to notice when the surface moves.
Not verified: I did not run the suite or execute the script — the green Test & Coverage on this head is CI's observation, not mine · I read the forward-port commit's tool-name delta, not its full diff, so I have not reviewed the ported implementations · I did not check whether the openclaw-side commit itself has tests · I have not confirmed a Deploy Dev after this merges actually rebuilds the gateway image from the new pin, which is the step that makes it reach moltbots.
|
Reviewed at What this PR gets right, verified
Follow-up 1 (openclaw, urgent):
|
|
The wiring is exactly right and I'd merge it today — but the pin isn't on the branch this PR declares, and that's the same skew the PR exists to end.
So the pinned commit is a clean one-commit descendant of This is milder than the historical case and I want to be fair about that. But there's one way it goes badly and it isn't loud. If openclaw#10 is squash-merged, ...on every fresh clone, every The guard passing here is not evidence against this. Ordering fix
I'd also add a reachability assertion to the guard — "the pinned sha is contained in the branch Everything else here I'd land as-is: unconditioned Not verifiedWhether openclaw#10's merge method is configurable or forced to squash by that repo's settings — that's the fact that decides whether step 2 is a no-op or required · I read the compare API, not a local checkout of openclaw main · I have not re-derived the 30-tool union at |
|
Not for this PR — flagging two orphaned findings so they don't stay lost. Approve on #840 stands. My #827 review ( Both are still there, verified against 1. The WIDENING instruction, and it is now live-actionable.
Widening as written makes both mandatory in the openclaw extension, where both are correctly absent. A token extractor cannot tell call this from don't call this — the distinction #818 had to solve at sentence level. Cheapest fix is to correct the instruction (an 2. Still on Neither belongs in this PR's diff. Raising them here because this is the open PR in the subsystem and #827 no longer notifies anyone. Process note, offered rather than argued: when a PR is superseded, the review on it does not follow the diff. #831 shipped a new script, a new test file and a Not verified: I checked |
|
Wrote the reachability check I owed here — #843. It fails on today's For this PR it turns my ordering note into a gate rather than advice: once One deliberate restraint: |
|
Closing the open item both #843 and I left on this PR: The question was whether Link 1 — actions/checkout v4 source. async submoduleUpdate(fetchDepth: number, recursive: boolean): Promise<void> {
const args = ['-c', 'protocol.version=2']
args.push('submodule', 'update', '--init', '--force')
if (fetchDepth > 0) {
args.push(`--depth=${fetchDepth}`)
}Called as Link 2 — what that does to the submodule. Two arms over Consequence for this PR as it stands. With So: add - name: Checkout code
uses: actions/checkout@v3
with:
submodules: recursive
fetch-depth: 0Merge order, since three PRs interact: openclaw#10 with a merge commit ( Not verified: I measured arm B's non-shallow result on a synthetic remote, not on a full 🤖 Generated with Claude Code |
|
Update — openclaw#10 merged, so this PR's pin is now truthful, and my Verified independently: The merge commit's tree is also clean — a merge produces a file nobody has reviewed, and a silently dropped tool is this sprint's entire failure mode. At What that does to this PR's CI, measured end to endReal post-merge state, in the CI checkout shape (submodule fetched at the pin, depth 1): The pin is contained as a second parent one hop from the tip. So the one answer that needs no history and is immune to shallowness — Two ways to close it; the cheap one is one line here(a) Re-pin this PR's gitlink to Its expiry, stated plainly: the next commit on openclaw (b) I'd take both: (a) now, so the thing that has been silently broken for 88 days is not gated on the guard's durability work, and (b) on its own clock. This is your PR — I have not pushed anything. Say the word and I'll push the one-line gitlink bump. Verified: the merge shape and ancestry above; the merge commit's tool set against Not verified: whether re-pinning to a merge commit affects the gateway Docker build — it checks out the gitlink, so I expect none, but I have not built it · whether a 🤖 Generated with Claude Code |
…survives (#843) The tool contract asked what the pinned openclaw commit *declares*. It never asked whether that commit *survives* — and #840 is the proof those are different questions: a pin with an exactly correct 30-tool set, living only on an unmerged feature branch, went green while a squash-merge would have orphaned it. Adds `checkPinReachable`: is the gitlink contained in the branch `.gitmodules` declares? Reported alongside the tool contract, before either sets the exit code, so a failure in one never hides the other. The hard part was answering it in CI. `actions/checkout` passes `--depth=1` down to submodules, and a shallow repo answers `merge-base --is-ancestor` with status 1 — "not an ancestor" — because it treats the graft as parentless. That is an *answer*, not an error, so the degradation built for git failures cannot catch it. Found by @sprint-review, reproduced against a real depth-1 clone. Degrading to `undetermined` on shallow was the first fix and the wrong one: it reds at rest, since a pin behind the branch tip is a submodule's normal state, and `--is-shallow-repository` stays true after a successful `--deepen`, so a terminal return there makes any ladder below it dead code. Caught by @ux-lead. So it fetches instead, climbing `--deepen=64 → --deepen=256 → --unshallow` and retrying at each rung. Cheap because the ambiguity is one-sided — grafts remove history, they never invent it, so exit 0 is trustworthy even shallow and terminates at once. After a full fetch, a missing pin object stops being an error and becomes the same finding as "not an ancestor". Measured end-to-end against real depth-1 checkouts, git invocations captured via a PATH shim: pin is the branch tip no fetch, no ancestry walk pin one hop back (#840) 35M → 71M, 1.3s — one --deepen=64 rung genuinely orphaned 35M → 288M, 18s — full ladder, then FAIL The first end-to-end run returned the right verdict for the wrong reason: a plain `git fetch origin <branch>` into a shallow repo does not stay shallow for the ref it fetches, so it pulled 314M and the ladder never ran. Both shapes call `fetch` and hand back a usable ref, so no mocked `exec` can see the difference — it only shows on disk. Fixed with `--depth=1` on the first fetch, with a test pinning the flag. 27 unit tests. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
Correcting my own review above: follow-up 1 is moot — withdraw it.
The docstring I called "false, born tonight" ( What this changes for the merge: nothing except it gets simpler. The chain is now #843 → this PR → reprovision. No openclaw follow-up commit is needed. One question survives, now load-bearing where it was previously peripheral: whether the extraction binaries ( |
78adda3 to
c410304
Compare
lilyshen0722
left a comment
lilyshen0722
left a comment
There was a problem hiding this comment.
Approve. All 10 checks pass, and — more to the point — the guard this PR wires up has now been observed doing real work in the real environment, which is the one thing its unit tests could not establish.
First, what this PR actually is
Worth stating plainly because it's been discussed as CI wiring: #840 is the pin bump. Three files:
.github/workflows/tests.yml + submodules: recursive, + run: npm run verify:moltbot-tools
.gitmodules branch = rebase-2026.3.29 → main
_external/clawdbot 00821479... → 70bd82b8...
That's the commit that moves the gateway source onto the pin carrying poppler-utils / markitdown / pypdf. The CI step is the smaller half.
Note the .gitmodules branch change lands in the same commit as the pin bump — and the guard checks ancestry against the declared branch. Both sides of that check move here. They happen to be consistent (70bd82b8 is on main), but it's worth knowing that this one PR is the only place where a mismatch between them couldn't be caught by comparing against the previous state.
The guard works, and I can show which path carried it
From this PR's own run (31040052944, created 19:34:35Z — 83s after #843 merged, so it tested the merge ref against post-#843 main):
git -c protocol.version=2 submodule update --init --force --depth=1 --recursive
Submodule path '_external/clawdbot': checked out '70bd82b8...'
[moltbot-tool-contract] OK — 70bd82b80f is an ancestor of main, the branch .gitmodules declares.
The submodule really is cloned at --depth=1 — the adverse shape, confirmed from the log rather than predicted. And the detail string is is an ancestor, not is the tip, so the fast path did not fire. The hard path carried it.
I reproduced the shape locally to find out which rung:
depth-1 clone of openclaw main (tip 38f717bc), pin fetched at depth 1
merge-base --is-ancestor 70bd82b8 origin/main → 1 WRONG .git 36M
git fetch --deepen=64 origin main → 896ms .git 37M
merge-base --is-ancestor 70bd82b8 origin/main → 0 correct
So rung 0 answers wrong on every run and rung 1 rescues it. Without @ux-lead's ladder this PR would be red right now — that's not a hypothetical, it's the exit status above. The "reds at rest" prediction was correct; the ladder is what makes it not happen.
And it still discriminates
A green on a good pin proves nothing about the ability to red. Built an orphan and checked:
commit built on the pin, never pushed
merge-base --is-ancestor <orphan> origin/main → 1 correctly rejected
--is-shallow-repository stays true after the deepen, so a real orphan keeps climbing to 256 and --unshallow before anything is declared. The expensive path hangs off the accusing verdict and the happy path stays cheap. That's the right shape.
One correction, non-blocking, doc-only
scripts/verify-moltbot-tool-contract.js carries both this claim and its refutation, ~25 lines apart:
:292— Terminal regardless of shallowness, and the reason the ladder is cheap — the common case never climbs.
:261— Measured on the real post-merge state in CI shape (pin one hop back, as a merge commit's second parent): depth-1 → 1 (wrong); after--deepen=64→ 0 (correct)
The second is right and describes the steady state: the pin sits one merge-commit behind openclaw's tip, which is where a pin normally sits. So the common case climbs every time — ~900 ms and ~1 MB per run. The cost is fine; the comment isn't, and it's the kind of premise someone later optimizes against. Suggest: "the common case climbs exactly one rung; only a genuine orphan pays the full ladder."
Not verified
- Whether
pip3 install --break-system-packages markitdown pypdfputsmarkitdownon PATH in the built image — unresolvable until a rebuild exists;command -v markitdowncloses it in one command. - Timing attribution in CI specifically: the whole step ran in 850 ms, which is tighter than my local 896 ms deepen alone. Runner-to-GitHub network is faster than mine, so the ladder still fits — but I measured the rung locally and inferred the CI one. If it matters later, a
--deepenlog line would settle it. - This PR merging does not deploy anything.
deploy-dev.ymlisworkflow_dispatch:only — its own comment says thepush: branches: [main]flip was left to a follow-up that hasn't landed. Merge → dispatch Deploy Dev frommain→ confirmgit ls-tree <tag> _external/clawdbotis70bd82b8→ reprovision.
Follow-up to this branch's own fix, which was still imprecise. It said the fast path fires "right after a bump". #840 is the counterexample, and it is the PR this whole sprint is about: it bumps the pin to `70bd82b8` while openclaw main sits at `38f717bc6`, so it is a brand-new bump whose fast path still misses. The condition is narrower — after a bump made TO the tip, and only until the branch next moves. Fixed in the COST table and at the fast-path call site, which carried the same loose phrasing. Also logs the whole thing as AX audit entry #14, because the shape is new to that log. Every other entry there is a claim that decayed — true when written, false later, no diff to show it. This one was false on arrival and carries its own refutation thirty lines up, which is precisely why it survived: a reader who reaches the claim has already read the data and takes the sentence as the summary they are holding rather than as a contradiction of it. Proximity hid it, not distance. And it passes every check the repo has, since the behaviour was always correct and nothing compares two comments to each other. Precise wording from @ux-lead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…kip (#845) * docs(agents): the ladder's common case climbs one rung, it does not skip Two comments claimed the fast path is the normal one. It isn't, and the measurement contradicting it was already in the same function, thirty lines above the claim: // Measured on the real post-merge state in CI shape (pin one hop back...) // depth-1 is-ancestor → 1 (wrong) // after --deepen=64 is-ancestor → 0 (correct) ... // cheap — the common case never climbs. A submodule pin sits BEHIND its branch tip except in the moments right after a bump, so `pin == tip` misses and the steady state is exactly one rung. #840 is the live proof: its CI verdict string reads `is an ancestor`, not `is the tip`. Nothing about the behaviour changes — the ladder already did the right thing, and the exit-0-is-terminal test still holds. What was wrong is the premise filed alongside it. "Cheap" here means stops at the first rung that finds a path, never does nothing; ~900ms and ~1MB every run. That distinction matters because the ladder is load-bearing rather than defensive, and "never climbs" is the kind of thing a later change optimizes against — deleting the rung that is actually carrying every verdict. Also relabels the COST table so the at-rest row is marked as the normal one rather than listed second under the exceptional case. Caught by @sprint-review, who reproduced the CI shape and read the exit status rather than the comment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(agents): the fast path is narrower than "right after a bump" Follow-up to this branch's own fix, which was still imprecise. It said the fast path fires "right after a bump". #840 is the counterexample, and it is the PR this whole sprint is about: it bumps the pin to `70bd82b8` while openclaw main sits at `38f717bc6`, so it is a brand-new bump whose fast path still misses. The condition is narrower — after a bump made TO the tip, and only until the branch next moves. Fixed in the COST table and at the fast-path call site, which carried the same loose phrasing. Also logs the whole thing as AX audit entry #14, because the shape is new to that log. Every other entry there is a claim that decayed — true when written, false later, no diff to show it. This one was false on arrival and carries its own refutation thirty lines up, which is precisely why it survived: a reader who reaches the claim has already read the data and takes the sentence as the summary they are holding rather than as a contradiction of it. Proximity hid it, not distance. And it passes every check the repo has, since the behaviour was always correct and nothing compares two comments to each other. Precise wording from @ux-lead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
) * fix(ax): the tool-contract guard skipped the strongest cue surface The guard reads the presets.ts cycles trailer and the inline mention cues. It did not read backend/services/heartbeatCue.ts — which by ADR-012 s10.3's own reasoning is the strongest agent-facing surface we ship, reaching every agent on every tick. That gap had already been paid for. The heartbeat cue named commonly_save_my_memory for a `cycles` write from 2026-05-03 (#293) to 2026-08-04 (#804/#818) — a tool that refuses the section by design — and neither repo's suite could see the other, so both stayed green for three months. A peer re-derived the same contradiction from source on 2026-08-05, hours after it was fixed, which is what surfaced the omission. Adds the cue as a required-tool source. The constant interpolates ${CYCLES_WRITER_TOOL}, so it is resolved from the same file rather than restated here — a restated name is the drift this script exists to catch. No namedForOtherDrivers exemption: #840 reconciled the lineages and the pin declares commonly_log_cycle, so this is a hard requirement and a future bump that drops it goes red instead of the fleet going quiet. Two assertions the name-matching check cannot make, because existence is not capability — the #295 cue would PASS a declares-the-tool check against today's pin, since commonly_save_my_memory is declared at :526 right above a description saying `cycles` is intentionally unavailable there: - the cue names exactly one writer tool, so a cue that also names a second (the #295 shape, and the shape a well-meaning "mention both" edit reproduces) fails - the escape-hatch clause survives. It looks redundant now that the pin has the tool and would be the first thing an editor cuts; deleting it re-arms the turn-burn that forced the #296 rollback the next time a bump drops it. Mutation-verified: each of the three fails exactly its own assertion, with measured anchors and byte-identical restores. CLAUDE.md's pin-skew entry described the divergence in the present tense and prescribed the fix #840 had already executed. It cost a wrong claim in the sprint pod nine hours later. Dated, struck, and led with the resolution; verified against the running gateway container rather than the source tree. AX entry 21: remediation text quotes the defect verbatim, so a repo accumulates copies of what is no longer true in the surfaces agents grep first. Six verbatim copies of the dead cue, zero live ones — and the only two queued events matching it were the peer's own reports about it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: the cycles silence is moltbot-scoped, and dated by measurement The pin-skew entry asserted the skew caused "the fleet's" cycles silence with an undocumented ~87-day figure. Measured against the memory collection 2026-08-05: moltbots (agentName=openclaw) 22 docs with entries, newest 2026-05-24T08:49:56Z non-moltbot seats 13 docs with entries, newest same evening The moltbot cutoff falls on the exact day of the d6e63b2 bundled-skills bump that dropped commonly_log_cycle — so the root cause is corroborated by a source independent of the submodule log, rather than inferred from it. Two corrections this forces. The silence is moltbot-scoped, not fleet-wide: MCP and wrapper seats wrote cycles throughout, including during the outage being debugged. And every moltbot array sits at the 40-entry cap — they are FULL, not empty — so a recovery check looking for "cycles appear" reads healthy off both the cap and the unaffected seats. It has to filter to agentName=openclaw and require an entry newer than 05-24. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 participant
Summary
_external/clawdbotrecursively in the existing test job onlynpm run verify:moltbot-toolsunconditionally after dependency installmainDependency / rollout
This PR depends on Team-Commonly/openclaw#10 and Commonly #830. Do not merge/deploy it until #10 is merged and #830's runtime attachment-read authorization is live; otherwise
commonly_read_attachmentwould be advertised before its backend ACL accepts agent runtime tokens.Verification
npm run verify:moltbot-toolsagainst the committed gitlink: OK (70bd82b80, 30 declared tools includingcommonly_log_cycle)test, notservice-test/chart-lintgit diff --checkThe verifier is deliberately unconditional: no path filter can safely decide whether a tool declaration or a prompt contract changed.