feat(spec)!: remove tool.requiresConfirmation — a safety flag nothing enforced (#3715, ADR-0033 §2) - #3876
Merged
Conversation
… 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 — 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. 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 it. Migration — FROM `requiresConfirmation: true` on a tool, TO an action carrying `ai.requiresConfirmation: true`, the flag the HITL approval queue actually reads (runtime/src/action-execution.ts). For AI metadata mutations nothing changes: the ADR-0033 draft/publish workspace is the gate. ToolSchema is now .strict(), which is load-bearing rather than tidying. Removing a key from a non-strict schema swaps one silent no-op for another — zod strips it 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 with the FROM -> TO prescription attached, because a parse error is the one channel every consumer bumping @objectstack/spec is guaranteed to hit. Typos on a tool definition are now located parse errors too. Also removed: the Studio form row; its four generated locale bundles, which still promised "Ask user to approve before executing (for destructive actions)" — a translated false promise; the liveness-ledger entry (tool dead 5 -> 4); and the generated reference-doc row. objectui's ToolPreview reads it as `!!d.requiresConfirmation`, so it degrades to "not shown" with no error; removing that badge is a follow-up in that repo. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ajwvrmd1hDC9RBofYBhGuR
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
github-actions
Bot
added
documentation
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 104 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
|
… skill example + clarify the MCP namesake CI's check:skill-examples caught what my own sweep missed: skills/objectstack-ai SKILL.md:326 authored `requiresConfirmation: true` in a `defineTool` example — one an AI copies verbatim — so removing the field left the skill teaching a call that now fails to parse. Fixed the example, the property list above it (which also promised `default false`), and the guardrails note, which now says the field was removed rather than merely unenforced. I missed it because my grep ended in `| head -10` and the skills/ hit was #11 — the exact failure the liveness README documents under "How to verify a claim without fooling yourself" (`… | head -3` hid the real hit further down the list). Re-swept untruncated. Also clarified the two MCP docs whose `requiresConfirmation` is a DIFFERENT, still-live thing — the MCP capability descriptor hint surfaced in list_actions (shape: handler + sideEffects + array params, not ToolSchema). Their notes cited #3715 as "declared but unenforced", which after the removal reads as if THIS is the field that went away. They now name which is which. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ajwvrmd1hDC9RBofYBhGuR
os-zhuang
marked this pull request as ready for review
July 28, 2026 14:06
os-zhuang
added a commit
that referenced
this pull request
Jul 28, 2026
…just skills/ (#3882) check:skill-examples compiles the TypeScript in prose against the built spec, so an example that stops compiling fails CI instead of quietly teaching code that no longer works. It does its job — it caught the broken defineTool example in #3876. But it only ever walked skills/: skills/ 9 files with ts blocks, 9 compiled content/docs/ 124 files with ts blocks, 0 compiled The identical break in a docs page ships. Docs examples are copied verbatim by humans and AI exactly like skill examples — the same shape as the stale-evidence and orphan-proof warnings fixed in #3857 / #3868: a gate covering a fraction of the surface it appears to cover reads as coverage. The walker is now a SOURCE_ROOTS list, and this lands the first batch: 164 docs blocks across 63 pages, taking the gate from 32 to 196 checked examples. content/docs/references/ is excluded — build-docs.ts regenerates it from the schemas, so it cannot drift independently. THE MARKER IS NOW PER-FORMAT. MDX has no HTML comments: `<!-- os:check -->` in a .mdx does not degrade, it fails the fumadocs build outright ("Unexpected character `!`… to create a comment in MDX, use `{/* text */}`"). Found by building the docs site after the first attempt broke 60+ pages; nothing in the type-check gate would have said so, because block extraction is regex-level and never parses MDX. skills/**/*.md keeps `<!-- os:check -->`; content/docs/**/*.mdx uses `{/* os:check */}`. Both spellings are recognised for ORPHAN detection, so a wrong-format marker fails loudly instead of silently checking nothing. The batch was measured, not guessed: marking all 780 docs blocks and compiling showed which are self-contained. One subtlety — a block that "passes" inside a 780-file program can be leaning on globals declared by OTHER blocks (a file with no import/export is a global script), so the first pass's 564 "passing" blocks collapsed to 164 once compiled as the real, smaller set. Converged by recompiling and dropping newly-failing blocks until green. The remaining blocks are mostly fragments, which the opt-in design anticipates. Whether any are genuine rot is now answerable for the first time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The enforce-or-remove the ledger flagged as its most severe entry.
The property
ToolSchema.requiresConfirmationacceptedtrueand no execution path ever read it:vercel-adapter.ts)ToolRegistry.executePOST /ai/tools/:name/executemcp-server-runtime.ts)destructiveHintfrom a hardcoded name listFor an ordinary dead property that's untidy. For a safety property it's false compliance — 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.
What makes it worse than most dead surface is the near-miss:
action.ai.requiresConfirmationhas the same name and does work (runtime/src/action-execution.ts:264). So the mistake reads as correct in review, and every grep for the identifier lands on hits that are genuinely live. ADR-0033 §2 already resolved this: "We delete the unenforcedrequiresConfirmationplaceholder on metadata tools."Migration
requiresConfirmation: trueon a tool definitionai.requiresConfirmation: truethere — the flag the HITL approval queue reads, and the only path that actually stops execution.ToolSchemais now.strict()— this is the load-bearing partRemoving a key from a non-strict schema swaps one silent no-op for another: zod strips it wordlessly, the author keeps writing
requiresConfirmation: true, and the safety flag goes on meaning nothing. That's the "silent strip" ADR-0032 / #1535 closed for objects, and it would have made this PR a rename of the problem rather than a fix.So the retired key now rejects, carrying the FROM → TO above — a parse error is the one channel every consumer bumping
@objectstack/specis guaranteed to hit. This follows the house pattern (ObjectCapabilities,tenancy): the tombstone lives in aTOOL_RETIRED_KEY_GUIDANCEmap wired through a$ZodErrorMap.Side benefit: a typo on a tool definition (
buildIn,catagory) is now a located parse error instead of a silently dropped field.Also removed
tool.form.ts);en/zh-CN/ja-JP/es-ESstill carried "Ask user to approve before executing (for destructive actions)", a translated false promise. The vocabulary test only checks retired groups, not fields, so nothing would have caught this;dead5 → 4);pnpm gen:docs— exactly one line, no churn).Verification
pnpm --filter @objectstack/spec test— 262 files, 6828 passed. Three new tests pin the behaviour: the key rejects rather than being stripped, the message namesai.requiresConfirmationand#3715, and an unrelated unknown key rejects too (strictness isn't special-cased to the tombstone).check:liveness— exits 0, zero warnings,toolnow 18 classified (13 live / 4 dead / 1 experimental).defineTool/ToolSchema.parsecall sites exist outside spec itself, so nothing in-repo constructs a Tool with extra keys.@objectstack/mcpand someplatform-objectssuites fail in this sandbox on unbuilt workspace deps (@objectstack/core,@objectstack/formula). Confirmed pre-existing by re-running them on a stashed tree — identical failure. CI builds first.Follow-up in objectui
ToolPreview.tsxreads the field as!!d.requiresConfirmation, so it degrades to "badge not shown" with no error — no break, but removing that badge (and thepreview-samples.tsentry) is a follow-up in that repo. Worth noting it was a preview renderer, which by this ledger's own doctrine was never evidence of a consumer in the first place.Generated by Claude Code