feat(coding-agent): enforced tool decision requirement in plan mode

- Enforced tool decision in plan mode--agent now requires calling either `ask` or `exit_plan_mode` when a turn ends without a required tool call. - Fixed cancellation behavior of `ask` tool to abort the current turn instead of returning a normal cancelled selection, while timeout-driven auto-cancel still returns without aborting. - Added plan-mode-tool-decision-reminder system prompt to guide agent when required tools are not called. - Improved agent_end event handling to use fallback assistant message when #lastAssistantMessage is unavailable.
2026-03-01 11:00:13 +01:00
parent 8663794c56
commit 3321cb8061
8 changed files with 128 additions and 69 deletions
@@ -1431,7 +1431,8 @@ mod tests {

 	#[test]
 	fn test_wrap_text_with_ansi_resets_strike_without_resetting_colors() {
-		let data = to_u16("\x1b[38;5;196m\x1b[48;5;236m\x1b[9mstrikethrough content wraps\x1b[29m\x1b[0m");
+		let data =
+			to_u16("\x1b[38;5;196m\x1b[48;5;236m\x1b[9mstrikethrough content wraps\x1b[29m\x1b[0m");
 		let lines = wrap_text_with_ansi_impl(&data, 12, DEFAULT_TAB_WIDTH);
 		assert!(lines.len() > 1);

@@ -1,8 +1,10 @@
 # Changelog

 ## [Unreleased]
+
 ### Added

+- Enforced tool decision in plan mode—agent now requires calling either `ask` or `exit_plan_mode` when a turn ends without a required tool call
 - Auto-correction of escaped tab indentation in edits (enabled by default, controllable via `PI_HASHLINE_AUTOCORRECT_ESCAPED_TABS` environment variable)
 - Warning when suspicious Unicode escape placeholder `\uDDDD` is detected in edit content

@@ -10,6 +12,10 @@

 - Updated hashline documentation to clarify that `\t` in JSON represents a real tab character, not a literal backslash-t sequence

+### Fixed
+
+- Cancelling the `ask` tool now aborts the current turn instead of returning a normal cancelled selection, while timeout-driven auto-cancel still returns without aborting
+
 ## [13.5.2] - 2026-03-01

 ### Added
@@ -255,7 +255,8 @@ handlebars.registerHelper("SECTION_SEPERATOR", (name: unknown): string => sectio
 */
 function formatHashlineRef(lineNum: unknown, content: unknown): { num: number; text: string; ref: string } {
 	const num = typeof lineNum === "number" ? lineNum : Number.parseInt(String(lineNum), 10);
-	const text = typeof content === "string" ? content : String(content ?? "");
+	const raw = typeof content === "string" ? content : String(content ?? "");
+	const text = raw.replace(/\\t/g, "\t").replace(/\\n/g, "\n").replace(/\\r/g, "\r");
 	const ref = `${num}#${computeLineHash(num, text)}`;
 	return { num, text, ref };
 }
@@ -0,0 +1,9 @@
+<system-reminder>
+Plan mode turn ended without a required tool call.
+
+You **MUST** choose exactly one next action now:
+1. Call `{{askToolName}}` to gather required clarification, OR
+2. Call `{{exitToolName}}` to finish planning and request approval
+
+You **MUST NOT** output plain text in this turn.
+</system-reminder>
@@ -49,6 +49,7 @@ Every edit has `op`, `pos`, and `lines`. Range replaces also have `end`. Both `p
   - You **MUST NOT** set `end` to an interior line and then re-add the boundary token in `lines`; that duplicates the next surviving line.
   - To remove a line while keeping its neighbors, **delete** it (`lines: null`). You **MUST NOT** replace it with the content of an adjacent line — that line still exists and will be duplicated.
 4. **Match surrounding indentation:** Leading whitespace in `lines` **MUST** be copied verbatim from adjacent lines in the `read` output. Do not infer or reconstruct indentation from memory — count the actual leading spaces on the lines immediately above and below the insertion or replacement point.
+5. **Preserve idiomatic sibling spacing:** When inserting declarations between top-level siblings, you **MUST** preserve existing blank-line separators. If siblings are separated by one blank line, include a trailing `""` in `lines` so inserted code keeps the same spacing.
 </rules>

 <recovery>
@@ -121,18 +122,17 @@ Range — add `end`:
 {{hlinefull 62 "      return null;"}}
 {{hlinefull 63 "    }"}}
 ```
+Target only the inner lines that change — leave unchanged boundaries out of the range.
 ```
 {
  path: "…",
  edits: [{
    op: "replace",
-    pos: {{hlinejsonref 60 "    } catch (err) {"}},
-    end: {{hlinejsonref 63 "    }"}},
+    pos: {{hlinejsonref 61 "      console.error(err);"}},
+    end: {{hlinejsonref 62 "      return null;"}},
    lines: [
-      "    } catch (err) {",
      "      if (isEnoent(err)) return null;",
-      "      throw err;",
-      "    }"
+      "      throw err;"
    ]
  }]
 }
@@ -141,39 +141,24 @@ Range — add `end`:

 <example name="inclusive end avoids duplicate boundary">
 ```ts
-{{hlinefull 70 "if (ok) {"}}
-{{hlinefull 71 "  run();"}}
-{{hlinefull 72 "}"}}
-{{hlinefull 73 "after();"}}
+{{hlinefull 70 "\tif (user.isAdmin) {"}}
+{{hlinefull 71 "\t\tdeleteRecord(id);"}}
+{{hlinefull 72 "\t}"}}
+{{hlinefull 73 "\tafter();"}}
 ```
-Bad — `end` stops before `}` while `lines` already includes `}`:
+The block grows by one line and the condition changes — two single-line ops would be needed otherwise. Since `}` appears in `lines`, `end` must include `72`:
 ```
 {
  path: "…",
  edits: [{
    op: "replace",
-    pos: {{hlinejsonref 70 "if (ok) {"}},
-    end: {{hlinejsonref 71 "  run();"}},
+    pos: {{hlinejsonref 70 "\tif (user.isAdmin) {"}},
+    end: {{hlinejsonref 72 "\t}"}},
    lines: [
-      "if (ok) {",
-      "  runSafe();",
-      "}"
-    ]
-  }]
-}
-```
-Good — include original `}` in the replaced range when replacement keeps `}`:
-```
-{
-  path: "…",
-  edits: [{
-    op: "replace",
-    pos: {{hlinejsonref 70 "if (ok) {"}},
-    end: {{hlinejsonref 72 "}"}},
-    lines: [
-      "if (ok) {",
-      "  runSafe();",
-      "}"
+      "\tif (user.isAdmin && confirmed) {",
+      "\t\tauditLog(id);",
+      "\t\tdeleteRecord(id);",
+      "\t}"
    ]
  }]
 }
@@ -191,6 +176,7 @@ Also apply the same rule to `);`, `],`, and `},` closers: if replacement include
 {{hlinefull 49 "  runY();"}}
 {{hlinefull 50 "}"}}
 ```
+Use a trailing `""` to preserve the blank line between top-level sibling declarations.
 ```
 {
  path: "…",
@@ -206,20 +192,6 @@ Also apply the same rule to `);`, `],`, and `},` closers: if replacement include
  }]
 }
 ```
-Result:
-```ts
-{{hlinefull 44 "function x() {"}}
-{{hlinefull 45 "  runX();"}}
-{{hlinefull 46 "}"}}
-{{hlinefull 47 ""}}
-{{hlinefull 48 "function z() {"}}
-{{hlinefull 49 "  runZ();"}}
-{{hlinefull 50 "}"}}
-{{hlinefull 51 ""}}
-{{hlinefull 52 "function y() {"}}
-{{hlinefull 53 "  runY();"}}
-{{hlinefull 54 "}"}}
-```
 </example>

 <example name="anchor to structure, not whitespace">
@@ -255,7 +227,7 @@ Leading whitespace in `lines` **MUST** be copied from the `read` output, not rec
 {{hlinefull 11 "\tbar() {"}}
 {{hlinefull 12 "\t\treturn 1;"}}
 {{hlinefull 13 "\t}"}}
-{{hlinefull 14 "}}"}}
+{{hlinefull 14 "}"}}
 ```
 Bad — indent guessed as spaces; `\\t` emits literal backslash-t:
 ```
@@ -82,6 +82,9 @@ import { normalizeDiff, normalizeToLF, ParseError, previewPatch, stripBom } from
 import type { PlanModeState } from "../plan-mode/state";
 import planModeActivePrompt from "../prompts/system/plan-mode-active.md" with { type: "text" };
 import planModeReferencePrompt from "../prompts/system/plan-mode-reference.md" with { type: "text" };
+import planModeToolDecisionReminderPrompt from "../prompts/system/plan-mode-tool-decision-reminder.md" with {
+	type: "text",
+};
 import ttsrInterruptTemplate from "../prompts/system/ttsr-interrupt.md" with { type: "text" };
 import type { SecretObfuscator } from "../secrets/obfuscator";
 import type { CheckpointState } from "../tools/checkpoint";
@@ -733,9 +736,13 @@ export class AgentSession {
 		}

 		// Check auto-retry and auto-compaction after agent completes
-		if (event.type === "agent_end" && this.#lastAssistantMessage) {
-			const msg = this.#lastAssistantMessage;
+		if (event.type === "agent_end") {
+			const fallbackAssistant = [...event.messages]
+				.reverse()
+				.find((message): message is AssistantMessage => message.role === "assistant");
+			const msg = this.#lastAssistantMessage ?? fallbackAssistant;
 			this.#lastAssistantMessage = undefined;
+			if (!msg) return;

 			if (this.#skipPostTurnMaintenanceAssistantTimestamp === msg.timestamp) {
 				this.#skipPostTurnMaintenanceAssistantTimestamp = undefined;
@@ -1901,6 +1908,9 @@ export class AgentSession {
 			: { role: "user" as const, content: userContent, timestamp: Date.now() };

 		await this.#promptWithMessage(message, expandedText, options);
+		if (!options?.synthetic) {
+			await this.#enforcePlanModeToolDecision();
+		}
 	}

 	async promptCustomMessage<T = unknown>(
@@ -3329,6 +3339,52 @@ Be thorough - include exact file paths, function names, error messages, and tech
 		this.#checkpointState = undefined;
 		this.#pendingRewindReport = undefined;
 	}
+	async #enforcePlanModeToolDecision(): Promise<void> {
+		if (!this.#planModeState?.enabled) {
+			return;
+		}
+		const assistantMessage = this.#findLastAssistantMessage();
+		if (!assistantMessage) {
+			return;
+		}
+		if (assistantMessage.stopReason === "error" || assistantMessage.stopReason === "aborted") {
+			return;
+		}
+
+		const calledRequiredTool = assistantMessage.content.some(
+			content => content.type === "toolCall" && (content.name === "ask" || content.name === "exit_plan_mode"),
+		);
+		if (calledRequiredTool) {
+			return;
+		}
+
+		const askTool = this.#toolRegistry.get("ask");
+		const exitPlanModeTool = this.#toolRegistry.get("exit_plan_mode");
+		if (!askTool || !exitPlanModeTool) {
+			logger.warn("Plan mode enforcement skipped because ask/exit tools are unavailable", {
+				activeToolNames: this.agent.state.tools.map(tool => tool.name),
+			});
+			return;
+		}
+		const forcedTools = [askTool, exitPlanModeTool];
+
+		const reminder = renderPromptTemplate(planModeToolDecisionReminderPrompt, {
+			askToolName: "ask",
+			exitToolName: "exit_plan_mode",
+		});
+
+		const previousTools = this.agent.state.tools;
+		this.agent.setTools(forcedTools);
+		try {
+			await this.prompt(reminder, {
+				synthetic: true,
+				expandPromptTemplates: false,
+				toolChoice: "required",
+			});
+		} finally {
+			this.agent.setTools(previousTools);
+		}
+	}
 	/**
 	 * Check if agent stopped with incomplete todos and prompt to continue.
 	 */
@@ -86,9 +86,10 @@ export function formatPromptContent(content: string, options: PromptFormatOption

 	for (let i = 0; i < lines.length; i++) {
 		let line = lines[i].trimEnd();
-		const trimmed = line.trimStart();
+		const trimmed = line.trim();
+		const trimmedStart = line.trimStart();

-		if (CODE_FENCE.test(trimmed)) {
+		if (CODE_FENCE.test(trimmedStart)) {
 			inCodeBlock = !inCodeBlock;
 			result.push(line);
 			continue;
@@ -103,29 +104,26 @@ export function formatPromptContent(content: string, options: PromptFormatOption
 			line = replaceCommonAsciiSymbols(line);
 		}

-		const isOpeningXml = OPENING_XML.test(trimmed) && !trimmed.endsWith("/>");
-		if (isOpeningXml && line.length === trimmed.length) {
-			const match = OPENING_XML.exec(trimmed);
+		const isOpeningXml = OPENING_XML.test(trimmedStart) && !trimmedStart.endsWith("/>");
+		if (isOpeningXml && line.length === trimmedStart.length) {
+			const match = OPENING_XML.exec(trimmedStart);
 			if (match) topLevelTags.push(match[1]);
 		}

-		const closingMatch = CLOSING_XML.exec(trimmed);
+		const closingMatch = CLOSING_XML.exec(trimmedStart);
 		if (closingMatch) {
 			const tagName = closingMatch[1];
 			if (topLevelTags.length > 0 && topLevelTags[topLevelTags.length - 1] === tagName) {
-				line = trimmed;
 				topLevelTags.pop();
-			} else {
-				line = line.trimEnd();
 			}
-		} else if (isPreRender && trimmed.startsWith("{{")) {
-			line = trimmed;
-		} else if (TABLE_SEP.test(trimmed)) {
-			line = compactTableSep(trimmed);
-		} else if (TABLE_ROW.test(trimmed)) {
-			line = compactTableRow(trimmed);
-		} else {
-			line = line.trimEnd();
+		} else if (isPreRender && trimmedStart.startsWith("{{")) {
+			/* keep indentation as-is in pre-render for Handlebars markers */
+		} else if (TABLE_SEP.test(trimmedStart)) {
+			const leadingWhitespace = line.slice(0, line.length - trimmedStart.length);
+			line = `${leadingWhitespace}${compactTableSep(trimmedStart)}`;
+		} else if (TABLE_ROW.test(trimmedStart)) {
+			const leadingWhitespace = line.slice(0, line.length - trimmedStart.length);
+			line = `${leadingWhitespace}${compactTableRow(trimmedStart)}`;
 		}

 		if (shouldBoldRfc2119) {
@@ -2,12 +2,28 @@ import { describe, expect, test } from "bun:test";
 import { formatPromptContent } from "@oh-my-pi/pi-coding-agent/utils/prompt-format";

 describe("formatPromptContent renderPhase", () => {
-	test("pre-render mode strips indentation from Handlebars block lines", () => {
+	test("pre-render preserves indentation on Handlebars block lines", () => {
 		const input = "<root>\n  {{#if ok}}\n    value\n  {{/if}}\n</root>";

 		const output = formatPromptContent(input, { renderPhase: "pre-render" });

-		expect(output).toBe("<root>\n{{#if ok}}\n    value\n{{/if}}\n</root>");
+		expect(output).toBe("<root>\n  {{#if ok}}\n    value\n  {{/if}}\n</root>");
+	});
+
+	test("pre-render preserves leading tabs", () => {
+		const input = "\t<root>\n\t  {{#if ok}}\n\t    value\n\t  {{/if}}\n</root>";
+
+		const output = formatPromptContent(input, { renderPhase: "pre-render" });
+
+		expect(output).toBe(input);
+	});
+
+	test("pre-render trims trailing whitespace", () => {
+		const input = "\t<root>   \n\t  {{#if ok}}\t\n\t    value   \n\t  {{/if}} \n</root>";
+
+		const output = formatPromptContent(input, { renderPhase: "pre-render" });
+
+		expect(output).toBe("\t<root>\n\t  {{#if ok}}\n\t    value\n\t  {{/if}}\n</root>");
 	});

 	test("post-render mode preserves indentation on Handlebars-like lines", () => {