diff --git a/.changeset/fn-7692-merger-confirmation-copy.md b/.changeset/fn-7692-merger-confirmation-copy.md new file mode 100644 index 0000000000..0954b30faa --- /dev/null +++ b/.changeset/fn-7692-merger-confirmation-copy.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fix misleading merger "awaiting-confirmation" copy that claimed a hard block when auto-merge advances the merge automatically. +category: fix +dev: `decidePlannerRecovery` now accepts an additive `autoMergeWillProceed` flag (threaded from `allowsAutoMergeProcessing` in `PlannerRecoveryController.tick`) that only shapes the confirmation `reason` string; no gating/behavior change to `action`/`requiresConfirmation`/`sideEffectClass`. diff --git a/docs/architecture.md b/docs/architecture.md index f49588f7dc..39d39d8c7a 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1510,8 +1510,14 @@ other rule (level gate, attempt bound, exhaustion) is unchanged. transitions. `ProjectEngine` wires the concrete handlers in `buildPlannerRecoveryHandlers`: `requestConfirmation` -posts a `[planner-oversight] confirmation required (...)` steering comment (reusing the same -`addSteeringComment` channel as bounded recovery, so a human sees it). `executeMergePrAction` branches +posts a `[planner-oversight] merge checkpoint (...)` steering comment (reusing the same +`addSteeringComment` channel as bounded recovery, so a human sees it). FN-7692: the wrapper prefix is +deliberately neutral ("checkpoint", not "confirmation required") because the trailing `reason` string +accurately states whether the merge/PR will advance automatically under the active auto-merge policy +(advisory) or genuinely awaits a human approval (blocking) — `decidePlannerRecovery`'s additive +`autoMergeWillProceed` input, threaded from the existing `allowsAutoMergeProcessing` predicate, selects +the accurate wording; this is messaging-only and does not change `action`/`requiresConfirmation`/ +`sideEffectClass`. `executeMergePrAction` branches on `request.proposedAction` (falling back to `request.watchedStage` defensively) rather than treating every approved `"merge_pr"` request identically: ONLY `"advance_merge"` (the `merger` stage) reuses the EXISTING `store.mergeTask(taskId)` merge mechanism; `"advance_pull_request"` (the `pull-request` stage) diff --git a/packages/core/src/__tests__/planner-recovery.test.ts b/packages/core/src/__tests__/planner-recovery.test.ts index a635a588b0..5e351e9f1d 100644 --- a/packages/core/src/__tests__/planner-recovery.test.ts +++ b/packages/core/src/__tests__/planner-recovery.test.ts @@ -103,6 +103,72 @@ describe("decidePlannerRecovery", () => { } }); + // FN-7692: the `reason` copy must accurately reflect whether the active + // auto-merge policy will advance the merge/PR unattended (advisory, + // non-blocking wording) or genuinely requires a human approval (blocking + // wording) — never the old unconditional "requires explicit confirmation + // before ... may run" claim, which was false whenever auto-merge would + // proceed on its own (observed on FN-7689). `action`/`requiresConfirmation`/ + // `sideEffectClass` must stay byte-for-byte identical across all three + // `autoMergeWillProceed` states — this is messaging-only. + it("produces accurate advisory copy when auto-merge will proceed unattended, for both merger and pull-request stages", () => { + for (const stage of ["merger", "pull-request"] as const) { + const decision = decidePlannerRecovery({ + snapshot: observation({ stage, signal: "failed" }), + autoMergeWillProceed: true, + }); + expect(decision.action, `stage=${stage}`).toBe("await_confirmation"); + expect(decision.requiresConfirmation, `stage=${stage}`).toBe(true); + expect(decision.sideEffectClass, `stage=${stage}`).toBe("merge_pr"); + expect(decision.reason, `stage=${stage}`).not.toMatch(/requires explicit confirmation before .* may run/); + expect(decision.reason, `stage=${stage}`).toMatch(/automatically/i); + expect(decision.reason, `stage=${stage}`).toMatch(/advisory/i); + } + }); + + it("produces accurate blocking copy when auto-merge will NOT proceed unattended, for both merger and pull-request stages", () => { + for (const stage of ["merger", "pull-request"] as const) { + const decision = decidePlannerRecovery({ + snapshot: observation({ stage, signal: "failed" }), + autoMergeWillProceed: false, + }); + expect(decision.action, `stage=${stage}`).toBe("await_confirmation"); + expect(decision.requiresConfirmation, `stage=${stage}`).toBe(true); + expect(decision.sideEffectClass, `stage=${stage}`).toBe("merge_pr"); + expect(decision.reason, `stage=${stage}`).toMatch(/will not .* until a human explicitly approves/); + expect(decision.reason, `stage=${stage}`).not.toMatch(/advisory/i); + } + }); + + it("uses neutral, non-overclaiming copy when the auto-merge policy is unknown to the caller", () => { + for (const stage of ["merger", "pull-request"] as const) { + const decision = decidePlannerRecovery({ snapshot: observation({ stage, signal: "failed" }) }); + expect(decision.reason, `stage=${stage}`).not.toMatch(/requires explicit confirmation before .* may run/); + expect(decision.reason, `stage=${stage}`).not.toMatch(/automatically/i); + expect(decision.reason, `stage=${stage}`).not.toMatch(/will not .* until a human explicitly approves/); + } + }); + + it("keeps action/requiresConfirmation/sideEffectClass byte-for-byte unchanged across autoMergeWillProceed states (messaging-only guard)", () => { + for (const stage of ["merger", "pull-request"] as const) { + const base = decidePlannerRecovery({ snapshot: observation({ stage, signal: "failed" }) }); + const proceeds = decidePlannerRecovery({ + snapshot: observation({ stage, signal: "failed" }), + autoMergeWillProceed: true, + }); + const blocks = decidePlannerRecovery({ + snapshot: observation({ stage, signal: "failed" }), + autoMergeWillProceed: false, + }); + for (const decision of [base, proceeds, blocks]) { + expect(decision.action, `stage=${stage}`).toBe("await_confirmation"); + expect(decision.requiresConfirmation, `stage=${stage}`).toBe(true); + expect(decision.sideEffectClass, `stage=${stage}`).toBe("merge_pr"); + expect(decision.proposedAction, `stage=${stage}`).toBe(base.proposedAction); + } + } + }); + it("keeps requiresConfirmation false for bounded-recovery decisions", () => { const decision = decidePlannerRecovery({ snapshot: observation({ stage: "executor", signal: "failed" }) }); expect(decision.requiresConfirmation).toBe(false); diff --git a/packages/core/src/planner-recovery.ts b/packages/core/src/planner-recovery.ts index 426c2b4904..304da7e664 100644 --- a/packages/core/src/planner-recovery.ts +++ b/packages/core/src/planner-recovery.ts @@ -120,6 +120,24 @@ export interface DecidePlannerRecoveryInput { snapshot: PlannerRecoveryObservation | null | undefined; /** Current attempt state for this `(taskId, watchedStage)`; omit for a fresh stage. */ attemptState?: PlannerRecoveryAttemptState; + /** + * FNXC:PlannerOversight 2026-07-08-00:00: + * FN-7692 requirement: additive, messaging-only signal for whether the + * active auto-merge policy will advance a `merger`/`pull-request` + * `"await_confirmation"` decision WITHOUT a human clicking approve + * (`allowsAutoMergeProcessing(task, settings)` from `@fusion/core`'s + * `task-merge.ts`, computed by the caller). This ONLY shapes the `reason` + * string built for that branch below — it must never influence `action`, + * `requiresConfirmation`, `sideEffectClass`, `proposedAction`, attempt + * accounting, or any other decision field. `true` = auto-merge will + * proceed unattended (the confirmation is advisory, not a real block); + * `false` = the stage genuinely will not advance without human approval; + * `undefined` = policy unknown to the caller — use neutral, non- + * overclaiming wording that asserts neither outcome. Fixes FN-7689's + * misleading "requires explicit confirmation" copy shown even though + * auto-merge proceeded unattended a few minutes later. + */ + autoMergeWillProceed?: boolean; } /** @@ -213,9 +231,25 @@ export function decidePlannerRecovery(input: DecidePlannerRecoveryInput): Planne if (snapshot.stage === "merger" || snapshot.stage === "pull-request") { const proposedAction = snapshot.stage === "merger" ? "advance_merge" : "advance_pull_request"; const sideEffectClass = classifyPlannerActionSideEffect({ watchedStage: snapshot.stage, proposedAction }); + const actionPhrase = proposedAction.replace(/_/g, " "); + // FNXC:PlannerOversight 2026-07-08-00:00: + // FN-7692 fix: this `reason` string previously read "requires explicit + // confirmation before ... may run" UNCONDITIONALLY, which is false when + // the active auto-merge policy will advance the merge/PR unattended + // (observed on FN-7689: the Intervention Timeline claimed a hard block + // that the merge sailed past ~4 minutes later with no human approval). + // `input.autoMergeWillProceed` (threaded by the engine controller from + // the existing `allowsAutoMergeProcessing` predicate) selects accurate + // wording per policy state; it is messaging-only and does not change + // `action`/`requiresConfirmation`/`sideEffectClass` below. + const reason = input.autoMergeWillProceed === true + ? `Stage "${snapshot.stage}" will ${actionPhrase} automatically under the active auto-merge policy — this confirmation checkpoint is advisory and does not block progress` + : input.autoMergeWillProceed === false + ? `Stage "${snapshot.stage}" will not ${actionPhrase} until a human explicitly approves — auto-merge is not enabled for this task` + : `Stage "${snapshot.stage}" is awaiting confirmation before ${actionPhrase} may run`; return { action: "await_confirmation", - reason: `Stage "${snapshot.stage}" requires explicit confirmation before ${proposedAction.replace(/_/g, " ")} may run`, + reason, attemptCount, attemptLimit, exhausted: false, diff --git a/packages/engine/src/__tests__/planner-overseer-intervention-wiring.test.ts b/packages/engine/src/__tests__/planner-overseer-intervention-wiring.test.ts index 357b516cfd..5f782b1e1c 100644 --- a/packages/engine/src/__tests__/planner-overseer-intervention-wiring.test.ts +++ b/packages/engine/src/__tests__/planner-overseer-intervention-wiring.test.ts @@ -224,6 +224,43 @@ describe("FN-7551 — overseer decision points populate the intervention timelin expect(confirmationEntries.some((e) => e.outcome === "succeeded")).toBe(true); }); + // FN-7692: the recorded `overseer:intervention` reason for a merger/ + // pull-request confirmation must accurately reflect whether auto-merge will + // proceed unattended (advisory copy) or genuinely requires a human approval + // (blocking copy) — reproducing the FN-7689 scenario where the timeline + // claimed a hard block that the merge sailed past unattended. A pending + // confirmation must still be recorded either way (no dispatch change). + it("records accurate advisory copy (not a false hard-block claim) when ctx.settings.autoMerge is truthy for an in-review merger task", async () => { + const task = await seedTask("in-review"); + const { monitor, controllerFromMonitor: controller } = wireRealEngineOverseer(store); + await monitor.observeTask(task, "autonomous"); // merger stage (plain in-review, no PR/reviewState) + + const decision = await controller.tick(task, { settings: { autoMerge: true } }); + expect(decision?.requiresConfirmation).toBe(true); + expect(decision?.action).toBe("await_confirmation"); + + const timeline = getPlannerInterventionTimeline(store, task.id); + const requestEntry = timeline.find((e) => e.action === "request-confirmation"); + expect(requestEntry).toBeTruthy(); + expect(requestEntry?.outcome).toBe("awaiting-confirmation"); + expect(requestEntry?.reason).not.toMatch(/requires explicit confirmation before .* may run/); + expect(requestEntry?.reason).toMatch(/automatically/i); + + // A pending confirmation is still recorded — no dispatch, no behavior change. + expect(controller.getPendingConfirmations(task.id)).toHaveLength(1); + }); + + // Note: a genuinely-blocking `autoMergeWillProceed: false` state is + // exercised directly against the pure `decidePlannerRecovery` in + // `planner-recovery.test.ts` (@fusion/core). It is NOT independently + // reachable through this engine's real `tick()` wiring: `allowsAutoMerge + // Processing(task, settings) === false` is exactly the condition + // `evaluateOverseerHumanControl` uses to withhold ALL oversight action + // (including confirmation recording) BEFORE `decidePlannerRecovery` is ever + // called — so a real merger/pull-request confirmation entry can only ever + // be recorded when auto-merge WILL proceed. This is documented here rather + // than asserted redundantly to avoid a test that can never legitimately fail. + it("denying a confirmation resolution emits a 'skipped' outcome entry", async () => { const task = await seedTask("in-review"); const { monitor, controllerFromMonitor: controller } = wireRealEngineOverseer(store); diff --git a/packages/engine/src/planner-recovery-controller.ts b/packages/engine/src/planner-recovery-controller.ts index ccf820c354..6beff92b8c 100644 --- a/packages/engine/src/planner-recovery-controller.ts +++ b/packages/engine/src/planner-recovery-controller.ts @@ -41,7 +41,7 @@ */ import type { PlannerConfirmationRequest, PlannerRecoveryDecision, PlannerRecoveryObservation, Settings, Task } from "@fusion/core"; -import { decidePlannerRecovery, PLANNER_RECOVERY_MAX_ATTEMPTS } from "@fusion/core"; +import { allowsAutoMergeProcessing, decidePlannerRecovery, PLANNER_RECOVERY_MAX_ATTEMPTS } from "@fusion/core"; import { createLogger, type Logger } from "./logger.js"; import type { OverseerStageObservation } from "./planner-overseer.js"; import { @@ -282,9 +282,21 @@ export class PlannerRecoveryController { const attemptCount = this.attempts.get(key) ?? 0; + // FNXC:PlannerOversight 2026-07-08-00:00: + // FN-7692: reuse the existing `allowsAutoMergeProcessing` predicate + // (same fallback semantics as the `evaluateOverseerHumanControl` guard + // above — `ctx.settings ?? { autoMerge: true }`) to tell + // `decidePlannerRecovery` whether the pending merger/pull-request + // confirmation is actually advisory (auto-merge will proceed unattended) + // or a genuine block. Messaging-only: this does not add a settings + // lookup beyond what `ctx.settings` already provides, and does not + // change the confirmation-required gating below. + const autoMergeWillProceed = allowsAutoMergeProcessing(task, ctx.settings ?? { autoMerge: true }); + const decision = decidePlannerRecovery({ snapshot: snapshot as unknown as PlannerRecoveryObservation, attemptState: { attemptCount, attemptLimit: PLANNER_RECOVERY_MAX_ATTEMPTS }, + autoMergeWillProceed, }); if (decision.action === "none") { diff --git a/packages/engine/src/project-engine.ts b/packages/engine/src/project-engine.ts index 60f73efb52..1f95df1ec6 100644 --- a/packages/engine/src/project-engine.ts +++ b/packages/engine/src/project-engine.ts @@ -1415,8 +1415,17 @@ export class ProjectEngine { // so a human sees it; it never performs the side effect itself. The // dashboard confirmation UI/badge that lets a human act on this is // owned by FN-7515+/FN-7517. + // FNXC:PlannerOversight 2026-07-08-00:00: + // FN-7692 fix: this prefix previously read "confirmation required" + // unconditionally, which contradicted `request.reason` once FN-7692 + // made that reason accurately advisory under an active auto-merge + // policy. "checkpoint" is neutral and consistent whether the trailing + // `reason` describes an advisory (auto-merge will proceed) or a + // genuine block (human approval required) — messaging-only, no change + // to the `addSteeringComment` channel/timing or `emitOverseerConfirmation` + // below. requestConfirmation: async (task, request) => { - const text = `[planner-oversight] confirmation required (${request.sideEffectClass}): ${request.reason}`; + const text = `[planner-oversight] merge checkpoint (${request.sideEffectClass}): ${request.reason}`; await store.addSteeringComment(task.id, text, "agent"); this.emitOverseerInterventionSafe(() => emitOverseerConfirmation({