From eb739e9819850199c883281b715a09283d4991c1 Mon Sep 17 00:00:00 2001 From: Shangxin Date: Sat, 17 Oct 2026 12:18:51 +0000 Subject: [PATCH] refactor(agent): decouple input suggestion and typing service architecture 1. Decoupled TextTypingExecutor domain interface from concrete TextTypingService. 2. Eliminated fake ToolDefinition.execute calls and dummy contexts in InputSuggestionService. 3. Decoupled inputSuggestionExtension to accept pure typeTextTool and Resettable lifecycle. 4. Cleaned up watch-session composition root into a strict acyclic DAG topology. 5. Handled tool abortion and typing errors defensively. 6. Expanded unit test suite and passed visual smoke tests. --- src/input-suggestion-extension.ts | 22 +++--- src/input-suggestion.ts | 99 +++++++++++++++++------- src/text-typing-service.ts | 14 +++- src/type-text-tool.ts | 55 +++++++------ src/watch-session.ts | 11 +-- tests/input-suggestion-extension.test.ts | 57 ++++++++++++-- tests/input-suggestion.test.ts | 42 ++++++++++ 7 files changed, 227 insertions(+), 73 deletions(-) diff --git a/src/input-suggestion-extension.ts b/src/input-suggestion-extension.ts index 0e375d7..dae64de 100644 --- a/src/input-suggestion-extension.ts +++ b/src/input-suggestion-extension.ts @@ -1,17 +1,18 @@ import type { ExtensionAPI, InlineExtension, ToolDefinition } from "@earendil-works/pi-coding-agent"; -import type { InputSuggestionService } from "./input-suggestion.js"; +import type { Resettable } from "./input-suggestion.js"; export interface InputSuggestionExtensionOptions { - readonly service: InputSuggestionService; readonly typeTextTool?: ToolDefinition | undefined; + readonly lifecycle?: Resettable | undefined; + readonly service?: (Resettable & { getTypeTextTool?: () => ToolDefinition }) | undefined; } /** - * Pi Agent InlineExtension for proactive input suggestions. + * Pi Agent InlineExtension for desktop text typing & proactive input suggestions. * * Adheres strictly to the `@earendil-works/pi-coding-agent` ExtensionAPI specification. - * Integrates the suggestion service lifecycle with the Pi Agent session and registers - * the official `type_text` tool dynamically. + * Dynamically registers the official `type_text` tool and ties lifecycle cleanup to + * `session_shutdown`. */ export function inputSuggestionExtension( options: InputSuggestionExtensionOptions, @@ -20,14 +21,17 @@ export function inputSuggestionExtension( name: "squid-input-suggestion", hidden: true, factory: (pi: ExtensionAPI) => { - const toolToRegister = options.typeTextTool ?? options.service.getTypeTextTool?.(); + const toolToRegister = options.typeTextTool ?? options.service?.getTypeTextTool?.(); if (toolToRegister !== undefined) { pi.registerTool(toolToRegister); } - pi.on("session_shutdown", () => { - options.service.reset?.(); - }); + const targetLifecycle = options.lifecycle ?? options.service; + if (targetLifecycle?.reset) { + pi.on("session_shutdown", () => { + targetLifecycle.reset(); + }); + } }, }; } diff --git a/src/input-suggestion.ts b/src/input-suggestion.ts index b0bdfb0..f036140 100644 --- a/src/input-suggestion.ts +++ b/src/input-suggestion.ts @@ -5,7 +5,7 @@ * editor, or terminal). Infers/predicts what the user likely intends to enter, * presents the proposed text to the user via Squid's native ConfirmationCard, * and upon user acceptance ("Allow"), automatically types the suggested text - * into the target field using the standard Pi Agent `type_text` tool. + * into the target field using the TextTypingExecutor. * * Honors the GUI switch / environment variable (SQUID_INPUT_SUGGESTIONS !== "off"). */ @@ -16,7 +16,11 @@ import type { ActionConfirmationRequest, } from "./action-confirmation.js"; import type { FocusChangedPayload, WorldState } from "./protocol.js"; -import { TextTypingService } from "./text-typing-service.js"; +import { + TextTypingService, + type TextTypingExecutor, + type TextTypingTarget, +} from "./text-typing-service.js"; import { createTypeTextTool, type typeTextParams, @@ -33,12 +37,22 @@ export interface SuggestionContext { readonly worldState: WorldState | null; } +export type InputPredictor = ( + context: SuggestionContext, +) => Promise | InputSuggestion | null; + +export interface Resettable { + reset(): void; +} + export interface InputSuggestionServiceOptions { readonly isEnabled: () => boolean; readonly confirmationPolicy: ActionConfirmationPolicy; - readonly textTypingService?: TextTypingService | undefined; + readonly textTypingService?: TextTypingExecutor | undefined; + /** @deprecated Pass textTypingService instead. Retained for backward compatibility. */ readonly typeTextTool?: ToolDefinition | undefined; - readonly predictSuggestion?: ((context: SuggestionContext) => Promise | InputSuggestion | null) | undefined; + readonly predictSuggestion?: InputPredictor | undefined; + readonly onSuggestionAccepted?: ((suggestion: InputSuggestion, payload: FocusChangedPayload) => void) | undefined; readonly cooldownMs?: number | undefined; readonly promptTimeoutMs?: number | undefined; readonly log?: ((line: string) => void) | undefined; @@ -142,17 +156,19 @@ export function heuristicSuggestInput(context: SuggestionContext): InputSuggesti }; } -export class InputSuggestionService { +export class InputSuggestionService implements Resettable { private readonly isEnabled: () => boolean; private readonly confirmationPolicy: ActionConfirmationPolicy; - private readonly typeTextTool: ToolDefinition; + private readonly typingExecutor: TextTypingExecutor; private readonly predictSuggestion: (context: SuggestionContext) => Promise; + private readonly onSuggestionAccepted?: ((suggestion: InputSuggestion, payload: FocusChangedPayload) => void) | undefined; private readonly cooldownMs: number; private readonly promptTimeoutMs: number; private readonly log?: ((line: string) => void) | undefined; private readonly logError?: ((line: string) => void) | undefined; private readonly now: () => number; + private _typeTextTool?: ToolDefinition | undefined; private lastTargetKey = ""; private lastTriggerTime = 0; private inFlight = false; @@ -160,15 +176,36 @@ export class InputSuggestionService { constructor(options: InputSuggestionServiceOptions) { this.isEnabled = options.isEnabled; this.confirmationPolicy = options.confirmationPolicy; - if (options.typeTextTool !== undefined) { - this.typeTextTool = options.typeTextTool; - } else if (options.textTypingService !== undefined) { - this.typeTextTool = createTypeTextTool(options.textTypingService); + if (options.textTypingService !== undefined) { + this.typingExecutor = options.textTypingService; + } else if (options.typeTextTool !== undefined) { + const tool = options.typeTextTool; + this._typeTextTool = tool; + this.typingExecutor = { + async typeText(target: TextTypingTarget) { + const res = await tool.execute( + `exec-${Date.now()}`, + { + text: target.text, + ...(target.targetWindowTitle !== undefined ? { targetWindowTitle: target.targetWindowTitle } : {}), + ...(target.targetApplicationId !== undefined ? { targetApplicationId: target.targetApplicationId } : {}), + }, + undefined, + undefined, + {} as any, + ); + return { + ok: res.details?.ok ?? true, + ...(res.details?.error !== undefined ? { error: res.details.error } : {}), + }; + }, + }; } else { - this.typeTextTool = createTypeTextTool(new TextTypingService()); + this.typingExecutor = new TextTypingService(); } const predictor = options.predictSuggestion ?? heuristicSuggestInput; this.predictSuggestion = async (ctx) => predictor(ctx); + this.onSuggestionAccepted = options.onSuggestionAccepted; this.cooldownMs = options.cooldownMs ?? 4_000; this.promptTimeoutMs = options.promptTimeoutMs ?? 25_000; this.log = options.log; @@ -177,10 +214,20 @@ export class InputSuggestionService { } /** - * Returns the registered Pi Agent ToolDefinition. + * Returns the registered Pi Agent ToolDefinition adapter. */ getTypeTextTool(): ToolDefinition { - return this.typeTextTool; + if (!this._typeTextTool) { + this._typeTextTool = createTypeTextTool(this.typingExecutor); + } + return this._typeTextTool; + } + + /** + * Returns the underlying TextTypingExecutor. + */ + getTypingExecutor(): TextTypingExecutor { + return this.typingExecutor; } /** @@ -253,28 +300,26 @@ export class InputSuggestionService { }), ); - // 6. User accepted: auto-type into target window via standard Pi Agent tool - const toolResult = await this.typeTextTool.execute( - `suggest-${nowMs}`, - { - text: suggestion.text, - ...(payload.windowTitle !== undefined ? { targetWindowTitle: payload.windowTitle } : {}), - ...(payload.applicationId !== undefined ? { targetApplicationId: payload.applicationId } : {}), - }, - undefined, - undefined, - {} as any, - ); + // 6. User accepted: auto-type into target window via typing executor + const target: TextTypingTarget = { + text: suggestion.text, + ...(payload.windowTitle !== undefined ? { targetWindowTitle: payload.windowTitle } : {}), + ...(payload.applicationId !== undefined ? { targetApplicationId: payload.applicationId } : {}), + }; + + const result = await this.typingExecutor.typeText(target); - if (toolResult.details?.ok === false) { + if (!result.ok) { this.logError?.( JSON.stringify({ inputSuggestionTypingFailed: { target: targetKey, - error: toolResult.details.error, + error: result.error ?? "unknown error", }, }), ); + } else { + this.onSuggestionAccepted?.(suggestion, payload); } } else { this.log?.( diff --git a/src/text-typing-service.ts b/src/text-typing-service.ts index 4edaa4f..99db87a 100644 --- a/src/text-typing-service.ts +++ b/src/text-typing-service.ts @@ -16,6 +16,7 @@ export interface TextTypingTarget { readonly targetWindowId?: string; readonly targetWindowTitle?: string; readonly targetApplicationId?: string; + readonly signal?: AbortSignal | undefined; } export interface TextTypingResult { @@ -23,6 +24,14 @@ export interface TextTypingResult { readonly error?: string; } +/** + * Abstraction for text typing execution. + * Decouples consumers (e.g. InputSuggestionService, typeTextTool) from concrete OS runners. + */ +export interface TextTypingExecutor { + typeText(target: TextTypingTarget): Promise; +} + export interface SubprocessExecutionResult { readonly exitCode: number; readonly stdout?: string; @@ -89,7 +98,7 @@ export function escapeSendKeys(text: string): string { .replace(/\r?\n/g, "{ENTER}"); } -export class TextTypingService { +export class TextTypingService implements TextTypingExecutor { private readonly platform: NodeJS.Platform; private readonly runner: SubprocessRunner; private readonly timeoutMs: number; @@ -110,6 +119,9 @@ export class TextTypingService { * Types the specified text into the focused target input. */ async typeText(target: TextTypingTarget): Promise { + if (target.signal?.aborted) { + return { ok: false, error: "Aborted" }; + } const { text, targetWindowId, targetWindowTitle, targetApplicationId } = target; if (text.length === 0) { return { ok: true }; diff --git a/src/type-text-tool.ts b/src/type-text-tool.ts index 06b562b..c077525 100644 --- a/src/type-text-tool.ts +++ b/src/type-text-tool.ts @@ -1,28 +1,28 @@ -import { Type } from "typebox"; -import type { Static } from "typebox"; import { defineTool, type AgentToolResult, type ExtensionToolContext, type ToolDefinition, } from "@earendil-works/pi-coding-agent"; -import { TextTypingService, type TextTypingTarget } from "./text-typing-service.js"; +import type { TextTypingExecutor, TextTypingTarget } from "./text-typing-service.js"; +import { Type, type Static } from "typebox"; /** * Parameters for the standard Pi Agent `type_text` tool. */ export const typeTextParams = Type.Object({ text: Type.String({ - description: "The text to type into the focused text target.", + description: "The text string to type into the currently active or specified input target.", }), targetWindowTitle: Type.Optional( Type.String({ - description: "Optional window title to activate before typing.", + description: + "Optional window title to focus and verify before typing, preventing text injection into the wrong window.", }), ), targetApplicationId: Type.Optional( Type.String({ - description: "Optional application name or process identifier to activate before typing.", + description: "Optional application identifier (e.g. process name or bundle id) for targeting.", }), ), }); @@ -30,19 +30,19 @@ export const typeTextParams = Type.Object({ export type TypeTextArgs = Static; export interface TypeTextToolDetails { - ok: boolean; - charsTyped?: number; - error?: string; + readonly ok: boolean; + readonly charsTyped?: number; + readonly error?: string; } /** * Creates the official Pi Agent ToolDefinition for typing text into desktop fields. * - * Wraps `TextTypingService` to satisfy the official `@earendil-works/pi-coding-agent` + * Wraps `TextTypingExecutor` to satisfy the official `@earendil-works/pi-coding-agent` * tool specification, standardizing argument validation, execution, and result formatting. */ export function createTypeTextTool( - service: TextTypingService, + service: TextTypingExecutor, ): ToolDefinition { return defineTool({ name: "type_text", @@ -70,23 +70,32 @@ export function createTypeTextTool( text: params.text, ...(params.targetWindowTitle !== undefined ? { targetWindowTitle: params.targetWindowTitle } : {}), ...(params.targetApplicationId !== undefined ? { targetApplicationId: params.targetApplicationId } : {}), + ...(signal !== undefined ? { signal } : {}), }; - const result = await service.typeText(target); - if (!result.ok) { + try { + const result = await service.typeText(target); + if (!result.ok) { + return { + content: [{ type: "text", text: `Failed to type text: ${result.error ?? "unknown error"}` }], + details: { + ok: false, + ...(result.error !== undefined ? { error: result.error } : {}), + }, + }; + } + + return { + content: [{ type: "text", text: `Successfully typed ${params.text.length} characters.` }], + details: { ok: true, charsTyped: params.text.length }, + }; + } catch (err: unknown) { + const message = err instanceof Error ? err.message : String(err); return { - content: [{ type: "text", text: `Failed to type text: ${result.error ?? "unknown error"}` }], - details: { - ok: false, - ...(result.error !== undefined ? { error: result.error } : {}), - }, + content: [{ type: "text", text: `Failed to type text: ${message}` }], + details: { ok: false, error: message }, }; } - - return { - content: [{ type: "text", text: `Successfully typed ${params.text.length} characters.` }], - details: { ok: true, charsTyped: params.text.length }, - }; }, }); } diff --git a/src/watch-session.ts b/src/watch-session.ts index ac8bc20..35287ec 100644 --- a/src/watch-session.ts +++ b/src/watch-session.ts @@ -1,7 +1,7 @@ import { realpath } from "node:fs/promises"; import { relative, sep } from "node:path"; import { createWatchRuntime, type WatchRuntime } from "./create-watch-runtime.js"; -import { agentCustomTools } from "./agent-tools.js"; +import { agentCustomTools, createTypeTextTool } from "./agent-tools.js"; import { describeErrorForAudit } from "./audit-text.js"; import { CaptureGateActivitySource } from "./capture-gate-source.js"; import { resolveCaptureGate } from "./resolve-capture-gate.js"; @@ -37,7 +37,7 @@ import { startPanelServer, type PanelServer } from "./panel-server.js"; import type { DesktopNotifierFactory } from "./desktop-notice.js"; import { PanelConfirmationSurface } from "./panel-confirmation-surface.js"; import { SerialConfirmationPolicy } from "./action-confirmation.js"; -import { TextTypingService } from "./text-typing-service.js"; +import { TextTypingService, type TextTypingExecutor } from "./text-typing-service.js"; import { InputSuggestionService } from "./input-suggestion.js"; import { inputSuggestionExtension } from "./input-suggestion-extension.js"; import { @@ -186,7 +186,7 @@ export interface WatchSessionOptions { * tests inject a spy instead of ending the process. */ readonly stopExit?: () => void; - readonly textTypingService?: TextTypingService; + readonly textTypingService?: TextTypingExecutor; readonly inputSuggestionService?: InputSuggestionService; } @@ -386,6 +386,7 @@ export async function startWatchSession( // current custom tools are read-only and Pi's sandbox owns tool safety. const panelConfirmation = new PanelConfirmationSurface({ server: () => panelServer }); const textTypingService = options.textTypingService ?? new TextTypingService({ env }); + const typeTextTool = createTypeTextTool(textTypingService); const confirmationPolicy = new SerialConfirmationPolicy({ surface: panelConfirmation, timeoutMs: 30_000, @@ -436,8 +437,8 @@ export async function startWatchSession( heartbeatExtension(heartbeatSink), llmLogExt, inputSuggestionExtension({ - service: inputSuggestionService, - typeTextTool: inputSuggestionService.getTypeTextTool?.(), + typeTextTool, + lifecycle: inputSuggestionService, }), ...(decisionCurator === undefined ? [] : [decisionCurator]), ], diff --git a/tests/input-suggestion-extension.test.ts b/tests/input-suggestion-extension.test.ts index 5a5305d..6800d5f 100644 --- a/tests/input-suggestion-extension.test.ts +++ b/tests/input-suggestion-extension.test.ts @@ -1,10 +1,56 @@ import { describe, expect, it } from "bun:test"; import { inputSuggestionExtension } from "../src/input-suggestion-extension.js"; -import { InputSuggestionService } from "../src/input-suggestion.js"; import type { ExtensionAPI, ToolDefinition } from "@earendil-works/pi-coding-agent"; describe("inputSuggestionExtension", () => { - it("conforms to Pi InlineExtension interface and registers type_text tool", () => { + it("conforms to Pi InlineExtension interface and registers type_text tool via decoupled options", () => { + let resetCalled = false; + const fakeTool = { + name: "type_text", + } as unknown as ToolDefinition; + + const fakeLifecycle = { + reset: () => { + resetCalled = true; + }, + }; + + const extension = inputSuggestionExtension({ + typeTextTool: fakeTool, + lifecycle: fakeLifecycle, + }); + + if (typeof extension === "function") { + throw new Error("expected object extension"); + } + + expect(extension.name).toBe("squid-input-suggestion"); + expect(extension.hidden).toBe(true); + + const registeredTools: any[] = []; + const eventHandlers: Record void> = {}; + + const fakePi = { + registerTool: (tool: any) => { + registeredTools.push(tool); + }, + on: (event: string, handler: () => void) => { + eventHandlers[event] = handler; + }, + } as unknown as ExtensionAPI; + + extension.factory(fakePi); + + expect(registeredTools).toHaveLength(1); + expect(registeredTools[0]?.name).toBe("type_text"); + + // Test session_shutdown lifecycle hook + expect(eventHandlers["session_shutdown"]).toBeDefined(); + eventHandlers["session_shutdown"]?.(); + expect(resetCalled).toBe(true); + }); + + it("supports legacy service options for backwards compatibility", () => { let resetCount = 0; const fakeService = { reset: () => { @@ -14,16 +60,13 @@ describe("inputSuggestionExtension", () => { ({ name: "type_text", }) as unknown as ToolDefinition, - } as unknown as InputSuggestionService; + }; const extension = inputSuggestionExtension({ service: fakeService }); if (typeof extension === "function") { throw new Error("expected object extension"); } - expect(extension.name).toBe("squid-input-suggestion"); - expect(extension.hidden).toBe(true); - const registeredTools: any[] = []; const eventHandlers: Record void> = {}; @@ -41,8 +84,6 @@ describe("inputSuggestionExtension", () => { expect(registeredTools).toHaveLength(1); expect(registeredTools[0]?.name).toBe("type_text"); - // Test session_shutdown lifecycle hook - expect(eventHandlers["session_shutdown"]).toBeDefined(); eventHandlers["session_shutdown"]?.(); expect(resetCount).toBe(1); }); diff --git a/tests/input-suggestion.test.ts b/tests/input-suggestion.test.ts index c0f2b07..486dbdd 100644 --- a/tests/input-suggestion.test.ts +++ b/tests/input-suggestion.test.ts @@ -291,4 +291,46 @@ describe("InputSuggestionService", () => { await suggestionService.onFocusChanged(payload, null); expect(confirmedRequests.length).toBe(2); }); + + it("supports deprecated typeTextTool option as fallback executor", async () => { + const { policy } = createMockPolicy({ + outcome: "accepted", + reason: "user_accepted", + }); + const executedParams: any[] = []; + const fakeTool = { + name: "type_text", + async execute(_id: string, params: any) { + executedParams.push(params); + return { + content: [{ type: "text", text: "ok" }], + details: { ok: true, charsTyped: params.text.length }, + }; + }, + } as any; + + const service = new InputSuggestionService({ + isEnabled: () => true, + confirmationPolicy: policy, + typeTextTool: fakeTool, + predictSuggestion: async () => ({ + text: "git status", + why: "Check repo status", + }), + }); + + await service.onFocusChanged( + { + role: "text_input", + applicationId: "terminal", + windowTitle: "bash", + label: null, + }, + null, + ); + + expect(executedParams.length).toBe(1); + expect(executedParams[0].text).toBe("git status"); + expect(executedParams[0].targetWindowTitle).toBe("bash"); + }); });