Skip to content

Commit 8a097bf

Browse files
authored
Merge pull request #557 from objectstack-ai/claude/copilots-undefined-tools-1n2wn1
fix(skills): drop the last undefined tool reference, guard the class in CI (#493)
2 parents 1db7b95 + 786e6fe commit 8a097bf

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)