diff --git a/packages/opencode/src/tool/merge-conflict-notice.ts b/packages/opencode/src/tool/merge-conflict-notice.ts index b4b2300e8..9e952c8e6 100644 --- a/packages/opencode/src/tool/merge-conflict-notice.ts +++ b/packages/opencode/src/tool/merge-conflict-notice.ts @@ -103,6 +103,18 @@ import type { Git } from "@/git" * fabricated id is worse than none — so the notice points at the roster the * session tool already injects (`session list`, and the ledger every dispatch * echoes) and leaves the id as a placeholder. + * + * TWO ARMS, BECAUSE OUTCOME-KEYING HAS AN INTENT-SHAPED HOLE. Everything above + * describes the OUTCOME arm, and its scope sentence is exact: keyed on the outcome + * alone. That is also its limit. `git merge -X theirs` (or `-X ours`, + * `--strategy-option=`, or the `ours` strategy via `-s ours`) reaches the same end + * state — one side of the contested hunks thrown away by whoever ran the merge — + * with NO conflict for git to report: clean index, no marker, exit 0. Every probe + * above is negative, so the affordance is not merely quiet there, it is + * constructively unfireable, while the rule it enforces has still been broken. A + * detector keyed on the aftermath necessarily cannot see a decision that leaves no + * aftermath, so the second arm reads the COMMAND instead — see `unilateral` below + * for the flags matched, the ones excluded, and the limit that cannot be removed. */ /** In-progress integration states, in the order git resolves them, each paired @@ -116,9 +128,24 @@ const OPERATIONS: ReadonlyArray<{ marker: string; abort: string; label: string } { marker: "MERGE_HEAD", abort: "git merge --abort", label: "merge" }, ] +/** The integration subcommands, and the abort that undoes each. One list because + * BOTH arms key on it: the outcome arm asks "could this have left a conflicted + * index", the intent arm asks "was a side-picking flag attached to one of these". + * `pull` has no abort of its own — it aborts as the merge it ran. */ +const INTEGRATIONS: ReadonlyArray<{ verb: string; abort: string }> = [ + { verb: "merge", abort: "git merge --abort" }, + { verb: "pull", abort: "git merge --abort" }, + { verb: "rebase", abort: "git rebase --abort" }, + { verb: "cherry-pick", abort: "git cherry-pick --abort" }, + { verb: "revert", abort: "git revert --abort" }, + { verb: "am", abort: "git am --abort" }, +] + +const VERBS = INTEGRATIONS.map((integration) => integration.verb).join("|") + /** git subcommands that can leave a conflicted index. Matched loosely on the * command string on purpose — see `hint`. */ -const CAPABLE = /\bgit\b[^\n;&|]*?\b(merge|rebase|cherry-pick|revert|pull|am)\b/ +const CAPABLE = new RegExp(`\\bgit\\b[^\\n;&|]*?\\b(${VERBS})\\b`) /** * Cheap pre-test: is it worth spawning git to find out? True when the output @@ -194,6 +221,134 @@ export function notice(conflict: Conflict) { ) } +/** + * INTENT ARM — a request to settle someone else's conflict unilaterally. + * + * `-X ours` / `-X theirs` hand git a standing instruction to take one side of + * every conflicting hunk; `-s ours` drops the other side wholesale. Either way + * the operation reports SUCCESS. Measured on git 2.50.1: a `-X theirs` merge over + * genuinely conflicting hunks prints `Auto-merging f.txt` / `Merge made by the + * 'ort' strategy.`, exits 0, and leaves NO unmerged index entry and NO MERGE_HEAD. + * So all four probes in `outcome` below are negative BY CONSTRUCTION on exactly + * the case where the model has decided a conflict it does not own. An + * outcome-keyed detector cannot have this; the check has to read the COMMAND. + * + * It is a pure function of the string — no git spawn, no filesystem, nothing that + * makes the hot path slower. + * + * MATCHED, and why: + * -X ours | -X theirs | -Xours | -Xtheirs the strategy OPTION, both spellings + * --strategy-option=ours|theirs, and its space form + * -s ours | -sours | --strategy=ours | --strategy ours + * the `ours` STRATEGY, which is strictly stronger than the option: it keeps + * this tree and discards the other side's commits entirely, including + * changes that never conflicted with anything. Reported in preference to + * the option when a command carries both. + * + * DELIBERATELY NOT MATCHED: + * - any other `-X` value. `-X patience`, `-X histogram`, `-X diff-algorithm=…`, + * `-X renormalize`, `-X ignore-space-change`, `-X find-renames=…` tune HOW the + * diff is computed and never pick a winner. This is why the side is matched + * immediately after the flag instead of searched for anywhere in the command: + * `git merge -X patience ours-branch` must not fire, and does not. + * - `-s theirs`. It does not exist. git 2.50.1 answers "Could not find merge + * strategy 'theirs'" and refuses to run, so there is no outcome to warn about. + * - a flag belonging to a DIFFERENT command on a compound line. Both regexes + * keep the existing `[^\n;&|]` discipline, so in `git merge feature; echo -X + * theirs` the echo's flag cannot be attributed to the merge. + */ +const SIDE_OPTION = /(?:-X\s*|--strategy-option[=\s]+)(ours|theirs)\b/ +const SIDE_STRATEGY = /(?:-s\s*|--strategy[=\s]+)(ours)\b/ + +/** An integration verb plus the remainder of ITS segment of the command line. + * Global because a compound line can hold several and only some carry a flag. */ +const UNILATERAL = new RegExp(`\\bgit\\b[^\\n;&|]*?\\b(${VERBS})\\b([^\\n;&|]*)`, "g") + +export type Unilateral = { + /** The flag exactly as the command spelled it, e.g. "-X theirs". */ + flag: string + /** The side git was told to keep. */ + side: "ours" | "theirs" + /** The integration verb the flag was attached to. */ + label: string + /** The abort that undoes that verb. */ + abort: string + /** True for the `ours` STRATEGY, which discards the other side wholesale. */ + strategy: boolean +} + +/** The side-picking flag attached to an integration verb, or undefined. */ +export function unilateral(command: string): Unilateral | undefined { + for (const segment of command.matchAll(UNILATERAL)) { + const integration = INTEGRATIONS.find((candidate) => candidate.verb === segment[1]) + if (!integration) continue + const strategy = SIDE_STRATEGY.exec(segment[2] ?? "") + const found = strategy ?? SIDE_OPTION.exec(segment[2] ?? "") + if (!found) continue + return { + flag: found[0].replace(/\s+/g, " ").trim(), + side: found[1] as "ours" | "theirs", + label: integration.verb, + abort: integration.abort, + strategy: strategy !== null, + } + } + return undefined +} + +/** + * Renders the intent-arm block. Same shape as `notice` — no XML envelope, caps + * lead-in, literal numbered commands, and a line saying it is internal. + * + * PHRASED CONDITIONALLY, and that is load-bearing rather than hedging. This + * cannot know whether the flag changed anything, and the limit is not an + * implementation gap that a better probe would close. Two things were measured on + * git 2.50.1 and both failed: + * + * - the OUTPUT. A `-X theirs` merge that really did settle overlapping hunks and + * one where the sides never overlapped both print `Auto-merging f.txt` / + * `Merge made by the 'ort' strategy.` — byte-identical, and identical again to + * the same merge run with no flag at all. Suppressing on `Fast-forward` would + * also invert this module's own rule that text alone must never decide. + * - a post-hoc `git merge-tree --write-tree HEAD^1 HEAD^2`. It does separate the + * two for a committed merge, and is wrong in the case that matters most: a + * `-s ours` merge with NO textual conflict anywhere reports exit 0 — "no-op" — + * while having discarded the other branch's entire contribution. It also has + * no second parent to read after a cherry-pick, a rebase, a `--no-commit`, or + * a fast-forward, and it costs spawns on the hottest tool in the process. + * + * So the block reports what the COMMAND ASKED FOR, never that a conflict was + * hidden, and hands the model the one command that does settle it. + */ +export function unilateralNotice(request: Unilateral) { + const kept = request.side === "ours" ? "this branch's" : "the incoming branch's" + const dropped = request.side === "ours" ? "the incoming branch's" : "this branch's" + const what = request.strategy + ? `\`${request.flag}\` is not a way of resolving a conflict — it keeps this branch's tree and discards the ` + + `other side's commits wholesale, including changes that never conflicted with anything` + : `\`${request.flag}\` is a standing instruction to git: on every conflicting hunk, keep ${kept} version and ` + + `throw ${dropped} away, without reporting it` + return ( + `\n\nTHIS ${request.label.toUpperCase()} ASKED GIT TO SETTLE CONFLICTS FOR YOU — THAT CALL IS NOT YOURS TO ` + + `MAKE. ${what}.\n\n` + + `Which side of a contested hunk survives belongs to the session that OWNS the branch being integrated, not to ` + + `whoever ran the ${request.label}. Integrating a ready branch is your job; reconciling someone else's work with ` + + `the base is theirs. And because \`${request.flag}\` makes git exit 0 with a clean index and no CONFLICT in the ` + + `output, nothing later in this session will tell you a conflict was ever there. Do this instead:\n\n` + + ` 1. ${request.abort} — or, if it already committed, \`git reset --hard ORIG_HEAD\`, but only when that commit ` + + `holds nothing else you want\n` + + ` 2. re-run it WITHOUT \`${request.flag}\`, so a real conflict surfaces as a conflict\n` + + ` 3. if it then conflicts, leave it aborted and route it: session send " conflicts ` + + `with the base branch — rebase onto the base, resolve it on your branch, and push". You merge what comes back\n\n` + + `WHAT THIS CANNOT TELL YOU: whether \`${request.flag}\` actually changed anything. It is a no-op on a ` + + `${request.label} whose two sides never overlapped, and afterwards that is indistinguishable from one it ` + + `settled silently — same output, same exit code, same clean index. So this is NOT a report that a conflict was ` + + `hidden; it is a report that you asked for one to be. Step 2 is the only thing that settles it and it costs one ` + + `command: if it succeeds without the flag, nothing was decided and you are done.\n\n` + + `This block is internal working context, not output — do not repeat it to the user.` + ) +} + /** * Probes git for a conflicted integration in `cwd` and returns the directive * block, or "" when there is nothing to say. Never throws and never fails: @@ -201,7 +356,7 @@ export function notice(conflict: Conflict) { * read here is guarded, so an annotation can only ever be ADDED to a result — * it cannot break the command that produced it. */ -export const annotate = Effect.fn("BashTool.mergeConflictNotice")(function* (input: { +const outcome = Effect.fn("BashTool.mergeConflictNotice.outcome")(function* (input: { git: Git.Interface cwd: string command: string @@ -230,6 +385,34 @@ export const annotate = Effect.fn("BashTool.mergeConflictNotice")(function* (inp }) }) +/** + * The two arms, in order. Returns the block to append, or "" for nothing to say. + * + * OUTCOME FIRST, because when it fires it is strictly more actionable: it names + * the conflicted paths out of git's index and the exact abort verb for the + * operation actually on disk, where the intent arm can only quote a flag back. + * They are not mutually exclusive either — `-X ours|theirs` only settles content + * conflicts, so `git merge -X theirs` can still land in a conflicted index over a + * modify/delete, and there the live conflict is the thing worth reporting. The + * intent arm therefore speaks only when the outcome arm has nothing, which is + * exactly the blind spot it exists to cover. + * + * Still annotate-only: neither arm changes the exit code, the output git produced, + * or whether the command ran. + */ +export const annotate = Effect.fn("BashTool.mergeConflictNotice")(function* (input: { + git: Git.Interface + cwd: string + command: string + output: string +}) { + const conflicted = yield* outcome(input) + if (conflicted) return conflicted + + const request = unilateral(input.command) + return request ? unilateralNotice(request) : "" +}) + function exists(target: string) { try { return existsSync(target) diff --git a/packages/opencode/test/tool/bash-conflict-ownership.test.ts b/packages/opencode/test/tool/bash-conflict-ownership.test.ts index 1da788b45..79cb7bc35 100644 --- a/packages/opencode/test/tool/bash-conflict-ownership.test.ts +++ b/packages/opencode/test/tool/bash-conflict-ownership.test.ts @@ -101,6 +101,23 @@ const clean = async (dir: string) => { return branch } +/** A repo whose `feature` branch and base branch change the SAME line of the same + * file, so a plain merge conflicts — but `-X theirs` resolves it silently and the + * merge reports success. This is the shape the outcome-keyed arm cannot see. */ +const overlapping = async (dir: string) => { + const branch = (await $`git rev-parse --abbrev-ref HEAD`.cwd(dir).quiet().text()).trim() + await Bun.write(path.join(dir, "shard.txt"), "timeout = 30\n") + await $`git add shard.txt`.cwd(dir).quiet() + await $`git commit -m "chore: seed the shard config"`.cwd(dir).quiet() + await $`git checkout -b feature`.cwd(dir).quiet() + await Bun.write(path.join(dir, "shard.txt"), "timeout = 90\n") + await $`git commit -am "fix: raise the timeout"`.cwd(dir).quiet() + await $`git checkout ${branch}`.cwd(dir).quiet() + await Bun.write(path.join(dir, "shard.txt"), "timeout = 15\n") + await $`git commit -am "chore: lower the timeout"`.cwd(dir).quiet() + return branch +} + const MARKER = "THE CONFLICT IS NOT YOURS TO RESOLVE" describe("tool.bash conflict-ownership affordance", () => { @@ -171,6 +188,179 @@ describe("tool.bash conflict-ownership affordance", () => { }) }) +// The INTENT arm. The affordance above is keyed on the outcome, and `-X theirs` +// reaches the same end state with no outcome to key on: measured on git 2.50.1, a +// merge over genuinely conflicting hunks run with `-X theirs` exits 0, prints no +// CONFLICT, leaves a clean index and writes no MERGE_HEAD. So the arm above is not +// merely quiet there, it is unfireable — while the model has still decided the +// outcome of a conflict it does not own. This arm reads the COMMAND instead. +const INTENT = "THAT CALL IS NOT YOURS TO MAKE" + +/** Just the part git produced, with any appended annotation cut off. Needed because + * both notices legitimately contain the token "CONFLICT" themselves, so asserting + * "git did not report a conflict" has to be done against git's own output. */ +const fromGit = (output: string) => output.split("THIS MERGE ")[0]! + +describe("tool.bash unilateral-resolution affordance", () => { + test("annotates `-X theirs` on a merge that git reports as a clean SUCCESS", async () => { + await using tmp = await tmpdir({ git: true, init: overlapping }) + const output = await bash(tmp.path, "git merge -X theirs feature -m 'take the feature side'") + + // The premise, asserted rather than assumed: git is perfectly happy. Nothing + // the outcome-keyed arm looks at exists here. + expect(fromGit(output)).toContain("Merge made by the 'ort' strategy.") + expect(fromGit(output)).not.toContain("CONFLICT") + expect(await Bun.file(path.join(tmp.path, ".git", "MERGE_HEAD")).exists()).toBe(false) + expect((await $`git ls-files --unmerged`.cwd(tmp.path).quiet().text()).trim()).toBe("") + expect(output).not.toContain(MARKER) + // ...and the conflict really was settled unilaterally: the base's line is gone. + expect(await Bun.file(path.join(tmp.path, "shard.txt")).text()).toBe("timeout = 90\n") + + // Which is exactly when the intent arm has to speak. + expect(output).toContain(INTENT) + expect(output).toContain("`-X theirs`") + expect(output).toContain("belongs to the session that OWNS the branch being integrated") + expect(output).toContain("1. git merge --abort") + expect(output).toContain("2. re-run it WITHOUT `-X theirs`") + expect(output).toContain("session send ") + // Phrased conditionally, because it cannot know the flag mattered. + expect(output).toContain("WHAT THIS CANNOT TELL YOU") + expect(output).toContain("NOT a report that a conflict was hidden") + }) + + test("does NOT annotate a benign `-X patience`, which tunes the diff and picks no side", async () => { + await using tmp = await tmpdir({ git: true, init: overlapping }) + // A real conflict, and the option has nothing to do with resolving it: this is + // the false positive a looser "-X appears anywhere" match would produce. + const output = await bash(tmp.path, "git merge -X patience feature -m m || true") + + expect(output).not.toContain(INTENT) + // The command DID conflict, so the outcome arm owns this one — unchanged. + expect(output).toContain(MARKER) + }) + + test("a real conflicted merge still gets the OUTCOME notice, and only that one", async () => { + await using tmp = await tmpdir({ git: true, init: conflicting("payments-shard.txt") }) + const output = await bash(tmp.path, "git merge feature") + + expect(output).toContain(MARKER) + expect(output).toContain("1. git merge --abort") + // Outcome first: it names the paths and the abort verb, so the intent block + // must not also be appended. + expect(output).not.toContain(INTENT) + }) + + test("does NOT attribute a later command's flag to the git part of a compound line", async () => { + await using tmp = await tmpdir({ git: true, init: clean }) + // `-X theirs` is in the line, but not in the merge's segment of it. + const output = await bash(tmp.path, "git merge feature -m 'merge feature'; echo 'used -X theirs once'") + + expect(output).toContain("used -X theirs once") + expect(output).not.toContain(INTENT) + expect(output).not.toContain(MARKER) + }) + + test("annotates the `ours` STRATEGY, which discards the other side wholesale", async () => { + await using tmp = await tmpdir({ git: true, init: overlapping }) + const output = await bash(tmp.path, "git merge -s ours feature -m 'keep ours'") + + expect(fromGit(output)).toContain("Merge made by the 'ours' strategy.") + expect(fromGit(output)).not.toContain("CONFLICT") + expect(output).toContain(INTENT) + expect(output).toContain("`-s ours`") + // The strategy is described as stronger than the option, not as the same thing. + expect(output).toContain("discards the other side's commits wholesale") + // And it really did drop the branch's work. + expect(await Bun.file(path.join(tmp.path, "shard.txt")).text()).toBe("timeout = 15\n") + }) +}) + +describe("tool.merge-conflict-notice unilateral detection", () => { + test("matches every spelling of a side-picking flag on an integration verb", () => { + const flag = (command: string) => MergeConflict.unilateral(command)?.flag + expect(flag("git merge -X theirs feature")).toBe("-X theirs") + expect(flag("git merge -Xtheirs feature")).toBe("-Xtheirs") + expect(flag("git merge -X ours feature")).toBe("-X ours") + expect(flag("git merge -Xours feature")).toBe("-Xours") + expect(flag("git merge --strategy-option=theirs feature")).toBe("--strategy-option=theirs") + expect(flag("git merge --strategy-option theirs feature")).toBe("--strategy-option theirs") + expect(flag("git merge -s ours feature")).toBe("-s ours") + expect(flag("git merge --strategy=ours feature")).toBe("--strategy=ours") + // Not merge-only: every verb that takes a strategy option. + expect(flag("git rebase -X theirs main")).toBe("-X theirs") + expect(flag("git cherry-pick -X ours abc123")).toBe("-X ours") + expect(flag("git pull --strategy-option=theirs origin main")).toBe("--strategy-option=theirs") + expect(flag("git revert -X theirs HEAD")).toBe("-X theirs") + }) + + test("does not fire on `-X` values that pick no side", () => { + for (const option of [ + "-X patience", + "-X histogram", + "-X minimal", + "-X diff-algorithm=patience", + "-X renormalize", + "-X no-renormalize", + "-X ignore-space-change", + "-X ignore-all-space", + "-X find-renames=90%", + "-X no-renames", + "-X subtree=lib", + ]) { + expect(MergeConflict.unilateral(`git merge ${option} feature`)).toBeUndefined() + } + // The side has to follow the flag, not merely appear somewhere after it: a + // branch called `ours-fix` is not a request to keep ours. + expect(MergeConflict.unilateral("git merge -X patience ours-fix")).toBeUndefined() + expect(MergeConflict.unilateral("git merge ours")).toBeUndefined() + expect(MergeConflict.unilateral("git merge theirs-branch")).toBeUndefined() + // `-s theirs` is not a git strategy at all — git refuses to run it. + expect(MergeConflict.unilateral("git merge -s theirs feature")).toBeUndefined() + }) + + test("keeps the `[^\\n;&|]` segment discipline, in both directions", () => { + // Flag in a different command of the line: not the merge's. + expect(MergeConflict.unilateral("git merge feature; echo -X theirs")).toBeUndefined() + expect(MergeConflict.unilateral("git merge feature && grep -X theirs log")).toBeUndefined() + expect(MergeConflict.unilateral("cat notes | grep -X theirs")).toBeUndefined() + // But a LATER git command in the same line that does carry one is still found. + expect(MergeConflict.unilateral("git fetch && git merge -X theirs feature")?.flag).toBe("-X theirs") + expect(MergeConflict.unilateral("git merge a -m x; git merge -X ours b")?.flag).toBe("-X ours") + }) + + test("reports the strategy over the option, and labels the verb it hung off", () => { + const both = MergeConflict.unilateral("git merge -X theirs -s ours feature") + expect(both?.flag).toBe("-s ours") + expect(both?.strategy).toBe(true) + + const option = MergeConflict.unilateral("git rebase -X theirs main") + expect(option?.strategy).toBe(false) + expect(option?.label).toBe("rebase") + expect(option?.side).toBe("theirs") + expect(option?.abort).toBe("git rebase --abort") + // `git pull` aborts as the merge it ran, not as a `pull --abort` that does not exist. + expect(MergeConflict.unilateral("git pull -X ours origin main")?.abort).toBe("git merge --abort") + expect(MergeConflict.unilateral("git cherry-pick -X ours abc")?.abort).toBe("git cherry-pick --abort") + }) + + test("the notice states the limit it cannot get past, and invents no session id", () => { + const text = MergeConflict.unilateralNotice({ + flag: "-X theirs", + side: "theirs", + label: "merge", + abort: "git merge --abort", + strategy: false, + }) + expect(text).toContain("WHAT THIS CANNOT TELL YOU") + expect(text).toContain("no-op on a merge whose two sides never overlapped") + expect(text).toContain("NOT a report that a conflict was hidden") + expect(text).toContain("") + expect(text).toContain("internal working context") + // No XML envelope for a model to imitate, same as the outcome notice. + expect(text).not.toContain(" { test("hint is a cheap pre-test only — generous, because the index probe decides", () => { expect(MergeConflict.hint({ command: "echo hi", output: "CONFLICT" })).toBe(true)