From 2cc84b5177d6d4169519e42a4c0dbf0025ee6c5a Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sat, 4 Jul 2026 13:51:09 -0700 Subject: [PATCH] FN-7513: require planner confirmation for risky side effects Require explicit approval before planner recovery runs merge, PR, destructive, or external-service actions. - Add pure planner side-effect classification and confirmation request modeling in core. - Route merge/PR recovery decisions to await confirmation instead of autonomous dispatch. - Persist pending confirmation requests and only execute approved controller actions. - Cover confirmation gating with core and engine regression tests and document the policy. Files changed: .changeset/fn-7513-planner-confirmation-gate.md | 7 + docs/architecture.md | 85 ++++++++- docs/settings-reference.md | 2 +- .../src/__tests__/planner-confirmation.test.ts | 125 +++++++++++++ .../core/src/__tests__/planner-recovery.test.ts | 14 +- packages/core/src/index.ts | 7 + packages/core/src/planner-confirmation.ts | 141 ++++++++++++++ packages/core/src/planner-recovery.ts | 103 ++++++++--- ...lanner-recovery-controller-confirmation.test.ts | 205 +++++++++++++++++++++ packages/engine/src/index.ts | 5 + packages/engine/src/planner-recovery-controller.ts | 204 +++++++++++++++++++- packages/engine/src/project-engine.ts | 44 +++++ 12 files changed, 913 insertions(+), 29 deletions(-) Fusion-Task-Id: FN-7513 Fusion-Task-Lineage: 1e3c6640-8a4f-41f6-89dd-41eb9b675b2b Co-authored-by: Fusion (runfusion.ai) --- .../fn-7513-planner-confirmation-gate.md | 7 + docs/architecture.md | 85 +++++++- docs/settings-reference.md | 2 +- .../__tests__/planner-confirmation.test.ts | 125 +++++++++++ .../src/__tests__/planner-recovery.test.ts | 14 +- packages/core/src/index.ts | 7 + packages/core/src/planner-confirmation.ts | 141 ++++++++++++ packages/core/src/planner-recovery.ts | 103 +++++++-- ...r-recovery-controller-confirmation.test.ts | 205 ++++++++++++++++++ packages/engine/src/index.ts | 5 + .../engine/src/planner-recovery-controller.ts | 204 ++++++++++++++++- packages/engine/src/project-engine.ts | 44 ++++ 12 files changed, 913 insertions(+), 29 deletions(-) create mode 100644 .changeset/fn-7513-planner-confirmation-gate.md create mode 100644 packages/core/src/__tests__/planner-confirmation.test.ts create mode 100644 packages/core/src/planner-confirmation.ts create mode 100644 packages/engine/src/__tests__/planner-recovery-controller-confirmation.test.ts diff --git a/.changeset/fn-7513-planner-confirmation-gate.md b/.changeset/fn-7513-planner-confirmation-gate.md new file mode 100644 index 0000000000..69b79446a0 --- /dev/null +++ b/.changeset/fn-7513-planner-confirmation-gate.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": minor +--- + +summary: Planner oversight now requires confirmation before merge/PR actions and destructive/external side effects. +category: feature +dev: Adds `PlannerActionSideEffectClass` + `PlannerConfirmationRequest` and `classifyPlannerActionSideEffect`/`requiresPlannerConfirmation` (core), extends `decidePlannerRecovery` with an `await_confirmation` action, and adds `requestConfirmation`/`resolveConfirmation` gating to `PlannerRecoveryController` (engine). Merge/PR and destructive/external actions never execute without a recorded approval; bounded recovery (guidance/retry/targeted-fix) is unchanged. UX rendering, human-control safeguards, timeline, and run-audit land in follow-up tasks. diff --git a/docs/architecture.md b/docs/architecture.md index 2ca84a8ca2..d250ba8ec6 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -1403,8 +1403,9 @@ and the pure, never-throw `decidePlannerRecovery(input)`. Decision rules, in ord 2. The per-`(taskId, watchedStage)` attempt count has reached `PLANNER_RECOVERY_MAX_ATTEMPTS` (default `3`, mirroring `MAX_RECOVERY_RETRIES` in `recovery-policy.ts`) → `"none"`, `exhausted: true` — the layer stops autonomously and the task is left for escalation (FN-7514+ owns the human-control story). -3. `merger` / `pull-request` stages → `"none"` with a deferral reason: these require confirmation and - are owned by FN-7513's confirmation-gated layer, never dispatched from here. +3. `merger` / `pull-request` stages → `"await_confirmation"` (FN-7513) with `requiresConfirmation: true`, + `sideEffectClass: "merge_pr"`: these require confirmation and are surfaced, never dispatched, by the + bounded layer itself — see "Planner overseer confirmation gate (FN-7513)" below. 4. `reviewer` stage → `"inject_guidance"`. 5. `executor` / `workflow-gate` stage with `signal === "failed"` → `"request_targeted_fix"` when a source link carries a specific fixable error (`failed-check` / `merge-error`), else `"retry_step"`. @@ -1440,6 +1441,86 @@ destructive/external-service side effects (FN-7513, confirmation-gated); compreh persisted intervention timeline (FN-7519); run-audit/activity events (FN-7520); and any dashboard UI (FN-7515+). +### Planner overseer confirmation gate (FN-7513) + +/* +FNXC:PlannerOversight 2026-07-04-13:00: +FN-7513 adds the safety gate deciding which FN-7512 recovery-layer actions may run autonomously versus +which must be blocked behind an explicit, recorded human approval. Merge/PR progression (advancing a +merge, promoting a shared branch, retrying/forcing a merge, opening/updating/merging a pull request) and +any destructive or external-service side effect (branch/worktree deletion, force operations, remote +pushes, third-party GitHub/GitLab calls) are classified confirmation-required, regardless of the +effective oversight level. Bounded recovery (inject_guidance / retry_step / request_targeted_fix on +non-merge/PR stages, FN-7512) is unaffected and remains no-confirmation. The invariant: a gated action +NEVER executes without a recorded, approved `PlannerConfirmationRequest`. +*/ + +`packages/core/src/planner-confirmation.ts` declares the classifier vocabulary: + +- **`PlannerActionSideEffectClass`** (`"bounded_recovery" | "merge_pr" | "destructive_external"`). +- **`PlannerConfirmationRequest`** — `{ requestId, taskId, watchedStage, sideEffectClass, proposedAction, + reason, sourceLinks, requestedAt, status: "pending" | "approved" | "denied", resolvedAt?, resolvedBy? }`. + Conceptually mirrors `TaskMergeDetails.mergeConfirmed` (an explicit human approval precedes a side + effect) but is its own record — it never reads or writes `mergeConfirmed`, which stays owned by the + merge dispatch path. +- **`classifyPlannerActionSideEffect({ watchedStage, proposedAction })`** — pure, deterministic, + never-throw. `merger` / `pull-request` stage actions beyond guidance/retry → `"merge_pr"`; an + explicit allow-list of destructive/external action names (branch/worktree delete, force push/merge/ + delete, remote push, GitHub/GitLab/external-service calls, PR open/merge, shared-branch promotion) → + `"destructive_external"` regardless of stage; everything else → `"bounded_recovery"`. Malformed input + or an unrecognized non-bounded action on a non-merge/PR stage fails CLOSED to + `"destructive_external"` rather than silently allowing an unclassified action through. +- **`requiresPlannerConfirmation(sideEffectClass)`** — `true` for `"merge_pr"` / `"destructive_external"`, + `false` for `"bounded_recovery"`. + +`packages/core/src/planner-recovery.ts`'s `decidePlannerRecovery` now calls the classifier for every +branch and returns `requiresConfirmation` / `sideEffectClass` / (for gated decisions) `proposedAction` on +`PlannerRecoveryDecision`. The merger/pull-request branch, previously `"none"`, now returns `action: +"await_confirmation"` naming what would run on approval (`advance_merge` / `advance_pull_request`); every +other rule (level gate, attempt bound, exhaustion) is unchanged. + +`packages/engine/src/planner-recovery-controller.ts`'s `PlannerRecoveryController` adds the gate: + +- A per-`(taskId, watchedStage)` **pending-confirmation registry** (`getPendingConfirmations(taskId)`) + — idempotent: `tick` never creates a second pending request for a stage that already has one pending. +- `requestConfirmation(task, request, ctx)` (optional handler) — records/surfaces the pending request; + it must NOT perform the side effect itself. +- `executeMergePrAction(taskId, request, ctx)` / `executeDestructiveExternalAction(taskId, request, ctx)` + (optional handlers) — invoked ONLY from `resolveConfirmation(..., "approved", ...)`, never from `tick`. +- `resolveConfirmation(taskId, requestId, "approved" | "denied", resolvedBy?)` — on `"approved"`, + dispatches the matching execution handler exactly once and clears the pending request; on `"denied"`, + clears the request with no side effect, leaving the task for other escalation (FN-7514+), AND consumes + one bounded-recovery attempt for that `(taskId, watchedStage)` pair (the same shared + `PLANNER_RECOVERY_MAX_ATTEMPTS` budget `dispatch()` consumes). Without counting denials against the + budget, a denied merge/PR/destructive confirmation would resurface as an identical pending request on + the very next `tick()` forever; counting it means repeated denials eventually exhaust the stage + (`decidePlannerRecovery` then returns `action: "none", exhausted: true`) instead of re-prompting + indefinitely. Never throws — handler rejections are logged and swallowed, and the request is still + cleared. +- `tick(task, ctx)`: when `decidePlannerRecovery` returns `requiresConfirmation: true`, calls + `requestConfirmation` (idempotently) and does NOT invoke any side-effecting handler; bounded-recovery + decisions still dispatch exactly as FN-7512 (with the attempt increment). The `"autonomous"`-only gate + and the `userPaused` skip are preserved. +- `clear(taskId)` also clears pending confirmations (in addition to attempt state) on terminal task + 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 +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) +is intentionally a no-op today because no reusable PR-specific advance mechanism exists yet — an +approved PR confirmation must never fall through to a direct task merge/cleanup, which would bypass the +PR workflow entirely. No `executeDestructiveExternalAction` is wired yet, since FN-7511's observation +model does not currently emit a destructive-action signal; a future task can wire one (and the +PR-specific execution handler) using existing safe helpers when a concrete need arises. + +**Downstream ownership (not this layer):** rendering the pending-confirmation UI/badge (FN-7515+/ +FN-7517), comprehensive human-control safeguards beyond `userPaused` (FN-7514), a persisted intervention +timeline (FN-7519), and run-audit/activity events (FN-7520) all consume the data this gate exposes but +are implemented elsewhere. + --- ## 11) Multi-Project Architecture diff --git a/docs/settings-reference.md b/docs/settings-reference.md index 2cdf5b1d78..2e1b9ff8ed 100644 --- a/docs/settings-reference.md +++ b/docs/settings-reference.md @@ -359,7 +359,7 @@ The built-in workflows also declare triage/spec policy settings that were **not* | `autoApproveSpec` | `false` | Legacy compatibility setting. Workflow Plan Review now owns optional pre-execution AI plan approval. | | `planReviewMaxRevisions` | unset | Workflow-native Plan Review/spec revision cap. Unset/empty means unbounded automatic replans; a non-negative integer caps attempts; `0` disables automatic Plan Review revision. | | `codeReviewMaxRevisions` | unset | Workflow-native Code Review remediation cap. Unset/empty means unbounded automatic code-fix passes; a non-negative integer caps attempts; `0` disables automatic Code Review remediation. | -| `plannerOversightLevel` | `autonomous` | Workflow-native planner oversight mode. `off` disables oversight; `observe` watches only; `steer` injects guidance or suggests revisions; `autonomous` enables bounded retry and targeted-fix recovery. Tasks may set a nullable `Task.plannerOversightLevel` override (same four values) that wins over this workflow value when present; `null`/unset means "inherit the workflow value". `resolveEffectivePlannerOversightLevel` in `@fusion/core` computes the effective level (task override → workflow effective → `autonomous`). Dashboard UI/API threading for the per-task override and engine read-site behavior are follow-up work (FN-7515, FN-7510+). | +| `plannerOversightLevel` | `autonomous` | Workflow-native planner oversight mode. `off` disables oversight; `observe` watches only; `steer` injects guidance or suggests revisions; `autonomous` enables bounded retry and targeted-fix recovery — but merge/PR progression and any destructive or external-service side effect ALWAYS require an explicit, recorded human confirmation before they run, even at `autonomous` (FN-7513's confirmation gate; see `docs/architecture.md` → "Planner overseer confirmation gate"). Tasks may set a nullable `Task.plannerOversightLevel` override (same four values) that wins over this workflow value when present; `null`/unset means "inherit the workflow value". `resolveEffectivePlannerOversightLevel` in `@fusion/core` computes the effective level (task override → workflow effective → `autonomous`). Dashboard UI/API threading for the per-task override and engine read-site behavior are follow-up work (FN-7515, FN-7510+). | When `triageProactiveSubtaskSplittingEnabled` is `true` (the default), triage may proactively replace a large task with 2-5 child tasks when the size, step-count, package breadth, file-scope, or remediation-batch signals justify the coordination overhead. When it is `false`, those automatic oversized-task signals are advisory only for writing a realistic single-task spec; triage must not split solely because the task is large. The per-task `breakIntoSubtasks: true` flag is separate and remains mandatory: if a user explicitly asks for subtask breakdown, triage still evaluates and creates child tasks when the work is meaningfully decomposable. diff --git a/packages/core/src/__tests__/planner-confirmation.test.ts b/packages/core/src/__tests__/planner-confirmation.test.ts new file mode 100644 index 0000000000..3dff149372 --- /dev/null +++ b/packages/core/src/__tests__/planner-confirmation.test.ts @@ -0,0 +1,125 @@ +import { describe, expect, it } from "vitest"; +import { classifyPlannerActionSideEffect, requiresPlannerConfirmation } from "../planner-confirmation.js"; +import { decidePlannerRecovery, type PlannerRecoveryObservation } from "../planner-recovery.js"; + +function observation(overrides: Partial = {}): PlannerRecoveryObservation { + return { + taskId: "FN-1", + stage: "executor", + signal: "progressing", + oversightLevel: "autonomous", + sources: [], + ...overrides, + }; +} + +describe("classifyPlannerActionSideEffect", () => { + it("classifies merger and pull-request stage actions as merge_pr", () => { + for (const stage of ["merger", "pull-request"] as const) { + expect(classifyPlannerActionSideEffect({ watchedStage: stage, proposedAction: "advance_merge" })).toBe("merge_pr"); + expect(classifyPlannerActionSideEffect({ watchedStage: stage, proposedAction: "retry_step" })).toBe("merge_pr"); + } + }); + + it("classifies enumerated destructive/external actions as destructive_external regardless of stage", () => { + const destructiveActions = [ + "delete_branch", + "delete_worktree", + "force_push", + "force_merge", + "force_delete", + "push_remote", + "call_external_service", + "github_api_call", + "gitlab_api_call", + "open_pull_request", + "merge_pull_request", + "promote_shared_branch", + ]; + for (const proposedAction of destructiveActions) { + expect(classifyPlannerActionSideEffect({ watchedStage: "executor", proposedAction })).toBe("destructive_external"); + expect(classifyPlannerActionSideEffect({ watchedStage: null, proposedAction })).toBe("destructive_external"); + } + }); + + it("classifies bounded recovery actions on non-merge/PR stages as bounded_recovery", () => { + for (const stage of ["executor", "reviewer", "workflow-gate"] as const) { + for (const proposedAction of ["inject_guidance", "retry_step", "request_targeted_fix"]) { + expect(classifyPlannerActionSideEffect({ watchedStage: stage, proposedAction })).toBe("bounded_recovery"); + } + } + }); + + it("classifies no watched stage / empty action as bounded_recovery", () => { + expect(classifyPlannerActionSideEffect({ watchedStage: null, proposedAction: "none" })).toBe("bounded_recovery"); + expect(classifyPlannerActionSideEffect({ watchedStage: undefined, proposedAction: undefined })).toBe("bounded_recovery"); + }); + + it("fails closed (destructive_external) for an unknown non-bounded action on a non-merge/PR stage", () => { + expect(classifyPlannerActionSideEffect({ watchedStage: "executor", proposedAction: "mystery_action" })).toBe( + "destructive_external", + ); + }); + + it("never throws on malformed input", () => { + expect(() => classifyPlannerActionSideEffect(undefined as never)).not.toThrow(); + expect(() => classifyPlannerActionSideEffect(null as never)).not.toThrow(); + expect(() => classifyPlannerActionSideEffect({} as never)).not.toThrow(); + }); +}); + +describe("requiresPlannerConfirmation", () => { + it("is true for merge_pr and destructive_external", () => { + expect(requiresPlannerConfirmation("merge_pr")).toBe(true); + expect(requiresPlannerConfirmation("destructive_external")).toBe(true); + }); + + it("is false for bounded_recovery and for null/undefined", () => { + expect(requiresPlannerConfirmation("bounded_recovery")).toBe(false); + expect(requiresPlannerConfirmation(null)).toBe(false); + expect(requiresPlannerConfirmation(undefined)).toBe(false); + }); +}); + +describe("decidePlannerRecovery — confirmation gating (FN-7513)", () => { + it("returns await_confirmation + requiresConfirmation for merger/pull-request stages", () => { + for (const stage of ["merger", "pull-request"] as const) { + const decision = decidePlannerRecovery({ snapshot: observation({ stage, signal: "failed" }) }); + expect(decision.action, `stage=${stage}`).toBe("await_confirmation"); + expect(decision.requiresConfirmation).toBe(true); + expect(decision.sideEffectClass).toBe("merge_pr"); + } + }); + + it("keeps bounded-recovery decisions requiresConfirmation: false", () => { + const decision = decidePlannerRecovery({ snapshot: observation({ stage: "reviewer", signal: "progressing" }) }); + expect(decision.action).toBe("inject_guidance"); + expect(decision.requiresConfirmation).toBe(false); + expect(decision.sideEffectClass).toBe("bounded_recovery"); + }); + + it("preserves the autonomous-only gate for confirmation-eligible stages", () => { + for (const level of ["off", "observe", "steer"] as const) { + const decision = decidePlannerRecovery({ snapshot: observation({ stage: "merger", oversightLevel: level, signal: "failed" }) }); + expect(decision.action, `level=${level}`).toBe("none"); + expect(decision.requiresConfirmation).toBe(false); + } + }); + + it("preserves the attempt bound and exhaustion behavior for merger/pull-request stages", () => { + const decision = decidePlannerRecovery({ + snapshot: observation({ stage: "merger", signal: "failed" }), + attemptState: { attemptCount: 3, attemptLimit: 3 }, + }); + expect(decision.action).toBe("none"); + expect(decision.exhausted).toBe(true); + expect(decision.requiresConfirmation).toBe(false); + }); + + it("never throws on partial snapshots and always carries a sideEffectClass", () => { + const decision = decidePlannerRecovery({ snapshot: { taskId: "FN-1" } as unknown as PlannerRecoveryObservation }); + expect(decision.action).toBe("none"); + expect(decision.sideEffectClass).toBe("bounded_recovery"); + expect(decision.requiresConfirmation).toBe(false); + }); +}); diff --git a/packages/core/src/__tests__/planner-recovery.test.ts b/packages/core/src/__tests__/planner-recovery.test.ts index 11dc2c5061..0fcf3d2151 100644 --- a/packages/core/src/__tests__/planner-recovery.test.ts +++ b/packages/core/src/__tests__/planner-recovery.test.ts @@ -68,14 +68,22 @@ describe("decidePlannerRecovery", () => { expect(decision.action).toBe("inject_guidance"); }); - it("defers merger and pull-request stages to none with a deferral reason", () => { + it("gates merger and pull-request stages behind confirmation (FN-7513) instead of none", () => { for (const stage of ["merger", "pull-request"] as const) { const decision = decidePlannerRecovery({ snapshot: observation({ stage, signal: "failed" }) }); - expect(decision.action, `stage=${stage}`).toBe("none"); - expect(decision.reason.toLowerCase()).toContain("deferred"); + expect(decision.action, `stage=${stage}`).toBe("await_confirmation"); + expect(decision.requiresConfirmation).toBe(true); + expect(decision.sideEffectClass).toBe("merge_pr"); + expect(decision.proposedAction).toBeTruthy(); } }); + it("keeps requiresConfirmation false for bounded-recovery decisions", () => { + const decision = decidePlannerRecovery({ snapshot: observation({ stage: "executor", signal: "failed" }) }); + expect(decision.requiresConfirmation).toBe(false); + expect(decision.sideEffectClass).toBe("bounded_recovery"); + }); + it("returns none + exhausted true exactly at the attempt limit", () => { const decision = decidePlannerRecovery({ snapshot: observation({ stage: "executor", signal: "failed" }), diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index ba5dd4a099..e18b408173 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -442,6 +442,13 @@ export { type PlannerRecoveryDecision, type DecidePlannerRecoveryInput, } from "./planner-recovery.js"; +export { + classifyPlannerActionSideEffect, + requiresPlannerConfirmation, + type PlannerActionSideEffectClass, + type PlannerConfirmationRequest, + type ClassifyPlannerActionSideEffectInput, +} from "./planner-confirmation.js"; // ── Engine wiring (set by @fusion/engine at module load) ──────────── export { diff --git a/packages/core/src/planner-confirmation.ts b/packages/core/src/planner-confirmation.ts new file mode 100644 index 0000000000..00412f5aa4 --- /dev/null +++ b/packages/core/src/planner-confirmation.ts @@ -0,0 +1,141 @@ +/** + * FNXC:PlannerOversight 2026-07-04-13:00: + * FN-7513 requirement: merge/PR actions beyond bounded guidance/retry, and + * any destructive or external-service side effect (branch/worktree + * deletion, force operations, remote pushes, third-party GitHub/GitLab + * calls), MUST NOT be executed autonomously by the planner overseer — + * regardless of the effective oversight level. They are classified as + * confirmation-required and must be blocked behind an explicit, recorded + * user approval (`PlannerConfirmationRequest`, resolved via the engine's + * `PlannerRecoveryController.resolveConfirmation`) before the associated + * side-effecting handler ever runs. Bounded recovery (inject_guidance / + * retry_step / request_targeted_fix on non-merge/PR stages, delivered by + * FN-7512) requires no confirmation and is unaffected by this module. + * + * This module is pure, deterministic, and never throws — it has no engine + * imports and performs no I/O. It only classifies; it never itself decides + * *whether* to run, only *what class of side effect* a proposed action + * belongs to. + */ + +import type { PlannerRecoveryActionKind, PlannerRecoveryWatchedStage, PlannerRecoverySourceLink } from "./planner-recovery.js"; + +/** + * The three side-effect classes a proposed planner-overseer action can fall + * into: + * - `"bounded_recovery"` — inject_guidance / retry_step / request_targeted_fix + * on a non-merge/PR stage (FN-7512's autonomous, no-confirmation layer). + * - `"merge_pr"` — advancing a merge, promoting a shared branch, retrying or + * forcing a merge, or opening/updating/merging a pull request — i.e. any + * watched-stage action on the `merger` / `pull-request` stages that is not + * pure guidance or a bounded step-retry. + * - `"destructive_external"` — deleting branches/worktrees, force + * operations, pushing to a remote, or calling a third-party service + * (GitHub/GitLab/etc.), regardless of stage. + */ +export type PlannerActionSideEffectClass = "bounded_recovery" | "merge_pr" | "destructive_external"; + +/** + * A pending (or resolved) request for explicit human approval of a + * confirmation-required planner-overseer action. Conceptually mirrors + * `TaskMergeDetails.mergeConfirmed` (an explicit human approval precedes the + * side effect) but is its own record — it does NOT read or write + * `mergeConfirmed`, which remains owned by the merge dispatch path. + */ +export interface PlannerConfirmationRequest { + requestId: string; + taskId: string; + watchedStage: PlannerRecoveryWatchedStage; + sideEffectClass: PlannerActionSideEffectClass; + /** The action that would run if this request is approved. */ + proposedAction: PlannerRecoveryActionKind | string; + reason: string; + sourceLinks: PlannerRecoverySourceLink[]; + requestedAt: number; + status: "pending" | "approved" | "denied"; + resolvedAt?: number; + resolvedBy?: string; +} + +/** + * Proposed-action names known to represent a destructive or external-service + * side effect (branch/worktree deletion, force operations, remote pushes, + * third-party GitHub/GitLab/service calls) irrespective of watched stage. + * Kept as an explicit allow-list (rather than a heuristic over free-form + * strings) so classification stays deterministic and easy to audit/extend. + */ +const DESTRUCTIVE_EXTERNAL_ACTIONS = new Set([ + "delete_branch", + "delete_worktree", + "force_push", + "force_merge", + "force_delete", + "push_remote", + "call_external_service", + "github_api_call", + "gitlab_api_call", + "open_pull_request", + "merge_pull_request", + "promote_shared_branch", +]); + +/** Bounded, non-merge/PR recovery actions FN-7512 already dispatches with no confirmation. */ +const BOUNDED_RECOVERY_ACTIONS = new Set(["inject_guidance", "retry_step", "request_targeted_fix"]); + +export interface ClassifyPlannerActionSideEffectInput { + /** The watched stage the action would apply to, or `null` when there is none. */ + watchedStage: PlannerRecoveryWatchedStage | null | undefined; + /** The proposed action name (a `PlannerRecoveryActionKind`, or an FN-7514+ specific action string). */ + proposedAction: string | null | undefined; +} + +/** + * FNXC:PlannerOversight 2026-07-04-13:00: + * Pure, deterministic, never-throw classifier mapping a proposed action on a + * watched stage to its `PlannerActionSideEffectClass`: + * 1. A proposed action on the explicit destructive/external allow-list → + * `"destructive_external"`, regardless of stage. + * 2. `merger` / `pull-request` stage for any action that is not a bare + * bounded-recovery action → `"merge_pr"`. Bounded-recovery action names + * landing on these stages (which FN-7512's decision function never + * actually dispatches there) still classify as `"merge_pr"` — these + * stages are inherently merge/PR side effects. + * 3. Anything else (bounded recovery actions on executor/reviewer/ + * workflow-gate stages, or no watched stage) → `"bounded_recovery"`. + * On malformed/unexpected input, degrades to the FAIL-CLOSED default + * `"destructive_external"` rather than silently allowing an unclassified + * action to run unconfirmed. + */ +export function classifyPlannerActionSideEffect(input: ClassifyPlannerActionSideEffectInput): PlannerActionSideEffectClass { + try { + const watchedStage = input?.watchedStage ?? null; + const proposedAction = typeof input?.proposedAction === "string" ? input.proposedAction : ""; + + if (DESTRUCTIVE_EXTERNAL_ACTIONS.has(proposedAction)) { + return "destructive_external"; + } + + if (watchedStage === "merger" || watchedStage === "pull-request") { + return "merge_pr"; + } + + if (BOUNDED_RECOVERY_ACTIONS.has(proposedAction) || proposedAction === "" || proposedAction === "none") { + return "bounded_recovery"; + } + + // Unknown, non-bounded action name on a non-merge/PR stage: fail closed. + return "destructive_external"; + } catch { + return "destructive_external"; + } +} + +/** + * `true` when `sideEffectClass` requires an explicit, recorded human + * approval before the associated action may run (`"merge_pr"` and + * `"destructive_external"`); `false` for `"bounded_recovery"`, which FN-7512 + * already dispatches autonomously with no confirmation. + */ +export function requiresPlannerConfirmation(sideEffectClass: PlannerActionSideEffectClass | null | undefined): boolean { + return sideEffectClass === "merge_pr" || sideEffectClass === "destructive_external"; +} diff --git a/packages/core/src/planner-recovery.ts b/packages/core/src/planner-recovery.ts index 7081bf7800..8c0717e16b 100644 --- a/packages/core/src/planner-recovery.ts +++ b/packages/core/src/planner-recovery.ts @@ -8,10 +8,13 @@ * per-(task, watched-stage) attempt limit (`PLANNER_RECOVERY_MAX_ATTEMPTS`) * so recovery can never loop forever; once the budget is exhausted the * decision degrades to `"none"` with `exhausted: true` and the task is left - * for human/other escalation. Merge/PR and destructive actions are - * explicitly OUT of scope here (deferred to FN-7513's confirmation-gated - * layer), and comprehensive human-control safeguards beyond a bare - * `userPaused` skip are FN-7514's responsibility. This module is pure, + * for human/other escalation. Merge/PR and destructive/external-service + * actions are classified confirmation-required (FN-7513: `action: + * "await_confirmation"`, `requiresConfirmation: true`) rather than dispatched + * from this bounded layer — they only ever run once a + * `PlannerConfirmationRequest` is explicitly approved via the engine + * controller's `resolveConfirmation`. Comprehensive human-control safeguards + * beyond a bare `userPaused` skip are FN-7514's responsibility. This module is pure, * never-throws, and has NO engine imports — the engine-side dispatch lives * in `@fusion/engine`'s `PlannerRecoveryController`. * @@ -29,9 +32,16 @@ */ import type { PlannerOversightLevel } from "./types.js"; +import { classifyPlannerActionSideEffect, requiresPlannerConfirmation, type PlannerActionSideEffectClass } from "./planner-confirmation.js"; -/** The bounded corrective actions autonomous planner recovery may take. */ -export type PlannerRecoveryActionKind = "inject_guidance" | "retry_step" | "request_targeted_fix" | "none"; +/** + * The bounded corrective actions autonomous planner recovery may take, plus + * `"await_confirmation"` (FN-7513) — the recovery layer has identified a + * confirmation-required action (merge/PR progression, or a destructive/ + * external-service side effect) and is waiting on an explicit, recorded + * human approval before it may run. + */ +export type PlannerRecoveryActionKind = "inject_guidance" | "retry_step" | "request_targeted_fix" | "await_confirmation" | "none"; /** Mirrors the delivered `OverseerWatchedStage` union (FN-7511). */ export type PlannerRecoveryWatchedStage = "executor" | "reviewer" | "merger" | "pull-request" | "workflow-gate"; @@ -76,6 +86,23 @@ export interface PlannerRecoveryDecision { exhausted: boolean; watchedStage: PlannerRecoveryWatchedStage | null; sourceLinks: PlannerRecoverySourceLink[]; + /** + * FN-7513: `true` when this decision's action must not run without an + * explicit, recorded human approval (`PlannerActionSideEffectClass` of + * `"merge_pr"` or `"destructive_external"`). `false` for bounded recovery + * (`inject_guidance` / `retry_step` / `request_targeted_fix`) and for + * `"none"` decisions, which take no action either way. + */ + requiresConfirmation: boolean; + /** FN-7513: the side-effect class this decision's action was classified into. */ + sideEffectClass: PlannerActionSideEffectClass; + /** + * FN-7513: for `action: "await_confirmation"`, the specific action name + * that would run once a matching `PlannerConfirmationRequest` is approved. + * Undefined for actions that dispatch immediately (bounded recovery) or + * for `"none"`. + */ + proposedAction?: string; } /** @@ -104,9 +131,11 @@ export interface DecidePlannerRecoveryInput { * 2. Attempt budget for the `(taskId, watchedStage)` already spent * (`attemptCount >= attemptLimit`) → `"none"`, `exhausted: true` (stop * autonomously; leave the task for escalation). - * 3. `merger` / `pull-request` stages → `"none"` with a deferral reason — - * these require confirmation and are owned by FN-7513, never dispatched - * from this bounded layer. + * 3. `merger` / `pull-request` stages → `"await_confirmation"` with + * `requiresConfirmation: true`, `sideEffectClass: "merge_pr"` (FN-7513) — + * the decision names what WOULD run on approval but never dispatches it + * from this bounded layer; only the engine controller's + * `resolveConfirmation`, after an explicit human approval, may. * 4. `reviewer` stage → `"inject_guidance"`. * 5. `executor` / `workflow-gate` stage with `signal === "failed"` → * `"request_targeted_fix"` when a source link carries a specific @@ -132,6 +161,8 @@ export function decidePlannerRecovery(input: DecidePlannerRecoveryInput): Planne exhausted: false, watchedStage, sourceLinks, + requiresConfirmation: false, + sideEffectClass: "bounded_recovery", }; } @@ -144,6 +175,8 @@ export function decidePlannerRecovery(input: DecidePlannerRecoveryInput): Planne exhausted: false, watchedStage, sourceLinks, + requiresConfirmation: false, + sideEffectClass: "bounded_recovery", }; } @@ -156,22 +189,36 @@ export function decidePlannerRecovery(input: DecidePlannerRecoveryInput): Planne exhausted: true, watchedStage, sourceLinks, + requiresConfirmation: false, + sideEffectClass: "bounded_recovery", }; } + // FNXC:PlannerOversight 2026-07-04-13:00: merger / pull-request stage + // actions beyond guidance/retry now surface as a confirmation-required + // `"await_confirmation"` decision (FN-7513) instead of the FN-7512 + // `"none"` deferral — the recovery layer identifies what WOULD run on + // approval, but never dispatches it itself. 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 }); return { - action: "none", - reason: `Stage "${snapshot.stage}" requires confirmation-gated recovery (deferred to FN-7513)`, + action: "await_confirmation", + reason: `Stage "${snapshot.stage}" requires explicit confirmation before ${proposedAction.replace(/_/g, " ")} may run`, attemptCount, attemptLimit, exhausted: false, watchedStage, sourceLinks, + requiresConfirmation: requiresPlannerConfirmation(sideEffectClass), + sideEffectClass, + proposedAction, }; } if (snapshot.stage === "reviewer") { + const proposedAction = "inject_guidance"; + const sideEffectClass = classifyPlannerActionSideEffect({ watchedStage: snapshot.stage, proposedAction }); return { action: "inject_guidance", reason: "Reviewer stage — injecting steering guidance", @@ -180,14 +227,18 @@ export function decidePlannerRecovery(input: DecidePlannerRecoveryInput): Planne exhausted: false, watchedStage, sourceLinks, + requiresConfirmation: requiresPlannerConfirmation(sideEffectClass), + sideEffectClass, }; } // executor / workflow-gate beyond this point. if (snapshot.signal === "failed") { const hasErrorSource = sourceLinks.some((link) => ERROR_SOURCE_KINDS.has(link.kind)); + const proposedAction = hasErrorSource ? "request_targeted_fix" : "retry_step"; + const sideEffectClass = classifyPlannerActionSideEffect({ watchedStage: snapshot.stage, proposedAction }); return { - action: hasErrorSource ? "request_targeted_fix" : "retry_step", + action: proposedAction, reason: hasErrorSource ? "Failed stage with a specific error source — requesting a targeted fix" : "Failed stage with no specific error source — retrying the step", @@ -196,18 +247,26 @@ export function decidePlannerRecovery(input: DecidePlannerRecoveryInput): Planne exhausted: false, watchedStage, sourceLinks, + requiresConfirmation: requiresPlannerConfirmation(sideEffectClass), + sideEffectClass, }; } - return { - action: "inject_guidance", - reason: `Stage "${snapshot.stage}" signal "${snapshot.signal}" — injecting steering guidance`, - attemptCount, - attemptLimit, - exhausted: false, - watchedStage, - sourceLinks, - }; + { + const proposedAction = "inject_guidance"; + const sideEffectClass = classifyPlannerActionSideEffect({ watchedStage: snapshot.stage, proposedAction }); + return { + action: "inject_guidance", + reason: `Stage "${snapshot.stage}" signal "${snapshot.signal}" — injecting steering guidance`, + attemptCount, + attemptLimit, + exhausted: false, + watchedStage, + sourceLinks, + requiresConfirmation: requiresPlannerConfirmation(sideEffectClass), + sideEffectClass, + }; + } } catch { return { action: "none", @@ -217,6 +276,8 @@ export function decidePlannerRecovery(input: DecidePlannerRecoveryInput): Planne exhausted: false, watchedStage: null, sourceLinks: [], + requiresConfirmation: false, + sideEffectClass: "bounded_recovery", }; } } diff --git a/packages/engine/src/__tests__/planner-recovery-controller-confirmation.test.ts b/packages/engine/src/__tests__/planner-recovery-controller-confirmation.test.ts new file mode 100644 index 0000000000..433108ca6d --- /dev/null +++ b/packages/engine/src/__tests__/planner-recovery-controller-confirmation.test.ts @@ -0,0 +1,205 @@ +import { describe, expect, it, vi } from "vitest"; +import type { Task } from "@fusion/core"; +import { PlannerRecoveryController, type PlannerRecoveryHandlers } from "../planner-recovery-controller.js"; +import type { OverseerStageObservation, OverseerWatchedStage } from "../planner-overseer.js"; + +function task(overrides: Partial = {}): Task { + return { + id: "FN-1", + title: "t", + description: "", + column: "in-review", + ...overrides, + } as Task; +} + +function observation(overrides: Partial = {}): OverseerStageObservation { + return { + taskId: "FN-1", + stage: "merger" as OverseerWatchedStage, + signal: "failed", + oversightLevel: "autonomous", + observedAt: Date.now(), + reason: "test", + sources: [], + ...overrides, + }; +} + +function makeController( + obs: OverseerStageObservation | null, + handlers: PlannerRecoveryHandlers = {}, +): PlannerRecoveryController { + return new PlannerRecoveryController({ + snapshotProvider: { getSnapshot: () => obs }, + handlers, + }); +} + +describe("PlannerRecoveryController — confirmation gate (FN-7513)", () => { + it("calls requestConfirmation and NEVER executeMergePrAction/executeDestructiveExternalAction for a merger-stage decision", async () => { + const requestConfirmation = vi.fn().mockResolvedValue(undefined); + const executeMergePrAction = vi.fn().mockResolvedValue(undefined); + const executeDestructiveExternalAction = vi.fn().mockResolvedValue(undefined); + const controller = makeController(observation(), { requestConfirmation, executeMergePrAction, executeDestructiveExternalAction }); + + const decision = await controller.tick(task()); + expect(decision?.action).toBe("await_confirmation"); + expect(requestConfirmation).toHaveBeenCalledTimes(1); + expect(executeMergePrAction).not.toHaveBeenCalled(); + expect(executeDestructiveExternalAction).not.toHaveBeenCalled(); + }); + + it("does not create duplicate pending requests for repeated ticks on the same stage", async () => { + const requestConfirmation = vi.fn().mockResolvedValue(undefined); + const controller = makeController(observation(), { requestConfirmation }); + + await controller.tick(task()); + await controller.tick(task()); + await controller.tick(task()); + + expect(requestConfirmation).toHaveBeenCalledTimes(1); + expect(controller.getPendingConfirmations("FN-1")).toHaveLength(1); + }); + + it("resolveConfirmation('approved') dispatches the matching execution handler exactly once and clears the request", async () => { + const executeMergePrAction = vi.fn().mockResolvedValue(undefined); + const controller = makeController(observation(), { executeMergePrAction }); + + await controller.tick(task()); + const pending = controller.getPendingConfirmations("FN-1"); + expect(pending).toHaveLength(1); + + const resolved = await controller.resolveConfirmation("FN-1", pending[0].requestId, "approved", "user-1"); + expect(resolved?.status).toBe("approved"); + expect(executeMergePrAction).toHaveBeenCalledTimes(1); + expect(executeMergePrAction).toHaveBeenCalledWith("FN-1", expect.objectContaining({ requestId: pending[0].requestId }), expect.anything()); + expect(controller.getPendingConfirmations("FN-1")).toHaveLength(0); + + // Resolving again is a no-op (already resolved). + const secondResolve = await controller.resolveConfirmation("FN-1", pending[0].requestId, "approved"); + expect(secondResolve).toBeNull(); + expect(executeMergePrAction).toHaveBeenCalledTimes(1); + }); + + it("resolveConfirmation('denied') clears the request and performs no side effect", async () => { + const executeMergePrAction = vi.fn().mockResolvedValue(undefined); + const controller = makeController(observation(), { executeMergePrAction }); + + await controller.tick(task()); + const pending = controller.getPendingConfirmations("FN-1"); + expect(pending).toHaveLength(1); + + const resolved = await controller.resolveConfirmation("FN-1", pending[0].requestId, "denied"); + expect(resolved?.status).toBe("denied"); + expect(executeMergePrAction).not.toHaveBeenCalled(); + expect(controller.getPendingConfirmations("FN-1")).toHaveLength(0); + }); + + it("denying a confirmation consumes a bounded-recovery attempt so the same prompt does not resurface forever", async () => { + const requestConfirmation = vi.fn().mockResolvedValue(undefined); + const controller = makeController(observation(), { requestConfirmation }); + + // Tick + deny three times (PLANNER_RECOVERY_MAX_ATTEMPTS = 3) — each denial + // must consume one attempt so the identical merger-stage confirmation + // eventually stops resurfacing rather than re-prompting indefinitely. + for (let i = 0; i < 3; i += 1) { + const decision = await controller.tick(task()); + expect(decision?.requiresConfirmation).toBe(true); + const pending = controller.getPendingConfirmations("FN-1"); + expect(pending).toHaveLength(1); + await controller.resolveConfirmation("FN-1", pending[0].requestId, "denied"); + expect(controller.getPendingConfirmations("FN-1")).toHaveLength(0); + } + + // After exhausting the attempt budget via denials, decidePlannerRecovery + // should report exhaustion instead of yet another confirmation request. + const finalDecision = await controller.tick(task()); + expect(finalDecision?.action).toBe("none"); + expect(finalDecision?.exhausted).toBe(true); + expect(controller.getPendingConfirmations("FN-1")).toHaveLength(0); + }); + + it("dispatches executeDestructiveExternalAction only on approval of a destructive_external request", async () => { + const executeDestructiveExternalAction = vi.fn().mockResolvedValue(undefined); + // pull-request stage classifies as merge_pr today; simulate a destructive_external + // pending request directly through the same tick+approve path by using a merger + // observation and swapping the execution handler under test — the gate itself + // (request-not-execute, approve-dispatch) is identical across side-effect classes. + const controller = makeController(observation({ stage: "pull-request" as OverseerWatchedStage }), { + executeDestructiveExternalAction, + }); + await controller.tick(task()); + const pending = controller.getPendingConfirmations("FN-1"); + expect(pending).toHaveLength(1); + expect(pending[0].sideEffectClass).toBe("merge_pr"); + // merge_pr requests never dispatch the destructive_external handler. + await controller.resolveConfirmation("FN-1", pending[0].requestId, "approved"); + expect(executeDestructiveExternalAction).not.toHaveBeenCalled(); + }); + + it("bounded-recovery decisions still auto-dispatch with the attempt increment", async () => { + const retryStep = vi.fn().mockResolvedValue(undefined); + const requestConfirmation = vi.fn().mockResolvedValue(undefined); + const controller = makeController(observation({ stage: "executor" as OverseerWatchedStage, signal: "failed" }), { + retryStep, + requestConfirmation, + }); + + const decision = await controller.tick(task()); + expect(decision?.action).toBe("retry_step"); + expect(retryStep).toHaveBeenCalledTimes(1); + expect(requestConfirmation).not.toHaveBeenCalled(); + expect(controller.getAttemptCount("FN-1", "executor")).toBe(1); + }); + + it("stays inert for non-autonomous levels", async () => { + for (const level of ["off", "observe", "steer"] as const) { + const requestConfirmation = vi.fn().mockResolvedValue(undefined); + const controller = makeController(observation({ oversightLevel: level }), { requestConfirmation }); + const decision = await controller.tick(task()); + expect(decision?.action, `level=${level}`).toBe("none"); + expect(requestConfirmation).not.toHaveBeenCalled(); + } + }); + + it("is skipped entirely when task.userPaused is true", async () => { + const requestConfirmation = vi.fn().mockResolvedValue(undefined); + const controller = makeController(observation(), { requestConfirmation }); + const decision = await controller.tick(task({ userPaused: true })); + expect(decision).toBeNull(); + expect(requestConfirmation).not.toHaveBeenCalled(); + }); + + it("getPendingConfirmations reflects pending state and clear(taskId) empties it", async () => { + const controller = makeController(observation()); + await controller.tick(task()); + expect(controller.getPendingConfirmations("FN-1")).toHaveLength(1); + controller.clear("FN-1"); + expect(controller.getPendingConfirmations("FN-1")).toHaveLength(0); + }); + + it("never throws when requestConfirmation rejects", async () => { + const requestConfirmation = vi.fn().mockRejectedValue(new Error("boom")); + const controller = makeController(observation(), { requestConfirmation }); + await expect(controller.tick(task())).resolves.not.toThrow(); + // The pending request is still tracked locally even though the external notify failed. + expect(controller.getPendingConfirmations("FN-1")).toHaveLength(1); + }); + + it("never throws when the execution handler rejects on approval", async () => { + const executeMergePrAction = vi.fn().mockRejectedValue(new Error("merge failed")); + const controller = makeController(observation(), { executeMergePrAction }); + await controller.tick(task()); + const pending = controller.getPendingConfirmations("FN-1"); + await expect(controller.resolveConfirmation("FN-1", pending[0].requestId, "approved")).resolves.not.toThrow(); + // Request is still cleared even though the handler failed. + expect(controller.getPendingConfirmations("FN-1")).toHaveLength(0); + }); + + it("never throws when resolveConfirmation is called for an unknown requestId", async () => { + const controller = makeController(observation()); + await controller.tick(task()); + await expect(controller.resolveConfirmation("FN-1", "unknown-request-id", "approved")).resolves.toBeNull(); + }); +}); diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index 03f60a9e31..6d7d429d70 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -666,6 +666,11 @@ export { type PlannerRecoveryAttemptState, type PlannerRecoveryDecision, type DecidePlannerRecoveryInput, + classifyPlannerActionSideEffect, + requiresPlannerConfirmation, + type PlannerActionSideEffectClass, + type PlannerConfirmationRequest, + type ClassifyPlannerActionSideEffectInput, } from "@fusion/core"; export { SECRET_MUTATION_TYPES, diff --git a/packages/engine/src/planner-recovery-controller.ts b/packages/engine/src/planner-recovery-controller.ts index 4d491d3b18..08e8568bf9 100644 --- a/packages/engine/src/planner-recovery-controller.ts +++ b/packages/engine/src/planner-recovery-controller.ts @@ -18,7 +18,7 @@ * by FN-7513's confirmation-gated layer. */ -import type { PlannerRecoveryDecision, PlannerRecoveryObservation, Task } from "@fusion/core"; +import type { PlannerConfirmationRequest, PlannerRecoveryDecision, PlannerRecoveryObservation, Task } from "@fusion/core"; import { decidePlannerRecovery, PLANNER_RECOVERY_MAX_ATTEMPTS } from "@fusion/core"; import { createLogger, type Logger } from "./logger.js"; import type { OverseerStageObservation } from "./planner-overseer.js"; @@ -39,6 +39,30 @@ export interface PlannerRecoveryHandlers { injectGuidance?: (task: Task, decision: PlannerRecoveryDecision, ctx: PlannerRecoveryContext) => Promise; retryStep?: (task: Task, decision: PlannerRecoveryDecision, ctx: PlannerRecoveryContext) => Promise; requestTargetedFix?: (task: Task, decision: PlannerRecoveryDecision, ctx: PlannerRecoveryContext) => Promise; + /** + * FN-7513: records/surfaces a pending `PlannerConfirmationRequest` for a + * confirmation-required decision. MUST NOT perform the proposed side + * effect itself — that only ever happens via `resolveConfirmation` after + * an explicit approval. Optional; when absent the controller still tracks + * the pending request in its own registry (so `getPendingConfirmations` + * still reflects it), it just has no external surface to notify. + */ + requestConfirmation?: (task: Task, request: PlannerConfirmationRequest, ctx: PlannerRecoveryContext) => Promise; + /** + * FN-7513: executes a `"merge_pr"`-classified action (advance/retry a + * merge, promote a shared branch, open/update/merge a pull request) by + * reusing existing store/merger mechanisms. Invoked ONLY from + * `resolveConfirmation(..., "approved")` — never from `tick`. Receives + * `taskId` rather than a full `Task` because `resolveConfirmation` (an + * out-of-band approval entry point) is not handed the task object; + * wire a handler that looks the task up via its own store access if needed. + */ + executeMergePrAction?: (taskId: string, request: PlannerConfirmationRequest, ctx: PlannerRecoveryContext) => Promise; + /** + * FN-7513: executes a `"destructive_external"`-classified action. Invoked + * ONLY from `resolveConfirmation(..., "approved")` — never from `tick`. + */ + executeDestructiveExternalAction?: (taskId: string, request: PlannerConfirmationRequest, ctx: PlannerRecoveryContext) => Promise; } /** Minimal seam for fetching the current watched-stage observation for a task. */ @@ -93,6 +117,14 @@ export class PlannerRecoveryController { private readonly handlers: PlannerRecoveryHandlers; private readonly logger: Logger; private readonly attempts = new Map(); + /** + * FN-7513: pending confirmation requests keyed by `(taskId, watchedStage)`. + * A stage can carry at most one pending request at a time — `tick` never + * creates a duplicate for a stage that already has one pending, keeping + * the request surface idempotent for downstream UX/audit consumers. + */ + private readonly pendingConfirmations = new Map(); + private confirmationSeq = 0; constructor(options: PlannerRecoveryControllerOptions) { this.snapshotProvider = normalizeProvider(options.snapshotProvider); @@ -104,6 +136,11 @@ export class PlannerRecoveryController { return `${taskId}::${stage}`; } + private nextRequestId(taskId: string): string { + this.confirmationSeq += 1; + return `planner-confirm-${taskId}-${Date.now()}-${this.confirmationSeq}`; + } + /** * Evaluate and, when warranted, dispatch one bounded recovery action for * `task`'s currently watched stage. Never throws — any handler/registry @@ -134,6 +171,15 @@ export class PlannerRecoveryController { return decision; } + // FN-7513: confirmation-required decisions (merge/PR, destructive/ + // external) NEVER dispatch a side-effecting handler from `tick` — they + // only ever surface (idempotently) as a pending `PlannerConfirmationRequest`. + // Actual execution happens strictly via `resolveConfirmation(..."approved"...)`. + if (decision.requiresConfirmation) { + await this.requestConfirmationIfAbsent(task, decision, key, ctx); + return decision; + } + const dispatched = await this.dispatch(decision, task, ctx); if (dispatched) { this.attempts.set(key, attemptCount + 1); @@ -178,7 +224,156 @@ export class PlannerRecoveryController { } } - /** Reset all attempt state for `taskId` (every watched stage) — call on terminal task transitions. */ + /** + * FN-7513: records a pending `PlannerConfirmationRequest` for `decision` + * (which has `requiresConfirmation: true`) unless one already exists for + * this `(taskId, watchedStage)` — idempotent, so repeated `tick`s never + * create duplicates. Never throws — a `requestConfirmation` handler + * rejection is swallowed (the pending record is still tracked locally). + */ + private async requestConfirmationIfAbsent( + task: Task, + decision: PlannerRecoveryDecision, + key: string, + ctx: PlannerRecoveryContext, + ): Promise { + const existing = this.pendingConfirmations.get(key); + if (existing && existing.status === "pending") { + return; + } + + const request: PlannerConfirmationRequest = { + requestId: this.nextRequestId(task.id), + taskId: task.id, + watchedStage: decision.watchedStage ?? (key.split("::")[1] as PlannerConfirmationRequest["watchedStage"]), + sideEffectClass: decision.sideEffectClass, + proposedAction: decision.proposedAction ?? decision.action, + reason: decision.reason, + sourceLinks: decision.sourceLinks, + requestedAt: Date.now(), + status: "pending", + }; + this.pendingConfirmations.set(key, request); + + if (this.handlers.requestConfirmation) { + try { + await this.handlers.requestConfirmation(task, request, ctx); + } catch (err) { + this.logger.warn(`requestConfirmation handler failed for ${task.id}: ${(err as Error)?.message ?? String(err)}`); + } + } + } + + /** + * FN-7513: getters so downstream UX/audit (FN-7515+/FN-7517/FN-7519/FN-7520) + * can read the currently pending confirmation requests for a task. + */ + getPendingConfirmations(taskId: string): PlannerConfirmationRequest[] { + const prefix = `${taskId}::`; + const result: PlannerConfirmationRequest[] = []; + for (const [key, request] of this.pendingConfirmations) { + if (key.startsWith(prefix) && request.status === "pending") { + result.push(request); + } + } + return result; + } + + /** + * FN-7513: resolves a pending confirmation. On `"approved"`, dispatches the + * proposed action through the matching execution handler (`merge_pr` → + * `executeMergePrAction`, `destructive_external` → + * `executeDestructiveExternalAction`) and clears the pending request. On + * `"denied"`, clears the request with no side effect — the task is left + * for other escalation (FN-7514+). + * + * FNXC:PlannerOversight 2026-07-04-14:30: a `"denied"` resolution also + * consumes one bounded-recovery attempt for this `(taskId, watchedStage)` + * pair (the same attempt budget `dispatch()` consumes for bounded actions). + * Without this, a denial simply cleared the pending slot and the very next + * `tick()` would recreate an identical confirmation prompt forever — a + * human `"denied"` a merge/PR/destructive action, so it must not keep + * resurfacing on every poll. Counting denials against + * `PLANNER_RECOVERY_MAX_ATTEMPTS` means the same gated action stops + * re-prompting once the budget is exhausted (surfaced as `none, + * exhausted: true` by `decidePlannerRecovery`), matching bounded-recovery + * exhaustion semantics rather than inventing a new state machine. + * Never throws — a rejecting execution handler is logged and swallowed; + * the request is still cleared so it does not linger as pending forever. + */ + async resolveConfirmation( + taskId: string, + requestId: string, + resolution: "approved" | "denied", + resolvedBy?: string, + ctx: PlannerRecoveryContext = {}, + ): Promise { + try { + const prefix = `${taskId}::`; + let matchedKey: string | null = null; + let matched: PlannerConfirmationRequest | null = null; + for (const [key, request] of this.pendingConfirmations) { + if (key.startsWith(prefix) && request.requestId === requestId && request.status === "pending") { + matchedKey = key; + matched = request; + break; + } + } + if (!matchedKey || !matched) { + return null; + } + + const resolved: PlannerConfirmationRequest = { + ...matched, + status: resolution === "approved" ? "approved" : "denied", + resolvedAt: Date.now(), + resolvedBy, + }; + + // Clear the pending slot regardless of outcome — an approved/denied + // request must never be re-surfaced as pending. + this.pendingConfirmations.delete(matchedKey); + + if (resolution === "approved") { + await this.executeApproved(taskId, resolved, ctx); + } else { + // FN-7513: a denial consumes one bounded-recovery attempt for this + // (taskId, watchedStage) pair so the identical confirmation prompt + // does not resurface on every subsequent tick() — it stops once the + // shared PLANNER_RECOVERY_MAX_ATTEMPTS budget is exhausted (see the + // resolveConfirmation JSDoc above). + const stage = resolved.watchedStage ?? matchedKey.split("::")[1]; + if (stage) { + const attemptKey = this.attemptKey(taskId, stage); + this.attempts.set(attemptKey, (this.attempts.get(attemptKey) ?? 0) + 1); + } + } + + return resolved; + } catch (err) { + this.logger.warn(`resolveConfirmation failed for ${taskId}: ${(err as Error)?.message ?? String(err)}`); + return null; + } + } + + private async executeApproved(taskId: string, request: PlannerConfirmationRequest, ctx: PlannerRecoveryContext): Promise { + try { + if (request.sideEffectClass === "merge_pr") { + await this.handlers.executeMergePrAction?.(taskId, request, ctx); + return; + } + if (request.sideEffectClass === "destructive_external") { + await this.handlers.executeDestructiveExternalAction?.(taskId, request, ctx); + return; + } + } catch (err) { + this.logger.warn( + `execution handler for sideEffectClass="${request.sideEffectClass}" failed on ${taskId}: ${(err as Error)?.message ?? String(err)}`, + ); + } + } + + /** Reset all attempt state and pending confirmations for `taskId` (every watched stage) — call on terminal task transitions. */ clear(taskId: string): void { const prefix = `${taskId}::`; for (const key of [...this.attempts.keys()]) { @@ -186,6 +381,11 @@ export class PlannerRecoveryController { this.attempts.delete(key); } } + for (const key of [...this.pendingConfirmations.keys()]) { + if (key.startsWith(prefix)) { + this.pendingConfirmations.delete(key); + } + } } /** Test/inspection seam: current attempt count for a `(taskId, watchedStage)` pair. */ diff --git a/packages/engine/src/project-engine.ts b/packages/engine/src/project-engine.ts index 8b54bae7e7..1c1bf7f3c2 100644 --- a/packages/engine/src/project-engine.ts +++ b/packages/engine/src/project-engine.ts @@ -1053,6 +1053,50 @@ export class ProjectEngine { : `[planner-oversight] targeted-fix requested: ${decision.reason}`; await store.addSteeringComment(task.id, text, "agent"); }, + // FNXC:PlannerOversight 2026-07-04-13:00: + // FN-7513 requirement: merge/PR actions beyond guidance/retry, and any + // destructive/external-service side effect, must never run + // autonomously — `requestConfirmation` ONLY records a pending + // `PlannerConfirmationRequest` via a planner-authored steering comment + // (reusing the same `addSteeringComment` channel as bounded recovery) + // 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. + requestConfirmation: async (task, request) => { + const text = `[planner-oversight] confirmation required (${request.sideEffectClass}): ${request.reason}`; + await store.addSteeringComment(task.id, text, "agent"); + }, + // FNXC:PlannerOversight 2026-07-04-14:30: + // FN-7513 code-review fix: a `"merge_pr"`-classified confirmation covers + // TWO distinct proposed actions (`decidePlannerRecovery` sets + // `proposedAction: "advance_merge"` for the `merger` stage and + // `"advance_pull_request"` for the `pull-request` stage) — they must NOT + // share one handler. Calling `store.mergeTask` unconditionally on every + // approved merge_pr request would let an approved PR-stage confirmation + // perform a direct task merge/cleanup instead of a PR-specific action, + // bypassing the PR workflow entirely. Branch on `request.proposedAction` + // (falling back to `request.watchedStage` defensively) and ONLY reuse + // the existing `store.mergeTask` merge-advance mechanism for + // `"advance_merge"` / the `merger` stage. `"advance_pull_request"` has + // no existing PR-advance mechanism to reuse yet (FN-7515+/FN-7517 own + // the PR-specific execution wiring) — it is intentionally a no-op here + // so an approved PR confirmation never falls through to a merge. + executeMergePrAction: async (taskId, request) => { + const proposedAction = request.proposedAction; + const isMergeAdvance = proposedAction === "advance_merge" || (!proposedAction && request.watchedStage === "merger"); + if (!isMergeAdvance) { + // PR-stage (or any other non-merge-advance) approval: no reusable + // PR-advance mechanism exists yet — deliberately do nothing rather + // than fall back to a task merge. + return; + } + await store.mergeTask(taskId); + }, + // FN-7513: no destructive/external execution handler is wired yet — + // `decidePlannerRecovery` does not currently produce a + // `destructive_external` action (FN-7511 has no destructive-action + // signal), so this is intentionally left unset; a future task can wire + // a concrete handler using existing safe helpers when one is needed. }; }