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
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -22,7 +22,8 @@ Asks user when you need clarification or input during task execution.
|
||||
|
||||
<examples>
|
||||
# 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"}]}]
|
||||
</examples>
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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 = "";
|
||||
|
||||
@@ -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", () => {
|
||||
|
||||
Reference in New Issue
Block a user