feat(FN-3202): handle manual PR linking and feedback follow-ups
- Add scheduler logic to create dependency-linked follow-up tasks when actionable PR feedback remains after a PR is merged or closed - Update engine runtime/project wiring to support manual PR create flows and branch publish behavior for fusion/<task-id> - Add dashboard route coverage for manual PR creation/linking behavior and corresponding engine/runtime tests - Document manual PR branch conventions and follow-up behavior in task management and dashboard docs Fusion-Task-Id: FN-3202
This commit is contained in:
@@ -456,9 +456,9 @@ When the merge strategy is **Pull request**:
|
||||
- A blocking review state (for example, active changes requested) prevents auto-merge until cleared
|
||||
- Closed PRs do not auto-merge
|
||||
- GitHub access for PR-first workflows must be available via `gh auth login`
|
||||
- kb expects the task branch to already be pushed using the standard branch name `kb/<task-id-lower>`
|
||||
|
||||
**Non-goal:** the dashboard does not implicitly push branches before PR creation. Use your normal git workflow or automation to publish task branches first.
|
||||
- Task PR flows use the canonical branch name `fusion/<task-id-lower>`
|
||||
- Manual PR creation (`POST /api/tasks/:id/pr/create`) first checks for an existing PR on the task branch and links it instead of creating duplicates
|
||||
- When no PR exists, the dashboard publishes `fusion/<task-id-lower>` (`git push -u origin ...`) before creating the PR so manual PR creation works even when `autoMerge` is disabled
|
||||
|
||||
## Theming
|
||||
|
||||
|
||||
@@ -16,6 +16,7 @@ import {
|
||||
getScopedStore as resolveRouteScopedStore,
|
||||
} from "../routes/context.js";
|
||||
import { GitHubClient } from "../github.js";
|
||||
import * as resolveDiffBaseModule from "../routes/resolve-diff-base.js";
|
||||
import { githubRateLimiter } from "../github-poll.js";
|
||||
import type { TaskStore, TaskAttachment, Routine, RoutineCreateInput, RoutineUpdateInput, RoutineExecutionResult, ChatSession, ChatMessage } from "@fusion/core";
|
||||
import type { TaskDetail } from "@fusion/core";
|
||||
@@ -6569,6 +6570,59 @@ describe("Pause/Unpause endpoints", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it("reuses an existing branch PR without pushing or creating a duplicate", async () => {
|
||||
const originalEnv = process.env.GITHUB_REPOSITORY;
|
||||
process.env.GITHUB_REPOSITORY = "owner/repo";
|
||||
(store.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(mockInReviewTask);
|
||||
const existingPr = { ...mockPrInfo, number: 77, url: "https://github.com/owner/repo/pull/77" };
|
||||
const findSpy = vi.spyOn(GitHubClient.prototype, "findPrForBranch").mockResolvedValue(existingPr);
|
||||
const createSpy = vi.spyOn(GitHubClient.prototype, "createPr").mockResolvedValue(mockPrInfo);
|
||||
const pushSpy = vi.spyOn(resolveDiffBaseModule, "runGitCommand").mockResolvedValue("ok");
|
||||
|
||||
const res = await REQUEST(
|
||||
buildApp(),
|
||||
"POST",
|
||||
"/api/tasks/KB-001/pr/create",
|
||||
JSON.stringify({ title: "Test PR" }),
|
||||
{ "Content-Type": "application/json" }
|
||||
);
|
||||
|
||||
expect(res.status).toBe(201);
|
||||
expect(findSpy).toHaveBeenCalledWith(expect.objectContaining({ head: "fusion/fn-001", state: "all" }));
|
||||
expect(createSpy).not.toHaveBeenCalled();
|
||||
expect(pushSpy).not.toHaveBeenCalledWith(["push", "-u", "origin", "fusion/fn-001"], expect.anything(), expect.anything());
|
||||
expect(store.logEntry).toHaveBeenCalledWith("FN-001", "Linked existing PR", "PR #77: https://github.com/owner/repo/pull/77");
|
||||
|
||||
if (originalEnv) process.env.GITHUB_REPOSITORY = originalEnv;
|
||||
else delete process.env.GITHUB_REPOSITORY;
|
||||
});
|
||||
|
||||
it("pushes the task branch before creating a PR when no existing PR is found", async () => {
|
||||
const originalEnv = process.env.GITHUB_REPOSITORY;
|
||||
process.env.GITHUB_REPOSITORY = "owner/repo";
|
||||
(store.getTask as ReturnType<typeof vi.fn>).mockResolvedValue(mockInReviewTask);
|
||||
const findSpy = vi.spyOn(GitHubClient.prototype, "findPrForBranch").mockResolvedValue(null);
|
||||
const createSpy = vi.spyOn(GitHubClient.prototype, "createPr").mockResolvedValue(mockPrInfo);
|
||||
const pushSpy = vi.spyOn(resolveDiffBaseModule, "runGitCommand").mockResolvedValue("ok");
|
||||
|
||||
const res = await REQUEST(
|
||||
buildApp(),
|
||||
"POST",
|
||||
"/api/tasks/KB-001/pr/create",
|
||||
JSON.stringify({ title: "Test PR" }),
|
||||
{ "Content-Type": "application/json" }
|
||||
);
|
||||
|
||||
expect(res.status).toBe(201);
|
||||
expect(findSpy).toHaveBeenCalled();
|
||||
expect(pushSpy).toHaveBeenCalledWith(["push", "-u", "origin", "fusion/fn-001"], "/fake/root", 60_000);
|
||||
expect(createSpy).toHaveBeenCalledWith(expect.objectContaining({ head: "fusion/fn-001" }));
|
||||
expect(store.logEntry).toHaveBeenCalledWith("FN-001", "Created PR", "PR #42: https://github.com/owner/repo/pull/42");
|
||||
|
||||
if (originalEnv) process.env.GITHUB_REPOSITORY = originalEnv;
|
||||
else delete process.env.GITHUB_REPOSITORY;
|
||||
});
|
||||
|
||||
it("returns 404 for non-existent task", async () => {
|
||||
// Create error with proper ENOENT code
|
||||
const error = new Error("ENOENT: task not found") as NodeJS.ErrnoException;
|
||||
|
||||
@@ -2854,21 +2854,27 @@ export function registerGitGitHubRoutes(ctx: ApiRoutesContext): void {
|
||||
});
|
||||
}
|
||||
|
||||
// Create the PR
|
||||
const client = new GitHubClient();
|
||||
const existingPr = await client.findPrForBranch({ head: branchName, state: "all", owner, repo });
|
||||
|
||||
const prInfo = await client.createPr({
|
||||
owner,
|
||||
repo,
|
||||
title,
|
||||
body,
|
||||
head: branchName,
|
||||
base,
|
||||
});
|
||||
let prInfo: PrInfo;
|
||||
if (existingPr) {
|
||||
prInfo = existingPr;
|
||||
} else {
|
||||
await runGitCommand(["push", "-u", "origin", branchName], scopedStore.getRootDir(), 60_000);
|
||||
prInfo = await client.createPr({
|
||||
owner,
|
||||
repo,
|
||||
title,
|
||||
body,
|
||||
head: branchName,
|
||||
base,
|
||||
});
|
||||
}
|
||||
|
||||
// Store PR info
|
||||
await scopedStore.updatePrInfo(task.id, prInfo);
|
||||
await scopedStore.logEntry(task.id, "Created PR", `PR #${prInfo.number}: ${prInfo.url}`);
|
||||
await scopedStore.logEntry(task.id, existingPr ? "Linked existing PR" : "Created PR", `PR #${prInfo.number}: ${prInfo.url}`);
|
||||
|
||||
res.status(201).json(prInfo);
|
||||
} catch (err: unknown) {
|
||||
|
||||
@@ -24,6 +24,8 @@ const mocks = vi.hoisted(() => ({
|
||||
notifierNotifyGridlock: vi.fn(),
|
||||
notificationServiceStart: vi.fn(async () => undefined),
|
||||
notificationServiceStop: vi.fn(),
|
||||
runtimeConfigurePrMonitoring: vi.fn(),
|
||||
prHandlerCreateFollowUpTask: vi.fn(async () => undefined),
|
||||
}));
|
||||
|
||||
vi.mock("@fusion/core", async (importOriginal) => {
|
||||
@@ -71,6 +73,7 @@ vi.mock("../pr-monitor.js", () => ({
|
||||
vi.mock("../pr-comment-handler.js", () => ({
|
||||
PrCommentHandler: vi.fn().mockImplementation(() => ({
|
||||
handleNewComments: vi.fn(),
|
||||
createFollowUpTask: mocks.prHandlerCreateFollowUpTask,
|
||||
})),
|
||||
}));
|
||||
|
||||
@@ -101,6 +104,7 @@ vi.mock("../runtimes/in-process-runtime.js", () => ({
|
||||
getRoutineRunner: vi.fn(),
|
||||
getHeartbeatMonitor: vi.fn(),
|
||||
getTriggerScheduler: vi.fn(),
|
||||
configurePrMonitoring: mocks.runtimeConfigurePrMonitoring,
|
||||
})),
|
||||
}));
|
||||
|
||||
@@ -284,6 +288,36 @@ describe("ProjectEngine notification ownership wiring", () => {
|
||||
});
|
||||
});
|
||||
|
||||
describe("ProjectEngine PR monitoring wiring", () => {
|
||||
it("wires runtime scheduler PR monitoring with closed-PR follow-up handler", async () => {
|
||||
const { store } = createMockStore(baseSettings);
|
||||
mocks.currentStore = store;
|
||||
|
||||
const engine = createEngine();
|
||||
await engine.start();
|
||||
|
||||
expect(mocks.runtimeConfigurePrMonitoring).toHaveBeenCalled();
|
||||
const configArg = mocks.runtimeConfigurePrMonitoring.mock.calls.at(-1)?.[0] as {
|
||||
onClosedPrFeedback?: (taskId: string, prInfo: Record<string, unknown>, comments: unknown[]) => Promise<void> | void;
|
||||
};
|
||||
expect(typeof configArg.onClosedPrFeedback).toBe("function");
|
||||
|
||||
await configArg.onClosedPrFeedback?.(
|
||||
"FN-3202",
|
||||
{ number: 12, status: "merged", url: "https://example/pr/12" } as never,
|
||||
[{ id: 1, body: "please fix", user: { login: "reviewer" } }] as never,
|
||||
);
|
||||
|
||||
expect(mocks.prHandlerCreateFollowUpTask).toHaveBeenCalledWith(
|
||||
"FN-3202",
|
||||
expect.objectContaining({ number: 12 }),
|
||||
expect.any(Array),
|
||||
);
|
||||
|
||||
await engine.stop();
|
||||
});
|
||||
});
|
||||
|
||||
describe("ProjectEngine auto-summarize wiring", () => {
|
||||
beforeEach(() => {
|
||||
vi.clearAllMocks();
|
||||
|
||||
@@ -273,6 +273,11 @@ export class ProjectEngine {
|
||||
this.prMonitor.onNewComments((taskId, prInfo, comments) =>
|
||||
this.prCommentHandler!.handleNewComments(taskId, prInfo, comments),
|
||||
);
|
||||
this.runtime.configurePrMonitoring({
|
||||
prMonitor: this.prMonitor,
|
||||
onClosedPrFeedback: (taskId, prInfo, comments) =>
|
||||
this.prCommentHandler!.createFollowUpTask(taskId, prInfo, comments),
|
||||
});
|
||||
|
||||
// 3. Initialize notification services (unless caller manages them externally)
|
||||
if (!this.options.skipNotifier) {
|
||||
|
||||
@@ -18,6 +18,7 @@ const {
|
||||
mockResumeOrphaned,
|
||||
mockTaskStoreSettings,
|
||||
mockMessageStoreSetHook,
|
||||
mockSchedulerConfigurePrMonitoring,
|
||||
} = vi.hoisted(() => ({
|
||||
mockSelfHealingStart: vi.fn(),
|
||||
mockSelfHealingStop: vi.fn(),
|
||||
@@ -28,6 +29,7 @@ const {
|
||||
mockResumeOrphaned: vi.fn().mockResolvedValue(undefined),
|
||||
mockTaskStoreSettings: {} as Record<string, unknown>,
|
||||
mockMessageStoreSetHook: vi.fn(),
|
||||
mockSchedulerConfigurePrMonitoring: vi.fn(),
|
||||
}));
|
||||
|
||||
// Mock the TaskStore class
|
||||
@@ -108,6 +110,7 @@ vi.mock("../../scheduler.js", async () => {
|
||||
self.start = vi.fn();
|
||||
self.stop = vi.fn();
|
||||
self.reconcileAllMissionFeatures = vi.fn().mockResolvedValue(0);
|
||||
self.configurePrMonitoring = mockSchedulerConfigurePrMonitoring;
|
||||
return self;
|
||||
}),
|
||||
};
|
||||
@@ -473,6 +476,19 @@ describe("InProcessRuntime", () => {
|
||||
it("should return undefined TriggerScheduler before start", () => {
|
||||
expect(runtime.getTriggerScheduler()).toBeUndefined();
|
||||
});
|
||||
|
||||
it("configures scheduler PR monitoring after start", async () => {
|
||||
await runtime.start();
|
||||
runtime.configurePrMonitoring({
|
||||
prMonitor: {} as never,
|
||||
onClosedPrFeedback: vi.fn(),
|
||||
});
|
||||
|
||||
expect(mockSchedulerConfigurePrMonitoring).toHaveBeenCalledTimes(1);
|
||||
expect(mockSchedulerConfigurePrMonitoring).toHaveBeenCalledWith(expect.objectContaining({
|
||||
prMonitor: expect.any(Object),
|
||||
}));
|
||||
});
|
||||
});
|
||||
|
||||
describe("trigger scheduler wiring", () => {
|
||||
|
||||
@@ -13,6 +13,8 @@ import type {
|
||||
} from "@fusion/core";
|
||||
import { isEphemeralAgent } from "@fusion/core";
|
||||
import { Scheduler } from "../scheduler.js";
|
||||
import type { PrMonitor, PrComment } from "../pr-monitor.js";
|
||||
import type { PrInfo } from "@fusion/core";
|
||||
import { TaskExecutor, type TaskExecutorOptions } from "../executor.js";
|
||||
import { WorktreePool, isGitRepository } from "../worktree-pool.js";
|
||||
import { AgentSemaphore } from "../concurrency.js";
|
||||
@@ -1004,6 +1006,17 @@ export class InProcessRuntime
|
||||
return this.scheduler;
|
||||
}
|
||||
|
||||
configurePrMonitoring(options: {
|
||||
prMonitor: PrMonitor;
|
||||
onClosedPrFeedback?: (taskId: string, prInfo: PrInfo, comments: PrComment[]) => void | Promise<void>;
|
||||
}): void {
|
||||
if (!this.scheduler) {
|
||||
throw new Error("Scheduler not initialized. Call start() first.");
|
||||
}
|
||||
|
||||
this.scheduler.configurePrMonitoring(options);
|
||||
}
|
||||
|
||||
/**
|
||||
* Get current runtime metrics.
|
||||
*/
|
||||
|
||||
@@ -424,6 +424,32 @@ export class Scheduler {
|
||||
return this.options.missionAutopilot;
|
||||
}
|
||||
|
||||
configurePrMonitoring(options: {
|
||||
prMonitor?: PrMonitor;
|
||||
onClosedPrFeedback?: SchedulerOptions["onClosedPrFeedback"];
|
||||
}): void {
|
||||
this.options.prMonitor = options.prMonitor;
|
||||
this.options.onClosedPrFeedback = options.onClosedPrFeedback;
|
||||
|
||||
if (!options.prMonitor) {
|
||||
return;
|
||||
}
|
||||
|
||||
void this.store.listTasks({ slim: true, includeArchived: false })
|
||||
.then((tasks) => {
|
||||
const repo = getCurrentRepo(this.store.getRootDir());
|
||||
if (!repo) return;
|
||||
|
||||
for (const task of tasks) {
|
||||
if (task.column !== "in-review" || !task.prInfo) continue;
|
||||
options.prMonitor!.startMonitoring(task.id, repo.owner, repo.repo, task.prInfo);
|
||||
}
|
||||
})
|
||||
.catch((err) => {
|
||||
schedulerLog.error("Failed to hydrate PR monitoring from existing in-review tasks:", err);
|
||||
});
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolve the base branch for a task being started.
|
||||
*
|
||||
|
||||
Reference in New Issue
Block a user