From cae7847085123cad7c89f23e400c557fe6bcc9b2 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 15 Jul 2026 20:14:25 -0700 Subject: [PATCH] fix(FN-8004): retry ACP provider blips in auto-merge instead of parking failed (#2157) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What happened FN-8004's implementation work finished and passed review. The auto-merge then failed with `Grok ACP turn failed: Internal error` — a ~20 second provider blip — and the task was parked `status: "failed"` with 8 files of complete, reviewed work stranded on its branch. The park is the interesting part: `status: "failed"` is precisely what tells recovery to stop. So a misclassification here isn't a missed retry, it's **terminal**. Both recovery paths were disabled by the same wrong verdict: - `maybeRetryTransientMerge` (inline, 3 retries w/ backoff) — never fired once (`mergeTransientRetryCount: 0`). - `recoverTransientMergeFailures` (self-healing sweep, exists exactly to rescue parked in-review tasks) — skipped it, gated on the same classifier. ## Three defects fixed **1. No AI-provider failure class existed.** The AI merge drives a real LLM turn, but `classifyTransientMergeError` only modeled git/lease/spawn faults. Adds `ai-provider-turn-failure`. **2. ACP dropped the error detail.** `promptAcpSession` rethrew the SDK error unchanged, discarding the JSON-RPC `code`/`data` — the only evidence the fault was provider-side. ("Internal error" is just the standard text for `-32603`.) It now preserves them, keeping the original as `cause`: ``` Internal error (acp rpc code -32603, retryable) ``` Classification anchors on that envelope, **not** on the bare `"Internal error"` — matching that unanchored would disguise genuine application defects as retryable blips. Only provider-fault codes (`-32603`, `-32000`..`-32003`) are retryable; caller-fault codes (`-32600`..`-32602`) stay permanent, since retrying just repeats the failing call. **3. Sweep/inline asymmetry** (found while tracing; latent and unreported). The inline gate accepted `isTransientError(msg) || classify(msg)`, but the sweep consulted **only** the classifier. So `ECONNRESET` / `socket hang up` during a merge earned inline retries and then went **invisible to the sweep** once parked — stranded forever. The classifier now delegates to `isTransientError`, so both gates agree by construction. To keep that delegation from importing the detector's `usage-limit-detector → logger` chain (the chain FN-5627 split the classifier out to avoid, which would break `notification-service.test.ts`'s partial `vi.mock`), the pure predicates moved to the import-free leaf `transient-error-patterns.ts`, re-exported from `transient-error-detector.ts`. All 13 exports preserved, verified programmatically. ## Loosened budgets Per request, so more self-heals. Both apply **only** to errors already proven transient; the ceiling and `merger:transient-failure-budget-exhausted` audit path remain. | Budget | Before | After | |---|---|---| | `MAX_AUTO_MERGE_TRANSIENT_RETRIES` | 3 | 5 (backoff 5s/10s/20s/40s/80s) | | `MAX_TRANSIENT_MERGE_RECOVERIES` | 2 | 5 | The bump broke two suites that had hardcoded the old `3`. Rather than swap in another magic number, both now derive the cap from the constant so future tuning doesn't re-break them. ## Verification - `pnpm test:gate` green · `pnpm lint` clean · engine + ACP typecheck clean · `pnpm verify:fast` PASS (5/5) - ACP plugin 230 tests green · Grok plugin 64 green · engine transient/merge suites 136 green - Regression tests assert the **invariant across every surface** (per *Fix the Invariant, Not the Repro*), not just the reported Grok string: both ACP runtime prefixes, all retryable/non-retryable rpc codes, both SDK error shapes, network delegation, class-ordering, and negative cases proving bare `"Internal error"` and real defects stay permanent. - A test caught a genuine bug in my own code mid-review (nested-shape message shadowing), now fixed. - `notifier.test.ts > "awaiting approval"` fails — **confirmed pre-existing on clean main**, unrelated. ## Note FN-8004's own branch (`fusion/fn-8004`) is still unmerged and its work looks complete. Once this lands, its merge should be retried separately. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.8 (1M context) --- ...8004-acp-transient-merge-classification.md | 7 + docs/architecture.md | 5 +- .../__tests__/merge-error-recovery.test.ts | 7 +- .../dual-observe-merge-seam.test.ts | 5 +- .../transient-error-detector.test.ts | 47 ++++++ .../transient-merge-error-classifier.test.ts | 81 ++++++++++ packages/engine/src/project-engine.ts | 16 +- packages/engine/src/self-healing.ts | 11 +- .../engine/src/transient-error-detector.ts | 124 ++------------ .../engine/src/transient-error-patterns.ts | 153 ++++++++++++++++++ .../src/transient-merge-error-classifier.ts | 34 ++++ .../src/__tests__/acp-turn-error.test.ts | 112 +++++++++++++ .../fusion-plugin-acp-runtime/src/provider.ts | 96 ++++++++++- .../src/runtime-adapter.ts | 10 ++ 14 files changed, 584 insertions(+), 124 deletions(-) create mode 100644 .changeset/fn-8004-acp-transient-merge-classification.md create mode 100644 packages/engine/src/transient-error-patterns.ts create mode 100644 plugins/fusion-plugin-acp-runtime/src/__tests__/acp-turn-error.test.ts diff --git a/.changeset/fn-8004-acp-transient-merge-classification.md b/.changeset/fn-8004-acp-transient-merge-classification.md new file mode 100644 index 0000000000..6d36968398 --- /dev/null +++ b/.changeset/fn-8004-acp-transient-merge-classification.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Auto-merge now retries AI provider blips instead of permanently failing the task. +category: fix +dev: ACP provider faults (`promptAcpSession` now preserves the JSON-RPC code as `acp rpc code -32603`) classify as transient via a new `ai-provider-turn-failure` class. `classifyTransientMergeError` also delegates to `isTransientError`, so the self-healing sweep and the inline retry gate share one definition — previously network errors got inline retries but were invisible to the sweep once parked `failed`. Pure predicates moved to the import-free leaf `transient-error-patterns.ts` (re-exported from `transient-error-detector.ts`) to keep the logger chain out of the classifier per FN-5627. Transient budgets raised: `MAX_AUTO_MERGE_TRANSIENT_RETRIES` 3→5, `MAX_TRANSIENT_MERGE_RECOVERIES` 2→5. diff --git a/docs/architecture.md b/docs/architecture.md index 97b45652ae..6c1fd5e875 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -705,7 +705,10 @@ If loop recovery times out during compact-and-resume and the executor does not u - `recoverMissingWorktreeReviewFailures()` is a narrow failed-review recovery: only `status: "failed"` `in-review` tasks with the explicit session-start signature `Refusing to start coding agent in missing worktree:` (from `assertValidWorktreeSession()`) are requeued. Recovery clears stale session metadata (`worktree`, `branch`, `sessionFile`, transient failure state), preserves valid step progress/retry counters, logs the auto-recovery reason, and moves the task back to `todo` for a clean retry. - `recoverMergeableReviewTasks()` only re-enqueues truly eligible tasks; retry-exhausted review tasks are skipped to avoid re-enqueue/no-op loops that keep refreshing `updatedAt`. - `recoverAlreadyMergedReviewTasks()` auto-finalizes retry-exhausted `in-review` tasks when self-healing can prove their work already landed on the merge target. On this landed-content path it clears soft blockers (`paused`, stale `status: "failed"`, and residual `error`) before moving to `done`; true hard blockers (for example incomplete steps, awaiting-user-review, or failed pre-merge workflow steps) still park the task in stable `in-review/failed` state with a blocker error instead of entering an auto-finalize loop. Already-merged/tip recovery must prove task ownership before setting `mergeDetails.mergeConfirmed` or moving to `done`: accepted evidence is a matching `Fusion-Task-Id`, matching `Fusion-Task-Lineage`, a task-ID anchored conventional subject, or a patch-id/tree-equal fallback from the canonical `fusion/` branch whose tip and candidate commit are not explicitly attributed to another task/lineage. Foreign task tips (for example an FN-7143 row pointing at an FN-7187 tip) are rejected in place with `[recovery] already-merged rejected ... reason=foreign-task-tip` and `task:auto-recover-already-merged-rejected` audit metadata instead of finalizing the wrong task. - - `recoverTransientMergeFailures()` handles retry-exhausted `in-review` merge failures only when `classifyTransientMergeError()` returns a bounded transient class: `lease-handoff-target-not-queued`, `spurious-concurrent-advance-same-sha`, or `process-spawn-failure` (`spawn ENOTDIR`, `spawn … ENOENT`, or a clean-room path reported as `is not a working tree`). Recovery resets `mergeRetries`, clears transient `status`/`error`, increments `mergeDetails.transientRecoveryCount`, and requeues auto-merge so the next attempt recreates the AI-merge clean room. The budget stays capped by `MAX_TRANSIENT_MERGE_RECOVERIES`; exhausted tasks remain parked with the `merger:transient-failure-budget-exhausted` audit path so real structural failures cannot loop forever. FN-6278 makes this recovery mostly after-the-fact insurance for cwd spawn faults: the merge runner now preflights reuse integration roots and repairs/reacquires missing or de-registered task worktrees before the first git spawn, so a stale `task.worktree` should not consume the transient recovery budget by repeatedly producing `spawn git ENOENT`. + - `recoverTransientMergeFailures()` handles retry-exhausted `in-review` merge failures only when `classifyTransientMergeError()` returns a bounded transient class: `lease-handoff-target-not-queued`, `spurious-concurrent-advance-same-sha`, `process-spawn-failure` (`spawn ENOTDIR`, `spawn … ENOENT`, or a clean-room path reported as `is not a working tree`), `ai-provider-turn-failure`, or `network-transport-failure`. + - FN-8004 added the last two classes. `ai-provider-turn-failure` covers ACP-backed merge models (Grok/OMP/generic ACP) whose turn fails provider-side: `promptAcpSession` preserves the JSON-RPC code as `… (acp rpc code -32603, retryable)` and the classifier anchors on that envelope or the ` ACP turn failed:` prefix. Anchoring is deliberate — the bare JSON-RPC text is `Internal error`, which must never match unanchored or it would disguise genuine application defects as retryable blips. Only provider-fault codes (`-32603`, `-32000`…`-32003`) are retryable; caller-fault codes (`-32600`…`-32602`) stay permanent because retrying repeats the failing call. + - `network-transport-failure` is a delegation to `isTransientError()`, closing a real asymmetry: the inline retry gate (`ProjectEngine.maybeRetryTransientMerge`) accepted `isTransientError(msg) || classifyTransientMergeError(msg)`, while this sweep consulted only the classifier. Errors such as `ECONNRESET`/`socket hang up` therefore earned inline retries but became invisible to the sweep once parked `failed` — stranding them permanently. Both gates now share one definition by construction. To keep this delegation from importing the detector's `usage-limit-detector.js → logger.js` chain (the chain FN-5627 split the classifier out to avoid), the pure predicates live in the import-free leaf `transient-error-patterns.ts`, re-exported by `transient-error-detector.ts`. + - Because `status:"failed"` is itself what suppresses both recovery paths, a misclassification here is not merely a missed retry — it is terminal. FN-8004's 20-second Grok blip permanently parked a task whose branch held complete, reviewed work. Recovery resets `mergeRetries`, clears transient `status`/`error`, increments `mergeDetails.transientRecoveryCount`, and requeues auto-merge so the next attempt recreates the AI-merge clean room. The budget stays capped by `MAX_TRANSIENT_MERGE_RECOVERIES`; exhausted tasks remain parked with the `merger:transient-failure-budget-exhausted` audit path so real structural failures cannot loop forever. FN-6278 makes this recovery mostly after-the-fact insurance for cwd spawn faults: the merge runner now preflights reuse integration roots and repairs/reacquires missing or de-registered task worktrees before the first git spawn, so a stale `task.worktree` should not consume the transient recovery budget by repeatedly producing `spawn git ENOENT`. - `reconcileTaskWorktreeMetadata()` (FN-4962) reconciles stale `task.worktree`/`task.branch` rows against authoritative `git worktree list --porcelain` branch mappings during startup recovery, periodic maintenance, and completion fan-out. The stage must run before `reclaim-stale-active-branches`: stale rows rebound to live `fusion/` worktrees emit `task:auto-recover-worktree-metadata-rebound`; stale rows with no live branch mapping are nulled (`worktree=null`, `branch=null`, `baseCommitSha` unchanged) and emit `task:auto-recover-worktree-metadata-cleared`. - `recoverInProgressLimbo()` (FN-5219) is the safety net for stranded executor rows: reset/requeue paths must never leave a task in `in-progress` without a runnable execution context. After metadata reconcile, stale `in-progress` tasks with null branch, missing/cleared worktree metadata, no live executor claim, and all-pending steps are audited and moved back to `todo`. diff --git a/packages/engine/src/__tests__/merge-error-recovery.test.ts b/packages/engine/src/__tests__/merge-error-recovery.test.ts index 5aa7b746cd..abaf584646 100644 --- a/packages/engine/src/__tests__/merge-error-recovery.test.ts +++ b/packages/engine/src/__tests__/merge-error-recovery.test.ts @@ -651,8 +651,13 @@ describe("ProjectEngine merge error recovery", () => { }); it("parks direct merge when transient retry cap is exhausted", async () => { + // FNXC:MergeReliability 2026-07-15-19:25 (FN-8004): seed AT the cap, read from the constant. + // This previously hardcoded 3; raising the budget to 5 silently turned this into a + // "retries once more" case. Deriving the seed keeps the invariant (park once the budget is + // spent) under test regardless of how the budget is tuned. + const atCap = ProjectEngine.MAX_AUTO_MERGE_TRANSIENT_RETRIES; const store = makeStore({ - tasks: [makeTask({ mergeTransientRetryCount: 3 }), makeTask({ mergeTransientRetryCount: 3 })], + tasks: [makeTask({ mergeTransientRetryCount: atCap }), makeTask({ mergeTransientRetryCount: atCap })], }); vi.mocked(runAiMerge).mockRejectedValueOnce(new Error("socket hang up")); diff --git a/packages/engine/src/__tests__/reliability-interactions/dual-observe-merge-seam.test.ts b/packages/engine/src/__tests__/reliability-interactions/dual-observe-merge-seam.test.ts index 9fa1c4c77f..2a90568b09 100644 --- a/packages/engine/src/__tests__/reliability-interactions/dual-observe-merge-seam.test.ts +++ b/packages/engine/src/__tests__/reliability-interactions/dual-observe-merge-seam.test.ts @@ -234,7 +234,10 @@ describe("FN-5742 dual-observe merge seam", () => { getActiveMergingTask: vi.fn().mockReturnValue(null), } as any; - if ((ProjectEngine.prototype as any).isTransientMergeRetryExhausted.call({}, { mergeTransientRetryCount: 3 }, "socket hang up")) { + // FNXC:MergeReliability 2026-07-15-19:25 (FN-8004): derive the exhausted seed from the + // constant. Hardcoding 3 silently inverted this test's premise when the budget rose to 5. + const exhaustedCount = ProjectEngine.MAX_AUTO_MERGE_TRANSIENT_RETRIES; + if ((ProjectEngine.prototype as any).isTransientMergeRetryExhausted.call({}, { mergeTransientRetryCount: exhaustedCount }, "socket hang up")) { const record = store.getMergeRequestRecord("FN-MR"); if (record.state === "running") { store.transitionMergeRequestState("FN-MR", "retrying", { attemptCount: record.attemptCount, lastError: "socket hang up" }); diff --git a/packages/engine/src/__tests__/transient-error-detector.test.ts b/packages/engine/src/__tests__/transient-error-detector.test.ts index 751dc43810..39e1d84868 100644 --- a/packages/engine/src/__tests__/transient-error-detector.test.ts +++ b/packages/engine/src/__tests__/transient-error-detector.test.ts @@ -1,4 +1,8 @@ import { describe, it, expect } from "vitest"; +// FNXC:Reliability-ErrorClassification 2026-07-15-19:15 (FN-8004): the pure predicates moved to +// the import-free leaf `transient-error-patterns.ts`; this module re-exports them. Importing via +// BOTH paths here pins the re-export contract so existing callers keep working. +import { isTransientError as isTransientErrorViaLeaf } from "../transient-error-patterns.js"; import { isTransientError, isTransientAuthCredentialError, @@ -550,4 +554,47 @@ describe("Transient Error Detector", () => { expect(isSilentTransientError("The operation was aborted by user")).toBe(false); }); }); + + /* + FNXC:AcpRuntime 2026-07-15-19:15 (FN-8004): + ACP-backed runtimes (Grok, OMP, generic ACP) surface provider-side turn failures as JSON-RPC + errors. Treating them as permanent parked a task `failed` over a ~20s blip, and since + `status:"failed"` is what suppresses recovery, the work stranded until a human noticed. + + The anchoring is the load-bearing part: the bare JSON-RPC text is "Internal error", which must + NEVER match globally or it would disguise real application defects as retryable blips. + */ + describe("ACP provider turn failures (FN-8004)", () => { + it("treats ACP turn failures as transient across every runtime prefix", () => { + expect(isTransientError("Grok ACP turn failed: Internal error")).toBe(true); + expect(isTransientError("OMP ACP turn failed: Internal error")).toBe(true); + expect(isTransientError("Grok ACP turn failed: Internal error (acp rpc code -32603, retryable)")).toBe(true); + }); + + it("treats retryable ACP rpc codes as transient", () => { + for (const code of [-32603, -32000, -32001, -32002, -32003]) { + expect(isTransientError(`Server error (acp rpc code ${code}, retryable)`)).toBe(true); + } + }); + + it("treats ACP startup and dead-connection diagnostics as transient", () => { + expect(isTransientError("Grok ACP failed to start: spawn grok ENOENT")).toBe(true); + expect(isTransientError("Grok ACP session has no live connection. The `grok agent stdio` process failed to start.")) + .toBe(true); + }); + + it("does NOT match the bare JSON-RPC message or caller-fault codes", () => { + expect(isTransientError("Internal error")).toBe(false); + expect(isTransientError("Application threw Internal error while saving")).toBe(false); + for (const code of [-32600, -32601, -32602]) { + expect(isTransientError(`Bad call (acp rpc code ${code})`)).toBe(false); + } + }); + + it("re-exports the identical predicate from the leaf module", () => { + // The detector must stay a pure re-export — a divergent copy would let the merge + // classifier (which imports the leaf) and the executor drift apart again. + expect(isTransientErrorViaLeaf).toBe(isTransientError); + }); + }); }); diff --git a/packages/engine/src/__tests__/transient-merge-error-classifier.test.ts b/packages/engine/src/__tests__/transient-merge-error-classifier.test.ts index 1b5001c67d..147048c12c 100644 --- a/packages/engine/src/__tests__/transient-merge-error-classifier.test.ts +++ b/packages/engine/src/__tests__/transient-merge-error-classifier.test.ts @@ -39,4 +39,85 @@ describe("classifyTransientMergeError", () => { "Integration branch main advanced concurrently (expected aaa1111aaa1111aaa1111aaa1111aaa1111aaaa, observed bbb2222bbb2222bbb2222bbb2222bbb2222bbbb) while applying ccc3333ccc3333ccc3333ccc3333ccc3333cccc", )).toBeNull(); }); + + /* + FNXC:MergeReliability 2026-07-15-19:00 (FN-8004): + The AI merge drives a real LLM turn, but no provider-side fault was modeled as transient, so a + ~20s Grok `-32603` blip parked a task `failed` with 8 files of finished, reviewed work stranded + in in-review. Because `status:"failed"` is exactly what suppresses both recovery paths, the + misclassification was self-sealing. + + Per "Fix the Invariant, Not the Repro": assert the invariant across EVERY surface that can emit + an ACP provider fault — not just the one reported Grok string. + */ + describe("ai-provider-turn-failure (FN-8004)", () => { + it("classifies the exact error string that terminally failed FN-8004", () => { + // Verbatim from .fusion/tasks/FN-8004/task.json `error` — the pre-fix adapter output. + expect(classifyTransientMergeError("Grok ACP turn failed: Internal error")) + .toBe("ai-provider-turn-failure"); + }); + + it("classifies every ACP-backed runtime's turn-failure prefix", () => { + // Surface enumeration: all ACP adapters funnel through acp-runtime's promptAcpSession. + for (const runtime of ["Grok", "OMP"]) { + expect(classifyTransientMergeError(`${runtime} ACP turn failed: Internal error`)) + .toBe("ai-provider-turn-failure"); + } + }); + + it("classifies post-fix diagnostics carrying the JSON-RPC code envelope", () => { + expect(classifyTransientMergeError("Grok ACP turn failed: Internal error (acp rpc code -32603, retryable)")) + .toBe("ai-provider-turn-failure"); + // Reserved server-error range -32000..-32003 is retryable too. + for (const code of [-32000, -32001, -32002, -32003]) { + expect(classifyTransientMergeError(`Server error (acp rpc code ${code}, retryable)`)) + .toBe("ai-provider-turn-failure"); + } + }); + + it("does NOT swallow the bare JSON-RPC message without an ACP envelope", () => { + // "Internal error" is far too generic to treat as transient globally — matching it + // unanchored would mask real application defects as retryable infrastructure blips. + expect(classifyTransientMergeError("Internal error")).toBeNull(); + expect(classifyTransientMergeError("Application threw Internal error while saving")).toBeNull(); + expect(classifyTransientMergeError("AssertionError: expected Internal error to be handled")).toBeNull(); + }); + + it("does not classify non-retryable JSON-RPC codes (caller bugs, not provider faults)", () => { + // -32600 invalid request / -32601 method not found / -32602 invalid params are OUR bugs; + // retrying them just repeats the failing call. + for (const code of [-32600, -32601, -32602]) { + expect(classifyTransientMergeError(`Bad call (acp rpc code ${code})`)).toBeNull(); + } + }); + }); + + /* + FNXC:MergeReliability 2026-07-15-19:00 (FN-8004): + The inline retry gate (`project-engine.ts#maybeRetryTransientMerge`) accepted + `isTransientError(msg) || classify(msg)`, but the self-healing sweep + (`recoverTransientMergeFailures`) consulted ONLY this classifier. Any network-class error + therefore got inline retries and then became invisible to the sweep once parked `failed`. + Delegating to `isTransientError` makes the two gates agree by construction. + */ + describe("network-transport-failure delegation (FN-8004 asymmetry)", () => { + it("classifies network transport errors the sweep previously could not see", () => { + for (const msg of ["socket hang up", "read ECONNRESET", "connect ECONNREFUSED 127.0.0.1:443", "upstream connect error"]) { + expect(classifyTransientMergeError(msg)).toBe("network-transport-failure"); + } + }); + + it("still returns the specific git/merge class when both could match", () => { + // Ordering invariant: the precise class must win the audit label. + expect(classifyTransientMergeError("Merge handoff refused (lease-handoff-failed): target-not-queued")) + .toBe("lease-handoff-target-not-queued"); + expect(classifyTransientMergeError("spawn git ENOENT")).toBe("process-spawn-failure"); + }); + + it("leaves genuine task defects permanent", () => { + expect(classifyTransientMergeError("Test suite failed: 3 assertions failed")).toBeNull(); + expect(classifyTransientMergeError("FileScopeViolationError: commit touches files outside scope")).toBeNull(); + expect(classifyTransientMergeError("CONFLICT (content): Merge conflict in src/app.ts")).toBeNull(); + }); + }); }); diff --git a/packages/engine/src/project-engine.ts b/packages/engine/src/project-engine.ts index a473ddfd97..c0c6f3a208 100644 --- a/packages/engine/src/project-engine.ts +++ b/packages/engine/src/project-engine.ts @@ -541,9 +541,19 @@ export class ProjectEngine { } /** FN-5697/FN-5674: cap transient provider/network abort retries in auto-merge. - * Examples: "This operation was aborted", "socket hang up", `server_error`. - * After this cap, the task is parked failed for human visibility. */ - private static readonly MAX_AUTO_MERGE_TRANSIENT_RETRIES = 3; + * Examples: "This operation was aborted", "socket hang up", `server_error`, + * and (FN-8004) ACP provider turn failures such as `acp rpc code -32603`. + * After this cap, the task is parked failed for human visibility. + * + * FNXC:MergeReliability 2026-07-15-18:50 (FN-8004): + * Raised 3 → 5. Applies only to errors already classified transient, and each retry + * is spaced by exponential backoff (5s/10s/20s/40s/80s — ~2.5 min total), so the + * widened budget rides out provider incidents lasting minutes rather than seconds + * without meaningfully delaying a genuinely broken merge's park. + * + * Readable (not private) so tests derive the cap from this single source of truth rather + * than hardcoding it — the FN-8004 bump broke two suites that had baked in the old `3`. */ + static readonly MAX_AUTO_MERGE_TRANSIENT_RETRIES = 5; private static readonly MERGE_REQUEST_RETRY_EXHAUSTED_AGE_MS = 30 * 60 * 1000; /** Cap on outer in-review→in-progress bounces caused by deterministic * verification failures during auto-merge. After this many failed merges diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 35de2f10d5..4e64da9148 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -517,8 +517,17 @@ export const MAX_AUTO_MERGE_RETRIES = 3; * `mergeRetries` and re-enqueueing the same task, the task is considered * genuinely stuck and stays parked as `failed` for manual review. Tracked via * `task.mergeDetails.transientRecoveryCount`. + * + * FNXC:MergeReliability 2026-07-15-18:50 (FN-8004): + * Raised 2 → 5. This budget only ever applies to errors already PROVEN transient by + * `classifyTransientMergeError` — provider blips, network drops, lease races. For that + * population the cost asymmetry is lopsided: an extra retry costs one merge attempt, + * while giving up strands completed, reviewed work in `in-review` until a human notices. + * Operators reported the old budget surrendering while the underlying fault was still + * clearing. Recovery remains strictly bounded and audited — this widens the window, + * it does not remove the ceiling. */ -export const MAX_TRANSIENT_MERGE_RECOVERIES = 2; +export const MAX_TRANSIENT_MERGE_RECOVERIES = 5; // FN-5627: classifier extracted to `transient-merge-error-classifier.ts` // to avoid pulling `createLogger` into modules that mock `../logger.js` diff --git a/packages/engine/src/transient-error-detector.ts b/packages/engine/src/transient-error-detector.ts index bbf89995f4..8fcbdf3497 100644 --- a/packages/engine/src/transient-error-detector.ts +++ b/packages/engine/src/transient-error-detector.ts @@ -16,94 +16,16 @@ */ import { isUsageLimitError } from "./usage-limit-detector.js"; +/* +FNXC:Reliability-ErrorClassification 2026-07-15-18:40: +The pure predicates (TRANSIENT_ERROR_PATTERNS / isTransientError / isTransientAuthCredentialError) +now live in the import-free leaf `transient-error-patterns.ts` so the merge classifier can share +one definition of "transient" without inheriting this module's logger chain (FN-8004). +Re-exported here so every existing importer of this module keeps working unchanged. +*/ +import { isTransientAuthCredentialError, isTransientError } from "./transient-error-patterns.js"; +export { TRANSIENT_ERROR_PATTERNS, isTransientAuthCredentialError, isTransientError } from "./transient-error-patterns.js"; -/** - * Patterns that indicate transient network/infrastructure errors. - * These are checked case-insensitively against error messages. - * - * These patterns cover: - * - Proxy/gateway connection errors (upstream connect, disconnect/reset) - * - Connection refusal/reset (ECONNREFUSED, connection reset) - * - Timeouts (ETIMEDOUT, timeout in connection context) - * - Socket errors (socket hang up) - * - Transport layer failures - * - AI provider abort errors (request was aborted — temporary streaming/API cancellations) - * - OpenAI/Codex infrastructure errors surfaced as structured `server_error` payloads - */ -export const TRANSIENT_ERROR_PATTERNS: RegExp[] = [ - // Proxy/gateway errors - indicate temporary routing issues - /upstream connect error/i, - /disconnect\/reset before headers/i, - /retried and the latest reset reason/i, - /remote connection failure/i, - /transport failure reason/i, - /delayed connect error/i, - - // Connection establishment failures - usually temporary - /Connection refused/i, - /connection reset/i, - /ECONNRESET/i, - /ECONNREFUSED/i, - /ETIMEDOUT/i, - /socket hang up/i, - - // Timeout patterns (only when related to connections, not general timeouts) - /timeout.*connection/i, - /connection.*timeout/i, - - // AI provider abort errors — temporary request cancellations (e.g., Anthropic streaming aborts) - // These occur when the provider's infrastructure drops an in-flight request. - /request was aborted/i, - // DOMException-style AbortError ("This operation was aborted"), emitted by fetch/ - // AbortController when a provider drops an in-flight operation. Excludes user- - // initiated cancellations like "operation was aborted by user" — those are not transient. - /operation was aborted(?!\s+by\b)/i, - - // OpenAI/Codex structured infrastructure failures. These arrive as JSON-ish payloads - // like {"type":"error","error":{"type":"server_error","code":"server_error",...}} - // and are temporary service-side failures rather than task-specific defects. - /"type":"server_error"/i, - /"code":"server_error"/i, - /An error occurred while processing your request\./i, - - // pi-ai openai-codex-responses WebSocket transport errors. The provider holds - // a long-lived WebSocket to the Codex backend; transient drops surface as - // bare "WebSocket error" / "WebSocket closed " / a half-open - // stream that ended before `response.completed`. All three are network-layer - // hiccups, not task defects — retry them. - /WebSocket error\b/i, - /WebSocket closed\b/i, - /WebSocket stream closed before response\.completed/i, -]; - -/** - * Check if an error message indicates a transient network/infrastructure error. - * - * Transient errors are temporary conditions that typically resolve after a delay: - * - Network blips and temporary routing issues - * - Proxy/gateway hiccups (upstream connect errors) - * - Connection resets during establishment - * - Temporary service unavailability (connection refused) - * - Socket timeouts during connection - * - * Returns `true` for transient errors — these should trigger a retry by moving - * the task back to "todo" rather than marking as "failed". - * - * Returns `false` for permanent failures (code errors, test failures) or - * usage limit errors (rate limits that need global pause). - * - * @param errorMessage - The error message to classify - * @returns true if the error appears transient and retryable - */ -export function isTransientError(errorMessage: string): boolean { - if (!errorMessage || typeof errorMessage !== "string") { - return false; - } - if (isTransientAuthCredentialError(errorMessage)) { - return true; - } - return TRANSIENT_ERROR_PATTERNS.some((pattern) => pattern.test(errorMessage)); -} /* * FNXC:PlanReviewReplan 2026-07-15-12:00: @@ -141,34 +63,6 @@ export function isNonPlanDefectPlanReviewFailure(input: { ); } -/* -FNXC:Reliability-ErrorClassification 2026-07-12-20:10: -A long-running agent session holds its OAuth access token in memory. Claude Max access tokens rotate mid-run (~8 h lifetime); the in-flight call fails with a 401 {"type":"authentication_error","message":"Invalid authentication credentials"} even though the credentials file has already been refreshed, and the very next call succeeds. These must classify as TRANSIENT (retryable) and NOT operator-actionable, so in-run retry (withRateLimitRetry) and durable-agent heartbeat error recovery (FN-7835/FN-7844/FN-7859) auto-recover instead of parking agents paused with pauseReason "error-unrecoverable". Previously the message matched the operator-actionable /credential/ and /unauthorized/ patterns and defaulted to "permanent", so a routine token rotation parked every durable agent for manual operator repair. -Genuinely operator-actionable auth failures are excluded first: OAuth scope/permission-grant errors (token valid but lacks grants) and explicit API-key problems (invalid/missing x-api-key) — retrying those only repeats the failing call. -*/ -const TRANSIENT_AUTH_CREDENTIAL_ROTATION_PATTERN = - /"type":\s*"authentication_error"|invalid authentication credentials|token[_\s]?expired/i; -/* -FNXC:Reliability-ErrorClassification 2026-07-12-21:05: -PR #2027 review: the `"type":"authentication_error"` envelope is intentionally broad (providers put rotation failures behind it with varying messages), so the exclusion list must carry the operator-actionable load. Beyond scope grants and invalid/missing API keys, exclude account/credential states no retry can fix: revoked/suspended/disabled/deactivated keys or accounts and inactive subscriptions. A message matching any of these stays permanent/operator-actionable even inside an authentication_error envelope; retries are pointless and would un-park agents a human must repair. Unmatched novel auth messages still classify transient, but the bounded heartbeat error-recovery budget re-parks them as `error-retry-exhausted` after a few attempts, so the failure mode is a handful of visible retries, not an unpark loop. -*/ -const OPERATOR_ACTIONABLE_AUTH_EXCLUSION_PATTERN = - /oauth token does not meet scope|insufficient[_\s-]?scope|invalid[_\s-]?scope|invalid (?:api[_\s-]?key|x-api-key)|missing\s+(?:\S+\s+)?(?:api[_\s-]?)?key|revoked|suspend(?:ed)?|disabled|deactivated|subscription|account (?:is )?(?:locked|closed|inactive)|access denied/i; - -/** - * Detect a transient authentication failure caused by credential rotation - * (e.g. a Claude Max OAuth access token expiring mid-run). Scope-grant and - * API-key misconfiguration errors are excluded — those need operator action. - */ -export function isTransientAuthCredentialError(errorMessage: string): boolean { - if (!errorMessage || typeof errorMessage !== "string") { - return false; - } - if (OPERATOR_ACTIONABLE_AUTH_EXCLUSION_PATTERN.test(errorMessage)) { - return false; - } - return TRANSIENT_AUTH_CREDENTIAL_ROTATION_PATTERN.test(errorMessage); -} /** * Patterns for transient errors that should be silently retried without diff --git a/packages/engine/src/transient-error-patterns.ts b/packages/engine/src/transient-error-patterns.ts new file mode 100644 index 0000000000..f472bee558 --- /dev/null +++ b/packages/engine/src/transient-error-patterns.ts @@ -0,0 +1,153 @@ +/** + * Pure transient-error predicates — NO module imports. + * + * FNXC:Reliability-ErrorClassification 2026-07-15-18:40: + * Extracted from `transient-error-detector.ts` (FN-8004) so that + * `transient-merge-error-classifier.ts` can share ONE definition of "transient" + * without inheriting the detector's `usage-limit-detector.js → logger.js` import + * chain. FN-5627 originally split the merge classifier out precisely to keep that + * chain away from consumers whose tests `vi.mock("../logger.js")` with a partial + * surface (notification-service.test.ts) — importing the detector directly would + * have silently reintroduced it. + * + * INVARIANT: this module must stay import-free. Anything needing `isUsageLimitError` + * or a logger belongs in `transient-error-detector.ts`, not here. + * + * `transient-error-detector.ts` re-exports every symbol below, so existing importers + * are unaffected and may continue importing from either module. + */ + +/** + * Patterns that indicate transient network/infrastructure errors. + * These are checked case-insensitively against error messages. + * + * These patterns cover: + * - Proxy/gateway connection errors (upstream connect, disconnect/reset) + * - Connection refusal/reset (ECONNREFUSED, connection reset) + * - Timeouts (ETIMEDOUT, timeout in connection context) + * - Socket errors (socket hang up) + * - Transport layer failures + * - AI provider abort errors (request was aborted — temporary streaming/API cancellations) + * - OpenAI/Codex infrastructure errors surfaced as structured `server_error` payloads + */ +export const TRANSIENT_ERROR_PATTERNS: RegExp[] = [ + // Proxy/gateway errors - indicate temporary routing issues + /upstream connect error/i, + /disconnect\/reset before headers/i, + /retried and the latest reset reason/i, + /remote connection failure/i, + /transport failure reason/i, + /delayed connect error/i, + + // Connection establishment failures - usually temporary + /Connection refused/i, + /connection reset/i, + /ECONNRESET/i, + /ECONNREFUSED/i, + /ETIMEDOUT/i, + /socket hang up/i, + + // Timeout patterns (only when related to connections, not general timeouts) + /timeout.*connection/i, + /connection.*timeout/i, + + // AI provider abort errors — temporary request cancellations (e.g., Anthropic streaming aborts) + // These occur when the provider's infrastructure drops an in-flight request. + /request was aborted/i, + // DOMException-style AbortError ("This operation was aborted"), emitted by fetch/ + // AbortController when a provider drops an in-flight operation. Excludes user- + // initiated cancellations like "operation was aborted by user" — those are not transient. + /operation was aborted(?!\s+by\b)/i, + + // OpenAI/Codex structured infrastructure failures. These arrive as JSON-ish payloads + // like {"type":"error","error":{"type":"server_error","code":"server_error",...}} + // and are temporary service-side failures rather than task-specific defects. + /"type":"server_error"/i, + /"code":"server_error"/i, + /An error occurred while processing your request\./i, + + // pi-ai openai-codex-responses WebSocket transport errors. The provider holds + // a long-lived WebSocket to the Codex backend; transient drops surface as + // bare "WebSocket error" / "WebSocket closed " / a half-open + // stream that ended before `response.completed`. All three are network-layer + // hiccups, not task defects — retry them. + /WebSocket error\b/i, + /WebSocket closed\b/i, + /WebSocket stream closed before response\.completed/i, + + /* + FNXC:AcpRuntime 2026-07-15-18:25: + ACP-backed runtimes (Grok, OMP, generic ACP) surface provider-side turn failures as JSON-RPC + errors. `provider.ts#describeAcpTurnError` renders these as `... (acp rpc code -32603, retryable)`; + the adapters wrap that as ` ACP turn failed: ...`. Both signatures are matched here. + + Anchoring is deliberate: the bare JSON-RPC text is "Internal error", far too generic to match + globally (it would swallow unrelated application failures and mask real defects). We only treat + it as transient when it carries the ACP rpc-code envelope or the adapter's turn-failure prefix. + + FN-8004: a Grok `-32603` blip during AI merge was classified permanent, parked the task `failed`, + and — because `status:"failed"` is what suppresses recovery — stranded 8 files of finished work. + */ + /\bacp rpc code -32(?:603|00[0-3])\b/i, + /\bACP turn failed\b/i, + /\bACP failed to start\b/i, + /\bACP session has no live connection\b/i, +]; + +/** + * Check if an error message indicates a transient network/infrastructure error. + * + * Transient errors are temporary conditions that typically resolve after a delay: + * - Network blips and temporary routing issues + * - Proxy/gateway hiccups (upstream connect errors) + * - Connection resets during establishment + * - Temporary service unavailability (connection refused) + * - Socket timeouts during connection + * + * Returns `true` for transient errors — these should trigger a retry by moving + * the task back to "todo" rather than marking as "failed". + * + * Returns `false` for permanent failures (code errors, test failures) or + * usage limit errors (rate limits that need global pause). + * + * @param errorMessage - The error message to classify + * @returns true if the error appears transient and retryable + */ +export function isTransientError(errorMessage: string): boolean { + if (!errorMessage || typeof errorMessage !== "string") { + return false; + } + if (isTransientAuthCredentialError(errorMessage)) { + return true; + } + return TRANSIENT_ERROR_PATTERNS.some((pattern) => pattern.test(errorMessage)); +} + +/* +FNXC:Reliability-ErrorClassification 2026-07-12-20:10: +A long-running agent session holds its OAuth access token in memory. Claude Max access tokens rotate mid-run (~8 h lifetime); the in-flight call fails with a 401 {"type":"authentication_error","message":"Invalid authentication credentials"} even though the credentials file has already been refreshed, and the very next call succeeds. These must classify as TRANSIENT (retryable) and NOT operator-actionable, so in-run retry (withRateLimitRetry) and durable-agent heartbeat error recovery (FN-7835/FN-7844/FN-7859) auto-recover instead of parking agents paused with pauseReason "error-unrecoverable". Previously the message matched the operator-actionable /credential/ and /unauthorized/ patterns and defaulted to "permanent", so a routine token rotation parked every durable agent for manual operator repair. +Genuinely operator-actionable auth failures are excluded first: OAuth scope/permission-grant errors (token valid but lacks grants) and explicit API-key problems (invalid/missing x-api-key) — retrying those only repeats the failing call. +*/ +const TRANSIENT_AUTH_CREDENTIAL_ROTATION_PATTERN = + /"type":\s*"authentication_error"|invalid authentication credentials|token[_\s]?expired/i; +/* +FNXC:Reliability-ErrorClassification 2026-07-12-21:05: +PR #2027 review: the `"type":"authentication_error"` envelope is intentionally broad (providers put rotation failures behind it with varying messages), so the exclusion list must carry the operator-actionable load. Beyond scope grants and invalid/missing API keys, exclude account/credential states no retry can fix: revoked/suspended/disabled/deactivated keys or accounts and inactive subscriptions. A message matching any of these stays permanent/operator-actionable even inside an authentication_error envelope; retries are pointless and would un-park agents a human must repair. Unmatched novel auth messages still classify transient, but the bounded heartbeat error-recovery budget re-parks them as `error-retry-exhausted` after a few attempts, so the failure mode is a handful of visible retries, not an unpark loop. +*/ +const OPERATOR_ACTIONABLE_AUTH_EXCLUSION_PATTERN = + /oauth token does not meet scope|insufficient[_\s-]?scope|invalid[_\s-]?scope|invalid (?:api[_\s-]?key|x-api-key)|missing\s+(?:\S+\s+)?(?:api[_\s-]?)?key|revoked|suspend(?:ed)?|disabled|deactivated|subscription|account (?:is )?(?:locked|closed|inactive)|access denied/i; + +/** + * Detect a transient authentication failure caused by credential rotation + * (e.g. a Claude Max OAuth access token expiring mid-run). Scope-grant and + * API-key misconfiguration errors are excluded — those need operator action. + */ +export function isTransientAuthCredentialError(errorMessage: string): boolean { + if (!errorMessage || typeof errorMessage !== "string") { + return false; + } + if (OPERATOR_ACTIONABLE_AUTH_EXCLUSION_PATTERN.test(errorMessage)) { + return false; + } + return TRANSIENT_AUTH_CREDENTIAL_ROTATION_PATTERN.test(errorMessage); +} diff --git a/packages/engine/src/transient-merge-error-classifier.ts b/packages/engine/src/transient-merge-error-classifier.ts index 3d66546cc9..c1319343bf 100644 --- a/packages/engine/src/transient-merge-error-classifier.ts +++ b/packages/engine/src/transient-merge-error-classifier.ts @@ -42,12 +42,41 @@ * stale temp merge checkout), not that the task branch's code failed. A * fresh merge attempt gets a fresh/revalidated worktree, so the self-healing * sweep can recover these within its bounded retry budget. + * + * - `ai-provider-turn-failure`: the AI merge's LLM turn failed provider-side + * (FN-8004). The merger drives a real model to resolve/compose the squash; + * when that provider returns an internal/server error, the *merge* failed but + * the task branch is untouched and a fresh attempt typically succeeds. Before + * FN-8004 no provider fault was modeled here at all, so every one of them was + * treated as a permanent defect. + * + * - `network-transport-failure`: delegated to `isTransientError` (see below). */ +// Imports the import-free leaf, NOT `transient-error-detector.js` — that module pulls +// `usage-limit-detector.js → logger.js`, the exact chain FN-5627 split this file out to avoid. +import { isTransientError } from "./transient-error-patterns.js"; + +/* +FNXC:MergeReliability 2026-07-15-18:30: +This classifier used to recognize only git/lease/spawn faults, while the inline retry gate in +`project-engine.ts#maybeRetryTransientMerge` accepted `isTransientError(msg) || classify(msg)`. +The self-healing sweep (`recoverTransientMergeFailures`) consulted ONLY this classifier — so any +network-class error (ECONNRESET, socket hang up, WebSocket drop) got inline retries but became +invisible to the sweep once parked `failed`, stranding it forever. + +Delegating to `isTransientError` here makes the two gates agree by construction. Both consumers +now see one definition of "transient", which is what the FN-5627 header above already claimed. +*/ export function classifyTransientMergeError(error: string | null | undefined): string | null { if (!error) return null; if (/lease-handoff-failed[^a-z]+target-not-queued/i.test(error)) { return "lease-handoff-target-not-queued"; } + // FNXC:MergeReliability 2026-07-15-18:30 (FN-8004): AI-merge provider faults precede the + // generic network check so the more specific class wins in the audit/log trail. + if (/\bACP turn failed\b/i.test(error) || /\bacp rpc code -32(?:603|00[0-3])\b/i.test(error)) { + return "ai-provider-turn-failure"; + } if (/\bspawn(?:\s+\S+)?\s+ENO(?:TDIR|ENT)\b/i.test(error)) { return "process-spawn-failure"; } @@ -58,5 +87,10 @@ export function classifyTransientMergeError(error: string | null | undefined): s if (sameSha && sameSha[1].toLowerCase() === sameSha[2].toLowerCase()) { return "spurious-concurrent-advance-same-sha"; } + // FNXC:MergeReliability 2026-07-15-18:30 (FN-8004): last — the specific git/merge classes above + // must win the label. This aligns the sweep with the inline retry gate (see header). + if (isTransientError(error)) { + return "network-transport-failure"; + } return null; } diff --git a/plugins/fusion-plugin-acp-runtime/src/__tests__/acp-turn-error.test.ts b/plugins/fusion-plugin-acp-runtime/src/__tests__/acp-turn-error.test.ts new file mode 100644 index 0000000000..977892ecbe --- /dev/null +++ b/plugins/fusion-plugin-acp-runtime/src/__tests__/acp-turn-error.test.ts @@ -0,0 +1,112 @@ +/* +FNXC:AcpRuntime 2026-07-15-19:10 (FN-8004): +An ACP turn failing server-side arrives as a JSON-RPC error whose `message` is the bare +protocol-standard text — `-32603` renders as literally "Internal error". Rethrowing it unchanged +discarded `code`/`data`, the only evidence the fault was provider-side and retryable. The engine's +transient classifier then saw an unclassifiable string and parked the task permanently, stranding +completed work (FN-8004: a ~20s Grok blip terminally failed an auto-merge). + +These tests pin the diagnostic SHAPE, because `transient-error-patterns.ts` and +`transient-merge-error-classifier.ts` match on it. Changing the format here without updating those +regexes silently reintroduces the FN-8004 stranding — the string is a cross-package contract. +*/ +import { describe, expect, it } from "vitest"; +import { describeAcpTurnError, inspectAcpTurnError, promptAcpSession } from "../provider.js"; + +/** Flat `{ code, message, data }` — one of two shapes the SDK throws depending on build. */ +function flatRpcError(code: number, message: string, data?: unknown): Error { + return Object.assign(new Error(message), { code, ...(data === undefined ? {} : { data }) }); +} + +/** Nested `{ error: { code, message } }` — the other observed SDK shape. */ +function nestedRpcError(code: number, message: string, data?: unknown): Error { + return Object.assign(new Error("request failed"), { + error: { code, message, ...(data === undefined ? {} : { data }) }, + }); +} + +describe("inspectAcpTurnError", () => { + it("extracts code and retryability from a flat JSON-RPC error", () => { + const detail = inspectAcpTurnError(flatRpcError(-32603, "Internal error")); + expect(detail).toMatchObject({ message: "Internal error", code: -32603, retryable: true }); + }); + + it("extracts code from the nested { error: { ... } } shape", () => { + const detail = inspectAcpTurnError(nestedRpcError(-32603, "Internal error")); + expect(detail).toMatchObject({ message: "Internal error", code: -32603, retryable: true }); + }); + + it("marks caller-fault codes non-retryable", () => { + // -32600/-32601/-32602 mean WE sent a bad request; retrying repeats the failure. + for (const code of [-32600, -32601, -32602]) { + expect(inspectAcpTurnError(flatRpcError(code, "Invalid request")).retryable).toBe(false); + } + }); + + it("marks the provider server-error range retryable", () => { + for (const code of [-32603, -32000, -32001, -32002, -32003]) { + expect(inspectAcpTurnError(flatRpcError(code, "Server error")).retryable).toBe(true); + } + }); + + it("degrades gracefully on non-RPC errors and non-Error throws", () => { + expect(inspectAcpTurnError(new Error("boom"))).toMatchObject({ message: "boom", code: undefined, retryable: false }); + expect(inspectAcpTurnError("plain string")).toMatchObject({ message: "plain string", retryable: false }); + expect(inspectAcpTurnError(null)).toMatchObject({ message: "unknown error", retryable: false }); + }); +}); + +describe("describeAcpTurnError", () => { + it("renders the rpc code so downstream classifiers can anchor on it", () => { + expect(describeAcpTurnError(flatRpcError(-32603, "Internal error"))) + .toBe("Internal error (acp rpc code -32603, retryable)"); + }); + + it("omits the retryable marker for caller-fault codes", () => { + expect(describeAcpTurnError(flatRpcError(-32602, "Invalid params"))) + .toBe("Invalid params (acp rpc code -32602)"); + }); + + it("appends structured data when the provider supplies a cause", () => { + const out = describeAcpTurnError(flatRpcError(-32603, "Internal error", { reason: "upstream timeout" })); + expect(out).toContain("acp rpc code -32603, retryable"); + expect(out).toContain('[data: {"reason":"upstream timeout"}]'); + }); + + it("leaves plain errors untouched so unrelated failures are not disguised as ACP faults", () => { + expect(describeAcpTurnError(new Error("Test suite failed"))).toBe("Test suite failed"); + }); + + it("survives unserializable data without throwing", () => { + const circular: Record = {}; + circular.self = circular; + expect(() => describeAcpTurnError(flatRpcError(-32603, "Internal error", circular))).not.toThrow(); + }); + + it("bounds oversized data payloads", () => { + const out = describeAcpTurnError(flatRpcError(-32603, "Internal error", { blob: "x".repeat(5_000) })); + expect(out.length).toBeLessThan(700); + }); +}); + +describe("promptAcpSession error propagation", () => { + const session = { conn: { prompt: async () => ({ stopReason: "end_turn" }) } }; + + it("returns the stopReason on success", async () => { + await expect(promptAcpSession(session as never, "s1", [])).resolves.toBe("end_turn"); + }); + + it("rethrows provider faults with the rpc code preserved in the message", async () => { + const failing = { conn: { prompt: async () => { throw flatRpcError(-32603, "Internal error"); } } }; + // The regression: this message previously read only "Internal error". + await expect(promptAcpSession(failing as never, "s1", [])).rejects.toThrow( + "Internal error (acp rpc code -32603, retryable)", + ); + }); + + it("retains the original error as `cause` for debugging", async () => { + const original = flatRpcError(-32603, "Internal error"); + const failing = { conn: { prompt: async () => { throw original; } } }; + await expect(promptAcpSession(failing as never, "s1", [])).rejects.toMatchObject({ cause: original }); + }); +}); diff --git a/plugins/fusion-plugin-acp-runtime/src/provider.ts b/plugins/fusion-plugin-acp-runtime/src/provider.ts index b604535372..d98f72fb57 100644 --- a/plugins/fusion-plugin-acp-runtime/src/provider.ts +++ b/plugins/fusion-plugin-acp-runtime/src/provider.ts @@ -489,20 +489,112 @@ export async function newAcpSession( return { sessionId: boundIdentifier(res.sessionId), modes: res.modes ?? undefined }; } +/* +FNXC:AcpRuntime 2026-07-15-18:20: +An ACP turn that fails server-side arrives as a JSON-RPC error object whose `message` is the +bare protocol-standard text — `-32603` renders as literally "Internal error". Rethrowing the +SDK error as-is discards `code`/`data`, the only fields identifying the fault as provider-side +and retryable. Downstream transient classification then sees an unclassifiable string and parks +the task permanently (FN-8004: a 20s Grok blip terminally failed an auto-merge). + +`describeAcpTurnError` re-shapes the error into a stable, greppable diagnostic carrying the +numeric code, so classifiers anchor on an ACP-specific signature instead of pattern-matching the +dangerously generic phrase "Internal error" (which could appear in unrelated application output). +Format is load-bearing — `ACP_TRANSIENT_ERROR_PATTERNS` in the engine's transient-error-detector +matches it. Keep the two in sync. +*/ + +/** JSON-RPC error codes that indicate a provider-side, retryable fault rather than a bad request. */ +const RETRYABLE_JSONRPC_CODES = new Set([ + -32603, // Internal error — the agent blew up server-side. + -32000, // Server error (generic, reserved implementation-defined range). + -32001, + -32002, + -32003, +]); + +/** Structured view of a thrown ACP/JSON-RPC error; all fields best-effort. */ +export interface AcpTurnErrorDetail { + message: string; + code?: number; + data?: unknown; + retryable: boolean; +} + +/** + * Extract JSON-RPC `code`/`data` from a thrown ACP error. + * + * The SDK surfaces request errors in more than one shape depending on build: a flat + * `{ code, message, data }` and a nested `{ error: { code, message, data } }`. Read both. + */ +export function inspectAcpTurnError(error: unknown): AcpTurnErrorDetail { + type RpcShape = { code?: unknown; data?: unknown; message?: unknown }; + const raw = error as (RpcShape & { error?: RpcShape }) | null; + const nested = raw && typeof raw === "object" ? raw.error : undefined; + + /* + Read message/code/data from ONE source. Reading `code` from the nested payload while taking + `message` from the outer Error yields "request failed (acp rpc code -32603)" — the outer + wrapper text, not the provider's actual reason. The flat shape wins when it carries a numeric + code; otherwise the nested envelope is authoritative. + */ + const source: RpcShape | undefined = + typeof raw?.code === "number" ? raw : typeof nested?.code === "number" ? nested : undefined; + + const code = typeof source?.code === "number" ? source.code : undefined; + const data = source?.data; + const message = + (typeof source?.message === "string" && source.message) + || (typeof raw?.message === "string" && raw.message) + || (error instanceof Error ? error.message : String(error ?? "unknown error")); + + return { message, code, data, retryable: code !== undefined && RETRYABLE_JSONRPC_CODES.has(code) }; +} + +/** + * Render a thrown ACP turn error as a single-line diagnostic that preserves the JSON-RPC code + * and any `data` payload, so transient classification has something to anchor on. + * + * Shape: `Internal error (acp rpc code -32603, retryable) [data: {...}]` + */ +export function describeAcpTurnError(error: unknown): string { + const { message, code, data, retryable } = inspectAcpTurnError(error); + if (code === undefined) return message; + const suffix = retryable ? ", retryable" : ""; + let out = `${message} (acp rpc code ${code}${suffix})`; + if (data !== undefined) { + let rendered: string; + try { + rendered = typeof data === "string" ? data : JSON.stringify(data); + } catch { + rendered = String(data); + } + if (rendered && rendered !== "{}") out += ` [data: ${rendered.slice(0, 500)}]`; + } + return out; +} + /** * Send a prompt turn via `session/prompt` and return the terminal `stopReason`. * * The SDK prompt promise resolves only AFTER every `session/update` for the turn * has been delivered to the client handler — so resolving here is the correct * "turn complete" signal (no extra draining required). + * + * FNXC:AcpRuntime 2026-07-15-18:20: rethrows with `describeAcpTurnError` so the JSON-RPC code + * survives to the engine's transient classifier (FN-8004). `cause` retains the original error. */ export async function promptAcpSession( connection: AcpConnection, sessionId: string, blocks: ContentBlock[], ): Promise { - const res = await connection.conn.prompt({ sessionId, prompt: blocks }); - return res.stopReason; + try { + const res = await connection.conn.prompt({ sessionId, prompt: blocks }); + return res.stopReason; + } catch (error) { + throw new Error(describeAcpTurnError(error), { cause: error }); + } } /** diff --git a/plugins/fusion-plugin-grok-runtime/src/runtime-adapter.ts b/plugins/fusion-plugin-grok-runtime/src/runtime-adapter.ts index 727c69c165..749509e053 100644 --- a/plugins/fusion-plugin-grok-runtime/src/runtime-adapter.ts +++ b/plugins/fusion-plugin-grok-runtime/src/runtime-adapter.ts @@ -89,6 +89,16 @@ function describeCreateFailure(error: unknown): string { ); } +/* +FNXC:GrokAcp 2026-07-15-18:45: +`promptAcpSession` (acp-runtime provider.ts) already re-shapes JSON-RPC faults into a diagnostic +carrying the rpc code — e.g. `Internal error (acp rpc code -32603, retryable)`. Pass that through +verbatim rather than re-flattening to `error.message`, so the engine's transient classifier can +recognize a provider-side blip and retry instead of parking the task permanently. + +FN-8004: the bare message reaching the merger was "Internal error", matched no transient pattern, +and terminally failed an auto-merge whose branch work was complete and correct. +*/ function describePromptFailure(error: unknown): string { const reason = error instanceof Error ? error.message : String(error ?? "unknown error"); return compactDiagnostic(`Grok ACP turn failed: ${reason}`);