Jelajahi Sumber

fix: close recovery review gaps

Tianyi Cui 1 bulan lalu
induk
melakukan
cc2e14f76e

+ 1 - 1
docs/core-data-structures/session.md

@@ -430,7 +430,7 @@ declare class Session {
 - `context/message` → a user-role message carrying its `content` verbatim at its chronological position. Optional JSON `meta` remains in the event log and is never rendered.
 - `steering/message` → a user-role message carrying its content verbatim at its chronological position.
 
-Everything else (`turn/*`, `step/*`, plugin-owned `llm/retry`) is structural and does not project into a message. Token usage is observed on `assistant/message.usage` (the step that produced it); an operational error's step number is on `turn/end.reason` for `kind: 'error'`, with normalized `LlmFailure` facts for a final model-request failure and message/code for other live errors. Because this unreleased format intentionally has no compatibility promise, seed/load validation rejects request headers without provider+model and assistant messages without provider/model provenance instead of guessing a route for historical data.
+Everything else (`turn/*`, `step/*`, plugin-owned `llm/retry`) is structural and does not project into a message. Token accounting reads per-step `assistant/chunk { type: 'usage' }` records and treats `assistant/message.usage` as the committed-step fallback when no usage chunk exists; failed model-request attempts have no assistant message, so their usage chunk is the durable accounting record. An operational error's step number is on `turn/end.reason` for `kind: 'error'`, with normalized `LlmFailure` facts for a final model-request failure and message/code for other live errors. Because this unreleased format intentionally has no compatibility promise, seed/load validation rejects request headers without provider+model and assistant messages without provider/model provenance instead of guessing a route for historical data.
 
 ## Live-session fork API
 

+ 8 - 2
packages/core/agent-loop/src/loop.ts

@@ -32,7 +32,7 @@ class TerminalModelRequestFailure extends Error {
     readonly requestError: RequestError,
     readonly failure: LlmFailure,
   ) {
-    super(requestError.message, { cause: requestError })
+    super(failure.message, { cause: requestError })
     this.name = 'TerminalModelRequestFailure'
   }
 }
@@ -69,6 +69,12 @@ function errorData(err: RequestError): { message: string; code?: string } {
   return { message: errorChain(err), ...typeof err.code === 'string' ? { code: err.code } : {} }
 }
 
+/** Preserve cause diagnostics, falling back to adapter-normalized prose for a hostile Error. */
+function durableFailure(err: RequestError, failure: LlmFailure): LlmFailure {
+  const message = errorChain(err)
+  return { ...failure, message: message === '<unrenderable value>' ? failure.message : message }
+}
+
 /** Map a successful max-token finish onto the turn reason; other successful finishes add nothing. */
 function stepFinishReason(finish: FinishReason): TurnEndReason | undefined {
   switch (finish.kind) {
@@ -231,7 +237,7 @@ async function runTurn(
     errorReported = true
     reason = failure === undefined
       ? { kind: 'error', step, ...errorData(err) }
-      : { kind: 'error', step, failure: { ...failure, message: errorChain(err) } }
+      : { kind: 'error', step, failure: durableFailure(err, failure) }
     try {
       events.emit('agent/error', turn, step, err)
     } catch {

+ 26 - 0
packages/core/agent-loop/tests/request-recovery.spec.ts

@@ -3,6 +3,7 @@ import { Context } from 'cordis'
 import LlmService, {
   CallId,
   CONTEXT_WINDOW_EXCEEDED_CODE,
+  HarnessError,
   LlmAdapter,
   LlmError,
   ProviderRequestId,
@@ -419,6 +420,31 @@ describe('agent post-step and request-error lifecycle', () => {
     expect(seen).toBe(original)
   })
 
+  it('keeps an adapter error with a hostile message accessor on the recovery path', async () => {
+    const original = Object.defineProperty(new HarnessError('provider failed', 'SERVER'), 'message', {
+      get() { throw new Error('SDK message accessor trap') },
+    })
+    const ctx = await harness(new SynchronousDispatchFailureAdapter(original))
+    const agent = ctx.agentLoop.create(SessionId('hostile-message-recovery'), { provider: 'mock', model: 'mock' })
+    let seenError: Error | undefined
+    let seenFailure: LlmFailure | undefined
+    ctx.on('agent/request-error', async (_agent, _turn, _step, error, failure, _history, _signal, next) => {
+      seenError = error
+      seenFailure = failure
+      return next()
+    })
+
+    send(agent)
+    await waitForIdle(ctx, agent)
+
+    expect(seenError).toBe(original)
+    expect(seenFailure).toEqual({ message: 'LLM adapter failed', code: 'SERVER' })
+    expect(agent.session.events.at(-1)).toMatchObject({
+      type: 'turn/end',
+      data: { reason: { kind: 'error', failure: { message: 'LLM adapter failed', code: 'SERVER' } } },
+    })
+  })
+
   it('passes structured facts beside the original Error and records its cause chain on exhaustion', async () => {
     const original = new LlmError('provider busy', 'RATE_LIMIT', {
       cause: new Error('upstream connection reset'),

+ 1 - 1
packages/core/session/README.md

@@ -60,7 +60,7 @@ Durable values need one accepted representation, not a check followed by a secon
 
 ### Session event vocabulary (`types.ts`)
 
-The append-only log's event types, enumerated member by member — payloads, surface badges, provenance — in the generated [persistence log event catalog](../../../docs/persistence-catalog.md). Token usage and provider/model/replay provenance ride on `assistant/message`; an operational error's step is on `turn/end.reason` for `kind: 'error'`, with structured provider facts for a final model-request failure.
+The append-only log's event types, enumerated member by member — payloads, surface badges, provenance — in the generated [persistence log event catalog](../../../docs/persistence-catalog.md). Token accounting reads per-step `assistant/chunk { type: 'usage' }` records and treats `assistant/message.usage` as the committed-step fallback when no usage chunk exists; failed model-request attempts have no assistant message. Provider/model/replay provenance rides on `assistant/message`; an operational error's step is on `turn/end.reason` for `kind: 'error'`, with structured provider facts for a final model-request failure.
 
 Merge-extensible via `SessionEventMap` — a plugin declaration-merges its own types (the compaction seam's `compact/*`, bounded recovery's non-surface `llm/retry`, the hook bridges' `hook/*`); merged members appear in the same catalog.
 

+ 4 - 1
packages/llm/llm-pi-ai/src/stream.ts

@@ -35,7 +35,10 @@ function classifyPiAiError(message: string): string {
   if (/\b400\b|invalid.?request/i.test(message)) return 'INVALID_REQUEST'
   if (/\b5\d\d\b/.test(message)) return 'SERVER'
   if (/\btime(?:d)?\s*out\b|timeout/i.test(message)) return 'TIMEOUT'
-  if (/\b(?:network|connection|socket|fetch)\b|\bECONN[A-Z]+\b/i.test(message)) return 'TRANSPORT'
+  if (/\b(?:network|connection|socket|fetch)\b|\bECONN[A-Z]+\b/i.test(message)
+    || /\b(?:other side closed|HTTP2 request did not get a response|WebSocket closed unexpectedly)\b/i.test(message)) {
+    return 'TRANSPORT'
+  }
   return 'PI_AI_ERROR'
 }
 

+ 13 - 0
packages/llm/llm-pi-ai/tests/convert.spec.ts

@@ -535,6 +535,10 @@ describe('mapStopReason / mapUsage', () => {
       .toMatchObject({ kind: 'error', failure: { code: 'RATE_LIMIT' } })
     expect(mapStopReason(assistant({ stopReason: 'error', errorMessage: 'HTTP 429: insufficient_quota' })))
       .toMatchObject({ kind: 'error', failure: { code: 'QUOTA' } })
+    expect(mapStopReason(assistant({
+      stopReason: 'error',
+      errorMessage: 'OpenAI API error (429): You exceeded your current quota, please check your plan and billing details.',
+    }))).toMatchObject({ kind: 'error', failure: { code: 'QUOTA' } })
     expect(mapStopReason(assistant({ stopReason: 'error', errorMessage: 'HTTP 500: backend down' })))
       .toMatchObject({ kind: 'error', failure: { code: 'SERVER' } })
     expect(mapStopReason(assistant({ stopReason: 'error', errorMessage: 'provider timed out' })))
@@ -555,6 +559,15 @@ describe('mapStopReason / mapUsage', () => {
     }))).toMatchObject({ kind: 'error', failure: { code: 'INVALID_REQUEST' } })
   })
 
+  it.each([
+    'other side closed',
+    'HTTP2 request did not get a response',
+    'WebSocket closed unexpectedly',
+  ])('maps pi-ai transport wording %j', (errorMessage) => {
+    expect(mapStopReason(assistant({ stopReason: 'error', errorMessage })))
+      .toMatchObject({ kind: 'error', failure: { code: 'TRANSPORT' } })
+  })
+
   it('uses pi-ai provider-specific overflow classification without losing rate-limit exclusions', () => {
     expect(mapStopReason(assistant({
       stopReason: 'error',

+ 1 - 0
packages/llm/llm/src/error.ts

@@ -74,6 +74,7 @@ export function isContextWindowExceededError(detail: string): boolean {
 export function isQuotaExceededError(detail: string): boolean {
   return /\binsufficient[\s_-]+(?:quota|balance|credits?)\b/i.test(detail)
     || /\b(?:quota|usage[\s_-]+limit)[\s_-]+(?:exceeded|exhausted|reached)\b/i.test(detail)
+    || /\bexceed(?:ed|s)?[\s_-]+(?:(?:your|the)[\s_-]+)?(?:current[\s_-]+)?quota\b/i.test(detail)
     || /\b(?:balance|credits?)[\s_-]+(?:exhausted|depleted)\b/i.test(detail)
     || /\bout[\s_-]+of[\s_-]+(?:credits?|budget)\b/i.test(detail)
 }

+ 1 - 0
packages/llm/llm/tests/service.spec.ts

@@ -90,6 +90,7 @@ describe('LlmService', () => {
       'account balance depleted',
       'usage-limit-exceeded',
       'out of credits',
+      'OpenAI API error (429): You exceeded your current quota, please check your plan and billing details.',
     ]) expect(isQuotaExceededError(detail)).toBe(true)
     expect(isQuotaExceededError('HTTP 429: rate limit reached')).toBe(false)
     expect(isQuotaExceededError('quota resets in one minute')).toBe(false)

+ 2 - 5
packages/ui/acp/src/index.ts

@@ -1124,11 +1124,8 @@ export function streamSessionEventUpdate(
       return
     }
     case 'turn/end': {
-      if (event.data.reason.kind !== 'error') return
-      const message = 'failure' in event.data.reason
-        ? event.data.reason.failure.message
-        : event.data.reason.message
-      const text = `\n\n[Model attempt failed; any partial output above is discarded: ${message}]\n\n`
+      if (event.data.reason.kind !== 'error' || !('failure' in event.data.reason)) return
+      const text = `\n\n[Model attempt failed; any partial output above is discarded: ${event.data.reason.failure.message}]\n\n`
       notify({ sessionId, update: { sessionUpdate: 'agent_message_chunk', content: { type: 'text', text } } })
       return
     }

+ 5 - 1
packages/ui/acp/tests/stream-update.spec.ts

@@ -65,7 +65,7 @@ describe('streamSessionEventUpdate', () => {
       .toEqual([])
   })
 
-  it('marks retry and terminal failure boundaries in the append-only update stream', () => {
+  it('marks retry and terminal model failure boundaries but not ordinary turn errors', () => {
     expect(updatesFor(evt('llm/retry', {
       turn: 1,
       step: 1,
@@ -90,6 +90,10 @@ describe('streamSessionEventUpdate', () => {
         text: '\n\n[Model attempt failed; any partial output above is discarded: still busy]\n\n',
       },
     }])
+    expect(updatesFor(evt('turn/end', {
+      turn: 1,
+      reason: { kind: 'error', step: 2, message: 'post-step failed' },
+    }))).toEqual([])
   })
 
   it('maps tool/call to an in_progress tool_call with kind other and parsed rawInput (generic fallback, no presenter)', () => {