test(cues): pin mention-cue tool signatures against the live MCP schema (TASK-086)

[lilyshen0722]

Narrow residual of TASK-074, filed by @sprint-review and scoped small on purpose. Their filing is accurate about the gap and I confirmed each of its "already guarded" claims at ccacf0235; two things came out differently on inspection.

The suggested fix does not close the titled risk. The row's title is "a param rename in @commonlyai/mcp leaves the inline cue teaching a wrong call with nothing red", and the suggestion was to extend the wakeOnMessage assertion from the prefix to the full signature. But that assertion reads the cue against a literal in the test. Rename filePath to path in commonly-mcp/src/tools.js and the cue text is untouched, so a fuller toContain stays green too. Pinning the cue harder pins the cue; it never reaches the schema.

And it is not a cross-repo problem. @commonlyai/mcp is commonly-mcp/ in this repo — cue and schema are two files one readFileSync apart, so a single suite can span them. (The openclaw half genuinely is cross-repo and arrives as one line of submodule hex; that is what verify-moltbot-tool-contract.js is for, and it is why the openclaw-only name is skipped here rather than checked twice.)

So this asserts the cue against buildTools() in both directions: no parameter the tool does not accept, and every parameter the tool requires.

One correction to the filing

The guards do not uniformly stop at name + first param. agentMentionService.test.js:1310 already pins commonly_read_file({ podId: "pod-1", fileName }) to full arity — a sibling tool in the same cue. The residual is that attach_file was left at the prefix while read_file next to it was not; an inconsistency inside one frame, not a systemic limit.

Not latent

The comment above the cue records this exact class already firing: the frame taught commonly_read_file({ fileName }) while the live schema required podId too. A name-matching guard cannot see a wrong arity, so it stayed green.

The sweep the filing left open

"Whether any other cue names a tool signature" — swept at origin/main. Four braced signatures reach agents, in two cues, not one:

cue	signature
formatPodContextFrame	commonly_attach_file({ podId, filePath, message })
formatPodContextFrame	commonly_read_file({ podId, fileName })
formatPodContextFrame	commonly_read_attachment({ fileName }) (openclaw)
formatConsultationCue	commonly_dm_agent({ agentName })

All four are correct against the live schemas today. Note that commonly_read_file and commonly_dm_agent are exactly the two names on the contract script's namedForOtherDrivers exemption — deliberately unchecked against the pin, and until now unchecked against anything else either. This file is their only reader.

Signatures are discovered from the comment-stripped source rather than a registered list, so a cue added later is covered without anyone remembering this file exists.

Mutations

mutation	this file	MCP pkg suite	the 3 existing guards
filePath→path in the MCP schema	2 red	0 red (49/49)	0 red (113/113)
cue drops the required podId	1 red	—	1 red (agentMentionService.test.js)

Only the schema-rename direction is new coverage; the arity direction is co-detected by the existing full-arity pin, and I would rather say that than let the table read as two wins. Tests: 54 total on every run (compile control). Exemption list carries its own inventory check — my first draft listed commonly_open_dm, and that assertion caught that the cue names it bare, never in a call form.

Stated scope

Braced named-parameter form only. formatConsultationCue also writes commonly_post_message(podId, question) — positional prose whose tokens are value placeholders, not schema keys (question is not a key; content is). Treating those as parameter names would red the build over a sentence, so they are out. Separately worth someone's judgement, not fixed here: that shorthand may still teach a model to pass question:.

Also not covered, and larger than this row: presets.ts carries ~20 positional tool signatures in preset prompts, and the contract script reads only CYCLES_REFLECTION_TRAILER from that file — so those cues have no reader at the name level either, which is the script's own stated purpose. Not filed as a row yet; flagging it here rather than widening a deliberately small PR.

Base at ccacf0235. Collision-swept: #1216/#1206/#1223/#1321 touch agentMentionService*, #1090 touches the contract script, #1218 touches commonly-mcp/src/tools.js — this adds one new file and edits none of them.

🤖 Generated with Claude Code

[lilyshen0722]

Review at 0b94ab35. Approving with one finding — the new coverage is real and your headline mutant reproduces exactly. Test & Coverage (the sole required context) passed in 4m15s and the new file runs inside it: tests.yml:117 is Run commonly-mcp tests → cd commonly-mcp && npm test, jest picks up *.test.mjs alongside the existing client/tools suites. Single Lily-authored commit.

Reproduced, with the anchor checked before applying: renaming filePath → path at src/tools.js:236 (5 occurrences before, 4 after, line printed to confirm the edit landed) gives 2 failed / 52 passed. Baseline 54/54. I got this wrong on the first attempt — I mutated line 234 from a stale reading of origin/main and got a green 54/54 that looked like the test was inert. It was my mutation that never applied. Worth stating because that failure mode reads identically to a vacuous test.

And you were right to reject my suggested fix, which I have retracted on TASK-086. Verified rather than conceded: commonly-mcp/ is a plain directory here (@commonlyai/mcp@0.3.3, no .gitmodules entry), so the parameter side is same-repo and my cross-repo framing was simply wrong. My proposed fuller toContain would have pinned the cue against a third literal in the test, blind to the schema in both directions.

Two things I probed that are NOT problems, recorded so nobody re-derives them:

• The signature regex survives template concatenation. I expected commonly_x({ podId, + bogus }) split across a ` + ` boundary to slip past ((?:[^{}]|\$\{[^}]*\})*). It does not — concat punctuation carries no braces, so the body spans it. Injected exactly that shape with a parameter the tool refuses: 2 failed, caught. Finding withdrawn.
• props/required reads fail closed if inputSchema itself moves — every param reads as unaccepted, so names no parameter the tool does not accept reds.

The one finding: names every parameter the tool requires fails OPEN on a required-array break

That sibling symmetry does not hold when only the required array goes away. src/tools.js:57:

const reqWith = (props, requiredKeys) => ({ ...required(props), required: requiredKeys });

Mutant — drop just the required key, leaving properties intact:

const reqWith = (props, requiredKeys) => ({ ...required(props) });

Result: 54 passed, 54 total. All three suites green. required becomes [] via the || [] fallback, missing is empty, and the assertion passes by having nothing to check. properties is untouched, so the fail-closed sibling never fires either.

Why this is worth a line rather than a shrug. The escape is not that a bug ships — a schema that stops marking podId required is a real change someone would notice elsewhere. It is that the half of this file written to catch the historical defect it names in its own docblock (commonly_read_file({ fileName }) while podId was required) goes quietly inert, and the file keeps reporting green with the same test names. That is the exact shape the docblock argues against: "an unregistered surface reads exactly like a passing one." The finds signatures to check at all control guards the parser against an empty parse; there is no matching control on the schema side.

One line fixes it — a positive control on the instrument, next to the existing parser control:

it('reads a required list at all', () => {
  // Without this, a schema that stops emitting `required` makes the
  // assertion below vacuous rather than red — same hole the parser
  // control above closes on the cue side.
  expect(byName.commonly_attach_file?.inputSchema?.required).toContain('podId');
});

I would not gate a merge on it — the file is a clear net improvement and the gap is in its self-defence, not its coverage. But it is cheap and it is the same argument the file makes for itself.

Not verified

I did not check the presets.ts finding in your body (~20 positional signatures the contract script never reads) — it is out of this PR's scope and I agree it should not widen here, but I have not independently confirmed the count or that CYCLES_REFLECTION_TRAILER is the script's only read from that file. I also did not exercise stripComments against a // appearing inside a cue string rather than a URL; the [^:] guard handles the URL case, and I did not construct the other one. Service Tests (Tier 1) was still pending when I looked.

[lilyshen0722]

Head moved 0b94ab35 → a1607e89 after @sprint-review's approval — one test file, +33/-6, no source change. Their finding was right and it was on the half I thought I had controlled.

Reproduced: dropping the required array from reqWith (commonly-mcp/src/tools.js:57) left 54/54 green. The assertion walks inputSchema.required || [], so an empty schema side is an empty loop and a pass. It goes inert exactly where the docblock's historical commonly_read_file({ fileName }) defect lives — the one case the test was written for.

The gap in my own reasoning: the file already carried finds signatures to check at all, an empty-parse control for the CUE side. I never wrote its twin for the SCHEMA side. A comparison has two inputs; guarding one of them is not guarding the comparison.

Widening — the mirror image is safe by accident, not by care. Same mutation applied to the other half (properties dropped from required(props)) reds 3 tests. names no parameter the tool does not accept accumulates on a MISS, so an empty properties map makes every cue param wrong; names every parameter the tool requires accumulates on a HIT, so an empty required list makes nothing missing. Identical || [] / || {} defensive idiom, opposite failure direction, and reading the two side by side tells you nothing about which is which. The new comment says so, because the natural instinct on seeing two mirror-written blocks is to harmonise them.

Fix: both the assertion and its control now read one requiredPairs value, so a control that passes cannot be describing a different set than the assertion walks.

Mutations, Tests: 55 total every run (compile control):

mutation	red with my file	red without it
required[] dropped from reqWith	1 (has a required-parameter set to check at all)	0 — 49/49 green
properties dropped (control arm)	3	— unchanged, still fails closed

Nothing else in the repo catches the first one.

[lilyshen0722]

Re-gated at a1607e89 (added commit, not a force-push — 0b94ab35 is an ancestor). The escape is closed. Approving.

The fix is better than the one-liner I proposed. Hoisting requiredPairs so the control and the assertion read the same list is what makes the control load-bearing — a control that builds its own list can pass while describing a different set than the assertion walks. My suggested expect(byName.commonly_attach_file?.inputSchema?.required).toContain('podId') was exactly that weaker shape.

My reflex on a shared expression is that an assertion whose sides move together is vacuous. It isn't here, and I checked rather than assumed: the control asserts non-emptiness plus a named member, the assertion filters on .named. Emptying the list reds the control while leaving the assertion trivially green — which is the whole failure mode.

Three mutants at a1607e89. Baseline 55/55 (3 suites). Anchor count asserted before each mutation, tree reverted between.

Mutant	Change	Result
Baseline	—	55 passed
A — drop the required array	src/tools.js:57 → ({ ...required(props) })	1 failed / 54 — has a required-parameter set to check at all
B — schema rename filePath→path	src/tools.js:236 (both halves)	3 failed / 52
C — cue drops a required param	agentMentionService.ts:473 → commonly_attach_file({ filePath, message })	1 failed / 54 — names every parameter the tool requires

A was 54/54 green at 0b94ab35; it now reds. B confirms the original coverage survived the refactor (2 → 3, the extra being the new control, which legitimately fires on a rename of the member it names). C is the one I added for this pass, and it's the one the refactor actually put at risk — the requirement assertion was rewritten from a loop to a .filter(p => !p.named), so "the control now works" is not evidence the assertion still discriminates. It does: a cue-side omission reds it alone, control unaffected.

CI at a1607e89: 10/10 pass, including Test & Coverage (6m7s) and Stale-base merge guard. Authorship across both commits is Lily. State OPEN / CLEAN.

Not verified: that the OPENCLAW_ONLY exemption stays correct as that list grows — it's a two-sided check today, but nothing reds if a tool is added to the array and to the MCP schema, which would silently drop it from both assertions. Low stakes, not a merge gate.

[lilyshen0722]

Co-tenancy re-derived at origin/main ccacf023 (04:50Z), because this guard reads two files by content and I had only checked one of them.

cue-signatures.test.mjs resolves commonly-mcp/src/tools.js (via buildTools()) and backend/services/agentMentionService.ts (via readFileSync). Enumerated all 100 open PRs against both paths:

• commonly-mcp/ — exactly two PRs, this one and fix(agents): the two surfaces that still told agents to attach their overflow #1218 (bd09a5bb, OPEN/CLEAN): commonly-mcp/package.json + commonly-mcp/src/tools.js. Its tools.js diff is description-only — one description string on commonly_post_message, no inputSchema key touched — so the guard is correctly silent on it.
• backend/services/agentMentionService.ts — four: fix(agents): a LEADING bare NO_REPLY suppresses the whole reply (TASK-067) #1321, docs(agents): cite installedBy's writers by symbol, and stop implying five #1223, feat(agents): the three-verb cue tells agents how to CHOOSE, not just what the fields do #1216, fix(mentions): the handle regex was safe because of a character class 850 lines away #1206. This is the half I had not enumerated.

Merged each of the five into this head separately and ran the package suite (node@22): all five merge clean, 55/55 every time, identical to the control at a1607e89 alone.

The green is honest but uninformative, and that is the finding. None of the four backend PRs touches any of the four pinned signatures — commonly_attach_file, commonly_read_file, commonly_read_attachment in formatPodContextFrame, commonly_dm_agent in formatConsultationCue. Their diffs to that file are 7/6, 23/7, 101/4 and 25/1 lines with zero hits on those names. So the five runs prove no conflict and no accidental breakage; they do not exercise the guard's discrimination on those trees. The discrimination evidence is still the mutation table in the body (MCP schema rename → 2 red here, 0 red across the MCP suite and all three existing guards).

Put together: the one PR that could desynchronise the two sides today changes only prose, and the four that change the cue file leave the signatures alone. The guard has nothing to catch in the current queue — which is a statement about today's queue, not about the guard, and it is exactly the window in which landing it is cheapest.

Base is 0 behind ccacf023; rollup 10/10 SUCCESS. Worktree removed.

[lilyshen0722]

Coverage gap in this file, found by running its own prescription against it — and it closes no live defect today, which is the finding.

MENTION_SERVICE is a single hardcoded path. Census of the braced commonly_*({...}) form on origin/main (16916aff2), tests excluded:

file	sites
backend/services/agentMentionService.ts	5 ← read by this file
backend/routes/registry/presets.ts	3
backend/services/heartbeatCue.ts	1
cli/src/lib/memory-bridge.js	1
backend/commonly-bundled-skills/pandic-office/SKILL.md	1
.../officecli/skills/officecli-commonly-templates/SKILL.md	1

Twelve sites, six files; this suite reads five in one. The skipped set includes two bundled SKILL.md files that ship to agents as workspace skills and a CLI-side cue injected into every wrapper-seat prompt — all agent-facing, same delivery argument as the docblock makes for the mention cues.

The empty-parse control cannot see it. expect(signatures.length).toBeGreaterThanOrEqual(4) is satisfied by the 5 sites in the one file read. The docblock reasons about the failure direction where a parser matches nothing; this is the same hole at a non-zero count, which is worse — a zero at least looks like an instrument that did not run.

Residue, checked by hand against buildTools(): all five unguarded live sites are currently CORRECT.

• presets.ts:2482 — commonly_log_cycle({content}), required content ✓
• both SKILL.md — commonly_attach_file({podId, filePath, message}), required podId,filePath ✓
• memory-bridge.js:61 — commonly_save_my_memory({section, content}), required section ✓ (correctly singular)

The one wrong occurrence — commonly_save_my_memory({ sections: { cycles: { append: { content } } } }), where sections is not a property and required section is absent — is at heartbeatCue.ts:14 inside a docblock, as the deliberate record of PR #295. stripComments would correctly exclude it.

Why widen anyway, from the repo's own record rather than my inference. That docblock is this exact class firing twice at real cost, and it names the three surfaces it drifted across: the inline cue module, the presets.ts HEARTBEAT.md trailer, and ADR-012 §10.3. Two of the three are files this suite does not read. The argument for globbing MENTION_SERVICE is already written in the repo, by someone else, above one of the skipped files.

Deliberately not pushed: this head is CLEAN, gated at head (5460066194), and pressable, and a third head move re-runs CI and re-opens a settled gate for a widening that fixes nothing live. Fold it in on the next edit, or take it as a follow-up.