From 0cdd381aeea9ae7c838729bded60af344cb42107 Mon Sep 17 00:00:00 2001 From: can1357 Date: Tue, 12 May 2026 11:03:26 +0200 Subject: [PATCH] feat(coding-agent/tools): added scoped conflict URI parsing in read tool - Added `ConflictScope` parsing for `conflict:///` 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. --- packages/coding-agent/CHANGELOG.md | 2 +- .../coding-agent/src/tools/conflict-detect.ts | 90 ++++++++++++++++-- packages/coding-agent/src/tools/read.ts | 44 ++++++++- packages/coding-agent/src/tools/write.ts | 5 + .../test/tools/conflict-detect.test.ts | 93 +++++++++++++++++++ .../test/tools/conflict-integration.test.ts | 81 ++++++++++++++++ 6 files changed, 304 insertions(+), 11 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index acab28dda..edbc1ac6a 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -1,13 +1,13 @@ # Changelog ## [Unreleased] - ### Breaking Changes - Changed the `eval` tool input format to a single-line `*** Cell :"" [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 diff --git a/packages/coding-agent/src/tools/conflict-detect.ts b/packages/coding-agent/src/tools/conflict-detect.ts index 4161473b1..7703ed718 100644 --- a/packages/coding-agent/src/tools/conflict-detect.ts +++ b/packages/coding-agent/src/tools/conflict-detect.ts @@ -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; /** diff --git a/packages/coding-agent/src/tools/read.ts b/packages/coding-agent/src/tools/read.ts index 62f330f70..05f4fa4e6 100644 --- a/packages/coding-agent/src/tools/read.ts +++ b/packages/coding-agent/src/tools/read.ts @@ -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. diff --git a/packages/coding-agent/src/tools/write.ts b/packages/coding-agent/src/tools/write.ts index efe82b0cd..cd98adc21 100644 --- a/packages/coding-agent/src/tools/write.ts +++ b/packages/coding-agent/src/tools/write.ts @@ -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( diff --git a/packages/coding-agent/test/tools/conflict-detect.test.ts b/packages/coding-agent/test/tools/conflict-detect.test.ts index 6c7527e1a..a4c5a07c1 100644 --- a/packages/coding-agent/test/tools/conflict-detect.test.ts +++ b/packages/coding-agent/test/tools/conflict-detect.test.ts @@ -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(""); diff --git a/packages/coding-agent/test/tools/conflict-integration.test.ts b/packages/coding-agent/test/tools/conflict-integration.test.ts index 3e3e28fba..7b402322b 100644 --- a/packages/coding-agent/test/tools/conflict-integration.test.ts +++ b/packages/coding-agent/test/tools/conflict-integration.test.ts @@ -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);