From 0b94ab3585f07eb6a9fc17be6ccd13a53b6795ac Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 28 Aug 2026 20:01:43 -0700 Subject: [PATCH 1/2] test(cues): pin mention-cue tool signatures against the live MCP schema (TASK-086) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The inline mention cues name tool SIGNATURES. Two guards already read them and both stop at the name: `moltbotToolContract.test.js` requires every cue-named `commonly_*` to be declared by the pinned openclaw extension, and `wakeOnMessage.test.js:315` pins the attach_file cue as far as `commonly_attach_file({ podId: "pod-1"`. Neither answers whether the call the cue teaches still typechecks against the tool. The parameter side is not a cross-repo problem: `@commonlyai/mcp` is `commonly-mcp/` in this repo, so cue and schema are one readFileSync apart. This adds the span. It discovers braced signatures from the comment-stripped cue source (so a cue added later is covered without registering it), and asserts both directions against `buildTools()`: no parameter the tool does not accept, and every parameter the tool requires. The arity half already fired once — the comment above the cue records the frame teaching `commonly_read_file({ fileName })` while `podId` was required. Mutations: rename `filePath`→`path` in the MCP schema → 2 red here, 0 red in the MCP package's own suite and 113/113 green across all three existing guards. Cue drops the required `podId` → 1 red here, and 1 red in `agentMentionService.test.js`, which already pins that one signature to full arity — co-detected, so only the schema-rename direction is new coverage. 54 total each run (compile control). Co-Authored-By: Claude Opus 5 --- .../__tests__/cue-signatures.test.mjs | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 commonly-mcp/__tests__/cue-signatures.test.mjs diff --git a/commonly-mcp/__tests__/cue-signatures.test.mjs b/commonly-mcp/__tests__/cue-signatures.test.mjs new file mode 100644 index 000000000..e58986c3f --- /dev/null +++ b/commonly-mcp/__tests__/cue-signatures.test.mjs @@ -0,0 +1,146 @@ +/** + * The inline mention cues name TOOL SIGNATURES, and nothing spanned the cue + * text and the tool schema until this file. + * + * WHAT WAS ALREADY COVERED, verified at `ccacf0235`: + * - `moltbotToolContract.test.js` requires every `commonly_*` NAME a cue + * mentions to be declared by the pinned openclaw extension. + * - `agentMentionService.wakeOnMessage.test.js:315` pins the attach_file cue + * as far as `commonly_attach_file({ podId: "pod-1"`. + * - `agentMentionService.test.js:1310` already pins ONE signature to full + * arity — `commonly_read_file({ podId: "pod-1", fileName })`. + * + * Both existing guards answer "does this name exist". Neither answers "does + * this call still typecheck against the tool", and the two questions come + * apart on a parameter rename: rename `filePath` to `path` in `src/tools.js` + * and the name still exists, the prefix assertion still matches, and every + * woken agent is taught a call the tool rejects. + * + * THE PARAMETER SIDE IS NOT A CROSS-REPO PROBLEM, which is why this is worth + * building. `@commonlyai/mcp` is `commonly-mcp/` in THIS repo, so cue and + * schema are two files one `fs.readFileSync` apart — a rename lands in a diff + * a single suite can read. (The openclaw half genuinely is cross-repo and + * arrives as one line of submodule hex; that is what the contract script is + * for, and it is why the openclaw-only names below are skipped here rather + * than checked twice.) + * + * The arity half is NOT hypothetical. The comment block above the cue records + * this exact failure 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. + * + * SCOPE, stated rather than implied: this reads the BRACED named-parameter + * form `tool({ a, b })` only. `formatConsultationCue` also writes + * `commonly_post_message(podId, question)` — a positional prose shorthand + * 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 deliberately out. Their risk is real but it is + * a copy question, not a schema one. + */ + +import { readFileSync } from 'node:fs'; +import { fileURLToPath } from 'node:url'; +import { dirname, join } from 'node:path'; +import { buildTools } from '../src/tools.js'; + +const HERE = dirname(fileURLToPath(import.meta.url)); +const MENTION_SERVICE = join(HERE, '..', '..', 'backend', 'services', 'agentMentionService.ts'); + +const tools = buildTools({ baseUrl: 'https://x.example', token: 'cm_agent_t' }); +const byName = Object.fromEntries(tools.map((t) => [t.name, t])); + +/** + * Comments are not delivered to agents, and the comments around these cues + * quote WRONG signatures on purpose as history — including the very + * `commonly_read_file({ fileName })` arity bug this file exists to prevent. + * Counting one would red the build over a paragraph. Blank them out rather + * than deleting so nothing downstream shifts. (`[^:]` keeps `https://` from + * reading as a line comment.) + */ +const stripComments = (src) => src + .replace(/\/\*[\s\S]*?\*\//g, (m) => m.replace(/[^\n]/g, ' ')) + .replace(/(^|[^:])\/\/[^\n]*/g, (m, lead) => lead + m.slice(lead.length).replace(/[^\n]/g, ' ')); + +/** + * Discovered from the whole (comment-stripped) file rather than from a + * registered list of cue names: a cue added tomorrow is covered without anyone + * remembering this file exists. An unregistered surface reads exactly like a + * passing one, which is the failure mode all of this is about. + */ +const collectCueSignatures = () => { + const src = stripComments(readFileSync(MENTION_SERVICE, 'utf8')); + const found = []; + // `\$\{podId\}` inside the cue's own template carries a `}`, so a naive + // `[^}]*` body stops mid-signature and silently drops the tools whose first + // argument is interpolated — which is every one that matters here. + for (const m of src.matchAll(/commonly_([a-z_]+)\(\{((?:[^{}]|\$\{[^}]*\})*)\}\)/g)) { + const params = m[2] + .split(',') + .map((p) => p.split(':')[0].trim()) + .filter(Boolean); + found.push({ tool: `commonly_${m[1]}`, params }); + } + return found; +}; + +/** + * Named on purpose for openclaw seats, beside the MCP name for the same + * capability — the cue ships to every driver class unconditionally. The MCP + * package neither has nor owes these. Same exemption shape as the contract + * script's `namedForOtherDrivers`, including the check that the list cannot + * outlive the line justifying it. + */ +// `commonly_open_dm` is deliberately NOT here: the consultation cue names it +// bare ("or commonly_open_dm on openclaw runtimes"), never in a call form, so +// it carries no signature for this file to check. Listing it would have been +// an exemption for something that was never in scope — the inventory +// assertion below is what caught that. +const OPENCLAW_ONLY = ['commonly_read_attachment']; + +describe('inline mention cues teach signatures the MCP tools accept', () => { + const signatures = collectCueSignatures(); + + it('finds signatures to check at all', () => { + // A parser that silently matched nothing would pass every assertion below + // by having nothing to look at — the same empty-parse hole the contract + // script guards with its own control. + expect(signatures.length).toBeGreaterThanOrEqual(4); + expect(signatures.map((s) => s.tool)).toContain('commonly_attach_file'); + }); + + it.each(OPENCLAW_ONLY)('%s is skipped because it is an openclaw name, and is still named by a cue', (tool) => { + // If a cue stops naming it, the exemption is a hole with no remaining + // justification and would excuse a future MCP use of the same name. + expect(signatures.map((s) => s.tool)).toContain(tool); + expect(byName[tool]).toBeUndefined(); + }); + + it('names no parameter the tool does not accept', () => { + const wrong = []; + for (const { tool, params } of signatures) { + if (OPENCLAW_ONLY.includes(tool)) continue; + const props = Object.keys(byName[tool]?.inputSchema?.properties || {}); + params.filter((p) => !props.includes(p)).forEach((p) => wrong.push(`${tool}.${p}`)); + } + expect(wrong).toEqual([]); + }); + + it('names every parameter the tool requires', () => { + // The historical defect, in assertion form: the cue taught + // `commonly_read_file({ fileName })` while `podId` was required. + const missing = []; + for (const { tool, params } of signatures) { + if (OPENCLAW_ONLY.includes(tool)) continue; + const required = byName[tool]?.inputSchema?.required || []; + required.filter((r) => !params.includes(r)).forEach((r) => missing.push(`${tool}.${r}`)); + } + expect(missing).toEqual([]); + }); + + it('every cue-named tool that is not openclaw-only is a real MCP tool', () => { + const unknown = signatures + .map((s) => s.tool) + .filter((t) => !OPENCLAW_ONLY.includes(t) && !byName[t]); + expect(unknown).toEqual([]); + }); +}); From a1607e892292d9cc4fe991ba8797345c98eae052 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 28 Aug 2026 20:18:02 -0700 Subject: [PATCH 2/2] =?UTF-8?q?test(cue-signatures):=20the=20requirement?= =?UTF-8?q?=20half=20failed=20open=20=E2=80=94=20add=20its=20positive=20co?= =?UTF-8?q?ntrol?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review found it on #1330: drop the `required` array from `reqWith` in `commonly-mcp/src/tools.js` and all 54 tests stayed green. The assertion walks `inputSchema.required || []`, so an empty schema side means an empty loop and a pass — going inert exactly where the docblock's historical `commonly_read_file({ fileName })` defect lives. The file already carried an empty-parse control for the CUE side and none for the SCHEMA side. A comparison has two inputs; guarding one of them is not guarding the comparison. Its mirror image is safe by accident, not by care. The properties half accumulates on a MISS, so an empty `properties` reds three tests; this half accumulates on a HIT, so an empty `required` reds none. Identical `|| []` / `|| {}` idiom, opposite failure direction — reading them tells you nothing about which is which, so the comment says not to harmonise them. 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): required[] dropped from reqWith 1 red mine / 0 without my file (49/49) properties dropped (control arm) 3 red — unchanged, still fails closed Co-Authored-By: Claude Opus 5 --- .../__tests__/cue-signatures.test.mjs | 39 ++++++++++++++++--- 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/commonly-mcp/__tests__/cue-signatures.test.mjs b/commonly-mcp/__tests__/cue-signatures.test.mjs index e58986c3f..2b14d12a8 100644 --- a/commonly-mcp/__tests__/cue-signatures.test.mjs +++ b/commonly-mcp/__tests__/cue-signatures.test.mjs @@ -125,15 +125,42 @@ describe('inline mention cues teach signatures the MCP tools accept', () => { expect(wrong).toEqual([]); }); + /** + * Both the requirement assertion and its control read THIS, so a control + * that passes cannot be describing a different set than the assertion walks. + */ + const requiredPairs = signatures + .filter((s) => !OPENCLAW_ONLY.includes(s.tool)) + .flatMap(({ tool, params }) => + (byName[tool]?.inputSchema?.required || []).map((param) => ({ + tool, + param, + named: params.includes(param), + }))); + + it('has a required-parameter set to check at all', () => { + // The empty-parse control above guards the CUE side of the comparison. + // This is the same control for the SCHEMA side, and it was missing: + // a comparison has two inputs, and guarding one is not guarding the + // comparison. Drop the `required` array from `reqWith` in `src/tools.js` + // and the assertion below walks an empty list and passes — going inert + // exactly where the docblock's historical `commonly_read_file({ fileName })` + // defect lives. Measured: that mutation left all 54 tests green. + // + // Its mirror image is safe by accident rather than by care. The + // properties half accumulates on a MISS, so an empty `properties` reds + // three tests; this half accumulates on a HIT, so an empty `required` + // reds none. Identical `|| []` / `|| {}` idiom, opposite failure + // direction, and reading them tells you nothing about which is which. + expect(requiredPairs.length).toBeGreaterThanOrEqual(4); + expect(requiredPairs.map((p) => `${p.tool}.${p.param}`)) + .toContain('commonly_attach_file.filePath'); + }); + it('names every parameter the tool requires', () => { // The historical defect, in assertion form: the cue taught // `commonly_read_file({ fileName })` while `podId` was required. - const missing = []; - for (const { tool, params } of signatures) { - if (OPENCLAW_ONLY.includes(tool)) continue; - const required = byName[tool]?.inputSchema?.required || []; - required.filter((r) => !params.includes(r)).forEach((r) => missing.push(`${tool}.${r}`)); - } + const missing = requiredPairs.filter((p) => !p.named).map((p) => `${p.tool}.${p.param}`); expect(missing).toEqual([]); });