Fix plan mode review regressions

Honor shell quoting when screening plan-mode bash commands, allow explicitly declared MCP servers to connect in token economy plan mode, and strip the legacy plan marker from restored display text.

Co-authored-by: SivanCola <32437197+SivanCola@users.noreply.github.com>

Author: SivanCola

internal/boot/boot.go

@@ -922,7 +922,8 @@ func Build(ctx context.Context, opts Options) (*control.Controller, error) {
 				}
 				return fmt.Sprintf("enabled MCP server %q tools: %s.", spec.Name, strings.Join(names, ", ")), nil
 			},
-			mcpNames: onDemandMCPNames,
+			mcpNames:             onDemandMCPNames,
+			planModeAllowedTools: cfg.Agent.PlanModeAllowedTools,
 		})
 	}
 

internal/boot/boot_test.go

@@ -1360,6 +1360,76 @@ READ ONLY SKILL BODY`)
 	}
 }
 
+func TestBuildTokenEconomyPlanModeCanConnectAllowedMCPSource(t *testing.T) {
+	isolateConfigHome(t)
+	dir := robustTempDir(t)
+	t.Chdir(dir)
+
+	registerBootTokenProfileTestProvider()
+	prov := testutil.NewMock("token-economy",
+		testutil.Turn{ToolCalls: []provider.ToolCall{
+			{ID: "source-1", Name: "connect_tool_source", Arguments: `{"source":"mcp","name":"mockmcp"}`},
+		}},
+		testutil.Turn{Text: "done"},
+	)
+	setBootTokenProfileTestProvider(t, prov)
+	writeFile(t, dir, "reasonix.toml", fmt.Sprintf(`
+default_model = "test-model"
+
+[agent]
+system_prompt = "BASE"
+plan_mode_allowed_tools = ["mcp__mockmcp__echo"]
+
+[[providers]]
+name = "test-model"
+kind = "boot-token-profile-test"
+model = "x"
+
+[[plugins]]
+name = "mockmcp"
+command = %q
+args = ["-test.run=TestHelperProcess", "--"]
+env = { GO_WANT_HELPER_PROCESS = "1" }
+`, os.Args[0]))
+
+	ctrl, err := Build(context.Background(), Options{Sink: event.Discard, TokenMode: TokenModeEconomy})
+	if err != nil {
+		t.Fatalf("Build: %v", err)
+	}
+	defer ctrl.Close()
+	ctrl.SetPlanMode(true)
+	if err := ctrl.Run(context.Background(), "connect allowed mcp while planning"); err != nil {
+		t.Fatalf("Run: %v", err)
+	}
+
+	reqs := prov.Requests()
+	if len(reqs) != 2 {
+		t.Fatalf("requests = %d, want 2", len(reqs))
+	}
+	if !requestHasTool(reqs[1], "mcp__mockmcp__echo") {
+		t.Fatalf("second request should expose allowed MCP source in plan economy mode; tools=%v", toolSchemaNames(reqs[1].Tools))
+	}
+	for _, msg := range ctrl.History() {
+		if msg.Role == provider.RoleTool && msg.Name == "connect_tool_source" {
+			if strings.Contains(msg.Content, "blocked:") {
+				t.Fatalf("connect_tool_source should not block allowed MCP in plan mode, got:\n%s", msg.Content)
+			}
+			if !strings.Contains(msg.Content, `enabled MCP server "mockmcp" tools: mcp__mockmcp__echo`) {
+				t.Fatalf("connect_tool_source should report enabled MCP tools, got:\n%s", msg.Content)
+			}
+		}
+	}
+}
+
+func TestPlanModeAllowsMCPServerRequiresConcreteToolName(t *testing.T) {
+	if planModeAllowsMCPServer([]string{"mcp__mockmcp__"}, "mockmcp") {
+		t.Fatal("bare MCP namespace prefix should not allow a server in plan mode")
+	}
+	if !planModeAllowsMCPServer([]string{"mcp__mockmcp__echo"}, "mockmcp") {
+		t.Fatal("concrete MCP tool name should allow its server in plan mode")
+	}
+}
+
 func TestBuildTokenEconomyPlanModeBlocksSourcesWithPolicy(t *testing.T) {
 	tests := []struct {
 		source          string

internal/boot/token_profile.go

@@ -10,6 +10,7 @@ import (
 
 	"reasonix/internal/agent"
 	"reasonix/internal/planmode"
+	"reasonix/internal/plugin"
 	"reasonix/internal/tool"
 )
 
@@ -80,6 +81,8 @@ type toolSourceConnector struct {
 	lsp           func(context.Context) (string, error)
 	mcp           func(context.Context, string) (string, error)
 	mcpNames      []string
+
+	planModeAllowedTools []string
 }
 
 func (*toolSourceConnector) Name() string { return "connect_tool_source" }
@@ -113,11 +116,12 @@ func (t *toolSourceConnector) Execute(ctx context.Context, args json.RawMessage)
 	if source == "" {
 		return "", fmt.Errorf("unknown tool source %q; available: %s", p.Source, strings.Join(t.availableSources(), ", "))
 	}
+	name := strings.TrimSpace(p.Name)
 
 	t.mu.Lock()
 	defer t.mu.Unlock()
 
-	if blocked, msg := planModeSourceBlocked(ctx, source); blocked {
+	if blocked, msg := t.planModeSourceBlocked(ctx, source, name); blocked {
 		return msg, nil
 	}
 
@@ -137,7 +141,6 @@ func (t *toolSourceConnector) Execute(ctx context.Context, args json.RawMessage)
 	case "lsp":
 		return runSourceInstaller(ctx, "lsp", t.lsp)
 	case "mcp":
-		name := strings.TrimSpace(p.Name)
 		if name == "" {
 			if len(t.mcpNames) == 0 {
 				return "No configured MCP servers are available in this session.", nil
@@ -155,10 +158,16 @@ func (t *toolSourceConnector) Execute(ctx context.Context, args json.RawMessage)
 	}
 }
 
-func planModeSourceBlocked(ctx context.Context, source string) (bool, string) {
+func (t *toolSourceConnector) planModeSourceBlocked(ctx context.Context, source, name string) (bool, string) {
 	if !agent.PlanModeFromContext(ctx) {
 		return false, ""
 	}
+	if source == "mcp" {
+		if name == "" || planModeAllowsMCPServer(t.planModeAllowedTools, name) {
+			return false, ""
+		}
+		return true, fmt.Sprintf("blocked: MCP source %q is not available in plan mode unless plan_mode_allowed_tools declares at least one concrete tool with prefix %q. Keep exploring with read-only tools, then write your plan for approval before using this MCP server.", name, plugin.ToolPrefix(name))
+	}
 	// Sources are read-only iff they expose only read-only research surfaces; the
 	// moderate plan-mode gate then trusts that ReadOnly flag (step 6), while any
 	// other source stays non-read-only and is fail-closed by the policy.
@@ -167,6 +176,17 @@ func planModeSourceBlocked(ctx context.Context, source string) (bool, string) {
 	return decision.Blocked, decision.Message
 }
 
+func planModeAllowsMCPServer(allowedTools []string, server string) bool {
+	prefix := plugin.ToolPrefix(server)
+	for _, name := range allowedTools {
+		name = strings.TrimSpace(name)
+		if strings.HasPrefix(name, prefix) && len(name) > len(prefix) {
+			return true
+		}
+	}
+	return false
+}
+
 func normalizeToolSource(source string) string {
 	switch strings.ToLower(strings.TrimSpace(source)) {
 	case "skill", "skills":

internal/control/input.go

@@ -15,6 +15,8 @@ import (
 // prompt prefix is left untouched and the toggle costs nothing in cache hits.
 const PlanModeMarker = planmode.Marker
 
+const legacyPlanModeMarker = "[Plan mode — read-only. Explore the codebase first (read_file, ls, grep, glob, web_fetch, task, ask are available; writers are refused by the harness). Before planning, if a decision that is genuinely the user's — tech stack, an ambiguous requirement, scope, an irreversible choice — would materially shape the plan and you can't settle it from the codebase or a sensible default, use the ask tool to clarify it first; otherwise pick the obvious default and state the assumption in the plan instead of asking. Then present a LAYERED plan as your reply and stop — do not write files, edit, or run side-effecting bash. Structure the plan as a two-level markdown list so it becomes a layered task list: each PHASE is a top-level numbered list item (a coherent milestone, e.g. \"1. Add the config loader\"), and each phase's concrete, verifiable sub-steps are bullets indented beneath it (e.g. \"   - parse the TOML into Config\"). Use plain numbered list items for phases — do NOT write phases as markdown headings (##, ###) — so both levels parse. Keep phases few (about 2-6). The user will be asked to approve before any changes are made.]"
+
 const (
 	activeGoalOpen  = "<active-goal>"
 	activeGoalClose = "</active-goal>"
@@ -44,12 +46,17 @@ const (
 // feature, or synthetic user messages injected by the controller).
 func StripComposePrefixes(content string) string {
 	s := agent.StripTransientUserBlocks(content)
-	s = strings.TrimPrefix(s, PlanModeMarker+"\n\n")
-	s = strings.TrimPrefix(s, PlanModeMarker)
+	s = stripComposeMarker(s, PlanModeMarker)
+	s = stripComposeMarker(s, legacyPlanModeMarker)
 	s = strings.TrimSpace(s)
 	return s
 }
 
+func stripComposeMarker(s, marker string) string {
+	s = strings.TrimPrefix(s, marker+"\n\n")
+	return strings.TrimPrefix(s, marker)
+}
+
 // StripReferencedContextPrefix removes the "Referenced context:" preamble and
 // the trailing XML reference blocks (<file>, <dir>, <resource>, <image>) that
 // controller.ResolveRefs injects when the user @-references files or resources.

internal/control/input_test.go

@@ -783,6 +783,11 @@ func TestStripComposePrefixes(t *testing.T) {
 			input: PlanModeMarker + "\n\nexplain this function",
 			want:  "explain this function",
 		},
+		{
+			name:  "legacy plan mode marker stripped",
+			input: legacyPlanModeMarker + "\n\nexplain this function",
+			want:  "explain this function",
+		},
 		{
 			name:  "plan mode marker without trailing newlines",
 			input: PlanModeMarker,

internal/planmode/policy.go

@@ -328,7 +328,14 @@ func decideBash(args json.RawMessage) Decision {
 		if !bashMatchesSafePrefix(lower, safe) {
 			continue
 		}
-		if arg := unsafeSafeCommandArg(cmd, safe); arg != "" {
+		arg, err := unsafeSafeCommandArg(cmd, safe)
+		if err != "" {
+			return Decision{
+				Blocked: true,
+				Message: fmt.Sprintf("blocked: bash command in plan mode has malformed shell quoting (%s). Use a simple read-only command while planning.", err),
+			}
+		}
+		if arg != "" {
 			return Decision{
 				Blocked: true,
 				Message: fmt.Sprintf("blocked: bash command in plan mode uses a write-capable argument (%q). Use a read-only command while planning.", arg),
@@ -354,11 +361,14 @@ func bashMatchesSafePrefix(lower, safe string) bool {
 	return unicode.IsSpace(r)
 }
 
-func unsafeSafeCommandArg(cmd, safe string) string {
-	fields := strings.Fields(cmd)
+func unsafeSafeCommandArg(cmd, safe string) (string, string) {
+	fields, err := shellFields(cmd)
+	if err != "" {
+		return "", err
+	}
 	base := strings.Fields(safe)
 	if len(fields) <= len(base) {
-		return ""
+		return "", ""
 	}
 	args := fields[len(base):]
 	lowerArgs := make([]string, len(args))
@@ -368,7 +378,7 @@ func unsafeSafeCommandArg(cmd, safe string) string {
 	if strings.HasPrefix(safe, "git ") {
 		for _, arg := range lowerArgs {
 			if arg == "--output" || strings.HasPrefix(arg, "--output=") || arg == "--ext-diff" {
-				return arg
+				return arg, ""
 			}
 		}
 	}
@@ -377,21 +387,93 @@ func unsafeSafeCommandArg(cmd, safe string) string {
 		for i, arg := range args {
 			lowerArg := lowerArgs[i]
 			if arg == "-O" || strings.HasPrefix(arg, "-O") || strings.HasPrefix(lowerArg, "--open-files-in-pager") {
-				return arg
+				return arg, ""
 			}
 		}
 	case "find":
 		for _, arg := range lowerArgs {
 			if findWriteArgs[arg] {
-				return arg
+				return arg, ""
 			}
 		}
 	case "go list", "go vet":
 		for _, arg := range lowerArgs {
 			if goWriteOrExecArgs[arg] || strings.HasPrefix(arg, "-mod=mod") || strings.HasPrefix(arg, "-modfile=") || strings.HasPrefix(arg, "-toolexec=") || strings.HasPrefix(arg, "-vettool=") {
-				return arg
+				return arg, ""
+			}
+		}
+	}
+	return "", ""
+}
+
+func shellFields(s string) ([]string, string) {
+	var fields []string
+	var b strings.Builder
+	inSingle := false
+	inDouble := false
+	escaped := false
+	haveField := false
+	flush := func() {
+		if haveField {
+			fields = append(fields, b.String())
+			b.Reset()
+			haveField = false
+		}
+	}
+	for _, r := range s {
+		if escaped {
+			b.WriteRune(r)
+			haveField = true
+			escaped = false
+			continue
+		}
+		if inSingle {
+			if r == '\'' {
+				inSingle = false
+				continue
 			}
+			b.WriteRune(r)
+			haveField = true
+			continue
 		}
+		if inDouble {
+			switch r {
+			case '"':
+				inDouble = false
+			case '\\':
+				escaped = true
+			default:
+				b.WriteRune(r)
+				haveField = true
+			}
+			continue
+		}
+		switch {
+		case unicode.IsSpace(r):
+			flush()
+		case r == '\'':
+			inSingle = true
+			haveField = true
+		case r == '"':
+			inDouble = true
+			haveField = true
+		case r == '\\':
+			escaped = true
+			haveField = true
+		default:
+			b.WriteRune(r)
+			haveField = true
+		}
+	}
+	if escaped {
+		return nil, "dangling escape"
+	}
+	if inSingle {
+		return nil, "unterminated single quote"
+	}
+	if inDouble {
+		return nil, "unterminated double quote"
 	}
-	return ""
+	flush()
+	return fields, ""
 }