diff --git a/agents/claude.ts b/agents/claude.ts index f128094..9189cfb 100644 --- a/agents/claude.ts +++ b/agents/claude.ts @@ -18,7 +18,13 @@ import { performance } from "node:perf_hooks"; import { pullfrogMcpName } from "../external.ts"; import { BEDROCK_MODEL_ID_ENV, isBedrockAnthropicId } from "../models.ts"; -import { getIdleMs, markActivity } from "../utils/activity.ts"; +import { + getIdleMs, + isActivitySuspended, + markActivity, + resumeActivity, + suspendActivity, +} from "../utils/activity.ts"; import { formatJsonValue, log } from "../utils/cli.ts"; import { installFromNpmTarball } from "../utils/install.ts"; import { findProviderErrorMatch } from "../utils/providerErrors.ts"; @@ -367,6 +373,13 @@ export async function runClaude(params: RunParams): Promise { } } else if (block.type === "tool_use") { const toolName = block.name || "unknown"; + // suspend the activity watchdog across the tool call. claude's + // stdout pipe goes silent while it awaits the synchronous MCP + // tools/call HTTP response; without this, long fetches/deepens + // (issue #760) trip the spawn-level idle timer at 300s. paired + // with resumeActivity() in tool_result below; bounded by the + // MAX_TOOL_CALL_SUSPENSION_MS auto-resume in activity.ts. + suspendActivity(); if (params.onToolUse) { params.onToolUse({ toolName, @@ -443,6 +456,7 @@ export async function runClaude(params: RunParams): Promise { for (const block of content) { if (typeof block === "string") continue; if (block.type === "tool_result") { + resumeActivity(); timerFor(label).markToolResult(); const outputContent = @@ -576,6 +590,7 @@ export async function runClaude(params: RunParams): Promise { env: params.env, activityTimeout: 300_000, onActivityTimeout: params.onActivityTimeout, + isPausedExternally: isActivitySuspended, stdio: ["ignore", "pipe", "pipe"], // run claude in its own process group so SIGKILL on activity timeout / // outer cancellation reaches any subprocesses it spawns (rg, file diff --git a/agents/opencode.ts b/agents/opencode.ts index 7064721..22a36dd 100644 --- a/agents/opencode.ts +++ b/agents/opencode.ts @@ -19,7 +19,13 @@ import * as core from "@actions/core"; import { pullfrogMcpName } from "../external.ts"; import { BEDROCK_MODEL_ID_ENV, modelAliases } from "../models.ts"; import type { ToolState } from "../toolState.ts"; -import { getIdleMs, markActivity } from "../utils/activity.ts"; +import { + getIdleMs, + isActivitySuspended, + markActivity, + resumeActivity, + suspendActivity, +} from "../utils/activity.ts"; import { type AgentDiagnostic, formatAgentHangBody } from "../utils/agentHangReport.ts"; import { formatJsonValue, log } from "../utils/cli.ts"; import { installCodexAuth } from "../utils/codexHome.ts"; @@ -651,6 +657,21 @@ async function runOpenCode(params: RunParams): Promise { return; } + // suspend the activity watchdog across the tool call (issue #760). + // for `task` tool dispatches the injected plugin already reverbs + // child.stdout chunks, so this is mostly defense-in-depth there; + // for non-task MCP tools (checkout_pr, etc.) the suspend is the + // only thing keeping a multi-minute fetch from tripping the 300s + // spawn-level idle timer. gate by part status: bus-envelope + // re-dispatches at line 915 fire only on terminal statuses + // (`completed`/`error`) and never produce a paired `tool_result`, + // so suspending on those would leak the watchdog open until the + // 15min auto-resume — exactly the issue #12 zombie-run window. + const status = event.part?.state?.status; + if (status !== "completed" && status !== "error") { + suspendActivity(); + } + // when the orchestrator dispatches a subagent via the `task` tool, push // a label for the upcoming child session so its events are attributable. // record BEFORE label lookup: this event's session is the parent (whose @@ -732,6 +753,7 @@ async function runOpenCode(params: RunParams): Promise { } }, tool_result: (event: OpenCodeToolResultEvent) => { + resumeActivity(); const toolId = event.part?.callID || event.tool_id; const state = event.part?.state; const status = state?.status ?? event.status ?? "unknown"; @@ -979,13 +1001,15 @@ async function runOpenCode(params: RunParams): Promise { // wrapper would grow unbounded for multi-lens Reviews and previously // crashed the wrapper with RangeError at ~1 GiB. see issue #680. retain: "none", - // NB: we used to pass `isPausedExternally: isSubagentInFlight` to suspend - // the activity timer during subagent dispatches. unnecessary now that - // our injected plugin (action/agents/opencodePlugin.ts) re-emits - // subagent `message.part.updated` events on opencode's stdout — those - // arrive at child.stdout here, fire updateActivity(), and reset - // lastActivityTime naturally. verified empirically in PR #634 - // (~3.3 plugin events/sec during a typical subagent run). + // suspend the spawn-level idle watchdog across MCP tool calls (issue + // #760). bracketed by suspendActivity()/resumeActivity() in the + // tool_use/tool_result handlers above, bounded by + // MAX_TOOL_CALL_SUSPENSION_MS in activity.ts. the injected plugin + // (action/agents/opencodePlugin.ts) re-emits subagent + // `message.part.updated` events on opencode's stdout, so subagent + // dispatches keep marking child.stdout activity as well — defense + // in depth (verified empirically in PR #634, ~3.3 plugin events/sec). + isPausedExternally: isActivitySuspended, onStdout: async (chunk) => { const text = chunk.toString(); output.append(text); diff --git a/mcp/git.ts b/mcp/git.ts index 5e96ba5..3aa697d 100644 --- a/mcp/git.ts +++ b/mcp/git.ts @@ -490,6 +490,30 @@ export function GitTool(ctx: ToolContext) { } } + // `git merge-base --is-ancestor` uses exit codes as data: 0 = ancestor, + // 1 = not-an-ancestor, >1 = real error. Surface the binary answer + // instead of throwing on exit 1. see #766. + if (command === "merge-base" && args.includes("--is-ancestor")) { + let isAncestor = true; + $("git", [command, ...args], { + log: false, + onError: (r) => { + if (r.status === 1) { + isAncestor = false; + return; + } + const detail = [r.stderr, r.stdout] + .map((s) => s.trim()) + .filter(Boolean) + .join("\n"); + throw new Error( + `git merge-base --is-ancestor failed (exit ${r.status}): ${detail || "Unknown error"}` + ); + }, + }); + return { success: true, isAncestor }; + } + const output = $("git", [command, ...args], { log: false }); const lineCount = output.split("\n").length; if (lineCount > COLLAPSE_THRESHOLD) { diff --git a/utils/activity.ts b/utils/activity.ts index d8c973d..37fa2cb 100644 --- a/utils/activity.ts +++ b/utils/activity.ts @@ -1,4 +1,5 @@ import { performance } from "node:perf_hooks"; +import { log } from "./log.ts"; function isMonitorDebugEnabled(): boolean { return ( @@ -79,6 +80,19 @@ type WriteFunction = { // module-level activity tracking - allows agents to mark activity on any event let _lastActivity = performance.now(); +/** + * upper bound on how long a single tool call can suspend the activity + * watchdog. matched against the typical worst-case `checkout_pr` + * fetch+deepen on a large monorepo (issue #760: 4-5min) plus generous + * headroom for slower MCP tools, while still bounding the worst case if + * a tool genuinely hangs and `tool_result` never arrives — auto-resume + * fires here and the normal idle clock takes over from a fresh baseline. + */ +export const MAX_TOOL_CALL_SUSPENSION_MS = 15 * 60 * 1000; + +let _suspendedAt: number | null = null; +let _suspensionTimer: NodeJS.Timeout | null = null; + /** * mark activity to reset the no-output timeout. * call this whenever the agent emits any event, even if it isn't logged to stdout. @@ -88,12 +102,55 @@ export function markActivity(): void { } /** - * get the time since last activity in milliseconds + * get the time since last activity in milliseconds. + * returns 0 while the watchdog is suspended (issue #760). */ export function getIdleMs(): number { + if (_suspendedAt !== null) return 0; return Math.round(performance.now() - _lastActivity); } +/** + * suspend the activity watchdog while a long-running, in-flight unit of + * work is happening (e.g. an MCP `tools/call` that synchronously awaits + * a multi-minute git fetch). bracket calls with `resumeActivity()` from + * the agent harness's `tool_use` / `tool_result` event handlers. + * + * - idempotent: nested suspends are no-ops; the first resume wins. + * - bounded: auto-resumes after `maxMs` so a buggy tool that never + * produces a `tool_result` can't pin the watchdog open forever. + * - safe: only the *agent harness* (claude.ts / opencode.ts) on explicit, + * paired CLI events should call this. NEVER blanket-suspend on internal + * noise — that would resurrect issue #12 zombie runs. + */ +export function suspendActivity(maxMs: number = MAX_TOOL_CALL_SUSPENSION_MS): void { + if (_suspendedAt !== null) return; + _suspendedAt = performance.now(); + _suspensionTimer = setTimeout(() => { + log.warning(`activity watchdog suspended >${Math.round(maxMs / 1000)}s — auto-resuming`); + resumeActivity(); + }, maxMs); + _suspensionTimer.unref?.(); +} + +/** + * resume the activity watchdog. resets the idle baseline so a stale + * idle window before the suspend can't immediately re-fire. + */ +export function resumeActivity(): void { + if (_suspendedAt === null) return; + _suspendedAt = null; + if (_suspensionTimer) { + clearTimeout(_suspensionTimer); + _suspensionTimer = null; + } + _lastActivity = performance.now(); +} + +export function isActivitySuspended(): boolean { + return _suspendedAt !== null; +} + function wrapWrite(original: WriteFunction, onActivity: () => void): WriteFunction { const wrapped: WriteFunction = ( chunk: string | Uint8Array, diff --git a/utils/github.ts b/utils/github.ts index 8d55cf3..b630e68 100644 --- a/utils/github.ts +++ b/utils/github.ts @@ -379,10 +379,30 @@ export async function acquireNewToken(opts?: AcquireTokenOptions): Promise s.trim()) + .filter(Boolean) + .join("\n"); throw new Error( - `Command failed with exit code ${errorResult.status}: ${stderr || "Unknown error"}` + `Command failed with exit code ${errorResult.status}: ${detail || "Unknown error"}` ); } diff --git a/utils/subprocess.ts b/utils/subprocess.ts index d1e6294..2874b7d 100644 --- a/utils/subprocess.ts +++ b/utils/subprocess.ts @@ -125,6 +125,14 @@ export interface SpawnOptions { // so that lingering SSE reconnects don't keep the outer activity timer // alive after the subprocess is already dead. onActivityTimeout?: (() => void) | undefined; + // optional pause predicate consulted on every activity check. when true, + // the spawn watchdog (a) skips its kill decision, (b) advances + // `lastActivityTime` so a stale baseline can't fire on resume. used by + // agent harnesses (claude.ts / opencode.ts) to suspend the watchdog + // across long synchronous MCP `tools/call` round-trips that the child's + // stdout pipe can't see (issue #760). bounded externally by + // `MAX_TOOL_CALL_SUSPENSION_MS` plus the outer agent timeout. + isPausedExternally?: () => boolean; cwd?: string; stdio?: ("pipe" | "ignore" | "inherit")[]; onStdout?: (chunk: string) => void; @@ -276,6 +284,13 @@ export async function spawn(options: SpawnOptions): Promise { `spawn activity timer: pid=${child.pid} cmd=${options.cmd} timeout=${activityTimeoutMs}ms` ); activityCheckIntervalId = setInterval(() => { + if (options.isPausedExternally?.()) { + // reset the baseline so a clean resume can't immediately fire on + // the pre-pause idle window. + lastActivityTime = performance.now(); + log.debug(`spawn activity check: pid=${child.pid} paused externally`); + return; + } const idleMs = performance.now() - lastActivityTime; log.debug( `spawn activity check: pid=${child.pid} idle=${Math.round(idleMs)}ms / ${activityTimeoutMs}ms`