diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 887d77666..03e8094fd 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -47,6 +47,7 @@ - Fixed bash internal-URL expansion skipping unquoted `skill://` (and other supported schemes) inside a legacy backtick command substitution nested directly in double quotes (e.g. ``echo "`cat skill://valid-skill/SKILL.md`"``); `isInsideShellQuote` now treats `` ` `` as an expansion-context boundary like `$()`, including `$()`/backtick nesting in either order, while single-quoted and escaped-backtick text stay literal ([#5645](https://github.com/can1357/oh-my-pi/issues/5645)). - Fixed `omp say` playing no audio for a short single-segment clip on hosts where the first streaming backend (the bundled ffmpeg built without pulse/alsa output) spawns then exits nonzero: the pipe write succeeds before that death and `player.end()` has already closed the input, so neither the broken-pipe replay nor the early-exit handler advanced to `paplay`/`aplay`. `StreamingAudioPlayer` now retains the utterance PCM and, when the streaming backend exits nonzero, replays it through per-file playback so short clips still reach the speakers ([#5875](https://github.com/can1357/oh-my-pi/issues/5875)). - Fixed mid-session `memory.backend` changes leaving runtime state, tools, listeners, and prompt context on different backends; Mnemopi clear/enqueue now rehydrate listeners, and legacy `memories.enabled` no longer activates the local pipeline after migration ([#5638](https://github.com/can1357/oh-my-pi/issues/5638)). +- Fixed Plan Review annotations being discarded on dismissal and limited to headings; review notes now persist per plan, feed Refine, and can target the top visible plan line. - Fixed `error.notify` raising a "Stopped with error" toast for provider failures while an auto-retry or async-delivery continuation was pending; the toast now waits for the true terminal settle. - Fixed concurrent MCP config mutations losing updates and racing on a shared temp path: every `mcp.json` read-modify-write (add/update/remove server, disabled/force-enabled lists) is now serialized under a per-file lock, and each atomic write uses a unique temp file so overlapping writers no longer rename each other's `.tmp` out from under them (ENOENT or clobbered config) — reachable in-process via the fire-and-forget extensions-dashboard toggle and across processes on a shared `~/.omp/mcp.json` ([#4104](https://github.com/can1357/oh-my-pi/issues/4104)). - Fixed transient provider stream stalls after tool calls failing to auto-retry even when every call already had a tool result, including synthetic `executed:false` results from OpenAI-completions stalls ([#6414](https://github.com/can1357/oh-my-pi/issues/6414)). diff --git a/packages/coding-agent/src/modes/components/plan-review-overlay.ts b/packages/coding-agent/src/modes/components/plan-review-overlay.ts index 75e37a2a5..8e2bd9687 100644 --- a/packages/coding-agent/src/modes/components/plan-review-overlay.ts +++ b/packages/coding-agent/src/modes/components/plan-review-overlay.ts @@ -23,11 +23,14 @@ import { Markdown, type MarkdownTheme, matchesKey, + replaceTabs, routeSgrMouseInput, ScrollView, truncateToWidth, visibleWidth, } from "@oh-my-pi/pi-tui"; +import { sanitizeText } from "@oh-my-pi/pi-utils"; +import { sanitizeStatusText } from "../shared"; import { getMarkdownTheme, theme } from "../theme/theme"; import { matchesAppExternalEditor, @@ -58,22 +61,59 @@ const MIN_BODY_ROWS = 3; const SIDEBAR_MIN_HEADINGS = 2; const SIDEBAR_MIN_TOTAL_WIDTH = 64; const SIDEBAR_MIN_BODY_WIDTH = 40; +/** Persisted line-context cap; render-time captions clamp again to the viewport. */ +const MAX_ANNOTATION_CONTEXT_WIDTH = 120; type Focus = "toc" | "body" | "actions"; +type AnnotationTarget = { kind: "section" } | { kind: "line"; row: number; context: string; contextTruncated: boolean }; + +interface OverlayAnnotation { + note: string; + target: AnnotationTarget; +} + interface OverlaySection { level: number; title: string; raw: string; md: Markdown; - annotations: string[]; + annotations: OverlayAnnotation[]; +} + +interface LineAnchorContext { + text: string; + truncated: boolean; +} + +interface BodyRowAnchor { + sectionIndex: number; + row: number; + context: string; + contextTruncated: boolean; +} + +/** Serializable annotations retained by the plan-review owner between overlays. */ +export interface PlanReviewAnnotationState { + annotations: Array<{ + section: { + index: number; + title: string; + /** Heading ancestry from the document root, when emitted by this overlay. */ + path?: string[]; + /** Hash of the section source, used to reject ambiguous moved headings. */ + contentHash?: string; + }; + target: { kind: "section" } | { kind: "line"; row: number; context: string; contextTruncated?: boolean }; + note: string; + }>; } /** Undo snapshot: joined plan text, annotations aligned by section, and the * accumulated deleted-section feedback at the time of the snapshot. */ interface UndoEntry { text: string; - annotations: string[][]; + annotations: OverlayAnnotation[][]; deleted: string[]; } @@ -92,6 +132,8 @@ export interface PlanReviewOverlayCallbacks { onPlanEdited?: (content: string) => void; /** Invoked with the Refine feedback markdown whenever annotations change. */ onFeedbackChange?: (feedback: string) => void; + /** Invoked with a serializable annotation snapshot whenever annotations change. */ + onAnnotationStateChange?: (state: PlanReviewAnnotationState) => void; } export interface PlanReviewOverlayOptions { @@ -108,6 +150,8 @@ export interface PlanReviewOverlayOptions { slider?: HookSelectorSlider; /** Display label for the external-editor key, surfaced in the footer help. */ externalEditorLabel?: string; + /** Serializable annotations restored into this overlay instance. */ + annotationState?: PlanReviewAnnotationState; } /** Default trailing footer hint when the caller supplies none. */ @@ -121,6 +165,8 @@ export class PlanReviewOverlay implements Component { /** Shallowest level among ToC entries, used to flatten indentation. */ #tocBaseLevel = 1; #sectionOffsets: number[] = []; + /** Rendered body row to underlying plan row; callouts retain their owner's anchor. */ + #bodyRowAnchors: BodyRowAnchor[] = []; #undo: UndoEntry[] = []; /** Titles of sections deleted in the overlay, surfaced as Refine feedback. */ #deleted: string[] = []; @@ -162,6 +208,7 @@ export class PlanReviewOverlay implements Component { #committedLabel: string | undefined; #annotating = false; #input: Input; + #annotationTarget: BodyRowAnchor | { sectionIndex: number; row: null; context: null } | undefined; constructor( planContent: string, @@ -194,6 +241,10 @@ export class PlanReviewOverlay implements Component { this.#input.onSubmit = value => this.#submitAnnotation(value); this.#input.onEscape = () => this.#exitAnnotate(); this.#setSections(planContent); + this.#restoreAnnotationState(options.annotationState); + if (Array.isArray(options.annotationState?.annotations) && options.annotationState.annotations.length > 0) { + this.#recomputeFeedback(); + } } invalidate(): void { @@ -204,7 +255,9 @@ export class PlanReviewOverlay implements Component { * reset scroll/focus so the operator starts at the top. Does not emit * `onPlanEdited` (the editor round-trip already persisted the file). */ setPlanContent(planContent: string): void { + const annotations = this.#annotationState(); this.#setSections(planContent); + this.#restoreAnnotationState(annotations); this.#scrollView.scrollToTop(); this.#scrollProgress = 0; this.#tocCursor = 0; @@ -220,11 +273,148 @@ export class PlanReviewOverlay implements Component { title: section.title, raw: section.raw, md: new Markdown(section.raw, 1, 0, this.#mdTheme), - annotations: [] as string[], + annotations: [], })); this.#rebuildToc(); this.#tocCursor = Math.min(this.#tocCursor, Math.max(0, this.#toc.length - 1)); } + #cloneAnnotation(annotation: OverlayAnnotation): OverlayAnnotation { + return { + note: annotation.note, + target: + annotation.target.kind === "section" + ? { kind: "section" } + : { + kind: "line", + row: annotation.target.row, + context: annotation.target.context, + contextTruncated: annotation.target.contextTruncated, + }, + }; + } + + #sectionPaths(): string[][] { + const stack: Array<{ level: number; title: string }> = []; + return this.#sections.map(section => { + if (section.level < 1) return []; + while (stack.length > 0 && stack[stack.length - 1]!.level >= section.level) stack.pop(); + stack.push({ level: section.level, title: section.title }); + return stack.map(entry => entry.title); + }); + } + + #sectionContentHash(section: OverlaySection): string { + return `${section.raw.length}:${Bun.hash(section.raw).toString(16)}`; + } + + #annotationState(): PlanReviewAnnotationState { + const annotations: PlanReviewAnnotationState["annotations"] = []; + const sectionPaths = this.#sectionPaths(); + for (let sectionIndex = 0; sectionIndex < this.#sections.length; sectionIndex++) { + const section = this.#sections[sectionIndex]!; + for (const annotation of section.annotations) { + annotations.push({ + section: { + index: sectionIndex, + title: section.title, + path: sectionPaths[sectionIndex]!, + contentHash: this.#sectionContentHash(section), + }, + target: + annotation.target.kind === "section" + ? { kind: "section" } + : { + kind: "line", + row: annotation.target.row, + context: annotation.target.context, + contextTruncated: annotation.target.contextTruncated, + }, + note: annotation.note, + }); + } + } + return { annotations }; + } + + #restoreAnnotationState(state: PlanReviewAnnotationState | undefined): void { + if (!state || !Array.isArray(state.annotations)) return; + const sectionPaths = this.#sectionPaths(); + const contentHashes = this.#sections.map(section => this.#sectionContentHash(section)); + for (const entry of state.annotations) { + if ( + !entry || + typeof entry.note !== "string" || + !entry.section || + !entry.target || + typeof entry.section.title !== "string" + ) { + continue; + } + const note = entry.note.trim(); + if (!note) continue; + const storedIndex = Number.isInteger(entry.section.index) ? entry.section.index : -1; + const storedPath = entry.section.path; + let matchingSections: number[]; + if ( + Array.isArray(storedPath) && + storedPath.every(segment => typeof segment === "string") && + typeof entry.section.contentHash === "string" + ) { + matchingSections = []; + for (let i = 0; i < this.#sections.length; i++) { + const path = sectionPaths[i]!; + if ( + this.#sections[i]!.title === entry.section.title && + contentHashes[i] === entry.section.contentHash && + path.length === storedPath.length && + path.every((segment, pathIndex) => segment === storedPath[pathIndex]) + ) { + matchingSections.push(i); + } + } + } else { + matchingSections = + storedIndex >= 0 && + storedIndex < this.#sections.length && + this.#sections[storedIndex]!.title === entry.section.title + ? [storedIndex] + : []; + } + if (matchingSections.length === 0) continue; + const sectionIndex = matchingSections.reduce((best, candidate) => + Math.abs(candidate - storedIndex) < Math.abs(best - storedIndex) ? candidate : best, + ); + const section = this.#sections[sectionIndex]!; + if (entry.target.kind === "section") { + if (section.level >= 1) section.annotations.push({ note, target: { kind: "section" } }); + continue; + } + if ( + entry.target.kind !== "line" || + !Number.isFinite(entry.target.row) || + typeof entry.target.context !== "string" + ) { + continue; + } + const contexts = section.md.render(MAX_ANNOTATION_CONTEXT_WIDTH).map(line => this.#lineContext(line)); + const row = this.#resolveLineRow( + entry.target.row, + { text: entry.target.context, truncated: entry.target.contextTruncated === true }, + contexts, + ); + if (row < 0) continue; + const context = contexts[row]!; + section.annotations.push({ + note, + target: { + kind: "line", + row, + context: context.text, + contextTruncated: context.truncated, + }, + }); + } + } #rebuildToc(): void { const headings: number[] = []; @@ -443,6 +633,10 @@ export class PlanReviewOverlay implements Component { } #handleBody(data: string): void { + if (data === "a") { + this.#startBodyAnnotate(); + return; + } if (matchesKey(data, "left") || matchesKey(data, "h")) { if (this.#sidebarShown) this.#setFocus("toc"); return; @@ -529,7 +723,7 @@ export class PlanReviewOverlay implements Component { return; } if (data === "a") { - this.#startAnnotate(); + this.#startSectionAnnotate(); return; } if (data === "u") { @@ -577,7 +771,9 @@ export class PlanReviewOverlay implements Component { #pushUndo(): void { this.#undo.push({ text: joinPlanSections(this.#sections), - annotations: this.#sections.map(section => [...section.annotations]), + annotations: this.#sections.map(section => + section.annotations.map(annotation => this.#cloneAnnotation(annotation)), + ), deleted: [...this.#deleted], }); } @@ -607,7 +803,8 @@ export class PlanReviewOverlay implements Component { if (!entry) return; this.#setSections(entry.text); for (let i = 0; i < this.#sections.length; i++) { - this.#sections[i]!.annotations = entry.annotations[i] ? [...entry.annotations[i]!] : []; + this.#sections[i]!.annotations = + entry.annotations[i]?.map(annotation => this.#cloneAnnotation(annotation)) ?? []; } this.#deleted = [...entry.deleted]; this.#tocCursor = Math.min(this.#tocCursor, Math.max(0, this.#toc.length - 1)); @@ -616,8 +813,21 @@ export class PlanReviewOverlay implements Component { this.#recomputeFeedback(); } - #startAnnotate(): void { - if (this.#toc[this.#tocCursor] === undefined) return; + #startSectionAnnotate(): void { + const sectionIndex = this.#toc[this.#tocCursor]; + if (sectionIndex === undefined) return; + this.#startAnnotate({ sectionIndex, row: null, context: null }); + } + + #startBodyAnnotate(): void { + const maxRow = this.#bodyRowAnchors.length - 1; + if (maxRow < 0) return; + const topRow = Math.max(0, Math.min(maxRow, Math.floor(this.#scrollView.getScrollOffset()))); + this.#startAnnotate(this.#bodyRowAnchors[topRow]!); + } + + #startAnnotate(target: BodyRowAnchor | { sectionIndex: number; row: null; context: null }): void { + this.#annotationTarget = target; this.#annotating = true; this.#input.setValue(""); } @@ -625,10 +835,23 @@ export class PlanReviewOverlay implements Component { #submitAnnotation(value: string): void { this.#annotating = false; const note = value.trim(); - const sectionIndex = this.#toc[this.#tocCursor]; - if (note && sectionIndex !== undefined) { + const target = this.#annotationTarget; + this.#annotationTarget = undefined; + const section = target ? this.#sections[target.sectionIndex] : undefined; + if (note && section && target) { this.#pushUndo(); - this.#sections[sectionIndex]!.annotations.push(note); + section.annotations.push({ + note, + target: + target.row === null + ? { kind: "section" } + : { + kind: "line", + row: target.row, + context: target.context, + contextTruncated: target.contextTruncated, + }, + }); this.#recomputeFeedback(); } this.#input.setValue(""); @@ -636,11 +859,13 @@ export class PlanReviewOverlay implements Component { #exitAnnotate(): void { this.#annotating = false; + this.#annotationTarget = undefined; this.#input.setValue(""); } #recomputeFeedback(): void { - const annotated = this.#sections.filter(section => section.level >= 1 && section.annotations.length > 0); + this.callbacks.onAnnotationStateChange?.(this.#annotationState()); + const annotated = this.#sections.filter(section => section.annotations.length > 0); if (annotated.length === 0 && this.#deleted.length === 0) { this.callbacks.onFeedbackChange?.(""); return; @@ -651,8 +876,11 @@ export class PlanReviewOverlay implements Component { for (const title of this.#deleted) feedback += `- ${title}\n`; } for (const section of annotated) { - feedback += `\n## ${section.title}\n`; - for (const note of section.annotations) feedback += this.#formatAnnotationFeedback(note); + feedback += `\n## ${section.title || "Plan preamble"}\n`; + for (const annotation of section.annotations) { + if (annotation.target.kind === "line") feedback += `> Line: ${annotation.target.context}\n`; + feedback += this.#formatAnnotationFeedback(annotation.note); + } } this.callbacks.onFeedbackChange?.(feedback); } @@ -717,7 +945,7 @@ export class PlanReviewOverlay implements Component { parts.push("↑↓ section", "⏎ open", "a annotate", "d delete", "u undo"); break; case "body": - parts.push("↑↓ scroll", "⇧ faster", "pgup/pgdn", "g/G ends"); + parts.push("↑↓ scroll", "⇧ faster", "pgup/pgdn", "g/G ends", "a annotate"); break; } if (this.callbacks.onCopyPlan) parts.push("c copy"); @@ -744,35 +972,113 @@ export class PlanReviewOverlay implements Component { if (maxOffset > 0) this.#scrollView.setScrollOffset(Math.round(this.#scrollProgress * maxOffset)); } - /** Build the concatenated body lines and record each section's start row. */ + /** Build the concatenated body lines and map each rendered row back to a plan anchor. */ #buildBody(bodyContentWidth: number): string[] { const lines: string[] = []; + const anchors: BodyRowAnchor[] = []; const offsets: number[] = new Array(this.#sections.length); - for (let i = 0; i < this.#sections.length; i++) { - const section = this.#sections[i]!; - offsets[i] = lines.length; + for (let sectionIndex = 0; sectionIndex < this.#sections.length; sectionIndex++) { + const section = this.#sections[sectionIndex]!; + offsets[sectionIndex] = lines.length; const rendered = section.md.render(bodyContentWidth); - if (section.level >= 1 && section.annotations.length > 0 && rendered.length > 0) { - lines.push(rendered[0]!); - for (const note of section.annotations) { - const noteLines = note.split(/\r?\n/); - for (let j = 0; j < noteLines.length; j++) { - const prefix = - j === 0 - ? `${theme.fg("warning", "▎ ")}${theme.fg("dim", "note: ")}` - : `${theme.fg("warning", "▎ ")}${theme.fg("dim", " ")}`; - lines.push(`${prefix}${theme.fg("accent", noteLines[j] ?? "")}`); + const contexts = rendered.map(line => this.#lineContext(line)); + for (let row = 0; row < rendered.length; row++) { + const context = contexts[row]!; + const anchor = { + sectionIndex, + row, + context: context.text, + contextTruncated: context.truncated, + }; + lines.push(rendered[row]!); + anchors.push(anchor); + for (const annotation of section.annotations) { + const annotationRow = + annotation.target.kind === "section" + ? 0 + : this.#resolveLineRow( + annotation.target.row, + { + text: annotation.target.context, + truncated: annotation.target.contextTruncated, + }, + contexts, + ); + if (annotationRow === row) { + this.#appendAnnotationCallout(lines, anchors, annotation.note, anchor, bodyContentWidth); } } - for (let k = 1; k < rendered.length; k++) lines.push(rendered[k]!); - } else { - for (const line of rendered) lines.push(line); } } this.#sectionOffsets = offsets; + this.#bodyRowAnchors = anchors; return lines; } + #lineContext(line: string): LineAnchorContext { + const sanitized = sanitizeStatusText(line); + const truncated = visibleWidth(sanitized) > MAX_ANNOTATION_CONTEXT_WIDTH; + const text = truncateToWidth(sanitized, MAX_ANNOTATION_CONTEXT_WIDTH, Ellipsis.Unicode); + return { text: text || "(blank line)", truncated }; + } + + #resolveLineRow( + storedRow: number, + storedContext: LineAnchorContext, + contexts: readonly LineAnchorContext[], + ): number { + if (contexts.length === 0) return -1; + const targetRow = Math.max(0, Math.floor(storedRow)); + const normalize = (context: LineAnchorContext): string => { + const normalized = sanitizeStatusText(context.text).replace(/\s+/g, " ").trim(); + return context.truncated && normalized.endsWith("…") ? normalized.slice(0, -1) : normalized; + }; + const normalizedStoredContext = normalize(storedContext); + if (!normalizedStoredContext) return -1; + let best = -1; + let bestDistance = Number.POSITIVE_INFINITY; + for (let row = 0; row < contexts.length; row++) { + const normalizedContext = normalize(contexts[row]!); + if ( + normalizedContext !== normalizedStoredContext && + !normalizedContext.includes(normalizedStoredContext) && + !normalizedStoredContext.includes(normalizedContext) + ) { + continue; + } + const distance = Math.abs(row - targetRow); + if (distance < bestDistance) { + best = row; + bestDistance = distance; + } + } + return best; + } + + #appendAnnotationCallout( + lines: string[], + anchors: BodyRowAnchor[], + note: string, + anchor: BodyRowAnchor, + bodyContentWidth: number, + ): void { + const noteLines = note.split(/\r?\n/); + for (let i = 0; i < noteLines.length; i++) { + const prefix = + i === 0 + ? `${theme.fg("warning", "▎ ")}${theme.fg("dim", "note: ")}` + : `${theme.fg("warning", "▎ ")}${theme.fg("dim", " ")}`; + const available = Math.max(0, bodyContentWidth - visibleWidth(prefix)); + const displayLine = truncateToWidth( + replaceTabs(sanitizeText(noteLines[i] ?? "")), + available, + Ellipsis.Unicode, + ); + lines.push(truncateToWidth(`${prefix}${theme.fg("accent", displayLine)}`, bodyContentWidth)); + anchors.push(anchor); + } + } + #sidebarWidthFor(width: number): number { return Math.max(18, Math.min(30, Math.round(width * 0.24))); } @@ -832,9 +1138,18 @@ export class PlanReviewOverlay implements Component { #renderFooterLines(innerWidth: number): string[] { if (this.#annotating) { - const section = this.#sections[this.#toc[this.#tocCursor]!]; - const title = section?.title ?? ""; - const caption = `${theme.fg("dim", "Annotate")} ${theme.fg("accent", `‹${title}›`)}`; + const target = this.#annotationTarget; + const section = target ? this.#sections[target.sectionIndex] : undefined; + const title = sanitizeStatusText(section?.title || "Plan preamble"); + const location = + target?.row === null + ? `‹${title}›` + : `‹${title}› · ${truncateToWidth(target?.context ?? "", Math.max(1, innerWidth - 16), Ellipsis.Unicode)}`; + const caption = truncateToWidth( + `${theme.fg("dim", "Annotate")} ${theme.fg("accent", location)}`, + innerWidth, + Ellipsis.Unicode, + ); const hintParts = ["enter save", "esc cancel"]; if (this.#externalEditorLabel) hintParts.push(`${this.#externalEditorLabel} editor`); return [caption, this.#input.render(innerWidth)[0] ?? "", theme.fg("dim", hintParts.join(" · "))]; diff --git a/packages/coding-agent/src/modes/interactive-mode.ts b/packages/coding-agent/src/modes/interactive-mode.ts index 332724b15..8ebd94d23 100644 --- a/packages/coding-agent/src/modes/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive-mode.ts @@ -153,7 +153,7 @@ import type { EvalExecutionComponent } from "./components/eval-execution"; import type { HookEditorComponent } from "./components/hook-editor"; import type { HookInputComponent } from "./components/hook-input"; import type { HookSelectorComponent, HookSelectorSlider } from "./components/hook-selector"; -import { PlanReviewOverlay } from "./components/plan-review-overlay"; +import { type PlanReviewAnnotationState, PlanReviewOverlay } from "./components/plan-review-overlay"; import { StatusLineComponent } from "./components/status-line"; import type { ToolExecutionHandle } from "./components/tool-execution"; import { TranscriptContainer } from "./components/transcript-container"; @@ -569,6 +569,10 @@ export class InteractiveMode implements InteractiveModeContext { #planReviewOverlay: PlanReviewOverlay | undefined; #planReviewOverlayHandle: OverlayHandle | undefined; #planReviewCancel: (() => void) | undefined; + /** Serializable review annotations keyed by the resolved plan file path. */ + #planReviewAnnotationState = new Map(); + /** Annotation state held until the associated queued refinement actually starts. */ + #planReviewAnnotationStateBySubmission = new WeakMap(); readonly lspServers: LspStartupServerInfo[] | undefined = undefined; mcpManager?: MCPManager; readonly #toolUiContextSetter: (uiContext: ExtensionUIContext, hasUI: boolean) => void; @@ -1587,6 +1591,11 @@ export class InteractiveMode implements InteractiveModeContext { return false; } input.started = true; + const annotationStateKey = this.#planReviewAnnotationStateBySubmission.get(input); + if (annotationStateKey) { + this.#planReviewAnnotationStateBySubmission.delete(input); + this.#planReviewAnnotationState.delete(annotationStateKey); + } return true; } @@ -2719,6 +2728,8 @@ export class InteractiveMode implements InteractiveModeContext { onExternalEditor?: () => void; onPlanEdited?: (content: string) => void; onFeedbackChange?: (feedback: string) => void; + annotationState?: PlanReviewAnnotationState; + onAnnotationStateChange?: (state: PlanReviewAnnotationState) => void; initialIndex?: number; }, extra?: { slider?: HookSelectorSlider }, @@ -2742,6 +2753,7 @@ export class InteractiveMode implements InteractiveModeContext { initialIndex: dialogOptions?.initialIndex, slider: extra?.slider, externalEditorLabel: this.keybindings.getDisplayString("app.editor.external") || undefined, + annotationState: dialogOptions?.annotationState, }, { onPick: choice => finish(choice), @@ -2751,6 +2763,7 @@ export class InteractiveMode implements InteractiveModeContext { onAnnotationExternalEditor: (draft, commit) => void this.#openPlanAnnotationInExternalEditor(draft, commit), onPlanEdited: dialogOptions?.onPlanEdited, onFeedbackChange: dialogOptions?.onFeedbackChange, + onAnnotationStateChange: dialogOptions?.onAnnotationStateChange, }, ); this.#planReviewOverlay = overlay; @@ -2980,7 +2993,7 @@ export class InteractiveMode implements InteractiveModeContext { compactBeforeExecute?: boolean; executionModel?: ResolvedRoleModel; }, - ): Promise { + ): Promise { const previousTools = this.#planModePreviousTools ?? this.session.getEnabledToolNames(); // Mark the pending abort caused by the plan-mode → compaction transition as @@ -3079,7 +3092,7 @@ export class InteractiveMode implements InteractiveModeContext { this.showWarning( "Plan approved, but compaction was cancelled — execution not dispatched. Submit a turn to continue.", ); - return; + return false; } // Approved plans land in a fresh (or compacted) session whose first user-visible @@ -3121,14 +3134,15 @@ export class InteractiveMode implements InteractiveModeContext { // noted below), catch `AgentBusyError` and fall back to the same queue. if (this.session.isStreaming) { await this.session.followUp(planModePrompt, undefined, { synthetic: true }); - return; - } - try { - await this.session.prompt(planModePrompt, { synthetic: true }); - } catch (error) { - if (!(error instanceof AgentBusyError)) throw error; - await this.session.followUp(planModePrompt, undefined, { synthetic: true }); + } else { + try { + await this.session.prompt(planModePrompt, { synthetic: true }); + } catch (error) { + if (!(error instanceof AgentBusyError)) throw error; + await this.session.followUp(planModePrompt, undefined, { synthetic: true }); + } } + return true; } async #abortPlanApprovalTurnSilently(): Promise { this.session.markPlanInternalAbortPending(); @@ -3657,6 +3671,7 @@ export class InteractiveMode implements InteractiveModeContext { // that the Refine branch re-prompts the model with. let editedContent: string | undefined; let feedback = ""; + const annotationStateKey = this.#resolvePlanFilePath(planFilePath); const choice = await this.showPlanReview( planContent, @@ -3672,6 +3687,11 @@ export class InteractiveMode implements InteractiveModeContext { onFeedbackChange: value => { feedback = value; }, + annotationState: this.#planReviewAnnotationState.get(annotationStateKey), + onAnnotationStateChange: state => { + if (state.annotations.length > 0) this.#planReviewAnnotationState.set(annotationStateKey, state); + else this.#planReviewAnnotationState.delete(annotationStateKey); + }, disabledIndices: keepContextDisabled ? [PLAN_KEEP_CONTEXT_OPTION_INDEX] : undefined, }, { slider }, @@ -3723,13 +3743,14 @@ export class InteractiveMode implements InteractiveModeContext { : -1; const executionModel = slider && cycle && selectedTierIndex !== restoredIndex ? cycle.models[selectedTierIndex] : undefined; - await this.#approvePlan(latestPlanContent, { + const executionDispatched = await this.#approvePlan(latestPlanContent, { planFilePath, title: details.title, preserveContext: choice !== "Approve and execute", compactBeforeExecute: choice === "Approve and compact context", executionModel, }); + if (executionDispatched) this.#planReviewAnnotationState.delete(annotationStateKey); } catch (error) { this.showError( `Failed to finalize approved plan: ${error instanceof Error ? error.message : String(error)}`, @@ -3744,9 +3765,12 @@ export class InteractiveMode implements InteractiveModeContext { try { if (refinement) { if (this.onInputCallback) { - this.onInputCallback(this.startPendingSubmission({ text: feedback })); + const input = this.startPendingSubmission({ text: feedback }); + this.#planReviewAnnotationStateBySubmission.set(input, annotationStateKey); + this.onInputCallback(input); } else { await this.session.prompt(feedback); + this.#planReviewAnnotationState.delete(annotationStateKey); } } else { this.showStatus("Refine plan: enter a follow-up prompt."); diff --git a/packages/coding-agent/test/interactive-mode-plan-review.test.ts b/packages/coding-agent/test/interactive-mode-plan-review.test.ts index 399e9bcf1..ddf831a64 100644 --- a/packages/coding-agent/test/interactive-mode-plan-review.test.ts +++ b/packages/coding-agent/test/interactive-mode-plan-review.test.ts @@ -10,9 +10,13 @@ import { resetSettingsForTest, Settings } from "@oh-my-pi/pi-coding-agent/config import { resolveLocalUrlToPath } from "@oh-my-pi/pi-coding-agent/internal-urls"; import { AssistantMessageComponent } from "@oh-my-pi/pi-coding-agent/modes/components/assistant-message"; import type { HookSelectorSlider } from "@oh-my-pi/pi-coding-agent/modes/components/hook-selector"; -import { PlanReviewOverlay } from "@oh-my-pi/pi-coding-agent/modes/components/plan-review-overlay"; +import { + type PlanReviewAnnotationState, + PlanReviewOverlay, +} from "@oh-my-pi/pi-coding-agent/modes/components/plan-review-overlay"; import { InteractiveMode } from "@oh-my-pi/pi-coding-agent/modes/interactive-mode"; import { initTheme } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; +import type { SubmittedUserInput } from "@oh-my-pi/pi-coding-agent/modes/types"; import { AgentSession } from "@oh-my-pi/pi-coding-agent/session/agent-session"; import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; import { SILENT_ABORT_MARKER, USER_INTERRUPT_LABEL } from "@oh-my-pi/pi-coding-agent/session/messages"; @@ -236,6 +240,127 @@ describe("InteractiveMode plan review rendering", () => { expect(review.mock.calls[1]?.[0]).not.toContain("First plan"); }); + it("restores dismissed annotations only when reopening the same plan", async () => { + const firstPlanFilePath = "local://first-plan.md"; + const secondPlanFilePath = "local://second-plan.md"; + const localOptions = { + getArtifactsDir: () => session.sessionManager.getArtifactsDir(), + getSessionId: () => session.sessionManager.getSessionId(), + }; + await Bun.write(resolveLocalUrlToPath(firstPlanFilePath, localOptions), "# First plan\n\nbody"); + await Bun.write(resolveLocalUrlToPath(secondPlanFilePath, localOptions), "# Second plan\n\nbody"); + + mode.planModeEnabled = true; + mode.planModePlanFilePath = firstPlanFilePath; + const annotationState: PlanReviewAnnotationState = { + annotations: [ + { + section: { index: 0, title: "First plan" }, + target: { kind: "line", row: 2, context: "body" }, + note: "Add the rollback path.", + }, + ], + }; + const restoredStates: Array = []; + vi.spyOn(mode, "showPlanReview").mockImplementation(async (_plan, _title, _options, dialogOptions) => { + restoredStates.push(dialogOptions?.annotationState); + if (restoredStates.length === 1) dialogOptions?.onAnnotationStateChange?.(annotationState); + return undefined; + }); + + await mode.handlePlanApproval({ planFilePath: firstPlanFilePath, planExists: true, title: "FIRST" }); + await mode.handlePlanApproval({ planFilePath: firstPlanFilePath, planExists: true, title: "FIRST" }); + await mode.handlePlanApproval({ planFilePath: secondPlanFilePath, planExists: true, title: "SECOND" }); + + expect(restoredStates).toEqual([undefined, annotationState, undefined]); + }); + + it("consumes annotations after Refine dispatches their feedback", async () => { + const planFilePath = "local://PLAN.md"; + const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, { + getArtifactsDir: () => session.sessionManager.getArtifactsDir(), + getSessionId: () => session.sessionManager.getSessionId(), + }); + await Bun.write(resolvedPlanPath, "# Plan\n\nbody"); + + mode.planModeEnabled = true; + mode.planModePlanFilePath = planFilePath; + const annotationState: PlanReviewAnnotationState = { + annotations: [ + { + section: { index: 0, title: "Plan" }, + target: { kind: "line", row: 2, context: "body" }, + note: "Clarify the rollback path.", + }, + ], + }; + const feedback = "Refinement feedback on the plan:\n\n> Line: body\n- Clarify the rollback path.\n"; + let reviewCount = 0; + vi.spyOn(mode, "showPlanReview").mockImplementation(async (_plan, _title, _options, dialogOptions) => { + reviewCount++; + if (reviewCount === 1) { + dialogOptions?.onAnnotationStateChange?.(annotationState); + dialogOptions?.onFeedbackChange?.(feedback); + return "Refine plan"; + } + expect(dialogOptions?.annotationState).toBeUndefined(); + return undefined; + }); + const promptSpy = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never); + + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN" }); + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN" }); + + expect(promptSpy).toHaveBeenCalledWith(feedback); + }); + + it("retains queued Refine annotations until the pending submission starts", async () => { + const planFilePath = "local://PLAN.md"; + const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, { + getArtifactsDir: () => session.sessionManager.getArtifactsDir(), + getSessionId: () => session.sessionManager.getSessionId(), + }); + await Bun.write(resolvedPlanPath, "# Plan\n\nbody"); + mode.planModeEnabled = true; + mode.planModePlanFilePath = planFilePath; + const annotationState: PlanReviewAnnotationState = { + annotations: [ + { + section: { index: 0, title: "Plan" }, + target: { kind: "line", row: 2, context: "body" }, + note: "Clarify the rollback path.", + }, + ], + }; + const feedback = "Refinement feedback on the plan:\n\n> Line: body\n- Clarify the rollback path.\n"; + const restoredStates: Array = []; + let reviewCount = 0; + vi.spyOn(mode, "showPlanReview").mockImplementation(async (_plan, _title, _options, dialogOptions) => { + reviewCount++; + restoredStates.push(dialogOptions?.annotationState); + if (reviewCount === 1) dialogOptions?.onAnnotationStateChange?.(annotationState); + if (reviewCount <= 2) { + dialogOptions?.onFeedbackChange?.(feedback); + return "Refine plan"; + } + return undefined; + }); + const submissions: SubmittedUserInput[] = []; + mode.onInputCallback = input => submissions.push(input); + const promptSpy = vi.spyOn(session, "prompt"); + + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN" }); + expect(mode.cancelPendingSubmission()).toBe(true); + + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN" }); + expect(mode.markPendingSubmissionStarted(submissions[1]!)).toBe(true); + + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN" }); + + expect(restoredStates).toEqual([undefined, annotationState, undefined]); + expect(promptSpy).not.toHaveBeenCalled(); + }); + it("re-prompts the model with annotation feedback when Refine is chosen", async () => { const planFilePath = "local://PLAN.md"; const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, { @@ -1572,6 +1697,47 @@ describe("InteractiveMode plan review rendering", () => { expect(promptSpy.mock.calls.some(isPlanApprovedCall)).toBe(false); }); + it("Approve and compact context retains annotations when compaction cancels before dispatch", async () => { + const planFilePath = "local://PLAN.md"; + const resolvedPlanPath = resolveLocalUrlToPath(planFilePath, { + getArtifactsDir: () => session.sessionManager.getArtifactsDir(), + getSessionId: () => session.sessionManager.getSessionId(), + }); + await Bun.write(resolvedPlanPath, "# Plan\n\nCancel mid-compact."); + mode.planModeEnabled = true; + mode.planModePlanFilePath = planFilePath; + const annotationState: PlanReviewAnnotationState = { + annotations: [ + { + section: { index: 0, title: "Plan" }, + target: { kind: "line", row: 2, context: "Cancel mid-compact." }, + note: "Keep the rollback path.", + }, + ], + }; + const restoredStates: Array = []; + let reviewCount = 0; + vi.spyOn(mode, "showPlanReview").mockImplementation(async (_plan, _title, _options, dialogOptions) => { + reviewCount++; + restoredStates.push(dialogOptions?.annotationState); + if (reviewCount === 1) { + dialogOptions?.onAnnotationStateChange?.(annotationState); + return "Approve and compact context"; + } + return undefined; + }); + vi.spyOn(mode, "handleCompactCommand").mockResolvedValue("cancelled"); + const promptSpy = vi.spyOn(session, "prompt").mockResolvedValue(undefined as never); + + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN" }); + mode.planModeEnabled = true; + mode.planModePlanFilePath = planFilePath; + await mode.handlePlanApproval({ planFilePath, planExists: true, title: "PLAN" }); + + expect(restoredStates).toEqual([undefined, annotationState]); + expect(promptSpy.mock.calls.some(isPlanApprovedCall)).toBe(false); + }); + it("Approve and compact context: failed outcome still dispatches plan-approved (best-effort)", async () => { // Mock `handleCompactCommand` to surface the "failed" outcome directly. // Failure → approval intent stands → synthetic dispatch fires. diff --git a/packages/coding-agent/test/modes/components/plan-review-overlay.test.ts b/packages/coding-agent/test/modes/components/plan-review-overlay.test.ts index 0463ed09a..7ebbace25 100644 --- a/packages/coding-agent/test/modes/components/plan-review-overlay.test.ts +++ b/packages/coding-agent/test/modes/components/plan-review-overlay.test.ts @@ -498,6 +498,236 @@ describe("PlanReviewOverlay", () => { expect(feedback).not.toContain("```md"); }); + it("anchors body annotations to the visible line and restores their serializable state", () => { + const onAnnotationStateChange = vi.fn(); + const onFeedbackChange = vi.fn(); + const scrollSteps = 12; + const note = "clarify this execution detail"; + const longPlan = `# Execution\n\n${Array.from( + { length: 80 }, + (_, i) => `body-row-${String(i).padStart(3, "0")}`, + ).join("\n\n")}\n`; + const overlay = new PlanReviewOverlay( + longPlan, + { promptTitle: "next", options: APPROVAL_OPTIONS }, + { onPick: vi.fn(), onCancel: vi.fn(), onAnnotationStateChange, onFeedbackChange }, + ); + const visibleRows = (): string[] => + render(overlay) + .split("\n") + .map(line => line.match(/body-row-\d{3}/)?.[0]) + .filter((row): row is string => row !== undefined); + + render(overlay); + overlay.handleInput(TAB); // actions -> body + for (let i = 0; i < scrollSteps; i++) overlay.handleInput(DOWN); + const topVisibleRow = visibleRows()[0]; + expect(topVisibleRow).toMatch(/^body-row-\d{3}$/); + + overlay.handleInput("a"); + expect(render(overlay)).toContain("Annotate"); + for (const ch of note) overlay.handleInput(ch); + overlay.handleInput(ENTER); + + expect(onAnnotationStateChange).toHaveBeenCalledTimes(1); + const annotationState = onAnnotationStateChange.mock.calls[0]?.[0]; + expect(annotationState.annotations).toHaveLength(1); + expect(annotationState.annotations[0]).toMatchObject({ + section: { index: 0, title: "Execution" }, + target: { kind: "line", context: topVisibleRow }, + note, + }); + const annotatedLines = render(overlay).split("\n"); + const annotatedRow = annotatedLines.findIndex(line => line.includes(topVisibleRow)); + const calloutRow = annotatedLines.findIndex(line => line.includes(note)); + expect(calloutRow).toBe(annotatedRow + 1); + const feedback = onFeedbackChange.mock.calls.at(-1)?.[0] as string; + expect(feedback).toContain(`> Line: ${topVisibleRow}`); + expect(feedback).toContain(note); + + const restoredFeedback = vi.fn(); + const restored = new PlanReviewOverlay( + longPlan, + { promptTitle: "next", options: APPROVAL_OPTIONS, annotationState }, + { onPick: vi.fn(), onCancel: vi.fn(), onFeedbackChange: restoredFeedback }, + ); + expect(restoredFeedback.mock.calls[0]?.[0]).toBe(feedback); + render(restored); + restored.handleInput(TAB); // actions -> body + for (let i = 0; i < scrollSteps; i++) restored.handleInput(DOWN); + const restoredLines = render(restored).split("\n"); + const restoredRow = restoredLines.findIndex(line => line.includes(topVisibleRow)); + const restoredCalloutRow = restoredLines.findIndex(line => line.includes(note)); + expect(restoredCalloutRow).toBe(restoredRow + 1); + }); + + it("clears non-empty restored state with stale anchors without notifying for empty state", () => { + const emptyStateChange = vi.fn(); + const emptyFeedbackChange = vi.fn(); + new PlanReviewOverlay( + "# B\n\nbeta body\n", + { promptTitle: "next", options: APPROVAL_OPTIONS, annotationState: { annotations: [] } }, + { + onPick: vi.fn(), + onCancel: vi.fn(), + onAnnotationStateChange: emptyStateChange, + onFeedbackChange: emptyFeedbackChange, + }, + ); + expect(emptyStateChange).not.toHaveBeenCalled(); + expect(emptyFeedbackChange).not.toHaveBeenCalled(); + + const onAnnotationStateChange = vi.fn(); + const onFeedbackChange = vi.fn(); + const note = "stale section A note"; + const restored = new PlanReviewOverlay( + "# B\n\nbeta body\n", + { + promptTitle: "next", + options: APPROVAL_OPTIONS, + annotationState: { + annotations: [{ section: { index: 0, title: "A" }, target: { kind: "section" }, note }], + }, + }, + { onPick: vi.fn(), onCancel: vi.fn(), onAnnotationStateChange, onFeedbackChange }, + ); + + expect(render(restored)).not.toContain(note); + expect(onAnnotationStateChange).toHaveBeenCalledTimes(1); + expect(onAnnotationStateChange).toHaveBeenCalledWith({ annotations: [] }); + expect(onFeedbackChange).toHaveBeenCalledTimes(1); + expect(onFeedbackChange).toHaveBeenCalledWith(""); + }); + + it("drops restored section annotations when the section title no longer exists", () => { + const onAnnotationStateChange = vi.fn(); + const onFeedbackChange = vi.fn(); + const note = "keep this attached to section A"; + const overlay = new PlanReviewOverlay( + "# A\n\nalpha body\n\n# B\n\nbeta body\n", + { promptTitle: "next", options: APPROVAL_OPTIONS }, + { onPick: vi.fn(), onCancel: vi.fn(), onAnnotationStateChange, onFeedbackChange }, + ); + render(overlay); + overlay.handleInput(TAB); // actions -> toc (A) + overlay.handleInput("a"); + for (const ch of note) overlay.handleInput(ch); + overlay.handleInput(ENTER); + expect(onAnnotationStateChange.mock.calls.at(-1)?.[0].annotations).toHaveLength(1); + + onAnnotationStateChange.mockClear(); + onFeedbackChange.mockClear(); + overlay.setPlanContent("# B\n\nbeta body\n"); + + expect(render(overlay)).not.toContain(note); + expect(onFeedbackChange).toHaveBeenCalledWith(""); + expect(onAnnotationStateChange).toHaveBeenCalledWith({ annotations: [] }); + }); + + it("does not migrate annotations between duplicate headings after a plan edit", () => { + const onAnnotationStateChange = vi.fn(); + const onFeedbackChange = vi.fn(); + const note = "keep this on phase B"; + const overlay = new PlanReviewOverlay( + "# Plan\n\n## Phase A\n\n### Steps\n\nalpha\n\n## Phase B\n\n### Steps\n\nbeta\n", + { promptTitle: "next", options: APPROVAL_OPTIONS }, + { onPick: vi.fn(), onCancel: vi.fn(), onAnnotationStateChange, onFeedbackChange }, + ); + render(overlay); + overlay.handleInput(TAB); // actions -> toc (Phase A) + for (let i = 0; i < 3; i++) overlay.handleInput(DOWN); // Phase B > Steps + overlay.handleInput("a"); + for (const ch of note) overlay.handleInput(ch); + overlay.handleInput(ENTER); + expect(onAnnotationStateChange.mock.calls.at(-1)?.[0].annotations[0]).toMatchObject({ + section: { title: "Steps", path: ["Plan", "Phase B", "Steps"] }, + note, + }); + + onAnnotationStateChange.mockClear(); + onFeedbackChange.mockClear(); + overlay.setPlanContent("# Plan\n\n## Phase A\n\n### Steps\n\nalpha\n"); + + expect(render(overlay)).not.toContain(note); + expect(onAnnotationStateChange).toHaveBeenCalledWith({ annotations: [] }); + expect(onFeedbackChange).toHaveBeenCalledWith(""); + }); + + it("restores an annotation whose literal line context is an ellipsis", () => { + const note = "expand this placeholder"; + const onAnnotationStateChange = vi.fn(); + const overlay = new PlanReviewOverlay( + "# Plan\n\n…\n", + { + promptTitle: "next", + options: APPROVAL_OPTIONS, + annotationState: { + annotations: [ + { + section: { index: 0, title: "Plan" }, + target: { kind: "line", row: 2, context: "…" }, + note, + }, + ], + }, + }, + { onPick: vi.fn(), onCancel: vi.fn(), onAnnotationStateChange }, + ); + + expect(render(overlay)).toContain(note); + expect(onAnnotationStateChange.mock.calls[0]?.[0].annotations[0]).toMatchObject({ + target: { kind: "line", context: "…", contextTruncated: false }, + note, + }); + }); + + it("drops restored line annotations when their context disappears from the section", () => { + const onAnnotationStateChange = vi.fn(); + const onFeedbackChange = vi.fn(); + const note = "keep this attached to its original line"; + const originalPlan = `# A\n\n${Array.from( + { length: 80 }, + (_, i) => `original-row-${String(i).padStart(3, "0")}`, + ).join("\n\n")}\n`; + const overlay = new PlanReviewOverlay( + originalPlan, + { promptTitle: "next", options: APPROVAL_OPTIONS }, + { onPick: vi.fn(), onCancel: vi.fn(), onAnnotationStateChange, onFeedbackChange }, + ); + const visibleRows = (): string[] => + render(overlay) + .split("\n") + .map(line => line.match(/original-row-\d{3}/)?.[0]) + .filter((row): row is string => row !== undefined); + + render(overlay); + overlay.handleInput(TAB); // actions -> body + for (let i = 0; i < 12; i++) overlay.handleInput(DOWN); + const annotatedContext = visibleRows()[0]; + expect(annotatedContext).toMatch(/^original-row-\d{3}$/); + overlay.handleInput("a"); + for (const ch of note) overlay.handleInput(ch); + overlay.handleInput(ENTER); + expect(onAnnotationStateChange.mock.calls.at(-1)?.[0].annotations[0]).toMatchObject({ + section: { title: "A" }, + target: { kind: "line", context: annotatedContext }, + note, + }); + + onAnnotationStateChange.mockClear(); + onFeedbackChange.mockClear(); + overlay.setPlanContent( + `# A\n\n${Array.from({ length: 12 }, (_, i) => `replacement-row-${String(i).padStart(3, "0")}`).join( + "\n\n", + )}\n`, + ); + + const out = render(overlay); + expect(out).not.toContain(note); + expect(onFeedbackChange).toHaveBeenCalledWith(""); + expect(onAnnotationStateChange).toHaveBeenCalledWith({ annotations: [] }); + }); + it("opens the external editor for an active annotation draft", () => { setKeybindings(KeybindingsManager.inMemory({ "tui.select.cancel": "ctrl+g", "app.editor.external": "ctrl+e" })); const onFeedbackChange = vi.fn();