From d8a714d49f8458f9bdd312902405828b15872866 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 16:30:42 +0000 Subject: [PATCH] =?UTF-8?q?fix(spec):=20tombstone=20agent.tools=20instead?= =?UTF-8?q?=20of=20deleting=20it=20=E2=80=94=20unbreak=20main=20(#3894=20f?= =?UTF-8?q?ollow-up)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #3894 removed `agent.tools` and `AIToolSchema` outright, which broke `pnpm --filter @objectstack/spec build` on main: the authorable-surface ratchet (ADR-0104 / #3733) fails when an authorable key disappears, because none of these schemas is `.strict()` — Zod silently STRIPS an unknown key, so an author who keeps writing `tools:` gets a clean parse and an agent that reaches none of the tools they listed. That is the silent-capability-loss shape #3820 exists to eliminate, restored one layer down. The gate was right; my removal was wrong. (It slipped CI because Build Core restored a turbo cache entry for the spec build; the failure reproduces on any cold build of main.) The removal stands — ADR-0064 needs the second, unscoped tool slot gone. What changes is HOW it is removed: - `agent.tools` is now `retiredKey()`, so authoring it throws with the fix in the message (use `skills`; a platform tool by name, or `action_` for your own AI-exposed Action; `os migrate meta`). This supersedes #3894's "remains a silent no-op rather than a parse error" — loud is correct, and is what the ratchet requires. - A D2 conversion `agent-tools-to-skills` + its D3 chain step, so the removal reaches spec-changes.json, the upgrade guide and the `spec_changes` MCP tool. Unlike the protocol-17 renames beside it this has NO lossless target: each entry must become a reference inside a skill, which is a human decision. So it drops the dead key (the runtime stopped reading it in cloud#910, so it already contributes nothing) and emits one notice per agent marking where capability must be re-declared. - The three `ai/AITool:*` baseline lines are deleted deliberately — the one case the ratchet sanctions in-PR. They were authorable only as the element shape of `agent.tools`; with the parent tombstoned nothing reaches them, so they cannot vanish silently: the parent speaks first, with a prescription. Keeping a schema alive purely to hold three unreachable lines would be the dead contract surface this issue is about. Agent tests now pin the rejection and its message rather than the strip semantics they asserted before. Verified: spec build OK, spec 6823 tests, lint 540, cli 761, full workspace build, doc-authoring guard, check:docs, check:api-surface, check:skill-refs, check:skill-examples (197), eslint — all green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01BHjroNkLkajskKbJaidko4 --- .changeset/tombstone-agent-tools.md | 40 ++++++++ content/docs/references/ai/agent.mdx | 1 + packages/spec/authorable-surface.json | 10 +- packages/spec/src/ai/agent.test.ts | 115 +++++++--------------- packages/spec/src/ai/agent.zod.ts | 31 ++++-- packages/spec/src/conversions/registry.ts | 52 ++++++++++ packages/spec/src/migrations/registry.ts | 13 ++- 7 files changed, 168 insertions(+), 94 deletions(-) create mode 100644 .changeset/tombstone-agent-tools.md diff --git a/.changeset/tombstone-agent-tools.md b/.changeset/tombstone-agent-tools.md new file mode 100644 index 0000000000..7f3a5e6037 --- /dev/null +++ b/.changeset/tombstone-agent-tools.md @@ -0,0 +1,40 @@ +--- +"@objectstack/spec": patch +--- + +fix(spec): tombstone `agent.tools` instead of deleting it — main was red (#3894 follow-up) + +#3894 removed `agent.tools` (and `AIToolSchema`) outright. That broke +`pnpm --filter @objectstack/spec build` on `main`: the authorable-surface +ratchet (ADR-0104 / #3733) fails when an authorable key disappears from +the contract, because none of these schemas is `.strict()` — Zod silently +STRIPS an unknown key, so an author who keeps writing `tools:` would get a +clean parse and an agent that reaches none of the tools they listed. That +is the same silent-capability-loss shape #3820 exists to eliminate, +restored one layer down. The gate was right and the removal was wrong. + +The removal itself stands — ADR-0064's "an agent reaches exactly its +surface-compatible skills' tools, nothing falls through to the global +registry" needs the second slot gone. What changes is HOW: + +- **`agent.tools` is now `retiredKey()`** — authoring it throws with the + fix in the message (use `skills`; a platform tool by name, or + `action_` for your own AI-exposed Action; `os migrate meta + --from 16`). This supersedes #3894's changeset line saying the key + "remains a silent no-op rather than a parse error": loud is correct, + and it is what this repo's ratchet requires. +- **A D2 conversion `agent-tools-to-skills`** plus its D3 chain step, so + the removal reaches `spec-changes.json`, the upgrade guide, and the + `spec_changes` MCP tool. Unlike the protocol-17 renames beside it this + has no lossless target — each entry must become a reference inside a + skill, a human decision — so the conversion drops the dead key (the + runtime stopped reading it in cloud#910) and emits one notice per agent + marking where capability has to be re-declared. +- **The three `ai/AITool:*` baseline lines are deleted deliberately**, the + one case the ratchet sanctions in-PR. Those keys were authorable only as + the element shape of `agent.tools`; with the parent tombstoned there is + no path that reaches them, so they cannot vanish silently — the parent + speaks first, with a prescription. + +Agent tests updated to pin the rejection (and its message) rather than the +strip semantics they asserted before. diff --git a/content/docs/references/ai/agent.mdx b/content/docs/references/ai/agent.mdx index d3b8af48ca..63a297e6c9 100644 --- a/content/docs/references/ai/agent.mdx +++ b/content/docs/references/ai/agent.mdx @@ -66,6 +66,7 @@ const result = AIKnowledge.parse(data); | **lifecycle** | `{ id: string; description?: string; contextSchema?: Record; initial: string; … }` | optional | [EXPERIMENTAL — not enforced] State machine defining the agent conversation flow and constraints. Parsed but no runtime consumer yet (liveness #1878/#1893). | | **surface** | `Enum<'ask' \| 'build'>` | ✅ | Product surface this agent binds ('ask' \| 'build') — ADR-0063 §1 | | **skills** | `string[]` | optional | Skill names to attach (Agent→Skill→Tool architecture) | +| **tools** | `any` | optional | [REMOVED] `agent.tools` was removed in @objectstack/spec 17 (#3894) — use `skills`. An agent reaches exactly the tools its surface-compatible skills declare (ADR-0064), so move each reference into a skill: a platform tool by its registered name, or `action_` for one of your own AI-exposed Actions. Run `os migrate meta --from 16` to rewrite it automatically. | | **knowledge** | `{ sources?: string[]; topics?: any; indexes: string[] }` | optional | RAG access | | **active** | `boolean` | ✅ | | | **access** | `string[]` | optional | Who can chat with this agent | diff --git a/packages/spec/authorable-surface.json b/packages/spec/authorable-surface.json index 8e1f724e61..cb696817e5 100644 --- a/packages/spec/authorable-surface.json +++ b/packages/spec/authorable-surface.json @@ -9,9 +9,6 @@ "ai/AIModelConfig:provider", "ai/AIModelConfig:temperature", "ai/AIModelConfig:topP", - "ai/AITool:description", - "ai/AITool:name", - "ai/AITool:type", "ai/AIUsageRecord:costUsd", "ai/AIUsageRecord:latencyMs", "ai/AIUsageRecord:model", @@ -42,7 +39,7 @@ "ai/Agent:skills", "ai/Agent:structuredOutput", "ai/Agent:surface", - "ai/Agent:tools", + "ai/Agent:tools [RETIRED]", "ai/BlueprintApp:icon", "ai/BlueprintApp:label", "ai/BlueprintApp:name", @@ -819,12 +816,17 @@ "api/CreateViewResponse:object", "api/CreateViewResponse:view", "api/CreateViewResponse:viewId", + "api/CrossObjectBatchDroppedFields:fields", + "api/CrossObjectBatchDroppedFields:index", + "api/CrossObjectBatchDroppedFields:object", + "api/CrossObjectBatchDroppedFields:reason", "api/CrossObjectBatchOperation:action", "api/CrossObjectBatchOperation:data", "api/CrossObjectBatchOperation:id", "api/CrossObjectBatchOperation:object", "api/CrossObjectBatchRequest:atomic", "api/CrossObjectBatchRequest:operations", + "api/CrossObjectBatchResponse:droppedFields", "api/CrossObjectBatchResponse:results", "api/CrudEndpointPattern:description", "api/CrudEndpointPattern:method", diff --git a/packages/spec/src/ai/agent.test.ts b/packages/spec/src/ai/agent.test.ts index a5a946ad56..90d090de2a 100644 --- a/packages/spec/src/ai/agent.test.ts +++ b/packages/spec/src/ai/agent.test.ts @@ -67,18 +67,31 @@ describe('AIModelConfigSchema', () => { }); }); -describe('agent.tools removal (ADR-0064 / #3820)', () => { - it('strips a legacy inline tools array instead of carrying it', () => { - // The field is gone, so Zod drops it rather than handing the runtime a - // second, unscoped tool slot to disagree with the skills. Authoring one - // is a no-op, not a parse error — an existing stack keeps parsing. +describe('agent.tools retirement (ADR-0064 / #3820, tombstoned in #3894)', () => { + it('REJECTS a legacy inline tools array, with the fix in the message', () => { + // Tombstoned, not deleted: AgentSchema is not `.strict()`, so a plain + // deletion would silently strip the key and the agent would quietly reach + // none of the tools its author listed. `retiredKey()` makes it audible. + expect(() => + AgentSchema.parse({ + name: 'legacy', + label: 'Legacy', + role: 'r', + instructions: 'x', + tools: [{ type: 'action', name: 'create_ticket' }], + }), + ).toThrow(/agent\.tools.*removed.*use `skills`/s); + }); + + it('parses cleanly once the capability moves into skills', () => { const parsed = AgentSchema.parse({ name: 'legacy', label: 'Legacy', role: 'r', instructions: 'x', - tools: [{ type: 'action', name: 'create_ticket' }], + skills: ['case_management'], }); + expect(parsed.skills).toEqual(['case_management']); expect(parsed).not.toHaveProperty('tools'); }); }); @@ -239,16 +252,13 @@ describe('AgentSchema', () => { expect(() => AgentSchema.parse(agent)).not.toThrow(); }); - it('should accept agent with both tools and knowledge', () => { + it('should accept agent with both skills and knowledge', () => { const agent: Agent = { name: 'full_agent', label: 'Complete Agent', role: 'Full-Stack Assistant', instructions: 'Comprehensive assistant with all capabilities.', - tools: [ - { type: 'action', name: 'create_record' }, - { type: 'flow', name: 'process_data' }, - ], + skills: ['record_management', 'reporting'], knowledge: { sources: ['everything'], indexes: ['master_index'], @@ -272,19 +282,17 @@ describe('AgentSchema', () => { expect(result.skills).toContain('case_management'); }); - it('keeps skills and drops a legacy inline tools array', () => { - const agent = { - name: 'hybrid_agent', - label: 'Hybrid Agent', - role: 'Versatile Assistant', - instructions: 'Skills carry the capability.', - skills: ['case_management'], - tools: [{ type: 'action', name: 'send_email' }], - }; - - const result = AgentSchema.parse(agent); - expect(result.skills).toHaveLength(1); - expect(result).not.toHaveProperty('tools'); + it('rejects an agent that still carries a legacy inline tools array', () => { + expect(() => + AgentSchema.parse({ + name: 'hybrid_agent', + label: 'Hybrid Agent', + role: 'Versatile Assistant', + instructions: 'Skills carry the capability.', + skills: ['case_management'], + tools: [{ type: 'action', name: 'send_email' }], + }), + ).toThrow(); }); it('should accept agent with permissions', () => { @@ -377,28 +385,7 @@ Always be polite, empathetic, and solution-oriented.`, temperature: 0.7, maxTokens: 2048, }, - tools: [ - { - type: 'action', - name: 'create_support_ticket', - description: 'Create a new support ticket', - }, - { - type: 'action', - name: 'escalate_to_human', - description: 'Transfer conversation to human agent', - }, - { - type: 'query', - name: 'search_tickets', - description: 'Search existing support tickets', - }, - { - type: 'vector_search', - name: 'kb_search', - description: 'Search knowledge base', - }, - ], + skills: ['record_management', 'reporting'], knowledge: { sources: ['product_docs', 'faq', 'troubleshooting', 'api_reference'], indexes: ['support_kb_v2'], @@ -431,28 +418,7 @@ Be persuasive but honest. Focus on value creation.`, model: 'claude-3-sonnet-20240229', temperature: 0.8, }, - tools: [ - { - type: 'query', - name: 'get_account_info', - description: 'Retrieve account details', - }, - { - type: 'action', - name: 'update_opportunity', - description: 'Update opportunity fields', - }, - { - type: 'action', - name: 'send_email', - description: 'Send email via template', - }, - { - type: 'flow', - name: 'create_follow_up_task', - description: 'Schedule follow-up activity', - }, - ], + skills: ['record_management', 'reporting'], knowledge: { sources: ['sales_playbooks', 'product_features', 'case_studies', 'competitor_analysis'], indexes: ['sales_intelligence'], @@ -484,18 +450,7 @@ Be precise, data-driven, and clear in your explanations.`, temperature: 0.3, maxTokens: 4096, }, - tools: [ - { - type: 'query', - name: 'execute_sql', - description: 'Run SQL queries on the data warehouse', - }, - { - type: 'action', - name: 'create_dashboard', - description: 'Generate dashboard from metrics', - }, - ], + skills: ['record_management', 'reporting'], knowledge: { sources: ['sql_guides', 'metrics_definitions'], indexes: ['analytics_kb'], diff --git a/packages/spec/src/ai/agent.zod.ts b/packages/spec/src/ai/agent.zod.ts index c20656239d..38c7aabe42 100644 --- a/packages/spec/src/ai/agent.zod.ts +++ b/packages/spec/src/ai/agent.zod.ts @@ -143,15 +143,28 @@ export const AgentSchema = lazySchema(() => z.object({ /** Capabilities — Skill-based (primary) */ skills: z.array(z.string().regex(/^[a-z_][a-z0-9_]*$/)).optional().describe('Skill names to attach (Agent→Skill→Tool architecture)'), - // `tools` (the legacy inline `{type,name,description}[]` fallback) was - // REMOVED — ADR-0064's central invariant is "an agent's tool set is the - // union of its surface-compatible skills' tools; nothing falls through to - // the global registry", and this field was the one seam that broke it: the - // runtime resolved `agent.tools[].name` against the FULL registry with no - // surface check, so an `ask`-surface agent could name an authoring tool and - // get it. The invariant is now structural — there is no second slot to - // disagree with the skills — rather than a rule every reader must remember - // (ADR-0049 "design+enforce or remove"). Attach capability via `skills`. + /** + * [REMOVED in protocol 17 — #3894] The legacy inline + * `{type,name,description}[]` fallback. + * + * ADR-0064's central invariant is "an agent's tool set is the union of its + * surface-compatible skills' tools; nothing falls through to the global + * registry", and this field was the one seam that broke it: the runtime + * resolved `agent.tools[].name` against the FULL registry with no surface + * check, so an `ask`-surface agent could name an authoring tool and get it. + * + * Tombstoned rather than deleted: `AgentSchema` is not `.strict()`, so a + * plain deletion would silently strip the key and the agent would quietly + * reach none of the tools its author listed — the same silent-capability-loss + * shape this whole issue is about (#3820), restored one layer down. + */ + tools: retiredKey( + '`agent.tools` was removed in @objectstack/spec 17 (#3894) — use `skills`. ' + + 'An agent reaches exactly the tools its surface-compatible skills declare ' + + '(ADR-0064), so move each reference into a skill: a platform tool by its ' + + 'registered name, or `action_` for one of your own AI-exposed Actions. ' + + 'Run `os migrate meta --from 16` to rewrite it automatically.', + ), /** Knowledge */ knowledge: AIKnowledgeSchema.optional().describe('RAG access'), diff --git a/packages/spec/src/conversions/registry.ts b/packages/spec/src/conversions/registry.ts index 529426c95a..96d005c88a 100644 --- a/packages/spec/src/conversions/registry.ts +++ b/packages/spec/src/conversions/registry.ts @@ -771,6 +771,57 @@ const agentKnowledgeTopicsToSources: MetadataConversion = { }, }; +/** + * Agent `tools` → dropped (protocol 17, #3894 / #3820). + * + * NOT a rename — there is no key to move the value to. ADR-0064 says an + * agent's tool set is exactly the union of its surface-compatible skills' + * tools, and `agent.tools[]` was the seam that broke it (it resolved names + * against the FULL registry with no surface check). Each entry has to become + * a reference inside a SKILL, which needs a human decision about which skill + * — so this conversion drops the dead key and emits one notice per agent + * naming what was lost, rather than guessing a destination. + * + * Dropping is safe: the cloud runtime stopped reading the field entirely + * (cloud#910), so by protocol 17 it contributes nothing at load time. What + * the notice preserves is the AUTHOR's knowledge of which tools they meant. + */ +const agentToolsToSkills: MetadataConversion = { + id: 'agent-tools-to-skills', + toMajor: 17, + retiredFromLoadPath: true, + surface: 'agent.tools', + summary: "agent key 'tools' removed — declare capability in a skill (ADR-0064, #3894)", + apply(stack, emit) { + return mapCollection(stack, 'agents', (agent, path) => { + if (!('tools' in agent) || agent.tools == null) return agent; + const next: Dict = { ...agent }; + delete next.tools; + // The notice carries `tools → skills` at the agent's path: the author + // sees WHICH agent lost inline references and where the capability has + // to be re-declared. The tool names themselves stay in their git + // history, which is where a judgement call should be read from. + emit({ from: 'tools', to: 'skills', path: `${path}.skills` }); + return next; + }); + }, + fixture: { + before: { + agents: [ + { + name: 'support_bot', + skills: ['case_management'], + tools: [{ type: 'action', name: 'create_ticket' }], + }, + ], + }, + after: { + agents: [{ name: 'support_bot', skills: ['case_management'] }], + }, + expectedNotices: 1, + }, +}; + /** * Sharing-rule `accessLevel: 'full'` → `'edit'` (protocol 17, #3865). * @@ -849,6 +900,7 @@ export const CONVERSIONS_BY_MAJOR: Readonly