Skip to content

Commit 786e6fe

Browse files
committed
fix(skills): drop the last undefined tool reference, guard the class in CI (#493)
`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_<name>` 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Y9STuduWbrAwcgviziaV4e
1 parent 1db7b95 commit 786e6fe

5 files changed

Lines changed: 292 additions & 18 deletions

File tree

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,26 @@
1+
---
2+
'hotcrm': patch
3+
---
4+
5+
Drop the last undefined tool reference from the AI skills and guard the class in
6+
CI. `customer_360` declared `tools: ['search_knowledge']` — the survivor of the
7+
eleven fictional tools issue #493 counted, which #512 missed because that skill
8+
looked "already correct". The runtime silently drops an unresolved tool, so the
9+
skill shipped with its only declared capability resolving to nothing while its
10+
instructions promised an account + cases + opportunities + knowledge roll-up it
11+
had no tool to read. Defining the missing tool would not have fixed it:
12+
`ToolSchema` is a read-only Studio projection with no `implementation` and no
13+
executor, so a hand-authored `search_knowledge` would validate, build, and still
14+
never run. The knowledge base is `crm_knowledge_article`, a normal object, so the
15+
skill now reads it with `query_records` alongside the rest of the profile.
16+
17+
Adds `test/skills-integrity.test.ts`: every skill tool must resolve to a platform
18+
built-in or an `action_<name>` tool materialised from an Action that is
19+
`ai.exposed` with a headless path; AI tool metadata cannot be used to satisfy a
20+
dangling reference; and every skill handed off to in instructions must exist (the
21+
defect that made `case_triage` point at a `response_drafting` skill that never
22+
existed). Also corrects two stale skill docstrings that explained the read-only
23+
posture of `case_triage` / `email_drafting` by calling `escalate_case`,
24+
`close_case` and `send_email` `type: 'modal'` — all three were retyped to
25+
flow/script since, and the real reason no tool is materialised is that none of
26+
them opts in via `ai.exposed`.

src/skills/case-triage.skill.ts

Lines changed: 10 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -9,13 +9,16 @@ import { defineSkill } from '@objectstack/spec';
99
* reasoning, not a tool — the agent reads the case with the platform's
1010
* data tools and applies the rubric below.
1111
*
12-
* It deliberately calls NOTHING that mutates. Escalate and Close are
13-
* `type: 'modal'` Actions: they collect a reason / resolution from a
14-
* person, so they have no headless path and the runtime never
15-
* materialises tools for them (ADR-0011). That is the right shape here —
16-
* the agent supplies the judgement, the human supplies the words and the
17-
* decision — so the skill recommends the button instead of pretending to
18-
* press it.
12+
* It deliberately calls NOTHING that mutates. Escalate and Close carry no
13+
* `ai` block, and ADR-0011 exposure is opt-in / default off, so the
14+
* runtime materialises no `action_escalate_case` / `action_close_case`
15+
* tool for the skill to call. That is the right shape here — both Actions
16+
* exist to collect a reason / resolution from a person, so the agent
17+
* supplies the judgement and the human supplies the words and the
18+
* decision — and the skill recommends the button instead of pretending to
19+
* press it. (Both became `type: 'flow'` in #515, so opting them in later
20+
* is a governance call, not a mechanical one; the review step is the
21+
* point.)
1922
*/
2023
export const CaseTriageSkill = defineSkill({
2124
name: 'case_triage',

src/skills/customer-360.skill.ts

Lines changed: 42 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -2,21 +2,56 @@
22

33
import { defineSkill } from '@objectstack/spec';
44

5+
/**
6+
* Customer 360 — one profile assembled from the records that already exist.
7+
*
8+
* ADR-0109: this skill declares no bespoke tools. It used to list a
9+
* `search_knowledge` tool that nothing defines — the last survivor of the
10+
* ten fictional tools #512 removed — and the runtime silently drops an
11+
* unresolved tool, so the skill shipped with a single declared capability
12+
* that never resolved, while its instructions promised an aggregation it
13+
* had no tool to read.
14+
*
15+
* Authoring the missing tool would not have helped: `ToolSchema` is a
16+
* READ-ONLY PROJECTION for Studio discovery — it carries no
17+
* `implementation` and no framework executor loads it, so a hand-authored
18+
* `search_knowledge` would validate, build, and still never run. The
19+
* knowledge base here is not a search service anyway; it is
20+
* `crm_knowledge_article`, a normal CRM object, so `query_records` reaches
21+
* it exactly like every other object this skill reads.
22+
*/
523
export const Customer360Skill = defineSkill({
624
name: 'customer_360',
725
label: 'Customer 360',
826
description: 'Aggregates account, recent cases, open opportunities, and knowledge hits into a single customer profile.',
927

1028
instructions: `When the user asks for "the full picture" of a
1129
customer / account / contact:
12-
1. Search the knowledge base via search_knowledge for any policy or
13-
playbook context relevant to this account.
14-
2. Summarise into three sections: Account Snapshot · Active Work ·
15-
Risks & Notes.
16-
3. Cite record IDs inline (e.g. "case CASE-1234") so the UI can deep
17-
link.`,
1830
19-
tools: ['search_knowledge'],
31+
1. Read the shape first — \`describe_object\` for \`crm_account\`, and for
32+
each related object before you query it. Admins add fields; never
33+
summarise from memory of a previous turn.
34+
2. \`get_record\` the account, then pull the related work with
35+
\`query_records\`, each filtered to this account:
36+
- \`crm_contact\` — who the people are, primary contact first.
37+
- \`crm_case\` — cases where \`is_closed\` is false, newest first.
38+
- \`crm_opportunity\` — open deals with stage, amount and close date.
39+
Quote totals from \`aggregate_data\` (open pipeline amount, case count
40+
by status) rather than adding up rows yourself.
41+
3. For the policy or playbook context, \`query_records\` on
42+
\`crm_knowledge_article\` — \`status\` published, matched on the
43+
\`category\` or \`tags\` of the cases you just read. There is no
44+
knowledge-search tool and you do not need one: the knowledge base is
45+
an object like any other.
46+
4. Summarise into three sections: **Account Snapshot** · **Active Work** ·
47+
**Risks & Notes**. Every risk names the record and the signal that
48+
raised it — an escalated case, a close date already past, a deal with
49+
no activity in 30+ days. Do not infer risk you cannot cite.
50+
5. Cite record IDs inline (e.g. "case CASE-01234", "article KA-0007") so
51+
the UI can deep link. If a section has no records, say so plainly
52+
instead of filling it.`,
53+
54+
tools: ['describe_object', 'get_record', 'query_records', 'aggregate_data'],
2055

2156
triggerPhrases: [
2257
'customer 360',

src/skills/email-drafting.skill.ts

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,11 @@ import { defineSkill } from '@objectstack/spec';
77
*
88
* ADR-0109: writing a subject line and personalising a body is what the
99
* model does; it is not a tool call. Sending is HotCRM's `send_email`
10-
* Action — `type: 'modal'`, so it collects the final copy from a person
11-
* and has no headless path (ADR-0011). The agent therefore drafts and
12-
* hands over; a human presses Send. For outbound email that review step
13-
* is a feature, not a limitation.
10+
* Action, which carries no `ai` block — ADR-0011 exposure is opt-in and
11+
* default off, so no `action_send_email` tool is materialised and the
12+
* skill has nothing to call. The agent therefore drafts and hands over; a
13+
* human reviews the copy and presses Send. For outbound email that review
14+
* step is a feature, not a limitation.
1415
*/
1516
export const EmailDraftingSkill = defineSkill({
1617
name: 'email_drafting',

test/skills-integrity.test.ts

Lines changed: 209 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,209 @@
1+
// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
import { describe, it, expect } from 'vitest';
4+
import stack from '../objectstack.config';
5+
6+
/**
7+
* Skill reference guards — every capability a skill CLAIMS must resolve.
8+
*
9+
* The defect class: a skill's `tools: [...]` is a list of NAMES. `os
10+
* validate` and `build` check that each name is a well-formed snake_case
11+
* string and stop there — nothing resolves it against a tool that exists.
12+
* At runtime an unresolved name is dropped silently, so the model is handed
13+
* instructions ("call `search_knowledge` for the policy context") describing
14+
* a capability it does not have, and improvises. Every instance of this bug
15+
* shipped green: issue #493 counted eleven fictional tools across six
16+
* skills, #512 removed ten of them, and `search_knowledge` survived in
17+
* `customer_360` because nothing in CI could see it.
18+
*
19+
* Note the trap in "just define the missing tool": per `ToolSchema`, AI tool
20+
* metadata is a READ-ONLY PROJECTION for Studio discovery — it has no
21+
* `implementation` field and no framework executor loads it. A hand-authored
22+
* `defineTool` record would satisfy a naive existence check while remaining
23+
* exactly as unrunnable. So the resolvable universe below is deliberately
24+
* narrow: platform built-ins, and the `action_<name>` tools the runtime
25+
* materialises from Actions that opted in.
26+
*
27+
* Kept in its own file rather than appended to metadata-references.test.ts,
28+
* which guards UI metadata and is a busy merge surface.
29+
*/
30+
31+
type AnyRec = Record<string, any>;
32+
const skills: AnyRec[] = (stack as any).skills ?? [];
33+
const actions: AnyRec[] = (stack as any).actions ?? [];
34+
const flows: AnyRec[] = (stack as any).flows ?? [];
35+
const toolMetadata: AnyRec[] = (stack as any).tools ?? [];
36+
37+
const skillNames = new Set(skills.map((s) => s.name));
38+
const flowNames = new Set(flows.map((f) => f.name));
39+
const action = (name: string) => actions.find((a) => a.name === name);
40+
41+
/**
42+
* Tools the AI runtime provides — the only names a skill may reference
43+
* without anything in this repo defining them.
44+
*
45+
* The runtime that serves them ships in the closed cloud package
46+
* (`@objectstack/service-ai`, ADR-0025 §2), so the open-edition
47+
* `node_modules` this repo installs cannot be read for the authoritative
48+
* list. What IS verifiable locally is the MCP bridge in
49+
* `@objectstack/mcp@16.1.0`, which registers the same registry's data +
50+
* metadata tools: `list_objects`, `describe_object`, `query_records`,
51+
* `get_record`, `aggregate_data`, `create_record`, `update_record`,
52+
* `delete_record`, `list_actions`, `run_action`, `validate_expression`.
53+
*
54+
* This list is the READ subset of that surface plus `visualize_data`.
55+
* Deliberately excluded: the write tools (`create_record` /
56+
* `update_record` / `delete_record`). HotCRM's state changes go through
57+
* Actions so that validation, sharing rules and audit match the UI path —
58+
* a skill reaching for raw record writes is a design regression, and this
59+
* list is where that gets caught.
60+
*
61+
* Adding a name here is a claim that the platform serves it. Make that
62+
* claim deliberately: an entry that is merely aspirational reintroduces
63+
* exactly the bug this file exists to prevent.
64+
*/
65+
const PLATFORM_TOOLS = new Set([
66+
'list_objects',
67+
'describe_object',
68+
'query_records',
69+
'get_record',
70+
'aggregate_data',
71+
// Charting tool served by the cloud AI runtime's analytics tier. Not in
72+
// the open-edition bundle (nothing AI-runtime is), verified against the
73+
// upstream reference-integrity rule when `revenue_forecasting` adopted it
74+
// in #512.
75+
'visualize_data',
76+
]);
77+
78+
/** `action_<name>` — the tool the runtime materialises from an Action (ADR-0011). */
79+
const ACTION_TOOL_PREFIX = 'action_';
80+
81+
describe('skill tool references resolve', () => {
82+
it('every skill declares at least one tool', () => {
83+
expect(skills.length).toBeGreaterThan(0);
84+
for (const skill of skills) {
85+
expect(Array.isArray(skill.tools), `${skill.name}: tools must be an array`).toBe(true);
86+
expect(skill.tools.length, `${skill.name}: declares no tools`).toBeGreaterThan(0);
87+
}
88+
});
89+
90+
it('every tool name resolves to a platform built-in or a materialised Action tool', () => {
91+
const dangling: string[] = [];
92+
93+
for (const skill of skills) {
94+
for (const tool of skill.tools as string[]) {
95+
// Trailing-wildcard subscriptions (`action_*`) match a family of
96+
// dynamically registered tools; resolve the prefix, not the name.
97+
if (tool.endsWith('*')) {
98+
const prefix = tool.slice(0, -1);
99+
const matches =
100+
[...PLATFORM_TOOLS].some((t) => t.startsWith(prefix)) ||
101+
(prefix === ACTION_TOOL_PREFIX && actions.length > 0) ||
102+
actions.some((a) => `${ACTION_TOOL_PREFIX}${a.name}`.startsWith(prefix));
103+
if (!matches) dangling.push(`${skill.name}${tool} (wildcard matches nothing)`);
104+
continue;
105+
}
106+
107+
if (PLATFORM_TOOLS.has(tool)) continue;
108+
109+
if (tool.startsWith(ACTION_TOOL_PREFIX)) {
110+
const actionName = tool.slice(ACTION_TOOL_PREFIX.length);
111+
if (!action(actionName)) {
112+
dangling.push(`${skill.name}${tool} (no Action named "${actionName}")`);
113+
}
114+
continue;
115+
}
116+
117+
dangling.push(`${skill.name}${tool}`);
118+
}
119+
}
120+
121+
expect(
122+
dangling,
123+
`Skills reference tools that nothing serves:\n ${dangling.join('\n ')}\n` +
124+
'Either drop the reference, or route the step through an Action with ' +
125+
'`ai: { exposed: true, description }`. Authoring `defineTool` metadata ' +
126+
'does NOT make a tool runnable (ToolSchema is a read-only projection).',
127+
).toEqual([]);
128+
});
129+
130+
it('every referenced Action is AI-exposed and has a headless path', () => {
131+
const referenced = new Set(
132+
skills
133+
.flatMap((s) => s.tools as string[])
134+
.filter((t) => t.startsWith(ACTION_TOOL_PREFIX) && !t.endsWith('*'))
135+
.map((t) => t.slice(ACTION_TOOL_PREFIX.length)),
136+
);
137+
138+
expect(referenced.size, 'expected at least one Action-backed skill step').toBeGreaterThan(0);
139+
140+
for (const name of referenced) {
141+
const a = action(name)!;
142+
143+
// ADR-0011: opt-in, default off. Without `exposed` the runtime never
144+
// materialises the tool, so the reference is dead however real the
145+
// Action is — the exact regression #512's follow-up commit fixed.
146+
expect(a.ai?.exposed, `${name}: referenced by a skill but not \`ai.exposed\``).toBe(true);
147+
expect(
148+
a.ai?.description?.length ?? 0,
149+
`${name}: \`ai.exposed\` requires an LLM-facing description (≥40 chars)`,
150+
).toBeGreaterThanOrEqual(40);
151+
152+
// A modal Action collects its input from a person and has no headless
153+
// path, so no tool is generated even when it opts in. Only flow-typed
154+
// Actions with a resolvable target, or script-typed Actions with a
155+
// body, can actually run for an agent.
156+
if (a.type === 'flow') {
157+
expect(a.target, `${name}: flow-typed Action with no target`).toBeTruthy();
158+
expect(flowNames, `${name}: target flow "${a.target}" is not defined`).toContain(a.target);
159+
} else if (a.type === 'script') {
160+
expect(a.body?.source, `${name}: script-typed Action with no body`).toBeTruthy();
161+
} else {
162+
expect.fail(
163+
`${name}: type "${a.type}" has no headless path, so \`${ACTION_TOOL_PREFIX}${name}\` ` +
164+
'is never materialised. Point the skill at the button instead of claiming to press it.',
165+
);
166+
}
167+
}
168+
});
169+
170+
it('does not lean on AI tool metadata, which never executes', () => {
171+
// ToolSchema: "[READ-ONLY PROJECTION — not an execution entry point]
172+
// Authoring a tool as metadata does NOT make it runnable." Declaring
173+
// `tools` on the stack to satisfy a dangling reference would restore
174+
// green CI and none of the behaviour.
175+
expect(
176+
toolMetadata.map((t) => t.name),
177+
'stack.tools is a Studio-discovery projection with no executor — a skill ' +
178+
'cannot call these. Route the capability through an Action instead.',
179+
).toEqual([]);
180+
});
181+
});
182+
183+
describe('skill cross-references resolve', () => {
184+
it('every skill handed off to in instructions exists', () => {
185+
// `case_triage` used to hand off to `response_drafting`, a skill that was
186+
// never defined (issue #493); the real one is `email_drafting`.
187+
const dangling: string[] = [];
188+
189+
for (const skill of skills) {
190+
const instructions: string = skill.instructions ?? '';
191+
for (const [, referenced] of instructions.matchAll(/`([a-z][a-z0-9_]*)`\s+skill/g)) {
192+
if (!skillNames.has(referenced)) {
193+
dangling.push(`${skill.name} → \`${referenced}\` skill`);
194+
}
195+
}
196+
}
197+
198+
expect(dangling, `Instructions hand off to skills that do not exist:\n ${dangling.join('\n ')}`).toEqual([]);
199+
});
200+
201+
it('every defined skill is registered on the stack', () => {
202+
// A skill left out of `src/skills/index.ts` is inert: it validates, it
203+
// typechecks, and the assistant never sees it.
204+
expect(skillNames.size, 'duplicate skill names on the stack').toBe(skills.length);
205+
for (const name of ['live_data', 'lead_qualification', 'email_drafting', 'revenue_forecasting', 'case_triage', 'customer_360']) {
206+
expect(skillNames, `skill "${name}" is not registered`).toContain(name);
207+
}
208+
});
209+
});

0 commit comments

Comments
 (0)