From 112f9fff3939f1ec0b62aa819ed88df0114ba1cc Mon Sep 17 00:00:00 2001 From: Simon <10131203+gaomeng1900@users.noreply.github.com> Date: Fri, 5 Jun 2026 20:27:30 +0800 Subject: [PATCH 1/5] docs: planning --- docs/plan-abort-signal.md | 176 ++++++++++++++++++++++++++++++++++++++ 1 file changed, 176 insertions(+) create mode 100644 docs/plan-abort-signal.md diff --git a/docs/plan-abort-signal.md b/docs/plan-abort-signal.md new file mode 100644 index 0000000..b8c3d51 --- /dev/null +++ b/docs/plan-abort-signal.md @@ -0,0 +1,176 @@ +# Plan: Unified Abort Signal for Tool Cancellation + +## Problem + +When `stop()` or `dispose()` is called while a tool is executing, the agent's main loop remains blocked on `await tool.execute(...)`. The abort signal only reaches the LLM HTTP fetch — it never reaches tool execution or async callbacks like `onAskUser`. + +Consequence: status never transitions, UI cannot clean up, panel cannot close. PR #188 patched this for `ask_user` alone by listening to `statuschange`/`dispose` events in Panel — a per-tool band-aid that doesn't scale. + +### Current abort coverage + +| Path | Receives abort? | +| -------------------------------- | ----------------------------------- | +| LLM HTTP request (`fetch`) | Yes — signal passed to fetch | +| MacroTool entry guard (line 382) | Yes — manual `signal.aborted` check | +| **In-flight tool execution** | No | +| `onAskUser` Promise | No | +| `wait` setTimeout | No | +| Custom tool async work | No | + +## Design Principles + +- **Expose errors, don't mask them.** If a tool doesn't respect abort, that's a bug in the tool. The framework should surface it loudly, not silently work around it. +- **AbortSignal as the single cancellation primitive.** Standard web platform pattern. All async work subscribes to it. +- **Internal tools must respect signal.** This solves the original issue (ask_user, wait). No forced unblock needed. +- **Abort trigger points must remain controlled.** The entire cancellation system hinges on this one signal. Its trigger conditions must be deliberate and auditable. See "Abort trigger audit" below. + +## Design + +Two layers, plus a diagnostic mechanism: + +### Layer 1: Expose signal to tools (cooperative cancellation) + +Pass signal into every tool execution. Well-behaved tools self-cancel immediately. + +**Tool signature change:** + +```ts +// Before +execute: (this: PageAgentCore, args: TParams) => Promise + +// After +execute: (this: PageAgentCore, args: TParams, ctx: { signal: AbortSignal }) => Promise +``` + +Backward-compatible — tools that ignore ctx still compile and run. + +**`onAskUser` signature change:** + +```ts +// Before +onAskUser?: (question: string) => Promise + +// After +onAskUser?: (question: string, options?: { signal: AbortSignal }) => Promise +``` + +### Layer 2: Abort deadline warning (diagnostic, not rescue) + +Instead of silently unblocking the main loop when a tool ignores signal, detect the violation and warn loudly. + +```ts +// packages/core/src/utils/index.ts +export function onAbortTimeout(signal: AbortSignal, ms: number, callback: () => void): () => void { + if (signal.aborted) { + callback() + return () => {} + } + let timer: ReturnType | null = null + const onAbort = () => { + timer = setTimeout(callback, ms) + } + signal.addEventListener('abort', onAbort, { once: true }) + return () => { + signal.removeEventListener('abort', onAbort) + if (timer) clearTimeout(timer) + } +} +``` + +Applied at the tool execution site in `#packMacroTool`: + +```ts +// PageAgentCore.ts — inside macroTool.execute +const signal = this.#abortController.signal + +const unsubscribe = onAbortTimeout(signal, 3000, () => { + console.warn( + `[PageAgent] Tool "${toolName}" did not respond to abort signal within 3s. ` + + `Tools MUST honor ctx.signal for proper cancellation. ` + + `See: https://page-agent.dev/docs/custom-tools#abort` + ) +}) + +const result = await tool.execute.bind(this)(toolInput, { signal }) +unsubscribe() +``` + +**This does NOT unblock the loop.** If a custom tool hangs, the agent hangs — visibly. The warning tells the developer exactly which tool is at fault. Hiding the problem (via forced unblock) would make bugs harder to diagnose and give false confidence that stop() always works instantly. + +### Layer 3 (future): PageController guard + +Not implemented now. Future enhancement: `PageController.arm(signal)` + `#assertAlive()` at the top of every public method. Would prevent orphaned tools from executing DOM side-effects. Adds hardening for custom tools that ignore signal but still call controller methods. + +## Abort trigger audit + +`#abortController.abort()` is called from exactly 4 sites: + +| Call site | Context | Can fire during tool execution? | +| ----------------- | ---------------------------------------------------------------------- | ------------------------------------------------- | +| `stop()` | User clicks stop | Yes — this is the primary cancel path | +| `dispose()` | Agent destroyed | Yes — equivalent to stop + teardown | +| `execute()` entry | Reset before new task, immediately followed by `new AbortController()` | No — new signal replaces old before any tool runs | +| `#onDone()` | Task already finished (loop exited) | No — defensive cleanup, no tool in flight | + +Only `stop()` and `dispose()` can fire the signal while a tool is executing. Both represent explicit, user-initiated intent to cancel. + +**Invariant:** No code path should call `abort()` as a side-effect, cleanup shortcut, or error-recovery mechanism. If a new call site is ever added, it must be audited against this table. Adding an accidental abort trigger would silently kill in-flight tools — a class of bug that's extremely hard to reproduce and diagnose. + +## Changes Required + +### `packages/core/src/utils/index.ts` + +- Add `onAbortTimeout()` utility. + +### `packages/core/src/tools/index.ts` + +- Update `PageAgentTool` interface: add `ctx: { signal: AbortSignal }` as second param to `execute`. +- `ask_user` tool: listen to signal, reject with AbortError on abort. Pass `{ signal }` to `this.onAskUser`. +- `wait` tool: race setTimeout against signal. On abort, resolve immediately with AbortError. + +### `packages/core/src/PageAgentCore.ts` + +- Expose `get abortSignal(): AbortSignal`. +- In `#packMacroTool` execute: pass `{ signal }` as ctx to tool. Add `onAbortTimeout` diagnostic. +- Keep the manual `if (signal.aborted) throw` guard at entry (cheap fast-path before tool runs). +- `stop()`: remains simple — `abort()` + cleanup. No `#setStatus('error')` hack. Status transitions naturally when the tool throws AbortError → catch block → `#onDone(false)`. +- Update `onAskUser` type to accept optional `{ signal }`. + +### `packages/ui/src/panel/Panel.ts` + +- `#askUser`: subscribe to signal abort → clean up question card, reset state, reject promise. +- No need for `statuschange`/`dispose` event listeners for cancellation (PR #188 approach eliminated). + +### `packages/core/src/types.ts` + +- Update `onAskUser` type in public API. + +## Error identification + +Unify abort detection in the main loop catch block: + +```ts +const isAbortError = this.#abortController.signal.aborted +``` + +Use `signal.aborted` as the canonical check — it's always true when we intentionally stop, regardless of how the error was thrown or wrapped. + +## Trade-offs + +| Decision | Rationale | +| ------------------------------------------------ | ------------------------------------------------------------------------------------------------------------ | +| No forced unblock (`abortable` rejected) | Exposing misbehaving tools is better than masking their bugs. Developers see the warning and fix their tool. | +| Internal tools guarantee signal compliance | Solves the original issue without framework-level workarounds. | +| PageController guard deferred | Adds coupling. Not needed when tools comply with signal. Revisit if custom tool ecosystem grows. | +| `ctx` as second param (not merged into `this`) | Keeps tool `this` clean as PageAgentCore. Signal is per-invocation context, not agent identity. | +| `onAskUser` signal is optional in options object | Non-breaking for existing consumers. | +| 3s deadline for warning | Long enough for legitimate async (DOM animations), short enough to surface bugs quickly. | + +## Validation + +- `stop()` during `ask_user` → signal fires → Panel rejects promise → tool throws → loop catches → `#onDone(false)` → status = error, panel closes. +- `stop()` during `wait` → signal fires → setTimeout race resolves → tool throws → same flow. +- `stop()` during `click_element` → PageController ops are near-instant, returns before timeout. If somehow blocked, the 3s warning fires. +- `dispose()` during any tool → same as stop, plus dispose event fires after. +- Non-compliant custom tool → loop blocks, 3s warning prints tool name, developer knows exactly what to fix. +- Normal execution (no abort) → zero behavioral change, `onAbortTimeout` is cleaned up before it fires. From 78b6e2ad3c01349226e838c7db1ee2f64321aa8e Mon Sep 17 00:00:00 2001 From: Simon <10131203+gaomeng1900@users.noreply.github.com> Date: Fri, 5 Jun 2026 21:23:18 +0800 Subject: [PATCH 2/5] feat: all sync tools should respect aborting --- packages/core/src/PageAgentCore.ts | 40 ++++++++++++++++++++++---- packages/core/src/tools/index.ts | 18 ++++++++---- packages/core/src/utils/index.ts | 45 ++++++++++++++++++++++++++++-- packages/ui/src/panel/Panel.ts | 38 ++++++++++++++++++------- packages/ui/src/panel/types.ts | 3 +- 5 files changed, 121 insertions(+), 23 deletions(-) diff --git a/packages/core/src/PageAgentCore.ts b/packages/core/src/PageAgentCore.ts index aa32146..be28287 100644 --- a/packages/core/src/PageAgentCore.ts +++ b/packages/core/src/PageAgentCore.ts @@ -21,7 +21,7 @@ import type { MacroToolInput, MacroToolResult, } from './types' -import { assert, fetchLlmsTxt, normalizeResponse, uid, waitFor } from './utils' +import { assert, fetchLlmsTxt, normalizeResponse, onAbortTimeout, uid, waitFor } from './utils' export { tool, type PageAgentTool } from './tools' export type * from './types' @@ -75,12 +75,20 @@ export class PageAgentCore extends EventTarget { /** * Called when the agent needs to ask the user questions. * If unset, the `ask_user` tool will be disabled. + * The optional `signal` aborts when the task is stopped or disposed — + * implementations should reject the promise when it fires. * @example onAskUser: (q) => window.prompt(q) || '' */ - onAskUser?: (question: string) => Promise + onAskUser?: (question: string, options?: { signal: AbortSignal }) => Promise #status: AgentStatus = 'idle' #llm: LLM + /** + * Task cancellation primitive: its signal reaches the LLM fetch, tools + * (via `ctx.signal`) and async callbacks. Aborted only by `stop`/`dispose` + * (during a task) or task setup, always WITHOUT a reason so `signal.reason` + * stays a standard `AbortError`. Never abort as a cleanup/error shortcut. + */ #abortController = new AbortController() #observations: string[] = [] @@ -140,6 +148,11 @@ export class PageAgentCore extends EventTarget { return this.#status } + /** Abort signal for the current task. Tools get it via `ctx.signal`. */ + get abortSignal(): AbortSignal { + return this.#abortController.signal + } + /** Emit statuschange event */ #emitStatusChange(): void { this.dispatchEvent(new Event('statuschange')) @@ -302,7 +315,8 @@ export class PageAgentCore extends EventTarget { } } catch (error: unknown) { console.groupEnd() // to prevent nested groups - const isAbortError = (error as any)?.name === 'AbortError' + // Canonical abort check, independent of how the error was wrapped. + const isAbortError = this.#abortController.signal.aborted if (!isAbortError) console.error('Task failed', error) const errorMessage = isAbortError ? 'Task stopped' : String(error) @@ -400,8 +414,24 @@ export class PageAgentCore extends EventTarget { const startTime = Date.now() - // Execute tool, bind `this` to PageAgent - const result = await tool.execute.bind(this)(toolInput) + // Run the tool with `this` = agent and the abort signal exposed. + // The deadline warning surfaces tools that ignore the signal + // without unblocking the loop, keeping the bug visible. + const signal = this.#abortController.signal + const unsubscribe = onAbortTimeout(signal, 3000, () => { + console.warn( + `[PageAgent] Tool "${toolName}" did not respond to abort signal within 3s. ` + + `Tools MUST honor ctx.signal for proper cancellation. ` + + `See: https://page-agent.dev/docs/custom-tools#abort` + ) + }) + + let result: string + try { + result = await tool.execute.bind(this)(toolInput, { signal }) + } finally { + unsubscribe() + } const duration = Date.now() - startTime console.log(chalk.green.bold(`Tool (${toolName}) executed for ${duration}ms`), result) diff --git a/packages/core/src/tools/index.ts b/packages/core/src/tools/index.ts index 4cc8d56..b6839b9 100644 --- a/packages/core/src/tools/index.ts +++ b/packages/core/src/tools/index.ts @@ -7,6 +7,14 @@ import * as z from 'zod/v4' import type { PageAgentCore } from '../PageAgentCore' import { waitFor } from '../utils' +/** + * Per-invocation context passed to every tool execution. + * Tools MUST honor `signal` to support cooperative cancellation. + */ +export interface ToolContext { + signal: AbortSignal +} + /** * Internal tool definition that has access to PageAgent `this` context */ @@ -14,7 +22,7 @@ export interface PageAgentTool { // name: string description: string inputSchema: z.ZodType - execute: (this: PageAgentCore, args: TParams) => Promise + execute: (this: PageAgentCore, args: TParams, ctx: ToolContext) => Promise } export function tool(options: PageAgentTool): PageAgentTool { @@ -50,12 +58,12 @@ tools.set( inputSchema: z.object({ seconds: z.number().min(1).max(10).default(1), }), - execute: async function (this: PageAgentCore, input) { + execute: async function (this: PageAgentCore, input, { signal }) { // try to subtract LLM calling time from the actual wait time const lastTimeUpdate = await this.pageController.getLastUpdateTime() const actualWaitTime = Math.max(0, input.seconds - (Date.now() - lastTimeUpdate) / 1000) console.log(`actualWaitTime: ${actualWaitTime} seconds`) - await waitFor(actualWaitTime) + await waitFor(actualWaitTime, signal) return `✅ Waited for ${input.seconds} seconds.` }, @@ -70,11 +78,11 @@ tools.set( inputSchema: z.object({ question: z.string(), }), - execute: async function (this: PageAgentCore, input) { + execute: async function (this: PageAgentCore, input, { signal }) { if (!this.onAskUser) { throw new Error('ask_user tool requires onAskUser callback to be set') } - const answer = await this.onAskUser(input.question) + const answer = await this.onAskUser(input.question, { signal }) return `User answered: ${answer}` }, }) diff --git a/packages/core/src/utils/index.ts b/packages/core/src/utils/index.ts index 973b6ec..0c27407 100644 --- a/packages/core/src/utils/index.ts +++ b/packages/core/src/utils/index.ts @@ -2,8 +2,49 @@ import chalk from 'chalk' export * from './autoFixer' -export async function waitFor(seconds: number): Promise { - await new Promise((resolve) => setTimeout(resolve, seconds * 1000)) +/** + * Wait for `seconds`. If a `signal` is provided, the wait is cancellable: + * aborting rejects with the signal's reason (an `AbortError`). + */ +export async function waitFor(seconds: number, signal?: AbortSignal): Promise { + if (!signal) { + await new Promise((resolve) => setTimeout(resolve, seconds * 1000)) + return + } + signal.throwIfAborted() + await new Promise((resolve, reject) => { + const timer = setTimeout(() => { + signal.removeEventListener('abort', onAbort) + resolve() + }, seconds * 1000) + const onAbort = () => { + clearTimeout(timer) + // reason is a DOMException AbortError. + reject(signal.reason as DOMException) + } + signal.addEventListener('abort', onAbort, { once: true }) + }) +} + +/** + * Diagnostic deadline: fire `callback` if `signal` stays aborted for `ms`, + * surfacing async work that ignores abort. Returns an unsubscribe to call once + * the awaited work settles. + */ +export function onAbortTimeout(signal: AbortSignal, ms: number, callback: () => void): () => void { + if (signal.aborted) { + callback() + return () => {} + } + let timer: ReturnType | null = null + const onAbort = () => { + timer = setTimeout(callback, ms) + } + signal.addEventListener('abort', onAbort, { once: true }) + return () => { + signal.removeEventListener('abort', onAbort) + if (timer) clearTimeout(timer) + } } // diff --git a/packages/ui/src/panel/Panel.ts b/packages/ui/src/panel/Panel.ts index ae15473..3972839 100644 --- a/packages/ui/src/panel/Panel.ts +++ b/packages/ui/src/panel/Panel.ts @@ -68,7 +68,7 @@ export class Panel { this.#i18n = new I18n(config.language ?? 'en-US') // Set up askUser callback on agent - this.#agent.onAskUser = (question) => this.#askUser(question) + this.#agent.onAskUser = (question, options) => this.#askUser(question, options?.signal) // Create UI elements this.#wrapper = this.#createWrapper() @@ -169,10 +169,12 @@ export class Panel { } /** - * Ask for user input (internal, called by agent via onAskUser) + * Ask for user input (internal, called by agent via onAskUser). + * Rejects when `signal` aborts (task stopped or disposed), cleaning up the + * question card and pending state so the agent loop can settle. */ - #askUser(question: string): Promise { - return new Promise((resolve) => { + #askUser(question: string, signal?: AbortSignal): Promise { + return new Promise((resolve, reject) => { // Set `waiting for user answer` state this.#isWaitingForUserAnswer = true this.#userAnswerResolver = resolve @@ -195,6 +197,27 @@ export class Panel { this.#scrollToBottom() this.#showInputArea(this.#i18n.t('ui.panel.userAnswerPrompt')) + + signal?.addEventListener( + 'abort', + () => { + this.#removeTempCards() + this.#isWaitingForUserAnswer = false + this.#userAnswerResolver = null + // reason is a DOMException AbortError (abort() takes no args). + reject(signal.reason as DOMException) + }, + { once: true } + ) + }) + } + + /** Remove temporary question cards (only direct children for safety) */ + #removeTempCards(): void { + Array.from(this.#historySection.children).forEach((child) => { + if (child.getAttribute('data-temp-card') === 'true') { + child.remove() + } }) } @@ -307,12 +330,7 @@ export class Panel { * Handle user answer */ #handleUserAnswer(input: string): void { - // Remove temporary question cards (only direct children for safety) - Array.from(this.#historySection.children).forEach((child) => { - if (child.getAttribute('data-temp-card') === 'true') { - child.remove() - } - }) + this.#removeTempCards() // Reset state this.#isWaitingForUserAnswer = false diff --git a/packages/ui/src/panel/types.ts b/packages/ui/src/panel/types.ts index 4b7a419..9e65b8a 100644 --- a/packages/ui/src/panel/types.ts +++ b/packages/ui/src/panel/types.ts @@ -63,8 +63,9 @@ export interface PanelAgentAdapter extends EventTarget { * Called when the agent needs to ask the user questions. * If unset, the `ask_user` tool will be disabled. * Panel will set this to handle user questions via its UI. + * The optional `signal` aborts when the task is stopped or disposed. */ - onAskUser?: (question: string) => Promise + onAskUser?: (question: string, options?: { signal: AbortSignal }) => Promise /** Execute a task */ execute(task: string): Promise From f8676a5cc2bf5ddbcf3ad0886d393f00187c4d17 Mon Sep 17 00:00:00 2001 From: Simon <10131203+gaomeng1900@users.noreply.github.com> Date: Mon, 8 Jun 2026 17:27:05 +0800 Subject: [PATCH 3/5] fix: throw unhonored AbortError & rm detection code for it --- packages/core/src/PageAgentCore.ts | 28 +++++++--------------------- packages/core/src/utils/index.ts | 21 --------------------- 2 files changed, 7 insertions(+), 42 deletions(-) diff --git a/packages/core/src/PageAgentCore.ts b/packages/core/src/PageAgentCore.ts index be28287..b6edab1 100644 --- a/packages/core/src/PageAgentCore.ts +++ b/packages/core/src/PageAgentCore.ts @@ -21,7 +21,7 @@ import type { MacroToolInput, MacroToolResult, } from './types' -import { assert, fetchLlmsTxt, normalizeResponse, onAbortTimeout, uid, waitFor } from './utils' +import { assert, fetchLlmsTxt, normalizeResponse, uid, waitFor } from './utils' export { tool, type PageAgentTool } from './tools' export type * from './types' @@ -87,7 +87,7 @@ export class PageAgentCore extends EventTarget { * Task cancellation primitive: its signal reaches the LLM fetch, tools * (via `ctx.signal`) and async callbacks. Aborted only by `stop`/`dispose` * (during a task) or task setup, always WITHOUT a reason so `signal.reason` - * stays a standard `AbortError`. Never abort as a cleanup/error shortcut. + * stays a standard `AbortError`. */ #abortController = new AbortController() #observations: string[] = [] @@ -382,7 +382,8 @@ export class PageAgentCore extends EventTarget { description: 'You MUST call this tool every step!', inputSchema: macroToolSchema as z.ZodType, execute: async (input: MacroToolInput): Promise => { - this.#abortController.signal.throwIfAborted() + const signal = this.#abortController.signal + signal.throwIfAborted() console.log(chalk.blue.bold('MacroTool input'), input) const action = input.action @@ -414,24 +415,9 @@ export class PageAgentCore extends EventTarget { const startTime = Date.now() - // Run the tool with `this` = agent and the abort signal exposed. - // The deadline warning surfaces tools that ignore the signal - // without unblocking the loop, keeping the bug visible. - const signal = this.#abortController.signal - const unsubscribe = onAbortTimeout(signal, 3000, () => { - console.warn( - `[PageAgent] Tool "${toolName}" did not respond to abort signal within 3s. ` + - `Tools MUST honor ctx.signal for proper cancellation. ` + - `See: https://page-agent.dev/docs/custom-tools#abort` - ) - }) - - let result: string - try { - result = await tool.execute.bind(this)(toolInput, { signal }) - } finally { - unsubscribe() - } + const result = await tool.execute.bind(this)(toolInput, { signal }) + // Enforce abort even if the tool ignored the signal and resolved normally. + signal.throwIfAborted() const duration = Date.now() - startTime console.log(chalk.green.bold(`Tool (${toolName}) executed for ${duration}ms`), result) diff --git a/packages/core/src/utils/index.ts b/packages/core/src/utils/index.ts index 0c27407..f030864 100644 --- a/packages/core/src/utils/index.ts +++ b/packages/core/src/utils/index.ts @@ -26,27 +26,6 @@ export async function waitFor(seconds: number, signal?: AbortSignal): Promise void): () => void { - if (signal.aborted) { - callback() - return () => {} - } - let timer: ReturnType | null = null - const onAbort = () => { - timer = setTimeout(callback, ms) - } - signal.addEventListener('abort', onAbort, { once: true }) - return () => { - signal.removeEventListener('abort', onAbort) - if (timer) clearTimeout(timer) - } -} - // export function truncate(text: string, maxLength: number): string { From 6530f0ef40dfeacbd33dd85fd10d23b7108545ed Mon Sep 17 00:00:00 2001 From: Simon <10131203+gaomeng1900@users.noreply.github.com> Date: Mon, 8 Jun 2026 17:40:44 +0800 Subject: [PATCH 4/5] chore: cleanup; rm `get abortSignal` --- docs/plan-abort-signal.md | 176 ----------------------------- packages/core/src/PageAgentCore.ts | 8 +- 2 files changed, 1 insertion(+), 183 deletions(-) delete mode 100644 docs/plan-abort-signal.md diff --git a/docs/plan-abort-signal.md b/docs/plan-abort-signal.md deleted file mode 100644 index b8c3d51..0000000 --- a/docs/plan-abort-signal.md +++ /dev/null @@ -1,176 +0,0 @@ -# Plan: Unified Abort Signal for Tool Cancellation - -## Problem - -When `stop()` or `dispose()` is called while a tool is executing, the agent's main loop remains blocked on `await tool.execute(...)`. The abort signal only reaches the LLM HTTP fetch — it never reaches tool execution or async callbacks like `onAskUser`. - -Consequence: status never transitions, UI cannot clean up, panel cannot close. PR #188 patched this for `ask_user` alone by listening to `statuschange`/`dispose` events in Panel — a per-tool band-aid that doesn't scale. - -### Current abort coverage - -| Path | Receives abort? | -| -------------------------------- | ----------------------------------- | -| LLM HTTP request (`fetch`) | Yes — signal passed to fetch | -| MacroTool entry guard (line 382) | Yes — manual `signal.aborted` check | -| **In-flight tool execution** | No | -| `onAskUser` Promise | No | -| `wait` setTimeout | No | -| Custom tool async work | No | - -## Design Principles - -- **Expose errors, don't mask them.** If a tool doesn't respect abort, that's a bug in the tool. The framework should surface it loudly, not silently work around it. -- **AbortSignal as the single cancellation primitive.** Standard web platform pattern. All async work subscribes to it. -- **Internal tools must respect signal.** This solves the original issue (ask_user, wait). No forced unblock needed. -- **Abort trigger points must remain controlled.** The entire cancellation system hinges on this one signal. Its trigger conditions must be deliberate and auditable. See "Abort trigger audit" below. - -## Design - -Two layers, plus a diagnostic mechanism: - -### Layer 1: Expose signal to tools (cooperative cancellation) - -Pass signal into every tool execution. Well-behaved tools self-cancel immediately. - -**Tool signature change:** - -```ts -// Before -execute: (this: PageAgentCore, args: TParams) => Promise - -// After -execute: (this: PageAgentCore, args: TParams, ctx: { signal: AbortSignal }) => Promise -``` - -Backward-compatible — tools that ignore ctx still compile and run. - -**`onAskUser` signature change:** - -```ts -// Before -onAskUser?: (question: string) => Promise - -// After -onAskUser?: (question: string, options?: { signal: AbortSignal }) => Promise -``` - -### Layer 2: Abort deadline warning (diagnostic, not rescue) - -Instead of silently unblocking the main loop when a tool ignores signal, detect the violation and warn loudly. - -```ts -// packages/core/src/utils/index.ts -export function onAbortTimeout(signal: AbortSignal, ms: number, callback: () => void): () => void { - if (signal.aborted) { - callback() - return () => {} - } - let timer: ReturnType | null = null - const onAbort = () => { - timer = setTimeout(callback, ms) - } - signal.addEventListener('abort', onAbort, { once: true }) - return () => { - signal.removeEventListener('abort', onAbort) - if (timer) clearTimeout(timer) - } -} -``` - -Applied at the tool execution site in `#packMacroTool`: - -```ts -// PageAgentCore.ts — inside macroTool.execute -const signal = this.#abortController.signal - -const unsubscribe = onAbortTimeout(signal, 3000, () => { - console.warn( - `[PageAgent] Tool "${toolName}" did not respond to abort signal within 3s. ` + - `Tools MUST honor ctx.signal for proper cancellation. ` + - `See: https://page-agent.dev/docs/custom-tools#abort` - ) -}) - -const result = await tool.execute.bind(this)(toolInput, { signal }) -unsubscribe() -``` - -**This does NOT unblock the loop.** If a custom tool hangs, the agent hangs — visibly. The warning tells the developer exactly which tool is at fault. Hiding the problem (via forced unblock) would make bugs harder to diagnose and give false confidence that stop() always works instantly. - -### Layer 3 (future): PageController guard - -Not implemented now. Future enhancement: `PageController.arm(signal)` + `#assertAlive()` at the top of every public method. Would prevent orphaned tools from executing DOM side-effects. Adds hardening for custom tools that ignore signal but still call controller methods. - -## Abort trigger audit - -`#abortController.abort()` is called from exactly 4 sites: - -| Call site | Context | Can fire during tool execution? | -| ----------------- | ---------------------------------------------------------------------- | ------------------------------------------------- | -| `stop()` | User clicks stop | Yes — this is the primary cancel path | -| `dispose()` | Agent destroyed | Yes — equivalent to stop + teardown | -| `execute()` entry | Reset before new task, immediately followed by `new AbortController()` | No — new signal replaces old before any tool runs | -| `#onDone()` | Task already finished (loop exited) | No — defensive cleanup, no tool in flight | - -Only `stop()` and `dispose()` can fire the signal while a tool is executing. Both represent explicit, user-initiated intent to cancel. - -**Invariant:** No code path should call `abort()` as a side-effect, cleanup shortcut, or error-recovery mechanism. If a new call site is ever added, it must be audited against this table. Adding an accidental abort trigger would silently kill in-flight tools — a class of bug that's extremely hard to reproduce and diagnose. - -## Changes Required - -### `packages/core/src/utils/index.ts` - -- Add `onAbortTimeout()` utility. - -### `packages/core/src/tools/index.ts` - -- Update `PageAgentTool` interface: add `ctx: { signal: AbortSignal }` as second param to `execute`. -- `ask_user` tool: listen to signal, reject with AbortError on abort. Pass `{ signal }` to `this.onAskUser`. -- `wait` tool: race setTimeout against signal. On abort, resolve immediately with AbortError. - -### `packages/core/src/PageAgentCore.ts` - -- Expose `get abortSignal(): AbortSignal`. -- In `#packMacroTool` execute: pass `{ signal }` as ctx to tool. Add `onAbortTimeout` diagnostic. -- Keep the manual `if (signal.aborted) throw` guard at entry (cheap fast-path before tool runs). -- `stop()`: remains simple — `abort()` + cleanup. No `#setStatus('error')` hack. Status transitions naturally when the tool throws AbortError → catch block → `#onDone(false)`. -- Update `onAskUser` type to accept optional `{ signal }`. - -### `packages/ui/src/panel/Panel.ts` - -- `#askUser`: subscribe to signal abort → clean up question card, reset state, reject promise. -- No need for `statuschange`/`dispose` event listeners for cancellation (PR #188 approach eliminated). - -### `packages/core/src/types.ts` - -- Update `onAskUser` type in public API. - -## Error identification - -Unify abort detection in the main loop catch block: - -```ts -const isAbortError = this.#abortController.signal.aborted -``` - -Use `signal.aborted` as the canonical check — it's always true when we intentionally stop, regardless of how the error was thrown or wrapped. - -## Trade-offs - -| Decision | Rationale | -| ------------------------------------------------ | ------------------------------------------------------------------------------------------------------------ | -| No forced unblock (`abortable` rejected) | Exposing misbehaving tools is better than masking their bugs. Developers see the warning and fix their tool. | -| Internal tools guarantee signal compliance | Solves the original issue without framework-level workarounds. | -| PageController guard deferred | Adds coupling. Not needed when tools comply with signal. Revisit if custom tool ecosystem grows. | -| `ctx` as second param (not merged into `this`) | Keeps tool `this` clean as PageAgentCore. Signal is per-invocation context, not agent identity. | -| `onAskUser` signal is optional in options object | Non-breaking for existing consumers. | -| 3s deadline for warning | Long enough for legitimate async (DOM animations), short enough to surface bugs quickly. | - -## Validation - -- `stop()` during `ask_user` → signal fires → Panel rejects promise → tool throws → loop catches → `#onDone(false)` → status = error, panel closes. -- `stop()` during `wait` → signal fires → setTimeout race resolves → tool throws → same flow. -- `stop()` during `click_element` → PageController ops are near-instant, returns before timeout. If somehow blocked, the 3s warning fires. -- `dispose()` during any tool → same as stop, plus dispose event fires after. -- Non-compliant custom tool → loop blocks, 3s warning prints tool name, developer knows exactly what to fix. -- Normal execution (no abort) → zero behavioral change, `onAbortTimeout` is cleaned up before it fires. diff --git a/packages/core/src/PageAgentCore.ts b/packages/core/src/PageAgentCore.ts index b6edab1..379df56 100644 --- a/packages/core/src/PageAgentCore.ts +++ b/packages/core/src/PageAgentCore.ts @@ -75,8 +75,7 @@ export class PageAgentCore extends EventTarget { /** * Called when the agent needs to ask the user questions. * If unset, the `ask_user` tool will be disabled. - * The optional `signal` aborts when the task is stopped or disposed — - * implementations should reject the promise when it fires. + * Implementations should reject the promise when `signal` aborts. * @example onAskUser: (q) => window.prompt(q) || '' */ onAskUser?: (question: string, options?: { signal: AbortSignal }) => Promise @@ -148,11 +147,6 @@ export class PageAgentCore extends EventTarget { return this.#status } - /** Abort signal for the current task. Tools get it via `ctx.signal`. */ - get abortSignal(): AbortSignal { - return this.#abortController.signal - } - /** Emit statuschange event */ #emitStatusChange(): void { this.dispatchEvent(new Event('statuschange')) From 4445bec08a3745f012a6db9f15499042c02ba68c Mon Sep 17 00:00:00 2001 From: Simon <10131203+gaomeng1900@users.noreply.github.com> Date: Mon, 8 Jun 2026 19:46:19 +0800 Subject: [PATCH 5/5] fix: abort may affect onAfterTask; clean up while loop --- packages/core/src/PageAgentCore.ts | 67 ++++++++++++------------------ 1 file changed, 27 insertions(+), 40 deletions(-) diff --git a/packages/core/src/PageAgentCore.ts b/packages/core/src/PageAgentCore.ts index 379df56..d6ac114 100644 --- a/packages/core/src/PageAgentCore.ts +++ b/packages/core/src/PageAgentCore.ts @@ -225,6 +225,8 @@ export class PageAgentCore extends EventTarget { this.#states = { totalWaitTime: 0, lastURL: '', browserState: null } let step = 0 + let taskSuccess: boolean + let taskResult: string while (true) { try { @@ -286,64 +288,49 @@ export class PageAgentCore extends EventTarget { } as AgentStepEvent) this.#emitHistoryChange() - // - await onAfterStep?.(this, this.history) console.groupEnd() - // finish task if done - if (actionName === 'done') { - const success = action.input?.success ?? false - const text = action.input?.text || 'no text provided' - console.log(chalk.green.bold('Task completed'), success, text) - this.#onDone(success) - const result: ExecutionResult = { - success, - data: text, - history: this.history, - } - await onAfterTask?.(this, result) - return result + taskSuccess = action.input?.success ?? false + taskResult = action.input?.text || 'no text provided' + console.log(chalk.green.bold('Task completed'), taskSuccess, taskResult) + break } } catch (error: unknown) { - console.groupEnd() // to prevent nested groups - // Canonical abort check, independent of how the error was wrapped. - const isAbortError = this.#abortController.signal.aborted - + console.groupEnd() + const isAbortError = (error as any)?.name === 'AbortError' if (!isAbortError) console.error('Task failed', error) - const errorMessage = isAbortError ? 'Task stopped' : String(error) - this.#emitActivity({ type: 'error', message: errorMessage }) - this.history.push({ type: 'error', message: errorMessage, rawResponse: error }) + taskResult = isAbortError ? 'Task aborted' : String(error) + taskSuccess = false + this.#emitActivity({ type: 'error', message: taskResult }) + this.history.push({ type: 'error', message: taskResult, rawResponse: error }) this.#emitHistoryChange() - this.#onDone(false) - const result: ExecutionResult = { - success: false, - data: errorMessage, - history: this.history, - } - await onAfterTask?.(this, result) - return result + break } step++ if (step > this.config.maxSteps) { - const errorMessage = 'Step count exceeded maximum limit' - this.history.push({ type: 'error', message: errorMessage }) + taskResult = 'Step count exceeded maximum limit' + taskSuccess = false + this.#emitActivity({ type: 'error', message: taskResult }) + this.history.push({ type: 'error', message: taskResult }) this.#emitHistoryChange() - this.#onDone(false) - const result: ExecutionResult = { - success: false, - data: errorMessage, - history: this.history, - } - await onAfterTask?.(this, result) - return result + break } await waitFor(this.config.stepDelay ?? 0.4) } + + this.#onDone(taskSuccess) + const result: ExecutionResult = { + success: taskSuccess, + data: taskResult, + history: this.history, + } + await onAfterTask?.(this, result) + return result } /**