← back to Cli Printing Press
fix(cli): honor explicit "mcp:read-only": "false" in tools-audit (#1485)
ede5c3506e20891804df592e787943f581b41278 · 2026-05-15 13:58:42 -0700 · Trevin Chow
* fix(cli): honor explicit "mcp:read-only": "false" in tools-audit
inspectAnnotations collapsed "true" and absent into hasReadOnly=false,
so kindMissingReadOnly fired on commands whose authors had correctly
opted out by writing "false" (e.g., `watch report` — name matches the
read-shape heuristic but the impl actually fetches+upserts).
Rename the signal to hasExplicitReadOnly: true when the annotation
key is present with ANY value, false only when genuinely absent. The
audit's job is to flag unannotated commands, not enforce that every
read-shaped name be read-only.
Closes #891
* test(cli): move hasExplicitReadOnly doc to right field; trim redundant case
Greptile noted two small issues in the previous commit:
- The new hasExplicitReadOnly doc comment sat above `short` due to a
struct-field-order accident. Move it so go doc/IDE hover land on the
field the comment describes.
- The auditCommandFields tests had two structurally identical cases
(explicit-true and explicit-false both bottom out at
hasExplicitReadOnly=true by the time they reach this function, so
the function can't distinguish them — the AST-level differentiation
lives in TestInspectAnnotationsExplicitReadOnlyFalse, which already
has all three input shapes). Collapse to a single "annotation present"
case at the auditCommandFields layer.
Refs #891
Files touched
M internal/cli/tools_audit.goM internal/cli/tools_audit_test.go
Diff
commit ede5c3506e20891804df592e787943f581b41278
Author: Trevin Chow <trevin@trevinchow.com>
Date: Fri May 15 13:58:42 2026 -0700
fix(cli): honor explicit "mcp:read-only": "false" in tools-audit (#1485)
* fix(cli): honor explicit "mcp:read-only": "false" in tools-audit
inspectAnnotations collapsed "true" and absent into hasReadOnly=false,
so kindMissingReadOnly fired on commands whose authors had correctly
opted out by writing "false" (e.g., `watch report` — name matches the
read-shape heuristic but the impl actually fetches+upserts).
Rename the signal to hasExplicitReadOnly: true when the annotation
key is present with ANY value, false only when genuinely absent. The
audit's job is to flag unannotated commands, not enforce that every
read-shaped name be read-only.
Closes #891
* test(cli): move hasExplicitReadOnly doc to right field; trim redundant case
Greptile noted two small issues in the previous commit:
- The new hasExplicitReadOnly doc comment sat above `short` due to a
struct-field-order accident. Move it so go doc/IDE hover land on the
field the comment describes.
- The auditCommandFields tests had two structurally identical cases
(explicit-true and explicit-false both bottom out at
hasExplicitReadOnly=true by the time they reach this function, so
the function can't distinguish them — the AST-level differentiation
lives in TestInspectAnnotationsExplicitReadOnlyFalse, which already
has all three input shapes). Collapse to a single "annotation present"
case at the auditCommandFields layer.
Refs #891
---
internal/cli/tools_audit.go | 34 ++++++++----
internal/cli/tools_audit_test.go | 117 +++++++++++++++++++++++++++++++++++++++
2 files changed, 141 insertions(+), 10 deletions(-)
diff --git a/internal/cli/tools_audit.go b/internal/cli/tools_audit.go
index 5ef3a20e..017a4400 100644
--- a/internal/cli/tools_audit.go
+++ b/internal/cli/tools_audit.go
@@ -278,11 +278,16 @@ func auditMCPManifest(m *pipeline.ToolsManifest) []ToolsAuditFinding {
}
type commandFields struct {
- use string
- short string
- hasReadOnly bool
- hasEndpoint bool
- hasRunE bool // true when the literal declares Run or RunE; parent groupers omit both
+ use string
+ short string
+ // hasExplicitReadOnly is true when the mcp:read-only annotation is
+ // present with ANY value (including "false"). The audit's
+ // missing-read-only finding fires only when the annotation is
+ // genuinely absent — an author who has explicitly opted out by
+ // writing "false" has done the right thing and shouldn't be flagged.
+ hasExplicitReadOnly bool
+ hasEndpoint bool
+ hasRunE bool // true when the literal declares Run or RunE; parent groupers omit both
}
func isCobraCommandType(expr ast.Expr) bool {
@@ -319,7 +324,7 @@ func extractCommandFields(lit *ast.CompositeLit) commandFields {
case "Short":
f.short = stringLit(kv.Value)
case "Annotations":
- f.hasReadOnly, f.hasEndpoint = inspectAnnotations(kv.Value)
+ f.hasExplicitReadOnly, f.hasEndpoint = inspectAnnotations(kv.Value)
case "Run", "RunE":
f.hasRunE = true
}
@@ -338,7 +343,16 @@ func stringLit(e ast.Expr) string {
return bl.Value
}
-func inspectAnnotations(e ast.Expr) (hasReadOnly, hasEndpoint bool) {
+// inspectAnnotations returns whether the literal declares the named
+// annotations at all (regardless of value). hasExplicitReadOnly is true
+// when `mcp:read-only` is present with ANY value — including "false",
+// which is the author's explicit opt-out for a read-shaped name that
+// actually mutates state. Collapsing "false" and absent into the same
+// signal makes the audit's missing-read-only finding fire on commands
+// the author already classified correctly; the audit's job is to flag
+// unannotated commands, not to enforce that every read-shaped name be
+// read-only.
+func inspectAnnotations(e ast.Expr) (hasExplicitReadOnly, hasEndpoint bool) {
lit, ok := e.(*ast.CompositeLit)
if !ok {
return false, false
@@ -350,12 +364,12 @@ func inspectAnnotations(e ast.Expr) (hasReadOnly, hasEndpoint bool) {
}
switch stringLit(kv.Key) {
case "mcp:read-only":
- hasReadOnly = stringLit(kv.Value) == "true"
+ hasExplicitReadOnly = true
case "pp:endpoint":
hasEndpoint = stringLit(kv.Value) != ""
}
}
- return hasReadOnly, hasEndpoint
+ return hasExplicitReadOnly, hasEndpoint
}
func auditCommandFields(file string, line int, f commandFields) []ToolsAuditFinding {
@@ -398,7 +412,7 @@ func auditCommandFields(file string, line int, f commandFields) []ToolsAuditFind
// missing-read-only applies only to commands that become shell-out
// MCP tools (typed endpoint tools get classification from the spec
// method; framework commands don't register at all).
- if isShellOut && !f.hasReadOnly && readShapedName(name) {
+ if isShellOut && !f.hasExplicitReadOnly && readShapedName(name) {
out = append(out, ToolsAuditFinding{
Kind: kindMissingReadOnly, Command: name, File: file, Line: line,
Evidence: "name matches read heuristic; no mcp:read-only annotation",
diff --git a/internal/cli/tools_audit_test.go b/internal/cli/tools_audit_test.go
index d440d5af..b20768a8 100644
--- a/internal/cli/tools_audit_test.go
+++ b/internal/cli/tools_audit_test.go
@@ -2,6 +2,9 @@ package cli
import (
"bytes"
+ "go/ast"
+ "go/parser"
+ "go/token"
"strings"
"testing"
"time"
@@ -459,3 +462,117 @@ func TestTruncate(t *testing.T) {
})
}
}
+
+// TestAuditCommandFieldsExplicitReadOnly pins #891 at the
+// auditCommandFields layer: the missing-read-only finding fires only
+// when the annotation is genuinely absent. The AST-level differentiation
+// between explicit-true and explicit-false collapses to the same
+// hasExplicitReadOnly=true signal by the time it reaches this function,
+// so a single "present" case suffices here; the explicit-false vs
+// explicit-true distinction is covered by TestInspectAnnotationsExplicitReadOnlyFalse.
+func TestAuditCommandFieldsExplicitReadOnly(t *testing.T) {
+ cases := []struct {
+ name string
+ fields commandFields
+ wantMissingReadOnly bool
+ }{
+ {
+ name: "explicit annotation (any value) suppresses missing-read-only",
+ fields: commandFields{
+ use: "report",
+ short: "Generate a report",
+ hasExplicitReadOnly: true,
+ hasRunE: true,
+ },
+ wantMissingReadOnly: false,
+ },
+ {
+ name: "absent annotation fires the finding",
+ fields: commandFields{
+ use: "report",
+ short: "Generate a report",
+ hasExplicitReadOnly: false,
+ hasRunE: true,
+ },
+ wantMissingReadOnly: true,
+ },
+ {
+ name: "non-read-shaped names never fire regardless of annotation",
+ fields: commandFields{
+ use: "post",
+ short: "Post a message",
+ hasExplicitReadOnly: false,
+ hasRunE: true,
+ },
+ wantMissingReadOnly: false,
+ },
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ findings := auditCommandFields("internal/cli/x.go", 1, tc.fields)
+ var got bool
+ for _, f := range findings {
+ if f.Kind == kindMissingReadOnly {
+ got = true
+ break
+ }
+ }
+ if got != tc.wantMissingReadOnly {
+ t.Fatalf("missing-read-only fired = %v, want %v (findings: %+v)", got, tc.wantMissingReadOnly, findings)
+ }
+ })
+ }
+}
+
+// TestInspectAnnotationsExplicitReadOnlyFalse pins the AST-level
+// helper: any value for `mcp:read-only` — including "false" — sets
+// hasExplicitReadOnly. The old behavior treated "false" as absent.
+func TestInspectAnnotationsExplicitReadOnlyFalse(t *testing.T) {
+ cases := []struct {
+ name string
+ src string
+ wantSetRO bool
+ }{
+ {
+ name: "explicit true",
+ src: `package x; var _ = map[string]string{"mcp:read-only": "true"}`,
+ wantSetRO: true,
+ },
+ {
+ name: "explicit false",
+ src: `package x; var _ = map[string]string{"mcp:read-only": "false"}`,
+ wantSetRO: true,
+ },
+ {
+ name: "absent",
+ src: `package x; var _ = map[string]string{}`,
+ wantSetRO: false,
+ },
+ }
+ for _, tc := range cases {
+ t.Run(tc.name, func(t *testing.T) {
+ fset := token.NewFileSet()
+ file, err := parser.ParseFile(fset, "x.go", tc.src, 0)
+ if err != nil {
+ t.Fatalf("parse: %v", err)
+ }
+ var lit *ast.CompositeLit
+ ast.Inspect(file, func(n ast.Node) bool {
+ if c, ok := n.(*ast.CompositeLit); ok {
+ if _, ok := c.Type.(*ast.MapType); ok {
+ lit = c
+ return false
+ }
+ }
+ return true
+ })
+ if lit == nil {
+ t.Fatalf("no map literal found")
+ }
+ gotRO, _ := inspectAnnotations(lit)
+ if gotRO != tc.wantSetRO {
+ t.Errorf("hasExplicitReadOnly = %v, want %v", gotRO, tc.wantSetRO)
+ }
+ })
+ }
+}
← f354bc7a docs(skills): document hierarchical resource_type in store-q
·
back to Cli Printing Press
·
chore(main): release 4.7.0 (#1377) e8eb4e30 →