feat(FN-840): add closed/merged PR feedback contract and clean up unused code
- Add closed/merged PR feedback contract to pr-monitor for post-merge follow-up behavior - Fix PR monitor drainComments + consume/drain behavior with comprehensive tests - Add PR feedback follow-up tests and scheduler follow-up logic - Remove unused activity log, agent log viewer, and multi-agent log code from dashboard - Remove unused store methods, types, API endpoints, and route handlers - Update README docs to document manual-merge PR follow-up behavior and fix auth wording - Add dashboard CLI test coverage for new commands
This commit is contained in:
10
README.md
10
README.md
@@ -540,12 +540,16 @@ Fusion supports two completion strategies once a task reaches **In Review**:
|
|||||||
|
|
||||||
PR-first automation is designed for repositories that require GitHub-side governance:
|
PR-first automation is designed for repositories that require GitHub-side governance:
|
||||||
|
|
||||||
- Authenticate GitHub access with `gh auth login` or `GITHUB_TOKEN`
|
- Authenticate GitHub access with `gh auth login` (the `gh` CLI is required for PR monitoring and badge updates)
|
||||||
- Ensure the task branch already exists on GitHub using the normal fusion branch naming convention: `fusion/<task-id-lower>`
|
- Ensure the task branch already exists on GitHub using the normal fusion branch naming convention: `fusion/<task-id-lower>`
|
||||||
- Expect the task to remain in **In Review** while required checks are pending/failing or a review is blocking merge
|
- Expect the task to remain in **In Review** while required checks are pending/failing or a review is blocking merge
|
||||||
|
|
||||||
**Important:** Fusion does **not** implicitly push task branches before creating a PR. PR-first mode assumes branch publishing is handled by your existing workflow or repository automation.
|
**Important:** Fusion does **not** implicitly push task branches before creating a PR. PR-first mode assumes branch publishing is handled by your existing workflow or repository automation.
|
||||||
|
|
||||||
|
#### Manual merge (`autoMerge=false`)
|
||||||
|
|
||||||
|
When `autoMerge` is off and a task's PR is closed or merged, Fusion checks for unaddressed review feedback that was collected while the PR was open. If actionable comments were left by reviewers, Fusion automatically creates a follow-up task in **Triage** depending on the original task. This ensures PR feedback is never silently dropped even when automatic merging is disabled.
|
||||||
|
|
||||||
### Spec Editing & AI Revision
|
### Spec Editing & AI Revision
|
||||||
|
|
||||||
The dashboard includes a **Spec** tab for managing task specifications directly in the UI:
|
The dashboard includes a **Spec** tab for managing task specifications directly in the UI:
|
||||||
@@ -573,9 +577,9 @@ When a task has a linked PR, Fusion automatically monitors it for new review com
|
|||||||
- **Adaptive polling**: Checks every 30 seconds when active, 5 minutes when idle
|
- **Adaptive polling**: Checks every 30 seconds when active, 5 minutes when idle
|
||||||
- **Actionable feedback detection**: Filters out "LGTM" and "Thanks" comments, detects requests like "fix", "change", "update"
|
- **Actionable feedback detection**: Filters out "LGTM" and "Thanks" comments, detects requests like "fix", "change", "update"
|
||||||
- **Steering comments**: Automatically adds actionable review feedback as steering comments on the task
|
- **Steering comments**: Automatically adds actionable review feedback as steering comments on the task
|
||||||
- **Follow-up tasks**: When a PR is closed with unaddressed feedback, a follow-up task is created
|
- **Follow-up tasks**: When a PR is closed or merged with unaddressed feedback, a follow-up task is created in Triage
|
||||||
|
|
||||||
Uses `gh` CLI authentication when available, falls back to `GITHUB_TOKEN` if set.
|
Requires `gh` CLI installed and authenticated (`gh auth login`). PR monitoring does not support `GITHUB_TOKEN` as a fallback.
|
||||||
|
|
||||||
## Task Comments vs Steering Comments
|
## Task Comments vs Steering Comments
|
||||||
|
|
||||||
|
|||||||
@@ -172,9 +172,11 @@ vi.mock("@fusion/engine", async (importOriginal) => {
|
|||||||
stopAll: vi.fn(),
|
stopAll: vi.fn(),
|
||||||
getTrackedPrs: vi.fn().mockReturnValue(new Map()),
|
getTrackedPrs: vi.fn().mockReturnValue(new Map()),
|
||||||
updatePrInfo: vi.fn(),
|
updatePrInfo: vi.fn(),
|
||||||
|
drainComments: vi.fn().mockReturnValue([]),
|
||||||
})),
|
})),
|
||||||
PrCommentHandler: vi.fn().mockImplementation(() => ({
|
PrCommentHandler: vi.fn().mockImplementation(() => ({
|
||||||
handleNewComments: vi.fn().mockResolvedValue(undefined),
|
handleNewComments: vi.fn().mockResolvedValue(undefined),
|
||||||
|
createFollowUpTask: vi.fn().mockResolvedValue(undefined),
|
||||||
})),
|
})),
|
||||||
aiMergeTask: vi.fn().mockImplementation(() => Promise.resolve({ merged: true })),
|
aiMergeTask: vi.fn().mockImplementation(() => Promise.resolve({ merged: true })),
|
||||||
CronRunner: vi.fn().mockImplementation(() => ({
|
CronRunner: vi.fn().mockImplementation(() => ({
|
||||||
@@ -1437,6 +1439,86 @@ describe("runDashboard — merge conflict retry logic", () => {
|
|||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("runDashboard — PR feedback follow-up wiring", () => {
|
||||||
|
let mockStore: ReturnType<typeof makeMockStore>;
|
||||||
|
|
||||||
|
beforeEach(async () => {
|
||||||
|
capturedExecutorOpts = undefined;
|
||||||
|
vi.clearAllMocks();
|
||||||
|
resetGitHubMocks();
|
||||||
|
mockStore = makeMockStore();
|
||||||
|
const { TaskStore } = await import("@fusion/core");
|
||||||
|
(TaskStore as unknown as ReturnType<typeof vi.fn>).mockImplementation(() => mockStore);
|
||||||
|
const engine = await import("@fusion/engine");
|
||||||
|
(engine.aiMergeTask as ReturnType<typeof vi.fn>).mockImplementation(() =>
|
||||||
|
Promise.resolve({ merged: true }),
|
||||||
|
);
|
||||||
|
(engine.TaskExecutor as unknown as ReturnType<typeof vi.fn>).mockImplementation(
|
||||||
|
(_store: unknown, _cwd: unknown, opts: unknown) => {
|
||||||
|
capturedExecutorOpts = opts as Record<string, unknown>;
|
||||||
|
return { resumeOrphaned: vi.fn().mockResolvedValue(undefined) };
|
||||||
|
},
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("wires onClosedPrFeedback callback to PrCommentHandler.createFollowUpTask", async () => {
|
||||||
|
const { PrMonitor, PrCommentHandler, Scheduler } = await import("@fusion/engine");
|
||||||
|
|
||||||
|
let capturedOnClosedPrFeedback: ((taskId: string, prInfo: any, comments: any[]) => void) | undefined;
|
||||||
|
|
||||||
|
(Scheduler as unknown as ReturnType<typeof vi.fn>).mockImplementation(
|
||||||
|
(_store: unknown, _opts: unknown) => {
|
||||||
|
capturedOnClosedPrFeedback = _opts.onClosedPrFeedback;
|
||||||
|
return { start: vi.fn(), stop: vi.fn() };
|
||||||
|
},
|
||||||
|
);
|
||||||
|
|
||||||
|
await runDashboard(0, { open: false });
|
||||||
|
|
||||||
|
// Verify the callback was passed to the scheduler
|
||||||
|
expect(capturedOnClosedPrFeedback).toBeDefined();
|
||||||
|
|
||||||
|
// Invoke it to verify it reaches createFollowUpTask
|
||||||
|
const mockPrInfo = { status: "merged", number: 42 };
|
||||||
|
const mockComments = [
|
||||||
|
{ id: 1, body: "Fix this", user: { login: "reviewer" }, created_at: "2024-01-01", updated_at: "2024-01-01", html_url: "" },
|
||||||
|
];
|
||||||
|
await capturedOnClosedPrFeedback("FN-001", mockPrInfo, mockComments);
|
||||||
|
|
||||||
|
// The PrCommentHandler mock should have been called
|
||||||
|
const handlerInstance = (PrCommentHandler as unknown as ReturnType<typeof vi.fn>).mock.results[0].value;
|
||||||
|
expect(handlerInstance.createFollowUpTask).toHaveBeenCalledWith("FN-001", mockPrInfo, mockComments);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("preserves existing onNewComments steering behavior", async () => {
|
||||||
|
const { PrMonitor, PrCommentHandler } = await import("@fusion/engine");
|
||||||
|
|
||||||
|
let capturedOnNewComments: ((taskId: string, prInfo: any, comments: any[]) => void) | undefined;
|
||||||
|
|
||||||
|
(PrMonitor as unknown as ReturnType<typeof vi.fn>).mockImplementation(() => ({
|
||||||
|
onNewComments: vi.fn((cb: any) => { capturedOnNewComments = cb; }),
|
||||||
|
startMonitoring: vi.fn(),
|
||||||
|
stopMonitoring: vi.fn(),
|
||||||
|
stopAll: vi.fn(),
|
||||||
|
getTrackedPrs: vi.fn().mockReturnValue(new Map()),
|
||||||
|
updatePrInfo: vi.fn(),
|
||||||
|
drainComments: vi.fn().mockReturnValue([]),
|
||||||
|
}));
|
||||||
|
|
||||||
|
await runDashboard(0, { open: false });
|
||||||
|
|
||||||
|
// The onNewComments callback should still be wired to handleNewComments
|
||||||
|
expect(capturedOnNewComments).toBeDefined();
|
||||||
|
const handlerInstance = (PrCommentHandler as unknown as ReturnType<typeof vi.fn>).mock.results[0].value;
|
||||||
|
const mockComments = [
|
||||||
|
{ id: 1, body: "Fix this", user: { login: "reviewer" }, created_at: "2024-01-01", updated_at: "2024-01-01", html_url: "" },
|
||||||
|
];
|
||||||
|
const mockPrInfo = { status: "open", number: 42 };
|
||||||
|
await capturedOnNewComments("FN-001", mockPrInfo, mockComments);
|
||||||
|
expect(handlerInstance.handleNewComments).toHaveBeenCalledWith("FN-001", mockPrInfo, mockComments);
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
// ── promptForPort tests ───────────────────────────────────────────────
|
// ── promptForPort tests ───────────────────────────────────────────────
|
||||||
|
|
||||||
import { promptForPort } from "./dashboard.js";
|
import { promptForPort } from "./dashboard.js";
|
||||||
|
|||||||
@@ -606,6 +606,9 @@ export async function runDashboard(port: number, opts: { paused?: boolean; dev?:
|
|||||||
prMonitor,
|
prMonitor,
|
||||||
onSchedule: (t) => console.log(`[engine] Scheduled ${t.id}`),
|
onSchedule: (t) => console.log(`[engine] Scheduled ${t.id}`),
|
||||||
onBlocked: (t, deps) => console.log(`[engine] ${t.id} blocked by ${deps.join(", ")}`),
|
onBlocked: (t, deps) => console.log(`[engine] ${t.id} blocked by ${deps.join(", ")}`),
|
||||||
|
onClosedPrFeedback: async (taskId, prInfo, comments) => {
|
||||||
|
await prCommentHandler.createFollowUpTask(taskId, prInfo, comments);
|
||||||
|
},
|
||||||
});
|
});
|
||||||
|
|
||||||
// ── CronRunner: scheduled task execution ──────────────────────────
|
// ── CronRunner: scheduled task execution ──────────────────────────
|
||||||
|
|||||||
@@ -126,4 +126,73 @@ describe("PrMonitor", () => {
|
|||||||
expect(() => new PrMonitor({ getGitHubToken: () => "token" })).not.toThrow();
|
expect(() => new PrMonitor({ getGitHubToken: () => "token" })).not.toThrow();
|
||||||
});
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
|
describe("drainComments", () => {
|
||||||
|
it("returns empty array when task is not tracked", () => {
|
||||||
|
const result = monitor.drainComments("FN-999");
|
||||||
|
expect(result).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns empty array for tracked PR with no buffered comments", () => {
|
||||||
|
monitor.startMonitoring("FN-001", "owner", "repo", mockPrInfo);
|
||||||
|
const result = monitor.drainComments("FN-001");
|
||||||
|
expect(result).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns buffered comments and clears buffer (single-consumption)", () => {
|
||||||
|
monitor.startMonitoring("FN-001", "owner", "repo", mockPrInfo);
|
||||||
|
|
||||||
|
// Simulate comments being buffered (this is what checkForComments does internally)
|
||||||
|
const tracked = monitor.getTrackedPrs().get("FN-001")!;
|
||||||
|
const comments: PrComment[] = [
|
||||||
|
{ id: 1, body: "Please fix this", user: { login: "reviewer" }, created_at: "2024-01-01", updated_at: "2024-01-01", html_url: "https://example.com" },
|
||||||
|
{ id: 2, body: "Change that", user: { login: "reviewer2" }, created_at: "2024-01-01", updated_at: "2024-01-01", html_url: "https://example.com" },
|
||||||
|
];
|
||||||
|
tracked.bufferedComments.push(...comments);
|
||||||
|
|
||||||
|
// First drain should return the comments
|
||||||
|
const drained = monitor.drainComments("FN-001");
|
||||||
|
expect(drained).toHaveLength(2);
|
||||||
|
expect(drained[0].id).toBe(1);
|
||||||
|
expect(drained[1].id).toBe(2);
|
||||||
|
|
||||||
|
// Second drain should return empty (buffer was cleared)
|
||||||
|
const drainedAgain = monitor.drainComments("FN-001");
|
||||||
|
expect(drainedAgain).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("returns empty array after PR is stopped", () => {
|
||||||
|
monitor.startMonitoring("FN-001", "owner", "repo", mockPrInfo);
|
||||||
|
const tracked = monitor.getTrackedPrs().get("FN-001")!;
|
||||||
|
tracked.bufferedComments.push(
|
||||||
|
{ id: 1, body: "Fix this", user: { login: "reviewer" }, created_at: "2024-01-01", updated_at: "2024-01-01", html_url: "" },
|
||||||
|
);
|
||||||
|
|
||||||
|
monitor.stopMonitoring("FN-001");
|
||||||
|
const result = monitor.drainComments("FN-001");
|
||||||
|
expect(result).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not affect other tracked PRs", () => {
|
||||||
|
monitor.startMonitoring("FN-001", "owner", "repo", mockPrInfo);
|
||||||
|
monitor.startMonitoring("FN-002", "owner", "repo", { ...mockPrInfo, number: 43 });
|
||||||
|
|
||||||
|
const tracked1 = monitor.getTrackedPrs().get("FN-001")!;
|
||||||
|
const tracked2 = monitor.getTrackedPrs().get("FN-002")!;
|
||||||
|
tracked1.bufferedComments.push(
|
||||||
|
{ id: 1, body: "Fix", user: { login: "r" }, created_at: "2024-01-01", updated_at: "2024-01-01", html_url: "" },
|
||||||
|
);
|
||||||
|
tracked2.bufferedComments.push(
|
||||||
|
{ id: 2, body: "Update", user: { login: "r" }, created_at: "2024-01-01", updated_at: "2024-01-01", html_url: "" },
|
||||||
|
);
|
||||||
|
|
||||||
|
// Drain FN-001 only
|
||||||
|
const drained1 = monitor.drainComments("FN-001");
|
||||||
|
expect(drained1).toHaveLength(1);
|
||||||
|
|
||||||
|
// FN-002 buffer should still be intact
|
||||||
|
const drained2 = monitor.drainComments("FN-002");
|
||||||
|
expect(drained2).toHaveLength(1);
|
||||||
|
});
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|||||||
@@ -9,6 +9,8 @@ export interface TrackedPr {
|
|||||||
lastCommentId?: number;
|
lastCommentId?: number;
|
||||||
consecutiveErrors: number;
|
consecutiveErrors: number;
|
||||||
isActive: boolean; // true if we've seen recent activity
|
isActive: boolean; // true if we've seen recent activity
|
||||||
|
/** Buffered comments collected since last drain, used for follow-up task creation. */
|
||||||
|
bufferedComments: PrComment[];
|
||||||
}
|
}
|
||||||
|
|
||||||
export interface PrComment {
|
export interface PrComment {
|
||||||
@@ -141,6 +143,7 @@ export class PrMonitor {
|
|||||||
lastCommentId: undefined,
|
lastCommentId: undefined,
|
||||||
consecutiveErrors: 0,
|
consecutiveErrors: 0,
|
||||||
isActive: true, // Start as active
|
isActive: true, // Start as active
|
||||||
|
bufferedComments: [],
|
||||||
};
|
};
|
||||||
|
|
||||||
this.trackedPrs.set(taskId, tracked);
|
this.trackedPrs.set(taskId, tracked);
|
||||||
@@ -164,6 +167,20 @@ export class PrMonitor {
|
|||||||
tracked.prInfo = prInfo;
|
tracked.prInfo = prInfo;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Drain (consume and return) buffered comments for a tracked PR.
|
||||||
|
* Returns all comments collected since the last drain and clears the buffer.
|
||||||
|
* This is used for creating follow-up tasks when a PR is closed/merged.
|
||||||
|
* Returns an empty array if the task is not tracked or has no buffered comments.
|
||||||
|
*/
|
||||||
|
drainComments(taskId: string): PrComment[] {
|
||||||
|
const tracked = this.trackedPrs.get(taskId);
|
||||||
|
if (!tracked) return [];
|
||||||
|
const comments = tracked.bufferedComments;
|
||||||
|
tracked.bufferedComments = [];
|
||||||
|
return comments;
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Stop monitoring a PR.
|
* Stop monitoring a PR.
|
||||||
*/
|
*/
|
||||||
@@ -253,6 +270,9 @@ export class PrMonitor {
|
|||||||
const maxId = Math.max(...newComments.map((c) => c.id));
|
const maxId = Math.max(...newComments.map((c) => c.id));
|
||||||
tracked.lastCommentId = maxId;
|
tracked.lastCommentId = maxId;
|
||||||
|
|
||||||
|
// Buffer comments for potential follow-up task creation on PR close/merge
|
||||||
|
tracked.bufferedComments.push(...newComments);
|
||||||
|
|
||||||
// Mark as active since we found new comments
|
// Mark as active since we found new comments
|
||||||
tracked.isActive = true;
|
tracked.isActive = true;
|
||||||
|
|
||||||
|
|||||||
@@ -794,6 +794,213 @@ describe("Scheduler", () => {
|
|||||||
|
|
||||||
expect(prMonitor.stopMonitoring).toHaveBeenCalledWith("FN-001");
|
expect(prMonitor.stopMonitoring).toHaveBeenCalledWith("FN-001");
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("invokes onClosedPrFeedback with drained comments for closed/merged PR", async () => {
|
||||||
|
const mockComments = [
|
||||||
|
{ id: 1, body: "Fix this", user: { login: "reviewer" }, created_at: "2024-01-01", updated_at: "2024-01-01", html_url: "https://example.com" },
|
||||||
|
{ id: 2, body: "Update that", user: { login: "reviewer2" }, created_at: "2024-01-01", updated_at: "2024-01-01", html_url: "https://example.com" },
|
||||||
|
];
|
||||||
|
|
||||||
|
const prMonitor = {
|
||||||
|
startMonitoring: vi.fn(),
|
||||||
|
stopMonitoring: vi.fn(),
|
||||||
|
updatePrInfo: vi.fn(),
|
||||||
|
getTrackedPrs: vi.fn().mockReturnValue(new Map()),
|
||||||
|
stopAll: vi.fn(),
|
||||||
|
drainComments: vi.fn().mockReturnValue(mockComments),
|
||||||
|
} as unknown as PrMonitor;
|
||||||
|
|
||||||
|
const onClosedPrFeedback = vi.fn().mockResolvedValue(undefined);
|
||||||
|
const store = createMockStore();
|
||||||
|
new Scheduler(store, { prMonitor, onClosedPrFeedback });
|
||||||
|
|
||||||
|
const onCalls = (store.on as any).mock.calls;
|
||||||
|
const movedHandler = onCalls.find((call: any) => call[0] === "task:moved")?.[1];
|
||||||
|
|
||||||
|
const task = createMockTask({
|
||||||
|
id: "FN-001",
|
||||||
|
column: "done",
|
||||||
|
prInfo: { status: "merged", number: 42 } as any,
|
||||||
|
});
|
||||||
|
|
||||||
|
movedHandler({ task, from: "in-review", to: "done" });
|
||||||
|
|
||||||
|
// Wait for the void Promise.resolve chain to complete
|
||||||
|
await new Promise((r) => setTimeout(r, 10));
|
||||||
|
|
||||||
|
expect(prMonitor.drainComments).toHaveBeenCalledWith("FN-001");
|
||||||
|
expect(onClosedPrFeedback).toHaveBeenCalledWith("FN-001", task.prInfo, mockComments);
|
||||||
|
expect(prMonitor.stopMonitoring).toHaveBeenCalledWith("FN-001");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not invoke onClosedPrFeedback when buffer is empty", async () => {
|
||||||
|
const prMonitor = {
|
||||||
|
startMonitoring: vi.fn(),
|
||||||
|
stopMonitoring: vi.fn(),
|
||||||
|
updatePrInfo: vi.fn(),
|
||||||
|
getTrackedPrs: vi.fn().mockReturnValue(new Map()),
|
||||||
|
stopAll: vi.fn(),
|
||||||
|
drainComments: vi.fn().mockReturnValue([]),
|
||||||
|
} as unknown as PrMonitor;
|
||||||
|
|
||||||
|
const onClosedPrFeedback = vi.fn();
|
||||||
|
const store = createMockStore();
|
||||||
|
new Scheduler(store, { prMonitor, onClosedPrFeedback });
|
||||||
|
|
||||||
|
const onCalls = (store.on as any).mock.calls;
|
||||||
|
const movedHandler = onCalls.find((call: any) => call[0] === "task:moved")?.[1];
|
||||||
|
|
||||||
|
const task = createMockTask({
|
||||||
|
id: "FN-001",
|
||||||
|
column: "done",
|
||||||
|
prInfo: { status: "merged", number: 42 } as any,
|
||||||
|
});
|
||||||
|
|
||||||
|
movedHandler({ task, from: "in-review", to: "done" });
|
||||||
|
|
||||||
|
await new Promise((r) => setTimeout(r, 10));
|
||||||
|
|
||||||
|
expect(prMonitor.drainComments).toHaveBeenCalledWith("FN-001");
|
||||||
|
expect(onClosedPrFeedback).not.toHaveBeenCalled();
|
||||||
|
expect(prMonitor.stopMonitoring).toHaveBeenCalledWith("FN-001");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not invoke onClosedPrFeedback for open PR", async () => {
|
||||||
|
const prMonitor = {
|
||||||
|
startMonitoring: vi.fn(),
|
||||||
|
stopMonitoring: vi.fn(),
|
||||||
|
updatePrInfo: vi.fn(),
|
||||||
|
getTrackedPrs: vi.fn().mockReturnValue(new Map()),
|
||||||
|
stopAll: vi.fn(),
|
||||||
|
drainComments: vi.fn(),
|
||||||
|
} as unknown as PrMonitor;
|
||||||
|
|
||||||
|
const onClosedPrFeedback = vi.fn();
|
||||||
|
const store = createMockStore();
|
||||||
|
new Scheduler(store, { prMonitor, onClosedPrFeedback });
|
||||||
|
|
||||||
|
const onCalls = (store.on as any).mock.calls;
|
||||||
|
const movedHandler = onCalls.find((call: any) => call[0] === "task:moved")?.[1];
|
||||||
|
|
||||||
|
const task = createMockTask({
|
||||||
|
id: "FN-001",
|
||||||
|
column: "done",
|
||||||
|
prInfo: { status: "open", number: 42 } as any,
|
||||||
|
});
|
||||||
|
|
||||||
|
movedHandler({ task, from: "in-review", to: "done" });
|
||||||
|
|
||||||
|
await new Promise((r) => setTimeout(r, 10));
|
||||||
|
|
||||||
|
expect(prMonitor.drainComments).not.toHaveBeenCalled();
|
||||||
|
expect(onClosedPrFeedback).not.toHaveBeenCalled();
|
||||||
|
expect(prMonitor.stopMonitoring).toHaveBeenCalledWith("FN-001");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("does not invoke onClosedPrFeedback when callback is not provided", async () => {
|
||||||
|
const prMonitor = {
|
||||||
|
startMonitoring: vi.fn(),
|
||||||
|
stopMonitoring: vi.fn(),
|
||||||
|
updatePrInfo: vi.fn(),
|
||||||
|
getTrackedPrs: vi.fn().mockReturnValue(new Map()),
|
||||||
|
stopAll: vi.fn(),
|
||||||
|
drainComments: vi.fn().mockReturnValue([
|
||||||
|
{ id: 1, body: "Fix", user: { login: "r" }, created_at: "2024-01-01", updated_at: "2024-01-01", html_url: "" },
|
||||||
|
]),
|
||||||
|
} as unknown as PrMonitor;
|
||||||
|
|
||||||
|
// No onClosedPrFeedback provided
|
||||||
|
const store = createMockStore();
|
||||||
|
new Scheduler(store, { prMonitor });
|
||||||
|
|
||||||
|
const onCalls = (store.on as any).mock.calls;
|
||||||
|
const movedHandler = onCalls.find((call: any) => call[0] === "task:moved")?.[1];
|
||||||
|
|
||||||
|
const task = createMockTask({
|
||||||
|
id: "FN-001",
|
||||||
|
column: "done",
|
||||||
|
prInfo: { status: "closed", number: 42 } as any,
|
||||||
|
});
|
||||||
|
|
||||||
|
// Should not throw
|
||||||
|
movedHandler({ task, from: "in-review", to: "done" });
|
||||||
|
|
||||||
|
await new Promise((r) => setTimeout(r, 10));
|
||||||
|
|
||||||
|
expect(prMonitor.drainComments).toHaveBeenCalledWith("FN-001");
|
||||||
|
expect(prMonitor.stopMonitoring).toHaveBeenCalledWith("FN-001");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("drains comments before stopping monitoring (order matters)", async () => {
|
||||||
|
const callOrder: string[] = [];
|
||||||
|
const mockComments = [
|
||||||
|
{ id: 1, body: "Fix this", user: { login: "reviewer" }, created_at: "2024-01-01", updated_at: "2024-01-01", html_url: "" },
|
||||||
|
];
|
||||||
|
|
||||||
|
const prMonitor = {
|
||||||
|
startMonitoring: vi.fn(),
|
||||||
|
stopMonitoring: vi.fn(() => { callOrder.push("stopMonitoring"); }),
|
||||||
|
updatePrInfo: vi.fn(),
|
||||||
|
getTrackedPrs: vi.fn().mockReturnValue(new Map()),
|
||||||
|
stopAll: vi.fn(),
|
||||||
|
drainComments: vi.fn(() => { callOrder.push("drainComments"); return mockComments; }),
|
||||||
|
} as unknown as PrMonitor;
|
||||||
|
|
||||||
|
const onClosedPrFeedback = vi.fn().mockResolvedValue(undefined);
|
||||||
|
const store = createMockStore();
|
||||||
|
new Scheduler(store, { prMonitor, onClosedPrFeedback });
|
||||||
|
|
||||||
|
const onCalls = (store.on as any).mock.calls;
|
||||||
|
const movedHandler = onCalls.find((call: any) => call[0] === "task:moved")?.[1];
|
||||||
|
|
||||||
|
const task = createMockTask({
|
||||||
|
id: "FN-001",
|
||||||
|
column: "done",
|
||||||
|
prInfo: { status: "merged", number: 42 } as any,
|
||||||
|
});
|
||||||
|
|
||||||
|
movedHandler({ task, from: "in-review", to: "done" });
|
||||||
|
|
||||||
|
await new Promise((r) => setTimeout(r, 10));
|
||||||
|
|
||||||
|
// drainComments should be called before stopMonitoring
|
||||||
|
expect(callOrder).toEqual(["drainComments", "stopMonitoring"]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("second move event with empty drain does not create duplicate follow-up", async () => {
|
||||||
|
const prMonitor = {
|
||||||
|
startMonitoring: vi.fn(),
|
||||||
|
stopMonitoring: vi.fn(),
|
||||||
|
updatePrInfo: vi.fn(),
|
||||||
|
getTrackedPrs: vi.fn().mockReturnValue(new Map()),
|
||||||
|
stopAll: vi.fn(),
|
||||||
|
drainComments: vi.fn().mockReturnValue([]),
|
||||||
|
} as unknown as PrMonitor;
|
||||||
|
|
||||||
|
const onClosedPrFeedback = vi.fn();
|
||||||
|
const store = createMockStore();
|
||||||
|
new Scheduler(store, { prMonitor, onClosedPrFeedback });
|
||||||
|
|
||||||
|
const onCalls = (store.on as any).mock.calls;
|
||||||
|
const movedHandler = onCalls.find((call: any) => call[0] === "task:moved")?.[1];
|
||||||
|
|
||||||
|
const task = createMockTask({
|
||||||
|
id: "FN-001",
|
||||||
|
column: "done",
|
||||||
|
prInfo: { status: "merged", number: 42 } as any,
|
||||||
|
});
|
||||||
|
|
||||||
|
// First move — comments were already drained, buffer is empty
|
||||||
|
movedHandler({ task, from: "in-review", to: "done" });
|
||||||
|
await new Promise((r) => setTimeout(r, 10));
|
||||||
|
|
||||||
|
// Second move — still empty
|
||||||
|
movedHandler({ task, from: "in-review", to: "done" });
|
||||||
|
await new Promise((r) => setTimeout(r, 10));
|
||||||
|
|
||||||
|
// onClosedPrFeedback should never be called since buffer is empty
|
||||||
|
expect(onClosedPrFeedback).not.toHaveBeenCalled();
|
||||||
|
});
|
||||||
});
|
});
|
||||||
|
|
||||||
describe("mission integration", () => {
|
describe("mission integration", () => {
|
||||||
|
|||||||
@@ -1,10 +1,10 @@
|
|||||||
import { resolveDependencyOrder, type TaskStore, type Task, type MissionStore } from "@fusion/core";
|
import { resolveDependencyOrder, type TaskStore, type Task, type MissionStore, type PrInfo } from "@fusion/core";
|
||||||
import { existsSync } from "node:fs";
|
import { existsSync } from "node:fs";
|
||||||
import { readFile } from "node:fs/promises";
|
import { readFile } from "node:fs/promises";
|
||||||
import { join } from "node:path";
|
import { join } from "node:path";
|
||||||
import type { AgentSemaphore } from "./concurrency.js";
|
import type { AgentSemaphore } from "./concurrency.js";
|
||||||
import { schedulerLog } from "./logger.js";
|
import { schedulerLog } from "./logger.js";
|
||||||
import type { PrMonitor } from "./pr-monitor.js";
|
import { type PrMonitor, type PrComment } from "./pr-monitor.js";
|
||||||
import { getCurrentGitHubRepo } from "./github.js";
|
import { getCurrentGitHubRepo } from "./github.js";
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -62,6 +62,17 @@ export interface SchedulerOptions {
|
|||||||
prMonitor?: PrMonitor;
|
prMonitor?: PrMonitor;
|
||||||
/** Optional MissionStore for slice activation and auto-advance */
|
/** Optional MissionStore for slice activation and auto-advance */
|
||||||
missionStore?: MissionStore;
|
missionStore?: MissionStore;
|
||||||
|
/**
|
||||||
|
* Called when a task with a closed/merged PR moves out of in-review
|
||||||
|
* and the PrMonitor has buffered actionable comments.
|
||||||
|
* The callback receives the task ID, PR info, and the drained comments.
|
||||||
|
* If no comments were buffered, this callback is NOT invoked.
|
||||||
|
*/
|
||||||
|
onClosedPrFeedback?: (
|
||||||
|
taskId: string,
|
||||||
|
prInfo: PrInfo,
|
||||||
|
comments: PrComment[]
|
||||||
|
) => void | Promise<void>;
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@@ -150,13 +161,23 @@ export class Scheduler {
|
|||||||
this.options.prMonitor.startMonitoring(task.id, repo.owner, repo.repo, task.prInfo);
|
this.options.prMonitor.startMonitoring(task.id, repo.owner, repo.repo, task.prInfo);
|
||||||
}
|
}
|
||||||
} else if (from === "in-review" && to !== "in-review") {
|
} else if (from === "in-review" && to !== "in-review") {
|
||||||
|
// If task has a closed/merged PR, drain buffered comments before
|
||||||
|
// stopping monitoring (drainComments needs the tracked PR to still exist)
|
||||||
|
if (task.prInfo && (task.prInfo.status === "closed" || task.prInfo.status === "merged")) {
|
||||||
|
const comments = this.options.prMonitor.drainComments(task.id);
|
||||||
|
if (comments.length > 0 && this.options.onClosedPrFeedback) {
|
||||||
|
void Promise.resolve(this.options.onClosedPrFeedback(task.id, task.prInfo, comments))
|
||||||
|
.then(() => {
|
||||||
|
schedulerLog.log(`Invoked onClosedPrFeedback for ${task.id} with ${comments.length} comment(s)`);
|
||||||
|
})
|
||||||
|
.catch((err) => {
|
||||||
|
schedulerLog.error(`Error in onClosedPrFeedback for ${task.id}:`, err);
|
||||||
|
});
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
// Task moved out of in-review, stop monitoring
|
// Task moved out of in-review, stop monitoring
|
||||||
this.options.prMonitor.stopMonitoring(task.id);
|
this.options.prMonitor.stopMonitoring(task.id);
|
||||||
|
|
||||||
// If task has a closed/merged PR, check for unaddressed feedback
|
|
||||||
if (task.prInfo && (task.prInfo.status === "closed" || task.prInfo.status === "merged")) {
|
|
||||||
// This would need the tracked PR data - handled by PrMonitor/PrCommentHandler
|
|
||||||
}
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user