diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 322ceb65f..3e0bb959c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -6,6 +6,7 @@ - Queued `/skill: [args]` invocations now show as compact `Steer: /skill: [args]` / `Follow-up: /skill: [args]` chips in the pending-messages bar and disappear when the agent consumes the queued message (parity with plain-text steer/follow-up). Previously the queued skill was invisible while queued and rendered as a full skill block at consumption with no chip ever appearing. - Plan-mode "Approve and compact context" no longer surfaces a red "Operation aborted" line on the plan-mode assistant message; the silent transition into compaction now renders cleanly on both live and replay paths. Real user-cancel aborts on unrelated turns and the existing "Compaction cancelled" path are unchanged. +- Auto-recover conflict-resolution `write`/`read` paths that the agent malformed as `:conflict://` (or `:conflict://*`) by mixing the `:conflicts` read selector with the `conflict://` scheme. The stripped `:` prefix is stored on `ParsedConflictUri.recoveredPrefix` and, for writes, surfaces as a trailing note in the result text so the agent learns the correct shape. Clean `conflict://…` URIs are unchanged. ### Added - Added `hide: true` frontmatter option for skill `SKILL.md` files. Hidden skills are still loaded and remain reachable via `skill://` URLs and (when enabled) `/skill:` slash commands, but are omitted from the rendered system prompt's `` listing so the model won't auto-discover them. Use for skills the user opts into explicitly rather than ones the model should pick up from descriptions. diff --git a/packages/coding-agent/src/modes/components/status-line.ts b/packages/coding-agent/src/modes/components/status-line.ts index 7b94c2507..10b7a76da 100644 --- a/packages/coding-agent/src/modes/components/status-line.ts +++ b/packages/coding-agent/src/modes/components/status-line.ts @@ -6,10 +6,10 @@ import { settings } from "../../config/settings"; import type { StatusLinePreset, StatusLineSegmentId, StatusLineSeparatorStyle } from "../../config/settings-schema"; import { theme } from "../../modes/theme/theme"; import type { AgentSession } from "../../session/agent-session"; -import { computeContextBreakdown } from "../utils/context-usage"; import * as git from "../../utils/git"; import { getSessionAccentAnsi, getSessionAccentHex } from "../../utils/session-color"; import { sanitizeStatusText } from "../shared"; +import { computeContextBreakdown } from "../utils/context-usage"; import { canReuseCachedPr, createPrCacheContext, diff --git a/packages/coding-agent/src/tools/conflict-detect.ts b/packages/coding-agent/src/tools/conflict-detect.ts index 3612d87f4..6aa79dd9d 100644 --- a/packages/coding-agent/src/tools/conflict-detect.ts +++ b/packages/coding-agent/src/tools/conflict-detect.ts @@ -250,9 +250,19 @@ export interface ParsedConflictUri { /** `"*"` selects every currently-registered conflict (bulk write only). */ id: number | "*"; scope?: ConflictScope; + /** + * When `raw` was a malformed `:conflict://…` path, the + * stripped prefix is preserved here so callers can surface a gentle + * "you don't need the file path" note. `undefined` for clean URIs. + */ + recoveredPrefix?: string; } -const CONFLICT_URI_RE = /^conflict:\/\/(.+)$/; +// Accept an optional `:` before the scheme so paths like +// `path/to/file.ts:conflict://3` (where the agent mixed the `:conflicts` +// read selector with the `conflict://` scheme) still resolve. The prefix +// is greedy so the LAST `:conflict://` wins for multi-colon inputs. +const CONFLICT_URI_RE = /^(?:(.+):)?conflict:\/\/(.+)$/; /** * Parse a `conflict://`, `conflict:///`, or `conflict://*` URI. @@ -269,7 +279,8 @@ const CONFLICT_URI_RE = /^conflict:\/\/(.+)$/; export function parseConflictUri(raw: string): ParsedConflictUri | null { const match = raw.match(CONFLICT_URI_RE); if (!match) return null; - const tail = match[1]; + const recoveredPrefix = match[1]; + const tail = match[2]; const slashIdx = tail.indexOf("/"); const idPart = slashIdx === -1 ? tail : tail.slice(0, slashIdx); const scopePart = slashIdx === -1 ? undefined : tail.slice(slashIdx + 1); @@ -280,7 +291,7 @@ export function parseConflictUri(raw: string): ParsedConflictUri | null { `Invalid conflict URI '${raw}': wildcard 'conflict://*' does not accept a scope segment. Drop '/${scopePart}' or use a numeric id.`, ); } - return { id: "*" }; + return recoveredPrefix !== undefined ? { id: "*", recoveredPrefix } : { id: "*" }; } if (!/^\d+$/.test(idPart)) { @@ -303,7 +314,7 @@ export function parseConflictUri(raw: string): ParsedConflictUri | null { scope = scopePart as ConflictScope; } - return { id, scope }; + return recoveredPrefix !== undefined ? { id, scope, recoveredPrefix } : { id, scope }; } /** diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index 74705ffde..0214077a0 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -85,6 +85,21 @@ function stripWriteContent(session: ToolSession, content: string): { text: strin return { text: cleaned.join("\n"), stripped: true }; } +/** + * Append a trailing note line to the first text block of a tool result. + * Mutates `result` in place (the result object is owned by this call). + */ +function appendNoteToResult(result: AgentToolResult, note: string): void { + const firstText = result.content.find( + (block): block is { type: "text"; text: string } => block.type === "text" && typeof block.text === "string", + ); + if (firstText) { + firstText.text = firstText.text.length > 0 ? `${firstText.text}\n${note}` : note; + } else { + result.content.push({ type: "text", text: note }); + } +} + // ═══════════════════════════════════════════════════════════════════════════ // Tool Class // ═══════════════════════════════════════════════════════════════════════════ @@ -489,6 +504,26 @@ export class WriteTool implements AgentTool> { + const entry = getConflictHistory(this.session).get(id); + if (!entry) { + throw new ToolError( + `Conflict #${id} not found. Conflict ids are registered when \`read\` surfaces a marker block; re-read the file to get a current id.`, + ); + } + return this.#resolveConflict(entry, replacementContent, stripped, signal, context); + } + /** * Bulk-resolve every registered conflict via `conflict://*`. * @@ -631,16 +666,17 @@ export class WriteTool implements AgentTool:conflict://${conflictUri.id}\`).`, ); } - return this.#resolveConflict(entry, cleanContent, stripped, signal, context); + return result; } const resolvedArchivePath = await this.#resolveArchiveWritePath(path); if (resolvedArchivePath) { diff --git a/packages/coding-agent/test/input-controller-skill-queue.test.ts b/packages/coding-agent/test/input-controller-skill-queue.test.ts index 6aebdbaf8..06029652c 100644 --- a/packages/coding-agent/test/input-controller-skill-queue.test.ts +++ b/packages/coding-agent/test/input-controller-skill-queue.test.ts @@ -25,16 +25,13 @@ import { ModelRegistry } from "@oh-my-pi/pi-coding-agent/config/model-registry"; import { Settings } from "@oh-my-pi/pi-coding-agent/config/settings"; import { EventController } from "@oh-my-pi/pi-coding-agent/modes/controllers/event-controller"; import { InputController } from "@oh-my-pi/pi-coding-agent/modes/controllers/input-controller"; +import { getThemeByName, setThemeInstance } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; import type { InteractiveModeContext } from "@oh-my-pi/pi-coding-agent/modes/types"; import { UiHelpers } from "@oh-my-pi/pi-coding-agent/modes/utils/ui-helpers"; import { AgentSession, type AgentSessionEvent } from "@oh-my-pi/pi-coding-agent/session/agent-session"; import { AuthStorage } from "@oh-my-pi/pi-coding-agent/session/auth-storage"; -import { - SKILL_PROMPT_MESSAGE_TYPE, - type SkillPromptDetails, -} from "@oh-my-pi/pi-coding-agent/session/messages"; +import { SKILL_PROMPT_MESSAGE_TYPE, type SkillPromptDetails } from "@oh-my-pi/pi-coding-agent/session/messages"; import { SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; -import { getThemeByName, setThemeInstance } from "@oh-my-pi/pi-coding-agent/modes/theme/theme"; import { Container } from "@oh-my-pi/pi-tui"; import { TempDir } from "@oh-my-pi/pi-utils"; @@ -59,10 +56,7 @@ type StubEditor = { onSubmit?: (text: string) => Promise; }; -function createStubInputControllerContext(opts: { - skillCommands: Map; - isStreaming: boolean; -}) { +function createStubInputControllerContext(opts: { skillCommands: Map; isStreaming: boolean }) { let editorText = ""; const editor: StubEditor = { setText(text) { @@ -76,9 +70,7 @@ function createStubInputControllerContext(opts: { const enqueueCustomMessageDisplay = vi.fn((_text: string, _mode: "steer" | "followUp") => "sk-test-0"); // Annotate parameters so `mock.calls[N]` is typed as a tuple (not `[]`) — // avoids TS2352/TS2493 when casting `.calls[0]` to a destructured shape. - const promptCustomMessage = vi.fn( - async (_message: { details?: SkillPromptDetails }, _options?: unknown) => {}, - ); + const promptCustomMessage = vi.fn(async (_message: { details?: SkillPromptDetails }, _options?: unknown) => {}); const updatePendingMessagesDisplay = vi.fn(); const requestRender = vi.fn(); const showError = vi.fn(); @@ -128,8 +120,10 @@ describe("InputController #invokeSkillCommand (E1-E3)", () => { }); it("E1: streaming + steer -> enqueueCustomMessageDisplay called and details.__pendingDisplayTag set", async () => { - const { ctx, editor, enqueueCustomMessageDisplay, promptCustomMessage } = - createStubInputControllerContext({ skillCommands, isStreaming: true }); + const { ctx, editor, enqueueCustomMessageDisplay, promptCustomMessage } = createStubInputControllerContext({ + skillCommands, + isStreaming: true, + }); const controller = new InputController(ctx); controller.setupEditorSubmitHandler(); @@ -148,8 +142,10 @@ describe("InputController #invokeSkillCommand (E1-E3)", () => { }); it("E2: streaming + followUp -> enqueueCustomMessageDisplay called with mode 'followUp', tag embedded", async () => { - const { ctx, editor, enqueueCustomMessageDisplay, promptCustomMessage } = - createStubInputControllerContext({ skillCommands, isStreaming: true }); + const { ctx, editor, enqueueCustomMessageDisplay, promptCustomMessage } = createStubInputControllerContext({ + skillCommands, + isStreaming: true, + }); const controller = new InputController(ctx); editor.setText("/skill:test-skill arg1 arg2"); @@ -167,8 +163,10 @@ describe("InputController #invokeSkillCommand (E1-E3)", () => { }); it("E3: not streaming -> enqueueCustomMessageDisplay NOT called and tag absent", async () => { - const { ctx, editor, enqueueCustomMessageDisplay, promptCustomMessage } = - createStubInputControllerContext({ skillCommands, isStreaming: false }); + const { ctx, editor, enqueueCustomMessageDisplay, promptCustomMessage } = createStubInputControllerContext({ + skillCommands, + isStreaming: false, + }); const controller = new InputController(ctx); controller.setupEditorSubmitHandler(); @@ -477,8 +475,7 @@ describe("EventController custom-role dequeue refresh (E10)", () => { }); it("E10: message_start with role=custom refreshes pending bar ONLY when __pendingDisplayTag is present", async () => { - const { controller, updatePendingMessagesDisplay, addMessageToChat } = - createEventControllerFixtureForE10(); + const { controller, updatePendingMessagesDisplay, addMessageToChat } = createEventControllerFixtureForE10(); // Positive case: tagged custom => refresh fires exactly once. The tag is the // unambiguous signal "this message was queued via enqueueCustomMessageDisplay"; 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 6894e3a16..6d962af2f 100644 --- a/packages/coding-agent/test/interactive-mode-plan-review.test.ts +++ b/packages/coding-agent/test/interactive-mode-plan-review.test.ts @@ -439,9 +439,7 @@ describe("InteractiveMode plan review rendering", () => { const showErrorSpy = vi.spyOn(mode, "showError"); await approveWithCompact("throw", new Error("synthetic compaction failure")); expect(session.isPlanCompactAbortPending).toBe(false); - expect(showErrorSpy).toHaveBeenCalledWith( - expect.stringContaining("synthetic compaction failure"), - ); + expect(showErrorSpy).toHaveBeenCalledWith(expect.stringContaining("synthetic compaction failure")); }); it("B5: Approve and execute (no compact) → markPlanCompactAbortPending never called; flag stays false", async () => { diff --git a/packages/coding-agent/test/session-manager-internal-details.test.ts b/packages/coding-agent/test/session-manager-internal-details.test.ts index 28323c6a6..6c76498d1 100644 --- a/packages/coding-agent/test/session-manager-internal-details.test.ts +++ b/packages/coding-agent/test/session-manager-internal-details.test.ts @@ -12,18 +12,12 @@ * `__`-prefixed fields not in the allowlist) is preserved verbatim. */ import { describe, expect, it } from "bun:test"; -import { - type SkillPromptDetails, - stripInternalDetailsFields, -} from "@oh-my-pi/pi-coding-agent/session/messages"; +import { type SkillPromptDetails, stripInternalDetailsFields } from "@oh-my-pi/pi-coding-agent/session/messages"; import { type CustomMessageEntry, SessionManager } from "@oh-my-pi/pi-coding-agent/session/session-manager"; const SKILL_TYPE = "skill-prompt"; -function readPersistedCustomMessageEntry( - session: SessionManager, - id: string, -): CustomMessageEntry { +function readPersistedCustomMessageEntry(session: SessionManager, id: string): CustomMessageEntry { const branch = session.getBranch(); const entry = branch.find(e => e.id === id); if (!entry || entry.type !== "custom_message") { @@ -110,9 +104,7 @@ describe("SessionManager.appendCustomMessageEntry (allowlist strip + persistence // `null as never` here only because the public signature is `T | undefined`, // but the runtime contract has to tolerate `null` defensively. expect(stripInternalDetailsFields(null as unknown as undefined)).toBeNull(); - expect(stripInternalDetailsFields("string" as unknown as undefined)).toBe( - "string" as unknown as undefined, - ); + expect(stripInternalDetailsFields("string" as unknown as undefined)).toBe("string" as unknown as undefined); }); it("F5: stripInternalDetailsFields preserves the input shape verbatim when no allowlisted field is present", () => { diff --git a/packages/coding-agent/test/tools/conflict-detect.test.ts b/packages/coding-agent/test/tools/conflict-detect.test.ts index bea75fdf1..ca8bb8264 100644 --- a/packages/coding-agent/test/tools/conflict-detect.test.ts +++ b/packages/coding-agent/test/tools/conflict-detect.test.ts @@ -213,6 +213,27 @@ describe("parseConflictUri", () => { expect(() => parseConflictUri("conflict://abc")).toThrow(ToolError); expect(() => parseConflictUri("conflict://1/extra")).toThrow(ToolError); }); + + it("recovers an erroneous `:` prefix and surfaces it as `recoveredPrefix`", () => { + expect(parseConflictUri("src/foo.ts:conflict://3")).toEqual({ + id: 3, + recoveredPrefix: "src/foo.ts", + }); + expect(parseConflictUri("packages/coding-agent/src/x.ts:conflict://*")).toEqual({ + id: "*", + recoveredPrefix: "packages/coding-agent/src/x.ts", + }); + expect(parseConflictUri("a.ts:conflict://2/theirs")).toEqual({ + id: 2, + scope: "theirs", + recoveredPrefix: "a.ts", + }); + }); + + it("does not set `recoveredPrefix` on clean URIs", () => { + expect(parseConflictUri("conflict://1")).not.toHaveProperty("recoveredPrefix"); + expect(parseConflictUri("conflict://*")).not.toHaveProperty("recoveredPrefix"); + }); }); function makeEntry(overrides: Partial = {}): ConflictEntry { diff --git a/packages/coding-agent/test/tools/conflict-integration.test.ts b/packages/coding-agent/test/tools/conflict-integration.test.ts index 15995831a..a9ac07efa 100644 --- a/packages/coding-agent/test/tools/conflict-integration.test.ts +++ b/packages/coding-agent/test/tools/conflict-integration.test.ts @@ -327,6 +327,46 @@ describe("write resolves conflicts via conflict://N", () => { expect(session.conflictHistory?.get(1)).toBeUndefined(); }); + it("auto-recovers a `:conflict://N` path and resolves the conflict", async () => { + const filePath = path.join(tempDir, "prefix.ts"); + await Bun.write(filePath, TWO_WAY); + const session = createTestSession(tempDir); + const read = await getTool(session, "read"); + const write = await getTool(session, "write"); + + await read.execute("read-prefix", { path: "prefix.ts" }); + const result = await write.execute("write-prefix", { + // Malformed path mixing the `:conflicts` read selector with the + // `conflict://` scheme — the write tool MUST recover and resolve. + path: "prefix.ts:conflict://1", + content: "@theirs", + }); + + const text = getText(result); + expect(text).toContain("Resolved conflict #1"); + expect(text).toContain("stripped erroneous 'prefix.ts:' prefix"); + expect(await Bun.file(filePath).text()).toBe("line 1\nnewApi(x)\nline N\n"); + }); + + it("auto-recovers a `:conflict://*` path and bulk-resolves", async () => { + const filePath = path.join(tempDir, "bulk-prefix.ts"); + await Bun.write(filePath, TWO_WAY); + const session = createTestSession(tempDir); + const read = await getTool(session, "read"); + const write = await getTool(session, "write"); + + await read.execute("read-bulk-prefix", { path: "bulk-prefix.ts" }); + const result = await write.execute("write-bulk-prefix", { + path: "bulk-prefix.ts:conflict://*", + content: "@ours", + }); + + const text = getText(result); + expect(text).toContain("Resolved 1 conflict"); + expect(text).toContain("stripped erroneous 'bulk-prefix.ts:' prefix"); + expect(await Bun.file(filePath).text()).toBe("line 1\noldApi(x)\nline N\n"); + }); + it("can resolve two blocks in the same file by id, in either order", async () => { const filePath = path.join(tempDir, "two.ts"); await Bun.write(filePath, TWO_BLOCKS);