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:
TestEveryRunnableCommandHasMCPAnnotationwalks the command tree and fails when any command the server would register (RunE != nil, matchingregisterToolsFromCommandsexactly) has no MCP annotation. Forgotten annotations used to drop commands from the tool surface silently. - Annotation gaps closed:
mr note list→ Safemr note resolve/mr note reopen→ Destructive (one factory, both subcommands)duo(the parent redirect command) → Excludegovern 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→Excludeonduo cliandorbit: neither is actually interactive (duo cli runis explicitly documented for "runners, scripts, and automated workflows";orbitforwards straight through to another binary viaDisableFlagParsing). Both already produced identical runtime behavior toExclude(never expose through MCP) — this is a naming/intent correction, not a behavior change.- Standardisation: six commands move from
Safe: "false"toDestructive: "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 — includingTestEveryRunnableCommandHasMCPAnnotation, 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'sSafeannotation and re-ran — test fails as expected. -
golangci-lintclean on every touched package.