fix(mcp): address PR review — warnings, tests, digest docs

Return MCP discovery warnings from ApplyMCPSafetyDiscovery and surface them
on validate (table + JSON). Use per-tool timeouts instead of a shared budget.
Add config.Resolve integration tests, digest drift/stability coverage, named
MCP meta key constants, and CHANGELOG guidance to pin spec.safety for stable
plan→run digests when relying on MCP flags.

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: leo-aa88

CHANGELOG.md

@@ -21,7 +21,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
 - **`spec.safety` on Tool resources** (issue #103): optional `trusted`, `sideEffects`, and `requiresApproval` fields. [NormalizeProjectGraph] materializes fail-closed defaults on load.
 - **Policy safety fallback**: when no `approvals.requiredFor` entry matches the exact `uses` string, [policy.Derive] consults resolved safety metadata. Unattended mutating tools require `--approve` (exit code **5**, `approval_required`).
 - **Plan risk hints** for tools that will require approval at run, including decision source (`explicit_policy_rule`, `safety_metadata`, `fail_closed_default`).
-- **MCP tool safety discovery** (issue #125): during config resolution, MCP `tools/list` descriptors supply `meta.mcp_flags` (`trusted`, `side_effects`, `requires_approval`) merged into `spec.safety` via [spec.SafetyFromMCPMeta] and [spec.MergeToolSafety]. Author-set YAML fields override MCP per field; discovery failures fall back to fail-closed defaults.
+- **MCP tool safety discovery** (issue #125): during config resolution, MCP `tools/list` descriptors supply `meta.mcp_flags` (`trusted`, `side_effects`, `requires_approval`) merged into `spec.safety` via [spec.SafetyFromMCPMeta] and [spec.MergeToolSafety]. Author-set YAML fields override MCP per field; discovery failures fall back to fail-closed defaults and emit validate-time warnings on [config.ResolvedConfig.MCPDiscoveryWarnings].
 
 ### Changed
 
@@ -40,3 +40,4 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/).
    ```
 2. For tools where you accept **tool-wide** unattended use but still gate specific operations, set `trusted: true` and list write operations under `Policy.spec.approvals.requiredFor` (exact `uses` strings).
 3. Do **not** set `trusted: true` unless you intend every operation on that tool to run without safety-derived approval; per-action gating remains `requiredFor` only (exact match at runtime).
+4. For **MCP tools** that rely on server-provided `meta.mcp_flags`, pin explicit `spec.safety` in YAML when you need stable `validate`/`plan`→`run` digests. Resolved-config digests include normalized tool safety; if MCP `tools/list` fails at `plan` time but succeeds at `run` time (or vice versa), effective safety and the digest can change even when project YAML is unchanged (exit **3** drift). `agentctl validate` surfaces non-fatal MCP discovery warnings when listing fails.

internal/cli/validate.go

@@ -6,6 +6,7 @@ import (
 	"github.com/LAA-Software-Engineering/agentic-control-plane/internal/policy"
 	"github.com/LAA-Software-Engineering/agentic-control-plane/internal/render"
 	"github.com/LAA-Software-Engineering/agentic-control-plane/internal/spec"
+	"github.com/LAA-Software-Engineering/agentic-control-plane/internal/tools"
 	"github.com/spf13/cobra"
 )
 
@@ -46,7 +47,7 @@ func runValidate(cmd *cobra.Command, args []string, strict bool) error {
 		}
 		return NewExitError(ExitValidationError, fmt.Errorf("validation failed: high-severity policy lint findings (--strict)"))
 	}
-	if err := writeValidateSuccess(cmd, graph, g, findings); err != nil {
+	if err := writeValidateSuccess(cmd, graph, g, findings, rc.MCPDiscoveryWarnings()); err != nil {
 		return err
 	}
 	if err := persistSnapshots(rc); err != nil {
@@ -55,7 +56,7 @@ func runValidate(cmd *cobra.Command, args []string, strict bool) error {
 	return nil
 }
 
-func writeValidateSuccess(cmd *cobra.Command, graph *spec.ProjectGraph, g *Global, findings []policy.LintFinding) error {
+func writeValidateSuccess(cmd *cobra.Command, graph *spec.ProjectGraph, g *Global, findings []policy.LintFinding, mcpWarnings []tools.MCPDiscoveryWarning) error {
 	out := cmd.OutOrStdout()
 	envLabel := g.Env
 	if envLabel == "" {
@@ -70,19 +71,21 @@ func writeValidateSuccess(cmd *cobra.Command, graph *spec.ProjectGraph, g *Globa
 	switch g.Output {
 	case render.FormatJSON:
 		payload := struct {
-			Project       string               `json:"project"`
-			Environment   string               `json:"environment"`
-			ResourceCount int                  `json:"resourceCount"`
-			Valid         bool                 `json:"valid"`
-			Message       string               `json:"message"`
-			PolicyLint    []policy.LintFinding `json:"policyLint,omitempty"`
+			Project              string                      `json:"project"`
+			Environment          string                      `json:"environment"`
+			ResourceCount        int                         `json:"resourceCount"`
+			Valid                bool                        `json:"valid"`
+			Message              string                      `json:"message"`
+			PolicyLint           []policy.LintFinding        `json:"policyLint,omitempty"`
+			MCPDiscoveryWarnings []tools.MCPDiscoveryWarning `json:"mcpDiscoveryWarnings,omitempty"`
 		}{
-			Project:       projName,
-			Environment:   envLabel,
-			ResourceCount: n,
-			Valid:         true,
-			Message:       "Validation successful",
-			PolicyLint:    findingsOrNil(findings),
+			Project:              projName,
+			Environment:          envLabel,
+			ResourceCount:        n,
+			Valid:                true,
+			Message:              "Validation successful",
+			PolicyLint:           findingsOrNil(findings),
+			MCPDiscoveryWarnings: mcpWarningsOrNil(mcpWarnings),
 		}
 		return render.WriteJSON(out, payload)
 	case render.FormatYAML:
@@ -96,6 +99,9 @@ func writeValidateSuccess(cmd *cobra.Command, graph *spec.ProjectGraph, g *Globa
 		if len(findings) > 0 {
 			body["policyLint"] = findings
 		}
+		if len(mcpWarnings) > 0 {
+			body["mcpDiscoveryWarnings"] = mcpWarnings
+		}
 		return render.WriteYAML(out, body)
 	default:
 		p := passPrefix(g)
@@ -117,6 +123,9 @@ func writeValidateSuccess(cmd *cobra.Command, graph *spec.ProjectGraph, g *Globa
 		if err := writeValidateLintTable(out, g, findings); err != nil {
 			return err
 		}
+		if err := writeValidateMCPDiscoveryWarnings(out, g, mcpWarnings); err != nil {
+			return err
+		}
 		_, err := fmt.Fprintf(out, "\nValidation successful\n")
 		return err
 	}
@@ -200,6 +209,32 @@ func findingsOrNil(findings []policy.LintFinding) []policy.LintFinding {
 	return findings
 }
 
+func mcpWarningsOrNil(warnings []tools.MCPDiscoveryWarning) []tools.MCPDiscoveryWarning {
+	if len(warnings) == 0 {
+		return nil
+	}
+	return warnings
+}
+
+func writeValidateMCPDiscoveryWarnings(out fmtWriter, g *Global, warnings []tools.MCPDiscoveryWarning) error {
+	if len(warnings) == 0 {
+		return nil
+	}
+	mark := "⚠"
+	if g != nil && g.NoColor {
+		mark = "*"
+	}
+	if _, err := fmt.Fprintf(out, "\nMCP discovery (%d warnings):\n", len(warnings)); err != nil {
+		return err
+	}
+	for _, w := range warnings {
+		if _, err := fmt.Fprintf(out, "%s %s\n", mark, tools.FormatMCPDiscoveryWarning(w)); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 func resourceCount(g *spec.ProjectGraph) int {
 	if g == nil {
 		return 0

internal/cli/validate_test.go

@@ -203,3 +203,57 @@ func TestValidate_validateOk_strictPasses(t *testing.T) {
 		t.Fatalf("validate_ok should pass strict lint: %v", err)
 	}
 }
+
+func TestValidate_mcpDiscoveryWarning_advisory(t *testing.T) {
+	root := t.TempDir()
+	writeFile(t, filepath.Join(root, "project.yaml"), `apiVersion: agentic.dev/v0
+kind: Project
+metadata:
+  name: mcp-warn
+spec:
+  imports:
+    - tools/
+  state:
+    backend: sqlite
+    dsn: .agentic/state.db
+`)
+	writeFile(t, filepath.Join(root, "tools", "mc.yaml"), `apiVersion: agentic.dev/v0
+kind: Tool
+metadata:
+  name: mc
+spec:
+  type: mcp
+  mcp:
+    transport: stdio
+    command: /nonexistent/mcp-binary-for-validate-test
+`)
+
+	ResetGlobalsForTest()
+	cmd := NewRootCmd()
+	var out bytes.Buffer
+	cmd.SetOut(&out)
+	cmd.SetErr(&out)
+	cmd.SetArgs([]string{"-o", "json", "validate", "--project", root, "--no-color"})
+	if err := cmd.Execute(); err != nil {
+		t.Fatal(err)
+	}
+	var payload struct {
+		Valid                bool `json:"valid"`
+		MCPDiscoveryWarnings []struct {
+			Tool    string `json:"tool"`
+			Message string `json:"message"`
+		} `json:"mcpDiscoveryWarnings"`
+	}
+	if err := json.Unmarshal(out.Bytes(), &payload); err != nil {
+		t.Fatal(err)
+	}
+	if !payload.Valid {
+		t.Fatalf("validate should succeed: %s", out.String())
+	}
+	if len(payload.MCPDiscoveryWarnings) != 1 {
+		t.Fatalf("expected MCP discovery warning: %s", out.String())
+	}
+	if payload.MCPDiscoveryWarnings[0].Tool != "mc" {
+		t.Fatalf("tool: %+v", payload.MCPDiscoveryWarnings[0])
+	}
+}

internal/config/resolved.go

@@ -10,7 +10,6 @@ import (
 	"os"
 	"path/filepath"
 	"strings"
-	"time"
 
 	"github.com/LAA-Software-Engineering/agentic-control-plane/internal/plan"
 	"github.com/LAA-Software-Engineering/agentic-control-plane/internal/project"
@@ -29,9 +28,6 @@ const DefaultStateDSN = ".agentic/state.db"
 
 const resolvedSnapshotRel = ".agentic/resolved-config.json"
 
-// mcpDiscoveryTimeout bounds MCP tools/list during config resolution; failures fall back to fail-closed defaults.
-const mcpDiscoveryTimeout = 10 * time.Second
-
 // ResolveOptions selects inputs for the configuration pipeline.
 type ResolveOptions struct {
 	ProjectRoot string
@@ -43,11 +39,12 @@ type ResolveOptions struct {
 // ResolvedConfig is a frozen snapshot of the fully resolved project configuration.
 // Graph returns a defensive copy; treat it as read-only.
 type ResolvedConfig struct {
-	graph     *spec.ProjectGraph
-	root      string
-	env       string
-	statePath string
-	digest    string
+	graph       *spec.ProjectGraph
+	root        string
+	env         string
+	statePath   string
+	digest      string
+	mcpWarnings []tools.MCPDiscoveryWarning
 }
 
 // Graph returns a defensive copy of the resolved, validated project graph.
@@ -94,6 +91,16 @@ func (r *ResolvedConfig) Digest() string {
 	return r.digest
 }
 
+// MCPDiscoveryWarnings returns non-fatal MCP tools/list failures from the last resolve pass.
+func (r *ResolvedConfig) MCPDiscoveryWarnings() []tools.MCPDiscoveryWarning {
+	if r == nil || len(r.mcpWarnings) == 0 {
+		return nil
+	}
+	out := make([]tools.MCPDiscoveryWarning, len(r.mcpWarnings))
+	copy(out, r.mcpWarnings)
+	return out
+}
+
 // Resolve loads, merges, normalizes, overlays, validates, and fingerprints the effective config.
 func Resolve(opts ResolveOptions) (*ResolvedConfig, error) {
 	root, err := filepath.Abs(filepath.Clean(opts.ProjectRoot))
@@ -113,9 +120,7 @@ func Resolve(opts ResolveOptions) (*ResolvedConfig, error) {
 
 	ApplyUserLocalUnder(&graph.Spec, userLocal)
 
-	ctx, cancel := context.WithTimeout(context.Background(), mcpDiscoveryTimeout)
-	defer cancel()
-	tools.ApplyMCPSafetyDiscovery(ctx, graph)
+	mcpWarnings := tools.ApplyMCPSafetyDiscovery(context.Background(), graph)
 	spec.NormalizeProjectGraph(graph)
 
 	graph, err = spec.ApplyEnvironment(graph, opts.Env)
@@ -144,11 +149,12 @@ func Resolve(opts ResolveOptions) (*ResolvedConfig, error) {
 	}
 
 	return &ResolvedConfig{
-		graph:     frozen,
-		root:      root,
-		env:       env,
-		statePath: statePath,
-		digest:    digest,
+		graph:       frozen,
+		root:        root,
+		env:         env,
+		statePath:   statePath,
+		digest:      digest,
+		mcpWarnings: mcpWarnings,
 	}, nil
 }