mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-11-14 10:39:32 +01:00
fix(approvals): the list leaves a dialog too new to judge
`GET /api/approvals` re-reads the pane of every pending dialog and resolves one whose frame shows no dialog, either because its options vanished or because the frame shows a turn running. `notePrompt` deliberately waits 3 s before making that same judgement, since the Notification hook can land before Ink paints the dialog and the working line is still on screen then. The list did not wait. That rarely mattered while it ran once per page load, but a client that polls it would resolve a live prompt as answered in the terminal, clearing the red alert on every device and the push Approve button with it. `sweepForList` skips a dialog younger than the delay. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
1 parent
e9b2ecd552
commit
324aa75896
3 files changed
+36
-3
No files matched your search
@@ -373,6 +373,21 @@ export class ApprovalInbox {
|
||||
return true;
|
||||
}
|
||||
|
||||
/**
|
||||
* The list route's sweep: `verifyStillAnswerable`, but never before the delay
|
||||
* `notePrompt` waits out. A frame that shows no dialog proves nothing about a
|
||||
* dialog Ink has not painted yet, and the working line still on screen then
|
||||
* would resolve a live prompt, clearing its red alert on every device. The
|
||||
* list used to run once per page load, which made that rare; a client that
|
||||
* polls it (the macOS app's logo) would hit it on every new dialog. Each
|
||||
* call on an older dialog costs one synchronous pane capture.
|
||||
*/
|
||||
sweepForList(id: string): boolean {
|
||||
const item = this.getById(id);
|
||||
if (item && Date.now() - item.createdAt < STALE_CHECK_DELAY_MS) return true;
|
||||
return this.verifyStillAnswerable(id);
|
||||
}
|
||||
|
||||
/**
|
||||
* "This session's pane started moving again": re-verify its pending DIALOG
|
||||
* item against the screen and resolve it if the dialog is gone.
|
||||
|
||||
@@ -84,8 +84,9 @@ export function registerApprovalRoutes(app: FastifyInstance, ctx: SessionPort):
|
||||
// conservative rule the answer path uses (`verifyStillAnswerable`): only
|
||||
// an item whose original frame parsed options can be resolved this way, so
|
||||
// an unreadable capture keeps the alert rather than dropping it. Resolving
|
||||
// here broadcasts `approval:resolved`, so the other devices clear too.
|
||||
return approvalInbox.verifyStillAnswerable(item.id);
|
||||
// here broadcasts `approval:resolved`, so the other devices clear too. A
|
||||
// dialog too new to judge is left alone (`sweepForList`).
|
||||
return approvalInbox.sweepForList(item.id);
|
||||
});
|
||||
return { success: true, data: { approvals } };
|
||||
});
|
||||
|
||||
@@ -7,7 +7,7 @@
|
||||
* The routes read the process-wide `approvalInbox` singleton, so every test
|
||||
* drains it in afterEach; a leaked pending item would bleed into the next test.
|
||||
*/
|
||||
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
|
||||
import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
|
||||
import Fastify, { type FastifyInstance } from 'fastify';
|
||||
import fastifyCookie from '@fastify/cookie';
|
||||
import { registerApprovalRoutes } from '../../src/web/routes/approval-routes.js';
|
||||
@@ -285,6 +285,10 @@ describe('approval routes', () => {
|
||||
});
|
||||
|
||||
describe('staleness sweep on GET /api/approvals', () => {
|
||||
/** Past the delay `notePrompt` waits before it judges a new dialog. */
|
||||
const pastTheDelay = () => vi.spyOn(Date, 'now').mockReturnValue(Date.now() + 3_100);
|
||||
afterEach(() => vi.restoreAllMocks());
|
||||
|
||||
it('resolves an item whose dialog left the pane, and tells the other clients', async () => {
|
||||
const resolved: Array<Record<string, unknown>> = [];
|
||||
await postHook(harness, 'permission_prompt', { tool_name: 'Bash' });
|
||||
@@ -294,11 +298,24 @@ describe('approval routes', () => {
|
||||
// the pane knows. The dialog is gone from the frame the next capture sees.
|
||||
approvalInbox.onResolved = (info) => resolved.push({ ...info });
|
||||
session.terminalBuffer = 'claude> back at the composer';
|
||||
pastTheDelay();
|
||||
|
||||
expect(await listApprovals(harness)).toHaveLength(0);
|
||||
expect(resolved).toEqual([expect.objectContaining({ resolution: 'resolved_in_terminal' })]);
|
||||
});
|
||||
|
||||
it('leaves a dialog too new to judge, however often it is listed', async () => {
|
||||
// The hook beat Ink's paint: the frame still shows the turn, which alone
|
||||
// would count as proof that no dialog is up.
|
||||
session.terminalBuffer = '✻ Actualizing… (13s · esc to interrupt)';
|
||||
await postHook(harness, 'permission_prompt', { tool_name: 'Bash' });
|
||||
expect(await listApprovals(harness)).toHaveLength(1);
|
||||
expect(await listApprovals(harness)).toHaveLength(1);
|
||||
|
||||
pastTheDelay();
|
||||
expect(await listApprovals(harness)).toHaveLength(0);
|
||||
});
|
||||
|
||||
it('keeps an item whose dialog is still on screen', async () => {
|
||||
await postHook(harness, 'permission_prompt', { tool_name: 'Bash' });
|
||||
// Pane unchanged (PERMISSION_DIALOG): the human has not answered yet.
|
||||
|
||||
Reference in new issue
Block a user