Support MCP tool annotations for composite tools

[JAORMX]

Summary

MCP clients use tool annotations (readOnlyHint, destructiveHint, idempotentHint, openWorldHint) to gate confirmation prompts and filter destructive actions. Composite tools never wired into the annotation path — ConvertWorkflowDefsToTools built each vmcp.Tool with Annotations left nil — so clients had no signal for composite tools even when the workflow author knew the semantics (e.g. a read-only report generator, or a workflow that deletes resources). Backend/aggregated tools already supported annotation overrides via ToolAnnotationsOverride; composite tools were the gap.

What changed:

• Added an optional Annotations field to CompositeToolConfig (reusing the existing ToolAnnotationsOverride type rather than introducing a third parallel annotations struct). It flows through composer.WorkflowDefinition into ConvertWorkflowDefsToTools, which now populates vmcp.Tool.Annotations. Because the CRD embeds CompositeToolConfig inline, the field appears automatically in both VirtualMCPCompositeToolDefinition and VirtualMCPServer schemas.
• Derive a conservative safety floor from the workflow's step tools when no explicit annotation is given: readOnlyHint is AND across steps, destructiveHint/openWorldHint are OR (an unknown/undeclared step taints them), and idempotentHint is never derived. The derivation fails closed: a workflow whose tool steps declare no annotations yields a conservative floor (not read-only, destructive, open-world) rather than no floor, so the guardrail still fires when backends are silent — the common case today.
• Contradiction guardrail: an explicit annotation that makes the tool look safer than the floor warrants (e.g. readOnlyHint: true while a step is destructive or undeclared) causes the tool to be dropped at advertise time with a warning naming the offending steps. A more-conservative explicit value (e.g. readOnlyHint: false on a read-only floor) is allowed.
• Extracted router.ResolveToolRef to hold the workload.tool step-ref resolution that was duplicated between isToolStepAccessible and the new stepAnnotationResolver (also fixes a latent nil-deref on a nil routing table).
• Regenerated CRDs + deepcopy; updated composite-tool docs and examples.

Closes #6192

Type of change

• New feature

Test plan

• Unit tests (task test)
• Linting (task lint-fix)

Unit tests cover the derivation truth tables (AND/OR/never-derived, fail-closed), the contradiction guardrail (each contradiction type + the allowed more-conservative case + nil-floor-hint guards), the merge semantics, the dotted-name resolver, and the conversion wiring, plus a YAML round-trip test guarding the json/yaml tag requirement. E2E specs were added under test/e2e/thv-operator/virtualmcp/ asserting derived annotations via ListTools, explicit pass-through, and drop-on-contradiction-while-Ready — these require a cluster and were written but not executed here (noted below).

API Compatibility

• This PR does not break the v1beta1 API, OR the api-break-allowed label is applied and the migration guidance is described above.

The change adds a single optional field (annotations) to CompositeToolConfig; it is not required, has no default that alters existing behavior, and no CEL rule that existing valid objects would violate. Fully backward-compatible.

Changes

File	Change
pkg/vmcp/config/config.go	Add Annotations *ToolAnnotationsOverride to CompositeToolConfig
pkg/vmcp/config/annotations.go	New ToAnnotations() converter on ToolAnnotationsOverride
pkg/vmcp/composer/composer.go	Carry Annotations on WorkflowDefinition
pkg/vmcp/server/workflow_converter.go	Map config annotations into the domain definition
pkg/vmcp/internal/compositetools/annotations.go	New derive / contradiction / merge logic (fail-closed)
pkg/vmcp/internal/compositetools/workflow_converter.go	Populate vmcp.Tool.Annotations; drop+warn on contradiction
pkg/vmcp/router/session_router.go	New shared ResolveToolRef step-ref resolution
pkg/vmcp/core/core_vmcp.go	Wire real resolver (advertise) / noop resolver (conflict check)
CRD yamls + zz_generated.deepcopy.go + crd-api.md	Regenerated
docs + examples	Annotations section, derivation rules, guardrail caveat

Does this introduce a user-facing change?

Yes. Workflow authors can now set annotations on a composite tool (inline in VirtualMCPServer, in a VirtualMCPCompositeToolDefinition, or in CLI vMCP config YAML). When omitted, safe defaults are derived from step tools. Note: thv vmcp validate performs structural validation only and does not check annotation contradictions — a contradictory annotation is surfaced at runtime (the tool is dropped and a warning is logged in vMCP), not at admission.

Implementation plan

Approved implementation plan

Reuse *config.ToolAnnotationsOverride as the field type (json+yaml tags, generated DeepCopy, controller-gen-covered) to avoid a third parallel annotations struct — *vmcp.ToolAnnotations was rejected (no yaml tags → breaks the operator yaml.Marshal path; no DeepCopy; pkg/vmcp root is outside controller-gen paths).

Flow: CompositeToolConfig.Annotations → composer.WorkflowDefinition.Annotations → three pure functions in a new compositetools/annotations.go (DeriveCompositeAnnotations, CheckAnnotationContradiction, MergeAnnotations), wired into ConvertWorkflowDefsToTools via an injected StepAnnotationResolver built at the one place backend annotations exist (core_vmcp.go advertisedTools). accessibleComposites uses a noop resolver (annotations are irrelevant to name-conflict detection).

Operator-approved decisions: (Q1) the floor-contradiction guardrail runs at runtime/advertise time with drop+warn posture — it cannot run at config load because step-tool refs are format-checked strings never resolved to backends pre-runtime; validate and the operator reconcile stay structural-only. (Q2) more-conservative explicit annotations are allowed. (Q3) full e2e through the operator Ginkgo harness.

A post-implementation review panel (spec / standards / security / architecture / duplication / library-reuse / devex / ux / QA) plus follow-up investigation surfaced and fixed: the fail-closed derivation defect (floor was nil when backends declared no annotations, bypassing the guardrail), a self-contradicting example, the duplicated step-ref resolution (extracted router.ResolveToolRef), pointer-aliasing in merge, and several UX/doc and test-adequacy gaps.

Special notes for reviewers

• Fail-closed derivation is the load-bearing decision. A workflow with ≥1 tool step whose backends declare no annotations yields a conservative floor (readOnlyHint=false, destructiveHint=true, openWorldHint=true), not a nil floor — otherwise the contradiction guardrail is silently bypassed whenever backends are silent (the common case today, e.g. the yardstick echo tool declares no annotations). A workflow with no tool steps (e.g. elicitation-only) still yields a nil floor. This is the behavior worth the closest look.
• Runtime (not admission) guardrail is deliberate. Step-tool refs are format-checked strings at config load and are never resolved to backends pre-runtime, so the floor only exists where agg.Tools is materialized. thv vmcp validate and the operator therefore cannot detect contradictions; this limitation is disclosed in the docs.
• E2E not executed here — no cluster available in the authoring environment. The new specs compile (go vet clean) and follow existing file patterns, but have not run against a live cluster; the e2e CI job should exercise them.
• Known follow-up (out of scope): the VirtualMCPCompositeToolDefinition Status.ValidationStatus is never written by any reconciler (pre-existing dead-status gap). A design for a small dedicated reconciler exists and is intentionally deferred to a separate PR — even with a writer, only structural validation can be surfaced there, never the runtime floor check.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 72.55%. Comparing base (227ee7b) to head (ac660b1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6208      +/-   ##
==========================================
+ Coverage   72.46%   72.55%   +0.08%     
==========================================
  Files         739      741       +2     
  Lines       76719    76838     +119     
==========================================
+ Hits        55594    55749     +155     
+ Misses      17160    17108      -52     
- Partials     3965     3981      +16     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[amirejaz]

Went through this properly. Two things I think need fixing before merge — I reproduced both with a throwaway test in pkg/vmcp/core (deleted afterwards), so they aren't theoretical.

1. A dropped composite is still executable

ConvertWorkflowDefsToTools drops the contradicting tool with the continue at workflow_converter.go:126-130, but that only filters what advertisedTools returns. accessibleComposites still returns defs untouched, and CallTool gates on accessibleComposites(agg)[name] (core_calls.go:56). So the guardrail hides the tool without withholding it.

Repro: workflow wf with readOnlyHint: true, one tool step against a backend tool that declares no annotations.

• advertisedTools(agg) → [be1.echo]; wf is dropped and the warning fires
• accessibleComposites(agg)["wf"] → present, so CallTool("wf") executes the workflow

Two comments assert the opposite and need updating either way: core_vmcp.go:654-656 ("single source of truth shared by advertisedTools and CallTool, so a withheld composite is never executed — advertised equals executed") and core_calls.go:52-55.

Knock-on effect: authorizeToolCall resolves the tool via findAdvertisedTool(c.advertisedTools(agg), name), so for a dropped composite it falls back to &vmcp.Tool{Name: name}. An annotation-gated admission policy then evaluates with no hints at all, on precisely the tool the guardrail flagged as unsafe.

2. The noop resolver lets genuine name conflicts through

accessibleComposites passes noopResolver (core_vmcp.go:670-676) on the basis that annotations are irrelevant to name collisions. But the noop resolver forces every step to "unknown", so the floor is always the conservative one, so any composite carrying a safety-claiming explicit hint gets dropped from the input to ValidateNoToolConflicts — including ones the real resolver would have allowed. A dropped composite can't be detected as conflicting.

Repro: composite named be1.echo (same name as the backend tool) with readOnlyHint: true survives the conflict check. accessibleComposites returns it, so a client calling the advertised backend tool be1.echo silently gets the workflow instead. Pre-PR that collision was caught and all composites dropped, so this is a regression in the collision guard.

Both bugs have the same root cause: ConvertWorkflowDefsToTools now does conversion and policy, while the conflict check only ever needed names. The D4 note at workflow_converter.go:211-215 defers extracting a CompositeToolNames primitive as low priority — it's actually load-bearing here. Extracting it fixes (2); applying the annotation guard inside accessibleComposites so it filters defs fixes (1) and restores the advertised-equals-executed invariant.

On drop-vs-clamp

Separate from the bugs, worth settling before this lands. The docs are candid enough about the drop semantics that they end up steering people away from the feature: the quick reference says "prefer leaving readOnlyHint unset", and the guide explains that with silent backends readOnlyHint: true "will contradict it and be dropped". Which means the motivating case in #6192 — a read-only report generator — can't be expressed against today's backends.

#6192 asked for rejection at config load/admission. The argument for moving it to runtime is sound (step refs aren't resolvable pre-aggregation), but drop is a separate choice on top of that, and it's the least forgiving one: silent, log-only, invisible to thv vmcp validate, and adding an annotation makes your tool disappear. Clamping instead — advertise the more conservative of explicit and floor, and warn — is equally safe for clients (nothing safety-lowering reaches them), keeps the tool working, and makes bug (1) moot because nothing is hidden in the first place. If drop stays, I think it needs a status condition rather than just a slog.Warn.

Related: the guide's headline example at virtualmcpcompositetooldefinition-guide.md:355-363 is readOnlyHint: true over step users.get, which the same document later explains would be dropped.

Smaller things

• The dedup is only half done. router.ResolveToolRef (session_router.go:91) was extracted, but RouteTool (:132) and ResolveToolName (:165) still carry byte-identical copies of the same resolution — three copies before, three after. ResolveToolName reduces to if n, ok := ResolveToolRef(...); ok { return n }; return toolName.
• The new e2e spec asserts echo_contradicting is absent from tools/list but not that it's uncallable. A CallTool on it expecting a failure is the assertion that would have caught bug (1).
• The nil-floor-hint branches in CheckAnnotationContradiction (annotations.go:102,108,114) are unreachable for any floor from DeriveCompositeAnnotations, which always populates all three hints — so the doc paragraph above them describes a case that can't happen.
• Every existing composite tool now advertises destructiveHint: true, openWorldHint: true where it previously advertised nothing. That matches the MCP spec defaults so conformant clients are unaffected, but clients that branch on annotations != nil will start badging composites destructive. Probably worth a line in the user-facing change section.
• ToolAnnotationsOverride's field docs all read "overrides the X annotation", which reads oddly for composites where they're declarations rather than overrides — and it shows up verbatim in the generated CRD schema and crd-api.md.

For what it's worth, reusing ToolAnnotationsOverride instead of adding a third annotations struct is the right call, and the AND/OR/never-derive truth table matches #6192 exactly. Unit coverage of the pure functions is solid.