mirror of
https://github.com/stablyai/orca.git
synced 2026-10-23 16:03:49 +00:00
fix(agent-status): close four tri-state evidence gaps found in review (STA-4119)
Independent review of the snapshot/replay fix found four ways the tri-state could report a positive "no background work" from evidence nobody reported, or stay "never observed" forever. All four are the same failure in different places: the new per-runtime observation marker was not kept in step with the inventory sets it qualifies. - Interrupt: clearing the inventory sets means "stop gating the pane", not "the provider reported an all-clear". The marker survived, so the resolver saw "observed, nothing running" and emitted a positive false over a process that may still be live. Interrupts now drop the marker in all three paths. - Session cron: a scheduled cron is provider-owned work under the same PTY, but the resolver consulted only the background-shell set and a cron-only inventory never set the marker. Such a pane was unknown forever, or reported a false all-clear from stale state. Cron now both counts as an observation and resolves as work. - Remote/relay: a host reports the inventory as a wire boolean, but the receiving runtime never recorded that an observation happened, so every remote pane resolved to unknown and a relayed all-clear could never make one eligible. - Local-hook rollback: the save/restore dance that protects against nested or rejected payloads tracked three fields and not the marker, so a discarded payload could leave the pane marked observed with the sets rolled back. Each is pinned by a test whose corresponding production revert makes it red.
This commit is contained in:
@@ -125,6 +125,12 @@ describe('AgentHookServer listener replay', () => {
|
||||
expect(server.getStatusSnapshot()[0]).toEqual(waiting)
|
||||
expect(server._getStateForTests().claudeRunningNonAgentTaskPaneKeys.has(PANE)).toBe(true)
|
||||
expect(server._getStateForTests().claudeActiveSessionCronPaneKeys.has(PANE)).toBe(true)
|
||||
// Why: the observation marker rides the same snapshot/restore as the sets above. A rejected
|
||||
// payload must not be able to move it either, or the tri-state could resolve from evidence
|
||||
// that was never accepted.
|
||||
expect(server._getStateForTests().claudeBackgroundInventoryObservedPaneKeys.has(PANE)).toBe(
|
||||
true
|
||||
)
|
||||
} finally {
|
||||
server.stop()
|
||||
}
|
||||
|
||||
@@ -79,4 +79,87 @@ describe('status snapshot carries provider background-work evidence', () => {
|
||||
server.stop()
|
||||
}
|
||||
})
|
||||
|
||||
// Why: an interrupt CLEARS the inventory sets to stop gating the pane. That is not the provider
|
||||
// reporting an all-clear, so it must leave the pane "never observed" (unknown) rather than
|
||||
// manufacturing a positive `false` over a process that may still be running.
|
||||
it('reports unknown, not a false all-clear, after an interrupt', async () => {
|
||||
const server = new AgentHookServer()
|
||||
await server.start({ env: 'production', userDataPath })
|
||||
try {
|
||||
await postHookEvent(
|
||||
server,
|
||||
buildBody({ hook_event_name: 'UserPromptSubmit', prompt: 'start the dev server' })
|
||||
)
|
||||
await postHookEvent(
|
||||
server,
|
||||
buildBody({ hook_event_name: 'Stop', background_tasks: [RUNNING_SHELL] })
|
||||
)
|
||||
expect(
|
||||
server.getStatusSnapshot().find((entry) => entry.paneKey === PANE)
|
||||
?.providerBackgroundWorkActive
|
||||
).toBe(true)
|
||||
|
||||
await postHookEvent(
|
||||
server,
|
||||
buildBody({ hook_event_name: 'Stop', is_interrupt: true, background_tasks: [] })
|
||||
)
|
||||
|
||||
expect(
|
||||
server.getStatusSnapshot().find((entry) => entry.paneKey === PANE)
|
||||
?.providerBackgroundWorkActive
|
||||
).toBeUndefined()
|
||||
} finally {
|
||||
server.stop()
|
||||
}
|
||||
})
|
||||
|
||||
// Why: a scheduled session cron is provider-owned work under this PTY just as a background shell
|
||||
// is. A cron-only inventory must both COUNT as an observation and resolve to live work.
|
||||
it('treats a cron-only inventory as observed live background work', async () => {
|
||||
const server = new AgentHookServer()
|
||||
await server.start({ env: 'production', userDataPath })
|
||||
try {
|
||||
await postHookEvent(
|
||||
server,
|
||||
buildBody({ hook_event_name: 'UserPromptSubmit', prompt: 'schedule a job' })
|
||||
)
|
||||
await postHookEvent(
|
||||
server,
|
||||
buildBody({ hook_event_name: 'Stop', session_crons: [{ id: 'cron-1' }] })
|
||||
)
|
||||
|
||||
const row = server.getStatusSnapshot().find((entry) => entry.paneKey === PANE)
|
||||
expect(row?.providerBackgroundWorkActive).toBe(true)
|
||||
} finally {
|
||||
server.stop()
|
||||
}
|
||||
})
|
||||
|
||||
// Why: a relayed host reports the inventory as a wire boolean. If the receiving runtime does not
|
||||
// record that an observation happened, every remote pane stays "never observed" forever and a
|
||||
// relayed all-clear can never make one eligible.
|
||||
it('records remote/relayed background-work evidence as an observation', async () => {
|
||||
const server = new AgentHookServer()
|
||||
await server.start({ env: 'production', userDataPath })
|
||||
try {
|
||||
server.ingestRemote(
|
||||
{
|
||||
paneKey: PANE,
|
||||
tabId: 'tab-1',
|
||||
worktreeId: 'wt-1',
|
||||
hookEventName: 'Stop',
|
||||
claudeRunningNonAgentTask: false,
|
||||
payload: { state: 'done', prompt: 'remote turn', agentType: 'claude' }
|
||||
},
|
||||
'conn-remote-1'
|
||||
)
|
||||
|
||||
const row = server.getStatusSnapshot().find((entry) => entry.paneKey === PANE)
|
||||
expect(row?.state).toBe('done')
|
||||
expect(row?.providerBackgroundWorkActive).toBe(false)
|
||||
} finally {
|
||||
server.stop()
|
||||
}
|
||||
})
|
||||
})
|
||||
|
||||
@@ -128,6 +128,10 @@ type ClaudeBackgroundEvidence = {
|
||||
hasRunningTask: boolean
|
||||
hasUnclassifiedTask: boolean
|
||||
hasActiveCron: boolean
|
||||
/** Why: the rollback below must restore this too. A nested/duplicate/rejected local payload that
|
||||
* carried an inventory would otherwise leave the pane marked "observed" with the running sets
|
||||
* rolled back, and the tri-state would resolve to a positive `false` nobody reported. */
|
||||
hasObservedInventory: boolean
|
||||
}
|
||||
|
||||
type NormalizedLocalHook = {
|
||||
@@ -1554,9 +1558,16 @@ export class AgentHookServer {
|
||||
if (agentType !== 'claude') {
|
||||
return undefined
|
||||
}
|
||||
return this.state.claudeBackgroundInventoryObservedPaneKeys.has(paneKey)
|
||||
? this.state.claudeRunningNonAgentTaskPaneKeys.has(paneKey)
|
||||
: undefined
|
||||
if (!this.state.claudeBackgroundInventoryObservedPaneKeys.has(paneKey)) {
|
||||
return undefined
|
||||
}
|
||||
// Why: a scheduled session cron is provider-owned work under this PTY exactly as a background
|
||||
// shell is, and the wire definition already treats both as the broad signal. Omitting cron
|
||||
// here would report an all-clear over a pane that still has work queued.
|
||||
return (
|
||||
this.state.claudeRunningNonAgentTaskPaneKeys.has(paneKey) ||
|
||||
this.state.claudeActiveSessionCronPaneKeys.has(paneKey)
|
||||
)
|
||||
}
|
||||
|
||||
/** Stamps the resolved evidence so every accepted row restates it, on the live push and on the
|
||||
@@ -2186,13 +2197,19 @@ export class AgentHookServer {
|
||||
return {
|
||||
hasRunningTask: this.state.claudeRunningNonAgentTaskPaneKeys.has(paneKey),
|
||||
hasUnclassifiedTask: this.state.claudeUnclassifiedBackgroundTaskPaneKeys.has(paneKey),
|
||||
hasActiveCron: this.state.claudeActiveSessionCronPaneKeys.has(paneKey)
|
||||
hasActiveCron: this.state.claudeActiveSessionCronPaneKeys.has(paneKey),
|
||||
hasObservedInventory: this.state.claudeBackgroundInventoryObservedPaneKeys.has(paneKey)
|
||||
}
|
||||
}
|
||||
|
||||
private setClaudeBackgroundEvidence(
|
||||
paneKey: string,
|
||||
{ hasRunningTask, hasUnclassifiedTask, hasActiveCron }: ClaudeBackgroundEvidence
|
||||
{
|
||||
hasRunningTask,
|
||||
hasUnclassifiedTask,
|
||||
hasActiveCron,
|
||||
hasObservedInventory
|
||||
}: ClaudeBackgroundEvidence
|
||||
): void {
|
||||
if (hasRunningTask) {
|
||||
this.state.claudeRunningNonAgentTaskPaneKeys.add(paneKey)
|
||||
@@ -2209,6 +2226,11 @@ export class AgentHookServer {
|
||||
} else {
|
||||
this.state.claudeActiveSessionCronPaneKeys.delete(paneKey)
|
||||
}
|
||||
if (hasObservedInventory) {
|
||||
this.state.claudeBackgroundInventoryObservedPaneKeys.add(paneKey)
|
||||
} else {
|
||||
this.state.claudeBackgroundInventoryObservedPaneKeys.delete(paneKey)
|
||||
}
|
||||
}
|
||||
|
||||
ingestTerminalStatus(event: {
|
||||
@@ -2558,6 +2580,10 @@ export class AgentHookServer {
|
||||
} else {
|
||||
this.state.claudeUnclassifiedBackgroundTaskPaneKeys.delete(paneKey)
|
||||
}
|
||||
// Why: the host DID report an inventory — that is what put the boolean on the wire.
|
||||
// Without recording it the tri-state stays "never observed" for every remote pane and
|
||||
// a relayed `false` can never make one eligible.
|
||||
this.state.claudeBackgroundInventoryObservedPaneKeys.add(paneKey)
|
||||
}
|
||||
: undefined
|
||||
)
|
||||
|
||||
@@ -2663,8 +2663,14 @@ function updateClaudeRunningNonAgentTask(
|
||||
state.claudeUnclassifiedBackgroundTaskPaneKeys.delete(paneKey)
|
||||
}
|
||||
// Why: reached only when the payload actually carried background_tasks, so this is the one
|
||||
// point where "Claude told us what is running" becomes true for the pane.
|
||||
state.claudeBackgroundInventoryObservedPaneKeys.add(paneKey)
|
||||
// point where "Claude told us what is running" becomes true for the pane. An interrupt is
|
||||
// excluded for the same reason the sets above are cleared: it ungates the pane rather than
|
||||
// reporting an all-clear.
|
||||
if (interrupted) {
|
||||
state.claudeBackgroundInventoryObservedPaneKeys.delete(paneKey)
|
||||
} else {
|
||||
state.claudeBackgroundInventoryObservedPaneKeys.add(paneKey)
|
||||
}
|
||||
}
|
||||
|
||||
function resolveClaudePaneState(
|
||||
@@ -2785,6 +2791,10 @@ export function markClaudeLeadTurnInterrupted(state: HookListenerState, paneKey:
|
||||
state.claudeRunningNonAgentTaskPaneKeys.delete(paneKey)
|
||||
state.claudeUnclassifiedBackgroundTaskPaneKeys.delete(paneKey)
|
||||
state.claudeActiveSessionCronPaneKeys.delete(paneKey)
|
||||
// Why: clearing the inventory sets here means "stop gating the pane", NOT "the provider reported
|
||||
// an all-clear". Leaving the observation behind would let the tri-state resolve to a positive
|
||||
// `false` from evidence nobody reported, over a process that may still be running.
|
||||
state.claudeBackgroundInventoryObservedPaneKeys.delete(paneKey)
|
||||
}
|
||||
|
||||
/** Rebuild a pane's working roster from a persisted snapshot; live activity confirms a seed, a complete task inventory may reap an unconfirmed one whose finish hook arrived while Orca was offline. */
|
||||
@@ -3053,6 +3063,11 @@ function normalizeClaudeEvent(
|
||||
} else {
|
||||
state.claudeActiveSessionCronPaneKeys.delete(paneKey)
|
||||
}
|
||||
// Why: a cron inventory is background-work evidence too. Without this a cron-only pane stays
|
||||
// "never observed" forever and can never become hibernatable.
|
||||
if (interrupted !== true) {
|
||||
state.claudeBackgroundInventoryObservedPaneKeys.add(paneKey)
|
||||
}
|
||||
} else if (eventAgentId === undefined && isTurnBoundary && backgroundTasks.present) {
|
||||
// Why: current Claude may omit an empty cron inventory while still emitting background_tasks.
|
||||
state.claudeActiveSessionCronPaneKeys.delete(paneKey)
|
||||
@@ -3160,6 +3175,9 @@ function normalizeClaudeEvent(
|
||||
state.claudeRunningNonAgentTaskPaneKeys.delete(paneKey)
|
||||
state.claudeUnclassifiedBackgroundTaskPaneKeys.delete(paneKey)
|
||||
state.claudeActiveSessionCronPaneKeys.delete(paneKey)
|
||||
// Why: an interrupt ungates the pane; it is not a reported all-clear. See
|
||||
// markClaudeLeadTurnInterrupted.
|
||||
state.claudeBackgroundInventoryObservedPaneKeys.delete(paneKey)
|
||||
}
|
||||
|
||||
const effectiveState = resolveClaudePaneState(state, paneKey, {
|
||||
|
||||
Reference in New Issue
Block a user