feat(mcp): close annotation coverage gaps and add coverage test

Summary

Lifts MCP annotation coverage from "most commands" to "every command the MCP server would register as a tool", and standardises the annotation idiom along the way.

  • Coverage test: TestEveryRunnableCommandHasMCPAnnotation walks the command tree and fails when any command the server would register (RunE != nil, matching registerToolsFromCommands exactly) has no MCP annotation. Forgotten annotations used to drop commands from the tool surface silently.
  • Annotation gaps closed:
    • mr note list → Safe
    • mr note resolve / mr note reopen → Destructive (one factory, both subcommands)
    • duo (the parent redirect command) → Exclude
    • govern audit sync, govern doctor, govern setup → Destructive, Safe, Destructive respectively (these three merged onto main after this MR was branched, and the coverage test's tightened rule — see below — is what caught them)
    • dependency-firewall ci-summary, repo remote add, security config enable/disable/status, skills install/update, stack infer → reviewed and annotated (thanks @viktomas for double-checking these)
  • Interactive → Exclude on duo cli and orbit: neither is actually interactive (duo cli run is explicitly documented for "runners, scripts, and automated workflows"; orbit forwards straight through to another binary via DisableFlagParsing). Both already produced identical runtime behavior to Exclude (never expose through MCP) — this is a naming/intent correction, not a behavior change.
  • Standardisation: six commands move from Safe: "false" to Destructive: "true" (milestone create / delete / edit, project members add / remove, runner assign). Runtime-identical; grep no longer returns the ambiguous form.

The coverage test's rule changed from "leaf commands only" to "any command with a RunE", matching what registerToolsFromCommands actually registers — a parent command with its own RunE (like mr note, whose RunE is the create tool) is registered by the server even though it also has subcommands, and the original "leaf" rule missed that. Caught in review by @viktomas, together with the duo/orbit annotation fix above.

The three per-command annotation tests this MR originally added (duo cli, mr note list, mr note resolve/reopen) are removed: the walker test already covers presence of an annotation for every registered command, so they only duplicated that check.

Context

Related to #8157

Stack #2 (closed) of 6 split out from the closed MR !3154. Builds on !3253 (closed) (fix(mcp): wrap non-object outputs as valid structuredContent).

Test plan

  • go test ./internal/commands/... passes — including TestEveryRunnableCommandHasMCPAnnotation, which scans every command the server would register.
  • grep -rn 'Safe: "false"' internal/commands/ returns no production-code matches.
  • Confirmed the coverage test actually catches gaps: temporarily removed govern/doctor's Safe annotation and re-ran — test fails as expected.
  • golangci-lint clean on every touched package.
Edited by James Hebden

Merge request reports

Loading
Loading