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); + } + }); +});