diff --git a/.changeset/tool-requires-confirmation-removed.md b/.changeset/tool-requires-confirmation-removed.md new file mode 100644 index 0000000000..8e21c8c30f --- /dev/null +++ b/.changeset/tool-requires-confirmation-removed.md @@ -0,0 +1,50 @@ +--- +"@objectstack/spec": minor +"@objectstack/platform-objects": patch +--- + +feat(spec)!: remove `tool.requiresConfirmation` — a safety flag nothing enforced (#3715, ADR-0033 §2) + +`ToolSchema.requiresConfirmation` accepted `true` and no execution path ever read +it. Not the LLM tool set (a tool reaches the model as name/description/parameters +only), not `ToolRegistry.execute`, not `POST /ai/tools/:name/execute`, and not the +MCP bridge — which derives `destructiveHint` from a hardcoded name list. Setting +it on a destructive tool produced **no pause**. + +For an ordinary dead property that is untidy. For a **safety** property it is +false compliance, which is the case ADR-0049 exists for: an author gates a +destructive tool, sees the flag accepted, and ships believing a human is in the +loop. It is made worse by the near-miss — `action.ai.requiresConfirmation` has +the same name and **does** work, so the mistake reads as correct in review. +ADR-0033 §2 already resolved to delete this one. + +## Migration + +- **FROM:** `requiresConfirmation: true` on a tool definition +- **TO:** put the operation behind an action and set `ai.requiresConfirmation: + true` there — that is the flag the HITL approval queue reads + (`packages/runtime/src/action-execution.ts`) and the only path that actually + stops execution. +- For AI *metadata* mutations there is nothing to migrate: the ADR-0033 + draft/publish workspace is the gate — nothing is live until a human publishes. + +**`ToolSchema` is now `.strict()`.** This is load-bearing, not tidying. Removing a +key from a non-strict schema swaps one silent no-op for another: zod strips the +key wordlessly, the author keeps writing it, and the safety flag goes on meaning +nothing — the "silent strip" ADR-0032 / #1535 closed for objects. The retired key +now **rejects**, and the error carries the FROM → TO above, because a parse error +is the one channel every consumer bumping `@objectstack/spec` is guaranteed to +hit. + +Strictness applies to *all* unknown keys on a tool definition, so a typo +(`buildIn`, `catagory`) is now a located parse error instead of a silently +dropped field. + +Also removed: the Studio form row, its four generated locale bundles (the +`en`/`zh-CN`/`ja-JP`/`es-ES` strings still promised *"Ask user to approve before +executing (for destructive actions)"* — a translated false promise), the +liveness-ledger entry, and the generated reference-doc row. + +objectui's `ToolPreview.tsx` reads the field via `!!d.requiresConfirmation`, so it +degrades to "not shown" with no error; removing that badge is a follow-up in that +repo. diff --git a/content/docs/references/ai/tool.mdx b/content/docs/references/ai/tool.mdx index 726b323ae6..b7c198e283 100644 --- a/content/docs/references/ai/tool.mdx +++ b/content/docs/references/ai/tool.mdx @@ -40,7 +40,6 @@ AI tool definition. [READ-ONLY PROJECTION — not an execution entry point] Auth | **parameters** | `Record` | ✅ | JSON Schema for tool parameters | | **outputSchema** | `Record` | optional | [EXPERIMENTAL — not enforced] JSON Schema for tool output. Keys are folded into the tool description only; outputs are not validated (liveness #1878/#1893). | | **objectName** | `string` | optional | Target object name (snake_case) | -| **requiresConfirmation** | `boolean` | ✅ | [EXPERIMENTAL — not enforced] Require user confirmation before execution. NOTHING pauses on this flag (#3715) — use the action-level `ai.requiresConfirmation` + approval queue for a real gate. | | **permissions** | `string[]` | optional | Required permission-set capabilities | | **active** | `boolean` | ✅ | Whether the tool is enabled | | **builtIn** | `boolean` | ✅ | Platform built-in tool flag | diff --git a/packages/platform-objects/src/apps/translations/en.metadata-forms.generated.ts b/packages/platform-objects/src/apps/translations/en.metadata-forms.generated.ts index 9960e9a8c0..f25e3ba0ca 100644 --- a/packages/platform-objects/src/apps/translations/en.metadata-forms.generated.ts +++ b/packages/platform-objects/src/apps/translations/en.metadata-forms.generated.ts @@ -1693,10 +1693,6 @@ export const enMetadataForms: NonNullable = { label: "Output Schema", helpText: "Output schema for validation (optional)" }, - requiresConfirmation: { - label: "Requires Confirmation", - helpText: "Ask user to approve before executing (for destructive actions)" - }, permissions: { label: "Permissions", helpText: "Required permissions to use this tool" diff --git a/packages/platform-objects/src/apps/translations/es-ES.metadata-forms.generated.ts b/packages/platform-objects/src/apps/translations/es-ES.metadata-forms.generated.ts index 99070244ff..716bc318b9 100644 --- a/packages/platform-objects/src/apps/translations/es-ES.metadata-forms.generated.ts +++ b/packages/platform-objects/src/apps/translations/es-ES.metadata-forms.generated.ts @@ -1693,10 +1693,6 @@ export const esESMetadataForms: NonNullable = label: "Esquema de salida", helpText: "Esquema de salida para validación (opcional)" }, - requiresConfirmation: { - label: "Requiere confirmación", - helpText: "Pide aprobación al usuario antes de ejecutar (para acciones destructivas)" - }, permissions: { label: "Permisos", helpText: "Permisos necesarios para usar esta herramienta" diff --git a/packages/platform-objects/src/apps/translations/ja-JP.metadata-forms.generated.ts b/packages/platform-objects/src/apps/translations/ja-JP.metadata-forms.generated.ts index 720545ff56..ea77e31474 100644 --- a/packages/platform-objects/src/apps/translations/ja-JP.metadata-forms.generated.ts +++ b/packages/platform-objects/src/apps/translations/ja-JP.metadata-forms.generated.ts @@ -1693,10 +1693,6 @@ export const jaJPMetadataForms: NonNullable = label: "出力スキーマ", helpText: "検証用出力スキーマ（任意）" }, - requiresConfirmation: { - label: "確認が必要", - helpText: "実行前にユーザー承認を求める（破壊的アクション用）" - }, permissions: { label: "権限", helpText: "このツールの使用に必要な権限" diff --git a/packages/platform-objects/src/apps/translations/zh-CN.metadata-forms.generated.ts b/packages/platform-objects/src/apps/translations/zh-CN.metadata-forms.generated.ts index 132c7a1b87..a3f01f614f 100644 --- a/packages/platform-objects/src/apps/translations/zh-CN.metadata-forms.generated.ts +++ b/packages/platform-objects/src/apps/translations/zh-CN.metadata-forms.generated.ts @@ -1693,10 +1693,6 @@ export const zhCNMetadataForms: NonNullable = label: "输出 Schema", helpText: "用于校验的输出结构（可选）" }, - requiresConfirmation: { - label: "需要确认", - helpText: "在执行前请求用户确认（用于破坏性操作）" - }, permissions: { label: "权限", helpText: "使用此工具所需的权限" diff --git a/packages/spec/README.md b/packages/spec/README.md index ed9c97ae8e..bce44161f4 100644 --- a/packages/spec/README.md +++ b/packages/spec/README.md @@ -133,9 +133,11 @@ export const objectStackMCP = MCPServerConfigSchema.parse({ ], handler: 'flows.create_record', sideEffects: 'write', - // NOTE: declarative only — nothing pauses on this flag (#3715). A real - // human-in-the-loop gate is `ai.requiresConfirmation` on the underlying - // action (+ the approval queue), or `approval: 'always'` on an MCP binding. + // NOTE: an MCP capability-descriptor hint — surfaced to the client, but + // nothing server-side pauses on it. (The `ToolSchema` field of the same + // name was removed in 16.x — #3715, ADR-0033 §2.) A real human-in-the-loop + // gate is `ai.requiresConfirmation` on the underlying action (+ the + // approval queue), or `approval: 'always'` on an MCP binding. requiresConfirmation: true, }, { diff --git a/packages/spec/docs/MCP_GUIDE.md b/packages/spec/docs/MCP_GUIDE.md index 862ce47907..ed1807b3e1 100644 --- a/packages/spec/docs/MCP_GUIDE.md +++ b/packages/spec/docs/MCP_GUIDE.md @@ -359,8 +359,11 @@ For operations with side effects, gate them with something that is enforced: `approval: 'always'` on the MCP tool binding, or `ai.requiresConfirmation` on the underlying **action** (+ the HITL approval queue). -> ⚠️ `requiresConfirmation` on the **tool definition** is declared but read by -> no execution path — it produces no pause (#3715). +> ⚠️ The `requiresConfirmation` in these examples is an **MCP capability +> descriptor** hint — surfaced to the client in `list_actions` so it *may* ask, +> but nothing server-side pauses on it. Distinct from the `ToolSchema` field of +> the same name, which was REMOVED in the 16.x line (#3715, ADR-0033 §2) +> precisely because a safety flag no path enforces is false compliance. ### 4. Security diff --git a/packages/spec/liveness/README.md b/packages/spec/liveness/README.md index ad97648a38..006bd5a95f 100644 --- a/packages/spec/liveness/README.md +++ b/packages/spec/liveness/README.md @@ -376,7 +376,7 @@ EOF | permission | 32 | – | 0 | – | CRUD/FLS/RLS live; dead `contextVariables` REMOVED (ADR-0105 D11 — RLS resolves only the `current_user.*` built-ins plus runtime-staged `rlsMembership` sets) | | position | 4 | – | – | – | (role's ADR-0090 successor) fully live | | agent | 13 | 5 | 1 | – | dead `tenantId` + `planning.strategy`/`allowReplan` REMOVED (#2377) — only `planning.maxIterations` live; autonomy tier experimental; `knowledge` CORRECTED to dead 2026-07 — `search_knowledge` takes `sourceIds` from the LLM's tool-call args, never from the agent record (#1878 §3 recheck) | -| tool | 5 | 1 | 5 | – | the whole authoring surface is inert: `permissions` (not permission-gated), plus `category`/`requiresConfirmation`/`active`/`builtIn` CORRECTED to dead 2026-07 (#3686 sweep). ⚠️ `requiresConfirmation` is SAFETY-shaped and unenforced on every path — ADR-0033 already resolved to delete it | +| tool | 5 | 1 | 4 | – | the whole authoring surface is inert: `permissions` (not permission-gated), plus `category`/`active`/`builtIn` CORRECTED to dead 2026-07 (#3686 sweep). `requiresConfirmation` REMOVED (#3715, ADR-0033 §2) — SAFETY-shaped and unenforced on every path, so it was false compliance, not merely dead; ToolSchema is now `.strict()` so the retired key REJECTS with the FROM → TO prescription instead of being silently stripped | | skill | 8 | – | 1 | – | `permissions` REMOVED 2026-07 (never gated anything — owner call was prune, #3704); `triggerPhrases` CORRECTED to dead — phrases are never matched against user messages; activation is `triggerConditions` + the agent's `skills[]` + explicit /skill-name pinning (#3686 sweep) | | dataset | 19 | – | 0 | – | `measures.certified` (declared-but-unenforced governance flag) REMOVED in 16.0 (#2377) | | page | 16 | – | – | 1 | fully live + one planned | diff --git a/packages/spec/liveness/tool.json b/packages/spec/liveness/tool.json index d7038e8b6a..2bedce37f5 100644 --- a/packages/spec/liveness/tool.json +++ b/packages/spec/liveness/tool.json @@ -38,13 +38,6 @@ "authorHint": "Display-only — no runtime groups, filters or routes tools by category.", "note": "RE-VERIFIED 2026-07 (#3686 preview-claim sweep): the prior `live` verdict cited only a metadata-admin PREVIEW panel, which echoes what the author typed." }, - "requiresConfirmation": { - "status": "dead", - "evidence": "zero reads off any TOOL definition in any repo; every requiresConfirmation read is action.ai.requiresConfirmation (a different, live property). Ignored by the LLM tool set (vercel-adapter.ts:263-272), ToolRegistry.execute (tool-registry.ts:183-210), POST /ai/tools/:name/execute (tool-routes.ts:91,105) and the MCP bridge (framework packages/mcp/src/mcp-server-runtime.ts:176, which uses a hardcoded destructive-name list)", - "authorWarn": true, - "authorHint": "NOT ENFORCED — nothing pauses for confirmation on this flag, on any path. To gate a destructive operation, put it behind an action with `ai.requiresConfirmation` + the HITL approval queue, which is the only path that actually stops execution.", - "note": "RE-VERIFIED 2026-07 (#3686 preview-claim sweep): the prior `live` verdict cited only a metadata-admin PREVIEW panel, which echoes what the author typed. SAFETY-SHAPED: tool.form.ts:36-42 places it in a section titled 'Access & safety' with helpText 'Ask user to approve before executing (for destructive actions)', and skills/objectstack-ai/SKILL.md:547 tells authors to rely on it — so the authoring surface actively teaches a guarantee nothing delivers. ADR-0033 (:31, :60) already recorded it as never-enforced and resolved to DELETE the placeholder (the draft/publish workspace is the real approval gate); that deletion was never executed. OWNER CALL 2026-07: the prune-or-wire decision is DEFERRED (#3715), but the false promise stops now — the spec `.describe()`, the Studio form section (formerly titled \"Access & safety\", whose copy told authors to use it for destructive operations) and the SKILL.md / MCP_GUIDE / README guidance all now say NOT ENFORCED and point at the action-level gate." - }, "permissions": { "status": "dead", "evidence": "tool.form.ts only; not on AIToolDefinition, no consumer", diff --git a/packages/spec/src/ai/tool.form.ts b/packages/spec/src/ai/tool.form.ts index 13b1e5da6a..80f5644ded 100644 --- a/packages/spec/src/ai/tool.form.ts +++ b/packages/spec/src/ai/tool.form.ts @@ -43,7 +43,9 @@ export const toolForm = defineForm({ collapsible: true, collapsed: true, fields: [ - { field: 'requiresConfirmation', helpText: 'NOT ENFORCED (#3715) — nothing pauses for confirmation on this flag. For a real gate use the action-level `ai.requiresConfirmation` + approval queue; AI metadata edits are already gated by draft/publish.' }, + // `requiresConfirmation` was REMOVED from ToolSchema (#3715, ADR-0033 + // §2). A safety flag no path enforced is false compliance; the real + // gate is the action-level `ai.requiresConfirmation` + approval queue. { field: 'permissions', widget: 'string-tags', helpText: 'NOT ENFORCED — tool invocation is not permission-gated by this list. Gate the underlying action via permission sets (ADR-0066), or restrict the agent that exposes the tool.' }, ], }, diff --git a/packages/spec/src/ai/tool.test.ts b/packages/spec/src/ai/tool.test.ts index 2e98bee199..59ba70ded6 100644 --- a/packages/spec/src/ai/tool.test.ts +++ b/packages/spec/src/ai/tool.test.ts @@ -39,7 +39,6 @@ describe('ToolSchema', () => { expect(result.name).toBe('list_records'); expect(result.active).toBe(true); expect(result.builtIn).toBe(false); - expect(result.requiresConfirmation).toBe(false); }); it('should accept full tool', () => { @@ -64,7 +63,6 @@ describe('ToolSchema', () => { }, }, objectName: 'support_case', - requiresConfirmation: true, permissions: ['case.create', 'support.agent'], active: true, builtIn: false, @@ -74,7 +72,6 @@ describe('ToolSchema', () => { expect(result.name).toBe('create_case'); expect(result.category).toBe('action'); expect(result.objectName).toBe('support_case'); - expect(result.requiresConfirmation).toBe(true); expect(result.permissions).toEqual(['case.create', 'support.agent']); }); @@ -162,7 +159,6 @@ describe('defineTool', () => { expect(tool.active).toBe(true); expect(tool.builtIn).toBe(false); - expect(tool.requiresConfirmation).toBe(false); }); it('should throw on invalid tool name', () => { @@ -173,4 +169,42 @@ describe('defineTool', () => { parameters: {}, })).toThrow(); }); + + // ── #3715 / ADR-0033 §2 — the retired `requiresConfirmation` safety flag ── + // Removing a key from a NON-strict schema would swap one silent no-op for + // another (zod strips it wordlessly). These pin the loud rejection AND the + // prescription it must carry — the parse error is the one channel a consumer + // bumping @objectstack/spec is guaranteed to hit. + + it('REJECTS the retired `requiresConfirmation` instead of silently stripping it', () => { + const authored = { + name: 'delete_everything', + label: 'Delete Everything', + description: 'Destructive', + parameters: {}, + requiresConfirmation: true, + }; + expect(() => ToolSchema.parse(authored)).toThrow(/requiresConfirmation/); + expect(() => defineTool(authored as never)).toThrow(); + }); + + it('the rejection names the REAL gate, not merely the removal', () => { + let message = ''; + try { + ToolSchema.parse({ + name: 't', label: 'T', description: 'd', parameters: {}, requiresConfirmation: true, + }); + } catch (e) { + message = String((e as Error).message); + } + // FROM → TO: the action-level flag is the only path that stops execution. + expect(message).toMatch(/ai\.requiresConfirmation/); + expect(message).toMatch(/#3715/); + }); + + it('rejects an unrelated unknown key too (strictness is not special-cased)', () => { + expect(() => ToolSchema.parse({ + name: 't', label: 'T', description: 'd', parameters: {}, notAToolField: 1, + })).toThrow(/notAToolField/); + }); }); diff --git a/packages/spec/src/ai/tool.zod.ts b/packages/spec/src/ai/tool.zod.ts index 5f328abf26..3b5b73f4cd 100644 --- a/packages/spec/src/ai/tool.zod.ts +++ b/packages/spec/src/ai/tool.zod.ts @@ -29,6 +29,50 @@ export type ToolCategory = z.infer; // Tool Schema // ========================================== +/** + * Retired `ToolSchema` keys — the rejection carries the upgrade prescription, + * because the parse error is the one channel every consumer bumping + * `@objectstack/spec` is guaranteed to hit (pattern of `object.zod.ts`'s + * `UNKNOWN_KEY_GUIDANCE`, ADR-0049 enforce-or-remove). + */ +const TOOL_RETIRED_KEY_GUIDANCE: Record = { + requiresConfirmation: + '`tool.requiresConfirmation` was removed from @objectstack/spec in the 16.x line ' + + '(#3715, ADR-0033 §2) — it never had a consumer, and a SAFETY flag that is merely ' + + 'accepted is false compliance: authors set it on destructive tools believing the ' + + 'call would pause, and nothing ever did. No execution path read it — not the LLM ' + + 'tool set (a tool reaches the model as name/description/parameters only), not ' + + '`ToolRegistry.execute`, not `POST /ai/tools/:name/execute`, and not the MCP bridge ' + + '(which derives `destructiveHint` from a hardcoded name list). Delete the key. For a ' + + 'REAL gate on a destructive operation, put it behind an action and set ' + + '`action.ai.requiresConfirmation` — that is the flag the HITL approval queue reads ' + + '(packages/runtime/src/action-execution.ts), and it is the only path that actually ' + + 'stops execution. For AI metadata mutations the ADR-0033 draft/publish workspace is ' + + 'the gate: nothing is live until a human publishes.', +}; + +/** + * Custom zod `error` for the `.strict()` ToolSchema. + * + * `.strict()` matters more than usual here. Removing a key from a NON-strict + * schema replaces one silent no-op with another: the author keeps writing + * `requiresConfirmation: true`, zod strips it without a word, and the safety + * flag goes on meaning nothing — the exact "silent strip" ADR-0032 / #1535 + * closed for objects. Rejecting loudly, with the prescription attached, is what + * turns the removal into a fix instead of a rename of the problem. + */ +const strictToolError: z.core.$ZodErrorMap = (issue) => { + if (issue.code !== 'unrecognized_keys') return undefined; + const keys = (issue as { keys?: readonly string[] }).keys ?? []; + const lines = keys.map((key) => + TOOL_RETIRED_KEY_GUIDANCE[key] ?? `\`${key}\` is not a ToolSchema field.`, + ); + return ( + `Unrecognized key(s) on the tool definition: ${keys.map((k) => `\`${k}\``).join(', ')}.\n` + + lines.map((l) => ` • ${l}`).join('\n') + ); +}; + /** * Tool Schema * @@ -56,7 +100,6 @@ export type ToolCategory = z.infer; * required: ['subject'], * }, * objectName: 'support_case', - * requiresConfirmation: true, * }); * ``` */ @@ -96,24 +139,6 @@ export const ToolSchema = lazySchema(() => z.object({ */ objectName: z.string().regex(/^[a-z_][a-z0-9_]*$/).optional().describe('Target object name (snake_case)'), - /** - * Whether the tool requires human confirmation before execution. - * - * ⚠️ EXPERIMENTAL — NOT ENFORCED (#3715). No execution path reads this flag: - * not the LLM tool set (a tool reaches the model as name/description/ - * parameters only), not `ToolRegistry.execute`, not - * `POST /ai/tools/:name/execute`, and not the MCP bridge (which derives - * `destructiveHint` from a hardcoded name list). Setting it `true` on a - * destructive tool produces NO pause. - * - * The enforced human-in-the-loop path is the ACTION-level - * `ai.requiresConfirmation` (+ the approval queue). For AI metadata - * mutations the draft/publish workspace is the real gate (ADR-0033). - * Whether this per-tool flag gets wired to that queue or removed is - * tracked in #3715. - */ - requiresConfirmation: z.boolean().default(false).describe('[EXPERIMENTAL — not enforced] Require user confirmation before execution. NOTHING pauses on this flag (#3715) — use the action-level `ai.requiresConfirmation` + approval queue for a real gate.'), - /** Permission-set capabilities required to use this tool */ permissions: z.array(z.string()).optional().describe('Required permission-set capabilities'), @@ -136,7 +161,7 @@ export const ToolSchema = lazySchema(() => z.object({ // ADR-0010 — runtime protection envelope (internal — set by loader). ...MetadataProtectionFields, -}).describe('AI tool definition. [READ-ONLY PROJECTION — not an execution entry point] Authoring a tool as metadata does NOT make it runnable: this schema has no `implementation`/`handler` field and no framework executor loads a metadata-authored tool. The runtime executes a separately-registered `AIToolDefinition` (cloud `@objectstack/service-ai`); tool metadata is a one-way projection for Studio/discovery. Do not expect a hand-authored tool to run in the open edition (liveness audit #1878/#1892).')); +}, { error: strictToolError }).strict().describe('AI tool definition. [READ-ONLY PROJECTION — not an execution entry point] Authoring a tool as metadata does NOT make it runnable: this schema has no `implementation`/`handler` field and no framework executor loads a metadata-authored tool. The runtime executes a separately-registered `AIToolDefinition` (cloud `@objectstack/service-ai`); tool metadata is a one-way projection for Studio/discovery. Do not expect a hand-authored tool to run in the open edition (liveness audit #1878/#1892).')); export type Tool = z.infer; diff --git a/skills/objectstack-ai/SKILL.md b/skills/objectstack-ai/SKILL.md index 6416c784a9..7388d53210 100644 --- a/skills/objectstack-ai/SKILL.md +++ b/skills/objectstack-ai/SKILL.md @@ -303,7 +303,8 @@ Tools are the atomic operations that skills expose to agents. A tool authored as metadata (`type: 'tool'`, `*.tool.ts`) is validated by `ToolSchema`: required `name` / `label` / `description`, a **JSON Schema** `parameters` object, plus optional `category`, `objectName`, `outputSchema`, -`requiresConfirmation` (default `false`), `permissions`, `active`. +`permissions`, `active`. `ToolSchema` is **strict** — an unknown key (a typo, or +the retired `requiresConfirmation`) is a parse error, not a silent strip. ```typescript @@ -323,7 +324,6 @@ export default defineTool({ required: ['subject'], }, objectName: 'support_case', - requiresConfirmation: true, }); ``` @@ -554,8 +554,9 @@ On validation failure the runtime retries by default `ai.requiresConfirmation` on the **action**, or `approval: 'always'` on an MCP tool binding. AI metadata edits are already gated: they land as drafts a human must publish (ADR-0033). - ⚠️ Do **not** rely on `requiresConfirmation` on the **tool** — it is declared - but read by no execution path, so it produces no pause (#3715). + ⚠️ `requiresConfirmation` on the **tool** was REMOVED (#3715, ADR-0033 §2) — + it was read by no execution path, so it produced no pause. `ToolSchema` is + strict, so authoring it now fails the parse with the migration attached. There is no `requireApprovalFor` field. 4. **Ignoring tool descriptions.** The LLM uses tool `description` to decide when to call it. Poor descriptions = wrong tool selection.