From cbe07ee86b4e730fc9590f045992f0c972df45d2 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 12 Jul 2026 13:41:39 -0700 Subject: [PATCH] =?UTF-8?q?fix(engine):=20address=20PR=20#2027=20review=20?= =?UTF-8?q?=E2=80=94=20tighten=20auth=20exclusions,=20accurate=20park=20ac?= =?UTF-8?q?counting?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Exclude revoked/suspended/disabled/deactivated keys, inactive subscriptions, and locked accounts from the transient-auth classifier: no retry fixes those, so they stay operator-actionable even inside an authentication_error envelope. - Self-healing sweep logs unrecoverable-error parks separately from recovered-to-active agents (return value still counts actions taken). - Document same-session retry continuation semantics at the heartbeat withRateLimitRetry call site (side-effect replay concern). Co-Authored-By: Claude Fable 5 --- .../__tests__/transient-error-detector.test.ts | 15 +++++++++++++++ packages/engine/src/agent-heartbeat.ts | 3 +++ packages/engine/src/self-healing.ts | 12 ++++++++++-- packages/engine/src/transient-error-detector.ts | 6 +++++- 4 files changed, 33 insertions(+), 3 deletions(-) diff --git a/packages/engine/src/__tests__/transient-error-detector.test.ts b/packages/engine/src/__tests__/transient-error-detector.test.ts index 2b3d59b3b5..2d3d63e67f 100644 --- a/packages/engine/src/__tests__/transient-error-detector.test.ts +++ b/packages/engine/src/__tests__/transient-error-detector.test.ts @@ -461,6 +461,21 @@ describe("Transient Error Detector", () => { expect(isOperatorActionableAgentError("missing OPENAI_API_KEY")).toBe(true); }); + it("keeps revoked/suspended/subscription credential states operator-actionable inside an authentication_error envelope (PR #2027 review)", () => { + const shapes = [ + '401 {"type":"error","error":{"type":"authentication_error","message":"Access denied: API key revoked"}}', + '401 {"type":"error","error":{"type":"authentication_error","message":"account suspended"}}', + '401 {"type":"error","error":{"type":"authentication_error","message":"subscription inactive"}}', + '401 {"type":"error","error":{"type":"authentication_error","message":"this API key has been disabled"}}', + '401 {"type":"error","error":{"type":"authentication_error","message":"credentials deactivated"}}', + '401 {"type":"error","error":{"type":"authentication_error","message":"account is locked"}}', + ]; + for (const shape of shapes) { + expect(isTransientAuthCredentialError(shape)).toBe(false); + expect(classifyError(shape)).toBe("permanent"); + } + }); + it("does not match unrelated auth failures or empty input", () => { expect(isTransientAuthCredentialError("Authentication failed for provider")).toBe(false); expect(isOperatorActionableAgentError("Authentication failed for provider")).toBe(true); diff --git a/packages/engine/src/agent-heartbeat.ts b/packages/engine/src/agent-heartbeat.ts index a1bbcdf53e..bdb9e8811d 100644 --- a/packages/engine/src/agent-heartbeat.ts +++ b/packages/engine/src/agent-heartbeat.ts @@ -3425,6 +3425,9 @@ export class HeartbeatMonitor { /* FNXC:AgentHeartbeat 2026-07-12-20:10: Heartbeat prompts must run under the same rate-limit + transient-auth retry wrapper as executor/triage/merger work. Claude Max OAuth tokens rotate mid-run (~8 h); the in-flight call 401s ("authentication_error: Invalid authentication credentials") even though refreshed credentials already exist, and the next attempt succeeds. Without this wrapper a routine token rotation failed the run, pushed every durable agent to `error`, and (via FN-7859 unrecoverable classification) parked them paused for operator action. Retrying in-run prevents the error state at the source; the durable-agent error-recovery budget stays the backstop for errors that escape. + + FNXC:AgentHeartbeat 2026-07-12-21:05: + PR #2027 review (side-effect replay): the retry re-prompts the SAME session, whose transcript already contains any tool calls completed before the failure, so the model continues from its partial work rather than blindly re-executing it — the same continuation semantics executor/triage/merger rely on under this wrapper. A rotation 401 additionally fails on the turn's FIRST provider call (the stale token never reaches a tool call), so the dominant retry case has no partial work to duplicate. */ await withRateLimitRetry(() => promptWithFallback(session, executionPrompt), { onRetry: (attempt, delayMs, retryError) => { diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 57e0b49757..1c7a5f015d 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -10555,6 +10555,11 @@ export class SelfHealingManager { } let recovered = 0; + /* + FNXC:AgentHeartbeat 2026-07-12-21:05: + PR #2027 review: unrecoverable-error parks are a handled outcome of the sweep (the return value counts actions taken, preserving the existing caller contract), but they are NOT recoveries to active — the summary log must say "parked for operator action", never fold them into "→ active", or maintenance logs misreport agents that still need manual repair. + */ + let parkedUnrecoverable = 0; for (const agent of orphaned) { const updatedAt = Date.parse(agent.updatedAt ?? ""); const stuckForMs = Math.max(0, now - updatedAt); @@ -10595,7 +10600,7 @@ export class SelfHealingManager { source: "self-healing", }); log.warn(`Suppressed durable-agent auto-restart for ${agent.id}: unrecoverable heartbeat error; paused for operator action`); - recovered++; + parkedUnrecoverable++; continue; } if (isStaleMissingModule) { @@ -10707,7 +10712,10 @@ export class SelfHealingManager { if (recovered > 0) { log.log(`Recovered ${recovered} orphaned agent(s) → active`); } - return recovered; + if (parkedUnrecoverable > 0) { + log.warn(`Parked ${parkedUnrecoverable} durable agent(s) with unrecoverable errors for operator action (not recovered)`); + } + return recovered + parkedUnrecoverable; } catch (err: unknown) { const errorMessage = err instanceof Error ? err.message : String(err); log.error(`Orphaned agent recovery failed: ${errorMessage}`); diff --git a/packages/engine/src/transient-error-detector.ts b/packages/engine/src/transient-error-detector.ts index 413dcc52da..334efd1f7a 100644 --- a/packages/engine/src/transient-error-detector.ts +++ b/packages/engine/src/transient-error-detector.ts @@ -111,8 +111,12 @@ Genuinely operator-actionable auth failures are excluded first: OAuth scope/perm */ 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/i; + /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