diff --git a/.gitattributes b/.gitattributes index 162d896c..aa9ee195 100644 --- a/.gitattributes +++ b/.gitattributes @@ -19,6 +19,12 @@ internal/server/testdata/*.golden.json text eol=lf # comparisons fail with a trailing \r. internal/server/testdata/**/*.golden.json text eol=lf +# Spec 098 tools/list merge-base snapshots (FR-015 byte-identity): these are +# plain .json (not *.golden.json), so the patterns above do not reach them. +# Without this Windows checks out CRLF and all three +# TestToolsListSnapshot_MatchesMergeBaseGoldens surfaces fail on \r alone. +internal/server/testdata/toolslist_goldens/*.json text eol=lf + # Self-contained verification/QA reports embed base64 PNG screenshots, so a # single file is multiple MB of "HTML". They are point-in-time artifacts, not # source. Mark them linguist-generated so GitHub's language stats reflect the diff --git a/CLAUDE.md b/CLAUDE.md index d4dc9785..a8311558 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -158,6 +158,6 @@ tail -f ~/Library/Logs/mcpproxy/main.log # main log (macOS; Linux: ~/.mcpproxy/ - **Windows installer**: [docs/github-actions-windows-wix-research.md](docs/github-actions-windows-wix-research.md). **Prerelease** (`next` branch + `v*-rc.*` tags, opt-in, off stable channels): [docs/prerelease-builds.md](docs/prerelease-builds.md). ## Recent Changes +- 098-tools-preflight: Added Go 1.24 module toolchain (repo builds with local Go 1.25) + existing only — chi (httpapi), bbolt (storage), Bleve (index), zap (logging), Cobra (CLI), swaggo/swag v2 (contract regen). **No new dependencies.** - 097-stored-scripts: Added Go 1.25 (os.Root/Root.ReadFile available — R1) + stdlib only (os.Root). **No new dependencies.** - 096-batched-call-tools: Added Go 1.24 module toolchain (repo builds with local Go 1.25) + existing only — goja (sandbox), mark3labs/mcp-go (tool surface), zap. **No new dependencies.** -- 095-update-failure-ux: Added Swift 5.9 (tray, AppKit + Sparkle 2.9.3 vendored via SwiftPM) · Go 1.24 module toolchain (repo builds with local Go 1.25) + existing only — Sparkle 2.9.3 (`SPUUpdater`, `SPUStandardUserDriver`), chi (httpapi), bbolt (diagnostics counters), swaggo/swag v2 (contract regen). **No new dependencies.** diff --git a/README.md b/README.md index a2031ea5..41eb3916 100644 --- a/README.md +++ b/README.md @@ -181,6 +181,72 @@ See [Configuration](https://docs.mcpproxy.app/configuration/config-file/) and [U --- +## How AI Agents Work Through MCPProxy + +Once connected, your agent sees a handful of built-in MCPProxy tools instead of hundreds of upstream schemas. A typical session has three beats — discover, call, audit — plus an optional preflight gate for unattended automations. + +### 1. Discover — spend one query, not your context window + +The agent asks for what it needs in plain keywords via `retrieve_tools`: + +```json +{ "query": "create github issue", "limit": 5 } +``` + +MCPProxy runs a BM25 search across every connected server and returns only the top-ranked matches — each with a `call_with` hint recommending the right call variant for its annotations: + +```json +{ + "tools": [ + { "name": "github:create_issue", "score": 0.89, "call_with": "call_tool_write" }, + { "name": "gitlab:create_issue", "score": 0.72, "call_with": "call_tool_write" } + ] +} +``` + +This is where the token savings come from: the schemas of the hundreds of tools the agent *didn't* need never enter its context. The agent loads full schemas on demand with `describe_tool` (batch up to 5 ids) only for the tools it's about to use. + +### 2. Call — with declared intent + +The agent executes the tool through the variant matching its intent (`call_tool_read`, `call_tool_write`, or `call_tool_destructive`), addressing it as `server:tool`: + +```json +{ + "name": "github:create_issue", + "args_json": "{\"repo\": \"acme/api\", \"title\": \"Bug report\"}", + "intent": { "operation_type": "write", "reason": "Filing bug per user request" } +} +``` + +MCPProxy validates the intent against the tool's annotations (a "read" call can't reach a destructive tool), checks quarantine and approval state, and scans arguments and responses for sensitive data before anything leaves the machine. + +### 3. Audit — every call is on the record + +Every call lands in the local [Activity Log](https://docs.mcpproxy.app/features/activity-log/) with a request ID, so you can reconstruct exactly what an agent did: + +```bash +mcpproxy activity list # everything, newest first +mcpproxy activity list --request-id # one workflow, correlated +``` + +### Gate automations before they burn tokens + +For recurring headless jobs (cron, CI, n8n), don't let the agent discover a missing tool the expensive way. One preflight command checks that every required tool is ready — without contacting any upstream server — and reports exactly why when it isn't (server quarantined, tool changed since approval, OAuth expired, typo'd id): + +```bash +mcpproxy tools preflight gh-ops:sync_issues slack:post_message --wait 10s +case $? in + 0) run-agent-session ;; # all ready — go + 10) exit 75 ;; # transient (server starting) — let the next cron tick retry + 11) page-operator ;; # blocked — someone must approve / enable / log in + 12) fail-pipeline ;; # unknown tool id — the automation itself is misconfigured +esac +``` + +See [Required-Tools Preflight](https://docs.mcpproxy.app/features/tools-preflight/) for the full reason taxonomy, REST endpoint, and GitHub Actions / n8n recipes. + +--- + ## 🔐 Optional HTTPS Setup MCPProxy works with HTTP by default for easy setup. HTTPS is optional and primarily useful for production environments or when stricter security is required. @@ -295,6 +361,7 @@ curl -k https://localhost:8080/api/v1/status - [OAuth Authentication](https://docs.mcpproxy.app/features/oauth-authentication/) - [Code Execution](https://docs.mcpproxy.app/features/code-execution/) - [Activity Log](https://docs.mcpproxy.app/features/activity-log/) +- [Required-Tools Preflight](https://docs.mcpproxy.app/features/tools-preflight/) - [Agent Tokens](https://docs.mcpproxy.app/features/agent-tokens/) - [Sensitive Data Detection](https://docs.mcpproxy.app/features/sensitive-data-detection/) diff --git a/ROADMAP.md b/ROADMAP.md index 5c32426f..2af9533f 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -799,3 +799,4 @@ Legend: `shipped` ≥95% checked · `in-flight` 1–94% · `drafted` 0% · `—` | [095-update-failure-ux](./specs/095-update-failure-ux/) | `shipped` | 28/28 (100%) | | [096-batched-call-tools](./specs/096-batched-call-tools/) | `in-flight` | 15/16 (94%) | | [097-stored-scripts](./specs/097-stored-scripts/) | `in-flight` | 13/14 (93%) | +| [098-tools-preflight](./specs/098-tools-preflight/) | `drafted` | 0/33 (0%) | diff --git a/cmd/generate-types/main.go b/cmd/generate-types/main.go index 47453619..203b59fe 100644 --- a/cmd/generate-types/main.go +++ b/cmd/generate-types/main.go @@ -114,6 +114,90 @@ export type RejectionReason = 'queue_full' | 'queue_timeout'; /** Limiter tier that shed the call (activity metadata rejection_scope). */ export type RejectionScope = 'server' | 'global'; +`) + + // Required-tools preflight (Spec 098) - generated from internal/contracts/types.go, + // which mirrors internal/preflight/reasons.go (the single source of truth for + // the taxonomy). A drift test in internal/preflight keeps the two identical. + sb.WriteString(`// Preflight (Spec 098) - generated from internal/contracts/types.go +export const PreflightStatusReady = 'ready' as const; +export const PreflightStatusUnavailable = 'unavailable' as const; +export type PreflightStatus = typeof PreflightStatusReady | typeof PreflightStatusUnavailable; + +/** + * Closed 15-code failure enum. Additive-only: treat an unknown code as + * non-retryable. 'server_saturated' is reserved and not emitted. + */ +export type PreflightReason = + | 'server_initializing' + | 'server_unhealthy' + | 'server_disabled' + | 'server_quarantined' + | 'tool_pending_approval' + | 'tool_changed' + | 'tool_blocked_by_user' + | 'oauth_required' + | 'hash_mismatch' + | 'server_not_in_scope' + | 'tool_denied_by_config' + | 'missing_annotation' + | 'policy_filtered' + | 'not_found' + | 'server_not_configured'; + +/** Set-level aggregate (worst class present); drives the CLI exit code 0/10/11/12. */ +export const PreflightVerdictReady = 'ready' as const; +export const PreflightVerdictDegradedRetryable = 'degraded_retryable' as const; +export const PreflightVerdictBlocked = 'blocked' as const; +export const PreflightVerdictUnknownIds = 'unknown_ids' as const; +export type PreflightVerdict = + | typeof PreflightVerdictReady + | typeof PreflightVerdictDegradedRetryable + | typeof PreflightVerdictBlocked + | typeof PreflightVerdictUnknownIds; + +export interface PreflightToolRef { + id: string; + /** "sha256/v{N}:{hex}" - the schema version distinguishes a proxy hash bump from upstream drift. */ + pin_hash?: string; +} + +export interface PreflightPolicy { + read_only_only?: boolean; + exclude_destructive?: boolean; + exclude_open_world?: boolean; +} + +export interface PreflightRequest { + tools: PreflightToolRef[]; + profile?: string; + policy?: PreflightPolicy; + wait_ms?: number; +} + +export interface PreflightToolResult { + id: string; + status: PreflightStatus; + /** Present only when status is 'unavailable'. */ + reason?: PreflightReason; + retryable?: boolean; + /** Health-action vocabulary; omitted (not 'none') when the reason has no action. */ + action?: HealthAction; + detail?: string; + remediation?: string; + /** Operator tier + ready results only; never disclosed to an agent token. */ + hash?: string; + /** Up to 3 nearest caller-visible ids, on not_found only. */ + did_you_mean?: string[]; +} + +export interface PreflightResponse { + verdict: PreflightVerdict; + checked_at: string; // RFC3339 + waited_ms?: number; + tools: PreflightToolResult[]; +} + `) // Server types @@ -241,6 +325,10 @@ export interface IsolationDefaults { held_reason?: string; held_verdict?: string; held_signals?: string[]; + // The tool's current hash in the preflight pin format "sha256/v{N}:{hex}" + // (spec 098 FR-011) — the value to paste into a preflight pin. Operator tier + // only: absent for agent-token callers and for tools with no stored hash. + hash?: string; } export interface SearchResult { diff --git a/cmd/mcpproxy/activity_cmd.go b/cmd/mcpproxy/activity_cmd.go index ed30dacf..8afa879b 100644 --- a/cmd/mcpproxy/activity_cmd.go +++ b/cmd/mcpproxy/activity_cmd.go @@ -11,6 +11,7 @@ import ( "net/url" "os" "os/signal" + "sort" "strings" "syscall" "time" @@ -23,6 +24,7 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" "github.com/smart-mcp-proxy/mcpproxy-go/internal/logs" "github.com/smart-mcp-proxy/mcpproxy-go/internal/socket" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" ) // Activity command flags @@ -85,6 +87,7 @@ func (f *ActivityFilter) Validate() error { validTypes := []string{ "tool_call", "policy_decision", "quarantine_change", "server_change", "system_start", "system_stop", "internal_tool_call", "config_change", // Spec 024: new types + string(storage.ActivityTypePreflight), // Spec 098: required-tools preflight } // Split by comma for multi-type support types := strings.Split(f.Type, ",") @@ -556,6 +559,219 @@ func displaySensitiveDataSection(activity map[string]interface{}) { } } +// --- Spec 098: preflight activity records ------------------------------------ +// +// A preflight record is set-scoped, not server-scoped: server_name and +// tool_name are empty and everything an operator wants to see lives in +// Metadata ({verdict, ids_count, reasons{code:count}, per_tool[{id,status, +// reason?}]}, written by runtime.ActivityService.RecordPreflight). Without the +// renderers below, `activity list` shows a bare row with three empty columns +// and `activity show` shows nothing at all — the FR-014 transparency promise +// only holds if the record is actually readable. +// +// The metadata arrives here as decoded JSON, so counts are float64 and the +// nested payloads are []interface{} / map[string]interface{}; every accessor +// below tolerates both that and the native Go shape used in tests. + +// maxPreflightSummaryReasons caps how many distinct reason codes the one-line +// summary names before it collapses the tail into "+N more". A preflight may +// carry up to 100 ids across 15 reason codes; an uncapped rollup would push +// every other column of `activity list` off screen. +const maxPreflightSummaryReasons = 3 + +// maxPreflightSummaryCell bounds the summary in the `activity list` TOOL +// column, matching the cap the tool tables already use for their widest cell. +const maxPreflightSummaryCell = 60 + +// isPreflightActivity reports whether a record is a Spec 098 preflight. +func isPreflightActivity(activity map[string]interface{}) bool { + return getStringField(activity, "type") == string(storage.ActivityTypePreflight) +} + +// preflightReasonCount is one {reason code, count} pair of the metadata rollup. +type preflightReasonCount struct { + Reason string + Count int +} + +// preflightReasonRollup reads metadata["reasons"] into a DETERMINISTIC order: +// most frequent first, ties broken alphabetically. Map iteration order would +// otherwise make the same record render differently on every invocation, which +// breaks both diffing two runs and any test that asserts the line. +func preflightReasonRollup(metadata map[string]interface{}) []preflightReasonCount { + counts := map[string]int{} + for reason, raw := range getMapField(metadata, storage.MetadataKeyPreflightReasons) { + if count, ok := numericMetadataValue(raw); ok { + counts[reason] = count + } + } + + // Fallback for a record whose rollup is missing: recount from the per-tool + // detail, which carries the same codes. + if len(counts) == 0 { + for _, entry := range getArrayField(metadata, storage.MetadataKeyPreflightPerTool) { + tool, ok := entry.(map[string]interface{}) + if !ok { + continue + } + if reason := getStringField(tool, storage.PreflightPerToolKeyReason); reason != "" { + counts[reason]++ + } + } + } + if len(counts) == 0 { + return nil + } + + rollup := make([]preflightReasonCount, 0, len(counts)) + for reason, count := range counts { + rollup = append(rollup, preflightReasonCount{Reason: reason, Count: count}) + } + sort.Slice(rollup, func(i, j int) bool { + if rollup[i].Count != rollup[j].Count { + return rollup[i].Count > rollup[j].Count + } + return rollup[i].Reason < rollup[j].Reason + }) + return rollup +} + +// formatPreflightReasons renders a rollup as "code xN, code xN". limit <= 0 +// means "name them all"; a positive limit collapses the tail into "+N more". +func formatPreflightReasons(rollup []preflightReasonCount, limit int) string { + if len(rollup) == 0 { + return "" + } + + shown := rollup + remaining := 0 + if limit > 0 && len(rollup) > limit { + shown = rollup[:limit] + remaining = len(rollup) - limit + } + + parts := make([]string, 0, len(shown)+1) + for _, entry := range shown { + parts = append(parts, fmt.Sprintf("%s x%d", entry.Reason, entry.Count)) + } + if remaining > 0 { + parts = append(parts, fmt.Sprintf("+%d more", remaining)) + } + return strings.Join(parts, ", ") +} + +// numericMetadataValue reads a metadata count that may have arrived as JSON +// (float64) or as the native int the writer used. +func numericMetadataValue(raw interface{}) (int, bool) { + switch v := raw.(type) { + case float64: + return int(v), true + case int: + return v, true + default: + return 0, false + } +} + +// preflightIDsCount is how many unique tool ids the run evaluated. ids_count is +// authoritative (the writer sets it); per_tool length is the fallback for a +// record written by an older/partial writer. +func preflightIDsCount(metadata map[string]interface{}) int { + if count := getIntField(metadata, storage.MetadataKeyPreflightIDsCount); count > 0 { + return count + } + return len(getArrayField(metadata, storage.MetadataKeyPreflightPerTool)) +} + +// preflightActivitySummary renders the one-line verdict summary shown in the +// `activity list` table, e.g. "blocked (4 tools): server_disabled x2, +// tool_changed x1". Empty string for anything that is not a readable preflight +// record, so the caller falls back to its usual "-" placeholder. +func preflightActivitySummary(activity map[string]interface{}) string { + if !isPreflightActivity(activity) { + return "" + } + metadata := getMapField(activity, "metadata") + if metadata == nil { + return "" + } + verdict := getStringField(metadata, storage.MetadataKeyPreflightVerdict) + if verdict == "" { + return "" + } + + count := preflightIDsCount(metadata) + unit := "tools" + if count == 1 { + unit = "tool" + } + summary := fmt.Sprintf("%s (%d %s)", verdict, count, unit) + + if reasons := formatPreflightReasons(preflightReasonRollup(metadata), maxPreflightSummaryReasons); reasons != "" { + summary += ": " + reasons + } + return summary +} + +// preflightDetailLines builds the `activity show` section for a preflight +// record. It returns lines instead of printing so the rendering is unit-tested +// without capturing stdout. Empty slice ⇒ nothing to render. +func preflightDetailLines(activity map[string]interface{}) []string { + if !isPreflightActivity(activity) { + return nil + } + metadata := getMapField(activity, "metadata") + if metadata == nil { + return nil + } + verdict := getStringField(metadata, storage.MetadataKeyPreflightVerdict) + if verdict == "" { + return nil + } + + lines := []string{ + "", + "Preflight:", + fmt.Sprintf(" Verdict: %s", verdict), + fmt.Sprintf(" Tools Checked: %d", preflightIDsCount(metadata)), + } + // The full rollup here — the detail view has the room the table row lacks. + if reasons := formatPreflightReasons(preflightReasonRollup(metadata), 0); reasons != "" { + lines = append(lines, fmt.Sprintf(" Reasons: %s", reasons)) + } + + perTool := getArrayField(metadata, storage.MetadataKeyPreflightPerTool) + if len(perTool) == 0 { + return lines + } + + lines = append(lines, "", " Tools:") + for i, entry := range perTool { + tool, ok := entry.(map[string]interface{}) + if !ok { + continue + } + // Tool ids are caller-supplied strings that round-trip through the + // activity log; escape them before they reach a tty (same trust + // boundary as `tools list`). + id := sanitizeName(getStringField(tool, storage.PreflightPerToolKeyID)) + status := getStringField(tool, storage.PreflightPerToolKeyStatus) + line := fmt.Sprintf(" [%d] %-40s %s", i+1, id, status) + if reason := getStringField(tool, storage.PreflightPerToolKeyReason); reason != "" { + line += " " + reason + } + lines = append(lines, line) + } + return lines +} + +// displayPreflightSection prints the preflight detail for `activity show`. +func displayPreflightSection(activity map[string]interface{}) { + for _, line := range preflightDetailLines(activity) { + fmt.Println(line) + } +} + // formatSeverityWithColor returns a severity string with visual indicator func formatSeverityWithColor(severity string) string { if activityNoIcons { @@ -734,7 +950,7 @@ func init() { activityCmd.AddCommand(activityExportCmd) // List command flags - activityListCmd.Flags().StringVarP(&activityType, "type", "t", "", "Filter by type (comma-separated for multiple): tool_call, system_start, system_stop, internal_tool_call, config_change, policy_decision, quarantine_change, server_change") + activityListCmd.Flags().StringVarP(&activityType, "type", "t", "", "Filter by type (comma-separated for multiple): tool_call, system_start, system_stop, internal_tool_call, config_change, policy_decision, quarantine_change, server_change, preflight") activityListCmd.Flags().StringVarP(&activityServer, "server", "s", "", "Filter by server name") activityListCmd.Flags().StringVar(&activityTool, "tool", "", "Filter by tool name") activityListCmd.Flags().StringVar(&activityStatus, "status", "", "Filter by status: success, error, blocked, rejected") @@ -755,7 +971,7 @@ func init() { activityListCmd.Flags().StringVar(&activityAuthType, "auth-type", "", "Filter by auth type: admin, agent") // Watch command flags - activityWatchCmd.Flags().StringVarP(&activityType, "type", "t", "", "Filter by type (comma-separated): tool_call, system_start, system_stop, internal_tool_call, config_change, policy_decision, quarantine_change, server_change") + activityWatchCmd.Flags().StringVarP(&activityType, "type", "t", "", "Filter by type (comma-separated): tool_call, system_start, system_stop, internal_tool_call, config_change, policy_decision, quarantine_change, server_change, preflight") activityWatchCmd.Flags().StringVarP(&activityServer, "server", "s", "", "Filter by server name") // Show command flags @@ -771,7 +987,7 @@ func init() { activityExportCmd.Flags().StringVarP(&activityExportFormat, "format", "f", "json", "Export format: json, csv") activityExportCmd.Flags().BoolVar(&activityIncludeBodies, "include-bodies", false, "Include full request/response bodies") // Reuse list filter flags for export - activityExportCmd.Flags().StringVarP(&activityType, "type", "t", "", "Filter by type (comma-separated): tool_call, system_start, system_stop, internal_tool_call, config_change, policy_decision, quarantine_change, server_change") + activityExportCmd.Flags().StringVarP(&activityType, "type", "t", "", "Filter by type (comma-separated): tool_call, system_start, system_stop, internal_tool_call, config_change, policy_decision, quarantine_change, server_change, preflight") activityExportCmd.Flags().StringVarP(&activityServer, "server", "s", "", "Filter by server name") activityExportCmd.Flags().StringVar(&activityTool, "tool", "", "Filter by tool name") activityExportCmd.Flags().StringVar(&activityStatus, "status", "", "Filter by status: success, error, blocked, rejected") @@ -896,6 +1112,15 @@ func runActivityList(cmd *cobra.Command, _ []string) error { durationMs := getIntField(act, "duration_ms") timestamp := getStringField(act, "timestamp") + // Spec 098: a preflight is set-scoped — server_name/tool_name are empty + // by construction, so the TOOL cell carries the verdict summary instead + // of rendering an empty row the operator cannot interpret. + if tool == "" { + if summary := preflightActivitySummary(act); summary != "" { + tool = sanitizeCell(summary, maxPreflightSummaryCell) + } + } + // Extract intent from metadata (Spec 018) intentStr := formatIntentIndicator(act) @@ -1392,6 +1617,13 @@ func runActivityShow(cmd *cobra.Command, args []string) error { fmt.Printf("Session ID: %s\n", sessionID) } + // Spec 098 (SC-005): the request id is how a preflight record is joined to + // the tool calls of the same workflow (`activity list --request-id `), + // so the detail view has to show it, not just accept it as a filter. + if requestID := getStringField(activity, "request_id"); requestID != "" { + fmt.Printf("Request ID: %s\n", requestID) + } + if errMsg := getStringField(activity, "error_message"); errMsg != "" { fmt.Printf("Error: %s\n", errMsg) } @@ -1402,6 +1634,9 @@ func runActivityShow(cmd *cobra.Command, args []string) error { // Sensitive Data Detection (Spec 026) displaySensitiveDataSection(activity) + // Preflight verdict + per-tool reasons (Spec 098) + displayPreflightSection(activity) + // Arguments if args, ok := activity["arguments"].(map[string]interface{}); ok && len(args) > 0 { fmt.Println() diff --git a/cmd/mcpproxy/activity_preflight_test.go b/cmd/mcpproxy/activity_preflight_test.go new file mode 100644 index 00000000..607cbdf3 --- /dev/null +++ b/cmd/mcpproxy/activity_preflight_test.go @@ -0,0 +1,246 @@ +package main + +import ( + "encoding/json" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/runtime" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" +) + +// Spec 098 T022 — `mcpproxy activity list` must accept and render the +// `preflight` activity type (FR-014, US3 acceptance 2). +// +// Every test here feeds the renderer a record that went through the same JSON +// round trip the REST client performs, because that is what turns the stored +// `int` counts into `float64` — the exact shape the CLI actually sees. + +// preflightRecordJSON builds a stored preflight activity record and round-trips +// it through JSON, mirroring GET /api/v1/activity. +func preflightRecordJSON(t *testing.T, rec runtime.PreflightActivity) map[string]interface{} { + t.Helper() + + tools := make([]map[string]interface{}, 0, len(rec.Tools)) + reasons := map[string]int{} + for _, tool := range rec.Tools { + entry := map[string]interface{}{ + storage.PreflightPerToolKeyID: tool.ID, + storage.PreflightPerToolKeyStatus: tool.Status, + } + if tool.Reason != "" { + entry[storage.PreflightPerToolKeyReason] = tool.Reason + reasons[tool.Reason]++ + } + tools = append(tools, entry) + } + + record := map[string]interface{}{ + "id": "01JPREFLIGHT0001", + "type": string(storage.ActivityTypePreflight), + "source": "api", + "status": runtime.PreflightActivityStatus(rec.Verdict), + "request_id": rec.RequestID, + "timestamp": "2026-08-15T10:00:00Z", + "metadata": map[string]interface{}{ + storage.MetadataKeyPreflightVerdict: rec.Verdict, + storage.MetadataKeyPreflightIDsCount: len(rec.Tools), + storage.MetadataKeyPreflightReasons: reasons, + storage.MetadataKeyPreflightPerTool: tools, + }, + } + + encoded, err := json.Marshal(record) + require.NoError(t, err) + var decoded map[string]interface{} + require.NoError(t, json.Unmarshal(encoded, &decoded)) + return decoded +} + +func TestActivityFilter_Validate_AcceptsPreflightType(t *testing.T) { + tests := []struct { + name string + typ string + }{ + {name: "preflight alone", typ: "preflight"}, + {name: "preflight combined with tool_call", typ: "tool_call,preflight"}, + {name: "preflight with surrounding spaces", typ: "preflight, tool_call"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + filter := ActivityFilter{Type: tt.typ} + assert.NoError(t, filter.Validate()) + }) + } +} + +func TestActivityFilter_Validate_StillRejectsUnknownType(t *testing.T) { + filter := ActivityFilter{Type: "preflights"} + err := filter.Validate() + require.Error(t, err) + assert.Contains(t, err.Error(), "invalid type") +} + +func TestPreflightActivitySummary(t *testing.T) { + t.Run("all ready", func(t *testing.T) { + record := preflightRecordJSON(t, runtime.PreflightActivity{ + Verdict: "ready", + Tools: []runtime.PreflightToolOutcome{ + {ID: "ctl:echo", Status: "ready"}, + {ID: "gh:sync", Status: "ready"}, + }, + }) + assert.Equal(t, "ready (2 tools)", preflightActivitySummary(record)) + }) + + t.Run("single tool is not pluralized", func(t *testing.T) { + record := preflightRecordJSON(t, runtime.PreflightActivity{ + Verdict: "ready", + Tools: []runtime.PreflightToolOutcome{{ID: "ctl:echo", Status: "ready"}}, + }) + assert.Equal(t, "ready (1 tool)", preflightActivitySummary(record)) + }) + + t.Run("reasons are rolled up, most frequent first", func(t *testing.T) { + record := preflightRecordJSON(t, runtime.PreflightActivity{ + Verdict: "blocked", + Tools: []runtime.PreflightToolOutcome{ + {ID: "ctl:echo", Status: "ready"}, + {ID: "gh:sync", Status: "unavailable", Reason: "server_disabled"}, + {ID: "gh:close", Status: "unavailable", Reason: "server_disabled"}, + {ID: "slack:post", Status: "unavailable", Reason: "tool_changed"}, + }, + }) + assert.Equal(t, + "blocked (4 tools): server_disabled x2, tool_changed x1", + preflightActivitySummary(record)) + }) + + t.Run("ties break alphabetically for a stable line", func(t *testing.T) { + record := preflightRecordJSON(t, runtime.PreflightActivity{ + Verdict: "unknown_ids", + Tools: []runtime.PreflightToolOutcome{ + {ID: "a:one", Status: "unavailable", Reason: "not_found"}, + {ID: "b:two", Status: "unavailable", Reason: "server_disabled"}, + {ID: "c:three", Status: "unavailable", Reason: "hash_mismatch"}, + }, + }) + summary := preflightActivitySummary(record) + for i := 0; i < 5; i++ { + assert.Equal(t, summary, preflightActivitySummary(record)) + } + assert.Equal(t, + "unknown_ids (3 tools): hash_mismatch x1, not_found x1, server_disabled x1", + summary) + }) + + t.Run("caps the reason list so the table cell stays readable", func(t *testing.T) { + record := preflightRecordJSON(t, runtime.PreflightActivity{ + Verdict: "blocked", + Tools: []runtime.PreflightToolOutcome{ + {ID: "a:1", Status: "unavailable", Reason: "server_disabled"}, + {ID: "b:1", Status: "unavailable", Reason: "tool_changed"}, + {ID: "c:1", Status: "unavailable", Reason: "not_found"}, + {ID: "d:1", Status: "unavailable", Reason: "hash_mismatch"}, + {ID: "e:1", Status: "unavailable", Reason: "oauth_required"}, + }, + }) + summary := preflightActivitySummary(record) + assert.Contains(t, summary, "+2 more") + assert.Equal(t, 3, strings.Count(summary, " x1")) + }) + + t.Run("falls back to per_tool when the rollup is missing", func(t *testing.T) { + record := map[string]interface{}{ + "type": string(storage.ActivityTypePreflight), + "metadata": map[string]interface{}{ + storage.MetadataKeyPreflightVerdict: "unknown_ids", + storage.MetadataKeyPreflightPerTool: []interface{}{ + map[string]interface{}{ + storage.PreflightPerToolKeyID: "a:1", + storage.PreflightPerToolKeyStatus: "unavailable", + storage.PreflightPerToolKeyReason: "not_found", + }, + }, + }, + } + assert.Equal(t, "unknown_ids (1 tool): not_found x1", preflightActivitySummary(record)) + }) + + t.Run("non-preflight records get no summary", func(t *testing.T) { + assert.Equal(t, "", preflightActivitySummary(map[string]interface{}{ + "type": "tool_call", + "metadata": map[string]interface{}{ + storage.MetadataKeyPreflightVerdict: "ready", + }, + })) + }) + + t.Run("a preflight record without metadata degrades to empty", func(t *testing.T) { + assert.Equal(t, "", preflightActivitySummary(map[string]interface{}{ + "type": string(storage.ActivityTypePreflight), + })) + }) +} + +func TestPreflightDetailLines(t *testing.T) { + record := preflightRecordJSON(t, runtime.PreflightActivity{ + RequestID: "req-098", + Verdict: "blocked", + Tools: []runtime.PreflightToolOutcome{ + {ID: "ctl:echo", Status: "ready"}, + {ID: "gh:sync", Status: "unavailable", Reason: "server_disabled"}, + {ID: "slack:post", Status: "unavailable", Reason: "tool_changed"}, + }, + }) + + lines := preflightDetailLines(record) + require.NotEmpty(t, lines) + joined := strings.Join(lines, "\n") + + assert.Contains(t, joined, "Preflight:") + assert.Contains(t, joined, "Verdict:") + assert.Contains(t, joined, "blocked") + assert.Contains(t, joined, "Tools Checked:") + assert.Contains(t, joined, "3") + assert.Contains(t, joined, "server_disabled x1, tool_changed x1") + + // Per-tool detail keeps the request order and names every reason. + assert.Contains(t, joined, "ctl:echo") + assert.Contains(t, joined, "gh:sync") + assert.Contains(t, joined, "slack:post") + assert.Less(t, strings.Index(joined, "gh:sync"), strings.Index(joined, "slack:post")) + + // A ready tool carries no reason. + for _, line := range lines { + if strings.Contains(line, "ctl:echo") { + assert.NotContains(t, line, "server_disabled") + } + } +} + +func TestPreflightDetailLines_NotAPreflightRecord(t *testing.T) { + assert.Empty(t, preflightDetailLines(map[string]interface{}{"type": "tool_call"})) + assert.Empty(t, preflightDetailLines(map[string]interface{}{ + "type": string(storage.ActivityTypePreflight), + })) +} + +// Tool ids are caller-supplied strings; an id carrying an ANSI escape must not +// reach the operator's terminal raw (same trust boundary as `tools list`). +func TestPreflightRenderingSanitizesToolIDs(t *testing.T) { + record := preflightRecordJSON(t, runtime.PreflightActivity{ + Verdict: "unknown_ids", + Tools: []runtime.PreflightToolOutcome{ + {ID: "\x1b[2J\x1b[1;1Hevil:tool", Status: "unavailable", Reason: "not_found"}, + }, + }) + + joined := strings.Join(preflightDetailLines(record), "\n") + assert.NotContains(t, joined, "\x1b[2J") + assert.Contains(t, joined, "evil:tool") +} diff --git a/cmd/mcpproxy/exit_codes.go b/cmd/mcpproxy/exit_codes.go index cb866234..f551f7a9 100644 --- a/cmd/mcpproxy/exit_codes.go +++ b/cmd/mcpproxy/exit_codes.go @@ -1,5 +1,12 @@ package main +import ( + "errors" + "fmt" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/preflight" +) + // Exit codes for mcpproxy to enable specific error handling by the tray launcher const ( @@ -20,4 +27,90 @@ const ( // ExitCodePermissionError indicates insufficient permissions (file access, port binding) ExitCodePermissionError = 5 + + // Spec 098 preflight verdict codes. They are a SEPARATE band from the codes + // above on purpose: 0-5 describe whether mcpproxy could run, 10-12 describe + // what a preflight found, so a cron wrapper can branch retry-vs-page-vs-fix + // on the exit code alone without parsing JSON (SC-003). Their values are the + // spec's, and preflight.ExitCode is the single mapping — these constants + // exist so the CLI can name them, not to re-derive them. + + // ExitCodePreflightDegradedRetryable: every failure is retryable (the proxy + // is mid-transition). The job should back off and retry. + ExitCodePreflightDegradedRetryable = preflight.ExitDegradedRetryable + + // ExitCodePreflightBlocked: at least one tool needs an operator action + // (approve, enable, log in, re-pin). Retrying will not help. + ExitCodePreflightBlocked = preflight.ExitBlocked + + // ExitCodePreflightUnknownIDs: at least one requested id does not exist in + // the caller's view — usually a typo or a removed server. + ExitCodePreflightUnknownIDs = preflight.ExitUnknownIDs ) + +// preflightVerdictError carries a non-ready preflight verdict out of the +// subcommand so the CENTRAL classifier assigns the exit code. +// +// The subcommand deliberately cannot call os.Exit with a code of its own: every +// exit code mcpproxy returns is decided in one place (classifyError), which is +// what keeps 10/11/12 from drifting into meaning something else in a second +// command later. +type preflightVerdictError struct { + verdict string + summary string +} + +func (e *preflightVerdictError) Error() string { + if e.summary == "" { + return fmt.Sprintf("preflight verdict: %s", e.verdict) + } + return fmt.Sprintf("preflight verdict: %s (%s)", e.verdict, e.summary) +} + +// ExitCode is the spec's worst-class-wins mapping, delegated to the evaluator +// package so the table lives once. +func (e *preflightVerdictError) ExitCode() int { + return preflight.ExitCode(e.verdict) +} + +// newPreflightVerdictError returns nil for a ready verdict — a successful +// preflight is not an error — and a typed error otherwise. +func newPreflightVerdictError(verdict, summary string) error { + if verdict == preflight.VerdictReady || verdict == "" { + return nil + } + return &preflightVerdictError{verdict: verdict, summary: summary} +} + +// preflightGeneralError marks a preflight failure that is NOT a verdict — bad +// arguments, an unreachable daemon, a rendering failure. +// +// FR-009 gives those exit code 1, and the type is what guarantees it: without +// it they fall through to classifyError's string heuristics, where the daemon's +// own "failed to load configuration" or a remediation string containing +// "permission denied" would come back as exit 4 or 5. A cron wrapper reads +// those as mcpproxy-level failures, and 4/5 are also inside the band a +// preflight wrapper watches, so the misclassification is silent. +type preflightGeneralError struct { + err error +} + +func (e *preflightGeneralError) Error() string { return e.err.Error() } + +func (e *preflightGeneralError) Unwrap() error { return e.err } + +// ExitCode is always the general error code, whatever the message says. +func (e *preflightGeneralError) ExitCode() int { return ExitCodeGeneralError } + +// newPreflightGeneralError wraps a non-verdict failure, passing nil through and +// leaving an already-typed verdict error alone (the verdict IS the answer). +func newPreflightGeneralError(err error) error { + if err == nil { + return nil + } + var verdictErr *preflightVerdictError + if errors.As(err, &verdictErr) { + return err + } + return &preflightGeneralError{err: err} +} diff --git a/cmd/mcpproxy/main.go b/cmd/mcpproxy/main.go index 34d597bd..400c8e20 100644 --- a/cmd/mcpproxy/main.go +++ b/cmd/mcpproxy/main.go @@ -779,6 +779,24 @@ func classifyError(err error) int { return ExitCodeSuccess } + // Spec 098: a preflight verdict is a RESULT, not a failure of mcpproxy, and + // it carries its own exit code (10/11/12). It is checked first so the + // string-matching heuristics below — "config", "invalid", "denied" all + // appear in remediation text — can never reclassify a verdict as a config + // or permission error. + var preflightErr *preflightVerdictError + if errors.As(err, &preflightErr) { + return preflightErr.ExitCode() + } + + // …and a preflight failure that is NOT a verdict is a plain general error + // (FR-009), for the same reason: the heuristics below read the daemon's + // message, which a transport or argument failure does not control. + var preflightGeneralErr *preflightGeneralError + if errors.As(err, &preflightGeneralErr) { + return preflightGeneralErr.ExitCode() + } + // Check for port conflict errors var portErr *server.PortInUseError if errors.As(err, &portErr) { diff --git a/cmd/mcpproxy/preflight_cmd_test.go b/cmd/mcpproxy/preflight_cmd_test.go new file mode 100644 index 00000000..b2c4f9a8 --- /dev/null +++ b/cmd/mcpproxy/preflight_cmd_test.go @@ -0,0 +1,444 @@ +package main + +import ( + "encoding/json" + "errors" + "fmt" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/yaml.v3" + + "github.com/smart-mcp-proxy/mcpproxy-go/internal/cli/output" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/preflight" +) + +// --- T017: typed exit-code error + central classification ------------------- + +func TestPreflightVerdictError_MapsToSpecExitCodes(t *testing.T) { + tests := []struct { + verdict string + want int + }{ + {preflight.VerdictDegradedRetryable, ExitCodePreflightDegradedRetryable}, + {preflight.VerdictBlocked, ExitCodePreflightBlocked}, + {preflight.VerdictUnknownIDs, ExitCodePreflightUnknownIDs}, + } + for _, tc := range tests { + t.Run(tc.verdict, func(t *testing.T) { + err := newPreflightVerdictError(tc.verdict, "1 of 1 tools unavailable") + require.Error(t, err) + assert.Equal(t, tc.want, classifyError(err), "the CENTRAL classifier owns the exit code") + assert.Contains(t, err.Error(), tc.verdict) + }) + } + + assert.Equal(t, 10, ExitCodePreflightDegradedRetryable) + assert.Equal(t, 11, ExitCodePreflightBlocked) + assert.Equal(t, 12, ExitCodePreflightUnknownIDs) +} + +func TestPreflightVerdictError_ReadyIsNotAnError(t *testing.T) { + assert.NoError(t, newPreflightVerdictError(preflight.VerdictReady, "")) + assert.Equal(t, ExitCodeSuccess, classifyError(nil)) +} + +// The verdict error must survive wrapping — a caller that adds context must not +// silently downgrade the exit code to the generic 1. +func TestPreflightVerdictError_SurvivesWrapping(t *testing.T) { + wrapped := fmt.Errorf("running scheduled job: %w", newPreflightVerdictError(preflight.VerdictBlocked, "")) + assert.Equal(t, ExitCodePreflightBlocked, classifyError(wrapped)) +} + +// Remediation text is full of words the string-matching heuristics look for +// ("configure", "invalid", "denies", "permission"). The verdict check runs +// first precisely so none of them can reclassify a verdict. +func TestPreflightVerdictError_NotReclassifiedByStringHeuristics(t *testing.T) { + err := newPreflightVerdictError(preflight.VerdictBlocked, + "1 of 1 tools unavailable: tool_denied_by_config (invalid configuration, permission denied)") + assert.Equal(t, ExitCodePreflightBlocked, classifyError(err)) +} + +// Transport and argument failures keep the general exit code 1 — they are not +// verdicts and a cron wrapper must be able to tell them apart. +func TestPreflightTransportErrorsUseGeneralExitCode(t *testing.T) { + assert.Equal(t, ExitCodeGeneralError, classifyError(errors.New("mcpproxy daemon is not reachable. Start with: mcpproxy serve"))) +} + +// A non-verdict preflight failure must be exit 1 even when its message is full +// of the words the string heuristics branch on. Untyped, each of these lands in +// the 4/5 band, which a cron wrapper cannot tell apart from a real config or +// permission problem in mcpproxy itself (FR-009). +func TestPreflightGeneralError_NotReclassifiedByStringHeuristics(t *testing.T) { + for _, message := range []string{ + "preflight failed: failed to load configuration", + "preflight failed: invalid configuration in ~/.mcpproxy/mcp_config.json", + "preflight failed: dial unix ~/.mcpproxy/mcpproxy.sock: permission denied", + "preflight failed: operation not permitted", + } { + t.Run(message, func(t *testing.T) { + untyped := errors.New(message) + require.NotEqual(t, ExitCodeGeneralError, classifyError(untyped), + "precondition: this message is exactly what the heuristics misfile") + assert.Equal(t, ExitCodeGeneralError, classifyError(newPreflightGeneralError(untyped))) + }) + } +} + +// Wrapping must never swallow a verdict: the verdict IS the command's answer. +func TestPreflightGeneralError_PassesVerdictsThrough(t *testing.T) { + verdict := newPreflightVerdictError(preflight.VerdictUnknownIDs, "1 of 1 tools unavailable: not_found") + assert.Equal(t, verdict, newPreflightGeneralError(verdict)) + assert.Equal(t, ExitCodePreflightUnknownIDs, classifyError(newPreflightGeneralError(verdict))) + assert.Nil(t, newPreflightGeneralError(nil)) +} + +// --- T018: exit-code precedence (worst class wins, 12 > 11 > 10) ------------ + +func TestPreflightExitVerdict_WorstClassWins(t *testing.T) { + result := func(id, reason string) contracts.PreflightToolResult { + if reason == "" { + return contracts.PreflightToolResult{ID: id, Status: preflight.StatusReady} + } + retryable := preflight.Retryable(reason) + return contracts.PreflightToolResult{ + ID: id, Status: preflight.StatusUnavailable, Reason: reason, Retryable: &retryable, + } + } + + tests := []struct { + name string + tools []contracts.PreflightToolResult + wantExit int + }{ + { + name: "all ready", + tools: []contracts.PreflightToolResult{result("a:1", ""), result("a:2", "")}, + wantExit: 0, + }, + { + name: "retryable only", + tools: []contracts.PreflightToolResult{result("a:1", ""), result("a:2", preflight.ReasonServerInitializing)}, + wantExit: 10, + }, + { + name: "blocked beats retryable", + tools: []contracts.PreflightToolResult{ + result("a:1", preflight.ReasonServerInitializing), + result("a:2", preflight.ReasonToolChanged), + }, + wantExit: 11, + }, + { + name: "unknown id beats blocked and retryable", + tools: []contracts.PreflightToolResult{ + result("a:1", preflight.ReasonServerInitializing), + result("a:2", preflight.ReasonToolChanged), + result("a:3", preflight.ReasonNotFound), + }, + wantExit: 12, + }, + { + name: "server_not_configured is an unknown id", + tools: []contracts.PreflightToolResult{result("ghost:1", preflight.ReasonServerNotConfigured)}, + wantExit: 12, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + resp := &contracts.PreflightResponse{ + // The daemon's own verdict is deliberately understated here so + // the local aggregation is what the assertion measures. + Verdict: preflight.VerdictReady, + Tools: tc.tools, + } + verdict := preflightExitVerdict(resp) + assert.Equal(t, tc.wantExit, preflight.ExitCode(verdict)) + assert.Equal(t, tc.wantExit, classifyErrorOrZero(newPreflightVerdictError(verdict, ""))) + }) + } +} + +// The daemon's verdict still wins when it is the WORSE reading — the local +// recomputation may only escalate, never soften. +func TestPreflightExitVerdict_TakesTheWorseOfBothReadings(t *testing.T) { + resp := &contracts.PreflightResponse{ + Verdict: preflight.VerdictBlocked, + Tools: []contracts.PreflightToolResult{{ID: "a:1", Status: preflight.StatusReady}}, + } + assert.Equal(t, preflight.VerdictBlocked, preflightExitVerdict(resp)) +} + +func classifyErrorOrZero(err error) int { + if err == nil { + return 0 + } + return classifyError(err) +} + +// --- T018: request building ------------------------------------------------- + +func TestBuildPreflightRequest(t *testing.T) { + t.Run("ids, pins, profile, filters and wait", func(t *testing.T) { + req, err := buildPreflightRequest( + []string{"ctl:echo", "ctl:add"}, + []string{"ctl:echo=sha256/v1:abcd"}, + "work", + 5*time.Second, + contracts.PreflightPolicy{ReadOnlyOnly: true, ExcludeDestructive: true}, + ) + require.NoError(t, err) + require.Len(t, req.Tools, 2) + assert.Equal(t, "ctl:echo", req.Tools[0].ID) + assert.Equal(t, "sha256/v1:abcd", req.Tools[0].PinHash) + assert.Equal(t, "ctl:add", req.Tools[1].ID) + assert.Empty(t, req.Tools[1].PinHash) + assert.Equal(t, "work", req.Profile) + assert.Equal(t, 5000, req.WaitMS) + require.NotNil(t, req.Policy) + assert.True(t, req.Policy.ReadOnlyOnly) + assert.True(t, req.Policy.ExcludeDestructive) + assert.False(t, req.Policy.ExcludeOpenWorld) + }) + + t.Run("no filters means no policy object", func(t *testing.T) { + req, err := buildPreflightRequest([]string{"ctl:echo"}, nil, "", 0, contracts.PreflightPolicy{}) + require.NoError(t, err) + assert.Nil(t, req.Policy) + assert.Zero(t, req.WaitMS) + }) + + t.Run("pin for an id that was not requested is a usage error", func(t *testing.T) { + _, err := buildPreflightRequest([]string{"ctl:echo"}, []string{"ctl:other=sha256/v1:abcd"}, "", 0, contracts.PreflightPolicy{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "ctl:other") + }) + + t.Run("malformed pin", func(t *testing.T) { + for _, pin := range []string{"ctl:echo", "=hash", "ctl:echo="} { + _, err := buildPreflightRequest([]string{"ctl:echo"}, []string{pin}, "", 0, contracts.PreflightPolicy{}) + assert.Error(t, err, "pin %q must be rejected", pin) + } + }) + + t.Run("conflicting pins for one id", func(t *testing.T) { + _, err := buildPreflightRequest([]string{"ctl:echo"}, + []string{"ctl:echo=sha256/v1:aa", "ctl:echo=sha256/v1:bb"}, "", 0, contracts.PreflightPolicy{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "conflicting") + }) + + // The wire field is milliseconds, so the range must be checked on the exact + // duration: truncation would turn both of these into accepted values (0 and + // 10000) that the daemon has no way to recognize as out of range. + t.Run("wait range is validated before the millisecond truncation", func(t *testing.T) { + _, err := buildPreflightRequest([]string{"ctl:echo"}, nil, "", -1*time.Nanosecond, contracts.PreflightPolicy{}) + require.Error(t, err, "-1ns must be rejected, not truncated to 0") + assert.Contains(t, err.Error(), "negative") + + _, err = buildPreflightRequest([]string{"ctl:echo"}, nil, "", 10*time.Second+time.Nanosecond, contracts.PreflightPolicy{}) + require.Error(t, err, "10s+1ns must be rejected, not truncated to the in-range 10000ms") + assert.Contains(t, err.Error(), "cap") + + req, err := buildPreflightRequest([]string{"ctl:echo"}, nil, "", 10*time.Second, contracts.PreflightPolicy{}) + require.NoError(t, err, "exactly the cap is allowed") + assert.Equal(t, preflight.MaxWaitMS, req.WaitMS) + }) + + t.Run("a hash containing colons and slashes survives the split", func(t *testing.T) { + req, err := buildPreflightRequest([]string{"ctl:echo"}, []string{"ctl:echo=sha256/v2:deadbeef"}, "", 0, contracts.PreflightPolicy{}) + require.NoError(t, err) + assert.Equal(t, "sha256/v2:deadbeef", req.Tools[0].PinHash) + }) +} + +// --- T018: output formats --------------------------------------------------- + +func samplePreflightResponse() *contracts.PreflightResponse { + retryable := true + waited := 750 + return &contracts.PreflightResponse{ + Verdict: preflight.VerdictDegradedRetryable, + CheckedAt: time.Date(2026, 8, 15, 10, 0, 0, 0, time.UTC), + WaitedMS: &waited, + Tools: []contracts.PreflightToolResult{ + {ID: "ctl:echo", Status: preflight.StatusReady}, + { + ID: "ctl:add", + Status: preflight.StatusUnavailable, + Reason: preflight.ReasonServerInitializing, + Retryable: &retryable, + Detail: "Server \"ctl\" is still starting up.", + Remediation: preflight.DefaultRemediation(preflight.ReasonServerInitializing), + }, + }, + } +} + +func TestRenderPreflight_JSONUsesWireKeys(t *testing.T) { + rendered, err := renderPreflight("json", samplePreflightResponse()) + require.NoError(t, err) + + var decoded map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(rendered), &decoded)) + assert.Equal(t, preflight.VerdictDegradedRetryable, decoded["verdict"]) + assert.Contains(t, decoded, "checked_at") + assert.EqualValues(t, 750, decoded["waited_ms"]) + tools, ok := decoded["tools"].([]interface{}) + require.True(t, ok) + require.Len(t, tools, 2) + first := tools[0].(map[string]interface{}) + assert.Equal(t, "ctl:echo", first["id"]) + assert.NotContains(t, first, "reason", "a ready result carries no failure fields") + second := tools[1].(map[string]interface{}) + assert.Equal(t, preflight.ReasonServerInitializing, second["reason"]) + assert.Equal(t, true, second["retryable"]) +} + +// YAML must use the SAME key names as JSON — a wrapper switching -o must not +// have to learn a second vocabulary. +func TestRenderPreflight_YAMLUsesTheSameKeysAsJSON(t *testing.T) { + rendered, err := renderPreflight("yaml", samplePreflightResponse()) + require.NoError(t, err) + + var decoded map[string]interface{} + require.NoError(t, yaml.Unmarshal([]byte(rendered), &decoded)) + assert.Equal(t, preflight.VerdictDegradedRetryable, decoded["verdict"]) + assert.Contains(t, decoded, "checked_at") + assert.Contains(t, decoded, "waited_ms") + assert.NotContains(t, decoded, "waitedms", "yaml must honour the json tags, not the Go field names") +} + +func TestRenderPreflight_TableCarriesVerdictAndPerToolReasons(t *testing.T) { + rendered, err := renderPreflight("table", samplePreflightResponse()) + require.NoError(t, err) + + assert.Contains(t, rendered, "VERDICT: degraded_retryable (exit 10)") + assert.Contains(t, rendered, "CHECKED: 2026-08-15T10:00:00Z") + assert.Contains(t, rendered, "WAITED: 750ms") + assert.Contains(t, rendered, "ctl:echo") + assert.Contains(t, rendered, "ctl:add") + assert.Contains(t, rendered, preflight.ReasonServerInitializing) + assert.Contains(t, rendered, "ID") + assert.Contains(t, rendered, "RETRYABLE") +} + +// NewFormatter is case-insensitive, so the render branch has to be too: +// `-o JSON` selected the JSON formatter and then took the table path, emitting +// a "VERDICT:" header followed by JSON-encoded table rows. +func TestRenderPreflight_FormatIsCaseInsensitive(t *testing.T) { + for _, format := range []string{"JSON", " Json ", "YAML", "TABLE"} { + t.Run(format, func(t *testing.T) { + rendered, err := renderPreflight(format, samplePreflightResponse()) + require.NoError(t, err) + + if strings.EqualFold(strings.TrimSpace(format), "table") { + assert.Contains(t, rendered, "VERDICT: degraded_retryable (exit 10)") + return + } + assert.NotContains(t, rendered, "VERDICT:", + "a structured format must not be rendered through the table branch") + + var decoded map[string]interface{} + require.NoError(t, yaml.Unmarshal([]byte(rendered), &decoded), + "YAML parses JSON too, so one check covers both structured formats") + assert.Equal(t, preflight.VerdictDegradedRetryable, decoded["verdict"]) + }) + } +} + +func TestRenderPreflight_UnknownFormatIsAStructuredError(t *testing.T) { + _, err := renderPreflight("xml", samplePreflightResponse()) + require.Error(t, err) + var structured output.StructuredError + require.True(t, errors.As(err, &structured)) + assert.Equal(t, output.ErrCodeInvalidOutputFormat, structured.Code) +} + +// MCPPROXY_OUTPUT drives the format when no -o flag is given, per the repo's +// CLI output conventions (FR-009). +func TestPreflightOutputFormatHonoursEnvVar(t *testing.T) { + t.Setenv("MCPPROXY_OUTPUT", "json") + prevFormat, prevJSON := globalOutputFormat, globalJSONOutput + t.Cleanup(func() { globalOutputFormat, globalJSONOutput = prevFormat, prevJSON }) + globalOutputFormat = "" + globalJSONOutput = false + + format := ResolveOutputFormat() + require.Equal(t, "json", format) + + rendered, err := renderPreflight(format, samplePreflightResponse()) + require.NoError(t, err) + assert.True(t, strings.HasPrefix(strings.TrimSpace(rendered), "{")) +} + +func TestPreflightSummaryNamesTheFailingReasons(t *testing.T) { + summary := preflightSummary(samplePreflightResponse()) + assert.Contains(t, summary, "1 of 2 tools unavailable") + assert.Contains(t, summary, preflight.ReasonServerInitializing) + + ready := &contracts.PreflightResponse{ + Verdict: preflight.VerdictReady, + Tools: []contracts.PreflightToolResult{{ID: "ctl:echo", Status: preflight.StatusReady}}, + } + assert.Empty(t, preflightSummary(ready)) +} + +// --- T018: command wiring & --help-json metadata ---------------------------- + +func TestToolsPreflightCommand_FlagsAndHelpJSON(t *testing.T) { + cmd := newToolsPreflightCmd() + + require.NotNil(t, cmd.Args, "at least one tool id is required") + assert.Error(t, cmd.Args(cmd, nil), "no ids must be rejected before any request is made") + assert.NoError(t, cmd.Args(cmd, []string{"ctl:echo"})) + + // --help-json must reach the help hook even with no ids: it is the + // discovery call an agent makes BEFORE it knows what to pass. + withHelpJSON := newToolsPreflightCmd() + withHelpJSON.Flags().Bool("help-json", false, "") + require.NoError(t, withHelpJSON.Flags().Set("help-json", "true")) + assert.NoError(t, withHelpJSON.Args(withHelpJSON, nil)) + + info := output.ExtractHelpInfo(cmd) + assert.Equal(t, "preflight", info.Name) + assert.NotEmpty(t, info.Description) + + flagNames := make(map[string]string, len(info.Flags)) + for _, f := range info.Flags { + flagNames[f.Name] = f.Type + } + for name, wantType := range map[string]string{ + "profile": "string", + "pin": "stringArray", + "read-only-only": "bool", + "exclude-destructive": "bool", + "exclude-open-world": "bool", + "wait": "duration", + } { + gotType, ok := flagNames[name] + assert.True(t, ok, "--%s must appear in --help-json metadata", name) + assert.Equal(t, wantType, gotType, "--%s type", name) + } + + // The exit-code contract is the command's whole point, so it must be + // discoverable from the help text alone. + for _, code := range []string{"10", "11", "12"} { + assert.Contains(t, cmd.Long, code) + } +} + +func TestToolsPreflightCommand_IsRegisteredUnderTools(t *testing.T) { + var found bool + for _, sub := range GetToolsCommand().Commands() { + if sub.Name() == "preflight" { + found = true + } + } + assert.True(t, found, "tools preflight must be registered on the tools command") +} diff --git a/cmd/mcpproxy/tools_cmd.go b/cmd/mcpproxy/tools_cmd.go index 43b6c684..a95da8f3 100644 --- a/cmd/mcpproxy/tools_cmd.go +++ b/cmd/mcpproxy/tools_cmd.go @@ -2,6 +2,7 @@ package main import ( "context" + "encoding/json" "fmt" "os" "path/filepath" @@ -12,7 +13,9 @@ import ( "github.com/smart-mcp-proxy/mcpproxy-go/internal/cli/output" "github.com/smart-mcp-proxy/mcpproxy-go/internal/cliclient" "github.com/smart-mcp-proxy/mcpproxy-go/internal/config" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/contracts" "github.com/smart-mcp-proxy/mcpproxy-go/internal/logs" + "github.com/smart-mcp-proxy/mcpproxy-go/internal/preflight" "github.com/smart-mcp-proxy/mcpproxy-go/internal/secret" "github.com/smart-mcp-proxy/mcpproxy-go/internal/security/detect" "github.com/smart-mcp-proxy/mcpproxy-go/internal/storage" @@ -196,6 +199,7 @@ func init() { toolsCmd.AddCommand(toolsDisableCmd) toolsCmd.AddCommand(newToolsApproveCmd()) toolsCmd.AddCommand(newToolsRejectCmd()) + toolsCmd.AddCommand(newToolsPreflightCmd()) initToolsFlags() } @@ -827,3 +831,368 @@ func runToolsListStandalone(ctx context.Context, serverName string, globalConfig return outputToolsFromMetadata(tools, serverName) } + +// --- Spec 098: tools preflight ---------------------------------------------- + +// newToolsPreflightCmd builds `mcpproxy tools preflight`, the cron/CI gate: one +// deterministic, side-effect-free check that answers "are these tools usable +// right now?" before a job spends model tokens finding out the hard way. +// +// The exit code is the product: 0 all ready, 10 retryable (back off), 11 an +// operator has to act, 12 an id does not exist. A wrapper can branch on it +// without parsing any JSON. +func newToolsPreflightCmd() *cobra.Command { + var ( + profile string + pins []string + readOnlyOnly bool + excludeDestructive bool + excludeOpenWorld bool + wait time.Duration + ) + + cmd := &cobra.Command{ + Use: "preflight [...]", + Short: "Check that required tools are ready, without calling any upstream server", + Long: `Check a list of required tools against local proxy state and report, per tool, +whether it is ready or exactly why it is not. + +The check performs zero upstream calls and changes nothing: it reads the tool +index, approval records, connection state and configuration policy only. + +Exit codes (worst class present wins): + 0 every tool is ready + 10 degraded but retryable (a server is starting up or unhealthy) — back off and retry + 11 blocked: an operator action is needed (approve, enable, log in, re-pin) + 12 at least one requested id is unknown in your view (typo, or removed server) + 1 the command itself failed (daemon unreachable, invalid arguments) + +Examples: + mcpproxy tools preflight gh-ops:sync_issues slack:post_message + mcpproxy tools preflight ctl:echo -o json + mcpproxy tools preflight ctl:echo --pin ctl:echo=sha256/v1:9f86d0... + mcpproxy tools preflight ctl:echo --profile work --wait 5s + mcpproxy tools preflight ctl:echo --read-only-only`, + // Tool ids are required — except under --help-json, which is a + // discovery call an agent makes before it knows what to pass. Cobra + // validates Args before the --help-json hook runs, so a plain + // MinimumNArgs(1) would make the command's own metadata unreachable. + Args: preflightArgs, + RunE: func(cmd *cobra.Command, args []string) error { + request, err := buildPreflightRequest(args, pins, profile, wait, + contracts.PreflightPolicy{ + ReadOnlyOnly: readOnlyOnly, + ExcludeDestructive: excludeDestructive, + ExcludeOpenWorld: excludeOpenWorld, + }) + if err != nil { + // Argument errors keep the usage block: the operator mistyped + // the invocation and the syntax is the answer. They are still + // exit 1, not a verdict code. + return newPreflightGeneralError(err) + } + cmd.SilenceUsage = true + // From here the command's return value is a VERDICT, not a usage + // problem. Cobra would print it a second time on top of the central + // handler's "Error: …" line, and a cron log with the same verdict + // twice reads like two failures. + cmd.SilenceErrors = true + return runToolsPreflight(request) + }, + } + + cmd.Flags().StringVar(&profile, "profile", "", "Evaluate under a named profile's server scope") + cmd.Flags().StringArrayVar(&pins, "pin", nil, "Pin a tool to a schema hash: --pin =sha256/v: (repeatable)") + cmd.Flags().BoolVar(&readOnlyOnly, "read-only-only", false, "Require tools to be annotated read-only") + cmd.Flags().BoolVar(&excludeDestructive, "exclude-destructive", false, "Require tools to be annotated non-destructive") + cmd.Flags().BoolVar(&excludeOpenWorld, "exclude-open-world", false, "Require tools to be annotated closed-world") + cmd.Flags().DurationVar(&wait, "wait", 0, "Poll local state for up to this long while every failure is retryable (max 10s)") + + return cmd +} + +// preflightArgs requires at least one tool id, but lets `--help-json` through +// with none: that flag is answered by a PersistentPreRunE hook, which cobra +// runs AFTER argument validation, so a bare MinimumNArgs(1) would hide the +// command's machine-readable help from the agents it exists for. +func preflightArgs(cmd *cobra.Command, args []string) error { + if helpJSON, err := cmd.Flags().GetBool("help-json"); err == nil && helpJSON { + return nil + } + return cobra.MinimumNArgs(1)(cmd, args) +} + +// buildPreflightRequest turns CLI arguments into the REST request body. +// +// It validates only what is genuinely local (pin syntax, pins naming an id that +// was not requested, and the wait range — see below). Everything else — the +// 100-id cap, unknown profiles — is the daemon's rule, and duplicating it here +// would give the two surfaces two chances to disagree. +// +// --wait is the exception, and only because the wire field is milliseconds: the +// conversion is LOSSY, so `--wait -1ns` would reach the daemon as 0 and +// `--wait 10.0000001s` as an in-range 10000. Both would be accepted as +// something the operator did not ask for. The range is checked on the exact +// duration, against the same cap the daemon enforces (preflight.MaxWaitMS). +func buildPreflightRequest(ids, pins []string, profile string, wait time.Duration, policy contracts.PreflightPolicy) (*contracts.PreflightRequest, error) { + pinByID, err := parsePreflightPins(pins) + if err != nil { + return nil, err + } + if err := validatePreflightWaitFlag(wait); err != nil { + return nil, err + } + + request := &contracts.PreflightRequest{ + Tools: make([]contracts.PreflightToolRef, 0, len(ids)), + Profile: strings.TrimSpace(profile), + WaitMS: int(wait.Milliseconds()), + } + if policy.ReadOnlyOnly || policy.ExcludeDestructive || policy.ExcludeOpenWorld { + policyCopy := policy + request.Policy = &policyCopy + } + + requested := make(map[string]bool, len(ids)) + for _, raw := range ids { + id := strings.TrimSpace(raw) + if id == "" { + return nil, fmt.Errorf("empty tool id in arguments (expected :)") + } + requested[id] = true + request.Tools = append(request.Tools, contracts.PreflightToolRef{ID: id, PinHash: pinByID[id]}) + } + + for id := range pinByID { + if !requested[id] { + return nil, fmt.Errorf("--pin names %q, which is not in the requested tool list", id) + } + } + + return request, nil +} + +// maxPreflightWait is the --wait cap as a duration, derived from the wire cap +// so the two can never drift apart. +const maxPreflightWait = preflight.MaxWaitMS * time.Millisecond + +// validatePreflightWaitFlag rejects a wait outside [0, 10s] on the EXACT +// duration, before it is truncated to milliseconds. +func validatePreflightWaitFlag(wait time.Duration) error { + if wait < 0 { + return fmt.Errorf("--wait must not be negative (got %s)", wait) + } + if wait > maxPreflightWait { + return fmt.Errorf("--wait is %s, which exceeds the cap of %s", wait, maxPreflightWait) + } + return nil +} + +// parsePreflightPins parses repeatable `--pin =` flags. The split is +// on the FIRST '=' because a pin value is "sha256/v1:" — it carries ':' +// and '/', but never '=' — while the id carries ':'. +func parsePreflightPins(pins []string) (map[string]string, error) { + out := make(map[string]string, len(pins)) + for _, pin := range pins { + idx := strings.Index(pin, "=") + if idx <= 0 { + return nil, fmt.Errorf("invalid --pin %q: expected :=", pin) + } + id := strings.TrimSpace(pin[:idx]) + hash := strings.TrimSpace(pin[idx+1:]) + if id == "" || hash == "" { + return nil, fmt.Errorf("invalid --pin %q: expected :=", pin) + } + if existing, ok := out[id]; ok && existing != hash { + return nil, fmt.Errorf("conflicting --pin values for %q: %q and %q", id, existing, hash) + } + out[id] = hash + } + return out, nil +} + +// runToolsPreflight calls the daemon, renders the result, and converts a +// non-ready verdict into the typed exit-code error. +func runToolsPreflight(request *contracts.PreflightRequest) error { + client, _, err := newSecurityCLIClient() + if err != nil { + return newPreflightGeneralError(err) + } + + // The request's own wait budget is capped at 10s daemon-side; the transport + // deadline just has to outlive it. + ctx, cancel := context.WithTimeout(context.Background(), 60*time.Second) + defer cancel() + + response, err := client.Preflight(ctx, request) + if err != nil { + return newPreflightGeneralError(cliError("preflight failed", err)) + } + + rendered, err := renderPreflight(ResolveOutputFormat(), response) + if err != nil { + return newPreflightGeneralError(err) + } + fmt.Print(rendered) + + // The verdict is not a command failure — it is the answer. It travels as a + // typed error purely so the CENTRAL classifier assigns 10/11/12. + return newPreflightVerdictError(preflightExitVerdict(response), preflightSummary(response)) +} + +// preflightExitVerdict is the verdict the exit code comes from: the worse of +// what the daemon reported and what the per-tool results imply. +// +// Recomputing locally is deliberate. The exit code is the contract a cron +// wrapper trusts, and taking the max of both readings means an older daemon +// that under-reports the set verdict can never make a blocked tool look like a +// clean run. Both readings use the same locked table (preflight.ExitCode), so +// "worse" is just the higher exit code. +func preflightExitVerdict(response *contracts.PreflightResponse) string { + if response == nil { + return preflight.VerdictReady + } + reasons := make([]string, 0, len(response.Tools)) + for _, tool := range response.Tools { + if tool.Status == preflight.StatusReady { + continue + } + reasons = append(reasons, tool.Reason) + } + worst := preflight.VerdictForReasons(reasons) + if preflight.ExitCode(response.Verdict) > preflight.ExitCode(worst) { + return response.Verdict + } + return worst +} + +// preflightSummary is the one-line context that rides along with the exit-code +// error, e.g. "2 of 5 tools unavailable: server_disabled, not_found". +func preflightSummary(response *contracts.PreflightResponse) string { + if response == nil { + return "" + } + unavailable := 0 + seen := make(map[string]bool) + var reasons []string + for _, tool := range response.Tools { + if tool.Status == preflight.StatusReady { + continue + } + unavailable++ + if tool.Reason != "" && !seen[tool.Reason] { + seen[tool.Reason] = true + reasons = append(reasons, tool.Reason) + } + } + if unavailable == 0 { + return "" + } + summary := fmt.Sprintf("%d of %d tools unavailable", unavailable, len(response.Tools)) + if len(reasons) > 0 { + summary += ": " + strings.Join(reasons, ", ") + } + return summary +} + +// renderPreflight formats one response for the requested output format. It is +// pure (returns the string instead of printing) so every format is unit-tested +// without capturing stdout. +func renderPreflight(outputFormat string, response *contracts.PreflightResponse) (string, error) { + // Normalize ONCE, before both the formatter lookup and the branch below. + // NewFormatter is case-insensitive, so `-o JSON` used to select the JSON + // formatter and then fall into the table branch, emitting a table header + // followed by JSON-encoded rows. + format := strings.ToLower(strings.TrimSpace(outputFormat)) + + formatter, err := output.NewFormatter(format) + if err != nil { + return "", output.NewStructuredError(output.ErrCodeInvalidOutputFormat, err.Error()). + WithGuidance("Use -o table, -o json, or -o yaml") + } + + if format == "json" || format == "yaml" { + // Marshal through the wire DTO's JSON tags so `-o yaml` emits the same + // key names as `-o json` and the REST payload, rather than yaml's + // lowercased Go field names. + payload, convErr := preflightWirePayload(response) + if convErr != nil { + return "", convErr + } + rendered, fmtErr := formatter.Format(payload) + if fmtErr != nil { + return "", fmt.Errorf("failed to format output: %w", fmtErr) + } + return rendered + "\n", nil + } + + headers, rows := preflightRows(response) + table, fmtErr := formatter.FormatTable(headers, rows) + if fmtErr != nil { + return "", fmt.Errorf("failed to format table: %w", fmtErr) + } + + var b strings.Builder + fmt.Fprintf(&b, "VERDICT: %s (exit %d)\n", response.Verdict, preflight.ExitCode(preflightExitVerdict(response))) + fmt.Fprintf(&b, "CHECKED: %s\n", response.CheckedAt.Format(time.RFC3339)) + if response.WaitedMS != nil { + fmt.Fprintf(&b, "WAITED: %dms\n", *response.WaitedMS) + } + b.WriteString("\n") + b.WriteString(table) + return b.String(), nil +} + +// preflightWirePayload converts the response to generic JSON values so the +// YAML formatter honours the wire key names. +func preflightWirePayload(response *contracts.PreflightResponse) (map[string]interface{}, error) { + encoded, err := json.Marshal(response) + if err != nil { + return nil, fmt.Errorf("failed to encode preflight response: %w", err) + } + var payload map[string]interface{} + if err := json.Unmarshal(encoded, &payload); err != nil { + return nil, fmt.Errorf("failed to decode preflight response: %w", err) + } + return payload, nil +} + +// preflightRows builds the table view. Detail and remediation are sanitized: +// detail can quote an upstream-controlled server or tool name. +func preflightRows(response *contracts.PreflightResponse) (headers []string, rows [][]string) { + headers = []string{"ID", "STATUS", "REASON", "RETRYABLE", "ACTION", "DETAIL"} + rows = make([][]string, 0, len(response.Tools)) + for _, tool := range response.Tools { + reason := tool.Reason + retryable := "" + if tool.Retryable != nil { + retryable = fmt.Sprintf("%t", *tool.Retryable) + } + action := tool.Action + if reason == "" { + reason = "-" + } + if retryable == "" { + retryable = "-" + } + if action == "" { + action = "-" + } + detail := tool.Detail + if detail == "" { + detail = tool.Remediation + } + if detail == "" { + detail = "-" + } + rows = append(rows, []string{ + sanitizeName(tool.ID), + tool.Status, + reason, + retryable, + action, + sanitizeCell(detail, maxToolDescriptionCell), + }) + } + return headers, rows +} diff --git a/cmd/mcpproxy/tools_hash_pin_test.go b/cmd/mcpproxy/tools_hash_pin_test.go new file mode 100644 index 00000000..7c6de199 --- /dev/null +++ b/cmd/mcpproxy/tools_hash_pin_test.go @@ -0,0 +1,82 @@ +package main + +import ( + "bytes" + "encoding/json" + "os" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// T020 (Spec 098 FR-011): `mcpproxy tools list -o json` is the CLI half of the +// hash-pin authoring surface — the value it prints under "hash" is pasted +// straight into `mcpproxy tools preflight --pin =`. The renderers +// pass the daemon payload through untouched, so these tests are the guard +// against a future typed-struct refactor silently dropping the field. + +func captureToolsOutput(t *testing.T, format string, run func() error) string { + t.Helper() + oldStdout := os.Stdout + r, w, err := os.Pipe() + require.NoError(t, err) + os.Stdout = w + defer func() { os.Stdout = oldStdout }() + + oldFormat, oldJSON := globalOutputFormat, globalJSONOutput + globalOutputFormat, globalJSONOutput = format, false + defer func() { globalOutputFormat, globalJSONOutput = oldFormat, oldJSON }() + + runErr := run() + + w.Close() + var buf bytes.Buffer + _, _ = buf.ReadFrom(r) + require.NoError(t, runErr) + return buf.String() +} + +func toolsWithPin() []map[string]interface{} { + return []map[string]interface{}{ + { + "name": "create_issue", + "server_name": "github", + "description": "Create a new GitHub issue", + "approval_status": "approved", + "hash": "sha256/v3:abc123", + }, + { + // No stored hash: the field is simply absent, never a placeholder. + "name": "no_record", + "server_name": "github", + "description": "Never approved", + }, + } +} + +func TestToolsList_JSONCarriesHashPin(t *testing.T) { + for name, run := range map[string]func() error{ + "global": func() error { return outputGlobalTools(toolsWithPin()) }, + "per-server": func() error { return outputTools(toolsWithPin(), nil) }, + } { + t.Run(name, func(t *testing.T) { + out := captureToolsOutput(t, "json", run) + + var parsed []map[string]interface{} + require.NoError(t, json.Unmarshal([]byte(out), &parsed)) + require.Len(t, parsed, 2) + assert.Equal(t, "sha256/v3:abc123", parsed[0]["hash"], + "the pin an operator copies into --pin must survive JSON rendering") + assert.NotContains(t, parsed[1], "hash") + }) + } +} + +// The pin is a long opaque string; the human table stays as it was (#938 +// columns) so it is not pushed off screen. JSON is the authoring surface. +func TestToolsList_TableOmitsHashPin(t *testing.T) { + out := captureToolsOutput(t, "table", func() error { return outputGlobalTools(toolsWithPin()) }) + assert.NotContains(t, out, "sha256/v3:abc123") + assert.Contains(t, out, "create_issue") +} diff --git a/docs/api/rest-api.md b/docs/api/rest-api.md index e39194fd..fac13f7d 100644 --- a/docs/api/rest-api.md +++ b/docs/api/rest-api.md @@ -602,9 +602,164 @@ search/filter/sort over the full set. For relevance-ranked discovery use server cannot be read the endpoint still returns every tool it could gather and sets `partial: true` with `failed_servers` (it does not fail the whole request). +Operator-tier callers (admin API key, Unix socket, named pipe) additionally +receive each approved tool's current schema-hash pin in `hash` +(`sha256/v{N}:{hex}`) — the authoring surface for `POST /api/v1/preflight` +pins. Agent tokens never receive hashes. + #### GET /api/v1/servers/{name}/tools -List tools for a specific server. +List tools for a specific server. Carries the same operator-tier `hash` pin +field as the global listing. + +#### POST /api/v1/preflight + +Required-tools preflight (Spec 098): a deterministic, side-effect-free +availability check for a caller-supplied list of tool IDs. It performs **zero +upstream calls** and mutates no runtime state — verdicts are computed from +local state only (tool index, approval records, connection-state snapshot, +config policy). The HTTP status reports whether the **check executed**, never +what it found: a fully blocked set is still a `200` carrying +`verdict: "blocked"` in the body. See +[Required-Tools Preflight](../features/tools-preflight.md) for the feature +guide and `mcpproxy tools preflight` for the CLI wrapper. + +**Request Body:** +```json +{ + "tools": [ + { "id": "gh-ops:sync_issues" }, + { "id": "ctl:echo", "pin_hash": "sha256/v1:9f86d081884c7d65..." } + ], + "profile": "work", + "policy": { "read_only_only": true }, + "wait_ms": 5000 +} +``` + +| Field | Type | Description | +|-------|------|-------------| +| `tools` | array | Required, 1–100 entries — the limit applies to the **raw** array, before dedup. Each entry carries `id` (`:`) and optional `pin_hash`. Duplicate IDs are deduplicated (one result per unique ID); duplicates carrying **different** `pin_hash` values are a `400`. | +| `profile` | string | Optional. Evaluate under this profile's server scope so verdicts match a profile-pinned session's view. Unknown profile: `400`. Omitted: unscoped operator view. | +| `policy` | object | Optional annotation filters, Spec 094 semantics: `read_only_only`, `exclude_destructive`, `exclude_open_world` (evaluated in that fixed order; the first excluding filter owns the verdict). | +| `wait_ms` | integer | Optional, 0–10000. Poll local state while every failure is retryable-class (see below). Values over the cap are a `400`, not a silent clamp. | + +**Response** (standard `APIResponse{data}` envelope): +```json +{ + "success": true, + "data": { + "verdict": "blocked", + "checked_at": "2026-08-15T06:00:00Z", + "waited_ms": 0, + "tools": [ + { + "id": "gh-ops:sync_issues", + "status": "ready", + "hash": "sha256/v1:9f86d081884c7d65..." + }, + { + "id": "slack:post_message", + "status": "unavailable", + "reason": "server_disabled", + "retryable": false, + "action": "enable", + "detail": "Server \"slack\" is disabled.", + "remediation": "Enable the server (mcpproxy upstream enable )." + } + ] + } +} +``` + +Results are ordered by first occurrence of each unique ID in the request. A +`ready` result omits all failure fields — `ready` is a status, not a reason. An +`action` with no value is **omitted**, not `"none"` (matching the health-action +vocabulary). A malformed ID (missing the `:` separator) gets a **per-ID** +`not_found` with a format hint in `detail`, never a request-level error — one +bad entry cannot mask verdicts for the rest. `not_found` results may carry +`did_you_mean` (up to 3 nearest caller-visible IDs). `waited_ms` is present +whenever `wait_ms` was requested, including as `0` (see wait semantics). + +**Failure reasons** (closed enum, Spec 098 FR-003). Evolution is additive-only; +treat unknown codes as non-retryable. `server_saturated` is reserved and never +emitted. When multiple states co-occur for one ID, exactly one reason is +reported per the fixed precedence order (server-level states before tool-level; +see the feature page). + +| `reason` | `retryable` | Default `action` | Set verdict | CLI exit | +|---|---|---|---|---| +| `server_initializing` | true | — (omitted) | `degraded_retryable` | 10 | +| `server_unhealthy` | true | best-effort from diagnostics (`restart`/`login`/`view_logs`; default `view_logs`) | `degraded_retryable` | 10 | +| `server_disabled` | false | `enable` | `blocked` | 11 | +| `server_quarantined` | false | `approve` | `blocked` | 11 | +| `tool_pending_approval` | false | `approve` | `blocked` | 11 | +| `tool_changed` | false | `approve` | `blocked` | 11 | +| `tool_blocked_by_user` | false | `enable` | `blocked` | 11 | +| `oauth_required` | false | `login` | `blocked` | 11 | +| `hash_mismatch` | false | `configure` | `blocked` | 11 | +| `server_not_in_scope` (operator tier only) | false | `configure` | `blocked` | 11 | +| `tool_denied_by_config` | false | `configure` | `blocked` | 11 | +| `missing_annotation` | false | `configure` | `blocked` | 11 | +| `policy_filtered` | false | — (omitted) | `blocked` | 11 | +| `not_found` | false | `configure` | `unknown_ids` | 12 | +| `server_not_configured` | false | `configure` | `unknown_ids` | 12 | + +The set-level `verdict` is the worst class present: +`unknown_ids` > `blocked` > `degraded_retryable` > `ready`. + +**Status codes:** + +- `200` — the check executed; the availability verdict is data in the body. +- `400` — validation error: invalid JSON, empty or oversized (>100 raw entries) + `tools`, a duplicate ID with conflicting `pin_hash` values, `wait_ms` out of + range, or an unknown `profile`. The body is read strictly — at most 1 MiB, + exactly one JSON object, and no unknown fields — so a mistyped key (`wait` + for `wait_ms`, `pin` for `pin_hash`) fails loudly instead of silently + weakening the check a pipeline then trusts. +- `401` — missing or invalid credentials. +- `503` — the check could not run honestly: the runtime is unavailable, an + index/storage/snapshot read failed (reduced-fidelity verdicts are never + emitted), or the activity record could not be persisted. + +A request rejected with `400`/`503` executed no preflight and writes **no** +activity record. + +**`wait_ms` semantics:** polling happens only while **every** current failure +is retryable-class (`server_initializing` / `server_unhealthy`). The endpoint +re-evaluates local state on a floor interval of ≥250 ms until every tool is +ready, a non-retryable failure appears (waiting cannot help, so it resolves +immediately), or the deadline passes; it always resolves with current reasons — +never hangs. Waiting capacity is a small fixed semaphore (4 slots) dedicated to +preflight; when it is exhausted the request degrades gracefully — it resolves +immediately with current verdicts and `waited_ms: 0` instead of queuing or +failing. + +**Disclosure tiers:** + +- **Operator tier** (admin API key, Unix socket, Windows named pipe): full + results — `hash` pins on ready results, `did_you_mean` suggestions, and the + `server_not_in_scope` diagnosis when a supplied `profile` excludes an + existing server (with a `detail` noting that a session under that profile + sees `not_found`). +- **Agent-token tier**: scope-silence — an out-of-scope ID's entire result is + byte-indistinguishable from an ordinary `not_found` (same wording; no hashes; + no `did_you_mean` crossing the scope boundary). `did_you_mean` is computed + over the caller-visible index only and never suggests a quarantined server's + tools. + +**Activity-record guarantee:** every request answered `200` writes an activity +record **synchronously, before the response is returned** — request ID, +requested-ID count, set verdict, and per-tool reason codes (tool IDs and enum +codes only; no descriptions, no arguments, no hashes; local-only, never +telemetry). Correlate via the `X-Request-Id` response header and +`mcpproxy activity list --request-id `. + +**Hash pins** (`pin_hash`): format `sha256/v{N}:{hex}`. The hash schema version +is embedded so a proxy-side hash-algorithm bump is distinguishable from genuine +upstream drift (both report `hash_mismatch`, with different `detail`). Current +pins are discoverable on ready preflight results and on the operator-tier tool +listings above. ### Registries @@ -1008,7 +1163,7 @@ List activity records with filtering and pagination. | Parameter | Type | Description | |-----------|------|-------------| -| `type` | string | Filter by type: `tool_call`, `policy_decision`, `quarantine_change`, `server_change` | +| `type` | string | Filter by type: `tool_call`, `policy_decision`, `quarantine_change`, `server_change`, `preflight` | | `server` | string | Filter by server name | | `tool` | string | Filter by tool name | | `session_id` | string | Filter by MCP session ID | diff --git a/docs/cli-management-commands.md b/docs/cli-management-commands.md index 186dbeb9..e165ecc5 100644 --- a/docs/cli-management-commands.md +++ b/docs/cli-management-commands.md @@ -738,12 +738,117 @@ mcpproxy tools reject --server github --all --- +### `mcpproxy tools preflight [...]` + +Check that required tools are ready — deterministically, side-effect-free, and +**without calling any upstream server** — before a cron/CI job spends model +tokens finding out the hard way (Spec 098). Wraps `POST /api/v1/preflight`; the +check reads the tool index, approval records, connection state and config +policy only, and changes nothing. Requires daemon. The exit code is the +product: a wrapper can branch retry-vs-page-vs-fix on it without parsing any +JSON. See [Required-Tools Preflight](features/tools-preflight.md). + +**Usage:** +```bash +mcpproxy tools preflight [...] [flags] +``` + +**Flags:** +- `--profile ` - Evaluate under a named profile's server scope (unknown profile fails with exit 1) +- `--pin =sha256/v:` - Pin a tool to a schema hash; a divergence reports `hash_mismatch` (repeatable; each pinned id must be in the requested list). Current pins come from `mcpproxy tools list -o json` (`hash` field, operator tier) +- `--read-only-only` - Require tools to be annotated read-only +- `--exclude-destructive` - Require tools to be annotated non-destructive +- `--exclude-open-world` - Require tools to be annotated closed-world +- `--wait ` - Poll local state for up to this long while every failure is retryable (max `10s`; larger values are rejected by the daemon) +- `--output, -o` - Output format: `table`, `json`, `yaml` (or `MCPPROXY_OUTPUT`); `--help-json` for machine-readable command metadata + +**Exit codes** (worst class present wins): + +| Exit | Verdict | Meaning | Wrapper action | +|------|---------|---------|----------------| +| `0` | `ready` | Every tool is ready | Proceed | +| `10` | `degraded_retryable` | A server is starting up or unhealthy | Back off and retry | +| `11` | `blocked` | Operator action needed (approve, enable, log in, re-pin) | Page the operator | +| `12` | `unknown_ids` | At least one requested id is unknown in your view (typo, removed server) | Fix the job's tool list | +| `1` | — | The command itself failed (daemon unreachable, invalid arguments, rejected request) | Investigate | + +**Examples:** +```bash +mcpproxy tools preflight gh-ops:sync_issues slack:post_message +mcpproxy tools preflight ctl:echo -o json +mcpproxy tools preflight ctl:echo --pin ctl:echo=sha256/v1:9f86d081884c7d65... +mcpproxy tools preflight ctl:echo --profile work --wait 5s +mcpproxy tools preflight ctl:echo --read-only-only +``` + +**Output:** +- `table` (default): a `VERDICT: (exit )` / `CHECKED: