From ee442cedeaa7de950173632c69293a04c7615753 Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 19 May 2026 02:09:47 +0000 Subject: [PATCH] fix(ask,plan): fix renderer crash and plan-mode exit loop for Qwen3 and similar models - Guard renderInlineMarkdown against non-string input: partial JSON during streaming can leave option label fields as undefined, causing marked.lexer to throw 'undefined is not an object (evaluating e.replace)'. The ask tool renderer now silently falls back to an empty string or baseColor output. - Sanitize normalizePlanTitle instead of hard-rejecting: models that produce natural-language plan titles like 'My Improvement Plan' were getting a ToolError on every resolve call, causing an infinite retry loop. Spaces are now converted to hyphens, remaining invalid chars are dropped, and only truly unresolvable titles (empty after sanitization, path separators) throw. - Fix ask.md prompt example: the example showed the legacy single-question format (question/options/recommended at the top level) while the schema requires questions: [{id, question, options}]. Models that follow examples closely (Qwen3) generated calls that always failed schema validation. Fixes #1176 --- packages/coding-agent/CHANGELOG.md | 3 ++ .../src/plan-mode/approved-plan.ts | 30 ++++++++---- .../coding-agent/src/prompts/tools/ask.md | 7 +-- .../test/plan-mode/approved-plan.test.ts | 46 ++++++++++++++++++- packages/tui/CHANGELOG.md | 4 ++ packages/tui/src/components/markdown.ts | 2 + packages/tui/test/markdown.test.ts | 16 +++++++ 7 files changed, 95 insertions(+), 13 deletions(-) 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", () => {