fix(engine): respect autoMerge and mergeStrategy in recover-mergeable-review sweep
The periodic maintenance job `recover-mergeable-review` was silently merging in-review tasks regardless of `autoMerge` and `mergeStrategy` settings, defeating the PR-based review flow for users with `autoMerge: false` and `mergeStrategy: "pull-request"`. Gate the sweep on `settings.autoMerge` (and globalPause/enginePaused for consistency with other merge entry points) and route through the engine's merge queue via the existing `enqueueMerge` callback so `mergeStrategy === "pull-request"` is honored. Falls back to the direct `store.mergeTask` path only when no enqueue callback is wired (standalone/tests). Closes https://github.com/Runfusion/Fusion/issues/21 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
13
.changeset/recover-mergeable-respects-merge-settings.md
Normal file
13
.changeset/recover-mergeable-respects-merge-settings.md
Normal file
@@ -0,0 +1,13 @@
|
|||||||
|
---
|
||||||
|
"@runfusion/fusion": patch
|
||||||
|
"runfusion.ai": patch
|
||||||
|
"@fusion/core": patch
|
||||||
|
"@fusion/dashboard": patch
|
||||||
|
"@fusion/desktop": patch
|
||||||
|
"@fusion/engine": patch
|
||||||
|
"@fusion/mobile": patch
|
||||||
|
"@fusion/pi-claude-cli": patch
|
||||||
|
"@fusion/plugin-sdk": patch
|
||||||
|
---
|
||||||
|
|
||||||
|
Fix [#21](https://github.com/Runfusion/Fusion/issues/21): the `recover-mergeable-review` maintenance sweep no longer bypasses `autoMerge` and `mergeStrategy`. The sweep now early-returns when `autoMerge !== true` (or when the engine is paused) and routes recovery merges through the engine's merge queue so `mergeStrategy: "pull-request"` is honored — eligible in-review tasks go through `processPullRequestMerge` instead of a raw local `git merge`. Operators using a PR-based review flow with `autoMerge: false` will no longer have tasks silently merged behind their back.
|
||||||
@@ -1518,6 +1518,11 @@ describe("SelfHealingManager", () => {
|
|||||||
const managerWithRecovery = new SelfHealingManager(store, {
|
const managerWithRecovery = new SelfHealingManager(store, {
|
||||||
rootDir: "/tmp/test-project",
|
rootDir: "/tmp/test-project",
|
||||||
});
|
});
|
||||||
|
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
|
||||||
|
autoMerge: true,
|
||||||
|
globalPause: false,
|
||||||
|
enginePaused: false,
|
||||||
|
});
|
||||||
|
|
||||||
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||||
{
|
{
|
||||||
@@ -1546,10 +1551,92 @@ describe("SelfHealingManager", () => {
|
|||||||
managerWithRecovery.stop();
|
managerWithRecovery.stop();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("routes through enqueueMerge when wired so mergeStrategy is honored", async () => {
|
||||||
|
const enqueueMerge = vi.fn();
|
||||||
|
const managerWithRecovery = new SelfHealingManager(store, {
|
||||||
|
rootDir: "/tmp/test-project",
|
||||||
|
enqueueMerge,
|
||||||
|
});
|
||||||
|
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
|
||||||
|
autoMerge: true,
|
||||||
|
globalPause: false,
|
||||||
|
enginePaused: false,
|
||||||
|
});
|
||||||
|
|
||||||
|
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||||
|
{
|
||||||
|
id: "FN-352-pr",
|
||||||
|
column: "in-review",
|
||||||
|
paused: false,
|
||||||
|
status: null,
|
||||||
|
error: null,
|
||||||
|
worktree: "/tmp/test-project/.worktrees/fn-352-pr",
|
||||||
|
steps: [{ name: "Ship it", status: "done" }],
|
||||||
|
workflowStepResults: [{ id: "ws-1", status: "passed", phase: "pre-merge" }],
|
||||||
|
mergeDetails: undefined,
|
||||||
|
log: [],
|
||||||
|
},
|
||||||
|
]);
|
||||||
|
|
||||||
|
const result = await managerWithRecovery.recoverMergeableReviewTasks();
|
||||||
|
|
||||||
|
expect(result).toBe(1);
|
||||||
|
expect(enqueueMerge).toHaveBeenCalledWith("FN-352-pr");
|
||||||
|
expect(store.mergeTask).not.toHaveBeenCalled();
|
||||||
|
|
||||||
|
managerWithRecovery.stop();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("skips entirely when autoMerge is disabled (respects PR-based review flow)", async () => {
|
||||||
|
const enqueueMerge = vi.fn();
|
||||||
|
const managerWithRecovery = new SelfHealingManager(store, {
|
||||||
|
rootDir: "/tmp/test-project",
|
||||||
|
enqueueMerge,
|
||||||
|
});
|
||||||
|
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
|
||||||
|
autoMerge: false,
|
||||||
|
globalPause: false,
|
||||||
|
enginePaused: false,
|
||||||
|
});
|
||||||
|
|
||||||
|
const result = await managerWithRecovery.recoverMergeableReviewTasks();
|
||||||
|
|
||||||
|
expect(result).toBe(0);
|
||||||
|
expect(store.listTasks).not.toHaveBeenCalled();
|
||||||
|
expect(store.mergeTask).not.toHaveBeenCalled();
|
||||||
|
expect(enqueueMerge).not.toHaveBeenCalled();
|
||||||
|
|
||||||
|
managerWithRecovery.stop();
|
||||||
|
});
|
||||||
|
|
||||||
|
it("skips when globalPause or enginePaused is set", async () => {
|
||||||
|
const managerWithRecovery = new SelfHealingManager(store, {
|
||||||
|
rootDir: "/tmp/test-project",
|
||||||
|
});
|
||||||
|
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
|
||||||
|
autoMerge: true,
|
||||||
|
globalPause: true,
|
||||||
|
enginePaused: false,
|
||||||
|
});
|
||||||
|
|
||||||
|
const result = await managerWithRecovery.recoverMergeableReviewTasks();
|
||||||
|
|
||||||
|
expect(result).toBe(0);
|
||||||
|
expect(store.listTasks).not.toHaveBeenCalled();
|
||||||
|
expect(store.mergeTask).not.toHaveBeenCalled();
|
||||||
|
|
||||||
|
managerWithRecovery.stop();
|
||||||
|
});
|
||||||
|
|
||||||
it("skips paused in-review tasks even when otherwise mergeable", async () => {
|
it("skips paused in-review tasks even when otherwise mergeable", async () => {
|
||||||
const managerWithRecovery = new SelfHealingManager(store, {
|
const managerWithRecovery = new SelfHealingManager(store, {
|
||||||
rootDir: "/tmp/test-project",
|
rootDir: "/tmp/test-project",
|
||||||
});
|
});
|
||||||
|
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
|
||||||
|
autoMerge: true,
|
||||||
|
globalPause: false,
|
||||||
|
enginePaused: false,
|
||||||
|
});
|
||||||
|
|
||||||
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||||
{
|
{
|
||||||
@@ -1578,6 +1665,11 @@ describe("SelfHealingManager", () => {
|
|||||||
const managerWithRecovery = new SelfHealingManager(store, {
|
const managerWithRecovery = new SelfHealingManager(store, {
|
||||||
rootDir: "/tmp/test-project",
|
rootDir: "/tmp/test-project",
|
||||||
});
|
});
|
||||||
|
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
|
||||||
|
autoMerge: true,
|
||||||
|
globalPause: false,
|
||||||
|
enginePaused: false,
|
||||||
|
});
|
||||||
|
|
||||||
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||||
{
|
{
|
||||||
|
|||||||
@@ -779,6 +779,14 @@ export class SelfHealingManager {
|
|||||||
*/
|
*/
|
||||||
async recoverMergeableReviewTasks(): Promise<number> {
|
async recoverMergeableReviewTasks(): Promise<number> {
|
||||||
try {
|
try {
|
||||||
|
// Respect user merge intent. Without these gates the sweep would
|
||||||
|
// silently merge tasks even when the operator has opted into a
|
||||||
|
// PR-based review flow (`autoMerge: false`, `mergeStrategy:
|
||||||
|
// "pull-request"`) — see GitHub issue #21.
|
||||||
|
const settings = await this.store.getSettings();
|
||||||
|
if (!settings.autoMerge) return 0;
|
||||||
|
if (settings.globalPause || settings.enginePaused) return 0;
|
||||||
|
|
||||||
const tasks = await this.store.listTasks({ column: "in-review", slim: true });
|
const tasks = await this.store.listTasks({ column: "in-review", slim: true });
|
||||||
|
|
||||||
const mergeable = tasks.filter((t) =>
|
const mergeable = tasks.filter((t) =>
|
||||||
@@ -793,15 +801,25 @@ export class SelfHealingManager {
|
|||||||
|
|
||||||
log.warn(`Found ${mergeable.length} mergeable review task(s) stuck in in-review`);
|
log.warn(`Found ${mergeable.length} mergeable review task(s) stuck in in-review`);
|
||||||
|
|
||||||
|
// Prefer the engine's merge queue so `mergeStrategy` (direct vs.
|
||||||
|
// pull-request) is honored. Fall back to a direct store merge only
|
||||||
|
// when no enqueue callback is wired (standalone/tests).
|
||||||
|
const enqueueMerge = this.options.enqueueMerge;
|
||||||
let recovered = 0;
|
let recovered = 0;
|
||||||
for (const task of mergeable) {
|
for (const task of mergeable) {
|
||||||
try {
|
try {
|
||||||
await this.store.mergeTask(task.id);
|
if (enqueueMerge) {
|
||||||
|
enqueueMerge(task.id);
|
||||||
|
} else {
|
||||||
|
await this.store.mergeTask(task.id);
|
||||||
|
}
|
||||||
await this.store.logEntry(
|
await this.store.logEntry(
|
||||||
task.id,
|
task.id,
|
||||||
"Auto-recovered: eligible in-review task was merged and moved to done",
|
enqueueMerge
|
||||||
|
? "Auto-recovered: eligible in-review task re-enqueued for merge"
|
||||||
|
: "Auto-recovered: eligible in-review task was merged and moved to done",
|
||||||
);
|
);
|
||||||
log.log(`Recovered mergeable review task ${task.id}: merged to done`);
|
log.log(`Recovered mergeable review task ${task.id}`);
|
||||||
recovered++;
|
recovered++;
|
||||||
} catch (err: unknown) { const errorMessage = err instanceof Error ? err.message : String(err);
|
} catch (err: unknown) { const errorMessage = err instanceof Error ? err.message : String(err);
|
||||||
log.error(`Failed to recover mergeable review task ${task.id}: ${errorMessage}`);
|
log.error(`Failed to recover mergeable review task ${task.id}: ${errorMessage}`);
|
||||||
|
|||||||
Reference in New Issue
Block a user