From dbdf51f4b7f8a469b6cf1f21e532458afc5ab07d Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 31 Jul 2026 03:31:39 +0000 Subject: [PATCH] =?UTF-8?q?fix(test):=20correct=20the=20platform-tool=20al?= =?UTF-8?q?lowlist=20=E2=80=94=20search=5Fknowledge=20is=20real?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The allowlist added in #557 guessed the platform tool set from what the `@objectstack/mcp@16.1.0` bridge happens to register, and got it wrong in both directions: - It omitted `search_knowledge`, so the guard would have FAILED a legitimate reference. The tool is real — it is in `PLATFORM_PROVIDED_TOOL_NAMES`, and 16.1.0 documented it all along at `spec/src/ai/knowledge-source.zod.ts:114` ("Whether `search_knowledge` may expose this source to AI agents"). One grep for that name would have caught it; the same absent-from-node_modules inference was applied to `visualize_data` and came out the other way. - It reasoned about deliberately excluding `create_record` / `update_record` / `delete_record`. Those are MCP-bridge tools and are not in the platform registry at all, so there was nothing to exclude. The list is now transcribed verbatim from `PLATFORM_PROVIDED_TOOL_NAMES` in `@objectstack/spec@17.0.0-rc.0` — all 30 entries, diffed equal to the upstream set — carrying instructions to delete the literal and import it on the 17.0 upgrade. A hand-copied registry is the drift risk this file exists to catch, one level up; it is a stopgap for 16.1.0, which exposes no such export. Cross-checked against upstream's own `ai-skill-tool-unresolved` rule, which ships in `@objectstack/lint@17.0.0-rc.0` (objectstack#3820). Both give identical verdicts on seven probes: `search_knowledge`, `query_data`, `todo_write` and `action_convert_lead` accepted; `action_escalate_case` (not `ai.exposed`), `search_knowledgebase` and `triage_case` rejected. Also corrects the "nothing defines it" claim in three places — the `customer_360` docstring, its changeset, and `code_examples.md`. The skill still does not reference the tool, for a narrower and accurate reason: retrieval needs a declared knowledge source, `AIKnowledgeSchema` mounts only on `AgentSchema.knowledge`, and #512 deleted the agents — so a skills-only app has nowhere to declare one and the tool would resolve and return nothing. The skill's instruction line now says the model has no such tool *in this skill*, rather than that none exists. 168 tests pass, typecheck and build clean, validate holds at its 2 pre-existing warnings. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Y9STuduWbrAwcgviziaV4e --- ...ills-platform-tool-allowlist-correction.md | 28 ++++++ .../skills-undefined-tool-references.md | 27 +++--- docs/developers/code_examples.md | 10 ++- src/skills/customer-360.skill.ts | 39 +++++---- test/skills-integrity.test.ts | 86 ++++++++++++------- 5 files changed, 130 insertions(+), 60 deletions(-) create mode 100644 .changeset/skills-platform-tool-allowlist-correction.md diff --git a/.changeset/skills-platform-tool-allowlist-correction.md b/.changeset/skills-platform-tool-allowlist-correction.md new file mode 100644 index 00000000..48afb809 --- /dev/null +++ b/.changeset/skills-platform-tool-allowlist-correction.md @@ -0,0 +1,28 @@ +--- +'hotcrm': patch +--- + +Correct the platform-tool allowlist in `test/skills-integrity.test.ts`. The first +pass guessed the set from what the `@objectstack/mcp@16.1.0` bridge happens to +register, and got it wrong in both directions: it omitted `search_knowledge` — a +real platform tool, documented in 16.1.0's own +`spec/src/ai/knowledge-source.zod.ts` — so the guard would have failed a +legitimate reference; and it reasoned about excluding `create_record` / +`update_record` / `delete_record`, which are MCP-bridge tools absent from the +platform registry entirely, so there was nothing to exclude. + +The list is now transcribed verbatim from `PLATFORM_PROVIDED_TOOL_NAMES` in +`@objectstack/spec@17.0.0-rc.0` — all 30 entries, verified equal to the upstream +set — with instructions to delete the literal and import it on the 17.0 upgrade. +Cross-checked against upstream's `ai-skill-tool-unresolved` rule (which ships in +17.0.0-rc.0, closing the gap this repo guarded locally): both give identical +verdicts on `search_knowledge`, `query_data`, `todo_write`, +`action_convert_lead`, `action_escalate_case`, `search_knowledgebase` and +`triage_case`. + +Also corrects the claim that `search_knowledge` is undefined, in the +`customer_360` docstring, its changeset, and `code_examples.md`. The tool exists; +it stays out of the skill for a narrower reason — retrieval needs a declared +knowledge source, `AIKnowledgeSchema` mounts only on `AgentSchema.knowledge`, and +#512 deleted the agents, so a skills-only app has nowhere to declare one and the +tool would resolve but return nothing. diff --git a/.changeset/skills-undefined-tool-references.md b/.changeset/skills-undefined-tool-references.md index d25999b5..e2ebb6d3 100644 --- a/.changeset/skills-undefined-tool-references.md +++ b/.changeset/skills-undefined-tool-references.md @@ -2,17 +2,22 @@ '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. +Give `customer_360` the tools its instructions always assumed, and guard skill +tool references in CI. Its whole tool list was `tools: ['search_knowledge']` +while the instructions promised an account + cases + opportunities + knowledge +roll-up — the skill could not fetch a single record. It now reads accounts, +contacts, cases and opportunities with the platform data tools, and the +knowledge base with `query_records`, since `crm_knowledge_article` is a normal +object. + +`search_knowledge` itself is a real platform tool — it is in +`PLATFORM_PROVIDED_TOOL_NAMES` and was documented in 16.1.0's +`spec/src/ai/knowledge-source.zod.ts`. Issue #493 listed it as undefined and an +earlier draft of this change repeated that; both were wrong. It is left out for +a narrower reason: retrieval needs a declared knowledge source, `AIKnowledgeSchema` +mounts only on `AgentSchema.knowledge`, and #512 deleted the agents — so a +skills-only app has nowhere to declare one and the tool would resolve but return +nothing. 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 diff --git a/docs/developers/code_examples.md b/docs/developers/code_examples.md index 24531d1f..ba7c261e 100644 --- a/docs/developers/code_examples.md +++ b/docs/developers/code_examples.md @@ -217,8 +217,14 @@ model with instructions describing a capability it does not have — the defect [#493](https://github.com/objectstack-ai/hotcrm/issues/493) catalogued. Two sources resolve, and only two: -- **Platform data tools** — `describe_object`, `list_objects`, `query_records`, - `get_record`, `aggregate_data`. +- **Platform-provided tools** — the registry the platform serves, not a list to + guess at. The ones a CRM skill wants are `describe_object`, `list_objects`, + `query_records`, `query_data`, `get_record`, `aggregate_data`, + `search_knowledge` and `visualize_data`. From 17.0 the authoritative set is + exported as `PLATFORM_PROVIDED_TOOL_NAMES` from `@objectstack/spec/system`; + on 16.1.0 it is transcribed into `test/skills-integrity.test.ts`. Note + `search_knowledge` retrieves over a *declared knowledge source*, and there is + currently nowhere in a skills-only app to declare one. - **`action_`** — materialised from an Action that opts in with `ai: { exposed: true, description }` (ADR-0011, default off) *and* has a headless path. See `ConvertLeadAction` in `src/actions/lead.actions.ts`. diff --git a/src/skills/customer-360.skill.ts b/src/skills/customer-360.skill.ts index c2d49465..b793c6f8 100644 --- a/src/skills/customer-360.skill.ts +++ b/src/skills/customer-360.skill.ts @@ -5,20 +5,27 @@ 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. + * ADR-0109: this skill declares no bespoke tools. Its whole tool list used + * to be `search_knowledge`, while the instructions promised an account + + * cases + opportunities + knowledge roll-up it had no tool to read — it + * could not fetch a single record. * - * 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. + * `search_knowledge` is NOT fictional; #557 dropped it on that premise and + * the premise was wrong. It is a real platform tool, listed in + * `PLATFORM_PROVIDED_TOOL_NAMES` (`@objectstack/spec@17`) and documented + * back in 16.1.0 at `spec/src/ai/knowledge-source.zod.ts:114`. + * + * It is left out for a narrower reason: it has nothing here to search. + * `search_knowledge` retrieves over a declared knowledge source, and + * `AIKnowledgeSchema` mounts only on `AgentSchema.knowledge` — the + * `indexes: [...]` block that #512 deleted along with the agents. A + * skills-only app has nowhere to declare a source, so the tool would + * resolve and return nothing. That is the same lie one layer down. + * + * The knowledge base is `crm_knowledge_article`, a normal CRM object, so + * `query_records` reaches it exactly like every other object this skill + * reads. Revisit if a stack-level knowledge source ever becomes + * declarable. */ export const Customer360Skill = defineSkill({ name: 'customer_360', @@ -40,9 +47,9 @@ customer / account / contact: 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. + \`category\` or \`tags\` of the cases you just read. You have no + knowledge-search tool in this skill and 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 diff --git a/test/skills-integrity.test.ts b/test/skills-integrity.test.ts index 306ecd82..c2628e3a 100644 --- a/test/skills-integrity.test.ts +++ b/test/skills-integrity.test.ts @@ -10,11 +10,10 @@ import stack from '../objectstack.config'; * 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. + * instructions ("call `triage_case` first") describing a capability it does + * not have, and improvises. Every instance of this bug shipped green: issue + * #493 counted eleven such references across six skills and #512 removed + * ten, none of which any check could see. * * 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 @@ -39,40 +38,65 @@ 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 + * Tools the platform 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`. + * TRANSCRIBED VERBATIM from `PLATFORM_PROVIDED_TOOL_NAMES` in + * `@objectstack/spec@17.0.0-rc.0` (`dist/system`), which the upstream + * `ai-skill-tool-unresolved` rule resolves against. On the 17.0 upgrade, + * DELETE this literal and import the real thing: * - * 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. + * import { PLATFORM_PROVIDED_TOOL_NAMES } from '@objectstack/spec/system'; * - * 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. + * The transcription is a stopgap for 16.1.0, which exposes no such + * registry — and a hand-copied list is exactly the drift risk this file + * exists to catch, one level up. Do not extend it from memory: an entry + * that is merely aspirational reintroduces the bug. + * + * The first pass of this file guessed the list from what the + * `@objectstack/mcp@16.1.0` bridge happens to register, and got it wrong + * in both directions. It omitted `search_knowledge` — a real platform + * tool, documented in 16.1.0's own + * `spec/src/ai/knowledge-source.zod.ts:114` — and so would have failed a + * legitimate reference. It also reasoned about `create_record` / + * `update_record` / `delete_record` as tools to deliberately exclude; + * they are MCP-bridge tools and are not in the platform registry at all, + * so there was nothing to exclude. */ const PLATFORM_TOOLS = new Set([ - 'list_objects', - 'describe_object', - 'query_records', - 'get_record', + // Data / analytics — the 'ask' surface these skills bind to. '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. + 'get_record', + 'query_data', + 'query_records', + 'search_knowledge', 'visualize_data', + // Metadata authoring — the 'build' surface. In the registry, so a + // reference resolves; no HotCRM skill has business with them. + 'add_field', + 'apply_blueprint', + 'apply_edit', + 'create_metadata', + 'create_object', + 'create_package', + 'create_seed', + 'delete_field', + 'describe_metadata', + 'describe_object', + 'get_active_package', + 'get_metadata_schema', + 'get_package', + 'list_metadata', + 'list_objects', + 'list_packages', + 'modify_field', + 'propose_blueprint', + 'set_active_package', + 'suggest_builder', + 'todo_write', + 'update_metadata', + 'validate_expression', + 'verify_build', ]); /** `action_` — the tool the runtime materialises from an Action (ADR-0011). */