docs: add subagent completion implementation plan
This commit is contained in:
@@ -0,0 +1,220 @@
|
||||
# Subagent Wrapper Semantic Completion Implementation Plan
|
||||
|
||||
> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.
|
||||
|
||||
**Goal:** Ensure the subagent tool completes once the wrapper has seen terminal assistant output, even if the spawned `pi` child process lingers instead of exiting promptly.
|
||||
|
||||
**Architecture:** Keep the fix on the smallest shared surface: `src/wrapper/cli.mjs`. Add a short grace-and-cleanup path that starts only after a terminal normalized `assistant_text` event, preserve the semantic result data already captured from that event, and leave monitor, runner selection, schema, and tmux helpers unchanged. Cover the behavior with a regression test in `src/wrapper/cli.test.ts` that reproduces the current stuck state.
|
||||
|
||||
**Tech Stack:** Node.js `child_process`, `fs/promises`, ESM `.mjs`, `tsx --test`, Pi JSON event normalization
|
||||
|
||||
---
|
||||
|
||||
## Worktree and file structure
|
||||
|
||||
This plan assumes work happens in a dedicated worktree already created during brainstorming.
|
||||
|
||||
**Modify**
|
||||
- `src/wrapper/cli.test.ts` — add regression coverage for terminal output followed by a lingering child process
|
||||
- `src/wrapper/cli.mjs` — add wrapper-internal grace period and forced cleanup after semantic completion
|
||||
|
||||
**Do not modify**
|
||||
- `src/monitor.ts`
|
||||
- `src/process-runner.ts`
|
||||
- `src/tmux-runner.ts`
|
||||
- `src/tmux.ts`
|
||||
- model/schema/tool registration files
|
||||
|
||||
### Task 1: Finish lingering wrapper runs after terminal assistant output
|
||||
|
||||
**Files:**
|
||||
- Modify: `src/wrapper/cli.test.ts`
|
||||
- Modify: `src/wrapper/cli.mjs`
|
||||
- Test: `src/wrapper/cli.test.ts`
|
||||
|
||||
- [ ] **Step 1: Write the failing regression test**
|
||||
|
||||
Add this test near the other wrapper regression tests in `src/wrapper/cli.test.ts`:
|
||||
|
||||
```ts
|
||||
test("wrapper exits and writes result.json after terminal output even if the pi child lingers", async () => {
|
||||
const dir = await mkdtemp(join(tmpdir(), "pi-subagents-wrapper-"));
|
||||
const metaPath = join(dir, "meta.json");
|
||||
const resultPath = join(dir, "result.json");
|
||||
const piPath = join(dir, "pi");
|
||||
|
||||
await writeFile(
|
||||
piPath,
|
||||
[
|
||||
`#!${process.execPath}`,
|
||||
"console.log(JSON.stringify({type:'message_end',message:{role:'assistant',content:[{type:'text',text:'done'}],model:'openai/gpt-5',stopReason:'stop'}}));",
|
||||
"setTimeout(() => {}, 10_000);",
|
||||
].join("\n"),
|
||||
"utf8",
|
||||
);
|
||||
await chmod(piPath, 0o755);
|
||||
|
||||
await writeFile(
|
||||
metaPath,
|
||||
JSON.stringify(
|
||||
{
|
||||
runId: "run-1",
|
||||
mode: "single",
|
||||
agent: "scout",
|
||||
agentSource: "builtin",
|
||||
task: "inspect auth",
|
||||
cwd: dir,
|
||||
requestedModel: "openai/gpt-5",
|
||||
resolvedModel: "openai/gpt-5",
|
||||
startedAt: "2026-04-09T00:00:00.000Z",
|
||||
sessionPath: join(dir, "child-session.jsonl"),
|
||||
eventsPath: join(dir, "events.jsonl"),
|
||||
resultPath,
|
||||
stdoutPath: join(dir, "stdout.log"),
|
||||
stderrPath: join(dir, "stderr.log"),
|
||||
transcriptPath: join(dir, "transcript.log"),
|
||||
systemPromptPath: join(dir, "system-prompt.md"),
|
||||
},
|
||||
null,
|
||||
2,
|
||||
),
|
||||
"utf8",
|
||||
);
|
||||
|
||||
const wrapperPath = join(dirname(fileURLToPath(import.meta.url)), "cli.mjs");
|
||||
const child = spawn(process.execPath, [wrapperPath, metaPath], {
|
||||
env: { ...process.env, PATH: dir },
|
||||
stdio: ["ignore", "pipe", "pipe"],
|
||||
});
|
||||
|
||||
const exitCode = await waitForExit(child);
|
||||
assert.equal(exitCode, 0);
|
||||
|
||||
const result = JSON.parse(await readFile(resultPath, "utf8"));
|
||||
assert.equal(result.exitCode, 0);
|
||||
assert.equal(result.stopReason, "stop");
|
||||
assert.equal(result.finalText, "done");
|
||||
assert.equal(result.resolvedModel, "openai/gpt-5");
|
||||
});
|
||||
```
|
||||
|
||||
- [ ] **Step 2: Run the wrapper tests to verify the new test fails for the right reason**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
npx tsx --test src/wrapper/cli.test.ts
|
||||
```
|
||||
|
||||
Expected: FAIL in the new test with `wrapper did not exit within 1500ms` from `waitForExit`, proving the wrapper still waits forever after terminal output when the child process lingers.
|
||||
|
||||
- [ ] **Step 3: Add shared timing helpers at the top of the wrapper**
|
||||
|
||||
Insert these declarations near the top of `src/wrapper/cli.mjs`, below the imports:
|
||||
|
||||
```js
|
||||
const SEMANTIC_EXIT_GRACE_MS = 250;
|
||||
const FORCED_EXIT_GRACE_MS = 250;
|
||||
|
||||
async function sleep(ms) {
|
||||
await new Promise((resolve) => setTimeout(resolve, ms));
|
||||
}
|
||||
```
|
||||
|
||||
- [ ] **Step 4: Wire terminal-event cleanup into `runWrapper` with the smallest shared change**
|
||||
|
||||
Make these exact edits inside `src/wrapper/cli.mjs`.
|
||||
|
||||
Add the new wrapper state immediately after the existing local state declarations inside `runWrapper`:
|
||||
|
||||
```js
|
||||
let childClosed = false;
|
||||
let completionCleanupStarted = false;
|
||||
let child;
|
||||
|
||||
const forceChildExitAfterSemanticCompletion = async () => {
|
||||
if (completionCleanupStarted) return;
|
||||
completionCleanupStarted = true;
|
||||
|
||||
await sleep(SEMANTIC_EXIT_GRACE_MS);
|
||||
if (childClosed) return;
|
||||
|
||||
child.kill("SIGTERM");
|
||||
|
||||
await sleep(FORCED_EXIT_GRACE_MS);
|
||||
if (childClosed) return;
|
||||
|
||||
child.kill("SIGKILL");
|
||||
};
|
||||
```
|
||||
|
||||
Change the `assistant_text` branch in `handleStdoutLine` to preserve the captured semantic result and trigger cleanup only when a terminal stop reason is present:
|
||||
|
||||
```js
|
||||
if (normalized.type === "assistant_text") {
|
||||
finalText = normalized.text;
|
||||
resolvedModel = normalized.model ?? resolvedModel;
|
||||
stopReason = normalized.stopReason ?? stopReason;
|
||||
usage = normalized.usage ?? usage;
|
||||
|
||||
if (normalized.stopReason) {
|
||||
void forceChildExitAfterSemanticCompletion();
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Change the child spawn assignment from `const child = spawn(...)` to:
|
||||
|
||||
```js
|
||||
child = spawn("pi", args, {
|
||||
cwd: meta.cwd,
|
||||
env: childEnv,
|
||||
stdio: ["ignore", "pipe", "pipe"],
|
||||
});
|
||||
```
|
||||
|
||||
Update the exit handlers so the cleanup helper can see when the child has already finished naturally:
|
||||
|
||||
```js
|
||||
child.on("error", (error) => {
|
||||
spawnError = error;
|
||||
childClosed = true;
|
||||
finish(1);
|
||||
});
|
||||
|
||||
child.on("close", (code) => {
|
||||
childClosed = true;
|
||||
finish(code ?? (spawnError ? 1 : 0));
|
||||
});
|
||||
```
|
||||
|
||||
This keeps all existing result-shaping logic in place, preserves best-effort artifact logging, preserves `PI_SUBAGENTS_CHILD=1`, and makes normal completion depend on the semantic terminal event rather than an indefinitely lingering process.
|
||||
|
||||
- [ ] **Step 5: Run the wrapper tests again and verify they all pass**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
npx tsx --test src/wrapper/cli.test.ts
|
||||
```
|
||||
|
||||
Expected: PASS. In particular, `wrapper exits and writes result.json after terminal output even if the pi child lingers` should now pass, and the existing initiator / child-env / artifact-write regression tests should remain green.
|
||||
|
||||
- [ ] **Step 6: Run the full package test suite**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
npm test
|
||||
```
|
||||
|
||||
Expected: all package tests PASS, including wrapper, runner, extension, and monitor coverage.
|
||||
|
||||
- [ ] **Step 7: Commit the finished bug fix**
|
||||
|
||||
Run:
|
||||
|
||||
```bash
|
||||
git add src/wrapper/cli.mjs src/wrapper/cli.test.ts
|
||||
git commit -m "fix: finish lingering subagent wrapper runs"
|
||||
```
|
||||
Reference in New Issue
Block a user