mirror of
https://github.com/stablyai/orca.git
synced 2026-09-22 08:02:28 +00:00
fix(agent-hooks): revive a retired pane on each provider's own new-turn event (#15344)
* fix(agent-hooks): revive a retired pane on each provider's own new-turn event The un-retirement gate matched two raw event-name literals, UserPromptSubmit and SessionStart. Only 5 of 18 hook sources name their turn boundary that way, so for the rest a reused pane stayed rowless forever: the user starts a new turn and no row ever appears in the sidebar or dashboard. Measured, not estimated — 10 providers fail the new test on main: gemini (BeforeAgent), antigravity (PreInvocation), amp (agent.start), cursor (beforeSubmitPrompt), pi/omp/prime-agent (before_agent_start), grok (user_prompt_submit), copilot (sessionStart, seen raw), hermes (pre_llm_call). The gate was also wrong in the other direction: opencode has no turn boundary at all, yet a literal SessionStart revived its pane. Both directions are covered. The correct per-provider classifier, isNewTurnEvent, was already imported into this file and already used 160 lines below — #14706 added that call specifically so consumers would stop re-deriving boundaries from literals, and left this one. Keeps the literal check when the remote envelope omits source: that field is optional, an older relay does not send it, and requiring it would have left every source-less remote pane retired forever. * fix(agent-hooks): tell an absent source apart from an unrecognized one Review of the first commit found that `isAgentHookSource(envelope.source) ? envelope.source : undefined` collapses two different wire conditions into one: an older relay that omits `source`, and a NEWER host relaying a provider this build does not know. They need opposite answers. Case two is the normal upgrade order — hosts and clients update independently — and its boundary event will not be UserPromptSubmit/SessionStart, because 13 of the 18 providers we already ship are named something else. So the legacy-literal fallback stranded that provider's panes permanently: the exact defect this branch fixes, silently reintroduced for traffic our own wire doctrine calls normal. Pass the raw wire value so the gate can distinguish them, and fail OPEN on an unrecognized provider. The costs are asymmetric: a stranded pane is invisible, permanent, and has no user-facing recovery, while a spurious revive produces a row that decays after AGENT_STATUS_STALE_AFTER_MS. Also from review: - Cover the source-less branches, which had no coverage at all — including a test pinning what the legacy shim CANNOT do, so nobody later widens the literal list to "fix" it. - Use the source as agentType rather than always 'claude'; 15 of 16 rows were describing a state that cannot occur. - Reword the opencode comment: no plugin in that family emits these literals, so this closes a hole rather than removing behavior. Checked and not changed: the local HTTP path 404s an unresolvable source, so the fallback is unreachable there and masks nothing. * test(agent-hooks): stop pinning opencode's fence behavior on a synthetic event The negative case asserted that an opencode pane stays retired after a literal SessionStart. That rests on a false premise: origin/main's opencode plugin never emits SessionStart at all — zero occurrences in opencode/hook-service.ts, and the shared family source is what mimo-code uses too. So the assertion pinned an event no plugin sends, and it would have become actively wrong the moment a pending change gives opencode a real SessionStart, silently re-breaking the rowless reused-pane case that change exists to fix. Assert against mimo-code and command-code instead. Both genuinely have no boundary event in any planned state, so the case tests what it claims to. * fix(agent-hooks): reject malformed relay sources at retired fence Only a non-empty unknown string can identify a future provider. Keep null, blank, numeric, and object source values behind the retired-pane fence instead of treating malformed wire data as a new turn.
This commit is contained in:
@@ -515,6 +515,12 @@ describe('AgentHookServer listener replay', () => {
|
||||
// Why: closedAgentStatusTabIds is LRU-bounded, so the tab fence is not permanent.
|
||||
// If restoring a sibling key lifted the closed tab's PANE fence too, eviction of the
|
||||
// tab id would leave nothing at all holding that pane shut.
|
||||
//
|
||||
// Why a non-boundary event: a genuine new turn deliberately lifts the pane fence
|
||||
// (STA-3386), so it cannot be used to prove the fence exists. Measured on the pre-fix
|
||||
// tree, `claude` + `UserPromptSubmit` already revived this pane; this case only ever
|
||||
// passed with `before_agent_start` because the gate matched two raw literals and did not
|
||||
// recognize pi's boundary. The control below keeps it from passing for that reason again.
|
||||
it('leaves a closed tab pane fenced once its tab id is evicted', async () => {
|
||||
const server = new AgentHookServer()
|
||||
await server.start({ env: 'production' })
|
||||
@@ -545,10 +551,18 @@ describe('AgentHookServer listener replay', () => {
|
||||
}
|
||||
|
||||
await postHook(
|
||||
{ hook_event_name: 'before_agent_start', prompt: 'after eviction' },
|
||||
{ hook_event_name: 'agent_end', prompt: 'after eviction' },
|
||||
{ paneKey: detachedPane, tabId: 'tab-2' }
|
||||
)
|
||||
expect(server.getStatusSnapshot()).toEqual([])
|
||||
|
||||
// Why: proves the empty snapshot above came from the fence and not from an inert
|
||||
// event — the same post on an unfenced pane must produce a row.
|
||||
await postHook(
|
||||
{ hook_event_name: 'agent_end', prompt: 'unfenced control' },
|
||||
{ paneKey: makePaneKey('tab-9', LEAF_3), tabId: 'tab-9' }
|
||||
)
|
||||
expect(server.getStatusSnapshot()).toHaveLength(1)
|
||||
} finally {
|
||||
server.stop()
|
||||
}
|
||||
|
||||
@@ -0,0 +1,126 @@
|
||||
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
|
||||
import type { AgentHookSource } from '../../shared/agent-hook-relay'
|
||||
import { AgentHookServer, _internals } from './server'
|
||||
import { PANE } from './server.test-fixtures'
|
||||
|
||||
const { trackMock, getCohortAtEmitMock } = vi.hoisted(() => ({
|
||||
trackMock: vi.fn(),
|
||||
getCohortAtEmitMock: vi.fn()
|
||||
}))
|
||||
vi.mock('../telemetry/client', () => ({ track: trackMock }))
|
||||
vi.mock('../telemetry/cohort-classifier', () => ({ getCohortAtEmit: getCohortAtEmitMock }))
|
||||
|
||||
beforeEach(() => {
|
||||
_internals.resetCachesForTests()
|
||||
trackMock.mockReset()
|
||||
getCohortAtEmitMock.mockReset()
|
||||
getCohortAtEmitMock.mockReturnValue({ nth_repo_added: 2 })
|
||||
})
|
||||
afterEach(() => vi.restoreAllMocks())
|
||||
|
||||
/** Each source's own new-turn boundary, as `isNewTurnEvent` classifies it. `null` means the
|
||||
* provider has no turn boundary at all, so a retired pane there stays retired by design. */
|
||||
const NEW_TURN_EVENT: Record<AgentHookSource, string | null> = {
|
||||
claude: 'SessionStart',
|
||||
kimi: 'UserPromptSubmit',
|
||||
codex: 'SessionStart',
|
||||
gemini: 'BeforeAgent',
|
||||
antigravity: 'PreInvocation',
|
||||
amp: 'agent.start',
|
||||
cursor: 'beforeSubmitPrompt',
|
||||
pi: 'before_agent_start',
|
||||
omp: 'before_agent_start',
|
||||
'prime-agent': 'before_agent_start',
|
||||
droid: 'UserPromptSubmit',
|
||||
grok: 'user_prompt_submit',
|
||||
copilot: 'sessionStart',
|
||||
hermes: 'pre_llm_call',
|
||||
devin: 'UserPromptSubmit',
|
||||
// Why null for opencode today: its plugin emits no SessionStart, so it has no reachable
|
||||
// boundary. A pending change adds one; this entry moves with that change, not before it.
|
||||
opencode: null,
|
||||
'mimo-code': null,
|
||||
'command-code': null
|
||||
}
|
||||
|
||||
function reviveRetiredPane(source: unknown, hookEventName: string): boolean {
|
||||
const server = new AgentHookServer()
|
||||
// Why: retirement is what command completion leaves behind on a reusable shell pane.
|
||||
server.retirePaneAuthority(PANE)
|
||||
server.ingestRemote(
|
||||
{
|
||||
paneKey: PANE,
|
||||
tabId: 'tab-1',
|
||||
worktreeId: 'wt-1',
|
||||
...(source === undefined ? {} : { source }),
|
||||
hookEventName,
|
||||
payload: {
|
||||
state: 'working',
|
||||
prompt: 'after reuse',
|
||||
agentType: typeof source === 'string' ? source : 'claude'
|
||||
}
|
||||
},
|
||||
'conn-1'
|
||||
)
|
||||
return server.getStatusSnapshot().some((entry) => entry.paneKey === PANE)
|
||||
}
|
||||
|
||||
describe("retired pane un-retires on each provider's own new-turn event", () => {
|
||||
// Why: the gate matched two raw literals, so only the 5 sources that happen to name their
|
||||
// boundary UserPromptSubmit/SessionStart could ever revive — the rest stayed rowless forever.
|
||||
// Why: keys of a Record<AgentHookSource, …> — a new source fails typecheck here rather than
|
||||
// silently skipping coverage, which is the same guarantee the runtime list would give.
|
||||
const revivable = (Object.keys(NEW_TURN_EVENT) as AgentHookSource[]).filter(
|
||||
(source) => NEW_TURN_EVENT[source] !== null
|
||||
)
|
||||
|
||||
it.each(revivable)('%s', (source) => {
|
||||
const hookEventName = NEW_TURN_EVENT[source]
|
||||
expect(hookEventName).not.toBeNull()
|
||||
expect(reviveRetiredPane(source, hookEventName as string)).toBe(true)
|
||||
})
|
||||
|
||||
// Why these two: every case above passes `source`, so the source-less compatibility path —
|
||||
// the branch added for older relays — would otherwise ship with no coverage at all.
|
||||
it('revives on a literal boundary when an older relay omits source', () => {
|
||||
expect(reviveRetiredPane(undefined, 'UserPromptSubmit')).toBe(true)
|
||||
})
|
||||
|
||||
it('cannot revive a non-literal provider when an older relay omits source', () => {
|
||||
// Why pinned: this is the accepted cost of the legacy shim, not an oversight. Widening the
|
||||
// literal list to "fix" it would re-create the two-literal gate this change removes.
|
||||
expect(reviveRetiredPane(undefined, 'before_agent_start')).toBe(false)
|
||||
})
|
||||
|
||||
it('revives on any event from a provider this build does not recognize', () => {
|
||||
// Why fail open: a newer host can relay a 19th provider whose boundary name is unknown here.
|
||||
// `isAgentHookSource` rejects it, so it must not fall through to the legacy literals and
|
||||
// strand the pane permanently.
|
||||
expect(reviveRetiredPane('future-provider-19' as AgentHookSource, 'SomethingNewEntirely')).toBe(
|
||||
true
|
||||
)
|
||||
})
|
||||
|
||||
it.each([null, '', 19, { provider: 'future-provider-19' }])(
|
||||
'keeps malformed source value %j behind the retired-pane fence',
|
||||
(source) => {
|
||||
expect(
|
||||
reviveRetiredPane(source, source === null ? 'SessionStart' : 'SomethingNewEntirely')
|
||||
).toBe(false)
|
||||
}
|
||||
)
|
||||
|
||||
it('leaves the pane retired for a source with no turn boundary', () => {
|
||||
// Why mimo-code and command-code rather than opencode: these two have no boundary event in
|
||||
// any planned state. Opencode is deliberately excluded — its plugin emits no SessionStart on
|
||||
// main (verified: zero occurrences in opencode/hook-service.ts), so asserting either outcome
|
||||
// for it would pin a synthetic event, and a pending change gives it a real one.
|
||||
expect(reviveRetiredPane('mimo-code', 'SessionStart')).toBe(false)
|
||||
expect(reviveRetiredPane('command-code', 'SessionStart')).toBe(false)
|
||||
})
|
||||
|
||||
it('leaves the pane retired for a non-boundary event on a revivable source', () => {
|
||||
// Why: guards the inverse — the gate must not open on any event that merely mentions a session.
|
||||
expect(reviveRetiredPane('gemini', 'AfterAgent')).toBe(false)
|
||||
})
|
||||
})
|
||||
@@ -1123,7 +1123,13 @@ export class AgentHookServer {
|
||||
|
||||
private getAgentStatusDisposition(
|
||||
paneKey: string,
|
||||
event?: { hookEventName?: string; isReplay?: boolean }
|
||||
event?: {
|
||||
source?: AgentHookSource
|
||||
/** Raw wire value, so the gate can tell "field absent" from "field present but unknown". */
|
||||
rawSource?: unknown
|
||||
hookEventName?: string
|
||||
isReplay?: boolean
|
||||
}
|
||||
): 'accept' | 'restart' | 'suppress' {
|
||||
const ownerPaneKey = this.resolvePaneKeyAlias(paneKey)
|
||||
const paneRetired =
|
||||
@@ -1137,12 +1143,28 @@ export class AgentHookServer {
|
||||
return 'accept'
|
||||
}
|
||||
// Why: command completion retires launch authority but leaves its shell pane reusable.
|
||||
// A live SessionStart proves a new agent process owns the retired pane just like a
|
||||
// A live new-turn event proves a new agent process owns the retired pane just like a
|
||||
// fresh prompt does — without it, a session resumed in a reused pane stays rowless (STA-3386).
|
||||
if (
|
||||
(event?.hookEventName === 'UserPromptSubmit' || event?.hookEventName === 'SessionStart') &&
|
||||
event.isReplay !== true
|
||||
) {
|
||||
// Why the classifier, not literals: only 5 of 18 sources name their boundary
|
||||
// `UserPromptSubmit`/`SessionStart`; the rest stayed retired forever.
|
||||
// Why four branches: `source` collapses to undefined when an older relay omits the field,
|
||||
// when a newer host sends an unknown string, and when the wire value is malformed. Only an
|
||||
// unknown string is valid future-provider evidence. Unreachable from the local path, which
|
||||
// 404s an unresolvable source.
|
||||
const isNewTurn =
|
||||
event?.source !== undefined
|
||||
? isNewTurnEvent(event.source, event.hookEventName)
|
||||
: typeof event?.rawSource === 'string' && event.rawSource.trim().length > 0
|
||||
? // Why fail OPEN for an unknown provider: its boundary event is unknowable here, and
|
||||
// the costs are asymmetric — a stranded pane is invisible and permanent with no user
|
||||
// recovery, while a spurious revive decays after AGENT_STATUS_STALE_AFTER_MS.
|
||||
true
|
||||
: event?.rawSource === undefined
|
||||
? // Why literals here: an older relay omits `source` entirely. Legacy shim only — it
|
||||
// cannot revive a provider whose boundary event is named anything else.
|
||||
event?.hookEventName === 'UserPromptSubmit' || event?.hookEventName === 'SessionStart'
|
||||
: false
|
||||
if (isNewTurn && event?.isReplay !== true) {
|
||||
this.closedAgentStatusPaneKeys.delete(paneKey)
|
||||
this.closedAgentStatusPaneKeys.delete(ownerPaneKey)
|
||||
return 'restart'
|
||||
@@ -2249,6 +2271,8 @@ export class AgentHookServer {
|
||||
? envelope.compactTrigger
|
||||
: undefined
|
||||
const statusDisposition = this.getAgentStatusDisposition(paneKey, {
|
||||
source,
|
||||
rawSource: envelope.source,
|
||||
hookEventName,
|
||||
isReplay: envelope.isReplay === true
|
||||
})
|
||||
@@ -2467,6 +2491,7 @@ export class AgentHookServer {
|
||||
const normalized = this.normalizeLocalHookPayload(source, aliasedBody)
|
||||
const statusDisposition = normalized.event
|
||||
? this.getAgentStatusDisposition(normalized.event.paneKey, {
|
||||
source,
|
||||
hookEventName: normalized.event.hookEventName,
|
||||
isReplay: normalized.event.isReplay
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user