From 786e6fefea7488d080ab7405396a07b529febc44 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 02:26:13 +0000 Subject: [PATCH] fix(skills): drop the last undefined tool reference, guard the class in CI (#493) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `customer_360` declared `tools: ['search_knowledge']` — the survivor of the eleven fictional tools #493 counted, missed by #512 because that skill looked "already correct". The runtime silently drops an unresolved tool, so the skill shipped with its only declared capability resolving to nothing, while its instructions promised an account + cases + opportunities + knowledge roll-up it had no tool to read. Defining the missing tool was never the fix: `ToolSchema` is a read-only Studio projection — no `implementation` field, no executor loads it — so a hand-authored `search_knowledge` would validate, build, and still never run. And the knowledge base here is not a search service: it is `crm_knowledge_article`, a normal object. The skill now reads it with `query_records` alongside the contacts, cases and opportunities the description already promised, and quotes totals from `aggregate_data`. Adds `test/skills-integrity.test.ts` (its own file — metadata-references is a busy merge surface): - every skill tool resolves to a platform built-in or an `action_` tool materialised from a real Action, with wildcard subscriptions resolved by prefix; - every referenced Action is `ai.exposed` with a ≥40-char LLM description AND a headless path (flow with a defined target, or a script body) — the regression #512's follow-up commit had to fix by hand; - stack tool metadata cannot be used to satisfy a dangling reference, since it never executes; - every skill handed off to in instructions exists — the defect that pointed `case_triage` at a `response_drafting` skill that was never defined. Each guard verified against the real defect before landing: reinstating `search_knowledge`, `action_escalate_case` and the `response_drafting` hand-off each fails its own assertion. Also corrects two stale docstrings. `case_triage` and `email_drafting` justified their read-only posture by calling `escalate_case`, `close_case` and `send_email` `type: 'modal'` with no headless path; #515 and the send_email rewrite retyped all three to flow/script. The posture is right, the reason was wrong — none of the three opts in via `ai.exposed` (ADR-0011 is default off), so no tool is materialised and the human review step stays a deliberate governance call. typecheck, validate, build and the full suite pass (146 tests, +6). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y9STuduWbrAwcgviziaV4e --- .../skills-undefined-tool-references.md | 26 +++ src/skills/case-triage.skill.ts | 17 +- src/skills/customer-360.skill.ts | 49 +++- src/skills/email-drafting.skill.ts | 9 +- test/skills-integrity.test.ts | 209 ++++++++++++++++++ 5 files changed, 292 insertions(+), 18 deletions(-) create mode 100644 .changeset/skills-undefined-tool-references.md create mode 100644 test/skills-integrity.test.ts diff --git a/.changeset/skills-undefined-tool-references.md b/.changeset/skills-undefined-tool-references.md new file mode 100644 index 00000000..d25999b5 --- /dev/null +++ b/.changeset/skills-undefined-tool-references.md @@ -0,0 +1,26 @@ +--- +'hotcrm': patch +--- + +Drop the last undefined tool reference from the AI skills and guard the class in +CI. `customer_360` declared `tools: ['search_knowledge']` — the survivor of the +eleven fictional tools issue #493 counted, which #512 missed because that skill +looked "already correct". The runtime silently drops an unresolved tool, so the +skill shipped with its only declared capability resolving to nothing while its +instructions promised an account + cases + opportunities + knowledge roll-up it +had no tool to read. Defining the missing tool would not have fixed it: +`ToolSchema` is a read-only Studio projection with no `implementation` and no +executor, so a hand-authored `search_knowledge` would validate, build, and still +never run. The knowledge base is `crm_knowledge_article`, a normal object, so the +skill now reads it with `query_records` alongside the rest of the profile. + +Adds `test/skills-integrity.test.ts`: every skill tool must resolve to a platform +built-in or an `action_` tool materialised from an Action that is +`ai.exposed` with a headless path; AI tool metadata cannot be used to satisfy a +dangling reference; and every skill handed off to in instructions must exist (the +defect that made `case_triage` point at a `response_drafting` skill that never +existed). Also corrects two stale skill docstrings that explained the read-only +posture of `case_triage` / `email_drafting` by calling `escalate_case`, +`close_case` and `send_email` `type: 'modal'` — all three were retyped to +flow/script since, and the real reason no tool is materialised is that none of +them opts in via `ai.exposed`. diff --git a/src/skills/case-triage.skill.ts b/src/skills/case-triage.skill.ts index e656ceaf..7094a6c2 100644 --- a/src/skills/case-triage.skill.ts +++ b/src/skills/case-triage.skill.ts @@ -9,13 +9,16 @@ import { defineSkill } from '@objectstack/spec'; * reasoning, not a tool — the agent reads the case with the platform's * data tools and applies the rubric below. * - * It deliberately calls NOTHING that mutates. Escalate and Close are - * `type: 'modal'` Actions: they collect a reason / resolution from a - * person, so they have no headless path and the runtime never - * materialises tools for them (ADR-0011). That is the right shape here — - * the agent supplies the judgement, the human supplies the words and the - * decision — so the skill recommends the button instead of pretending to - * press it. + * It deliberately calls NOTHING that mutates. Escalate and Close carry no + * `ai` block, and ADR-0011 exposure is opt-in / default off, so the + * runtime materialises no `action_escalate_case` / `action_close_case` + * tool for the skill to call. That is the right shape here — both Actions + * exist to collect a reason / resolution from a person, so the agent + * supplies the judgement and the human supplies the words and the + * decision — and the skill recommends the button instead of pretending to + * press it. (Both became `type: 'flow'` in #515, so opting them in later + * is a governance call, not a mechanical one; the review step is the + * point.) */ export const CaseTriageSkill = defineSkill({ name: 'case_triage', diff --git a/src/skills/customer-360.skill.ts b/src/skills/customer-360.skill.ts index 4a442226..c2d49465 100644 --- a/src/skills/customer-360.skill.ts +++ b/src/skills/customer-360.skill.ts @@ -2,6 +2,24 @@ import { defineSkill } from '@objectstack/spec'; +/** + * Customer 360 — one profile assembled from the records that already exist. + * + * ADR-0109: this skill declares no bespoke tools. It used to list a + * `search_knowledge` tool that nothing defines — the last survivor of the + * ten fictional tools #512 removed — and the runtime silently drops an + * unresolved tool, so the skill shipped with a single declared capability + * that never resolved, while its instructions promised an aggregation it + * had no tool to read. + * + * Authoring the missing tool would not have helped: `ToolSchema` is a + * READ-ONLY PROJECTION for Studio discovery — it carries no + * `implementation` and no framework executor loads it, so a hand-authored + * `search_knowledge` would validate, build, and still never run. The + * knowledge base here is not a search service anyway; it is + * `crm_knowledge_article`, a normal CRM object, so `query_records` reaches + * it exactly like every other object this skill reads. + */ export const Customer360Skill = defineSkill({ name: 'customer_360', label: 'Customer 360', @@ -9,14 +27,31 @@ export const Customer360Skill = defineSkill({ instructions: `When the user asks for "the full picture" of a customer / account / contact: -1. Search the knowledge base via search_knowledge for any policy or - playbook context relevant to this account. -2. Summarise into three sections: Account Snapshot · Active Work · - Risks & Notes. -3. Cite record IDs inline (e.g. "case CASE-1234") so the UI can deep - link.`, - tools: ['search_knowledge'], +1. Read the shape first — \`describe_object\` for \`crm_account\`, and for + each related object before you query it. Admins add fields; never + summarise from memory of a previous turn. +2. \`get_record\` the account, then pull the related work with + \`query_records\`, each filtered to this account: + - \`crm_contact\` — who the people are, primary contact first. + - \`crm_case\` — cases where \`is_closed\` is false, newest first. + - \`crm_opportunity\` — open deals with stage, amount and close date. + Quote totals from \`aggregate_data\` (open pipeline amount, case count + by status) rather than adding up rows yourself. +3. For the policy or playbook context, \`query_records\` on + \`crm_knowledge_article\` — \`status\` published, matched on the + \`category\` or \`tags\` of the cases you just read. There is no + knowledge-search tool and you do not need one: the knowledge base is + an object like any other. +4. Summarise into three sections: **Account Snapshot** · **Active Work** · + **Risks & Notes**. Every risk names the record and the signal that + raised it — an escalated case, a close date already past, a deal with + no activity in 30+ days. Do not infer risk you cannot cite. +5. Cite record IDs inline (e.g. "case CASE-01234", "article KA-0007") so + the UI can deep link. If a section has no records, say so plainly + instead of filling it.`, + + tools: ['describe_object', 'get_record', 'query_records', 'aggregate_data'], triggerPhrases: [ 'customer 360', diff --git a/src/skills/email-drafting.skill.ts b/src/skills/email-drafting.skill.ts index 4eda6b18..ae40d378 100644 --- a/src/skills/email-drafting.skill.ts +++ b/src/skills/email-drafting.skill.ts @@ -7,10 +7,11 @@ import { defineSkill } from '@objectstack/spec'; * * ADR-0109: writing a subject line and personalising a body is what the * model does; it is not a tool call. Sending is HotCRM's `send_email` - * Action — `type: 'modal'`, so it collects the final copy from a person - * and has no headless path (ADR-0011). The agent therefore drafts and - * hands over; a human presses Send. For outbound email that review step - * is a feature, not a limitation. + * Action, which carries no `ai` block — ADR-0011 exposure is opt-in and + * default off, so no `action_send_email` tool is materialised and the + * skill has nothing to call. The agent therefore drafts and hands over; a + * human reviews the copy and presses Send. For outbound email that review + * step is a feature, not a limitation. */ export const EmailDraftingSkill = defineSkill({ name: 'email_drafting', diff --git a/test/skills-integrity.test.ts b/test/skills-integrity.test.ts new file mode 100644 index 00000000..306ecd82 --- /dev/null +++ b/test/skills-integrity.test.ts @@ -0,0 +1,209 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import stack from '../objectstack.config'; + +/** + * Skill reference guards — every capability a skill CLAIMS must resolve. + * + * The defect class: a skill's `tools: [...]` is a list of NAMES. `os + * validate` and `build` check that each name is a well-formed snake_case + * string and stop there — nothing resolves it against a tool that exists. + * At runtime an unresolved name is dropped silently, so the model is handed + * instructions ("call `search_knowledge` for the policy context") describing + * a capability it does not have, and improvises. Every instance of this bug + * shipped green: issue #493 counted eleven fictional tools across six + * skills, #512 removed ten of them, and `search_knowledge` survived in + * `customer_360` because nothing in CI could see it. + * + * Note the trap in "just define the missing tool": per `ToolSchema`, AI tool + * metadata is a READ-ONLY PROJECTION for Studio discovery — it has no + * `implementation` field and no framework executor loads it. A hand-authored + * `defineTool` record would satisfy a naive existence check while remaining + * exactly as unrunnable. So the resolvable universe below is deliberately + * narrow: platform built-ins, and the `action_` tools the runtime + * materialises from Actions that opted in. + * + * Kept in its own file rather than appended to metadata-references.test.ts, + * which guards UI metadata and is a busy merge surface. + */ + +type AnyRec = Record; +const skills: AnyRec[] = (stack as any).skills ?? []; +const actions: AnyRec[] = (stack as any).actions ?? []; +const flows: AnyRec[] = (stack as any).flows ?? []; +const toolMetadata: AnyRec[] = (stack as any).tools ?? []; + +const skillNames = new Set(skills.map((s) => s.name)); +const flowNames = new Set(flows.map((f) => f.name)); +const action = (name: string) => actions.find((a) => a.name === name); + +/** + * Tools the AI runtime provides — the only names a skill may reference + * without anything in this repo defining them. + * + * The runtime that serves them ships in the closed cloud package + * (`@objectstack/service-ai`, ADR-0025 §2), so the open-edition + * `node_modules` this repo installs cannot be read for the authoritative + * list. What IS verifiable locally is the MCP bridge in + * `@objectstack/mcp@16.1.0`, which registers the same registry's data + + * metadata tools: `list_objects`, `describe_object`, `query_records`, + * `get_record`, `aggregate_data`, `create_record`, `update_record`, + * `delete_record`, `list_actions`, `run_action`, `validate_expression`. + * + * This list is the READ subset of that surface plus `visualize_data`. + * Deliberately excluded: the write tools (`create_record` / + * `update_record` / `delete_record`). HotCRM's state changes go through + * Actions so that validation, sharing rules and audit match the UI path — + * a skill reaching for raw record writes is a design regression, and this + * list is where that gets caught. + * + * Adding a name here is a claim that the platform serves it. Make that + * claim deliberately: an entry that is merely aspirational reintroduces + * exactly the bug this file exists to prevent. + */ +const PLATFORM_TOOLS = new Set([ + 'list_objects', + 'describe_object', + 'query_records', + 'get_record', + 'aggregate_data', + // Charting tool served by the cloud AI runtime's analytics tier. Not in + // the open-edition bundle (nothing AI-runtime is), verified against the + // upstream reference-integrity rule when `revenue_forecasting` adopted it + // in #512. + 'visualize_data', +]); + +/** `action_` — the tool the runtime materialises from an Action (ADR-0011). */ +const ACTION_TOOL_PREFIX = 'action_'; + +describe('skill tool references resolve', () => { + it('every skill declares at least one tool', () => { + expect(skills.length).toBeGreaterThan(0); + for (const skill of skills) { + expect(Array.isArray(skill.tools), `${skill.name}: tools must be an array`).toBe(true); + expect(skill.tools.length, `${skill.name}: declares no tools`).toBeGreaterThan(0); + } + }); + + it('every tool name resolves to a platform built-in or a materialised Action tool', () => { + const dangling: string[] = []; + + for (const skill of skills) { + for (const tool of skill.tools as string[]) { + // Trailing-wildcard subscriptions (`action_*`) match a family of + // dynamically registered tools; resolve the prefix, not the name. + if (tool.endsWith('*')) { + const prefix = tool.slice(0, -1); + const matches = + [...PLATFORM_TOOLS].some((t) => t.startsWith(prefix)) || + (prefix === ACTION_TOOL_PREFIX && actions.length > 0) || + actions.some((a) => `${ACTION_TOOL_PREFIX}${a.name}`.startsWith(prefix)); + if (!matches) dangling.push(`${skill.name} → ${tool} (wildcard matches nothing)`); + continue; + } + + if (PLATFORM_TOOLS.has(tool)) continue; + + if (tool.startsWith(ACTION_TOOL_PREFIX)) { + const actionName = tool.slice(ACTION_TOOL_PREFIX.length); + if (!action(actionName)) { + dangling.push(`${skill.name} → ${tool} (no Action named "${actionName}")`); + } + continue; + } + + dangling.push(`${skill.name} → ${tool}`); + } + } + + expect( + dangling, + `Skills reference tools that nothing serves:\n ${dangling.join('\n ')}\n` + + 'Either drop the reference, or route the step through an Action with ' + + '`ai: { exposed: true, description }`. Authoring `defineTool` metadata ' + + 'does NOT make a tool runnable (ToolSchema is a read-only projection).', + ).toEqual([]); + }); + + it('every referenced Action is AI-exposed and has a headless path', () => { + const referenced = new Set( + skills + .flatMap((s) => s.tools as string[]) + .filter((t) => t.startsWith(ACTION_TOOL_PREFIX) && !t.endsWith('*')) + .map((t) => t.slice(ACTION_TOOL_PREFIX.length)), + ); + + expect(referenced.size, 'expected at least one Action-backed skill step').toBeGreaterThan(0); + + for (const name of referenced) { + const a = action(name)!; + + // ADR-0011: opt-in, default off. Without `exposed` the runtime never + // materialises the tool, so the reference is dead however real the + // Action is — the exact regression #512's follow-up commit fixed. + expect(a.ai?.exposed, `${name}: referenced by a skill but not \`ai.exposed\``).toBe(true); + expect( + a.ai?.description?.length ?? 0, + `${name}: \`ai.exposed\` requires an LLM-facing description (≥40 chars)`, + ).toBeGreaterThanOrEqual(40); + + // A modal Action collects its input from a person and has no headless + // path, so no tool is generated even when it opts in. Only flow-typed + // Actions with a resolvable target, or script-typed Actions with a + // body, can actually run for an agent. + if (a.type === 'flow') { + expect(a.target, `${name}: flow-typed Action with no target`).toBeTruthy(); + expect(flowNames, `${name}: target flow "${a.target}" is not defined`).toContain(a.target); + } else if (a.type === 'script') { + expect(a.body?.source, `${name}: script-typed Action with no body`).toBeTruthy(); + } else { + expect.fail( + `${name}: type "${a.type}" has no headless path, so \`${ACTION_TOOL_PREFIX}${name}\` ` + + 'is never materialised. Point the skill at the button instead of claiming to press it.', + ); + } + } + }); + + it('does not lean on AI tool metadata, which never executes', () => { + // ToolSchema: "[READ-ONLY PROJECTION — not an execution entry point] + // Authoring a tool as metadata does NOT make it runnable." Declaring + // `tools` on the stack to satisfy a dangling reference would restore + // green CI and none of the behaviour. + expect( + toolMetadata.map((t) => t.name), + 'stack.tools is a Studio-discovery projection with no executor — a skill ' + + 'cannot call these. Route the capability through an Action instead.', + ).toEqual([]); + }); +}); + +describe('skill cross-references resolve', () => { + it('every skill handed off to in instructions exists', () => { + // `case_triage` used to hand off to `response_drafting`, a skill that was + // never defined (issue #493); the real one is `email_drafting`. + const dangling: string[] = []; + + for (const skill of skills) { + const instructions: string = skill.instructions ?? ''; + for (const [, referenced] of instructions.matchAll(/`([a-z][a-z0-9_]*)`\s+skill/g)) { + if (!skillNames.has(referenced)) { + dangling.push(`${skill.name} → \`${referenced}\` skill`); + } + } + } + + expect(dangling, `Instructions hand off to skills that do not exist:\n ${dangling.join('\n ')}`).toEqual([]); + }); + + it('every defined skill is registered on the stack', () => { + // A skill left out of `src/skills/index.ts` is inert: it validates, it + // typechecks, and the assistant never sees it. + expect(skillNames.size, 'duplicate skill names on the stack').toBe(skills.length); + for (const name of ['live_data', 'lead_qualification', 'email_drafting', 'revenue_forecasting', 'case_triage', 'customer_360']) { + expect(skillNames, `skill "${name}" is not registered`).toContain(name); + } + }); +});