diff --git a/.changeset/fn-7554-pr-based-revert.md b/.changeset/fn-7554-pr-based-revert.md new file mode 100644 index 0000000000..5b164b3be2 --- /dev/null +++ b/.changeset/fn-7554-pr-based-revert.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": minor +--- + +summary: Open a revert PR for done/archived tasks when autoMerge is disabled instead of refusing. +category: feature +dev: `POST /api/tasks/:id/revert` gains an additive `{ mode: "pr", clean: true, prUrl, prNumber, revertBranch, existingPr? }` result for clean single-repo reverts under `autoMerge:false`, reusing `GitHubClient.createPr`, `findPrForBranch` idempotency, and the `manual:true` PR handoff. New engine export `prepareRevertPrBranch` (packages/engine/src/task-revert.ts) prepares the dedicated `fusion/revert-` branch without ever mutating the base branch. Existing `{ mode: "git" | "ai", ... }` shapes and the `autoMerge:true` path are unchanged. diff --git a/docs/task-management.md b/docs/task-management.md index 6af1f9b47f..a3890b2ea8 100644 --- a/docs/task-management.md +++ b/docs/task-management.md @@ -685,7 +685,7 @@ Recovery/backfill guidance: - Git-path response contract (additive only): `{ mode: "git", clean, revertCommitSha?, revertCommitShas?, conflicts?, alreadyReverted?, unsupported?, needsHuman?, reason? }`. A clean revert lands a `revert(FN-xxxx): ...` commit carrying a `Fusion-Task-Id` trailer on the resolved base branch; `revertCommitShas` reports every commit created (all of them for `per-sha`, the single one for `squash`) alongside the existing `revertCommitSha`. - AI-undo response contract: `{ mode: "ai", createdTaskId: "FN-YYYY", alreadyOpen?: true }`. The created task is an ordinary `triage`-column board task (via the normal `store.createTask` path) that references the source task's id, mission, and landed files, and instructs undoing the source task's behavior while preserving unrelated later changes to the same files, using a `revert(FN-xxxx): ...` commit convention. It carries NO dependency on the (already done/archived) source task. A `sourceMetadata.revertOf` marker makes repeated fallback calls idempotent — while an AI-undo task for that source is still open, a further call returns the same `createdTaskId` with `alreadyOpen: true` instead of creating a duplicate; a prior undo task that itself reached `done`/`archived` does not suppress a fresh one. - **Workspace (multi-repo) tasks (FN-7547):** tasks with `workspaceWorktrees` populated (`isWorkspaceTask`) are revertable too — the route dispatches to a dedicated workspace path that reasons about every sub-repo's integration branch as ONE all-or-nothing unit. It resolves each sub-repo's attributable commit(s), dry-run classifies every sub-repo first, and only commits a `revert(FN-xxxx): ...` commit on EACH sub-repo when every sub-repo classifies clean/already-reverted; if any sub-repo conflicts, no sub-repo is committed and every touched sub-repo worktree is rolled back to its pre-call state. Response contract for workspace tasks: `{ mode: "git", clean, workspace: { repos: [{ repo, classification, revertCommitSha?, conflicts?, alreadyReverted? }] }, conflicts?: {repo, file, ...}[] }`. A conflicting workspace result still falls back to the AI-undo task under `"auto"` mode, same as a single-repo conflicting result. -- `autoMerge:false` projects return `needsHuman` and never trigger the AI-undo fallback (a human/future UI decides). +- **`autoMerge:false` PR-based revert (FN-7554):** for a single-repo task whose git revert classifies **clean**, `autoMerge:false` no longer dead-ends at `needsHuman`. The route prepares a dedicated `fusion/revert-` branch off the resolved base branch (via the engine's `prepareRevertPrBranch`, which NEVER writes to the base branch itself), pushes it, and opens a GitHub PR through the same owner/repo resolution, `githubRateLimiter` gate, `findPrForBranch` idempotency, and `manual: true` handoff as `POST /tasks/:id/pr/create`. Response: `{ mode: "pr", clean: true, prUrl, prNumber, revertBranch, existingPr? }` — a second call while the PR is still open links the existing PR (`existingPr: true`) instead of re-pushing. GitHub unconfigured or rate-limited still degrades gracefully to `{ mode: "git", needsHuman: true, reason }`, and a conflicting/unsupported/already-reverted classification is unaffected (no PR is opened; `"auto"` mode still falls back to the AI-undo task on conflict/unsupported). Workspace (multi-repo) tasks are not yet covered by this PR path — they keep the existing `needsHuman` result under `autoMerge:false`. - **Dashboard auto-linking (FN-7555):** the AI-undo task's card shows an "Undo of FN-xxxx" chip and its detail view shows a clickable "Created to undo FN-xxxx" link back to the source task. The source task's detail view shows an "Undo task: FN-YYYY" link whenever an OPEN undo task referencing it exists in the loaded tasks (matching `TaskStore.findOpenRevertTaskForSource`'s open-only semantics — a `done`/`archived`/soft-deleted undo task is never surfaced as active). Both directions are derived client-side from `sourceMetadata.revertOf`; no new API. A dedicated Done/Archived card revert-trigger action is still a separate follow-up (see FN-7525). diff --git a/packages/dashboard/src/__tests__/task-revert-route.test.ts b/packages/dashboard/src/__tests__/task-revert-route.test.ts index 9684e863db..0e60955724 100644 --- a/packages/dashboard/src/__tests__/task-revert-route.test.ts +++ b/packages/dashboard/src/__tests__/task-revert-route.test.ts @@ -18,6 +18,7 @@ import { execFileSync } from "node:child_process"; import type { Task, TaskStore } from "@fusion/core"; import { createApiRoutes } from "../routes.js"; import { request as performRequest } from "../test-request.js"; +import { githubRateLimiter } from "../github-poll.js"; // FNXC:TaskRevert 2026-07-04-00:00: the route now guards against `rootDir` // (the shared user checkout) sitting on a branch other than the resolved @@ -31,11 +32,18 @@ function makeGitRepoOnMain(): string { execFileSync("git", ["config", "user.email", "test@example.com"], { cwd: dir }); execFileSync("git", ["config", "user.name", "Test"], { cwd: dir }); execFileSync("git", ["commit", "--allow-empty", "-m", "init"], { cwd: dir }); + // FN-7554: a local bare "origin" remote lets `mode:"pr"` tests exercise a + // REAL `git push -u origin ` without any network dependency. + const originDir = mkdtempSync(join(tmpdir(), "kb-task-revert-route-origin-")); + execFileSync("git", ["init", "--bare", "-b", "main"], { cwd: originDir }); + execFileSync("git", ["remote", "add", "origin", originDir], { cwd: dir }); + execFileSync("git", ["push", "-u", "origin", "main"], { cwd: dir }); return dir; } const performTaskRevertMock = vi.fn(); const revertWorkspaceTaskMock = vi.fn(); +const prepareRevertPrBranchMock = vi.fn(); vi.mock("@fusion/engine", async (importOriginal) => { const actual = await importOriginal(); @@ -43,9 +51,25 @@ vi.mock("@fusion/engine", async (importOriginal) => { ...actual, performTaskRevert: (...args: unknown[]) => performTaskRevertMock(...args), revertWorkspaceTask: (...args: unknown[]) => revertWorkspaceTaskMock(...args), + prepareRevertPrBranch: (...args: unknown[]) => prepareRevertPrBranchMock(...args), }; }); +// FN-7554: stub GitHubClient at the route boundary — `findPrForBranch`/`createPr` +// idempotency + push/create behavior is exercised here; real GitHubClient HTTP/gh-CLI +// behavior is covered by github.test.ts. +const findPrForBranchMock = vi.fn(); +const createPrMock = vi.fn(); + +vi.mock("../github.js", () => ({ + GitHubClient: vi.fn().mockImplementation(function (this: unknown) { + return { + findPrForBranch: (...args: unknown[]) => findPrForBranchMock(...args), + createPr: (...args: unknown[]) => createPrMock(...args), + }; + }), +})); + // FNXC:TaskRevert 2026-07-04-00:00 (FN-7524): `createAiUndoTask` is NOT mocked — // these route tests exercise the real engine helper against a fake store // (`createTask`/`findOpenRevertTaskForSource`), proving the route wires the @@ -84,7 +108,7 @@ function makeWorkspaceTask(overrides: Partial): Task { function createMockStore( task: Task, - opts?: { openUndoTask?: Task | null; createdUndoTask?: Task }, + opts?: { openUndoTask?: Task | null; createdUndoTask?: Task; autoMerge?: boolean }, ): TaskStore { let nextId = 800; const createTask = vi.fn().mockImplementation(async (input: { description: string; source?: { sourceParentTaskId?: string; sourceMetadata?: Record } }) => { @@ -104,12 +128,15 @@ function createMockStore( const findOpenRevertTaskForSource = vi.fn().mockResolvedValue(opts?.openUndoTask ?? null); return { getSettings: vi.fn().mockResolvedValue({}), - getSettingsFast: vi.fn().mockResolvedValue({ autoMerge: true }), + getSettingsFast: vi.fn().mockResolvedValue({ autoMerge: opts?.autoMerge ?? true }), getRootDir: vi.fn().mockReturnValue(makeGitRepoOnMain()), getTask: vi.fn().mockResolvedValue(task), getTaskCommitAssociationsByLineageId: vi.fn().mockResolvedValue([]), createTask, findOpenRevertTaskForSource, + updatePrInfo: vi.fn().mockResolvedValue(task), + addPrInfo: vi.fn().mockResolvedValue(task), + logEntry: vi.fn().mockResolvedValue(undefined), on: vi.fn(), off: vi.fn(), } as unknown as TaskStore; @@ -459,3 +486,168 @@ describe("POST /tasks/:id/revert — FN-7524 mode + AI-undo fallback", () => { expect(performTaskRevertMock.mock.calls[0]?.[0]).toMatchObject({ granularity: "squash" }); }); }); + +// FN-7554: mode:"pr" — PR-based revert for autoMerge:false projects. +describe("POST /tasks/:id/revert — FN-7554 mode:'pr' (autoMerge:false)", () => { + const originalGithubRepository = process.env.GITHUB_REPOSITORY; + + afterEach(() => { + vi.clearAllMocks(); + vi.restoreAllMocks(); + if (originalGithubRepository === undefined) { + delete process.env.GITHUB_REPOSITORY; + } else { + process.env.GITHUB_REPOSITORY = originalGithubRepository; + } + }); + + it("clean + autoMerge:false → mode:'pr', pushes and creates the PR with manual:true persistence", async () => { + process.env.GITHUB_REPOSITORY = "o/r"; + const task = makeTask({ id: "FN-100", column: "done" }); + const store = createMockStore(task, { autoMerge: false }); + const rootDir = (store.getRootDir as () => string)(); + // `prepareRevertPrBranch` is mocked (real branch-prep behavior is proven by + // task-revert-pr.real-git.test.ts), so create the branch it would have + // created locally, so the route's REAL `git push -u origin ` has + // something to push. + execFileSync("git", ["branch", "fusion/revert-fn-100"], { cwd: rootDir }); + vi.spyOn(githubRateLimiter, "canMakeRequest").mockReturnValue(true); + findPrForBranchMock.mockResolvedValue(null); + prepareRevertPrBranchMock.mockResolvedValue({ + eligible: true, + revertBranch: "fusion/revert-fn-100", + revertCommitShas: ["abc"], + }); + createPrMock.mockResolvedValue({ number: 7, url: "https://github.com/o/r/pull/7" }); + + const res = await REQUEST(createApp(store), "POST", `/api/tasks/${task.id}/revert`); + expect(res.status).toBe(200); + expect(res.body).toMatchObject({ + mode: "pr", + clean: true, + prUrl: "https://github.com/o/r/pull/7", + prNumber: 7, + revertBranch: "fusion/revert-fn-100", + }); + expect(createPrMock).toHaveBeenCalledTimes(1); + expect(createPrMock.mock.calls[0]?.[0]).toMatchObject({ head: "fusion/revert-fn-100" }); + expect(typeof createPrMock.mock.calls[0]?.[0]?.body).toBe("string"); + expect((createPrMock.mock.calls[0]?.[0]?.body as string).length).toBeGreaterThan(0); + expect(store.updatePrInfo as ReturnType).toHaveBeenCalledWith( + task.id, + expect.objectContaining({ manual: true, number: 7 }), + ); + expect(performTaskRevertMock).not.toHaveBeenCalled(); + }); + + it("existing PR idempotency: links the existing PR without re-preparing/re-pushing", async () => { + process.env.GITHUB_REPOSITORY = "o/r"; + const task = makeTask({ id: "FN-100", column: "done" }); + const store = createMockStore(task, { autoMerge: false }); + vi.spyOn(githubRateLimiter, "canMakeRequest").mockReturnValue(true); + findPrForBranchMock.mockResolvedValue({ number: 9, url: "https://github.com/o/r/pull/9" }); + + const res = await REQUEST(createApp(store), "POST", `/api/tasks/${task.id}/revert`); + expect(res.status).toBe(200); + expect(res.body).toMatchObject({ + mode: "pr", + clean: true, + prUrl: "https://github.com/o/r/pull/9", + prNumber: 9, + existingPr: true, + }); + expect(prepareRevertPrBranchMock).not.toHaveBeenCalled(); + expect(createPrMock).not.toHaveBeenCalled(); + }); + + it("GitHub unconfigured degrade: no GITHUB_REPOSITORY and no git remote → needsHuman", async () => { + delete process.env.GITHUB_REPOSITORY; + const task = makeTask({ id: "FN-100", column: "done" }); + const store = createMockStore(task, { autoMerge: false }); + + const res = await REQUEST(createApp(store), "POST", `/api/tasks/${task.id}/revert`); + expect(res.status).toBe(200); + expect(res.body).toMatchObject({ mode: "git", needsHuman: true }); + expect(String((res.body as { reason?: string }).reason ?? "")).toMatch(/no GitHub repository/i); + expect(prepareRevertPrBranchMock).not.toHaveBeenCalled(); + expect(createPrMock).not.toHaveBeenCalled(); + }); + + it("rate-limited degrade: needsHuman without touching prepareRevertPrBranch/createPr", async () => { + process.env.GITHUB_REPOSITORY = "o/r"; + const task = makeTask({ id: "FN-100", column: "done" }); + const store = createMockStore(task, { autoMerge: false }); + vi.spyOn(githubRateLimiter, "canMakeRequest").mockReturnValue(false); + + const res = await REQUEST(createApp(store), "POST", `/api/tasks/${task.id}/revert`); + expect(res.status).toBe(200); + expect(res.body).toMatchObject({ mode: "git", needsHuman: true }); + expect(String((res.body as { reason?: string }).reason ?? "")).toMatch(/rate limit/i); + expect(prepareRevertPrBranchMock).not.toHaveBeenCalled(); + expect(createPrMock).not.toHaveBeenCalled(); + }); + + it("conflicting under autoMerge:false, mode:'git' → { mode: 'git', clean: false, conflicts } without a PR", async () => { + process.env.GITHUB_REPOSITORY = "o/r"; + const task = makeTask({ id: "FN-100", column: "done" }); + const store = createMockStore(task, { autoMerge: false }); + vi.spyOn(githubRateLimiter, "canMakeRequest").mockReturnValue(true); + findPrForBranchMock.mockResolvedValue(null); + prepareRevertPrBranchMock.mockResolvedValue({ + eligible: false, + classification: "conflicting", + conflicts: [{ file: "foo.ts", status: "UU" }], + }); + + const res = await POST_JSON(createApp(store), `/api/tasks/${task.id}/revert`, { mode: "git" }); + expect(res.status).toBe(200); + expect(res.body).toMatchObject({ mode: "git", clean: false, conflicts: [{ file: "foo.ts", status: "UU" }] }); + expect(createPrMock).not.toHaveBeenCalled(); + }); + + it("conflicting under autoMerge:false, mode:'auto' → falls back to the AI-undo task", async () => { + process.env.GITHUB_REPOSITORY = "o/r"; + const task = makeTask({ id: "FN-960", column: "done" }); + const store = createMockStore(task, { autoMerge: false }); + vi.spyOn(githubRateLimiter, "canMakeRequest").mockReturnValue(true); + findPrForBranchMock.mockResolvedValue(null); + prepareRevertPrBranchMock.mockResolvedValue({ + eligible: false, + classification: "conflicting", + conflicts: [{ file: "foo.ts", status: "UU" }], + }); + + const res = await POST_JSON(createApp(store), `/api/tasks/${task.id}/revert`, { mode: "auto" }); + expect(res.status).toBe(200); + expect(res.body).toMatchObject({ mode: "ai" }); + expect((res.body as { createdTaskId?: string }).createdTaskId).toBeTruthy(); + expect(createPrMock).not.toHaveBeenCalled(); + }); + + it("regression — autoMerge:true unchanged: still calls performTaskRevert and returns the existing shape", async () => { + process.env.GITHUB_REPOSITORY = "o/r"; + const task = makeTask({ id: "FN-970", column: "done" }); + const store = createMockStore(task, { autoMerge: true }); + performTaskRevertMock.mockResolvedValue({ mode: "git", clean: true, revertCommitSha: "abc123", revertCommitShas: ["abc123"] }); + + const res = await REQUEST(createApp(store), "POST", `/api/tasks/${task.id}/revert`); + expect(res.status).toBe(200); + expect(res.body).toMatchObject({ mode: "git", clean: true, revertCommitSha: "abc123" }); + expect(prepareRevertPrBranchMock).not.toHaveBeenCalled(); + expect(createPrMock).not.toHaveBeenCalled(); + expect(performTaskRevertMock).toHaveBeenCalledTimes(1); + }); + + it("regression — non-done/archived guard unchanged: still 4xx before any engine/GitHub call", async () => { + process.env.GITHUB_REPOSITORY = "o/r"; + const task = makeTask({ id: "FN-971", column: "in-progress" }); + const store = createMockStore(task, { autoMerge: false }); + + const res = await REQUEST(createApp(store), "POST", `/api/tasks/${task.id}/revert`); + expect(res.status).toBeGreaterThanOrEqual(400); + expect(res.status).toBeLessThan(500); + expect(prepareRevertPrBranchMock).not.toHaveBeenCalled(); + expect(performTaskRevertMock).not.toHaveBeenCalled(); + expect(createPrMock).not.toHaveBeenCalled(); + }); +}); diff --git a/packages/dashboard/src/routes/register-task-workflow-routes.ts b/packages/dashboard/src/routes/register-task-workflow-routes.ts index fd3d724078..ec268d558c 100644 --- a/packages/dashboard/src/routes/register-task-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-task-workflow-routes.ts @@ -14,6 +14,7 @@ import type { DuplicateMatch, RunAuditEvent, ArtifactType, + PrInfo, } from "@fusion/core"; import { COLUMNS, @@ -47,6 +48,7 @@ import { type NearDuplicateCandidate, } from "@fusion/core"; import { GitHubClient } from "../github.js"; +import { githubRateLimiter } from "../github-poll.js"; import { createTrackingIssueForTask } from "../github-tracking-hook.js"; import { parseGitHubBadgeUrl } from "./register-git-github.js"; import { @@ -56,7 +58,9 @@ import { revertWorkspaceTask, TaskRevertError, createAiUndoTask, + prepareRevertPrBranch, type AiUndoTaskResult, + type PrepareRevertPrBranchResult, } from "@fusion/engine"; import { buildBoardWorkflowsPayload } from "./board-workflows.js"; import { isBackwardMoveBlockedByOpenPr, PR_OPEN_BLOCKS_MOVE_BACK_MESSAGE } from "./register-pull-requests-routes.js"; @@ -1819,6 +1823,184 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork }); } + /* + FNXC:TaskRevert 2026-07-05-00:00 (FN-7554 — mode:"pr" contract, additive over FN-7523/24/47/48): + `performTaskRevert` refuses (`needsHuman`) whenever autoMerge is + effectively off, because it would otherwise force-write a revert commit + directly onto `baseBranch` — a branch this project has opted out of + automated writes to. Instead of stopping at that dead end, open a real + revert PR: `prepareRevertPrBranch` (engine) applies the revert commit(s) + onto a DEDICATED `fusion/revert-` branch off `baseBranch` HEAD + (never mutating `baseBranch` itself), then this route pushes that branch + and opens a PR via `GitHubClient.createPr` — reusing the exact + owner/repo resolution, `githubRateLimiter` gate, `findPrForBranch` + idempotency, and `manual: true` handoff that `/pr/create` already uses. + This ONLY applies to the single-repo git path (workspace tasks already + returned above) and ONLY for `mode !== "ai"` (the `mode === "ai"` case + already returned earlier in this handler). Every existing + `{ mode: "git" | "ai", ... }` result shape is unchanged — this adds a + new `{ mode: "pr", ... }` variant. Graceful `needsHuman` degrade (NOT a + thrown error) covers: GitHub unconfigured, and GitHub rate-limited — + both leave the caller with the same actionable `needsHuman` contract + the `autoMerge:true` code path never has to think about. + PR-based revert of WORKSPACE (multi-repo) tasks under autoMerge:false is + explicitly deferred (would require per-sub-repo branches + multiple + PRs) — see the FN-7554 follow-up task; workspace tasks above still get + the existing `needsHuman` result from `revertWorkspaceTask`. + */ + const effectiveAutoMerge = task.autoMerge ?? settings.autoMerge ?? true; + if (effectiveAutoMerge === false) { + let owner: string; + let repo: string; + const envRepo = process.env.GITHUB_REPOSITORY; + if (envRepo) { + const [o, r] = envRepo.split("/"); + owner = o; + repo = r; + } else { + const gitRepo = getCurrentRepo(rootDir); + if (!gitRepo) { + res.json({ + mode: "git", + needsHuman: true, + reason: "autoMerge is disabled and no GitHub repository is configured; cannot open a revert PR", + }); + return; + } + owner = gitRepo.owner; + repo = gitRepo.repo; + } + + const repoKey = `${owner}/${repo}`; + if (!githubRateLimiter.canMakeRequest(repoKey)) { + res.json({ + mode: "git", + needsHuman: true, + reason: "GitHub API rate limit exceeded; try again later", + }); + return; + } + + const revertBranch = `fusion/revert-${task.id.toLowerCase()}`; + const client = new GitHubClient(); + let existingPr: Awaited>; + try { + existingPr = await client.findPrForBranch({ head: revertBranch, state: "all", owner, repo }); + } catch (error) { + // FNXC:TaskRevert 2026-07-05-00:00 (FN-7554): GitHub reachability failure + // (network down, auth rejected, 5xx, etc.) degrades to needsHuman with an + // explicit reason rather than bubbling up as a 500 — mirrors the + // no-remote-configured / rate-limited degrade paths above. + res.json({ + mode: "git", + needsHuman: true, + reason: `GitHub is unavailable; could not check for an existing revert PR (${error instanceof Error ? error.message : String(error)})`, + }); + return; + } + + const persistPrInfo = async (prInfo: PrInfo): Promise => { + const existingPrs = task.prInfos ?? (task.prInfo ? [task.prInfo] : []); + if (existingPrs.length > 0) { + await scopedStore.addPrInfo(task.id, prInfo); + } else { + await scopedStore.updatePrInfo(task.id, prInfo); + } + }; + + if (existingPr) { + // Idempotency — mirrors `/pr/create`: never re-prepare/re-push when an + // open (or all-state) PR already exists for this branch, just link it. + const prInfo: PrInfo = { ...existingPr, manual: true }; + await persistPrInfo(prInfo); + await scopedStore.logEntry(task.id, "Linked existing revert PR", `PR #${prInfo.number}: ${prInfo.url}`); + res.json({ + mode: "pr", + clean: true, + prUrl: prInfo.url, + prNumber: prInfo.number, + revertBranch, + existingPr: true, + }); + return; + } + + const prepared: PrepareRevertPrBranchResult = await prepareRevertPrBranch({ + task, + worktreePath: rootDir, + baseBranch, + revertBranch, + commitAssociationSource: { + getTaskCommitAssociationsByLineageId: (lineageId: string) => + scopedStore.getTaskCommitAssociationsByLineageId(lineageId), + }, + }); + + if (!prepared.eligible) { + if ("alreadyReverted" in prepared && prepared.alreadyReverted) { + res.json({ mode: "git", clean: true, alreadyReverted: true }); + return; + } + if ("classification" in prepared && prepared.classification === "conflicting") { + if (mode === "auto") { + res.json(await createAiUndoResult()); + return; + } + res.json({ mode: "git", clean: false, conflicts: prepared.conflicts }); + return; + } + if ("unsupported" in prepared && prepared.unsupported) { + if (mode === "auto") { + res.json(await createAiUndoResult()); + return; + } + res.json({ mode: "git", unsupported: true, reason: prepared.reason }); + return; + } + } + + if (prepared.eligible) { + // FNXC:TaskRevert 2026-07-05-00:00 (FN-7554): push/create-PR failures + // (network down, auth rejected, remote rejects push, GitHub 5xx, etc.) + // degrade to needsHuman with an explicit reason instead of a thrown 500 — + // the revert branch/commit(s) already prepared locally are left in place + // (never force-written to `baseBranch`) so a retry can reuse them. + try { + await runGitCommand(["push", "-u", "origin", prepared.revertBranch], rootDir, 60_000); + const prTitle = `revert(${task.id}): undo landed work`; + const prBody = + `This PR reverts the work landed by task ${task.id}.\n\n` + +`See \`GET /api/tasks/${task.id}/diff\` for the landed diff being reverted.\n`; + const created = await client.createPr({ + owner, + repo, + title: prTitle, + body: prBody, + head: prepared.revertBranch, + base: baseBranch, + }); + const prInfo: PrInfo = { ...created, manual: true }; + await persistPrInfo(prInfo); + await scopedStore.logEntry(task.id, "Created revert PR", `PR #${prInfo.number}: ${prInfo.url}`); + res.json({ + mode: "pr", + clean: true, + prUrl: prInfo.url, + prNumber: prInfo.number, + revertBranch: prepared.revertBranch, + }); + return; + } catch (error) { + res.json({ + mode: "git", + needsHuman: true, + reason: `GitHub is unavailable; could not push the revert branch or open the PR (${error instanceof Error ? error.message : String(error)})`, + }); + return; + } + } + } + const result = await performTaskRevert({ task, worktreePath: rootDir, diff --git a/packages/engine/src/__tests__/task-revert-pr.real-git.test.ts b/packages/engine/src/__tests__/task-revert-pr.real-git.test.ts new file mode 100644 index 0000000000..73fb72b3c1 --- /dev/null +++ b/packages/engine/src/__tests__/task-revert-pr.real-git.test.ts @@ -0,0 +1,206 @@ +import { execSync, spawnSync } from "node:child_process"; +import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { prepareRevertPrBranch } from "../task-revert.js"; +import type { Task } from "@fusion/core"; + +const hasGit = spawnSync("git", ["--version"], { stdio: "pipe" }).status === 0; +const describeIfGit = hasGit ? describe : describe.skip; + +function git(repo: string, command: string): string { + return execSync(command, { cwd: repo, encoding: "utf-8", stdio: ["pipe", "pipe", "pipe"] }).trim(); +} + +function makeTask(overrides: Partial): Task { + return { + id: "FN-A", + lineageId: "FN-A", + description: "", + column: "done", + dependencies: [], + steps: [], + currentStep: 0, + ...overrides, + } as Task; +} + +// FN-7554: real-git regression coverage for the PR-based revert branch-prep +// helper — clean → dedicated branch (base never mutated), conflicting/ +// already-reverted/unsupported pass-through, idempotent local branch reset, +// and dirty-tree refusal. +describeIfGit("prepareRevertPrBranch real-git scenarios", { timeout: 30_000 }, () => { + const dirs: string[] = []; + afterEach(() => { + for (const dir of dirs.splice(0)) rmSync(dir, { recursive: true, force: true }); + }); + + function repoFixture() { + const repo = mkdtempSync(join(tmpdir(), "kb-revert-pr-")); + dirs.push(repo); + git(repo, "git init -b main"); + git(repo, 'git config user.email "test@example.com"'); + git(repo, 'git config user.name "Test User"'); + git(repo, "git config commit.gpgsign false"); + writeFileSync(join(repo, "foo.ts"), "line1\n"); + git(repo, "git add foo.ts && git commit -m 'init'"); + return repo; + } + + it("clean → eligible: creates fusion/revert- branch with revert commit, base untouched", async () => { + const repo = repoFixture(); + writeFileSync(join(repo, "foo.ts"), "line1\nfeature-a\n"); + git(repo, "git add foo.ts && git commit -m 'feat(FN-A): add feature a'"); + const sha = git(repo, "git rev-parse HEAD"); + const mainHeadBefore = git(repo, "git rev-parse main"); + + const task = makeTask({ mergeDetails: { commitSha: sha, mergeTargetBranch: "main" } }); + const result = await prepareRevertPrBranch({ + task, + worktreePath: repo, + baseBranch: "main", + revertBranch: "fusion/revert-fn-a", + }); + + expect(result).toMatchObject({ eligible: true, revertBranch: "fusion/revert-fn-a" }); + if (result.eligible) { + expect(result.revertCommitShas.length).toBe(1); + } + + // (a) revert branch exists, tip is a revert(FN-A): commit carrying the trailer. + const branchTipSubject = git(repo, "git log -1 --format=%s fusion/revert-fn-a"); + expect(branchTipSubject).toMatch(/^revert\(FN-A\):/); + const branchTipBody = git(repo, "git log -1 --format=%B fusion/revert-fn-a"); + expect(branchTipBody).toContain("Fusion-Task-Id: FN-A"); + expect(git(repo, "git show fusion/revert-fn-a:foo.ts")).toBe("line1"); + + // (b) main HEAD is byte-identical to before the call — base never written. + expect(git(repo, "git rev-parse main")).toBe(mainHeadBefore); + + // (c) checkout restored to main and clean. + expect(git(repo, "git rev-parse --abbrev-ref HEAD")).toBe("main"); + expect(git(repo, "git status --porcelain")).toBe(""); + }); + + it("conflicting → pass-through: no revert branch left behind, base + checkout unchanged", async () => { + const repo = repoFixture(); + writeFileSync(join(repo, "foo.ts"), "line1\nfeature-a\n"); + git(repo, "git add foo.ts && git commit -m 'feat(FN-A): add feature a'"); + const shaA = git(repo, "git rev-parse HEAD"); + + // Task B later modifies the exact same region touched by task A. + writeFileSync(join(repo, "foo.ts"), "line1\nfeature-a-modified-by-b\n"); + git(repo, "git commit -am 'feat(FN-B): modify same region'"); + + const mainHeadBefore = git(repo, "git rev-parse main"); + const statusBefore = git(repo, "git status --porcelain"); + + const task = makeTask({ mergeDetails: { commitSha: shaA, mergeTargetBranch: "main" } }); + const result = await prepareRevertPrBranch({ + task, + worktreePath: repo, + baseBranch: "main", + revertBranch: "fusion/revert-fn-a", + }); + + expect(result).toMatchObject({ eligible: false, classification: "conflicting" }); + if (!result.eligible && result.classification === "conflicting") { + expect(result.conflicts.length).toBeGreaterThan(0); + expect(result.conflicts.some((c) => c.file === "foo.ts")).toBe(true); + } + + const branchList = git(repo, "git branch --list fusion/revert-fn-a"); + expect(branchList).toBe(""); + expect(git(repo, "git rev-parse main")).toBe(mainHeadBefore); + expect(git(repo, "git rev-parse --abbrev-ref HEAD")).toBe("main"); + expect(git(repo, "git status --porcelain")).toBe(statusBefore); + }); + + it("already-reverted → pass-through: no branch, base unchanged", async () => { + const repo = repoFixture(); + writeFileSync(join(repo, "foo.ts"), "line1\nfeature-a\n"); + git(repo, "git add foo.ts && git commit -m 'feat(FN-A): add feature a'"); + const sha = git(repo, "git rev-parse HEAD"); + + // Manually revert the change on main before calling prepareRevertPrBranch. + git(repo, `git revert --no-edit ${sha}`); + const mainHeadBefore = git(repo, "git rev-parse main"); + + const task = makeTask({ mergeDetails: { commitSha: sha, mergeTargetBranch: "main" } }); + const result = await prepareRevertPrBranch({ + task, + worktreePath: repo, + baseBranch: "main", + revertBranch: "fusion/revert-fn-a", + }); + + expect(result).toMatchObject({ eligible: false, classification: "already-reverted", alreadyReverted: true }); + const branchList = git(repo, "git branch --list fusion/revert-fn-a"); + expect(branchList).toBe(""); + expect(git(repo, "git rev-parse main")).toBe(mainHeadBefore); + expect(git(repo, "git rev-parse --abbrev-ref HEAD")).toBe("main"); + }); + + it("workspace unsupported: a task with workspaceWorktrees populated is refused", async () => { + const repo = repoFixture(); + const task = makeTask({ + workspaceWorktrees: { "repo-a": { worktreePath: "/tmp/whatever", branch: "main" } }, + }); + const result = await prepareRevertPrBranch({ + task, + worktreePath: repo, + baseBranch: "main", + revertBranch: "fusion/revert-fn-a", + }); + expect(result).toMatchObject({ eligible: false, unsupported: true, reason: "workspace-task-pr-revert-unsupported" }); + }); + + it("idempotent local branch reset: a stale local branch pointing elsewhere is reset off base with the fresh revert commit", async () => { + const repo = repoFixture(); + writeFileSync(join(repo, "foo.ts"), "line1\nfeature-a\n"); + git(repo, "git add foo.ts && git commit -m 'feat(FN-A): add feature a'"); + const sha = git(repo, "git rev-parse HEAD"); + + // Pre-create a stale local branch pointing at an unrelated commit. + git(repo, "git branch fusion/revert-fn-a main~1"); + const staleTip = git(repo, "git rev-parse fusion/revert-fn-a"); + expect(staleTip).not.toBe(git(repo, "git rev-parse main")); + + const task = makeTask({ mergeDetails: { commitSha: sha, mergeTargetBranch: "main" } }); + const result = await prepareRevertPrBranch({ + task, + worktreePath: repo, + baseBranch: "main", + revertBranch: "fusion/revert-fn-a", + }); + + expect(result).toMatchObject({ eligible: true }); + const branchTipSubject = git(repo, "git log -1 --format=%s fusion/revert-fn-a"); + expect(branchTipSubject).toMatch(/^revert\(FN-A\):/); + expect(git(repo, "git rev-parse --abbrev-ref HEAD")).toBe("main"); + }); + + it("dirty-tree refusal: a stray staged change is refused without any branch/base mutation", async () => { + const repo = repoFixture(); + writeFileSync(join(repo, "foo.ts"), "line1\nfeature-a\n"); + git(repo, "git add foo.ts && git commit -m 'feat(FN-A): add feature a'"); + const sha = git(repo, "git rev-parse HEAD"); + + writeFileSync(join(repo, "stray.txt"), "stray change\n"); + git(repo, "git add stray.txt"); + + const mainHeadBefore = git(repo, "git rev-parse main"); + const preStatus = git(repo, "git status --porcelain"); + + const task = makeTask({ mergeDetails: { commitSha: sha, mergeTargetBranch: "main" } }); + await expect( + prepareRevertPrBranch({ task, worktreePath: repo, baseBranch: "main", revertBranch: "fusion/revert-fn-a" }), + ).rejects.toThrow(); + + const branchList = git(repo, "git branch --list fusion/revert-fn-a"); + expect(branchList).toBe(""); + expect(git(repo, "git rev-parse main")).toBe(mainHeadBefore); + expect(git(repo, "git status --porcelain")).toBe(preStatus); + }); +}); diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index 07df194aa3..90721e2a4e 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -292,6 +292,9 @@ export { type WorkspaceRepoRevertResult, type WorkspaceTaskRevertResult, type RevertWorkspaceTaskOptions, + prepareRevertPrBranch, + type PrepareRevertPrBranchResult, + type PrepareRevertPrBranchOptions, } from "./task-revert.js"; export { resolveBranchGroupMergeRouting, diff --git a/packages/engine/src/task-revert.ts b/packages/engine/src/task-revert.ts index e1c64537f6..7897beb377 100644 --- a/packages/engine/src/task-revert.ts +++ b/packages/engine/src/task-revert.ts @@ -756,6 +756,160 @@ async function applyAndCommitRevert(opts: { } } +// --------------------------------------------------------------------------- +// FN-7554: PR-based revert for autoMerge:false projects. +// --------------------------------------------------------------------------- + +export type PrepareRevertPrBranchResult = + | { eligible: true; revertBranch: string; revertCommitShas: string[] } + | { eligible: false; classification: "conflicting"; conflicts: TaskRevertConflict[] } + | { eligible: false; classification: "already-reverted"; alreadyReverted: true } + | { eligible: false; unsupported: true; reason: string }; + +export interface PrepareRevertPrBranchOptions { + task: Pick; + /** The shared checkout, verified on `baseBranch` by the caller before this is invoked. */ + worktreePath: string; + /** Resolved mergeTargetBranch / integration branch. NEVER written to — see the doc comment below. */ + baseBranch: string; + /** e.g. `fusion/revert-`. */ + revertBranch: string; + execAsyncImpl?: ExecAsyncImpl; + commitAssociationSource?: TaskCommitAssociationSource; +} + +/** + * FNXC:TaskRevert 2026-07-05-00:00 (FN-7554 — PR-based revert for + * autoMerge:false projects): + * + * `performTaskRevert` refuses (`needsHuman`) whenever autoMerge is + * effectively off, because it commits directly onto `worktreePath`'s current + * HEAD branch (the base branch) and this project has opted that branch out + * of automated writes. This function gives that dead end an actionable path: + * it prepares a DEDICATED `revertBranch` off `baseBranch`'s HEAD, applies the + * revert commit(s) onto THAT branch only, and leaves `baseBranch` itself + * completely untouched — the caller (the API route) then pushes the branch + * and opens a real GitHub PR against `baseBranch`, so the change still lands + * through the project's normal human-review flow instead of a forced write. + * + * NEVER-WRITE-TO-BASE INVARIANT: this function only ever mutates + * `revertBranch`. `baseBranch`'s ref is never advanced, reset, or committed + * to. The shared checkout (`worktreePath`) is ALWAYS restored to the branch + * it was on when this function was called (`originalBranch`), in a `finally` + * — regardless of success, classification pass-through, or thrown failure — + * so a caller that shares this checkout across requests never observes it + * left mid-revert on `revertBranch`. + * + * REUSE, NOT REIMPLEMENTATION: commit application/message/trailer generation + * is delegated entirely to the shared `applyAndCommitRevert` helper (the same + * one `performTaskRevert`'s squash path and `revertWorkspaceTask` use) — this + * function only adds the branch-prep/checkout-restore choreography around it. + * + * WORKSPACE DEFERRAL: workspace (multi-repo) tasks are refused with + * `{ eligible: false, unsupported: true, reason: "workspace-task-pr-revert-unsupported" }` + * — a single PR against a single base branch cannot coherently represent a + * multi-repo, multi-branch revert. PR-based workspace revert is explicitly + * out of scope here (see the FN-7554 PROMPT's Step 6 follow-up task). + */ +export async function prepareRevertPrBranch(opts: PrepareRevertPrBranchOptions): Promise { + const { task, worktreePath, baseBranch, revertBranch } = opts; + const execImpl = opts.execAsyncImpl ?? defaultExecAsync; + + if (isWorkspaceTask(task)) { + return { eligible: false, unsupported: true, reason: "workspace-task-pr-revert-unsupported" }; + } + + const resolved = await resolveTaskRevertCommits(task, { + worktreePath, + execAsyncImpl: execImpl, + commitAssociationSource: opts.commitAssociationSource, + }); + if (!resolved.supported) { + return { eligible: false, unsupported: true, reason: resolved.reason }; + } + + const classification = await classifyTaskRevert({ + worktreePath, + commits: resolved.shas, + execAsyncImpl: execImpl, + }); + + if (classification.classification === "already-reverted") { + return { eligible: false, classification: "already-reverted", alreadyReverted: true }; + } + if (classification.classification === "conflicting") { + return { eligible: false, classification: "conflicting", conflicts: classification.conflicts ?? [] }; + } + + // classification === "clean" — prepare the dedicated revert branch. + let originalBranch: string; + try { + const { stdout } = await runGit(execImpl, "git rev-parse --abbrev-ref HEAD", worktreePath); + originalBranch = stdout.trim(); + } catch (error) { + throw new TaskRevertError("failed to resolve current branch before preparing revert PR branch", "head-resolve-failed", error); + } + + const { stdout: statusOut } = await runGit(execImpl, "git status --porcelain", worktreePath); + if (statusOut.trim().length > 0) { + throw new TaskRevertError( + "working tree is dirty; refusing to prepare a revert PR branch", + "dirty-working-tree", + ); + } + + let branchCreated = false; + try { + // FNXC:TaskRevert 2026-07-05-00:00: `-B` (create-or-reset) makes re-running + // this idempotent when a stale local `revertBranch` already exists from a + // prior failed/aborted attempt — it is reset off `baseBranch` HEAD rather + // than accumulating on top of whatever it previously pointed at. `baseBranch` + // itself is only ever read here (`git checkout -B ` + // does not move `baseBranch`'s ref). + await runGit( + execImpl, + `git checkout -B ${quoteShellArg(revertBranch)} ${quoteShellArg(baseBranch)}`, + worktreePath, + ); + branchCreated = true; + + const applied = await applyAndCommitRevert({ + worktreePath, + commits: resolved.shas, + taskId: task.id, + execAsyncImpl: execImpl, + }); + + if ("alreadyReverted" in applied) { + // Defensive: the branch moved between classify and apply. Nothing to + // commit on the fresh revertBranch — treat as already-reverted. + return { eligible: false, classification: "already-reverted", alreadyReverted: true }; + } + if ("conflicts" in applied) { + // Late conflict — applyAndCommitRevert already rolled worktreePath back + // to the pre-apply HEAD (the tip of revertBranch, i.e. baseBranch's HEAD). + return { eligible: false, classification: "conflicting", conflicts: applied.conflicts }; + } + + return { eligible: true, revertBranch, revertCommitShas: [applied.revertCommitSha] }; + } catch (error) { + // On any thrown failure after branch creation, never leave a dangling + // partial revert branch behind — best-effort restore + delete. + if (branchCreated) { + await runGit(execImpl, `git checkout ${quoteShellArg(originalBranch)}`, worktreePath).catch(() => undefined); + await runGit(execImpl, `git branch -D ${quoteShellArg(revertBranch)}`, worktreePath).catch(() => undefined); + } + throw error instanceof TaskRevertError ? error : new TaskRevertError("failed to prepare revert PR branch", "revert-pr-branch-prepare-failed", error); + } finally { + // FNXC:TaskRevert 2026-07-05-00:00: ALWAYS restore the shared checkout to + // the branch it was on when this function was called, so the checkout is + // left exactly where it started — on `baseBranch` (unmutated) in the + // documented caller contract — regardless of success/pass-through/failure + // above. The revert commit(s) live ONLY on `revertBranch`. + await runGit(execImpl, `git checkout ${quoteShellArg(originalBranch)}`, worktreePath).catch(() => undefined); + } +} + export interface WorkspaceRepoRevertCommits { commits: string[]; source: TaskRevertCommitSource;