mirror of
https://github.com/Gitlawb/openclaude.git
synced 2026-08-24 10:14:19 -05:00
fix(sdk): report a permission timeout as a timeout (#2028)
* fix(sdk): report a permission timeout as a timeout
On timeout the handler called denyPendingPermission and then fell through
to the fallback. The deny resolves the promise registered by
registerPendingPermission, but Promise.race has already settled with
{timedOut: true}, so nothing is awaiting it and the decision is discarded.
The fallback is createDefaultCanUseTool, whose contract is that the host
supplied no permission callback at all. A host that wired up
onPermissionRequest and simply answered too slowly therefore got the tool
result 'no canUseTool or onPermissionRequest callback provided. Pass
canUseTool in options', plus the matching warning on stderr -- both false,
and both pointing at a configuration problem that does not exist. It also
consumed the one-shot warning latch, so a genuinely misconfigured later
query in the same process is never warned.
Return the timeout decision directly. The permission_timeout event and the
existing deny are unchanged.
* test(sdk): move the timeout cases into the existing permissions suite
tests/sdk/permissions.test.ts pinned the old behavior -- it asserted the
timeout result was the fallback's message, with a comment describing the
fall-through as intended. It is not: that message claims no permission
callback was provided, which is false whenever onPermissionRequest is
wired up. Assert the timeout reports itself instead.
The new cases live in that suite rather than a new file: a separate test
file adds a slot to bun's sequential file ordering, which shifted which
suite runs before which and surfaced an unrelated mock leak in CI
(taskReport git metadata and the /ads command).
* test(sdk): drive the permission-timeout case off a mocked clock
The no-callback-fallback-on-timeout test relied on a real 10ms wait, so
the deny hinged on scheduling. Use fake timers and advance the clock by
the timeout window instead, making the timer the deterministic cause of
the denial.
This commit is contained in:
@@ -1146,8 +1146,10 @@ describe('createExternalCanUseTool timeout scenarios', () => {
|
||||
)
|
||||
|
||||
expect(result.behavior).toBe('deny')
|
||||
// When timeout occurs, the implementation calls onTimeout and falls through to fallback
|
||||
expect(result.message).toBe('fallback')
|
||||
// A timeout reports itself. Falling through to the fallback would report
|
||||
// "no canUseTool or onPermissionRequest callback provided", which is not
|
||||
// true here -- onPermissionRequest was supplied, it just did not answer.
|
||||
expect(result.message).toContain('timed out')
|
||||
expect(onTimeout).toHaveBeenCalled()
|
||||
expect(onTimeout.mock.calls[0][0].type).toBe('permission_timeout')
|
||||
expect(onTimeout.mock.calls[0][0].tool_name).toBe('TestTool')
|
||||
@@ -1305,3 +1307,94 @@ describe('permission session_id dynamic resolution', () => {
|
||||
expect(capturedTimeoutSessionId).toBe('timeout-session')
|
||||
})
|
||||
})
|
||||
|
||||
describe('permission timeout does not masquerade as a missing callback', () => {
|
||||
// The timeout branch used to resolve its deny into a promise the race had
|
||||
// already abandoned and then fall through to the fallback, whose contract is
|
||||
// "no permission callback was provided at all". A host that wired up
|
||||
// onPermissionRequest and merely answered slowly was told it had supplied no
|
||||
// callback, so the reason for the denial never reached the model or the
|
||||
// host developer.
|
||||
test('the no-callback fallback is not consulted on timeout', async () => {
|
||||
const fallback = vi.fn(async () => ({
|
||||
behavior: 'deny' as const,
|
||||
message:
|
||||
'SDK: Tool "TestTool" denied — no canUseTool or onPermissionRequest callback provided. Pass canUseTool in options to control tool permissions.',
|
||||
}))
|
||||
const onTimeout = vi.fn()
|
||||
|
||||
const canUseTool = createExternalCanUseTool(
|
||||
undefined,
|
||||
fallback as never,
|
||||
createPermissionTarget(),
|
||||
// Provided, but never answers.
|
||||
() => {},
|
||||
onTimeout,
|
||||
10,
|
||||
)
|
||||
|
||||
// Drive the timeout off a mocked clock rather than a real 10ms wait, so the
|
||||
// deny is the timer firing deterministically and not a scheduling race.
|
||||
vi.useFakeTimers()
|
||||
try {
|
||||
const pending = canUseTool(
|
||||
{ name: 'TestTool' } as never,
|
||||
{},
|
||||
{} as never,
|
||||
{} as never,
|
||||
'test-id',
|
||||
undefined,
|
||||
)
|
||||
vi.advanceTimersByTime(10)
|
||||
const result = await pending
|
||||
|
||||
expect(result.behavior).toBe('deny')
|
||||
expect(result.behavior === 'deny' && result.message).toContain(
|
||||
'timed out',
|
||||
)
|
||||
// The misleading advice must not be what the model is told.
|
||||
expect(result.behavior === 'deny' && result.message).not.toContain(
|
||||
'no canUseTool or onPermissionRequest callback provided',
|
||||
)
|
||||
// Running the fallback would also burn its one-shot warning latch, so a
|
||||
// genuinely misconfigured later query in the process is never warned.
|
||||
expect(fallback).not.toHaveBeenCalled()
|
||||
expect(onTimeout).toHaveBeenCalledTimes(1)
|
||||
} finally {
|
||||
vi.useRealTimers()
|
||||
}
|
||||
})
|
||||
|
||||
test('a host answering before the timeout is unaffected', async () => {
|
||||
const fallback = vi.fn(async () => ({
|
||||
behavior: 'deny' as const,
|
||||
message: 'fallback',
|
||||
}))
|
||||
const onTimeout = vi.fn()
|
||||
const permissionTarget = createPermissionTarget()
|
||||
|
||||
const canUseTool = createExternalCanUseTool(
|
||||
undefined,
|
||||
fallback as never,
|
||||
permissionTarget,
|
||||
message => {
|
||||
permissionTarget.denyPendingPermission(message.tool_use_id, 'no')
|
||||
},
|
||||
onTimeout,
|
||||
5_000,
|
||||
)
|
||||
|
||||
const result = await canUseTool(
|
||||
{ name: 'TestTool' } as never,
|
||||
{},
|
||||
{} as never,
|
||||
{} as never,
|
||||
'test-id',
|
||||
undefined,
|
||||
)
|
||||
|
||||
expect(result.behavior === 'deny' && result.message).toBe('no')
|
||||
expect(onTimeout).not.toHaveBeenCalled()
|
||||
expect(fallback).not.toHaveBeenCalled()
|
||||
})
|
||||
})
|
||||
|
||||
Reference in New Issue
Block a user