diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 775437f92..c68665521 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -4,6 +4,9 @@ ### Fixed +- Fixed `normalizePlanTitle` rejecting plan titles that contain spaces or common punctuation (e.g. "My Feature Plan") — spaces are now converted to hyphens and other invalid characters are dropped, so models that produce natural-language plan titles no longer loop forever trying to call `resolve`. ([#1176](https://github.com/can1357/oh-my-pi/issues/1176)) +- Fixed `ask` tool prompt example showing the legacy `question`/`options` top-level format instead of the current `questions: [{id, question, options}]` array format; models that closely followed the example generated calls that always failed schema validation. ([#1176](https://github.com/can1357/oh-my-pi/issues/1176)) + - Fixed ACP command and custom tool-call notifications to carry the original tool arguments in replayed and final updates, so command text is preserved and raw input is no longer wrapped - Fixed ACP async-job draining to be scoped by session owner so `getAsyncJobSnapshot` and `drainAsyncJobDeliveriesForAcp` no longer consume or expose jobs from other sessions - Fixed async job status reporting to include in-flight completions so queued/delivering indicators remain accurate while callbacks are still running diff --git a/packages/coding-agent/src/plan-mode/approved-plan.ts b/packages/coding-agent/src/plan-mode/approved-plan.ts index 1e0d3388d..4733b837a 100644 --- a/packages/coding-agent/src/plan-mode/approved-plan.ts +++ b/packages/coding-agent/src/plan-mode/approved-plan.ts @@ -14,10 +14,12 @@ export interface PlanApprovalDetails { planExists: boolean; } -/** Validate the agent-supplied plan title and derive the destination filename. - * Filename uses the title with a `.md` suffix; characters are restricted to - * letters, numbers, underscores, and hyphens so the value is safe to splice - * into a `local://` URL without escaping. */ +/** Validate and normalize the agent-supplied plan title into a safe filename stem. + * Spaces and other URL-safe punctuation are replaced with hyphens so models that + * produce natural-language titles (e.g. "My feature plan") still succeed. + * Characters that cannot be safely represented after replacement are dropped. + * The result is restricted to letters, numbers, underscores, and hyphens so it + * is safe to splice into a `local://` URL without escaping. */ export function normalizePlanTitle(title: string): { title: string; fileName: string } { const trimmed = title.trim(); if (!trimmed) { @@ -28,13 +30,23 @@ export function normalizePlanTitle(title: string): { title: string; fileName: st throw new ToolError("Plan title must not contain path separators or '..'."); } - const withExtension = trimmed.toLowerCase().endsWith(".md") ? trimmed : `${trimmed}.md`; - if (!/^[A-Za-z0-9_-]+\.md$/.test(withExtension)) { - throw new ToolError("Plan title may only contain letters, numbers, underscores, or hyphens."); + // Strip a trailing `.md` if the model included it, then sanitize: + // spaces → hyphens, any remaining invalid char → dropped. + const withoutExt = trimmed.replace(/\.md$/i, ""); + const sanitized = withoutExt + .replace(/\s+/g, "-") + .replace(/[^A-Za-z0-9_-]/g, "") + .replace(/-{2,}/g, "-") + .replace(/^-+|-+$/g, ""); + + if (!sanitized) { + throw new ToolError( + "Plan title must contain at least one letter, number, underscore, or hyphen after sanitization.", + ); } - const normalizedTitle = withExtension.slice(0, -3); - return { title: normalizedTitle, fileName: withExtension }; + const fileName = `${sanitized}.md`; + return { title: sanitized, fileName }; } /** Humanize a normalized plan title for use as a session display name. diff --git a/packages/coding-agent/src/prompts/tools/ask.md b/packages/coding-agent/src/prompts/tools/ask.md index 9620922bb..631ac3356 100644 --- a/packages/coding-agent/src/prompts/tools/ask.md +++ b/packages/coding-agent/src/prompts/tools/ask.md @@ -22,7 +22,8 @@ Asks user when you need clarification or input during task execution. # Single question -question: "Which authentication method should this API use?" -options: [{"label": "JWT"}, {"label": "OAuth2"}, {"label": "Session cookies"}] -recommended: 0 +questions: [{"id": "auth_method", "question": "Which authentication method should this API use?", "options": [{"label": "JWT"}, {"label": "OAuth2"}, {"label": "Session cookies"}], "recommended": 0}] + +# Multiple questions +questions: [{"id": "storage_type", "question": "Which storage backend?", "options": [{"label": "SQLite"}, {"label": "PostgreSQL"}]}, {"id": "auth_method", "question": "Which auth method?", "options": [{"label": "JWT"}, {"label": "Session cookies"}]}] diff --git a/packages/coding-agent/test/plan-mode/approved-plan.test.ts b/packages/coding-agent/test/plan-mode/approved-plan.test.ts index 327c02e22..6d3c66185 100644 --- a/packages/coding-agent/test/plan-mode/approved-plan.test.ts +++ b/packages/coding-agent/test/plan-mode/approved-plan.test.ts @@ -2,7 +2,7 @@ import { afterEach, beforeEach, describe, expect, it } from "bun:test"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; -import { humanizePlanTitle, renameApprovedPlanFile } from "@oh-my-pi/pi-coding-agent/plan-mode/approved-plan"; +import { humanizePlanTitle, normalizePlanTitle, renameApprovedPlanFile } from "@oh-my-pi/pi-coding-agent/plan-mode/approved-plan"; describe("renameApprovedPlanFile", () => { let tmpDir: string; @@ -62,3 +62,47 @@ describe("humanizePlanTitle", () => { expect(humanizePlanTitle("---")).toBe(""); }); }); + +describe("normalizePlanTitle", () => { + it("accepts a clean identifier as-is", () => { + expect(normalizePlanTitle("my-plan")).toEqual({ title: "my-plan", fileName: "my-plan.md" }); + expect(normalizePlanTitle("feature_branch")).toEqual({ title: "feature_branch", fileName: "feature_branch.md" }); + }); + + it("strips a trailing .md suffix provided by the model", () => { + expect(normalizePlanTitle("my-plan.md")).toEqual({ title: "my-plan", fileName: "my-plan.md" }); + }); + + it("converts spaces to hyphens (natural-language titles)", () => { + expect(normalizePlanTitle("My Improvement Plan")).toEqual({ + title: "My-Improvement-Plan", + fileName: "My-Improvement-Plan.md", + }); + }); + + it("collapses consecutive spaces / resulting hyphens", () => { + expect(normalizePlanTitle("foo bar")).toEqual({ title: "foo-bar", fileName: "foo-bar.md" }); + }); + + it("drops characters outside the allowed set after space replacement", () => { + expect(normalizePlanTitle("plan: v1.0 (draft)")).toEqual({ title: "plan-v10-draft", fileName: "plan-v10-draft.md" }); + }); + + it("trims leading/trailing hyphens that result from sanitization", () => { + expect(normalizePlanTitle("!!! plan !!!")).toEqual({ title: "plan", fileName: "plan.md" }); + }); + + it("throws for empty title", () => { + expect(() => normalizePlanTitle("")).toThrow("Plan title is required"); + expect(() => normalizePlanTitle(" ")).toThrow("Plan title is required"); + }); + + it("throws for path separators", () => { + expect(() => normalizePlanTitle("../etc/passwd")).toThrow("path separators"); + expect(() => normalizePlanTitle("a/b")).toThrow("path separators"); + }); + + it("throws when sanitization produces empty result", () => { + expect(() => normalizePlanTitle("!!!")).toThrow("at least one letter"); + }); +}); diff --git a/packages/tui/CHANGELOG.md b/packages/tui/CHANGELOG.md index 0a133389b..b536d5622 100644 --- a/packages/tui/CHANGELOG.md +++ b/packages/tui/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `renderInlineMarkdown` crashing with `TypeError: undefined is not an object (evaluating 'e.replace')` when called with a non-string value during streaming — partial JSON parsing leaves option label fields temporarily unpopulated, causing the ask tool renderer to fail. ([#1176](https://github.com/can1357/oh-my-pi/issues/1176)) + ## [15.0.2] - 2026-05-15 ### Added diff --git a/packages/tui/src/components/markdown.ts b/packages/tui/src/components/markdown.ts index d6b651946..65360b4de 100644 --- a/packages/tui/src/components/markdown.ts +++ b/packages/tui/src/components/markdown.ts @@ -946,6 +946,8 @@ export class Markdown implements Component { * Unlike the full Markdown component, this produces a single line with no block-level elements. */ export function renderInlineMarkdown(text: string, mdTheme: MarkdownTheme, baseColor?: (t: string) => string): string { + // Guard against undefined/null during streaming — partial JSON can leave fields unpopulated. + if (typeof text !== "string") return (baseColor ?? (t => t))(text != null ? String(text) : ""); const tokens = marked.lexer(text); const applyText = baseColor ?? ((t: string) => t); let result = ""; diff --git a/packages/tui/test/markdown.test.ts b/packages/tui/test/markdown.test.ts index 2eba2bc9e..f6f84f1bd 100644 --- a/packages/tui/test/markdown.test.ts +++ b/packages/tui/test/markdown.test.ts @@ -27,6 +27,22 @@ describe("renderInlineMarkdown", () => { expect(plain).toBe("1. Review against a base branch (PR Style)"); }); + + it("returns empty string for undefined input (streaming guard)", () => { + // During streaming, partial JSON can leave option label fields as undefined. + // renderInlineMarkdown must not throw in that case. + const rendered = renderInlineMarkdown(undefined as unknown as string, defaultMarkdownTheme); + expect(rendered).toBe(""); + }); + + it("applies baseColor to fallback for non-string input", () => { + const rendered = renderInlineMarkdown( + null as unknown as string, + defaultMarkdownTheme, + t => `[${t}]`, + ); + expect(rendered).toBe("[]"); + }); }); describe("Markdown component", () => {