feat(coding-agent/tools): added scoped conflict URI parsing in read tool
- Added `ConflictScope` parsing for `conflict://<N>/<scope>` with `ours`, `theirs`, and `base` validation. - Added `read` support for full and scoped conflict URIs, rendering regions via `#readConflictRegion` with preserved formatting metadata. - Added conflict-count and error handling in read results, including `Conflict #N not found` responses. - Added `write`-path rejection for scoped conflict URIs, returning `ToolError` before lookup to enforce read-only behavior. - Added unit and integration tests for scope parsing, conflict rendering, and read/write conflict error scenarios.
This commit is contained in:
@@ -1,13 +1,13 @@
|
||||
# Changelog
|
||||
|
||||
## [Unreleased]
|
||||
|
||||
### Breaking Changes
|
||||
|
||||
- Changed the `eval` tool input format to a single-line `*** Cell <lang>:"<title>" [t:<duration>] [rst]` header per cell, replacing the `*** Begin <LANG>` / `*** End <LANG>` envelope and the standalone `*** Title:` / `*** Timeout:` / `*** Reset` directives. The lark grammar enforces a fixed attribute order; the runtime parser remains lenient (alias keys, bare positional tokens, single-quoted titles).
|
||||
|
||||
### Added
|
||||
|
||||
- Added `read` support for `conflict://<N>` and `read conflict://<N>/<scope>` to inspect unresolved conflict regions captured by a prior read, including `ours`, `theirs`, and `base` side views with original file line alignment
|
||||
- Added shorthand content tokens `@ours`, `@theirs`, `@both`, and `@base` to conflict-resolution writes using `path: "conflict://<N>"` so replacement content can be composed from recorded conflict sections
|
||||
- Added conflict count metadata to read results so conflict files now show a warning badge (`⚠ N`) in the read tool UI
|
||||
- Added support for explicit boolean `rst` values (`rst:true`, `rst:false`, `rst:1`, `rst:0`, `rst:yes`, `rst:no`, `rst:on`, `rst:off`) in `*** Cell` headers
|
||||
|
||||
@@ -209,33 +209,55 @@ export function getConflictHistory(session: ToolSession): ConflictHistory {
|
||||
return session.conflictHistory;
|
||||
}
|
||||
|
||||
/** Parsed `conflict://<N>` URI. */
|
||||
/** A side of a conflict block that `read conflict://N/<scope>` can render. */
|
||||
export type ConflictScope = "ours" | "theirs" | "base";
|
||||
|
||||
const CONFLICT_SCOPES = new Set<ConflictScope>(["ours", "theirs", "base"]);
|
||||
|
||||
/** Parsed `conflict://<N>` or `conflict://<N>/<scope>` URI. */
|
||||
export interface ParsedConflictUri {
|
||||
id: number;
|
||||
scope?: ConflictScope;
|
||||
}
|
||||
|
||||
const CONFLICT_URI_RE = /^conflict:\/\/(.+)$/;
|
||||
|
||||
/**
|
||||
* Parse a `conflict://<N>` URI. Returns `null` for non-conflict paths;
|
||||
* throws `ToolError` for a well-formed scheme with an invalid id so the
|
||||
* agent gets a clear actionable message rather than a confusing "not
|
||||
* found" later.
|
||||
* Parse a `conflict://<N>` or `conflict://<N>/<scope>` URI.
|
||||
*
|
||||
* Returns `null` for non-conflict paths; throws `ToolError` for a
|
||||
* well-formed scheme with an invalid id or scope so the agent gets a
|
||||
* clear actionable message rather than a confusing "not found" later.
|
||||
*/
|
||||
export function parseConflictUri(raw: string): ParsedConflictUri | null {
|
||||
const match = raw.match(CONFLICT_URI_RE);
|
||||
if (!match) return null;
|
||||
const tail = match[1];
|
||||
if (!/^\d+$/.test(tail)) {
|
||||
const slashIdx = tail.indexOf("/");
|
||||
const idPart = slashIdx === -1 ? tail : tail.slice(0, slashIdx);
|
||||
const scopePart = slashIdx === -1 ? undefined : tail.slice(slashIdx + 1);
|
||||
|
||||
if (!/^\d+$/.test(idPart)) {
|
||||
throw new ToolError(
|
||||
`Invalid conflict URI '${raw}': must be 'conflict://<N>' where N is a positive integer surfaced by a prior \`read\`.`,
|
||||
`Invalid conflict URI '${raw}': must be 'conflict://<N>' (or 'conflict://<N>/<scope>') where N is a positive integer surfaced by a prior \`read\`.`,
|
||||
);
|
||||
}
|
||||
const id = Number.parseInt(tail, 10);
|
||||
const id = Number.parseInt(idPart, 10);
|
||||
if (!Number.isFinite(id) || id < 1) {
|
||||
throw new ToolError(`Invalid conflict URI '${raw}': id must be ≥ 1.`);
|
||||
}
|
||||
return { id };
|
||||
|
||||
let scope: ConflictScope | undefined;
|
||||
if (scopePart !== undefined) {
|
||||
if (!CONFLICT_SCOPES.has(scopePart as ConflictScope)) {
|
||||
throw new ToolError(
|
||||
`Invalid conflict URI '${raw}': scope must be one of 'ours', 'theirs', 'base', or omitted (e.g. 'conflict://${id}/theirs').`,
|
||||
);
|
||||
}
|
||||
scope = scopePart as ConflictScope;
|
||||
}
|
||||
|
||||
return { id, scope };
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -324,6 +346,56 @@ export function expandContentTokens(content: string, entry: ConflictEntry): stri
|
||||
return out.join("\n");
|
||||
}
|
||||
|
||||
/** Reconstruct a conflict-marker line from prefix and optional label. */
|
||||
function markerLine(prefix: string, label: string | undefined): string {
|
||||
return label && label.length > 0 ? `${prefix} ${label}` : prefix;
|
||||
}
|
||||
|
||||
/**
|
||||
* Materialise a conflict block for `read conflict://<N>` (and its
|
||||
* `/ours` / `/theirs` / `/base` scopes).
|
||||
*
|
||||
* Returns:
|
||||
* - `lines`: the lines to render, ordered top-to-bottom.
|
||||
* - `startLine`: the 1-indexed file line number `lines[0]` corresponds
|
||||
* to, so the read formatter can label hashline anchors with the
|
||||
* original file positions.
|
||||
*
|
||||
* Bare (no scope) returns the full block including marker lines. A
|
||||
* scoped view returns only that side's body — `base` throws when the
|
||||
* recorded conflict is a 2-way merge with no base section.
|
||||
*/
|
||||
export function renderConflictRegion(
|
||||
entry: ConflictEntry,
|
||||
scope: ConflictScope | undefined,
|
||||
): { lines: string[]; startLine: number } {
|
||||
if (scope === "ours") {
|
||||
return { lines: [...entry.oursLines], startLine: entry.startLine + 1 };
|
||||
}
|
||||
if (scope === "theirs") {
|
||||
return { lines: [...entry.theirsLines], startLine: entry.separatorLine + 1 };
|
||||
}
|
||||
if (scope === "base") {
|
||||
if (entry.baseLines === undefined || entry.baseLine === undefined) {
|
||||
throw new ToolError(
|
||||
`Conflict #${entry.id} has no base section (2-way merge). 'conflict://${entry.id}/base' is only valid for diff3 conflicts.`,
|
||||
);
|
||||
}
|
||||
return { lines: [...entry.baseLines], startLine: entry.baseLine + 1 };
|
||||
}
|
||||
const out: string[] = [];
|
||||
out.push(markerLine("<<<<<<<", entry.oursLabel));
|
||||
out.push(...entry.oursLines);
|
||||
if (entry.baseLines !== undefined) {
|
||||
out.push(markerLine("|||||||", entry.baseLabel));
|
||||
out.push(...entry.baseLines);
|
||||
}
|
||||
out.push("=======");
|
||||
out.push(...entry.theirsLines);
|
||||
out.push(markerLine(">>>>>>>", entry.theirsLabel));
|
||||
return { lines: out, startLine: entry.startLine };
|
||||
}
|
||||
|
||||
const PREVIEW_SIDE_LINES = 6;
|
||||
|
||||
/**
|
||||
|
||||
@@ -33,7 +33,15 @@ import { ImageInputTooLargeError, loadImageInput, MAX_IMAGE_INPUT_BYTES } from "
|
||||
import { convertFileWithMarkit } from "../utils/markit";
|
||||
import { buildDirectoryTree, type DirectoryTree } from "../workspace-tree";
|
||||
import { type ArchiveReader, openArchive, parseArchivePathCandidates } from "./archive-reader";
|
||||
import { formatConflictWarning, getConflictHistory, scanConflictLines } from "./conflict-detect";
|
||||
import {
|
||||
type ConflictEntry,
|
||||
type ConflictScope,
|
||||
formatConflictWarning,
|
||||
getConflictHistory,
|
||||
parseConflictUri,
|
||||
renderConflictRegion,
|
||||
scanConflictLines,
|
||||
} from "./conflict-detect";
|
||||
import {
|
||||
executeReadUrl,
|
||||
isReadableUrlPath,
|
||||
@@ -1156,6 +1164,11 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
|
||||
if (readPath.startsWith("file://")) {
|
||||
readPath = expandPath(readPath);
|
||||
}
|
||||
|
||||
const conflictUri = parseConflictUri(readPath);
|
||||
if (conflictUri) {
|
||||
return this.#readConflictRegion(conflictUri.id, conflictUri.scope);
|
||||
}
|
||||
const displayMode = resolveFileDisplayMode(this.session);
|
||||
|
||||
const parsedUrlTarget = parseReadUrlTarget(readPath);
|
||||
@@ -1562,6 +1575,35 @@ export class ReadTool implements AgentTool<typeof readSchema, ReadToolDetails> {
|
||||
return resultBuilder.done();
|
||||
}
|
||||
|
||||
/**
|
||||
* Render a `conflict://<N>` (or `conflict://<N>/<scope>`) region as
|
||||
* regular file content. The lines are emitted with their original
|
||||
* file line numbers so hashline anchors line up with the source
|
||||
* file, and no truncation footer is appended.
|
||||
*/
|
||||
async #readConflictRegion(id: number, scope: ConflictScope | undefined): Promise<AgentToolResult<ReadToolDetails>> {
|
||||
const entry: ConflictEntry | undefined = 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.`,
|
||||
);
|
||||
}
|
||||
|
||||
const region = renderConflictRegion(entry, scope);
|
||||
const displayMode = resolveFileDisplayMode(this.session);
|
||||
const shouldAddHashLines = displayMode.hashLines;
|
||||
const shouldAddLineNumbers = shouldAddHashLines ? false : displayMode.lineNumbers;
|
||||
|
||||
const rawText = region.lines.join("\n");
|
||||
const formattedText = formatTextWithMode(rawText, region.startLine, shouldAddHashLines, shouldAddLineNumbers);
|
||||
|
||||
const details: ReadToolDetails = {
|
||||
resolvedPath: entry.absolutePath,
|
||||
displayContent: { text: rawText, startLine: region.startLine },
|
||||
};
|
||||
return toolResult<ReadToolDetails>(details).text(formattedText).sourcePath(entry.absolutePath).done();
|
||||
}
|
||||
|
||||
/**
|
||||
* Handle internal URLs (agent://, artifact://, memory://, skill://, rule://, local://, mcp://).
|
||||
* Supports pagination via offset/limit but rejects them when query extraction is used.
|
||||
|
||||
@@ -501,6 +501,11 @@ export class WriteTool implements AgentTool<typeof writeSchema, WriteToolDetails
|
||||
const { text: cleanContent, stripped } = stripWriteContent(this.session, content);
|
||||
const conflictUri = parseConflictUri(path);
|
||||
if (conflictUri) {
|
||||
if (conflictUri.scope) {
|
||||
throw new ToolError(
|
||||
`Conflict URI scope '/${conflictUri.scope}' is read-only — use \`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\`.`,
|
||||
);
|
||||
}
|
||||
const entry = getConflictHistory(this.session).get(conflictUri.id);
|
||||
if (!entry) {
|
||||
throw new ToolError(
|
||||
|
||||
@@ -5,6 +5,7 @@ import {
|
||||
expandContentTokens,
|
||||
formatConflictWarning,
|
||||
parseConflictUri,
|
||||
renderConflictRegion,
|
||||
scanConflictLines,
|
||||
spliceConflict,
|
||||
} from "@oh-my-pi/pi-coding-agent/tools/conflict-detect";
|
||||
@@ -186,6 +187,17 @@ describe("parseConflictUri", () => {
|
||||
expect(parseConflictUri("conflict://")).toBeNull();
|
||||
});
|
||||
|
||||
it("parses an optional scope segment", () => {
|
||||
expect(parseConflictUri("conflict://1/ours")).toEqual({ id: 1, scope: "ours" });
|
||||
expect(parseConflictUri("conflict://2/theirs")).toEqual({ id: 2, scope: "theirs" });
|
||||
expect(parseConflictUri("conflict://3/base")).toEqual({ id: 3, scope: "base" });
|
||||
});
|
||||
|
||||
it("rejects unknown scope tokens", () => {
|
||||
expect(() => parseConflictUri("conflict://1/both")).toThrow(/scope must be one of/);
|
||||
expect(() => parseConflictUri("conflict://1/extras")).toThrow(/scope must be one of/);
|
||||
});
|
||||
|
||||
it("rejects malformed ids with a ToolError", () => {
|
||||
expect(() => parseConflictUri("conflict://0")).toThrow(ToolError);
|
||||
expect(() => parseConflictUri("conflict://-1")).toThrow(ToolError);
|
||||
@@ -237,6 +249,87 @@ describe("spliceConflict", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("renderConflictRegion", () => {
|
||||
const twoWay = makeEntry({
|
||||
startLine: 10,
|
||||
separatorLine: 13,
|
||||
endLine: 15,
|
||||
oursLabel: "HEAD",
|
||||
theirsLabel: "feature/x",
|
||||
oursLines: ["ours-1", "ours-2"],
|
||||
theirsLines: ["theirs-1"],
|
||||
});
|
||||
const threeWay = makeEntry({
|
||||
startLine: 20,
|
||||
baseLine: 22,
|
||||
separatorLine: 24,
|
||||
endLine: 26,
|
||||
oursLabel: "HEAD",
|
||||
baseLabel: "common ancestor",
|
||||
theirsLabel: "feat",
|
||||
oursLines: ["o"],
|
||||
baseLines: ["b"],
|
||||
theirsLines: ["t"],
|
||||
});
|
||||
|
||||
it("returns full block with marker lines reconstructed from labels", () => {
|
||||
const region = renderConflictRegion(twoWay, undefined);
|
||||
expect(region.startLine).toBe(10);
|
||||
expect(region.lines).toEqual(["<<<<<<< HEAD", "ours-1", "ours-2", "=======", "theirs-1", ">>>>>>> feature/x"]);
|
||||
});
|
||||
|
||||
it("includes the base section in a diff3 full block", () => {
|
||||
const region = renderConflictRegion(threeWay, undefined);
|
||||
expect(region.startLine).toBe(20);
|
||||
expect(region.lines).toEqual([
|
||||
"<<<<<<< HEAD",
|
||||
"o",
|
||||
"||||||| common ancestor",
|
||||
"b",
|
||||
"=======",
|
||||
"t",
|
||||
">>>>>>> feat",
|
||||
]);
|
||||
});
|
||||
|
||||
it("omits the label when none was recorded", () => {
|
||||
const noLabels = makeEntry({
|
||||
startLine: 1,
|
||||
separatorLine: 3,
|
||||
endLine: 5,
|
||||
oursLabel: undefined,
|
||||
theirsLabel: undefined,
|
||||
oursLines: ["o"],
|
||||
theirsLines: ["t"],
|
||||
});
|
||||
const region = renderConflictRegion(noLabels, undefined);
|
||||
expect(region.lines[0]).toBe("<<<<<<<");
|
||||
expect(region.lines[region.lines.length - 1]).toBe(">>>>>>>");
|
||||
});
|
||||
|
||||
it("returns just the ours body with the line number after `<<<<<<<`", () => {
|
||||
const region = renderConflictRegion(twoWay, "ours");
|
||||
expect(region.startLine).toBe(11);
|
||||
expect(region.lines).toEqual(["ours-1", "ours-2"]);
|
||||
});
|
||||
|
||||
it("returns just the theirs body with the line number after `=======`", () => {
|
||||
const region = renderConflictRegion(twoWay, "theirs");
|
||||
expect(region.startLine).toBe(14);
|
||||
expect(region.lines).toEqual(["theirs-1"]);
|
||||
});
|
||||
|
||||
it("returns just the base body for a diff3 conflict", () => {
|
||||
const region = renderConflictRegion(threeWay, "base");
|
||||
expect(region.startLine).toBe(23);
|
||||
expect(region.lines).toEqual(["b"]);
|
||||
});
|
||||
|
||||
it("rejects `base` scope for a 2-way conflict", () => {
|
||||
expect(() => renderConflictRegion(twoWay, "base")).toThrow(/no base section/);
|
||||
});
|
||||
});
|
||||
|
||||
describe("formatConflictWarning", () => {
|
||||
it("emits empty string when no entries", () => {
|
||||
expect(formatConflictWarning([])).toBe("");
|
||||
|
||||
@@ -161,6 +161,72 @@ describe("read surfaces conflicts as a warning footer", () => {
|
||||
expect(session.conflictHistory?.get(1)).toBeDefined();
|
||||
expect(session.conflictHistory?.get(2)).toBeUndefined();
|
||||
});
|
||||
|
||||
it("renders the full conflict block via `read conflict://<N>`", async () => {
|
||||
const filePath = path.join(tempDir, "full.ts");
|
||||
await Bun.write(filePath, TWO_WAY);
|
||||
const session = createTestSession(tempDir);
|
||||
const read = await getTool(session, "read");
|
||||
|
||||
await read.execute("read-full-init", { path: "full.ts" });
|
||||
const result = await read.execute("read-full", { path: "conflict://1" });
|
||||
const text = getText(result);
|
||||
expect(text).toContain("<<<<<<< HEAD");
|
||||
expect(text).toContain("oldApi(x)");
|
||||
expect(text).toContain("=======");
|
||||
expect(text).toContain("newApi(x)");
|
||||
expect(text).toContain(">>>>>>> feature/x");
|
||||
// No conflict warning footer when expanding a single block by id.
|
||||
expect(text).not.toContain("⚠");
|
||||
});
|
||||
|
||||
it("renders only the theirs body via `read conflict://<N>/theirs`", async () => {
|
||||
const filePath = path.join(tempDir, "theirs.ts");
|
||||
await Bun.write(filePath, TWO_WAY);
|
||||
const session = createTestSession(tempDir);
|
||||
const read = await getTool(session, "read");
|
||||
|
||||
await read.execute("read-theirs-init", { path: "theirs.ts" });
|
||||
const result = await read.execute("read-theirs", { path: "conflict://1/theirs" });
|
||||
const text = getText(result);
|
||||
expect(text).toContain("newApi(x)");
|
||||
expect(text).not.toContain("<<<<<<<");
|
||||
expect(text).not.toContain("=======");
|
||||
expect(text).not.toContain(">>>>>>>");
|
||||
expect(text).not.toContain("oldApi(x)");
|
||||
});
|
||||
|
||||
it("renders the base body for a diff3 conflict via `/base`", async () => {
|
||||
const filePath = path.join(tempDir, "base.ts");
|
||||
await Bun.write(filePath, THREE_WAY);
|
||||
const session = createTestSession(tempDir);
|
||||
const read = await getTool(session, "read");
|
||||
|
||||
await read.execute("read-base-init", { path: "base.ts" });
|
||||
const result = await read.execute("read-base", { path: "conflict://1/base" });
|
||||
const text = getText(result);
|
||||
expect(text).toContain("base body");
|
||||
expect(text).not.toContain("ours body");
|
||||
expect(text).not.toContain("theirs body");
|
||||
});
|
||||
|
||||
it("rejects `/base` on a 2-way conflict with a clear error", async () => {
|
||||
const filePath = path.join(tempDir, "no-base.ts");
|
||||
await Bun.write(filePath, TWO_WAY);
|
||||
const session = createTestSession(tempDir);
|
||||
const read = await getTool(session, "read");
|
||||
|
||||
await read.execute("read-no-base-init", { path: "no-base.ts" });
|
||||
const promise = read.execute("read-no-base", { path: "conflict://1/base" });
|
||||
await expect(promise).rejects.toThrow(/no base section/);
|
||||
});
|
||||
|
||||
it("errors clearly when the conflict id is unknown", async () => {
|
||||
const session = createTestSession(tempDir);
|
||||
const read = await getTool(session, "read");
|
||||
const promise = read.execute("read-missing", { path: "conflict://99" });
|
||||
await expect(promise).rejects.toThrow(/Conflict #99 not found/);
|
||||
});
|
||||
});
|
||||
|
||||
describe("write resolves conflicts via conflict://N", () => {
|
||||
@@ -296,6 +362,21 @@ describe("write resolves conflicts via conflict://N", () => {
|
||||
);
|
||||
});
|
||||
|
||||
it("rejects scoped conflict URIs on write (read-only)", async () => {
|
||||
const filePath = path.join(tempDir, "scoped.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-scoped", { path: "scoped.ts" });
|
||||
await expect(write.execute("write-scoped", { path: "conflict://1/theirs", content: "x" })).rejects.toThrow(
|
||||
/read-only/,
|
||||
);
|
||||
// File untouched.
|
||||
expect(await Bun.file(filePath).text()).toBe(TWO_WAY);
|
||||
});
|
||||
|
||||
it("rejects stale resolutions when the file changed out of band", async () => {
|
||||
const filePath = path.join(tempDir, "stale.ts");
|
||||
await Bun.write(filePath, TWO_WAY);
|
||||
|
||||
Reference in New Issue
Block a user