From fd86fc6668066765381a00ab11c41be443fc68e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E9=9B=B7=E7=94=B5=E8=8A=BD=E8=A1=A3?= Date: Fri, 24 Apr 2026 00:58:07 -0400 Subject: [PATCH] refactor(artifacts): reconcile workspace artifacts at completion --- src/main/agent-runtime/agent-hooks.ts | 44 ++----- .../artifact-projection-events.ts | 10 +- src/main/agent-runtime/artifact-projector.ts | 62 +--------- src/main/agent-runtime/artifact-types.ts | 12 +- .../agent-runtime/artifact-work-products.ts | 25 +--- src/main/agent-runtime/hook-handlers.ts | 114 +----------------- src/main/agent-runtime/hook-rpc.ts | 40 ++---- src/main/agent-runtime/runtime-bridge.ts | 34 ++---- src/main/worker/entry/types.ts | 1 - .../internal-tools/internal-tool-rpc.ts | 12 +- src/main/worker/runtime/pi-agent-factory.ts | 13 +- src/main/worker/runtime/pi-hook-contract.ts | 18 +-- .../artifact-projection-ack.spec.ts | 41 +------ tests/integration/pi-runtime-contract.spec.ts | 4 +- .../work-products-lifecycle.spec.ts | 48 ++++++-- tests/unit/agent-hooks.test.ts | 23 ++-- tests/unit/artifact-projector-errors.test.ts | 49 +------- 17 files changed, 114 insertions(+), 436 deletions(-) diff --git a/src/main/agent-runtime/agent-hooks.ts b/src/main/agent-runtime/agent-hooks.ts index eca2236c..838002c9 100644 --- a/src/main/agent-runtime/agent-hooks.ts +++ b/src/main/agent-runtime/agent-hooks.ts @@ -1,11 +1,11 @@ /** - * Aggregated factory for the three pi-agent-core tool-execution hooks + * Aggregated factory for the pi-agent-core runtime hooks * installed by `pi-agent-factory.createPiAgent`. * - * Before this module the factory wired `beforeToolCall` / `afterToolCall` - * / `transformContext` as three inline closures, each re-establishing + * Before this module the factory wired `beforeToolCall` and + * `transformContext` as inline closures, each re-establishing * its own RPC envelope shape and its own error policy. A new hook (or - * a signature change in pi) meant touching the factory in three places, + * a signature change in pi) meant touching the factory in multiple places, * and the "what does each hook do on failure?" matrix lived only in * reader memory. * @@ -17,20 +17,17 @@ * so the full transcript never crosses the worker-main boundary. * * The signatures below are pinned to pi via `pi-hook-contract`: any - * drift in `AgentOptions.beforeToolCall` / `afterToolCall` / - * `transformContext` breaks the build on the contract file, so the + * drift in `AgentOptions.beforeToolCall` / `transformContext` breaks + * the build on the contract file, so the * aggregation cannot silently drift from upstream. */ import type { - AfterToolCallResult, AgentMessage, AgentOptions, BeforeToolCallResult, } from "@mariozechner/pi-agent-core"; import type { - AfterToolCallRpcPayload, - AfterToolCallRpcResult, BeforeToolCallRpcPayload, BeforeToolCallRpcResult, } from "./hook-rpc"; @@ -65,12 +62,11 @@ export interface AgentHooksDeps { */ export interface AgentHookBundle { beforeToolCall: NonNullable; - afterToolCall: NonNullable; transformContext: NonNullable; } /** - * Build the three pi-agent-core hooks as a single bundle. + * Build the pi-agent-core hooks as a single bundle. * * Each hook's behavior matches the pre-aggregation inline closures in * `pi-agent-factory`: @@ -78,8 +74,6 @@ export interface AgentHookBundle { * - `beforeToolCall`: fail-closed RPC. A thrown error travels up into * pi's loop and surfaces as a tool-error result — destructive tools * must never silently bypass main's approval gate. - * - `afterToolCall`: fail-closed RPC. The tool has already run, so - * main must durably acknowledge the artifact projection result. * - `transformContext`: worker-local compaction. The transcript stays * inside the worker; main receives telemetry only. */ @@ -107,28 +101,6 @@ export function buildAgentHooks(deps: AgentHooksDeps): AgentHookBundle { return { block: true, reason: result.reason ?? undefined }; }; - const afterToolCall: AgentHookBundle["afterToolCall"] = async ( - ctx, - ): Promise => { - const payload: AfterToolCallRpcPayload = { - toolName: ctx.toolCall.name, - toolCallId: ctx.toolCall.id, - args: ctx.args, - resultContent: ctx.result.content, - resultDetails: ctx.result.details, - isError: ctx.isError, - }; - const response = await rpc.invoke(runId, "afterToolCall", payload); - if (!response.ok) { - throw new Error(`afterToolCall RPC failed: ${response.error}`); - } - const result = response.result as AfterToolCallRpcResult; - const override: AfterToolCallResult = {}; - if (result.contentOverride) override.content = result.contentOverride; - if (result.isErrorOverride !== null) override.isError = result.isErrorOverride; - return Object.keys(override).length === 0 ? undefined : override; - }; - const transformContext: AgentHookBundle["transformContext"] = async ( messages, ): Promise => { @@ -152,5 +124,5 @@ export function buildAgentHooks(deps: AgentHooksDeps): AgentHookBundle { } }; - return { beforeToolCall, afterToolCall, transformContext }; + return { beforeToolCall, transformContext }; } diff --git a/src/main/agent-runtime/artifact-projection-events.ts b/src/main/agent-runtime/artifact-projection-events.ts index e940d1f4..f478d983 100644 --- a/src/main/agent-runtime/artifact-projection-events.ts +++ b/src/main/agent-runtime/artifact-projection-events.ts @@ -1,7 +1,7 @@ import type { AppDatabase } from "@main/database"; import type { ProjectionOutcome } from "./artifact-types"; -export type ArtifactProjectionPhase = "after_tool_call" | "completion_reconcile"; +export type ArtifactProjectionPhase = "completion_reconcile"; export interface ArtifactProjectionEventInput { companyId: string; @@ -10,8 +10,6 @@ export interface ArtifactProjectionEventInput { phase: ArtifactProjectionPhase; workspaceId: string | null; workspaceMode: "exclusive" | "shared" | "none"; - toolName?: string | null; - toolCallId?: string | null; outcomes: ProjectionOutcome[]; } @@ -22,8 +20,6 @@ export interface ArtifactProjectionFailureInput { phase: ArtifactProjectionPhase; workspaceId: string | null; workspaceMode: "exclusive" | "shared" | "none"; - toolName?: string | null; - toolCallId?: string | null; errorMessage: string; } @@ -73,8 +69,6 @@ export function appendArtifactProjectionEvent( phase: input.phase, workspaceId: input.workspaceId, workspaceMode: input.workspaceMode, - toolName: input.toolName ?? null, - toolCallId: input.toolCallId ?? null, counts, outcomes: input.outcomes, }, @@ -97,8 +91,6 @@ export function appendArtifactProjectionFailureEvent( phase: input.phase, workspaceId: input.workspaceId, workspaceMode: input.workspaceMode, - toolName: input.toolName ?? null, - toolCallId: input.toolCallId ?? null, errorMessage: input.errorMessage, }, }); diff --git a/src/main/agent-runtime/artifact-projector.ts b/src/main/agent-runtime/artifact-projector.ts index 8d4e57a3..fbea01b2 100644 --- a/src/main/agent-runtime/artifact-projector.ts +++ b/src/main/agent-runtime/artifact-projector.ts @@ -2,12 +2,10 @@ import type { AppDatabase } from "@main/database"; import { collectChangedPaths } from "./artifact-change-detector"; import { projectChangedPaths, - projectSinglePathArtifact, retireDeletedArtifacts, } from "./artifact-work-products"; import type { ArtifactProjectorRunContext, - ArtifactProjectorToolEvent, ChangeSet, ProjectionOutcome, } from "./artifact-types"; @@ -15,17 +13,16 @@ import type { export { captureBaselineIfRequired } from "./artifact-baseline"; export type { ArtifactProjectorRunContext, - ArtifactProjectorToolEvent, ProjectionOutcome, WorkspaceBaseline, } from "./artifact-types"; /** - * Filesystem artifact protocol for pi tool invocations. + * End-of-run filesystem artifact protocol. * - * Successful destructive tool calls (`write`, `edit`, `bash`) are - * projected into `work_products`, so the desktop UI can surface what - * the agent produced without relying on the agent to POST documents. + * Completion reconciliation projects changed and deleted workspace + * paths into `work_products`, so the desktop UI can surface what the + * agent produced without relying on the agent to POST documents. * * Projection is idempotent through the natural identity * `(companyId, workspaceId, provider="filesystem", externalId=)`. @@ -33,49 +30,9 @@ export type { * mtime baseline captured by `captureBaselineIfRequired`. */ -type WriteArgs = { path: unknown; content?: unknown }; -type EditArgs = { path: unknown }; - -/** Normalises pi tool-call args to the file path the agent touched. */ -function extractToolPath(toolName: string, args: unknown): string | null { - if (!args || typeof args !== "object") return null; - if (toolName === "write") { - const p = (args as WriteArgs).path; - return typeof p === "string" && p.length > 0 ? p : null; - } - if (toolName === "edit") { - const p = (args as EditArgs).path; - return typeof p === "string" && p.length > 0 ? p : null; - } - return null; -} - /** - * Upsert work_products rows for a successful destructive tool call. - * `write` / `edit` use their explicit path; `bash` scans the workspace - * change set through git status or the captured baseline. - */ -export function projectToolExecutionToWorkProduct( - db: AppDatabase, - ctx: ArtifactProjectorRunContext, - event: ArtifactProjectorToolEvent, -): ProjectionOutcome[] { - if (event.isError) return [{ kind: "skipped", reason: "tool_error" }]; - if (!ctx.workspacePath) return [{ kind: "skipped", reason: "no_workspace" }]; - - if (event.toolName === "write" || event.toolName === "edit") { - return [projectSinglePathArtifact(db, ctx, extractToolPath(event.toolName, event.args))]; - } - if (event.toolName === "bash") { - return projectBashArtifacts(db, ctx); - } - return [{ kind: "skipped", reason: "unsupported_tool" }]; -} - -/** - * End-of-run reconciliation sweep. The per-tool `afterToolCall` hook - * registers artifacts eagerly; the sweep catches files missed by a - * transient hook failure or a bash invocation that touched many paths. + * End-of-run reconciliation sweep. This is the single artifact + * projection boundary for files produced or removed during a run. */ export function reconcileWorkspaceArtifacts( db: AppDatabase, @@ -84,13 +41,6 @@ export function reconcileWorkspaceArtifacts( return projectCollectedChanges(db, ctx, collectChangedPaths(ctx)); } -function projectBashArtifacts( - db: AppDatabase, - ctx: ArtifactProjectorRunContext, -): ProjectionOutcome[] { - return projectCollectedChanges(db, ctx, collectChangedPaths(ctx)); -} - function projectCollectedChanges( db: AppDatabase, ctx: ArtifactProjectorRunContext, diff --git a/src/main/agent-runtime/artifact-types.ts b/src/main/agent-runtime/artifact-types.ts index 51102fc5..cf9c2e1f 100644 --- a/src/main/agent-runtime/artifact-types.ts +++ b/src/main/agent-runtime/artifact-types.ts @@ -1,12 +1,5 @@ import type { Logger } from "./artifact-logging"; -export type ArtifactProjectorToolEvent = { - toolName: string; - args: unknown; - result: unknown; - isError: boolean; -}; - export type ArtifactProjectorRunContext = { runId: string; agentId: string; @@ -18,9 +11,8 @@ export type ArtifactProjectorRunContext = { logger?: Logger; /** * mtime snapshot captured at run start for workspaces that are NOT - * a git repo. Diff-against-baseline is how `projectBashArtifacts` - * and `reconcileWorkspaceArtifacts` find files the agent wrote - * without a `.git` to fall back on. + * a git repo. Diff-against-baseline is how completion reconciliation + * finds files the agent wrote without a `.git` to fall back on. */ workspaceBaseline: WorkspaceBaseline | null; }; diff --git a/src/main/agent-runtime/artifact-work-products.ts b/src/main/agent-runtime/artifact-work-products.ts index 3dd8577f..b3963ce5 100644 --- a/src/main/agent-runtime/artifact-work-products.ts +++ b/src/main/agent-runtime/artifact-work-products.ts @@ -1,27 +1,10 @@ import { statSync } from "node:fs"; -import { isAbsolute, relative, resolve } from "node:path"; +import { relative, resolve } from "node:path"; import type { AppDatabase } from "@main/database"; import { logArtifactFsError } from "./artifact-logging"; import { isRegistrablePath } from "./artifact-path-filter"; import type { ArtifactProjectorRunContext, ProjectionOutcome } from "./artifact-types"; -const MAX_BASH_PROJECTED_FILES = 50; - -export function projectSinglePathArtifact( - db: AppDatabase, - ctx: ArtifactProjectorRunContext, - rawPath: string | null, -): ProjectionOutcome { - if (!ctx.workspacePath) return { kind: "skipped", reason: "no_workspace" }; - if (!rawPath) return { kind: "skipped", reason: "missing_path" }; - - const absPath = isAbsolute(rawPath) ? rawPath : resolve(ctx.workspacePath, rawPath); - if (!isRegistrablePath(absPath, ctx.workspacePath)) { - return { kind: "skipped", reason: "path_ignored" }; - } - return upsertFilesystemArtifact(db, ctx, ctx.workspacePath, absPath); -} - export function projectChangedPaths( db: AppDatabase, ctx: ArtifactProjectorRunContext, @@ -30,8 +13,7 @@ export function projectChangedPaths( const workspacePath = ctx.workspacePath; if (!workspacePath) return [{ kind: "skipped", reason: "no_workspace" }]; const outcomes: ProjectionOutcome[] = []; - const budget = changed.slice(0, MAX_BASH_PROJECTED_FILES); - for (const rel of budget) { + for (const rel of changed) { const absPath = resolve(workspacePath, rel); if (!isRegistrablePath(absPath, workspacePath)) { outcomes.push({ kind: "skipped", reason: "path_ignored" }); @@ -39,9 +21,6 @@ export function projectChangedPaths( } outcomes.push(upsertFilesystemArtifact(db, ctx, workspacePath, absPath)); } - if (changed.length > MAX_BASH_PROJECTED_FILES) { - outcomes.push({ kind: "skipped", reason: "bash_scan_cap_exceeded" }); - } return outcomes; } diff --git a/src/main/agent-runtime/hook-handlers.ts b/src/main/agent-runtime/hook-handlers.ts index f2d09c45..0d78f453 100644 --- a/src/main/agent-runtime/hook-handlers.ts +++ b/src/main/agent-runtime/hook-handlers.ts @@ -1,32 +1,20 @@ /** - * Main-side implementations of pi's tool-execution hooks. Invoked by - * `RuntimeBridge.handleRpc` when the worker proxies `beforeToolCall` - * or `afterToolCall`. + * Main-side implementation of pi's tool-execution approval hook. + * Invoked by `RuntimeBridge.handleRpc` when the worker proxies + * `beforeToolCall`. * - * Each handler is deliberately isolated to one concern: - * - `beforeToolCall`: operational-approval gate (blocks destructive - * tools only while an approval that gates EXECUTION is pending) - * - `afterToolCall`: filesystem artifact projection (bash/write/edit - * outputs become `work_products`) + * The handler is deliberately isolated to the operational-approval + * gate: it blocks destructive tools while an approval that gates + * EXECUTION is pending. * * Policy lives here; `RuntimeBridge` stays a pure IPC router. */ import type { AppDatabase } from "@main/database"; import type { - AfterToolCallRpcPayload, - AfterToolCallRpcResult, BeforeToolCallRpcPayload, BeforeToolCallRpcResult, } from "./hook-rpc"; -import { - projectToolExecutionToWorkProduct, - type ArtifactProjectorRunContext, -} from "./artifact-projector"; -import { - appendArtifactProjectionEvent, - appendArtifactProjectionFailureEvent, -} from "./artifact-projection-events"; /** * Lifecycle phase of an active pi run as seen by main. @@ -70,11 +58,6 @@ export interface HookHandlerDeps { }; } -const NO_OP_AFTER: AfterToolCallRpcResult = { - contentOverride: null, - isErrorOverride: null, -}; - /** * Tools that can mutate the workspace or execute host-level effects. * Every other tool (read/grep/glob) is always permitted — gating them @@ -132,88 +115,3 @@ function checkOperationalApprovalBlock( }; } } - -/** - * Post-execution projection. Today only filesystem artifacts flow - * through this hook; as further projections (browser actions, social - * posts) migrate off the event-observer path they plug in here. - * - * Projection is treated as a primary persistence action: the filesystem - * mutation and its run-event acknowledgement commit together. A failure - * returns an RPC error to the worker, which lets pi surface the hook - * failure as a deterministic tool error. - */ -export function handleAfterToolCall( - deps: HookHandlerDeps, - ctx: ActiveRunContext, - payload: AfterToolCallRpcPayload, -): AfterToolCallRpcResult { - const runCtx: ArtifactProjectorRunContext = { - runId: ctx.runId, - agentId: ctx.agentId, - companyId: ctx.companyId, - taskId: ctx.taskId, - workspaceId: ctx.workspaceId, - workspacePath: ctx.workspacePath, - workspaceMode: ctx.workspaceMode, - logger: deps.logger, - workspaceBaseline: ctx.workspaceBaseline, - }; - try { - const outcomes = deps.db.withTransaction(() => { - const projectionOutcomes = projectToolExecutionToWorkProduct( - deps.db, - runCtx, - { - toolName: payload.toolName, - args: payload.args, - result: payload.resultContent, - isError: payload.isError, - }, - ); - appendArtifactProjectionEvent(deps.db, { - companyId: ctx.companyId, - runId: ctx.runId, - agentId: ctx.agentId, - phase: "after_tool_call", - workspaceId: ctx.workspaceId, - workspaceMode: ctx.workspaceMode, - toolName: payload.toolName, - toolCallId: payload.toolCallId, - outcomes: projectionOutcomes, - }); - return projectionOutcomes; - }); - const registered = outcomes.filter((o) => o.kind === "inserted" || o.kind === "updated").length; - const retired = outcomes.filter((o) => o.kind === "retired").length; - if (registered > 0 || retired > 0) { - deps.logger.info( - `[hook-handlers] projection for run ${ctx.runId} via ${payload.toolName}: ${registered} registered, ${retired} retired`, - ); - } - } catch (error) { - const errorMessage = error instanceof Error ? error.message : String(error); - try { - appendArtifactProjectionFailureEvent(deps.db, { - companyId: ctx.companyId, - runId: ctx.runId, - agentId: ctx.agentId, - phase: "after_tool_call", - workspaceId: ctx.workspaceId, - workspaceMode: ctx.workspaceMode, - toolName: payload.toolName, - toolCallId: payload.toolCallId, - errorMessage, - }); - } catch (ackError) { - deps.logger.error( - `[hook-handlers] artifact projection failure ack failed for run ${ctx.runId} tool ${payload.toolName}: ${ackError instanceof Error ? ackError.message : String(ackError)}`, - ); - } - deps.logger.error( - `[hook-handlers] artifact projection threw for run ${ctx.runId} tool ${payload.toolName}: ${errorMessage}`, - ); - throw error; - } - return NO_OP_AFTER; -} diff --git a/src/main/agent-runtime/hook-rpc.ts b/src/main/agent-runtime/hook-rpc.ts index d5e09ccd..8aa0bd63 100644 --- a/src/main/agent-runtime/hook-rpc.ts +++ b/src/main/agent-runtime/hook-rpc.ts @@ -1,17 +1,16 @@ /** * Hook RPC contracts — the parent↔worker half of pi-agent-core's - * `beforeToolCall` / `afterToolCall` hooks. + * `beforeToolCall` hook. * - * The Agent instance lives in the utility process. Policy (approval, - * filesystem projection) lives in main. The hooks are therefore proxied: - * the worker invokes an RPC that main answers from the same - * `RuntimeBridge` that already owns agent-event and `persistMessage` - * handling. Context compaction runs in the worker process and reports - * telemetry through a worker message. + * The Agent instance lives in the utility process. Approval policy + * lives in main. The hook is therefore proxied: the worker invokes an + * RPC that main answers from the same `RuntimeBridge` that already owns + * agent-event and `persistMessage` handling. Context compaction runs in + * the worker process and reports telemetry through a worker message. * * Payloads intentionally exclude pi's `AgentContext` (systemPrompt, - * messages, tools) because main policy never needs it. Stripping it - * keeps each hook IPC well under the structured-clone budget even for + * messages, tools) because main policy only needs the tool call. This + * keeps the hook IPC well under the structured-clone budget even for * large transcripts. */ // ─── beforeToolCall ─────────────────────────────────────────────────────── @@ -26,26 +25,3 @@ export interface BeforeToolCallRpcResult { block: boolean; reason: string | null; } - -// ─── afterToolCall ──────────────────────────────────────────────────────── - -export interface AfterToolCallRpcPayload { - toolName: string; - toolCallId: string; - args: unknown; - /** Raw tool result content list as emitted by pi's tool executor. */ - resultContent: unknown; - /** Structured `details` payload if the tool exposed one. */ - resultDetails: unknown; - isError: boolean; -} - -/** - * Optional override for the executed tool result, mirroring pi's - * `AfterToolCallResult` shape. Main returns every field as `null` when - * the hook performs observation only (current projection use-case). - */ -export interface AfterToolCallRpcResult { - contentOverride: { type: "text"; text: string }[] | null; - isErrorOverride: boolean | null; -} diff --git a/src/main/agent-runtime/runtime-bridge.ts b/src/main/agent-runtime/runtime-bridge.ts index 87a4487c..812a6023 100644 --- a/src/main/agent-runtime/runtime-bridge.ts +++ b/src/main/agent-runtime/runtime-bridge.ts @@ -15,14 +15,10 @@ import { } from "./runtime-artifact-reconciler"; import { handleBeforeToolCall, - handleAfterToolCall, type ActiveRunContext, type ActiveRunLifecycleState, } from "./hook-handlers"; -import type { - AfterToolCallRpcPayload, - BeforeToolCallRpcPayload, -} from "./hook-rpc"; +import type { BeforeToolCallRpcPayload } from "./hook-rpc"; import { routeInternalToolCall } from "./internal-tools/router"; import type { InvokeInternalToolRpcPayload } from "@main/worker/runtime/internal-tools/internal-tool-rpc"; import { InboundCorrelationRegistryStore } from "./inbound-correlation-registry"; @@ -81,9 +77,8 @@ type RpcMethod = WorkerRpcMethod; * - Forwards `agent-event` from the worker to the renderer (push channel). * - Persists each `message_end` message via `SqliteSessionStore`. * - Calls `CostWriter` for assistant messages with usage. - * - Proxies pi's tool-execution hooks (`beforeToolCall` / `afterToolCall`) - * into policy handlers: approval-gate checks and filesystem artifact - * projection. + * - Proxies pi's `beforeToolCall` hook into the approval-gate policy handler. + * - Reconciles filesystem artifacts during run resource release. * * Policy itself lives in `hook-handlers.ts`; this class is a pure IPC * router plus an `ActiveRunContext` cache. @@ -132,11 +127,10 @@ export class RuntimeBridge { * `run-finished` worker message so the maps stay bounded by concurrent- * run count rather than process lifetime. * - * Before eviction, sweeps the workspace to register any filesystem - * artifacts the per-tool `afterToolCall` hook missed. This is the - * backstop that keeps the DocumentCenter consistent with the disk even - * when individual projections skipped (bash against a non-git repo, - * path-ignore false positives, transient `git status` failures). + * Before eviction, sweeps the workspace to register filesystem + * artifacts produced during the run. This completion boundary keeps + * the DocumentCenter consistent with disk through one durable + * reconciliation acknowledgement. */ releaseRunResources(runId: string): RunResourceReleaseResult { const ctx = this.activeRuns.get(runId); @@ -311,8 +305,6 @@ export class RuntimeBridge { return await this.handlePrepareRunRpc(request.runId, request.payload); case "beforeToolCall": return this.handleBeforeToolCallRpc(request.runId, request.payload as BeforeToolCallRpcPayload); - case "afterToolCall": - return this.handleAfterToolCallRpc(request.runId, request.payload as AfterToolCallRpcPayload); case "invokeInternalTool": return this.handleInvokeInternalToolRpc(request.runId, request.payload as InvokeInternalToolRpcPayload); default: { @@ -465,16 +457,4 @@ export class RuntimeBridge { return { ok: true, result }; } - private handleAfterToolCallRpc( - runId: string, - payload: AfterToolCallRpcPayload, - ): { ok: true; result: unknown } | { ok: false; error: string } { - const ctx = this.activeRuns.get(runId); - if (!ctx) { - return { ok: false, error: `afterToolCall: no active run ${runId}` }; - } - const result = handleAfterToolCall(this.deps, ctx, payload); - return { ok: true, result }; - } - } diff --git a/src/main/worker/entry/types.ts b/src/main/worker/entry/types.ts index b086f845..cbc391ba 100644 --- a/src/main/worker/entry/types.ts +++ b/src/main/worker/entry/types.ts @@ -24,7 +24,6 @@ export const workerRpcMethodSchema = z.enum([ "persistMessage", "prepareRun", "beforeToolCall", - "afterToolCall", "invokeInternalTool", ]); diff --git a/src/main/worker/runtime/internal-tools/internal-tool-rpc.ts b/src/main/worker/runtime/internal-tools/internal-tool-rpc.ts index 53a099d1..27f87b57 100644 --- a/src/main/worker/runtime/internal-tools/internal-tool-rpc.ts +++ b/src/main/worker/runtime/internal-tools/internal-tool-rpc.ts @@ -8,13 +8,11 @@ * they need the SQLite connection, approval subsystem, and telemetry — * none of which the worker should replicate. * - * This is structurally the same pattern as `beforeToolCall`/ - * `afterToolCall` in `hook-rpc.ts`; what's different - * is that those are side-effect hooks around pi's own tools, whereas - * this is a primary-effect tool whose business logic lives entirely in - * main. Keeping the payload shape minimal (name + JSON args) mirrors - * pi's `ToolCall` structure so adding a new internal tool requires no - * new RPC method — only a new handler entry. + * This mirrors the `beforeToolCall` RPC envelope in `hook-rpc.ts`: + * a minimal tool-call payload crosses the worker boundary and main owns + * the business effect. Keeping the payload shape minimal (name + JSON + * args) mirrors pi's `ToolCall` structure so adding a new internal tool + * requires no new RPC method — only a new handler entry. */ export interface InvokeInternalToolRpcPayload { diff --git a/src/main/worker/runtime/pi-agent-factory.ts b/src/main/worker/runtime/pi-agent-factory.ts index 501cf65c..0c1192df 100644 --- a/src/main/worker/runtime/pi-agent-factory.ts +++ b/src/main/worker/runtime/pi-agent-factory.ts @@ -1,15 +1,13 @@ /** * Factory that assembles a `pi-agent-core` Agent from a prepared worker * job. Encapsulates the full Pi contract (initial state, sessionId, - * thinkingLevel, beforeToolCall / afterToolCall / transformContext + * thinkingLevel, beforeToolCall / transformContext * hooks, streamFn) so `worker-runner` stays focused on run lifecycle. * - * The three tool-execution hooks proxy into main via the worker RPC - * client; main answers from `RuntimeBridge`. Pi's loop is tolerant of - * thrown hooks — a thrown `beforeToolCall` produces an error tool - * result, a thrown `afterToolCall` likewise — so RPC failures degrade - * to a deterministic tool-error instead of crashing the run. The - * `transformContext` runs the deterministic compactor in the worker + * `beforeToolCall` proxies into main via the worker RPC client; main + * answers from `RuntimeBridge`. Pi's loop is tolerant of thrown hooks, + * so approval RPC failures degrade to a deterministic tool-error. The + * `transformContext` hook runs the deterministic compactor in the worker * process and lets compaction failures fail the run explicitly. */ @@ -84,7 +82,6 @@ export function createPiAgent(input: PiAgentFactoryInput): Agent { headers: { ...(options?.headers ?? {}), ...(input.extraHeaders ?? {}) }, }), beforeToolCall: hooks.beforeToolCall, - afterToolCall: hooks.afterToolCall, transformContext: hooks.transformContext, toolExecution: "sequential", }); diff --git a/src/main/worker/runtime/pi-hook-contract.ts b/src/main/worker/runtime/pi-hook-contract.ts index 681d18db..499cfe3a 100644 --- a/src/main/worker/runtime/pi-hook-contract.ts +++ b/src/main/worker/runtime/pi-hook-contract.ts @@ -3,7 +3,7 @@ * `pi-agent-core` callback signatures. * * Root `package.json` pins `@mariozechner/pi-agent-core` to 0.67.68 - * while our worker wires local hooks — `beforeToolCall`, `afterToolCall`, + * while our worker wires local hooks — `beforeToolCall` and * `transformContext` — into the pi Agent constructor. Those hooks carry * deep structural coupling: pi decides the context shape passed in and * the result shape we return. A silent signature drift on upgrade would @@ -21,8 +21,6 @@ */ import type { - AfterToolCallContext, - AfterToolCallResult, AgentMessage, AgentOptions, BeforeToolCallContext, @@ -37,7 +35,6 @@ import type { * cascades into every `satisfies` check below. */ export type PiBeforeToolCall = NonNullable; -export type PiAfterToolCall = NonNullable; export type PiTransformContext = NonNullable; /** @@ -47,8 +44,6 @@ export type PiTransformContext = NonNullable; */ export type PiBeforeToolCallContext = BeforeToolCallContext; export type PiBeforeToolCallResult = BeforeToolCallResult; -export type PiAfterToolCallContext = AfterToolCallContext; -export type PiAfterToolCallResult = AfterToolCallResult; export type PiAgentMessage = AgentMessage; /** @@ -74,11 +69,6 @@ type LocalBeforeToolCallShape = ( signal?: AbortSignal, ) => Promise; -type LocalAfterToolCallShape = ( - ctx: AfterToolCallContext, - signal?: AbortSignal, -) => Promise; - type LocalTransformContextShape = ( messages: AgentMessage[], signal?: AbortSignal, @@ -92,7 +82,6 @@ type LocalTransformContextShape = ( * field on either side breaks the build. */ export type BeforeToolCallContract = ExpectHookType; -export type AfterToolCallContract = ExpectHookType; export type TransformContextContract = ExpectHookType; /** @@ -108,11 +97,6 @@ export type BeforeToolCallHandlerResult = { readonly reason: string | null; }; -export type AfterToolCallHandlerResult = { - readonly contentOverride: AfterToolCallResult["content"] | null; - readonly isErrorOverride: AfterToolCallResult["isError"] | null; -}; - export type TransformContextHandlerResult = { readonly changed: boolean; readonly messages: AgentMessage[] | null; diff --git a/tests/integration/artifact-projection-ack.spec.ts b/tests/integration/artifact-projection-ack.spec.ts index b48c369e..c99c8388 100644 --- a/tests/integration/artifact-projection-ack.spec.ts +++ b/tests/integration/artifact-projection-ack.spec.ts @@ -5,6 +5,7 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { RuntimeBridge } from "../../src/main/agent-runtime"; import { AppDatabase } from "../../src/main/database"; import type { CredentialResolver } from "../../src/main/credentials"; +import { workerRpcMethodSchema } from "../../src/main/worker/entry/types"; const tempDirs: string[] = []; @@ -108,42 +109,8 @@ async function createHarness() { } describe("artifact projection durable ack", () => { - it("persists afterToolCall projection outcomes to the run event tail", async () => { - const harness = await createHarness(); - try { - await mkdir(join(harness.workspacePath, "reports"), { recursive: true }); - await writeFile(join(harness.workspacePath, "reports", "hook.md"), "hook artifact", "utf8"); - - const response = await harness.bridge.handleRpc({ - rpcId: "after-tool", - method: "afterToolCall", - runId: harness.runId, - payload: { - toolName: "write", - toolCallId: "tool-call-hook", - args: { path: "reports/hook.md" }, - resultContent: [], - resultDetails: null, - isError: false, - }, - }); - - expect(response.ok).toBe(true); - const event = harness.db.core.runs.listRunEvents(harness.runId, harness.companyId)[0]; - expect(event).toMatchObject({ - eventType: "artifact_projection", - level: "info", - }); - expect(event?.payload).toMatchObject({ - phase: "after_tool_call", - toolName: "write", - toolCallId: "tool-call-hook", - workspaceMode: "none", - counts: { inserted: 1, updated: 0, retired: 0, skipped: 0 }, - }); - } finally { - harness.db.close(); - } + it("keeps artifact projection out of the per-tool RPC method surface", () => { + expect(workerRpcMethodSchema.safeParse("afterToolCall").success).toBe(false); }); it("runs completion reconciliation before resource release for non-exclusive observation workspaces", async () => { @@ -170,6 +137,8 @@ describe("artifact projection durable ack", () => { workspaceMode: "none", counts: { inserted: 1, updated: 0, retired: 0, skipped: 0 }, }); + expect(events[0]?.payload).not.toHaveProperty("toolName"); + expect(events[0]?.payload).not.toHaveProperty("toolCallId"); } finally { harness.db.close(); } diff --git a/tests/integration/pi-runtime-contract.spec.ts b/tests/integration/pi-runtime-contract.spec.ts index 97cc34c8..4583a916 100644 --- a/tests/integration/pi-runtime-contract.spec.ts +++ b/tests/integration/pi-runtime-contract.spec.ts @@ -62,8 +62,8 @@ describe("pi runtime integration contract", () => { expect(agent.state.tools).toEqual([]); expect(agent.state.messages).toEqual([]); expect(typeof agent.beforeToolCall).toBe("function"); - expect(typeof agent.afterToolCall).toBe("function"); expect(typeof agent.transformContext).toBe("function"); + expect(agent.afterToolCall).toBeUndefined(); }); it("keeps the coding-tool surface expected by worker runs", () => { @@ -133,12 +133,10 @@ describe("pi runtime integration contract", () => { it("keeps the aggregated hook bundle assignable to pi Agent options at runtime", () => { const hooks = buildAgentHooks({ runId: "run-contract", rpc: createRpc() }); expect(Object.keys(hooks).sort()).toEqual([ - "afterToolCall", "beforeToolCall", "transformContext", ]); expect(typeof hooks.beforeToolCall).toBe("function"); - expect(typeof hooks.afterToolCall).toBe("function"); expect(typeof hooks.transformContext).toBe("function"); }); }); diff --git a/tests/integration/work-products-lifecycle.spec.ts b/tests/integration/work-products-lifecycle.spec.ts index d2c44b92..7c590847 100644 --- a/tests/integration/work-products-lifecycle.spec.ts +++ b/tests/integration/work-products-lifecycle.spec.ts @@ -5,7 +5,6 @@ import { tmpdir } from "node:os"; import { afterEach, describe, expect, it } from "vitest"; import { AppDatabase } from "../../src/main/database"; import { - projectToolExecutionToWorkProduct, reconcileWorkspaceArtifacts, type ArtifactProjectorRunContext, } from "../../src/main/agent-runtime/artifact-projector"; @@ -237,7 +236,7 @@ describe("work_products archived visibility", () => { }); describe("work_products bulk retirement", () => { - it("registers bash artifacts from non-exclusive observation workspaces", async () => { + it("registers changed artifacts from non-exclusive observation workspaces", async () => { const harness = await createHarness(); try { const ctx: ArtifactProjectorRunContext = { @@ -257,12 +256,7 @@ describe("work_products bulk retirement", () => { await mkdir(join(harness.workspacePath, "reports"), { recursive: true }); await writeFile(join(harness.workspacePath, "reports", "non-exclusive.md"), "observed", "utf8"); - const outcomes = projectToolExecutionToWorkProduct(harness.db, ctx, { - toolName: "bash", - args: { command: "printf observed > reports/non-exclusive.md" }, - result: null, - isError: false, - }); + const outcomes = reconcileWorkspaceArtifacts(harness.db, ctx); expect(outcomes).toEqual([ expect.objectContaining({ @@ -280,6 +274,44 @@ describe("work_products bulk retirement", () => { } }); + it("registers every changed artifact past the previous 50-path budget", async () => { + const harness = await createHarness(); + try { + const paths: string[] = []; + await mkdir(join(harness.workspacePath, "bulk"), { recursive: true }); + for (let i = 0; i < 120; i += 1) { + const rel = `bulk/new-${i}.md`; + paths.push(rel); + await writeFile(join(harness.workspacePath, rel), `artifact ${i}`, "utf8"); + } + + const outcomes = reconcileWorkspaceArtifacts(harness.db, { + runId: harness.runId, + agentId: harness.agentId, + companyId: harness.companyId, + taskId: harness.taskId, + workspaceId: null, + workspacePath: harness.workspacePath, + workspaceMode: "none", + workspaceBaseline: { + mtimeByRelPath: new Map(), + capturedAt: Date.now(), + truncated: false, + }, + }); + + expect(outcomes.filter((o) => o.kind === "inserted")).toHaveLength(120); + const visible = new Set( + harness.db.projection.snapshot.listSnapshot().workProducts.map((entry) => entry.externalId), + ); + for (const path of paths) { + expect(visible.has(path)).toBe(true); + } + } finally { + harness.db.close(); + } + }); + it("retires deleted artifacts from non-exclusive observation workspaces", async () => { const harness = await createHarness(); try { diff --git a/tests/unit/agent-hooks.test.ts b/tests/unit/agent-hooks.test.ts index 1eb2fb69..c152f914 100644 --- a/tests/unit/agent-hooks.test.ts +++ b/tests/unit/agent-hooks.test.ts @@ -9,7 +9,7 @@ function createRpc(response: { ok: true; result: unknown } | { ok: false; error: } describe("agent hook RPC policy", () => { - it("fails afterToolCall when main cannot acknowledge artifact projection", async () => { + it("fails beforeToolCall closed when main cannot answer approval policy", async () => { const rpc = createRpc({ ok: false, error: "RuntimeBridge unavailable" }); const hooks = buildAgentHooks({ runId: "11111111-1111-4111-8111-111111111111", @@ -17,12 +17,21 @@ describe("agent hook RPC policy", () => { }); await expect( - hooks.afterToolCall({ - toolCall: { name: "write", id: "tool-call-1" }, + hooks.beforeToolCall({ + toolCall: { name: "bash", id: "tool-call-1" }, args: { path: "report.md" }, - result: { content: [], details: null }, - isError: false, - } as unknown as Parameters[0]), - ).rejects.toThrow("afterToolCall RPC failed: RuntimeBridge unavailable"); + } as unknown as Parameters[0]), + ).rejects.toThrow("beforeToolCall RPC failed: RuntimeBridge unavailable"); + }); + + it("installs no per-tool artifact projection hook", () => { + const rpc = createRpc({ ok: true, result: { block: false, reason: null } }); + const hooks = buildAgentHooks({ + runId: "11111111-1111-4111-8111-111111111111", + rpc, + }); + + expect(Object.keys(hooks).sort()).toEqual(["beforeToolCall", "transformContext"]); + expect("afterToolCall" in hooks).toBe(false); }); }); diff --git a/tests/unit/artifact-projector-errors.test.ts b/tests/unit/artifact-projector-errors.test.ts index 80a34482..06510c1e 100644 --- a/tests/unit/artifact-projector-errors.test.ts +++ b/tests/unit/artifact-projector-errors.test.ts @@ -5,7 +5,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import type { AppDatabase } from "../../src/main/database"; import { captureBaselineIfRequired, - projectToolExecutionToWorkProduct, reconcileWorkspaceArtifacts, type ArtifactProjectorRunContext, } from "../../src/main/agent-runtime/artifact-projector"; @@ -46,47 +45,7 @@ describe("artifact projector error taxonomy", () => { expect(logger.error).not.toHaveBeenCalled(); }); - it("classifies missing filesystem artifact projection failures", async () => { - const logger = createLogger(); - const workspacePath = await createTempDir("agentcompany-artifact-errors-"); - const absPath = join(workspacePath, "missing.md"); - const ctx: ArtifactProjectorRunContext = { - runId: "run-artifact-error", - agentId: "agent-artifact-error", - companyId: "company-artifact-error", - taskId: "task-artifact-error", - workspaceId: "workspace-artifact-error", - workspacePath, - workspaceMode: "exclusive", - logger, - workspaceBaseline: null, - }; - - const outcomes = projectToolExecutionToWorkProduct( - {} as AppDatabase, - ctx, - { - toolName: "write", - args: { path: "missing.md" }, - result: null, - isError: false, - }, - ); - - expect(outcomes).toEqual([{ kind: "skipped", reason: "path_missing" }]); - expect(logger.warn).toHaveBeenCalledWith( - "[artifact.fs.path_stat_failed] Failed to stat filesystem artifact before projection.", - { - workspacePath, - absPath, - runId: "run-artifact-error", - }, - expect.stringContaining("cause="), - ); - expect(logger.error).not.toHaveBeenCalled(); - }); - - it("skips broad bash projection when the run has no change source", async () => { + it("skips completion reconciliation when the run has no change source", async () => { const workspacePath = await createTempDir("agentcompany-shared-artifacts-"); const ctx: ArtifactProjectorRunContext = { runId: "run-shared", @@ -100,12 +59,6 @@ describe("artifact projector error taxonomy", () => { workspaceBaseline: null, }; - expect(projectToolExecutionToWorkProduct({} as AppDatabase, ctx, { - toolName: "bash", - args: { command: "touch report.md" }, - result: null, - isError: false, - })).toEqual([{ kind: "skipped", reason: "no_baseline" }]); expect(reconcileWorkspaceArtifacts({} as AppDatabase, ctx)).toEqual([ { kind: "skipped", reason: "no_baseline" }, ]);