Platform tool-registry contract: skill uses block + MCP tool annotations#309
Conversation
… annotations Two additive pieces the PLATFORM tool registry needs from forge (the registry itself lives on the platform; forge stays the runtime): - metadata.forge.uses — a skill's governed tool dependencies: references into the org's tool-registry (binary|mcp, entry key, selected operations). DECLARATIVE from forge's perspective: the platform resolves/validates refs at authoring/build time and materializes the runtime wiring (mcp servers + tool allowlists); forge parses the block so the declaration travels with the skill and never gates on it. Skills without the block parse byte-identically. - MCPToolDescriptor.Annotations — the MCP spec's readOnlyHint / destructiveHint / idempotentHint tool hints, surfaced from tools/list so platform-side discovery can seed side-effect classifications. Pointers keep absent vs explicit-false distinguishable; the runtime does not act on them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
initializ-mk
left a comment
There was a problem hiding this comment.
Review: platform tool-registry contract (skill uses + MCP annotations)
Two purely-additive, declarative-only changes, both inert in forge (runner untouched, platform validates + enforces). The scope discipline is exactly right for a contract-role PR, and the *bool tri-state (absent ≠ false) is the correct MCP-spec modeling — pinned by a test that specifically checks the nil-vs-false distinction. Old SKILL.md files parse unchanged (tested). CI fully green.
One actionable minor (inline), plus two observations:
2. Observation (not a blocker): uses fields are decoded leniently
No strict/DisallowUnknownFields decode on the forge-metadata path, so a type: mpc typo, a missing ref, or an unknown key inside a uses entry parses without complaint. This is consistent with the stated design (forge is declarative; the platform validates refs at authoring/build), so it's correct as-is — just being explicit that a skill author's uses typo is caught only platform-side, never by forge validate. If parse-time hygiene is ever wanted it'd mirror the auth_scheme/capability validation from #303/#297 (type ∈ {binary, mcp}, non-empty ref), but the platform-validates design legitimately defers that. No action needed.
3. Nit: MCP annotations omit openWorldHint (and title)
Capturing only the three side-effect hints is the right minimal scope for seeding side_effect_class; noting the omission looks deliberate and fine. openWorldHint is a one-field additive follow-up if platform discovery ever wants the open-world signal.
Test coverage
Good for an additive schema change: the annotations test pins all three hints AND the absent-stays-nil invariant (the load-bearing tri-state); the uses test covers a mixed binary/mcp block, asserts requires.bins still parses beside it (no legacy regression), and pins the absent-is-nil pre-registry path.
Verdict
Merge-ready — the only actionable item is the godoc placement (inline), a two-line comment relocation. Design correctly scoped, tri-state modeling right, tests pin the invariants that matter.
| // SkillToolDependency is one governed tool dependency: a reference to a | ||
| // platform tool-registry entry (by its stable key) with the operations this | ||
| // skill selects. Type distinguishes binary vs MCP registry entries. | ||
| type SkillToolDependency struct { |
There was a problem hiding this comment.
Minor (godoc bug): SkillToolDependency was inserted between the SkillRequirements doc comment and its type.
On the branch this now reads:
// SkillRequirements declares CLI binaries, environment variables, and runtime
// capabilities a skill needs.
// SkillToolDependency is one governed tool dependency: ...
type SkillToolDependency struct { ... }
type SkillRequirements struct { ... }With no blank line between the two comment paragraphs, godoc attaches the merged comment ("SkillRequirements declares… SkillToolDependency is one governed tool dependency…") to SkillToolDependency, and type SkillRequirements is left undocumented — its comment was orphaned onto the wrong type (go doc SkillRequirements now shows nothing).
Fix: move the whole SkillToolDependency block (with its own comment) above the // SkillRequirements declares… line, or below the SkillRequirements struct — anywhere that doesn't sit between a type's doc comment and its type keyword.
There was a problem hiding this comment.
Fixed in aa47131 — moved the whole SkillToolDependency block above the SkillRequirements doc comment. Verified with go doc: SkillRequirements shows its own comment again and SkillToolDependency carries only its own. Tests re-run with -count=1, green.
…godoc The insertion sat between SkillRequirements' doc comment and its type keyword, merging the comments onto the wrong type and leaving SkillRequirements undocumented (review #309). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
initializ-mk
left a comment
There was a problem hiding this comment.
Re-review of aa47131 — godoc finding resolved
The fix is exactly right: the // SkillRequirements declares… comment moved down to sit directly above type SkillRequirements struct, and SkillToolDependency now keeps only its own comment. Both types are correctly documented and neither comment straddles a type boundary — go doc SkillRequirements renders again. Pure comment relocation, no logic touched.
That was the sole actionable finding (observations 2–3 were correct-by-design and deliberate-scope, no change needed).
Verdict
Merge-ready once the pending Test check goes green (Lint already passed; this is a doc-comment-only move on an otherwise-green PR). Correctly-scoped additive contract, right tri-state modeling, invariant-pinning tests.
The pre-PDP Tool Registry lives on the platform (agent-builder catalog + console authoring); forge's part is only what a skill needs to declare and what discovery needs to read. Both additive:
No enforcement anywhere in forge — the PDP-era gates attach platform-side. Part of the tool-registry train with agent-builder + console PRs.
🤖 Generated with Claude Code