Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 30 additions & 0 deletions forge-core/mcp/annotations_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
package mcp

import (
"encoding/json"
"testing"
)

// tools/list responses may carry the MCP annotations object; decode must
// surface it (platform discovery seeds side-effect classes from these hints)
// and its absence must stay distinguishable from explicit false.
func TestMCPToolDescriptor_AnnotationsDecode(t *testing.T) {
raw := []byte(`[
{"name":"list_issues","inputSchema":{},"annotations":{"readOnlyHint":true}},
{"name":"delete_issue","inputSchema":{},"annotations":{"destructiveHint":true,"readOnlyHint":false}},
{"name":"create_issue","inputSchema":{}}
]`)
var descs []MCPToolDescriptor
if err := json.Unmarshal(raw, &descs); err != nil {
t.Fatalf("decode: %v", err)
}
if descs[0].Annotations == nil || descs[0].Annotations.ReadOnlyHint == nil || !*descs[0].Annotations.ReadOnlyHint {
t.Fatalf("readOnlyHint lost: %+v", descs[0].Annotations)
}
if descs[1].Annotations == nil || descs[1].Annotations.DestructiveHint == nil || !*descs[1].Annotations.DestructiveHint {
t.Fatalf("destructiveHint lost: %+v", descs[1].Annotations)
}
if descs[2].Annotations != nil {
t.Fatalf("absent annotations must stay nil, got %+v", descs[2].Annotations)
}
}
13 changes: 13 additions & 0 deletions forge-core/mcp/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,4 +106,17 @@ type MCPToolDescriptor struct {
Name string `json:"name"`
Description string `json:"description,omitempty"`
InputSchema json.RawMessage `json:"inputSchema"`
// Annotations are the MCP spec's optional tool behavior hints
// (readOnlyHint / destructiveHint / idempotentHint). Advisory metadata
// from the server — surfaced so platform-side discovery can seed
// side-effect classifications; the runtime does not act on them.
Annotations *MCPToolAnnotations `json:"annotations,omitempty"`
}

// MCPToolAnnotations mirrors the MCP tool annotations object. Pointers so
// "absent" and "false" stay distinguishable.
type MCPToolAnnotations struct {
ReadOnlyHint *bool `json:"readOnlyHint,omitempty"`
DestructiveHint *bool `json:"destructiveHint,omitempty"`
IdempotentHint *bool `json:"idempotentHint,omitempty"`
}
21 changes: 19 additions & 2 deletions forge-skills/contract/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,8 +70,16 @@ type ForgeSkillMeta struct {
// parent agent's `tool.<name>` span via TRACEPARENT env propagation
// (issue #182). Wrapping the binary in a bash skill works too, but
// adds a fork and a wrapper to maintain.
Runtime string `yaml:"runtime,omitempty" json:"runtime,omitempty"`
Requires *SkillRequirements `yaml:"requires,omitempty" json:"requires,omitempty"`
Runtime string `yaml:"runtime,omitempty" json:"runtime,omitempty"`
Requires *SkillRequirements `yaml:"requires,omitempty" json:"requires,omitempty"`
// Uses declares the skill's governed tool dependencies — references into
// the PLATFORM tool registry (the org-scoped catalog of admitted tools),
// chosen as binary or MCP with a selected operation subset. Additive and
// DECLARATIVE from forge's perspective: the platform resolves and
// validates refs at authoring/build time and materializes the runtime
// wiring (mcp servers, allowlists); forge parses the block so the
// declaration travels with the skill, and does not gate on it.
Uses []SkillToolDependency `yaml:"uses,omitempty" json:"uses,omitempty"`
EgressDomains []string `yaml:"egress_domains,omitempty" json:"egress_domains,omitempty"`
DeniedTools []string `yaml:"denied_tools,omitempty" json:"denied_tools,omitempty"`
WorkflowPhase string `yaml:"workflow_phase,omitempty" json:"workflow_phase,omitempty"`
Expand Down Expand Up @@ -171,6 +179,15 @@ func (b *BinRequirement) UnmarshalYAML(value *yaml.Node) error {
}
}

// 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 {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Type string `yaml:"type" json:"type"` // binary | mcp
Ref string `yaml:"ref" json:"ref"` // registry entry key, e.g. "mcp.linear"
Operations []string `yaml:"operations,omitempty" json:"operations,omitempty"`
}

// SkillRequirements declares CLI binaries, environment variables, and runtime
// capabilities a skill needs.
type SkillRequirements struct {
Expand Down
12 changes: 12 additions & 0 deletions forge-skills/parser/parser.go
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,18 @@ func ExtractForgeMeta(meta *contract.SkillMetadata) *contract.ForgeSkillMeta {
return &forgeMeta
}

// ExtractForgeUses extracts the skill's governed tool dependencies
// (metadata.forge.uses — references into the platform tool registry). Thin
// wrapper over ExtractForgeMeta, mirroring ExtractForgeReqs. Returns nil when
// the skill declares none — the pre-registry common case.
func ExtractForgeUses(meta *contract.SkillMetadata) []contract.SkillToolDependency {
fm := ExtractForgeMeta(meta)
if fm == nil {
return nil
}
return fm.Uses
}

// ExtractForgeReqs extracts SkillRequirements, egress_domains, and guardrails from the generic
// metadata map. Thin wrapper over ExtractForgeMeta for existing call sites.
func ExtractForgeReqs(meta *contract.SkillMetadata) (*contract.SkillRequirements, []string, *contract.SkillGuardrailConfig) {
Expand Down
59 changes: 59 additions & 0 deletions forge-skills/parser/uses_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package parser

import (
"strings"
"testing"
)

const skillWithUses = `---
name: linear-triage
description: Triage Linear issues
metadata:
forge:
requires:
bins: [jq]
uses:
- type: mcp
ref: mcp.linear
operations: [create_issue, list_issues]
- type: binary
ref: bin.jq
operations: [exec]
---
# Linear triage
`

func TestExtractForgeUses(t *testing.T) {
_, meta, err := ParseWithMetadata(strings.NewReader(skillWithUses))
if err != nil {
t.Fatalf("parse: %v", err)
}
uses := ExtractForgeUses(meta)
if len(uses) != 2 {
t.Fatalf("want 2 deps, got %d: %+v", len(uses), uses)
}
if uses[0].Type != "mcp" || uses[0].Ref != "mcp.linear" || len(uses[0].Operations) != 2 {
t.Fatalf("mcp dep mismatch: %+v", uses[0])
}
if uses[1].Type != "binary" || uses[1].Ref != "bin.jq" {
t.Fatalf("binary dep mismatch: %+v", uses[1])
}
// requires.bins still parses beside it — the legacy surface is untouched.
reqs, _, _ := ExtractForgeReqs(meta)
if reqs == nil || len(reqs.Bins) != 1 || reqs.Bins[0].Name != "jq" {
t.Fatalf("requires.bins lost: %+v", reqs)
}
}

// A skill with no uses block — the universal pre-registry case — must parse
// exactly as before, with ExtractForgeUses returning nil.
func TestExtractForgeUses_AbsentIsNil(t *testing.T) {
const legacy = "---\nname: x\nmetadata:\n forge:\n requires:\n bins: [curl]\n---\nbody\n"
_, meta, err := ParseWithMetadata(strings.NewReader(legacy))
if err != nil {
t.Fatalf("parse: %v", err)
}
if uses := ExtractForgeUses(meta); uses != nil {
t.Fatalf("want nil, got %+v", uses)
}
}
Loading