fix(tools): recovered malformed conflict:// paths during write conflict resolution
- Updated conflict URI parsing to accept `path:conflict://N` and record the removed prefix in `recoveredPrefix`. - Updated write conflict handling to resolve single or wildcard IDs through shared helpers and append a recovery note when a malformed prefix was stripped. - Added regression tests for recovered prefixes and end-to-end write-path recovery and documented the change in the changelog.
This commit is contained in:
@@ -6,6 +6,7 @@
|
||||
|
||||
- Queued `/skill:<name> [args]` invocations now show as compact `Steer: /skill:<name> [args]` / `Follow-up: /skill:<name> [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 `<file>:conflict://<N>` (or `<file>:conflict://*`) by mixing the `:conflicts` read selector with the `conflict://` scheme. The stripped `<file>:` 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://<name>` URLs and (when enabled) `/skill:<name>` slash commands, but are omitted from the rendered system prompt's `<skills>` 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.
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -250,9 +250,19 @@ export interface ParsedConflictUri {
|
||||
/** `"*"` selects every currently-registered conflict (bulk write only). */
|
||||
id: number | "*";
|
||||
scope?: ConflictScope;
|
||||
/**
|
||||
* When `raw` was a malformed `<file-prefix>: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 `<prefix>:` 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://<N>`, `conflict://<N>/<scope>`, 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 };
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
@@ -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<WriteToolDetails>, 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<typeof writeSchema, WriteToolDetails
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Look up a single conflict entry by id and dispatch to {@link #resolveConflict}.
|
||||
* Throws a clear `not found` error when the id has been invalidated.
|
||||
*/
|
||||
async #resolveSingleConflictById(
|
||||
id: number,
|
||||
replacementContent: string,
|
||||
stripped: boolean,
|
||||
signal: AbortSignal | undefined,
|
||||
context: AgentToolContext | undefined,
|
||||
): Promise<AgentToolResult<WriteToolDetails>> {
|
||||
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<typeof writeSchema, WriteToolDetails
|
||||
`Conflict URI scope '/${conflictUri.scope}' is read-only — read \`conflict://${conflictUri.id}/${conflictUri.scope}\` to inspect that side. To write, drop the scope (\`conflict://${conflictUri.id}\`) and put the chosen content (or shorthand like \`@${conflictUri.scope}\`) in \`content\`.`,
|
||||
);
|
||||
}
|
||||
if (conflictUri.id === "*") {
|
||||
return this.#resolveAllConflicts(cleanContent, stripped, signal, context);
|
||||
}
|
||||
const entry = getConflictHistory(this.session).get(conflictUri.id);
|
||||
if (!entry) {
|
||||
throw new ToolError(
|
||||
`Conflict #${conflictUri.id} not found. Conflict ids are registered when \`read\` surfaces a marker block; re-read the file to get a current id.`,
|
||||
const result =
|
||||
conflictUri.id === "*"
|
||||
? await this.#resolveAllConflicts(cleanContent, stripped, signal, context)
|
||||
: await this.#resolveSingleConflictById(conflictUri.id, cleanContent, stripped, signal, context);
|
||||
if (conflictUri.recoveredPrefix !== undefined) {
|
||||
appendNoteToResult(
|
||||
result,
|
||||
`Note: stripped erroneous '${conflictUri.recoveredPrefix}:' prefix from path; conflict URIs are global (use \`conflict://${conflictUri.id}\`, not \`<file>:conflict://${conflictUri.id}\`).`,
|
||||
);
|
||||
}
|
||||
return this.#resolveConflict(entry, cleanContent, stripped, signal, context);
|
||||
return result;
|
||||
}
|
||||
const resolvedArchivePath = await this.#resolveArchiveWritePath(path);
|
||||
if (resolvedArchivePath) {
|
||||
|
||||
@@ -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<void>;
|
||||
};
|
||||
|
||||
function createStubInputControllerContext(opts: {
|
||||
skillCommands: Map<string, string>;
|
||||
isStreaming: boolean;
|
||||
}) {
|
||||
function createStubInputControllerContext(opts: { skillCommands: Map<string, string>; 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";
|
||||
|
||||
@@ -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 () => {
|
||||
|
||||
@@ -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<T>(
|
||||
session: SessionManager,
|
||||
id: string,
|
||||
): CustomMessageEntry<T> {
|
||||
function readPersistedCustomMessageEntry<T>(session: SessionManager, id: string): CustomMessageEntry<T> {
|
||||
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", () => {
|
||||
|
||||
@@ -213,6 +213,27 @@ describe("parseConflictUri", () => {
|
||||
expect(() => parseConflictUri("conflict://abc")).toThrow(ToolError);
|
||||
expect(() => parseConflictUri("conflict://1/extra")).toThrow(ToolError);
|
||||
});
|
||||
|
||||
it("recovers an erroneous `<file>:` 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> = {}): ConflictEntry {
|
||||
|
||||
@@ -327,6 +327,46 @@ describe("write resolves conflicts via conflict://N", () => {
|
||||
expect(session.conflictHistory?.get(1)).toBeUndefined();
|
||||
});
|
||||
|
||||
it("auto-recovers a `<file>: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 `<file>: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);
|
||||
|
||||
Reference in New Issue
Block a user