mirror of
https://github.com/stablyai/orca.git
synced 2026-10-08 08:02:32 +00:00
fix(codex): latch the background-terminal capability on the right error class
The latch keyed on the complement of the class it meant to catch. The dispatcher converts method-not-found into CodexAppServerUnsupportedError and routes everything else to CodexAppServerRequestError; this caught the latter, which the repo states outright is not the class a capability cache may treat as unsupported. It therefore failed in both directions. A host that genuinely lacks the methods answers -32601, arrives as Unsupported, is never caught, and keeps being re-probed every turn — and if terminate is missing while list works, supportsTaskStop stays true and the Stop button silently does nothing, which is verbatim the outcome this was written to prevent. Meanwhile a single transient refusal — a thread lost to a resume race, an internal error — set supported false permanently and hid the row and the Stop for the rest of the session. Only Unsupported latches now. Everything else leaves supported at null, so the client is still shown nothing, and the next turn asks again. The three tests that covered this all constructed a RequestError carrying -32601 — a value the dispatcher can never build, since -32601 is exactly what becomes Unsupported. They exercised a fiction and would have stayed green through any latch change. They now use the class a real host produces, and new cases cover the transient refusals that must not latch, including the experimental-API gate.
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
import { describe, expect, it, vi } from 'vitest'
|
||||
import { CodexAppServerRequestError } from './codex-app-server-request-error'
|
||||
import { CodexAppServerUnsupportedError } from './codex-app-server-session'
|
||||
import {
|
||||
createCodexBackgroundTerminals,
|
||||
refreshCodexBackgroundTerminals,
|
||||
@@ -74,13 +75,14 @@ describe('codex background terminals', () => {
|
||||
|
||||
describe('capability probe', () => {
|
||||
it('latches off and stays quiet when the host refuses the operation', async () => {
|
||||
// Method-not-found reaches callers as CodexAppServerUnsupportedError:
|
||||
// the dispatcher converts -32601 before anyone sees it, so a
|
||||
// RequestError carrying -32601 is a value production cannot produce.
|
||||
const request = vi
|
||||
.fn()
|
||||
.mockRejectedValue(
|
||||
new CodexAppServerRequestError(
|
||||
'thread/backgroundTerminals/list',
|
||||
-32601,
|
||||
'method not found'
|
||||
new CodexAppServerUnsupportedError(
|
||||
'codex app-server does not support thread/backgroundTerminals/list: method not found'
|
||||
)
|
||||
)
|
||||
const terminals = createCodexBackgroundTerminals()
|
||||
@@ -157,10 +159,8 @@ describe('codex background terminals', () => {
|
||||
const request = vi
|
||||
.fn()
|
||||
.mockRejectedValue(
|
||||
new CodexAppServerRequestError(
|
||||
'thread/backgroundTerminals/clean',
|
||||
-32601,
|
||||
'method not found'
|
||||
new CodexAppServerUnsupportedError(
|
||||
'codex app-server does not support thread/backgroundTerminals/clean: method not found'
|
||||
)
|
||||
)
|
||||
const terminals = createCodexBackgroundTerminals()
|
||||
@@ -174,3 +174,52 @@ describe('codex background terminals', () => {
|
||||
})
|
||||
})
|
||||
})
|
||||
|
||||
// Errors the dispatcher really produces for a host that HAS the methods. None
|
||||
// of these may latch the capability off: one bad turn would otherwise hide the
|
||||
// row and the Stop button for the rest of the session.
|
||||
describe('refusals that must not latch the capability off', () => {
|
||||
const transient = (code: number, message: string): CodexAppServerRequestError =>
|
||||
new CodexAppServerRequestError(
|
||||
'thread/backgroundTerminals/list',
|
||||
code,
|
||||
`codex app-server thread/backgroundTerminals/list failed: ${message}`
|
||||
)
|
||||
|
||||
it.each([
|
||||
['a thread lost to a resume race', -32600, 'invalid_request: thread not found'],
|
||||
['an internal server error', -32603, 'internal_error'],
|
||||
['the experimental-API gate', -32600, 'invalid_request: experimental API not enabled']
|
||||
])('keeps asking after %s', async (_case, code, message) => {
|
||||
const request = vi.fn().mockRejectedValue(transient(code as number, message as string))
|
||||
const terminals = createCodexBackgroundTerminals()
|
||||
const session = makeSession(request)
|
||||
|
||||
await refreshCodexBackgroundTerminals(terminals, session)
|
||||
await refreshCodexBackgroundTerminals(terminals, session)
|
||||
|
||||
// Not latched, so the next turn tries again...
|
||||
expect(terminals.supported).not.toBe(false)
|
||||
expect(request).toHaveBeenCalledTimes(2)
|
||||
// ...and until one succeeds the client is shown nothing, which is the safe
|
||||
// direction to fail in.
|
||||
expect(terminals.state).toBeNull()
|
||||
})
|
||||
|
||||
it('does not hide a live row because one stop attempt failed', async () => {
|
||||
const terminals = createCodexBackgroundTerminals()
|
||||
terminals.supported = true
|
||||
terminals.state = { state: 'monitoring', tasks: [], supportsTaskStop: true }
|
||||
const request = vi
|
||||
.fn()
|
||||
.mockRejectedValue(
|
||||
new CodexAppServerRequestError('thread/backgroundTerminals/clean', -32603, 'internal_error')
|
||||
)
|
||||
|
||||
const result = await stopCodexBackgroundTerminals(terminals, makeSession(request))
|
||||
|
||||
expect(result).toEqual({ cancelled: false })
|
||||
expect(terminals.supported).toBe(true)
|
||||
expect(terminals.state).not.toBeNull()
|
||||
})
|
||||
})
|
||||
|
||||
@@ -2,7 +2,7 @@ import type {
|
||||
AgentSessionBackgroundTask,
|
||||
AgentSessionBackgroundTaskState
|
||||
} from '../../shared/agent-session-wire'
|
||||
import { isCodexAppServerRequestError } from './codex-app-server-connection'
|
||||
import { isCodexAppServerUnsupportedError } from './codex-app-server-session'
|
||||
import type { CodexSession } from './codex-structured-session-state'
|
||||
|
||||
/**
|
||||
@@ -10,10 +10,18 @@ import type { CodexSession } from './codex-structured-session-state'
|
||||
* outlive the turn. Interrupting a turn does not reap them; the app-server
|
||||
* exposes that as its own operation, and this is the only path to it.
|
||||
*
|
||||
* The operations are experimental upstream, so a host can answer any of them
|
||||
* with a refusal. `supported` starts null (never asked) and latches to false on
|
||||
* the first refusal, which keeps the client's Stop control hidden rather than
|
||||
* offering one that silently does nothing.
|
||||
* The operations are experimental upstream, so a host may not have them at all.
|
||||
* `supported` starts null (never asked) and latches to false only on
|
||||
* CodexAppServerUnsupportedError — the class the dispatcher raises for
|
||||
* method-not-found, and the only one capability caches are allowed to treat as
|
||||
* unsupported (see CodexAppServerUnsupportedError's own contract).
|
||||
*
|
||||
* Every other refusal is transient by definition: a `thread not found` after a
|
||||
* resume race, an internal error, the experimental-API gate on a host that may
|
||||
* admit the call next launch. Those must not latch, or one bad turn would hide
|
||||
* the control for the rest of the session. They leave `supported` at null, so
|
||||
* the client still shows nothing — the safe direction — and the next turn asks
|
||||
* again.
|
||||
*/
|
||||
export type CodexBackgroundTerminals = {
|
||||
supported: boolean | null
|
||||
@@ -99,7 +107,12 @@ export async function refreshCodexBackgroundTerminals(
|
||||
{ timeoutMs }
|
||||
)
|
||||
} catch (error) {
|
||||
if (isCodexAppServerRequestError(error)) {
|
||||
// Why only this class: method-not-found is the one signal that means "never
|
||||
// going to work". Latching on the general request error would mark a healthy
|
||||
// host unsupported after a single transient failure, and — worse — would
|
||||
// never fire on a host that genuinely lacks the method, since that arrives
|
||||
// as the unsupported class instead.
|
||||
if (isCodexAppServerUnsupportedError(error)) {
|
||||
const had = terminals.state !== null
|
||||
terminals.supported = false
|
||||
terminals.state = null
|
||||
@@ -145,7 +158,7 @@ export async function stopCodexBackgroundTerminals(
|
||||
{ timeoutMs }
|
||||
))
|
||||
} catch (error) {
|
||||
if (isCodexAppServerRequestError(error)) {
|
||||
if (isCodexAppServerUnsupportedError(error)) {
|
||||
terminals.supported = false
|
||||
terminals.state = null
|
||||
}
|
||||
|
||||
@@ -1,13 +1,13 @@
|
||||
import { describe, expect, it } from 'vitest'
|
||||
import type { AgentSessionJournalIdentity } from '../../shared/agent-session-journal-types'
|
||||
import type { AgentSessionBackgroundTaskState } from '../../shared/agent-session-wire'
|
||||
import {
|
||||
CodexAppServerRequestError,
|
||||
type CodexAppServerConnection,
|
||||
type CodexAppServerConnectionHandlers,
|
||||
type CodexAppServerLaunch,
|
||||
type openCodexAppServerConnection
|
||||
import type {
|
||||
CodexAppServerConnection,
|
||||
CodexAppServerConnectionHandlers,
|
||||
CodexAppServerLaunch,
|
||||
openCodexAppServerConnection
|
||||
} from './codex-app-server-connection'
|
||||
import { CodexAppServerUnsupportedError } from './codex-app-server-session'
|
||||
import { CodexStructuredSessionAdapter } from './codex-structured-session-adapter'
|
||||
|
||||
const THREAD_ID = 'thread-abc'
|
||||
@@ -171,9 +171,14 @@ describe('CodexStructuredSessionAdapter background terminals', () => {
|
||||
})
|
||||
|
||||
describe('a host without the operations', () => {
|
||||
// The dispatcher converts -32601 into CodexAppServerUnsupportedError before
|
||||
// any caller sees it, so a RequestError carrying -32601 is a value
|
||||
// production can never build. Refuse the way a real host refuses.
|
||||
const refuse = (method: string): Route => {
|
||||
return () => {
|
||||
throw new CodexAppServerRequestError(method, -32601, 'method not found')
|
||||
throw new CodexAppServerUnsupportedError(
|
||||
`codex app-server does not support ${method}: method not found`
|
||||
)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user