fix(skills): drop the last undefined tool reference, guard the class in CI (#493)

.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_<name>` 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`.

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',

src/skills/customer-360.skill.ts

@@ -2,21 +2,56 @@
 
 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',
   description: 'Aggregates account, recent cases, open opportunities, and knowledge hits into a single customer profile.',
 
   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',

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',

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_<name>` 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<string, any>;
+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_<name>` — 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);
+    }
+  });
+});