fix(cli): CLI contract follow-ups (exit codes, stdout purity, scope validation, redaction, config errors)

cmd/mcpproxy/activity_cmd.go

@@ -454,7 +454,7 @@ func activityPeriodFromRelative(from string) (string, error) {
 	case "-30d":
 		return "30d", nil
 	default:
-		return "", fmt.Errorf("--from on 'activity summary' accepts only -1h, -24h, -7d or -30d (aliases of --period); got '%s'", from)
+		return "", errors.New("summary supports --from -1h|-24h|-7d|-30d only")
 	}
 }
 
@@ -1585,6 +1585,11 @@ func runActivityWatch(cmd *cobra.Command, _ []string) error {
 	if err := validateActivityWatchView(); err != nil {
 		return outputActivityError(err, "INVALID_FILTER")
 	}
+	// Conflicting scope flags (--token A --agent B) would otherwise make
+	// activityWatchScopeMatches reject every event and stream nothing forever.
+	if _, err := resolveActivityScope(); err != nil {
+		return outputActivityError(err, "INVALID_FILTER")
+	}
 
 	// Setup logger
 	cmdLogLevel, _ := cmd.Flags().GetString("log-level")

cmd/mcpproxy/b5_cli_contract_test.go

@@ -0,0 +1,135 @@
+package main
+
+import (
+	"bytes"
+	"errors"
+	"os"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	clioutput "github.com/smart-mcp-proxy/mcpproxy-go/internal/cli/output"
+	"github.com/smart-mcp-proxy/mcpproxy-go/internal/connect"
+)
+
+// 1451-3: conflicting --token/--agent must fail up front for watch.
+func TestRunActivityWatch_ConflictingScopeFlagsFailFast(t *testing.T) {
+	resetActivityScopeFlags(t)
+	prevView, prevType := activityView, activityType
+	t.Cleanup(func() { activityView, activityType = prevView, prevType })
+	activityView, activityType = "all", ""
+	activityToken, activityAgent = "A", "B"
+	setOutputGlobals(t, "table", false)
+
+	err := runActivityWatch(activityWatchCmd, nil)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), "must name the same token")
+}
+
+// 1451-8: the confirmation prompt never touches stdout.
+func TestReadConfirmation_PromptGoesToWriter(t *testing.T) {
+	var out bytes.Buffer
+	ok, err := readConfirmation(strings.NewReader("y\n"), &out, "Revoke?")
+	require.NoError(t, err)
+	assert.True(t, ok)
+	assert.Equal(t, "Revoke? [y/N]: ", out.String())
+}
+
+// 1451-9: a REST refusal exits 1 even when its text mentions config.
+func TestParseAPIError_RefusalsExitOne(t *testing.T) {
+	for _, body := range []string{
+		`{"error":"invalid config value for scope"}`,
+		`{"error":"config error: server missing","field":"servers"}`,
+		`not json: config error invalid`,
+	} {
+		err := parseAPIError([]byte(body), 400, "create token")
+		require.Error(t, err)
+		assert.Equal(t, ExitCodeGeneralError, classifyError(err), body)
+	}
+	assert.Equal(t, ExitCodeConfigError, classifyError(errors.New("invalid configuration: x")))
+}
+
+// 1435-3b: a refused connect exits non-zero in every format, stdout unchanged.
+func TestPrintConnectResult_FailureReturnsError(t *testing.T) {
+	result := &connect.ConnectResult{Success: false, Client: "cursor", Action: "failed", Message: "cursor config write failed"}
+	for _, format := range []string{"table", "json", "yaml"} {
+		formatter, err := clioutput.NewFormatter(format)
+		require.NoError(t, err)
+		var perr error
+		out := captureStdout(t, func() { perr = printConnectResult(result, formatter, format) })
+		require.Error(t, perr, format)
+		assert.Equal(t, ExitCodeGeneralError, classifyError(perr))
+		assert.Contains(t, out, "cursor config write failed", format)
+	}
+	dup := &connect.ConnectResult{Success: false, Client: "cursor", Action: "already_exists", Message: "dup"}
+	jf, _ := clioutput.NewFormatter("json")
+	captureStdout(t, func() { require.NoError(t, printConnectResult(dup, jf, "json")) })
+	ok := &connect.ConnectResult{Success: true, Client: "cursor", Message: "ok", ConfigPath: "/x"}
+	formatter, _ := clioutput.NewFormatter("json")
+	captureStdout(t, func() { require.NoError(t, printConnectResult(ok, formatter, "json")) })
+}
+
+// 1466-12 / 1466-27 live in review tests; wording for --all:
+func TestReviewApproveWording_All(t *testing.T) {
+	_, summary := reviewApproveWording("m", 9, 9, nil, true)
+	assert.Equal(t, "Allowing all pending or changed tools; previously blocked tools stay blocked", summary)
+	_, summary = reviewApproveWording("m", 9, 9, nil, false)
+	assert.Equal(t, "Allowing 9 of 9 tools; blocking none", summary)
+}
+
+func TestReviewScanLine_EmptyCoverageIsNone(t *testing.T) {
+	assert.Equal(t, "Scan: none", reviewScanLine(map[string]interface{}{"name": "n", "scan": map[string]interface{}{"verdict": "clean"}}))
+}
+
+// 1466-15: an explicit, unloadable --config is an error, not a silent default.
+func TestLoadRegistryConfig_MissingExplicitPathErrors(t *testing.T) {
+	prev := registryConfigPath
+	t.Cleanup(func() { registryConfigPath = prev })
+	registryConfigPath = t.TempDir() + "/nope.json"
+	_, err := loadRegistryConfig()
+	require.Error(t, err)
+}
+
+// 1466-16: --data-dir with no config still gets the MCPPROXY_LISTEN overlay.
+func TestLoadCLIConfig_DataDirOnlyAppliesEnvOverlay(t *testing.T) {
+	home := t.TempDir()
+	t.Setenv("HOME", home)
+	t.Setenv("USERPROFILE", home)
+	t.Setenv("MCPPROXY_LISTEN", "127.0.0.1:18999")
+	prev := dataDir
+	t.Cleanup(func() { dataDir = prev })
+	dataDir = t.TempDir()
+	cfg, err := loadCLIConfig("")
+	require.NoError(t, err)
+	assert.Equal(t, "127.0.0.1:18999", cfg.Listen)
+}
+
+// 1466-17: an apostrophe inside a URL must not end the redaction early.
+func TestRedactDoctorString_ApostropheInURL(t *testing.T) {
+	got := redactDoctorString("see https://h.example/x?label=Bob's&%74oken=sekret-1 ok")
+	assert.NotContains(t, got, "sekret-1")
+	quoted := redactDoctorString("dial 'https://h.example/x?token=sekret-2' failed")
+	assert.NotContains(t, quoted, "sekret-2")
+	assert.Contains(t, quoted, "REDACTED'")
+}
+
+// 1394-5: FR-075 verbatim message.
+func TestActivityPeriodFromRelative_FR075Message(t *testing.T) {
+	_, err := activityPeriodFromRelative("-3d")
+	require.Error(t, err)
+	assert.Equal(t, "summary supports --from -1h|-24h|-7d|-30d only", err.Error())
+}
+
+// Disconnecting a never-registered client is a no-op and must exit 0.
+func TestConnectResultError_NotFoundDisconnectExitsZero(t *testing.T) {
+	nf := &connect.ConnectResult{Success: false, Client: "cursor", Action: "not_found", Message: "no entry"}
+	require.NoError(t, connectResultError(nf))
+	require.Error(t, connectResultError(&connect.ConnectResult{Action: "failed", Message: "x"}))
+}
+
+// The confirmation prompt must default to stderr, never stdout.
+func TestConfirmPromptOut_DefaultsToStderr(t *testing.T) {
+	assert.Same(t, os.Stderr, confirmPromptOut)
+}

cmd/mcpproxy/cli_config.go

@@ -33,6 +33,7 @@ func loadCLIConfig(explicitPath string) (*config.Config, error) {
 		// $HOME/.mcpproxy/mcp_config.json and report the HOME defaults, which
 		// contradicts the data dir the operator named.
 		cfg = config.DefaultConfig()
+		config.ApplyEnvOverrides(cfg)
 	} else {
 		cfg, err = config.Load()
 	}

cmd/mcpproxy/connect_cmd.go

@@ -343,7 +343,7 @@ func printConnectResult(result *connect.ConnectResult, formatter clioutput.Outpu
 		} else {
 			fmt.Printf("Failed: %s\n", result.Message)
 		}
-		return nil
+		return connectResultError(result)
 	}
 
 	// JSON/YAML
@@ -352,7 +352,20 @@ func printConnectResult(result *connect.ConnectResult, formatter clioutput.Outpu
 		return err
 	}
 	fmt.Println(out)
-	return nil
+	return connectResultError(result)
+}
+
+// connectResultError turns a refused connect/disconnect result into a non-nil
+// error so the exit code is 1; stdout is already printed unchanged. nil for a
+// successful result or an already_exists no-op.
+func connectResultError(result *connect.ConnectResult) error {
+	// already_exists (idempotent connect) and not_found (disconnecting a client
+	// that was never registered) are deliberate "result, not an error" no-ops,
+	// so they keep exiting 0.
+	if result == nil || result.Success || result.Action == "already_exists" || result.Action == "not_found" {
+		return nil
+	}
+	return cliRefusalError{fmt.Errorf("%s", result.Message)}
 }
 
 func loadConnectConfig() (*config.Config, error) {

cmd/mcpproxy/connect_hint_test.go

@@ -105,6 +105,7 @@ func TestPrintConnectResult_FailureHasNoNextLine(t *testing.T) {
 	}
 
 	out := captureStdout(t, func() {
+		// already_exists stays a result (exit 0); other failures exit 1 (#1435).
 		if err := printConnectResult(result, formatter, "table"); err != nil {
 			t.Errorf("printConnectResult: %v", err)
 		}

cmd/mcpproxy/daemon_rest.go

@@ -405,6 +405,10 @@ func warningFix(w runtime.Warning) string {
 	return ""
 }
 
+// confirmPromptOut is where the confirmation prompt is written (stderr, so
+// `-o json` stdout stays pure JSON). A var so a test can pin the default.
+var confirmPromptOut io.Writer = os.Stderr
+
 // confirmYes asks for confirmation on a terminal, or accepts --yes. A non-TTY
 // stdin without --yes is refused (exit 1) so a script never proceeds blind.
 func confirmYes(prompt string, yes bool) (bool, error) {
@@ -414,8 +418,14 @@ func confirmYes(prompt string, yes bool) (bool, error) {
 	if !term.IsTerminal(int(os.Stdin.Fd())) {
 		return false, flagValidationError{errors.New("confirmation required; pass --yes")}
 	}
-	fmt.Printf("%s [y/N]: ", prompt)
-	answer, err := bufio.NewReader(os.Stdin).ReadString('\n')
+	// The prompt goes to stderr so `-o json` stdout stays pure JSON.
+	return readConfirmation(os.Stdin, confirmPromptOut, prompt)
+}
+
+// readConfirmation writes the [y/N] prompt to out and reads one answer from in.
+func readConfirmation(in io.Reader, out io.Writer, prompt string) (bool, error) {
+	fmt.Fprintf(out, "%s [y/N]: ", prompt)
+	answer, err := bufio.NewReader(in).ReadString('\n')
 	if err != nil {
 		return false, fmt.Errorf("failed to read confirmation: %w", err)
 	}

cmd/mcpproxy/doctor_redact.go

@@ -20,7 +20,10 @@ const doctorRedactionMask = "REDACTED"
 // in doctor output even outside a URL. runDoctor sets it for the run.
 var doctorSecretLiterals []string
 
-var doctorURLPattern = regexp.MustCompile(`(?i)\b[a-z][a-z0-9+.-]*://[^\s"'<>]+`)
+// An apostrophe is legal inside a URL (`?label=Bob's`), so the body only stops
+// at whitespace, double quote or angle brackets; a trailing apostrophe that
+// merely closes a single-quoted URL is trimmed in redactDoctorString.
+var doctorURLPattern = regexp.MustCompile(`(?i)\b[a-z][a-z0-9+.-]*://[^\s"<>]+`)
 
 func doctorMask(string) string { return doctorRedactionMask }
 
@@ -32,7 +35,11 @@ func redactDoctorString(s string) string {
 	}
 	if strings.Contains(s, "://") {
 		s = doctorURLPattern.ReplaceAllStringFunc(s, func(u string) string {
-			return oauth.RedactURLQueryParamsWith(u, doctorMask)
+			trail := ""
+			if strings.HasSuffix(u, "'") {
+				u, trail = strings.TrimSuffix(u, "'"), "'"
+			}
+			return oauth.RedactURLQueryParamsWith(u, doctorMask) + trail
 		})
 	}
 	for _, lit := range doctorSecretLiterals {

cmd/mcpproxy/main.go

@@ -962,6 +962,14 @@ func applyServeRuntimeFlags(cmd *cobra.Command, cfg *config.Config) {
 // valid-values list happens not to contain "config").
 type flagValidationError struct{ error }
 
+// cliRefusalError marks a refusal reported by the daemon's REST API. Its text
+// is the daemon's, so classifyError must not run the config/permission string
+// heuristics over it (a message mentioning "config" is not a config-file
+// error); it exits 1.
+type cliRefusalError struct{ error }
+
+func (e cliRefusalError) Unwrap() error { return e.error }
+
 func newFlagValidationError(format string, args ...any) error {
 	return flagValidationError{fmt.Errorf(format, args...)}
 }
@@ -976,6 +984,10 @@ func classifyError(err error) int {
 	if errors.As(err, &flagErr) {
 		return ExitCodeGeneralError
 	}
+	var refusalErr cliRefusalError
+	if errors.As(err, &refusalErr) {
+		return ExitCodeGeneralError
+	}
 
 	// 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

cmd/mcpproxy/registry_cmd.go

@@ -627,6 +627,11 @@ func loadRegistryConfig() (*config.Config, error) {
 	// $HOME/.mcpproxy/mcp_config.json.
 	cfg, err := loadCLIConfig(registryConfigPath)
 	if err != nil {
+		// An explicitly named config that cannot be loaded is an error, not a
+		// reason to silently use defaults.
+		if resolveCLIConfigPath(registryConfigPath) != "" {
+			return nil, err
+		}
 		// Discovery should still work with defaults if no config is present.
 		cfg = config.DefaultConfig()
 		if dataDir != "" {

cmd/mcpproxy/review_cmd.go

@@ -72,7 +72,7 @@ func newReviewCommand(confirm func(string) (bool, error)) *cobra.Command {
 			if len(block) > 0 {
 				body["block"] = block
 			}
-			prompt, summary = reviewApproveWording(server, len(state.tools), allowed, block)
+			prompt, summary = reviewApproveWording(server, len(state.tools), allowed, block, all)
 		} else if len(tools) > 0 || len(except) > 0 {
 			// Nothing to select from: dropping --tools/--except would approve blind.
 			return fmt.Errorf("no tool definitions captured for server '%s'; fetch them first (mcpproxy review show %s) before using --tools or --except", server, server)
@@ -277,13 +277,19 @@ func reviewApproveSelection(tools []reviewToolState, all bool, only, except []st
 
 // reviewApproveWording builds the confirmation prompt and the table-mode
 // summary line, both naming the exact count.
-func reviewApproveWording(server string, total, allowed int, block []string) (prompt, summary string) {
+func reviewApproveWording(server string, total, allowed int, block []string, all bool) (prompt, summary string) {
 	noun := func(n int) string {
 		if n == 1 {
 			return "tool"
 		}
 		return "tools"
 	}
+	if len(block) == 0 && all {
+		// --all approves pending or changed tools only; tools blocked earlier
+		// stay blocked, so "N of N; blocking none" would overstate it.
+		return fmt.Sprintf("Approve server '%s' with all %d %s?", server, total, noun(total)),
+			"Allowing all pending or changed tools; previously blocked tools stay blocked"
+	}
 	if len(block) == 0 {
 		return fmt.Sprintf("Approve server '%s' with all %d %s?", server, total, noun(total)),
 			fmt.Sprintf("Allowing %d of %d %s; blocking none", allowed, total, noun(total))
@@ -371,10 +377,14 @@ func formatReviewResponse(format string, raw []byte, full bool) error {
 // as the Web and macOS review screens. It returns "" when the payload carries
 // no scan or predates coverage.
 func reviewScanLine(server map[string]interface{}) string {
-	scan, _ := server["scan"].(map[string]interface{})
+	scan, hasScan := server["scan"].(map[string]interface{})
+	if !hasScan {
+		return ""
+	}
 	coverage, _ := scan["coverage"].(string)
 	if coverage == "" {
-		return ""
+		// An older core sends no coverage: say "none" like the Web and macOS screens.
+		return "Scan: none"
 	}
 	rescan := "run: mcpproxy security rescan " + fmt.Sprint(server["name"])
 	switch coverage {

cmd/mcpproxy/review_cmd_test.go

@@ -263,9 +263,9 @@ func TestFormatReviewShowPrintsScanCoverage(t *testing.T) {
 		"Scan: not scanned yet; run: mcpproxy security rescan notes")
 	require.Contains(t, show(`,"scan":{"verdict":"not_scanned","coverage":"scanning"}`), "Scan: in progress")
 
-	// No scan key (or a daemon that predates coverage): no Scan line at all.
+	// No scan object: no line. A scan without coverage (older core): "none", as on Web and macOS.
 	require.NotContains(t, show(``), "Scan:")
-	require.NotContains(t, show(`,"scan":{"verdict":"clean"}`), "Scan:")
+	require.Contains(t, show(`,"scan":{"verdict":"clean"}`), "Scan: none")
 }
 
 func captureReviewOutput(t *testing.T, fn func() error) string {

cmd/mcpproxy/testdata/cli109/review-approve-all.golden

@@ -1,2 +1,2 @@
-Allowing 9 of 9 tools; blocking none
+Allowing all pending or changed tools; previously blocked tools stay blocked
 Approved server memory

cmd/mcpproxy/token_cmd.go

@@ -627,10 +627,10 @@ func parseAPIError(body []byte, statusCode int, operation string) error {
 				// other flag-validation failure.
 				return flagValidationError{fmt.Errorf("failed to %s: %s (field: %s)", operation, errMsg, field)}
 			}
-			return fmt.Errorf("failed to %s: %s", operation, errMsg)
+			return cliRefusalError{fmt.Errorf("failed to %s: %s", operation, errMsg)}
 		}
 	}
-	return fmt.Errorf("failed to %s: HTTP %d: %s", operation, statusCode, string(body))
+	return cliRefusalError{fmt.Errorf("failed to %s: HTTP %d: %s", operation, statusCode, string(body))}
 }
 
 func getMapString(m map[string]interface{}, key string) string {

docs/cli/review-commands.md

@@ -93,7 +93,7 @@ Using the wrong flag for the state fails with exit code 1 instead of doing somet
 - `--tools` or `--except` while no tool definitions are captured: `no tool definitions captured for server 's'; fetch them first ...`; nothing is written
 - `--except` on a trusted server: `--except applies only while approving a quarantined server`
 
-The confirmation prompt reads the review first and names the exact count, for example `Approve server 'memory' with 3 of 9 tools? Blocked: a, b, c.`, `Approve server 'memory' with all 9 tools?` or `Approve server 'memory' without seeing tools?` when nothing is captured. In table output one line precedes the result: `Allowing 3 of 9 tools; blocking 6: a, b, ...`. JSON and YAML output stay the REST data object.
+The confirmation prompt reads the review first and names the exact count, for example `Approve server 'memory' with 3 of 9 tools? Blocked: a, b, c.`, `Approve server 'memory' with all 9 tools?` or `Approve server 'memory' without seeing tools?` when nothing is captured. In table output one line precedes the result: `Allowing 3 of 9 tools; blocking 6: a, b, ...`; with `--all` it reads `Allowing all pending or changed tools; previously blocked tools stay blocked`. JSON and YAML output stay the REST data object.
 
 `mcpproxy review approve <server> --yes` used to allow every tool. It now allows the default selection, so it matches the review screens; add `--all` to approve every tool.
 

internal/config/loader.go

@@ -743,6 +743,11 @@ func expandDataDir(cfg *Config) {
 	cfg.DataDir = resolved
 }
 
+// ApplyEnvOverrides applies the MCPPROXY_* environment overlay (listen, TLS,
+// data dir, trusted hosts...) that Load applies, for callers that build a
+// config from DefaultConfig without reading a file.
+func ApplyEnvOverrides(cfg *Config) { applyTLSEnvOverrides(cfg) }
+
 // applyTLSEnvOverrides applies the MCPPROXY_* environment overrides. Each one
 // goes through OverrideForProcess so no save path persists it (see
 // process_overrides.go); the env-sourced set is rebuilt from scratch on every