3d393c36a3
* opencode: surface subagent events via injected plugin opencode's cli/cmd/run.ts event loop filters all message.part.updated events to the orchestrator's session id (`part.sessionID !== sessionID` continue), so subagent-internal tool_use / text / step events were silently discarded by the CLI in --format json mode. opencode plugins, by contrast, receive every bus event via bus.subscribeAll() regardless of session. ship a per-run plugin (action/agents/opencodePlugin.ts) that re-emits non-orchestrator message.part.updated events as `pullfrog_bus_event` envelopes on opencode's stdout. the plugin is staged into <XDG_CONFIG_HOME>/opencode/plugin/pullfrog-events.ts which is already redirected to ctx.tmpdir — never the user's repo working tree. the plugin also forwards the orchestrator's task tool dispatch at state.status="running" — that's the first moment state.input is populated with description / subagent_type / prompt and it lands BEFORE the subagent's first message.part.updated. forwarding this lets SessionLabeler register the lens label early, so subagent events bind to the correct lens name (e.g. lens:correctness) instead of the subagent#N fallback. the existing tool_use handler dedupes on callID so the late status=completed event from the CLI doesn't double-record. the parent's pullfrog_bus_event handler synthesizes the equivalent CLI-style event for each part type (tool/step-start/step-finish/text) and dispatches through the same handlers used by orchestrator events, so labeling, tool-call rendering, and the formatWithLabel magenta prefix all share one code path. verified end-to-end via `pnpm play --local --raw` with a prompt that dispatches a reviewfrog subagent: orchestrator's task call now logs "» dispatching subagent: lens:read-readme-and-report-purpose" before the subagent runs, the subagent's read tool call surfaces with [lens:...] magenta prefix, and the run-end "subagent finished" attribution shows the lens name. also adds an AGENTS.md rule formalizing the no-write-to-repo invariant: action runtime must never write into the user's working tree; auxiliary files go in ctx.tmpdir via HOME / XDG_CONFIG_HOME. * drop opencodePlugin.test.ts — bullshit-test cleanup these tests spied on process.stdout.write, loaded the plugin source into a temp file via dynamic import, and asserted the output strings matched the plugin source i'd just hand-written. zero unique signal over the e2e run in preview repo, plus they violate AGENTS.md's "mocks tend to add ceremony and brittleness" rule. real signal lives in the e2e: lens label rendering, dispatch attribution, no double events. if a syntactic regression in the plugin source ever ships, opencode logs it on plugin load and the e2e fails fast — the unit tests would catch the same regression no faster. * remove isPausedExternally — plugin makes it unnecessary empirical proof from PR #634's e2e debug trace: ~3.3 pullfrog_bus_event lines per second arrive on the parent's child.stdout pipe during a typical subagent run. each one fires updateActivity() and resets lastActivityTime, so the inner spawn activity timer naturally stays armed-but-not-fired throughout the subagent's lifetime — no suspend predicate needed. drop: - SpawnOptions.isPausedExternally + the check in spawn()'s activity loop - isSubagentInFlight() in opencode.ts + its callsite - two isPausedExternally unit tests in subprocess.test.ts keep: - killGroup (the actual zombie-prevention fix; still tested) - the plugin (action/agents/opencodePlugin.ts; the architectural fix) - everything in opencode.ts that derives lens labels from task dispatches the only edge case isPausedExternally covered that the plugin doesn't is a non-streaming provider going silent for >5min during a single LLM call inside a subagent. that's a provider-behavior question, not a harness-architecture one — best fixed at the provider level if it shows up. defense-in-depth that adds indirection is harmful when the upstream architectural fix is already in place. * opencode: address review feedback on bus envelope routing three findings from PR #634 review (2026-05-08T22:13:44Z): 1. token/cost double-count: routing subagent step_finish through the orchestrator's handler folded subagent tokens/cost into the run-wide accumulators that flow to logTokenTable + AgentUsage. neighbouring init/text handlers all gate on ORCHESTRATOR_LABEL for exactly this reason. fix: drop step_start AND step_finish from the bus envelope handler — those carry orchestrator-scoped state (currentStepId, stepHistory, token accumulators) that subagent events shouldn't touch. tool calls and text from subagents still surface — that's the user-visible activity. 2. subagent tool errors invisible: routed status="error" tool parts into handlers.tool_use which only emits "» <tool>(...)" with no error indication. fix: extend handlers.tool_use itself to log "» tool call failed: <msg>" when state.status==="error". benefits the orchestrator path too — opencode CLI also emits failed tool calls as tool_use at status=error and we were swallowing the failure signal there as well. 3. stale comments + leaked local paths: plugin source had /tmp/opencode-investigate/... paths from my local clone, specific line numbers from opencode's dev branch that don't match v1.1.56, forkDetach claim that's wrong for the pinned version, and JSDoc that still listed message.updated/session.error in the forwarded set after the runtime filter narrowed to message.part.updated only. fix: drop machine-local paths, drop version-fragile line numbers, correct the forwarded-set list, generalize the "why no @opencode-ai/plugin import" rationale to be version-agnostic. second review (2026-05-08T22:27:58Z) confirms these are the only findings still open — no new issues from the isPausedExternally removal.
101 lines
4.7 KiB
TypeScript
101 lines
4.7 KiB
TypeScript
import { performance } from "node:perf_hooks";
|
|
import { describe, expect, it } from "vitest";
|
|
import { spawn } from "./subprocess.ts";
|
|
|
|
describe("spawn error path", () => {
|
|
it("surfaces ENOENT-style spawn failures in stderr so callers can diagnose", async () => {
|
|
// before this regression-test's fix, spawn resolved with exitCode=1 and
|
|
// an empty stderr buffer when the command itself couldn't start —
|
|
// lifecycle hook warnings then said "output: (empty)" and users had no
|
|
// way to tell a broken script from a flaky one.
|
|
const result = await spawn({
|
|
cmd: "/nonexistent-command-for-spawn-test-xyz",
|
|
args: [],
|
|
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
|
|
activityTimeout: 0,
|
|
});
|
|
|
|
expect(result.exitCode).toBe(1);
|
|
expect(result.stderr).toContain("/nonexistent-command-for-spawn-test-xyz");
|
|
expect(result.stderr).toMatch(/ENOENT|not found/i);
|
|
});
|
|
|
|
it("clears the SIGKILL escalator when a timed-out child exits cleanly from SIGTERM", async () => {
|
|
// regression: the overall-timeout path did
|
|
// setTimeout(() => { if (!child.killed) child.kill("SIGKILL") }, 5000)
|
|
// without capturing the timer id. if the child responded to SIGTERM and
|
|
// `close` fired promptly, the SIGKILL escalator stayed in the event loop
|
|
// for up to 5 seconds — delaying any clean shutdown by that long.
|
|
const beforeHandles = process.getActiveResourcesInfo().filter((r) => r === "Timeout").length;
|
|
|
|
// sleep does not install a TERM trap, so the default action (terminate)
|
|
// fires immediately — `close` lands within ms of the SIGTERM, giving us
|
|
// the orphaned-escalator window that the bug would have triggered.
|
|
const result = await spawn({
|
|
cmd: "sleep",
|
|
args: ["30"],
|
|
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
|
|
activityTimeout: 0,
|
|
timeout: 200,
|
|
}).catch((err) => err);
|
|
|
|
// timed out, so we get the SpawnTimeoutError
|
|
expect(result).toBeInstanceOf(Error);
|
|
|
|
// the SIGKILL escalator (and any other timer spawn() owned) must be
|
|
// cleared by the time the promise settles — active timer count should
|
|
// not have grown past the pre-spawn baseline.
|
|
const afterHandles = process.getActiveResourcesInfo().filter((r) => r === "Timeout").length;
|
|
expect(afterHandles).toBeLessThanOrEqual(beforeHandles);
|
|
});
|
|
|
|
it("killGroup: true propagates SIGKILL to grandchildren so close fires promptly", async () => {
|
|
// regression: node_modules/opencode-ai/bin/opencode is a Node shim that
|
|
// spawnSyncs the native binary with stdio:"inherit". without killGroup,
|
|
// child.kill("SIGKILL") hit only the shim — the native binary was
|
|
// reparented to PID 1, kept holding our stdout pipe via the inherited
|
|
// fds, and `child.on("close")` never fired (because pipes stayed open).
|
|
// a 5-min outer safety-net timer eventually rejected the agent promise,
|
|
// but the grandchild kept running until the GitHub Actions job-level
|
|
// timeout. this test replicates the shape with bash + a backgrounded
|
|
// sleep grandchild: with killGroup, close fires promptly after SIGKILL;
|
|
// without it, the parent would wait for sleep to exit (30s).
|
|
//
|
|
// the activity-check interval is fixed at 5s so the earliest the kill
|
|
// can fire is ~5s after start. budget 15s end-to-end.
|
|
const before = performance.now();
|
|
const result = await spawn({
|
|
cmd: "bash",
|
|
args: ["-c", "sleep 30 & wait"],
|
|
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
|
|
activityTimeout: 1000,
|
|
killGroup: true,
|
|
}).catch((err) => err);
|
|
const elapsed = performance.now() - before;
|
|
|
|
expect(result).toBeInstanceOf(Error);
|
|
// 10s ceiling: 5s activity-check tick + signal delivery. a regression
|
|
// here (no killGroup) would hang for the full 30s sleep.
|
|
expect(elapsed).toBeLessThan(10_000);
|
|
}, 20_000);
|
|
|
|
it("reports signal-killed subprocesses as failures, not success", async () => {
|
|
// regression: before the fix, `child.on("close", (exitCode) => ...)`
|
|
// discarded the signal parameter and `exitCode || 0` coerced the
|
|
// node-delivered null to 0. lifecycle hooks killed by OOM, segfault,
|
|
// or external SIGTERM were silently reported as exit code 0, and
|
|
// lifecycle.ts's `if (result.exitCode !== 0)` skipped the warning —
|
|
// so callers proceeded as if setup/post-checkout/prepush had succeeded.
|
|
const result = await spawn({
|
|
cmd: "bash",
|
|
args: ["-c", "kill -KILL $$"],
|
|
env: { PATH: process.env.PATH ?? "", HOME: process.env.HOME ?? "" },
|
|
activityTimeout: 0,
|
|
});
|
|
|
|
expect(result.exitCode).not.toBe(0);
|
|
expect(result.stderr).toMatch(/killed by signal/i);
|
|
expect(result.stderr).toMatch(/SIGKILL/);
|
|
});
|
|
});
|