starred/orca
1
0
Fork 0
mirror of https://github.com/stablyai/orca.git synced 2026-09-22 08:02:28 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix(pi): detect ask_user_question and surface as blocked state (#9457)

Browse Source 
* fix(pi): detect ask_user_question and surface as blocked state

   Maps Pi tool_call/tool_execution_start events with ask_user_question
   to blocked state (was working), so worktrees surface in attention sort
   and trigger notifications — matching Claude/Codex/Grok behavior.

   Guards interactivePrompt derivation to Pi-only, OMP unchanged.
   5 new tests covering blocked transition, regression, malformed input,
   and OMP guard.

* fix(pi): gate ask_user_question blocked on raw tool_name and cover state exit

Address code-review findings on the Pi ask_user_question detection:

- Gate the Pi blocked classification on the event's own tool_name (matching the
  Claude/Grok normalizers) instead of the merged snapshot, so a partial
  follow-up event can't inherit a stale ask_user_question name from the tool
  cache and spuriously re-enter blocked. resolveToolState moves back after the
  state-name guard, so it no longer runs on discarded events.
- Make extractPiToolFields' agentKind parameter required; the sole call site
  always supplies it, and optional risked a future Pi caller silently falling
  back to OMP-safe (no interactivePrompt) behavior with no type error.
- Add coverage for the transition OUT of blocked (tool_execution_end -> working,
  agent_end -> done) and that a following regular Pi tool clears interactivePrompt.

* test(pty-connection): gate confirming null sample in idle-exit veto test

CI failed on a flaky call-count assertion: one timer advance can start
multiple getForegroundProcess reads, so the confirming null sample could
land before the replacement hook owner was installed. Hold 2nd+ null
samples until the veto owner is in place instead of requiring exactly
one extra call.

---------

Co-authored-by: Jinjing <6427696+AmethystLiang@users.noreply.github.com>
This commit is contained in:
Hugo Sáez
2026-07-19 17:42:41 -07:00
committed by GitHub
co-authored by Jinjing
parent 1def694e80
commit e3721e8cf2
3 changed files with 329 additions and 22 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/renderer/src/components/terminal-pane/pty-connection.test.ts
+26 -11
View File
@@ -17733,7 +17733,26 @@ describe('connectPanePty', () => {
const randomSpy = vi.spyOn(Math, 'random').mockReturnValue(0.5)
try {
const getForegroundProcess = vi.mocked(window.api.pty.getForegroundProcess)
getForegroundProcess.mockResolvedValue('codex')
// Why: call-count alone is flaky — one advanceTimersToNextTimerAsync can start
// multiple getForegroundProcess reads (completion cadence + pane tracker /
// confirmForegroundProcess alias). Gate the 2nd+ null sample until the
// replacement hook owner is installed so the confirming sample always sees it.
let idleMode = false
let nullSamplesStarted = 0
let releaseConfirmingNullSample: (() => void) | undefined
const confirmingNullSampleGate = new Promise<void>((resolve) => {
releaseConfirmingNullSample = resolve
})
getForegroundProcess.mockImplementation(async () => {
if (!idleMode) {
return 'codex'
}
nullSamplesStarted += 1
if (nullSamplesStarted >= 2) {
await confirmingNullSampleGate
}
return null
})
const paneKey = makePaneKey('tab-1', LEAF_1)
const pane = createPane(1)
const manager = createManager(1)
@@ -17753,21 +17772,16 @@ describe('connectPanePty', () => {
titleHandler('Codex working', 'Codex working')
await vi.advanceTimersByTimeAsync(2_500)
const inspectionsBeforeIdle = getForegroundProcess.mock.calls.length
getForegroundProcess.mockResolvedValue(null)
// Why: active process polling is jittered, so a fixed window can consume
// both null samples before the replacement hook row is installed.
for (
let attempts = 0;
getForegroundProcess.mock.calls.length === inspectionsBeforeIdle;
attempts += 1
) {
idleMode = true
for (let attempts = 0; nullSamplesStarted < 1; attempts += 1) {
if (attempts >= 10) {
throw new Error('Expected the first idle process inspection')
}
await vi.advanceTimersToNextTimerAsync()
}
expect(getForegroundProcess).toHaveBeenCalledTimes(inspectionsBeforeIdle + 1)
// Why: let the first null sample apply pendingProcessExitAgent before the
// replacement owner is installed; later samples stay gated until then.
await flushAsyncTicks()
mockStoreState.agentStatusByPaneKey[paneKey] = {
state: 'working',
@@ -17778,6 +17792,7 @@ describe('connectPanePty', () => {
paneKey,
stateHistory: []
}
releaseConfirmingNullSample?.()
idleHandler('Claude done')
await vi.advanceTimersByTimeAsync(800)
await vi.advanceTimersByTimeAsync(AGENT_TASK_COMPLETE_NOTIFICATION_MAX_WAIT_MS)
src/shared/agent-hook-listener.test.ts
+275
View File
@@ -372,6 +372,247 @@ describe('shared agent-hook-listener', () => {
expect(event?.payload.interactivePrompt).toBe(JSON.stringify(properties))
})
it('maps Pi tool_call ask_user_question to blocked with interactivePrompt', () => {
const questions = {
questions: [
{
question: 'What is your priority?',
options: ['A', 'B', 'C']
}
]
}
const blocked = normalizeHookPayload(
state,
'pi',
{
paneKey: PANE_KEY,
tabId: 'tab-1',
worktreeId: 'wt',
env: 'production',
version: '1',
payload: {
hook_event_name: 'tool_call',
tool_name: 'ask_user_question',
tool_input: questions
}
},
'production'
)
expect(blocked?.payload).toMatchObject({
state: 'blocked',
agentType: 'pi',
toolName: 'ask_user_question'
})
expect(blocked?.payload.interactivePrompt).toBe(JSON.stringify(questions))
})
it('maps Pi tool_execution_start ask_user_question to blocked with interactivePrompt', () => {
const questions = {
questions: [
{
question: 'Pick a path',
options: ['path-1', 'path-2']
}
]
}
const blocked = normalizeHookPayload(
state,
'pi',
{
paneKey: PANE_KEY,
tabId: 'tab-1',
worktreeId: 'wt',
env: 'production',
version: '1',
payload: {
hook_event_name: 'tool_execution_start',
tool_name: 'ask_user_question',
tool_input: questions
}
},
'production'
)
expect(blocked?.payload).toMatchObject({
state: 'blocked',
agentType: 'pi',
toolName: 'ask_user_question'
})
expect(blocked?.payload.interactivePrompt).toBe(JSON.stringify(questions))
})
it('keeps Pi regular tool_call notifications as working', () => {
const working = normalizeHookPayload(
state,
'pi',
{
paneKey: PANE_KEY,
tabId: 'tab-1',
worktreeId: 'wt',
env: 'production',
version: '1',
payload: {
hook_event_name: 'tool_call',
tool_name: 'bash',
tool_input: { command: 'git status' }
}
},
'production'
)
expect(working?.payload).toMatchObject({
state: 'working',
agentType: 'pi',
toolName: 'bash',
toolInput: 'git status'
})
expect(working?.payload.interactivePrompt).toBeUndefined()
})
it('keeps Pi ask_user_question blocked when tool_input is missing', () => {
const blocked = normalizeHookPayload(
state,
'pi',
{
paneKey: PANE_KEY,
tabId: 'tab-1',
worktreeId: 'wt',
env: 'production',
version: '1',
payload: {
hook_event_name: 'tool_call',
tool_name: 'ask_user_question'
}
},
'production'
)
expect(blocked?.payload).toMatchObject({
state: 'blocked',
agentType: 'pi',
toolName: 'ask_user_question'
})
expect(blocked?.payload.interactivePrompt).toBeUndefined()
})
it('clears Pi ask_user_question blocked once the tool_execution_end arrives', () => {
const questions = {
questions: [{ question: 'Ship it?', options: ['yes', 'no'] }]
}
const base = {
paneKey: PANE_KEY,
tabId: 'tab-1',
worktreeId: 'wt',
env: 'production' as const,
version: '1'
}
const blocked = normalizeHookPayload(
state,
'pi',
{
...base,
payload: {
hook_event_name: 'tool_call',
tool_name: 'ask_user_question',
tool_input: questions
}
},
'production'
)
expect(blocked?.payload.state).toBe('blocked')
expect(blocked?.payload.interactivePrompt).toBe(JSON.stringify(questions))
// Why: the answered question must leave the blocked/needs-attention state so
// the notification and attention sort clear; tool_execution_end is working.
const cleared = normalizeHookPayload(
state,
'pi',
{
...base,
payload: {
hook_event_name: 'tool_execution_end',
tool_name: 'ask_user_question'
}
},
'production'
)
expect(cleared?.payload.state).toBe('working')
expect(cleared?.payload.interactivePrompt).toBeUndefined()
})
it('marks Pi done when agent_end follows an ask_user_question block', () => {
const base = {
paneKey: PANE_KEY,
tabId: 'tab-1',
worktreeId: 'wt',
env: 'production' as const,
version: '1'
}
normalizeHookPayload(
state,
'pi',
{
...base,
payload: {
hook_event_name: 'tool_call',
tool_name: 'ask_user_question',
tool_input: { questions: [{ question: 'Pick', options: ['a', 'b'] }] }
}
},
'production'
)
const done = normalizeHookPayload(
state,
'pi',
{ ...base, payload: { hook_event_name: 'agent_end' } },
'production'
)
expect(done?.payload.state).toBe('done')
expect(done?.payload.interactivePrompt).toBeUndefined()
})
it('clears the ask_user_question interactivePrompt when a regular Pi tool runs next', () => {
const base = {
paneKey: PANE_KEY,
tabId: 'tab-1',
worktreeId: 'wt',
env: 'production' as const,
version: '1'
}
normalizeHookPayload(
state,
'pi',
{
...base,
payload: {
hook_event_name: 'tool_call',
tool_name: 'ask_user_question',
tool_input: { questions: [{ question: 'Pick', options: ['a', 'b'] }] }
}
},
'production'
)
// Why: a follow-up regular tool must not inherit the prior question's blocked
// state or its live interactivePrompt card.
const working = normalizeHookPayload(
state,
'pi',
{
...base,
payload: {
hook_event_name: 'tool_call',
tool_name: 'bash',
tool_input: { command: 'ls' }
}
},
'production'
)
expect(working?.payload).toMatchObject({
state: 'working',
agentType: 'pi',
toolName: 'bash',
toolInput: 'ls'
})
expect(working?.payload.interactivePrompt).toBeUndefined()
})
it('normalizes OMP Pi-compatible hooks with OMP attribution', () => {
const event = normalizeHookPayload(
state,
@@ -419,6 +660,40 @@ describe('shared agent-hook-listener', () => {
toolName: 'bash',
toolInput: 'pnpm test'
})
expect(tool?.payload.interactivePrompt).toBeUndefined()
})
it('keeps OMP ask_user_question behavior on Pi-compatible events', () => {
const tool = normalizeHookPayload(
state,
'omp',
{
paneKey: PANE_KEY,
tabId: 'tab-1',
worktreeId: 'wt',
env: 'production',
version: '1',
payload: {
hook_event_name: 'tool_call',
tool_name: 'ask_user_question',
tool_input: {
questions: [
{
question: 'Choose',
options: ['x', 'y']
}
]
}
}
},
'production'
)
expect(tool?.payload).toMatchObject({
state: 'working',
agentType: 'omp',
toolName: 'ask_user_question'
})
expect(tool?.payload.interactivePrompt).toBeUndefined()
})
it('captures Pi session ids on Pi-compatible status events', () => {
src/shared/agent-hook-listener.ts
+28 -11
View File
@@ -1898,7 +1898,8 @@ function extractCopilotToolFields(
function extractPiToolFields(
eventName: unknown,
hookPayload: Record<string, unknown>
hookPayload: Record<string, unknown>,
agentKind: 'pi' | 'omp'
): ToolSnapshot {
if (
eventName === 'tool_call' ||
@@ -1906,9 +1907,16 @@ function extractPiToolFields(
eventName === 'tool_execution_end'
) {
const toolName = readString(hookPayload, 'tool_name')
const toolInput = deriveToolInputPreview(toolName, hookPayload.tool_input)
const rawToolInput = hookPayload.tool_input
const toolInput = deriveToolInputPreview(toolName, rawToolInput)
// Why: OMP shares this extractor. interactivePrompt derivation should
// only apply to Pi so OMP ask_user_question metadata stays unchanged.
const interactivePrompt =
agentKind === 'pi' && (eventName === 'tool_call' || eventName === 'tool_execution_start')
? deriveInteractivePrompt(toolName, rawToolInput, eventName)
: undefined
return toolUpdate(
{ toolName, toolInput },
{ toolName, toolInput, interactivePrompt },
{ hasToolInputField: hasOwnField(hookPayload, 'tool_input') }
)
}
@@ -2386,7 +2394,7 @@ function extractToolFields(
return extractCursorToolFields(eventName, hookPayload)
case 'pi':
case 'omp':
return extractPiToolFields(eventName, hookPayload)
return extractPiToolFields(eventName, hookPayload, source)
case 'droid':
return extractDroidToolFields(eventName, hookPayload)
case 'command-code':
@@ -3422,13 +3430,22 @@ function normalizePiCompatibleEvent(
return null
}
const stateName =
eventName === 'before_agent_start' ||
eventName === 'agent_start' ||
eventName === 'tool_call' ||
eventName === 'tool_execution_start' ||
eventName === 'tool_execution_end' ||
eventName === 'message_end'
// Why: gate on the event's own tool_name (matching the Claude/Grok normalizers),
// not a merged snapshot, so a partial follow-up event can't inherit a stale
// ask_user_question name from the tool cache and spuriously re-enter blocked.
const isPiAskUserQuestion =
agentType === 'pi' &&
isAskUserQuestionTool(readString(hookPayload, 'tool_name')) &&
(eventName === 'tool_call' || eventName === 'tool_execution_start')
const stateName = isPiAskUserQuestion
? 'blocked'
: eventName === 'before_agent_start' ||
eventName === 'agent_start' ||
eventName === 'tool_call' ||
eventName === 'tool_execution_start' ||
eventName === 'tool_execution_end' ||
eventName === 'message_end'
? 'working'
: eventName === 'agent_end'
? 'done'