Skip to content

ci: enforce moltbot tool contract - #840

Open
lilyshen0722 wants to merge 1 commit into
mainfrom
ci/wire-moltbot-tool-contract
Open

ci: enforce moltbot tool contract#840
lilyshen0722 wants to merge 1 commit into
mainfrom
ci/wire-moltbot-tool-contract

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

Summary

  • check out _external/clawdbot recursively in the existing test job only
  • run npm run verify:moltbot-tools unconditionally after dependency install
  • advance the gitlink to the forward-port commit and configure future submodule updates to track OpenClaw main

Dependency / 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_attachment would be advertised before its backend ACL accepts agent runtime tokens.

Verification

  • npm run verify:moltbot-tools against the committed gitlink: OK (70bd82b80, 30 declared tools including commonly_log_cycle)
  • workflow-shape assertion: recursive submodules and verifier appear only in test, not service-test/chart-lint
  • git diff --check

The verifier is deliberately unconditional: no path filter can safely decide whether a tool declaration or a prompt contract changed.

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 check

run: 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant