fix(pr-merge): resolve repo from the project checkout, not process.cwd(), in PR-mode auto-merge (#2281)
## Summary In a centrally-installed, multi-project Fusion server (one process serving several repos, `process.cwd()` = the install dir, not any repo), every task under `mergeStrategy: "pull-request"` fails at the auto-merge stage with: ``` Could not determine repository. Specify owner/repo in params or run from a git repository with a GitHub remote. ``` PR creation from the dashboard and status polling work; only the engine's automatic PR path fails. This is the **non-workspace sibling of #1924** (FN-7610 routed workspace-mode tasks to direct merge but does not cover regular multi-project tasks) and the completion of #1797/FN-7133 (which fixed only the `getPrMergeStatus` arguments). ## Root cause `GitHubClient.resolveRepo()` (`packages/dashboard/src/github.ts`) falls back to a cwd-less `getCurrentRepo()` — i.e. `git remote get-url origin` in `process.cwd()` — whenever a PR method is called without explicit `owner`/`repo`. The engine merge path already resolves the correct repo from the per-project cwd (`prRepo = getCurrentRepo(cwd)`, FN-7133) but only threaded it into `getPrMergeStatus`. Every other GitHub call omitted it: - `processPullRequestMergeTask`: `findPrForBranch` / `createPr` / `mergePr` on both the per-task and shared-branch-group paths - `createGroupPrCallback` (group-PR promotion): `findPrForBranch` / `createPr` - `createPrNodeGithubOps` (`pr-create`/`pr-merge` workflow nodes): cwd-less `getCurrentRepo()` persisted `entity.repo` as `""` (poisoning the downstream `splitRepoSlug` consumers), and the git push/`createPr`/`mergePr` ran against `process.cwd()` - the engine's review-response run (`buildRespondCallback`): `respondOps.getCwd` collapses to `process.cwd()` because no CLI composition site wires `getTaskWorktree`, so its git ops and response agent ran outside the project repo In a central install the fallback throws; worse, if `process.cwd()` happens to be inside some *other* git repo, it silently targets the **wrong repository**. ## What changed - `fix(pr-merge): thread repo identity into PR auto-merge GitHub calls` — widens the CLI-local `GitHubOperations` interface (optional `owner`/`repo`, already accepted by `GitHubClient`'s `FindPrParams`/`CreatePrParams`/`MergePrParams`) and passes `prRepo` at all six call sites in `processPullRequestMergeTask`. - `fix(pr-merge): resolve group-PR repo from project cwd in createGroupPrCallback` — resolves via `getCurrentRepo(cwd)` from the callback input (same T4 pattern as `syncGroupPrCallback`) with a loud failure instead of a silent wrong-repo fallback. - `fix(pr-merge): resolve PR-node repo from task worktree instead of process cwd` — `resolvePrSource` resolves from `task.worktree`, git ops run in `getTaskWorktree(...) ?? task.worktree ?? process.cwd()`, and `createPr`/`mergePr` pass `owner`/`repo` parsed from `entity.repo`. - `fix(pr-merge): resolve review-response run cwd from the task worktree` — the engine owns the store, so `buildRespondCallback` prefers the task's recorded `worktree` for the response run's git ops + agent, keeping `ops.getCwd` as the single-project fallback (defensive against structural `PrNodeStore`s without `getTask`). - Changeset (`@runfusion/fusion` patch, structured body) included. Deliberately **not** done: a constructor-scoped default repo on `GitHubClient` — one client instance is shared across all projects in a central install (`serve.ts`/`daemon.ts`/`dashboard.ts`), so per-call `owner`/`repo` is the only correct scope. ## Testing - New regression tests simulate the central-install topology (`getCurrentRepo` mocked as `(cwd?) => cwd ? repo : null`, exactly the failing environment) and drive the merge flow end-to-end on the per-task path, the shared-branch-group path, `createGroupPrCallback`, and all three `createPrNodeGithubOps` ops, asserting every GitHub call carries explicit `owner`/`repo` (45 tests in `packages/cli/src/commands/__tests__/task-lifecycle.test.ts`, all green). - `packages/engine/src/__tests__/pr-respond-cwd-resolution.test.ts` covers the respond-run cwd: worktree preferred, `ops.getCwd` fallback when the task has no worktree, when the lookup fails, and when a structural store has no `getTask`. - Existing exact-argument assertions were extended to the new call contract (no assertions weakened or removed). - `pnpm lint`, `pnpm typecheck`, and `pnpm build` green locally; `pnpm test:gate`'s engine-core suite green (294/294) — its PostgreSQL-backend lane needs local PG credentials this environment lacks, so that lane defers to CI. `pnpm verify:fast` (scoped typecheck/build + CLI build + boot smoke) also passes. ## Repro 1. Install the CLI centrally; run the server from a dir that is not a git repo, serving ≥1 project with a GitHub `origin` and `mergeStrategy: "pull-request"`. 2. Run a task to completion and let it reach the merge stage. 3. Before this fix: the auto-merger throws `Could not determine repository …` (tasks with a persisted PR poll fine but never merge). Merging the same task from the Pull Requests tab succeeds, because the dashboard route resolves the repo explicitly (`parseBadgeUrl(...) ?? getCurrentRepo(rootDir)`). Full analysis: https://github.com/Tchori-Labs/Fusion/issues/4 --- Developed with Claude (co-authored on all commits). https://claude.ai/code/session_01ChEa8SHFYNAzjCdFbwFMfh <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Resolved pull request auto-merge failures in centrally installed, multi-project deployments. - Ensured explicit repository context (`owner/repo`) is used for pull request lookup, creation, and merging throughout the merge workflow. - Improved pull request response handling to prefer the task worktree for working-directory resolution, with safe error behavior when task details are unavailable. - **Tests** - Expanded coverage for multi-repository merge workflows and worktree-based repository/cwd resolution in PR response handling. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude <noreply@anthropic.com>
This commit is contained in:
7
.changeset/gh-4-pr-merge-repo-resolution.md
Normal file
7
.changeset/gh-4-pr-merge-repo-resolution.md
Normal file
@@ -0,0 +1,7 @@
|
||||
---
|
||||
"@runfusion/fusion": patch
|
||||
---
|
||||
|
||||
summary: Fix PR-mode auto-merge failing with "Could not determine repository" in centrally-installed multi-project deployments.
|
||||
category: fix
|
||||
dev: processPullRequestMergeTask, createGroupPrCallback, and createPrNodeGithubOps now pass explicit owner/repo (resolved from the per-project cwd / task worktree) into findPrForBranch/createPr/mergePr instead of relying on GitHubClient.resolveRepo's process.cwd() fallback; the engine's buildRespondCallback resolves the review-response run cwd from the task's recorded worktree. Fixes Tchori-Labs/Fusion#4; non-workspace sibling of upstream #1924/FN-7610.
|
||||
@@ -4,10 +4,12 @@ import { EventEmitter } from "node:events";
|
||||
// Mock child_process so we can intercept the `git push -u origin <branch>`
|
||||
// call that processPullRequestMergeTask issues before createPr.
|
||||
const execMock = vi.hoisted(() => vi.fn());
|
||||
// Records raw (file, args[]) tuples for execFile so tests can assert a no-shell
|
||||
// invocation (Fix #11) — i.e. the branch is a discrete argv entry, not shell-
|
||||
// interpolated.
|
||||
const execFileCalls = vi.hoisted(() => [] as Array<{ file: string; args: string[] }>);
|
||||
// Records raw (file, args[], cwd) tuples for execFile so tests can assert a
|
||||
// no-shell invocation (Fix #11) — i.e. the branch is a discrete argv entry, not
|
||||
// shell-interpolated — and that git ops target the task worktree cwd (gh-4).
|
||||
const execFileCalls = vi.hoisted(
|
||||
() => [] as Array<{ file: string; args: string[]; cwd: string | undefined }>,
|
||||
);
|
||||
vi.mock("node:child_process", () => ({
|
||||
exec: (cmd: string, opts: unknown, cb: (err: Error | null, stdout: string, stderr: string) => void) => {
|
||||
try {
|
||||
@@ -19,7 +21,7 @@ vi.mock("node:child_process", () => ({
|
||||
},
|
||||
execFile: (file: string, args: string[] | undefined, opts: unknown, cb: (err: Error | null, stdout: string, stderr: string) => void) => {
|
||||
try {
|
||||
execFileCalls.push({ file, args: args ?? [] });
|
||||
execFileCalls.push({ file, args: args ?? [], cwd: (opts as { cwd?: string } | undefined)?.cwd });
|
||||
const result = execMock(`${file} ${(args ?? []).join(" ")}`.trim(), opts);
|
||||
cb(null, typeof result === "string" ? result : "", "");
|
||||
} catch (err) {
|
||||
@@ -41,6 +43,7 @@ import { activeSessionRegistry } from "@fusion/engine";
|
||||
import {
|
||||
cleanupMergedTaskArtifacts,
|
||||
createGroupPrCallback,
|
||||
createPrNodeGithubOps,
|
||||
processPullRequestMergeTask,
|
||||
getTaskBranchName,
|
||||
syncGroupPrCallback,
|
||||
@@ -157,6 +160,141 @@ describe("processPullRequestMergeTask", () => {
|
||||
vi.mocked(getCurrentRepo).mockReturnValue({ owner: "owner", repo: "repo" });
|
||||
});
|
||||
|
||||
describe("central-install repo threading (gh-4)", () => {
|
||||
// FNXC:PrMergeAutoMerge 2026-07-17-19:18 (gh-4):
|
||||
// Simulate a centrally-installed multi-project daemon: process.cwd() is NOT
|
||||
// a git repo, so cwd-less getCurrentRepo() returns null; only the
|
||||
// per-project cwd resolves. Any PR call that relies on the client's
|
||||
// process-cwd fallback would blow up with "Could not determine repository".
|
||||
beforeEach(() => {
|
||||
vi.mocked(getCurrentRepo).mockImplementation(((cwd?: string) =>
|
||||
cwd ? { owner: "central-owner", repo: "central-repo" } : null) as never);
|
||||
});
|
||||
|
||||
it("threads explicit owner/repo into findPrForBranch, createPr, and mergePr on the per-task path", async () => {
|
||||
const task: MockTask = { id: "FN-9401", title: "t", description: "d", column: "in-review" };
|
||||
const branch = getTaskBranchName(task.id);
|
||||
const store = makeStatefulStore(task);
|
||||
execMock.mockReturnValue("");
|
||||
|
||||
const github = {
|
||||
findPrForBranch: vi.fn(async () => null),
|
||||
createPr: vi.fn(async () => ({
|
||||
number: 77,
|
||||
url: "https://github.com/central-owner/central-repo/pull/77",
|
||||
status: "open" as const,
|
||||
headBranch: branch,
|
||||
baseBranch: "main",
|
||||
})),
|
||||
getPrMergeStatus: vi.fn(async () => ({
|
||||
prInfo: {
|
||||
number: 77,
|
||||
url: "https://github.com/central-owner/central-repo/pull/77",
|
||||
status: "open" as const,
|
||||
},
|
||||
reviewDecision: null,
|
||||
checks: [],
|
||||
mergeReady: true,
|
||||
blockingReasons: [],
|
||||
})),
|
||||
mergePr: vi.fn(async () => ({
|
||||
number: 77,
|
||||
url: "https://github.com/central-owner/central-repo/pull/77",
|
||||
status: "merged" as const,
|
||||
})),
|
||||
};
|
||||
|
||||
const result = await processPullRequestMergeTask(
|
||||
store as never,
|
||||
"/projects/repo-a",
|
||||
task.id,
|
||||
github as never,
|
||||
() => undefined,
|
||||
);
|
||||
|
||||
expect(result).toBe("merged");
|
||||
expect(github.findPrForBranch).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ owner: "central-owner", repo: "central-repo", head: branch }),
|
||||
);
|
||||
expect(github.createPr).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ owner: "central-owner", repo: "central-repo", head: branch }),
|
||||
);
|
||||
expect(github.mergePr).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ owner: "central-owner", repo: "central-repo", number: 77, method: "squash" }),
|
||||
);
|
||||
});
|
||||
|
||||
it("threads explicit owner/repo into findPrForBranch, createPr, and mergePr on the shared-branch-group path", async () => {
|
||||
const task: MockTask = {
|
||||
id: "FN-9402",
|
||||
title: "t",
|
||||
description: "d",
|
||||
column: "in-review",
|
||||
branchContext: { groupId: "planning:g4", source: "planning", assignmentMode: "shared" },
|
||||
};
|
||||
const store = makeStore(task, { baseBranch: "main" });
|
||||
(store.getBranchGroup as ReturnType<typeof vi.fn>).mockReturnValue({
|
||||
id: "BG-4",
|
||||
sourceType: "planning",
|
||||
sourceId: "planning:g4",
|
||||
branchName: "fusion/groups/planning-g4",
|
||||
autoMerge: true,
|
||||
prState: "none",
|
||||
status: "open",
|
||||
createdAt: Date.now(),
|
||||
updatedAt: Date.now(),
|
||||
});
|
||||
(store.listTasksByBranchGroup as ReturnType<typeof vi.fn>).mockResolvedValue([task]);
|
||||
execMock.mockImplementation((cmd: string) => (cmd.includes("rev-list --count") ? "1\n" : ""));
|
||||
|
||||
const github = {
|
||||
findPrForBranch: vi.fn(async () => null),
|
||||
createPr: vi.fn(async () => ({
|
||||
number: 88,
|
||||
url: "https://github.com/central-owner/central-repo/pull/88",
|
||||
status: "open" as const,
|
||||
headBranch: "fusion/groups/planning-g4",
|
||||
baseBranch: "main",
|
||||
})),
|
||||
getPrMergeStatus: vi.fn(async () => ({
|
||||
prInfo: {
|
||||
number: 88,
|
||||
url: "https://github.com/central-owner/central-repo/pull/88",
|
||||
status: "open" as const,
|
||||
},
|
||||
reviewDecision: null,
|
||||
checks: [],
|
||||
mergeReady: true,
|
||||
blockingReasons: [],
|
||||
})),
|
||||
mergePr: vi.fn(async () => ({
|
||||
number: 88,
|
||||
url: "https://github.com/central-owner/central-repo/pull/88",
|
||||
status: "merged" as const,
|
||||
})),
|
||||
};
|
||||
|
||||
const result = await processPullRequestMergeTask(
|
||||
store as never,
|
||||
"/projects/repo-a",
|
||||
task.id,
|
||||
github as never,
|
||||
() => undefined,
|
||||
);
|
||||
|
||||
expect(result).toBe("merged");
|
||||
expect(github.findPrForBranch).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ owner: "central-owner", repo: "central-repo", head: "fusion/groups/planning-g4" }),
|
||||
);
|
||||
expect(github.createPr).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ owner: "central-owner", repo: "central-repo", head: "fusion/groups/planning-g4" }),
|
||||
);
|
||||
expect(github.mergePr).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ owner: "central-owner", repo: "central-repo", number: 88, method: "squash" }),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
it("pushes the per-task branch to origin before creating a new PR", async () => {
|
||||
const task: MockTask = {
|
||||
id: "FN-9001",
|
||||
@@ -1103,7 +1241,7 @@ describe("processPullRequestMergeTask", () => {
|
||||
);
|
||||
|
||||
expect(result).toBe("merged");
|
||||
expect(github.mergePr).toHaveBeenCalledWith({ number: 124, method: "squash" });
|
||||
expect(github.mergePr).toHaveBeenCalledWith({ owner: "owner", repo: "repo", number: 124, method: "squash" });
|
||||
expect(github.getPrMergeStatus).toHaveBeenCalledTimes(2);
|
||||
expect(github.getPrMergeStatus).toHaveBeenNthCalledWith(1, "owner", "repo", 124);
|
||||
expect(github.getPrMergeStatus).toHaveBeenNthCalledWith(2, "owner", "repo", 124);
|
||||
@@ -1174,7 +1312,7 @@ describe("processPullRequestMergeTask", () => {
|
||||
),
|
||||
).rejects.toThrow(mergeError.message);
|
||||
|
||||
expect(github.mergePr).toHaveBeenCalledWith({ number: 125, method: "squash" });
|
||||
expect(github.mergePr).toHaveBeenCalledWith({ owner: "owner", repo: "repo", number: 125, method: "squash" });
|
||||
expect(github.getPrMergeStatus).toHaveBeenCalledTimes(2);
|
||||
expect(store.updatePrInfo).not.toHaveBeenCalledWith("FN-9105", expect.objectContaining({ status: "merged" }));
|
||||
expect(store.moveTask).not.toHaveBeenCalled();
|
||||
@@ -1231,7 +1369,7 @@ describe("processPullRequestMergeTask", () => {
|
||||
),
|
||||
).rejects.toThrow(mergeError.message);
|
||||
|
||||
expect(github.mergePr).toHaveBeenCalledWith({ number: 126, method: "squash" });
|
||||
expect(github.mergePr).toHaveBeenCalledWith({ owner: "owner", repo: "repo", number: 126, method: "squash" });
|
||||
expect(github.getPrMergeStatus).toHaveBeenCalledTimes(2);
|
||||
expect(store.moveTask).not.toHaveBeenCalled();
|
||||
});
|
||||
@@ -1398,7 +1536,7 @@ describe("processPullRequestMergeTask", () => {
|
||||
);
|
||||
|
||||
expect(result).toBe("merged");
|
||||
expect(github.mergePr).toHaveBeenCalledWith({ number: 100, method: "squash" });
|
||||
expect(github.mergePr).toHaveBeenCalledWith({ owner: "owner", repo: "repo", number: 100, method: "squash" });
|
||||
});
|
||||
|
||||
it("preserves existing behavior when requirePrApproval is false", async () => {
|
||||
@@ -1654,7 +1792,12 @@ describe("createGroupPrCallback", () => {
|
||||
baseBranch: "main",
|
||||
});
|
||||
|
||||
expect(github.findPrForBranch).toHaveBeenCalledWith({ head: group.branchName, state: "open" });
|
||||
expect(github.findPrForBranch).toHaveBeenCalledWith({
|
||||
owner: "owner",
|
||||
repo: "repo",
|
||||
head: group.branchName,
|
||||
state: "open",
|
||||
});
|
||||
});
|
||||
|
||||
it("does not reuse a closed PR from a prior group — creates a fresh one", async () => {
|
||||
@@ -1679,7 +1822,12 @@ describe("createGroupPrCallback", () => {
|
||||
baseBranch: "main",
|
||||
});
|
||||
|
||||
expect(github.findPrForBranch).toHaveBeenCalledWith({ head: group.branchName, state: "open" });
|
||||
expect(github.findPrForBranch).toHaveBeenCalledWith({
|
||||
owner: "owner",
|
||||
repo: "repo",
|
||||
head: group.branchName,
|
||||
state: "open",
|
||||
});
|
||||
expect(github.createPr).toHaveBeenCalledTimes(1);
|
||||
expect(result).toEqual({
|
||||
prNumber: 123,
|
||||
@@ -1687,5 +1835,121 @@ describe("createGroupPrCallback", () => {
|
||||
prState: "open",
|
||||
});
|
||||
});
|
||||
|
||||
it("resolves the repo from the callback cwd and threads owner/repo into findPrForBranch/createPr (gh-4)", async () => {
|
||||
vi.mocked(getCurrentRepo).mockImplementation(((cwd?: string) =>
|
||||
cwd ? { owner: "central-owner", repo: "central-repo" } : null) as never);
|
||||
execMock.mockReturnValue("");
|
||||
|
||||
const github = {
|
||||
findPrForBranch: vi.fn(async () => null),
|
||||
createPr: vi.fn(async () => ({
|
||||
number: 5,
|
||||
url: "https://github.com/central-owner/central-repo/pull/5",
|
||||
status: "open" as const,
|
||||
})),
|
||||
};
|
||||
|
||||
const callback = createGroupPrCallback(github as never);
|
||||
const result = await callback({
|
||||
cwd: "/projects/repo-a",
|
||||
group: { id: "BG-9", branchName: "fusion/groups/g9" } as never,
|
||||
members: [{ id: "FN-9501", title: "m1" }] as never,
|
||||
headBranch: "fusion/groups/g9",
|
||||
baseBranch: "main",
|
||||
});
|
||||
|
||||
expect(vi.mocked(getCurrentRepo)).toHaveBeenCalledWith("/projects/repo-a");
|
||||
expect(github.findPrForBranch).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ owner: "central-owner", repo: "central-repo", head: "fusion/groups/g9" }),
|
||||
);
|
||||
expect(github.createPr).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ owner: "central-owner", repo: "central-repo", head: "fusion/groups/g9" }),
|
||||
);
|
||||
expect(result.prNumber).toBe(5);
|
||||
});
|
||||
});
|
||||
|
||||
describe("createPrNodeGithubOps repo resolution (gh-4)", () => {
|
||||
beforeEach(() => {
|
||||
execMock.mockReset();
|
||||
execFileCalls.length = 0;
|
||||
vi.mocked(getCurrentRepo).mockImplementation(((cwd?: string) =>
|
||||
cwd ? { owner: "central-owner", repo: "central-repo" } : null) as never);
|
||||
});
|
||||
|
||||
const githubStub = () => ({
|
||||
createPr: vi.fn(async () => ({ number: 9, url: "https://github.com/central-owner/central-repo/pull/9", status: "open" as const })),
|
||||
mergePr: vi.fn(async () => ({ number: 9, url: "https://github.com/central-owner/central-repo/pull/9", status: "merged" as const })),
|
||||
getPrStatus: vi.fn(),
|
||||
replyToReviewThread: vi.fn(),
|
||||
resolveReviewThread: vi.fn(),
|
||||
getViewerLogin: vi.fn(),
|
||||
getPrReviewThreadsDetailed: vi.fn(),
|
||||
});
|
||||
|
||||
it("resolvePrSource resolves the repo slug from the task worktree, not process.cwd()", async () => {
|
||||
const ops = createPrNodeGithubOps(githubStub() as never);
|
||||
const source = await ops.resolvePrSource(
|
||||
{ id: "FN-9601", worktree: "/projects/repo-a/.worktrees/fn-9601" } as never,
|
||||
{} as never,
|
||||
);
|
||||
expect(source.repo).toBe("central-owner/central-repo");
|
||||
expect(vi.mocked(getCurrentRepo)).toHaveBeenCalledWith("/projects/repo-a/.worktrees/fn-9601");
|
||||
});
|
||||
|
||||
it("resolvePrSource treats the configured getTaskWorktree resolver as authoritative over task.worktree", async () => {
|
||||
const ops = createPrNodeGithubOps(githubStub() as never, {
|
||||
getTaskWorktree: (taskId) => `/resolved/${taskId}`,
|
||||
});
|
||||
const source = await ops.resolvePrSource(
|
||||
{ id: "FN-9601", worktree: "/stale/recorded/worktree" } as never,
|
||||
{} as never,
|
||||
);
|
||||
expect(source.repo).toBe("central-owner/central-repo");
|
||||
expect(vi.mocked(getCurrentRepo)).toHaveBeenCalledWith("/resolved/FN-9601");
|
||||
});
|
||||
|
||||
it("resolvePrSource throws instead of persisting entity.repo as '' when no repo resolves (central install, no worktree)", async () => {
|
||||
// Worktree-less task in a central install: every cwd candidate (including
|
||||
// process.cwd(), the install dir) fails to resolve a repo. Persisting ""
|
||||
// would poison downstream splitRepoSlug consumers into the client's
|
||||
// process-cwd fallback — the exact gh-4 failure mode.
|
||||
vi.mocked(getCurrentRepo).mockReturnValue(null as never);
|
||||
const ops = createPrNodeGithubOps(githubStub() as never);
|
||||
expect(() => ops.resolvePrSource({ id: "FN-9601" } as never, {} as never)).toThrow(
|
||||
/pr-create: could not determine repository for task FN-9601/,
|
||||
);
|
||||
});
|
||||
|
||||
it("createPr pushes from the task worktree and passes owner/repo parsed from entity.repo", async () => {
|
||||
execMock.mockReturnValue("");
|
||||
const github = githubStub();
|
||||
const ops = createPrNodeGithubOps(github as never);
|
||||
const result = await ops.createPr({
|
||||
task: { id: "FN-9601", title: "t", description: "d", worktree: "/projects/repo-a/.worktrees/fn-9601" },
|
||||
entity: { id: "e1", sourceId: "FN-9601", repo: "central-owner/central-repo", headBranch: "fusion/fn-9601", baseBranch: "main" },
|
||||
} as never);
|
||||
expect(github.createPr).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ owner: "central-owner", repo: "central-repo", head: "fusion/fn-9601" }),
|
||||
);
|
||||
const pushCall = execFileCalls.find((c) => c.file === "git" && c.args[0] === "push");
|
||||
expect(pushCall).toBeDefined();
|
||||
// The push must run in the task worktree, not process.cwd() (gh-4).
|
||||
expect(pushCall?.cwd).toBe("/projects/repo-a/.worktrees/fn-9601");
|
||||
expect(result.prNumber).toBe(9);
|
||||
});
|
||||
|
||||
it("mergePr passes owner/repo parsed from entity.repo", async () => {
|
||||
const github = githubStub();
|
||||
const ops = createPrNodeGithubOps(github as never);
|
||||
const result = await ops.mergePr({
|
||||
entity: { id: "e1", sourceId: "FN-9601", repo: "central-owner/central-repo", prNumber: 9, headOid: "abc123" },
|
||||
} as never);
|
||||
expect(result).toEqual({ status: "merged-requested" });
|
||||
expect(github.mergePr).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ owner: "central-owner", repo: "central-repo", number: 9, method: "squash", expectedHeadOid: "abc123" }),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -49,8 +49,8 @@ import type {
|
||||
* Defined locally to avoid importing from @fusion/dashboard.
|
||||
*/
|
||||
interface GitHubOperations {
|
||||
findPrForBranch(params: { head: string; state?: "open" | "closed" | "all" }): Promise<PrInfo | null>;
|
||||
createPr(params: { title: string; body: string; head: string; base?: string }): Promise<PrInfo>;
|
||||
findPrForBranch(params: { owner?: string; repo?: string; head: string; state?: "open" | "closed" | "all" }): Promise<PrInfo | null>;
|
||||
createPr(params: { owner?: string; repo?: string; title: string; body: string; head: string; base?: string }): Promise<PrInfo>;
|
||||
getPrMergeStatus(owner?: string, repo?: string, number?: number): Promise<{
|
||||
prInfo: PrInfo;
|
||||
reviewDecision: string | null;
|
||||
@@ -58,7 +58,7 @@ interface GitHubOperations {
|
||||
mergeReady: boolean;
|
||||
blockingReasons: string[];
|
||||
}>;
|
||||
mergePr(params: { number: number; method?: "merge" | "squash" | "rebase"; expectedHeadOid?: string }): Promise<PrInfo>;
|
||||
mergePr(params: { owner?: string; repo?: string; number: number; method?: "merge" | "squash" | "rebase"; expectedHeadOid?: string }): Promise<PrInfo>;
|
||||
getPrStatus(owner: string, repo: string, number: number): Promise<PrInfo>;
|
||||
/** Reply to a specific review thread (U2). */
|
||||
replyToReviewThread(threadId: string, body: string): Promise<void>;
|
||||
@@ -256,12 +256,24 @@ function toBranchGroupPrState(prInfo: PrInfo | null): BranchGroupPrState {
|
||||
* Idempotency: reuses an existing PR for the group head branch on GitHub. The
|
||||
* coordinator additionally skips this call when a `prNumber` is already persisted,
|
||||
* so a re-promotion never opens a second PR.
|
||||
*
|
||||
* Repo identity is resolved from the per-project cwd in the callback input, mirroring
|
||||
* syncGroupPrCallback.
|
||||
*/
|
||||
export function createGroupPrCallback(
|
||||
github: Pick<GitHubOperations, "findPrForBranch" | "createPr">,
|
||||
): CreateGroupPrFn {
|
||||
return async ({ cwd, group, members, headBranch, baseBranch }) => {
|
||||
const existing = await github.findPrForBranch({ head: headBranch, state: "open" });
|
||||
// FNXC:PrMergeAutoMerge 2026-07-17-16:50 (gh-4):
|
||||
// Resolve the repo from the PROJECT cwd, not the process cwd (same T4
|
||||
// requirement as syncGroupPrCallback below) — in a centrally-installed
|
||||
// multi-project daemon process.cwd() is not the project repo, so the
|
||||
// client's fallback would throw or target the wrong repository.
|
||||
const repo = getCurrentRepo(cwd);
|
||||
if (!repo) {
|
||||
throw new Error("createGroupPr: could not determine repository");
|
||||
}
|
||||
const existing = await github.findPrForBranch({ owner: repo.owner, repo: repo.repo, head: headBranch, state: "open" });
|
||||
if (existing) {
|
||||
return { prNumber: existing.number, prUrl: existing.url, prState: toBranchGroupPrState(existing) };
|
||||
}
|
||||
@@ -273,6 +285,8 @@ export function createGroupPrCallback(
|
||||
branchName: getTaskBranchName(member.id),
|
||||
}));
|
||||
const created = await github.createPr({
|
||||
owner: repo.owner,
|
||||
repo: repo.repo,
|
||||
title: buildGroupPullRequestTitle(group, members),
|
||||
body: buildGroupPullRequestBody(group, membersWithBranch),
|
||||
head: headBranch,
|
||||
@@ -418,20 +432,41 @@ export function createPrNodeGithubOps(
|
||||
|
||||
return {
|
||||
resolvePrSource: (task) => {
|
||||
const repo = getCurrentRepo();
|
||||
const repoSlug = repo ? `${repo.owner}/${repo.repo}` : "";
|
||||
// FNXC:PrMergeAutoMerge 2026-07-17-19:18 (gh-4):
|
||||
// Resolve the repo from the task's worktree (a checkout of the project
|
||||
// repo), not process.cwd() — in a centrally-installed multi-project
|
||||
// server process.cwd() is not a repo, which persisted entity.repo as ""
|
||||
// and poisoned every downstream splitRepoSlug consumer. The configured
|
||||
// getTaskWorktree resolver is authoritative (same precedence as createPr),
|
||||
// and an unresolvable repo now throws instead of persisting "" — the
|
||||
// pr-create node maps the throw to a routable `source-error` failure,
|
||||
// and "" could only ever succeed by silently targeting whatever repo
|
||||
// process.cwd() happens to sit in.
|
||||
const cwd = options.getTaskWorktree?.(task.id) ?? task.worktree ?? process.cwd();
|
||||
const repo = getCurrentRepo(cwd);
|
||||
if (!repo) {
|
||||
throw new Error(
|
||||
`pr-create: could not determine repository for task ${task.id} (no GitHub remote resolved from ${cwd})`,
|
||||
);
|
||||
}
|
||||
return {
|
||||
sourceType: "task",
|
||||
sourceId: task.id,
|
||||
repo: repoSlug,
|
||||
repo: `${repo.owner}/${repo.repo}`,
|
||||
headBranch: getTaskBranchName(task.id),
|
||||
};
|
||||
},
|
||||
createPr: async ({ task, entity }) => {
|
||||
const cwd = process.cwd();
|
||||
// FNXC:PrMergeAutoMerge 2026-07-17-19:18 (gh-4):
|
||||
// Git ops run in the task worktree when known; process.cwd() only as the
|
||||
// single-project fallback.
|
||||
const cwd = options.getTaskWorktree?.(entity.sourceId) ?? task.worktree ?? process.cwd();
|
||||
const headBranch = entity.headBranch || getTaskBranchName(task.id);
|
||||
await pushTaskBranchToOrigin(cwd, headBranch);
|
||||
const { owner, name } = splitRepoSlug(entity.repo);
|
||||
const created = await github.createPr({
|
||||
owner,
|
||||
repo: name,
|
||||
title: task.title ?? `Task ${task.id}`,
|
||||
body: task.description ?? "",
|
||||
head: headBranch,
|
||||
@@ -444,8 +479,11 @@ export function createPrNodeGithubOps(
|
||||
if (entity.prNumber == null) {
|
||||
throw new Error(`pr-merge: entity ${entity.id} has no persisted prNumber`);
|
||||
}
|
||||
const { owner, name } = splitRepoSlug(entity.repo);
|
||||
try {
|
||||
await github.mergePr({
|
||||
owner,
|
||||
repo: name,
|
||||
number: entity.prNumber,
|
||||
method: "squash",
|
||||
expectedHeadOid: entity.headOid,
|
||||
@@ -735,6 +773,9 @@ export async function processPullRequestMergeTask(
|
||||
/*
|
||||
* FNXC:PrMergeAutoMerge 2026-06-27-13:14:
|
||||
* FN-7133 requires PR-mode merge status to resolve owner/repo from the project cwd because multi-project daemons cannot rely on process cwd. Never pass branch names into gh pr view --repo; getPrMergeStatus forwards its first two args as the repository slug.
|
||||
*
|
||||
* FNXC:PrMergeAutoMerge 2026-07-17-16:46 (gh-4):
|
||||
* The same requirement extends to EVERY GitHub call in this path: findPrForBranch/createPr/mergePr must carry prRepo explicitly, because GitHubClient.resolveRepo's fallback resolves from process.cwd(), which in a centrally-installed multi-project server is the install dir (throws "Could not determine repository") or, worse, some unrelated repo (silently targets the wrong repository).
|
||||
*/
|
||||
const prRepo = getCurrentRepo(cwd);
|
||||
if (!prRepo) {
|
||||
@@ -794,11 +835,13 @@ export async function processPullRequestMergeTask(
|
||||
// terminal PR for this head branch must NOT be reattached (that reintroduces
|
||||
// the terminal-PR reuse bug createGroupPrCallback fixed); treat it as
|
||||
// not-found and fall through to push + createPr for a fresh open PR.
|
||||
groupPrInfo = await github.findPrForBranch({ head: branchGroup.branchName, state: "open" });
|
||||
groupPrInfo = await github.findPrForBranch({ owner: prRepo.owner, repo: prRepo.repo, head: branchGroup.branchName, state: "open" });
|
||||
if (!groupPrInfo) {
|
||||
await pushTaskBranchToOrigin(cwd, branchGroup.branchName);
|
||||
try {
|
||||
groupPrInfo = await github.createPr({
|
||||
owner: prRepo.owner,
|
||||
repo: prRepo.repo,
|
||||
title: buildGroupPullRequestTitle(branchGroup, members),
|
||||
body: buildGroupPullRequestBody(branchGroup, membersWithCommits),
|
||||
head: branchGroup.branchName,
|
||||
@@ -867,7 +910,7 @@ export async function processPullRequestMergeTask(
|
||||
}
|
||||
|
||||
await store.updateTask(task.id, { status: "merging-pr" });
|
||||
const mergedPr = await github.mergePr({ number: refreshedPrInfo.number, method: "squash" });
|
||||
const mergedPr = await github.mergePr({ owner: prRepo.owner, repo: prRepo.repo, number: refreshedPrInfo.number, method: "squash" });
|
||||
await store.updateBranchGroup(branchGroup.id, {
|
||||
prNumber: mergedPr.number,
|
||||
prUrl: mergedPr.url,
|
||||
@@ -894,7 +937,7 @@ export async function processPullRequestMergeTask(
|
||||
if (!prInfo) {
|
||||
await store.updateTask(task.id, { status: "creating-pr" });
|
||||
|
||||
const existingPr = await github.findPrForBranch({ head: branch, state: "all" });
|
||||
const existingPr = await github.findPrForBranch({ owner: prRepo.owner, repo: prRepo.repo, head: branch, state: "all" });
|
||||
if (!existingPr) {
|
||||
// gh pr create / GitHub REST require the head branch to exist on
|
||||
// origin. Nothing else in the merge path publishes the per-task
|
||||
@@ -903,6 +946,8 @@ export async function processPullRequestMergeTask(
|
||||
}
|
||||
try {
|
||||
prInfo = existingPr ?? await github.createPr({
|
||||
owner: prRepo.owner,
|
||||
repo: prRepo.repo,
|
||||
title: buildPullRequestTitle(task),
|
||||
body: buildPullRequestBody(task),
|
||||
head: branch,
|
||||
@@ -987,7 +1032,7 @@ export async function processPullRequestMergeTask(
|
||||
await store.updateTask(task.id, { status: "merging-pr" });
|
||||
let mergedPr: PrInfo;
|
||||
try {
|
||||
mergedPr = await github.mergePr({ number: prInfo.number, method: "squash" });
|
||||
mergedPr = await github.mergePr({ owner: prRepo.owner, repo: prRepo.repo, number: prInfo.number, method: "squash" });
|
||||
} catch (err: unknown) {
|
||||
let refreshedStatus: Awaited<ReturnType<GitHubOperations["getPrMergeStatus"]>>;
|
||||
try {
|
||||
|
||||
127
packages/engine/src/__tests__/pr-respond-cwd-resolution.test.ts
Normal file
127
packages/engine/src/__tests__/pr-respond-cwd-resolution.test.ts
Normal file
@@ -0,0 +1,127 @@
|
||||
import { describe, it, expect, vi, beforeEach } from "vitest";
|
||||
|
||||
// Capture the cwd + store handed to the response agent runner and the git-ops
|
||||
// resolver, so these tests can assert which directory the respond run actually
|
||||
// targets and that getTask-less structural stores are withheld from the runner.
|
||||
const agentRunnerCalls = vi.hoisted(
|
||||
() => [] as Array<{ taskId: string; cwd: string; store: unknown }>,
|
||||
);
|
||||
const gitOpsResolvers = vi.hoisted(() => [] as Array<(entity: unknown) => string>);
|
||||
|
||||
vi.mock("../pr-response-run-ops.js", () => ({
|
||||
makePrResponseAgentRunner: vi.fn(
|
||||
(_settings: unknown, taskId: string, cwd: string, store: unknown) => {
|
||||
agentRunnerCalls.push({ taskId, cwd, store });
|
||||
return vi.fn();
|
||||
},
|
||||
),
|
||||
makePrResponseGitOps: vi.fn((getCwd: (entity: unknown) => string) => {
|
||||
gitOpsResolvers.push(getCwd);
|
||||
return {
|
||||
getChangedContent: vi.fn(),
|
||||
getWorktreeHeadOid: vi.fn(),
|
||||
fetchAndFastForwardPush: vi.fn(),
|
||||
};
|
||||
}),
|
||||
}));
|
||||
|
||||
vi.mock("../pr-response-run.js", () => ({
|
||||
runPrResponseRun: vi.fn(async () => ({ value: "resolved-all" })),
|
||||
}));
|
||||
|
||||
import { buildRespondCallback } from "../pr-nodes.js";
|
||||
|
||||
const entity = {
|
||||
id: "pr-entity-1",
|
||||
sourceType: "task",
|
||||
sourceId: "FN-1",
|
||||
repo: "owner/repo",
|
||||
headBranch: "fusion/fn-1",
|
||||
state: "open",
|
||||
autoMerge: false,
|
||||
unverified: false,
|
||||
responseRounds: 0,
|
||||
createdAt: 1,
|
||||
updatedAt: 1,
|
||||
prNumber: 7,
|
||||
} as never;
|
||||
|
||||
function makeOps(getCwd: (e: unknown) => string) {
|
||||
return {
|
||||
getReviewThreads: vi.fn(async () => []),
|
||||
getViewerLogin: vi.fn(async () => "viewer"),
|
||||
checkPrStillOpen: vi.fn(async () => ({ open: true, headOid: null })),
|
||||
replyToThread: vi.fn(),
|
||||
resolveThread: vi.fn(),
|
||||
getCwd,
|
||||
getTaskId: () => "FN-1",
|
||||
} as never;
|
||||
}
|
||||
|
||||
function makeStore(worktree?: string, opts?: { getTaskThrows?: boolean }) {
|
||||
return {
|
||||
getSettings: vi.fn(async () => ({})),
|
||||
getTask: opts?.getTaskThrows
|
||||
? vi.fn(async () => {
|
||||
throw new Error("missing task");
|
||||
})
|
||||
: vi.fn(async () => ({ id: "FN-1", worktree })),
|
||||
};
|
||||
}
|
||||
|
||||
beforeEach(() => {
|
||||
agentRunnerCalls.length = 0;
|
||||
gitOpsResolvers.length = 0;
|
||||
});
|
||||
|
||||
describe("buildRespondCallback cwd resolution (gh-4)", () => {
|
||||
it("prefers the task's recorded worktree over the CLI getCwd resolver (process.cwd() in central installs)", async () => {
|
||||
const getCwd = vi.fn(() => "/central/install-dir");
|
||||
const store = makeStore("/projects/repo-a/.worktrees/fn-1");
|
||||
const respond = buildRespondCallback(() => store as never, makeOps(getCwd));
|
||||
|
||||
const result = await respond({ entity } as never);
|
||||
|
||||
expect(result).toEqual({ value: "resolved-all" });
|
||||
expect(agentRunnerCalls).toEqual([
|
||||
{ taskId: "FN-1", cwd: "/projects/repo-a/.worktrees/fn-1", store },
|
||||
]);
|
||||
expect(gitOpsResolvers).toHaveLength(1);
|
||||
expect(gitOpsResolvers[0](entity)).toBe("/projects/repo-a/.worktrees/fn-1");
|
||||
});
|
||||
|
||||
it("falls back to the CLI getCwd resolver when the task has no recorded worktree", async () => {
|
||||
const getCwd = vi.fn(() => "/single-project/checkout");
|
||||
const store = makeStore(undefined);
|
||||
const respond = buildRespondCallback(() => store as never, makeOps(getCwd));
|
||||
|
||||
await respond({ entity } as never);
|
||||
|
||||
expect(agentRunnerCalls).toEqual([{ taskId: "FN-1", cwd: "/single-project/checkout", store }]);
|
||||
expect(gitOpsResolvers[0](entity)).toBe("/single-project/checkout");
|
||||
});
|
||||
|
||||
it("propagates a real store read failure instead of silently running in the fallback cwd (routable respond-error at the node)", async () => {
|
||||
const getCwd = vi.fn(() => "/single-project/checkout");
|
||||
const respond = buildRespondCallback(
|
||||
() => makeStore(undefined, { getTaskThrows: true }) as never,
|
||||
makeOps(getCwd),
|
||||
);
|
||||
|
||||
await expect(respond({ entity } as never)).rejects.toThrow("missing task");
|
||||
expect(agentRunnerCalls).toEqual([]);
|
||||
expect(gitOpsResolvers).toEqual([]);
|
||||
});
|
||||
|
||||
it("falls back to the CLI getCwd resolver on structural stores without getTask, and withholds the store from the agent runner", async () => {
|
||||
const getCwd = vi.fn(() => "/single-project/checkout");
|
||||
const storeWithoutGetTask = { getSettings: vi.fn(async () => ({})) };
|
||||
const respond = buildRespondCallback(() => storeWithoutGetTask as never, makeOps(getCwd));
|
||||
|
||||
await respond({ entity } as never);
|
||||
|
||||
expect(agentRunnerCalls).toEqual([
|
||||
{ taskId: "FN-1", cwd: "/single-project/checkout", store: undefined },
|
||||
]);
|
||||
});
|
||||
});
|
||||
@@ -222,7 +222,6 @@ export function buildRespondCallback(
|
||||
*/
|
||||
pluginRunner?: import("./plugin-runner.js").PluginRunner,
|
||||
): NonNullable<PrNodeDeps["respond"]> {
|
||||
const gitOps = makePrResponseGitOps(ops.getCwd);
|
||||
return async ({ entity }) => {
|
||||
const store = getStore();
|
||||
// The engine owns a concrete TaskStore behind the structural PrNodeStore; the
|
||||
@@ -247,8 +246,34 @@ export function buildRespondCallback(
|
||||
}
|
||||
|
||||
const taskId = ops.getTaskId(entity);
|
||||
const cwd = ops.getCwd(entity);
|
||||
const runAgent = makePrResponseAgentRunner(settings, taskId, cwd, fullStore, pluginRunner);
|
||||
/*
|
||||
* FNXC:PrMergeAutoMerge 2026-07-17-19:18 (gh-4):
|
||||
* Resolve the review-response run cwd from the task's recorded worktree first.
|
||||
* The CLI-injected ops.getCwd defaults to process.cwd() (no CLI composition
|
||||
* site wires getTaskWorktree), which in a centrally-installed multi-project
|
||||
* server is the install dir — git ops and the response agent would run outside
|
||||
* the project repo. The engine owns the store, so it recovers the worktree
|
||||
* here; ops.getCwd stays the fallback ONLY for structural stores without
|
||||
* getTask (PrNodeStore does not declare it — tests/specialized wiring). A
|
||||
* real getTask rejection (store failure, deleted task) propagates instead:
|
||||
* the pr-respond node maps it to a routable `respond-error`, which beats
|
||||
* running a mutating agent + git push in a cwd that may not even be the
|
||||
* project repo. Structural stores are also withheld from the agent runner,
|
||||
* whose store parameter is optional but assumed to have getTask.
|
||||
*/
|
||||
const hasGetTask = typeof (fullStore as Partial<typeof fullStore>).getTask === "function";
|
||||
const respondTask: { worktree?: string } | null = hasGetTask
|
||||
? await fullStore.getTask(taskId)
|
||||
: null;
|
||||
const cwd = respondTask?.worktree || ops.getCwd(entity);
|
||||
const gitOps = makePrResponseGitOps(() => cwd);
|
||||
const runAgent = makePrResponseAgentRunner(
|
||||
settings,
|
||||
taskId,
|
||||
cwd,
|
||||
hasGetTask ? fullStore : undefined,
|
||||
pluginRunner,
|
||||
);
|
||||
|
||||
const result = await runPrResponseRun({
|
||||
entity,
|
||||
|
||||
Reference in New Issue
Block a user