From a4fa3023c1684a70bebaba481bd4c30f87eb0b4b Mon Sep 17 00:00:00 2001
From: Charles Packer <packercharles@gmail.com>
Date: Wed, 24 Dec 2025 14:59:29 -0800
Subject: [PATCH] fix: always show detailed plan mode denial message (#386)

Co-authored-by: Letta <noreply@letta.com>
---
 src/cli/App.tsx                    | 25 ++++++++++++++++---------
 src/permissions/checker.ts         | 11 +++++------
 src/tests/permissions-mode.test.ts |  5 +++--
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/src/cli/App.tsx b/src/cli/App.tsx
index e314801..818ebcc 100644
--- a/src/cli/App.tsx
+++ b/src/cli/App.tsx
@@ -1222,10 +1222,13 @@ export default function App({
 
             // Create denial results for auto-denied tools and update buffers
             const autoDeniedResults = autoDenied.map((ac) => {
-              const reason =
-                "matchedRule" in ac.permission && ac.permission.matchedRule
+              // Prefer the detailed reason over the short matchedRule name
+              // (e.g., reason contains plan file path info, matchedRule is just "plan mode")
+              const reason = ac.permission.reason
+                ? `Permission denied: ${ac.permission.reason}`
+                : "matchedRule" in ac.permission && ac.permission.matchedRule
                   ? `Permission denied by rule: ${ac.permission.matchedRule}`
-                  : `Permission denied: ${ac.permission.reason || "Unknown reason"}`;
+                  : "Permission denied: Unknown reason";
 
               // Update buffers with tool rejection for UI
               onChunk(buffersRef.current, {
@@ -3623,10 +3626,12 @@ DO NOT respond to these messages or otherwise consider them in your response unl
 
               // Create denial results for auto-denied and update UI
               const autoDeniedResults = autoDenied.map((ac) => {
-                const reason =
-                  "matchedRule" in ac.permission && ac.permission.matchedRule
+                // Prefer the detailed reason over the short matchedRule name
+                const reason = ac.permission.reason
+                  ? `Permission denied: ${ac.permission.reason}`
+                  : "matchedRule" in ac.permission && ac.permission.matchedRule
                     ? `Permission denied by rule: ${ac.permission.matchedRule}`
-                    : `Permission denied: ${ac.permission.reason || "Unknown"}`;
+                    : "Permission denied: Unknown";
 
                 // Update buffers with denial for UI
                 onChunk(buffersRef.current, {
@@ -3704,10 +3709,12 @@ DO NOT respond to these messages or otherwise consider them in your response unl
 
               // Create denial reasons for auto-denied and update UI
               const autoDeniedWithReasons = autoDenied.map((ac) => {
-                const reason =
-                  "matchedRule" in ac.permission && ac.permission.matchedRule
+                // Prefer the detailed reason over the short matchedRule name
+                const reason = ac.permission.reason
+                  ? `Permission denied: ${ac.permission.reason}`
+                  : "matchedRule" in ac.permission && ac.permission.matchedRule
                     ? `Permission denied by rule: ${ac.permission.matchedRule}`
-                    : `Permission denied: ${ac.permission.reason || "Unknown"}`;
+                    : "Permission denied: Unknown";
 
                 // Update buffers with denial for UI
                 onChunk(buffersRef.current, {
diff --git a/src/permissions/checker.ts b/src/permissions/checker.ts
index 30aa8b5..cb24d0e 100644
--- a/src/permissions/checker.ts
+++ b/src/permissions/checker.ts
@@ -120,12 +120,11 @@ export function checkPermission(
     let reason = `Permission mode: ${currentMode}`;
     if (currentMode === "plan" && modeOverride === "deny") {
       const planFilePath = permissionMode.getPlanFilePath();
-      if (planFilePath) {
-        reason =
-          `Plan mode is active. You can only use read-only tools (Read, Grep, Glob, etc.) and write to the plan file. ` +
-          `Write your plan to: ${planFilePath}. ` +
-          `Use ExitPlanMode when your plan is ready for user approval.`;
-      }
+      // planFilePath should always be set when in plan mode - they're set together
+      reason =
+        `Plan mode is active. You can only use read-only tools (Read, Grep, Glob, etc.) and write to the plan file. ` +
+        `Write your plan to: ${planFilePath || "(error: plan file path not configured)"}. ` +
+        `Use ExitPlanMode when your plan is ready for user approval.`;
     }
     return {
       decision: modeOverride,
diff --git a/src/tests/permissions-mode.test.ts b/src/tests/permissions-mode.test.ts
index d14c3df..b5761b3 100644
--- a/src/tests/permissions-mode.test.ts
+++ b/src/tests/permissions-mode.test.ts
@@ -273,7 +273,8 @@ test("plan mode - denies Write", () => {
 
   expect(result.decision).toBe("deny");
   expect(result.matchedRule).toBe("plan mode");
-  expect(result.reason).toBe("Permission mode: plan");
+  // Reason now includes detailed guidance (planFilePath not set in test, so shows error fallback)
+  expect(result.reason).toContain("Plan mode is active");
 });
 
 test("plan mode - denies Bash", () => {
@@ -362,7 +363,7 @@ test("Permission mode takes precedence over CLI allowedTools", () => {
 
   // Permission mode denies take precedence over CLI allowedTools
   expect(result.decision).toBe("deny");
-  expect(result.reason).toBe("Permission mode: plan");
+  expect(result.reason).toContain("Plan mode is active");
 
   // Clean up
   cliPermissions.clear();