diff --git a/CHANGELOG.md b/CHANGELOG.md index 14d65565..4ff86857 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,224 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### New + +- **`supervisor_takeover(reason)` tool (TP-187, #538):** Non-destructive + escape hatch for misbehaving batches. Pauses the running wave, drains + every per-agent on-disk outbox for the current batch, and marks all + active lanes as terminated so any in-transit zombie alerts are dropped + before they reach the supervisor's user-message queue. Worktrees, + branches, batch state, and sessions are all preserved — distinct from + `orch_abort`, which kills sessions and deletes state. Use this when + the batch is producing alert spam or has hit a death-spiral pattern + but you may still want to resume the same batch. After takeover, call + `orch_status()` to inspect, then either `orch_resume(force=true)` to + continue (alert suppression is lifted automatically on resume) or + `orch_abort()` to escalate to destructive shutdown. Documented in + `templates/agents/supervisor.md` alongside the existing orch_* tool + surface, plus a new section codifying the lane-runner's text-reply + parser semantics (close keywords `skip` / `let it fail` / `close` / + `abort` / `stop` are only treated as session-close directives when + they appear in a reply under 30 characters; longer messages are + always treated as instructional re-prompts). + +### Fixed + +- **Zombie supervisor alerts after lane termination (TP-187, #538):** + Previously, when a worker lane was killed (no-progress threshold or + hard-fail), 3–5 "wants to exit" alerts that the worker emitted before + termination remained in the supervisor's user-message queue and the + agent's on-disk outbox, where they could be re-discovered later. + None of the documented operator responses (`steer`, `skip`, `let it + fail`, `orch_abort`, `orch_skip_task`) reliably drained either path. + Fix has three parts: (1) at every lane-termination decision point + (no-progress kill in `lane-runner.ts`, hard-fail in `engine.ts`), the + agent's outbox is now synchronously drained — pending `*.msg.json` + files are moved to `outbox/processed/` and other pending files (e.g., + `segment-expansion-*.json`) are renamed to `.drained` so they are + invisible to subsequent discovery scans; (2) the engine emits a new + `lane-terminated` IPC message to the supervisor process, which keys + a per-batch suppression filter (`terminatedLanes` / + `terminatedAgents` Maps) that drops any subsequent supervisor-alert + whose `context.laneNumber` or `context.agentId` matches before it + reaches `pi.sendUserMessage`; (3) the engine emits a complementary + `lane-respawned` IPC at the start of each `executeLaneV2` invocation + so a fresh task on a re-allocated lane number lifts the suppression. + The filter is also cleared on `orch_resume()`, on a new batch start, + and on `supervisor_takeover()`-then-resume. Implementation: new + `drainAgentOutbox` helper in `mailbox.ts`, `LaneTerminatedInfo` / + `LaneTerminatedCallback` types in `types.ts`, callback threading + through `engine.ts` / `execution.ts` / `resume.ts` / `engine-worker.ts`, + and IPC + filter wiring in `extension.ts`. + +- **`orch_resume(force=true)` cannot reattach after `orch_abort()` + (TP-187, #539):** `executeAbort()` deletes `.pi/batch-state.json` to + enforce its destructive contract, but the runtime registry, per-agent + manifests, lane snapshots, worktrees, and branches all survive. With + no batch-state.json, `loadBatchState()` returned null and force-resume + returned the generic "no batch found" error, forcing operators into + ~15 minutes of manual git surgery (fast-forward feature branches, + push, remove worktrees, edit STATUS, re-`orch_start`) just to do what + force-resume should have done. Fix adds a small `batch-meta.json` + runtime artifact written at batch-start to + `.pi/runtime//batch-meta.json` capturing the wave plan and + the few non-recoverable scalars (baseBranch, orchBranch, mode, + startedAt, totalWaves). On force-resume after abort, when + `loadBatchState()` returns null, the new + `reconstructBatchStateFromRuntime()` helper deterministically rebuilds + a validator-compliant `PersistedBatchState` from the surviving + artifacts: most-recent batch dir wins by mtime (lex tiebreak), + `batch-meta.json` provides wave topology and orchBranch, worker + manifests provide per-lane allocation, and the existing reconciliation + pass re-detects succeeded tasks via `.DONE` markers and STATUS.md. + When required artifacts are missing or validation fails, force-resume + fails loud with a new `resumeNoStateAfterAbort` message that names + the missing artifact and recommends `orch_start ` as the + recovery path. The non-force `orch_resume()` path is unchanged. + `orch_abort` itself remains semantically destructive — only + force-resume reads from the surviving runtime artifacts. + +- **`Worker said:` is empty in early no-progress alerts (TP-187, #540):** + When a worker exits an iteration without producing a visible assistant + message (a known failure mode in the death-spiral pattern), the + worker-exit-intercept alert sent to the supervisor showed + `Worker said: ""` — leaving the supervisor with no signal about why + the worker is stuck on the iterations where intervention could still + help. By the time the field has content, the worker is already at + no-progress count 3 (kill threshold). Fix has two parts: (1) + `templates/agents/task-worker.md` now requires a one-sentence reason + before any silent exit-with-no-progress, with concrete examples; (2) + `lane-runner.ts` falls back to walking the worker's `events.jsonl` + backward to find the most recent non-empty `assistant_message` + payload when the current turn produced no visible output, and tags + the alert with which source (`current-turn`, + `events-jsonl-fallback`, or `empty-sentinel`) produced the + `Worker said:` field. The 500-character truncation invariant is + preserved. + +- **`taskplane doctor` no longer shows empty parens for `pi installed ()` + (TP-189-C / TP-185 follow-up):** pi prints its `--version` output to + **stderr**, but `bin/taskplane.mjs`'s `getVersion()` only captured + stdout via `execSync(... { stdio: 'pipe' })`, so the doctor display was + `✅ pi installed ()` with empty parens. The fix extracts `getVersion` + to `bin/get-version.mjs` (testable ESM helper) and switches it to + `spawnSync` with `stdio: ['ignore', 'pipe', 'pipe']`. The new logic + prefers stdout but falls back to stderr when stdout is empty, and + preserves the prior fail-safe contract (returns `null` on subprocess + failure or non-zero exit — critical so shell error text isn't surfaced + as a fake version string). Manual verification: `taskplane doctor` now + shows `✅ pi installed (0.73.0)`. 7 new behavioral tests in + `extensions/tests/cli-doctor-version-capture.test.ts` cover the + stdout-precedence, stderr-fallback, trim, and null-on-failure cases. +- **`isStepMarkedComplete` death-spiral guard now skips fenced code + blocks (TP-189-A3 / TP-186 follow-up):** the helper that powers the + `review_step` REFUSED guard scanned STATUS.md line-by-line for the + literal `**Status:** ✅ Complete` pattern. If a step's body documented + that pattern inside a fenced code block (legitimate authoring of the + format itself), the guard would false-positive and refuse a legitimate + code review. The helper now uses CommonMark-aware fence tracking: + recognizes both ``` and ~~~ fences, tracks the opener char + length, + and only closes on a matching delimiter (same char, length ≥ opener + length, no trailing non-whitespace text). Mixed-delimiter examples and + `````info-string lines inside an outer fence no longer prematurely + close it. Step-heading detection is gated on being outside a fence so + a `### Step N:` line inside a code-block sample is treated as content + rather than a step boundary. 6 new unit tests cover the edge cases. + +### Docs + +- **`templates/agents/task-worker.md` reconciled with TP-186's Order of + Operations rule (TP-189-E):** two older sections were ambiguous when + read alongside the new review-gated step-completion contract from + TP-186. (1) Resume Algorithm step 6 ("all items checked → proceed to + next step") now splits behavior by Review Level: 0/1 may proceed, + but 2/3 must commit the implementation, call + `review_step(type="code")`, and only flip the per-step `**Status:**` + heading after APPROVE — with a cross-reference to the Order of + Operations section. (2) The Checkpoint Discipline / Git commits + example commit message changed from `feat(TASK-ID): complete Step N + — description` to `feat(TASK-ID): step N implementation`, plus + explicit Level 0/1 vs Level 2/3 paragraphs and a separate + `chore(TASK-ID): step N complete (code review APPROVE)` example for + the post-APPROVE status-flip commit. Both edits reuse canonical + wording from the Order of Operations + Recovery Recipe sections so + the existing source-pattern tests in + `extensions/tests/worker-step-completion-protocol.test.ts` continue to + pass; a new test 1.4b regression-guards the Resume Algorithm wording. +- **`skills/create-taskplane-task/SKILL.md` Complexity Assessment + augmented with **Per-Step Reviews vs. Consolidated Reviews + (Checkpoint Markers)** sub-section (TP-189-E):** the existing rubric + documents Review Levels 0–3 but not the second axis — *how many* + reviews fire for a given level. PROMPT authors had been discovering + this empirically (e.g., TP-186 fired only 2 reviews via checkpoint + markers vs the default ~8 it would have fired without them). The new + sub-section makes the choice explicit: per-step is the default and + right for independent multi-feature work; consolidation via + `**Plan-review checkpoint**` / `**Code review checkpoint**` markers + is appropriate for single-deliverable tasks where the steps are + mechanical applications of one design. TP-186 is referenced as the + canonical consolidation example. + +### Internal + +- **`DEFAULT_WORKER_USER_TOOLS` migrated to a shared lightweight + constants module (TP-189-B / TP-184 follow-up):** the literal + `"read,write,edit,bash,grep,find,ls"` was duplicated across + `extensions/taskplane/agent-host.ts` (canonical), `config-schema.ts` + (×2), and `types.ts` (×1), with `NOTE (TP-184)` comments pointing at + the canonical source. The duplication existed because `agent-host.ts` + imports `child_process`/`fs`, and pulling those into the schema/types + layer would either be circular (types.ts is the import root for + agent-host.ts) or pollute pure-data files with subprocess plumbing. + Sage flagged this as a future cleanup target. Fix: new + `extensions/taskplane/tool-allowlist-constants.ts` is a deliberately + import-free leaf module that owns the literal. `agent-host.ts` now + re-exports `DEFAULT_WORKER_USER_TOOLS` from the new module so + existing internal callers (`execution.ts`, + `worker-tools-allowlist.test.ts`) continue to work unchanged. + `config-schema.ts` and `types.ts` now import directly from the new + module. Verified no circular imports via a Node import probe; existing + 16-test `worker-tools-allowlist.test.ts` suite still passes (constant + value is unchanged, only its source module moved). `ENGINE_BRIDGE_TOOLS` + and `buildWorkerToolsAllowlist()` deliberately stay in `agent-host.ts` + — they have no duplication problem and live next to their consumers. +- **Architectural regression guard for the worker tool allowlist + spawn-site wiring (TP-189-A1 / TP-184 follow-up):** new + `extensions/tests/lane-runner-spawn-wiring.test.ts` (4 source-pattern + tests) asserts that `lane-runner.ts` imports `buildWorkerToolsAllowlist` + from `agent-host` and calls it as + `tools: buildWorkerToolsAllowlist(config.workerTools)` at the worker + spawn site, with explicit guards against passing `config.workerTools` + directly (which would silently drop engine bridge tools and + re-introduce issue #530). The call site is also bounded to within + ~80 lines of the surrounding `agentId:` field, sanity-checking the + call lives inside the AgentHostOptions object literal. +- **Runtime test of the `review_step` death-spiral guard's REFUSED path + (TP-189-A2 / TP-186 follow-up):** new + `extensions/tests/review-step-guard-runtime.test.ts` (5 tests) + exercises the actual `review_step` tool handler end-to-end via the + bridge-extension's tool registration. Confirms `type='code'` (and + `type='test'`) on a step marked `**Status:** ✅ Complete` returns + the documented REFUSED prose without spawning a reviewer subprocess + and without incrementing the Review Counter; `type='plan'` is exempt + even on a Complete step; `type='code'` on an In-Progress step + proceeds normally. Mocking strategy uses the bare `child_process` + specifier for portability across Node 22 and Node 24 (matches the + `windows-worktree-cleanup-fallback.test.ts` rationale). +- **Behavioral tests for `removeWorktree()` Windows MAX_PATH fallback + (TP-189-A4 / TP-188 follow-up):** new + `extensions/tests/windows-worktree-cleanup-behavioral.test.ts` (3 + tests) augments the existing source-pattern suite with end-to-end + decision-branch coverage. Uses a single `child_process` mock that + dispatches on the spawned command (git vs cmd) plus real on-disk temp + directories so the post-removal `existsSync` verification passes for + real. Covers: win32 + "Filename too long" stderr → `cmd /c rd /s /q` + fallback fires, prune-after-rd ordering verified, removed:true; win32 + + non-MAX_PATH error → fallback skipped, `WORKTREE_REMOVE_FAILED` + thrown with the original stderr; non-win32 + MAX_PATH text → + platform guard in `isWindowsMaxPathError` correctly skips the + fallback. + ## [0.28.8] - 2026-05-07 ### Enhanced diff --git a/bin/get-version.mjs b/bin/get-version.mjs new file mode 100644 index 00000000..5d05021f --- /dev/null +++ b/bin/get-version.mjs @@ -0,0 +1,50 @@ +/** + * `getVersion` — capture a CLI's version string with stdout-precedence, + * stderr-fallback, and null-on-failure semantics. + * + * Extracted from the inline `getVersion()` in `bin/taskplane.mjs` so it + * can be unit-tested without subprocessing the whole CLI. + * + * Behavior: + * - Spawns `${cmd} ${flag}` with shell:true and stdio:['ignore','pipe','pipe'] + * - Returns null if `spawnSync` itself throws (e.g., command not found) + * - Returns null if the subprocess errored OR exited with non-zero status + * (matches the prior `execSync`-throws-on-failure contract) + * - On success, returns stdout if non-empty, else stderr (some CLIs + * notably `pi` print version output to stderr) + * - Returns null if both streams are empty + * + * @since TP-189-C (extracted) / TP-185 follow-up (original fix scope) + * + * @param {string} cmd — command name (or already-formed token sequence) + * @param {string} [flag="--version"] — flag appended to cmd + * @returns {string | null} trimmed version string, or null on any failure + */ + +import { spawnSync } from "node:child_process"; + +export function getVersion(cmd, flag = "--version") { + let result; + try { + // shell:true matches the prior execSync behavior — accepts a + // space-joined command string and resolves via PATH lookup. + result = spawnSync(`${cmd} ${flag}`, [], { + shell: true, + encoding: "utf-8", + stdio: ["ignore", "pipe", "pipe"], + }); + } catch { + return null; + } + // Match prior contract: any non-success path → null. + // `execSync` previously threw on either spawn failure or non-zero exit, + // and the caller's catch returned null. Replicate that here so a CLI + // that exits 1 with shell error text in stderr (e.g., "command not + // found") does NOT surface as a fake version string. + if (!result || result.error || result.status !== 0) return null; + const stdout = (result.stdout ?? "").toString().trim(); + const stderr = (result.stderr ?? "").toString().trim(); + if (stdout) return stdout; + if (stderr) return stderr; + return null; +} diff --git a/bin/taskplane.mjs b/bin/taskplane.mjs index 7937e4eb..37468c64 100644 --- a/bin/taskplane.mjs +++ b/bin/taskplane.mjs @@ -36,6 +36,7 @@ import { ALL_GITIGNORE_PATTERNS, patternToRegex, } from "./gitignore-patterns.mjs"; +import { getVersion } from "./get-version.mjs"; // ─── Paths ────────────────────────────────────────────────────────────────── @@ -127,14 +128,10 @@ function commandExists(cmd) { } } -/** Get command version string. */ -function getVersion(cmd, flag = "--version") { - try { - return execSync(`${cmd} ${flag}`, { stdio: "pipe" }).toString().trim(); - } catch { - return null; - } -} +// `getVersion` lives in `./get-version.mjs` so it can be unit-tested +// without subprocessing the full CLI. Imported above. (TP-189-C / TP-185 +// follow-up: capture both stdout and stderr because `pi --version` +// prints to stderr; null on failure preserves the original contract.) /** * Parse the tabular output from `pi --list-models` into structured model rows. diff --git a/extensions/taskplane/agent-bridge-extension.ts b/extensions/taskplane/agent-bridge-extension.ts index 57381248..47711764 100644 --- a/extensions/taskplane/agent-bridge-extension.ts +++ b/extensions/taskplane/agent-bridge-extension.ts @@ -146,6 +146,12 @@ function writeSegmentExpansionRequest(request: SegmentExpansionRequest): string * section. All-checkboxes-checked is also NOT a trigger — it is the normal * pre-code-review state. * + * Fenced code blocks (delimited by ``` or ~~~) inside a step's body are + * skipped during the scan (TP-189-A3). This avoids a false-positive + * refusal when a step documents the literal `**Status:** ✅ Complete` + * pattern as part of its own instructions — a legitimate authoring case + * that doesn't represent an actual completion claim. + * * Designed to fail-open: any I/O error or a missing step heading returns * `false` (the review proceeds). The prompt-side Recovery Recipe is the * primary defense; this guard is a hard backstop, not a gatekeeper. @@ -165,15 +171,64 @@ export function isStepMarkedComplete(statusPath: string, stepNum: number): boole const lines = content.split(/\r?\n/); const stepHeadingRe = new RegExp(`^###\\s+Step\\s+${stepNum}\\b`); const nextStepHeadingRe = /^###\s+Step\s+\d+\b/; - + // TP-189-A3: track fenced-code-block state per CommonMark semantics. + // A fence opens with 3+ backticks OR 3+ tildes optionally followed by + // an info string (e.g., ```javascript). A fence CLOSES only when a + // matching delimiter (same char, length >= opener length) is seen on + // a line by itself — the closer line MUST NOT contain trailing + // non-whitespace text. This distinction matters: ```javascript + // inside an open fence is content, not a closer; mistreating it as a + // closer would let `**Status:** ✅ Complete` later in the same code + // block trip the guard. Tracking opener char + length also avoids + // premature close on `~~~` inside a backtick fence (or vice versa). + const openerRe = /^\s*(`{3,}|~{3,})(.*)$/; let inSection = false; + let fenceOpener: { char: string; length: number } | null = null; for (const line of lines) { if (!inSection) { if (stepHeadingRe.test(line)) inSection = true; continue; } - // Stop scanning at the next step heading. - if (nextStepHeadingRe.test(line)) break; + // Step boundaries are recognized only OUTSIDE a fenced block. + // (A `### Step N:` line inside a code-fence sample is content, + // not a real heading.) + if (fenceOpener === null && nextStepHeadingRe.test(line)) break; + // Detect fence delimiter lines. + const fenceMatch = line.match(openerRe); + if (fenceMatch) { + const delim = fenceMatch[1]; + const trailing = fenceMatch[2] ?? ""; + const char = delim[0]; // "`" or "~" + const length = delim.length; + if (fenceOpener === null) { + // Opening: any trailing text is the info string — allowed. + // CommonMark forbids backticks in a backtick info string, + // but rejecting that case here only risks false negatives + // (i.e., not opening a fence we should have); the worst- + // case impact is a real Status line being inspected as if + // outside a fence — which is the safe default. + fenceOpener = { char, length }; + continue; + } + // Already inside a fence — a line counts as a closer ONLY if: + // 1. delimiter char matches the opener, + // 2. delimiter length >= opener length, + // 3. nothing follows the delimiter except whitespace. + const trailingIsWhitespace = /^\s*$/.test(trailing); + if ( + char === fenceOpener.char && + length >= fenceOpener.length && + trailingIsWhitespace + ) { + fenceOpener = null; + continue; + } + // Else: this line is content INSIDE the open fence (e.g., + // ```javascript inside a 4-backtick fence, or a non-matching + // tilde delimiter). Fall through to the inFence skip below. + } + // Skip lines inside an open fenced code block. + if (fenceOpener !== null) continue; // Match a literal status line within this step's section. // Examples that should match: // **Status:** ✅ Complete diff --git a/extensions/taskplane/agent-host.ts b/extensions/taskplane/agent-host.ts index 699ddf15..e6c98d4d 100644 --- a/extensions/taskplane/agent-host.ts +++ b/extensions/taskplane/agent-host.ts @@ -87,23 +87,17 @@ export const ENGINE_BRIDGE_TOOLS = [ "request_segment_expansion", ] as const; -/** - * Default user-tools portion of the worker `--tools` allowlist. This is the - * fallback used when neither `taskRunner.worker.tools` config nor the - * `TASKPLANE_WORKER_TOOLS` env var supplies a value. Engine bridge tools - * (`ENGINE_BRIDGE_TOOLS`) are appended on top by - * `buildWorkerToolsAllowlist()` at the spawn site — they are NOT part of - * this default and should not be added by callers. - * - * NOTE: This literal is duplicated in `config-schema.ts` (defaults block) - * and `types.ts` (defaults block) as well. Those modules intentionally - * keep the literal to avoid pulling agent-host's heavy imports (child - * process, fs) into pure schema/type files. If you change the default - * here, update those copies too. - * - * @since TP-184 - */ -export const DEFAULT_WORKER_USER_TOOLS = "read,write,edit,bash,grep,find,ls"; +// TP-189 (Cluster B): `DEFAULT_WORKER_USER_TOOLS` now lives in the +// import-free `./tool-allowlist-constants.ts` module so that pure-data +// layers (`config-schema.ts`, `types.ts`) can import it without pulling +// agent-host's heavy `child_process`/`fs` imports into the schema/type +// graph. We re-export here so existing internal imports (e.g., +// `execution.ts`, `worker-tools-allowlist.test.ts`) continue to work +// without churn. +// +// @since TP-184 (constant introduced) / TP-189 (moved to constants module) +export { DEFAULT_WORKER_USER_TOOLS } from "./tool-allowlist-constants.ts"; +import { DEFAULT_WORKER_USER_TOOLS } from "./tool-allowlist-constants.ts"; /** * Build the final worker `--tools` allowlist string by combining the diff --git a/extensions/taskplane/config-schema.ts b/extensions/taskplane/config-schema.ts index f638948e..6d8f2ad0 100644 --- a/extensions/taskplane/config-schema.ts +++ b/extensions/taskplane/config-schema.ts @@ -37,6 +37,12 @@ * @module config/schema */ +// TP-189 (Cluster B): single source of truth for the worker user-tools +// default literal. This is a deliberately import-free module so we can +// import it here without pulling `agent-host.ts`'s `child_process`/`fs` +// imports into the schema layer. +import { DEFAULT_WORKER_USER_TOOLS } from "./tool-allowlist-constants.ts"; + // ── Config Version ─────────────────────────────────────────────────── /** @@ -594,14 +600,11 @@ export const DEFAULT_TASK_RUNNER_SECTION: TaskRunnerSection = { testing: { commands: {} }, standards: { docs: [], rules: [] }, standardsOverrides: {}, - // NOTE (TP-184): The user-tools default literal here mirrors - // `DEFAULT_WORKER_USER_TOOLS` in `agent-host.ts`. We keep the literal - // instead of importing the constant because this file is currently - // import-free (pure schema/defaults) and importing from agent-host.ts - // would pull child_process/fs into the schema layer. If you change the - // default, update both copies. Engine bridge tools are appended at the - // lane-runner spawn site by `buildWorkerToolsAllowlist()`, not here. - worker: { model: "", tools: "read,write,edit,bash,grep,find,ls", thinking: "", excludeExtensions: [] }, + // TP-189 (Cluster B): user-tools default sourced from + // `tool-allowlist-constants.ts` (single source of truth). Engine + // bridge tools are appended at the lane-runner spawn site by + // `buildWorkerToolsAllowlist()`, not here. + worker: { model: "", tools: DEFAULT_WORKER_USER_TOOLS, thinking: "", excludeExtensions: [] }, reviewer: { model: "", tools: "read,bash,grep,find,ls", thinking: "on", excludeExtensions: [] }, context: { workerContextWindow: 0, @@ -653,10 +656,12 @@ export const DEFAULT_ORCHESTRATOR_SECTION: OrchestratorSection = { }, merge: { model: "", - // NOTE (TP-184): Mirrors `DEFAULT_WORKER_USER_TOOLS`. Merge agent does - // not run through `buildWorkerToolsAllowlist()` (no bridge-tool needs) - // so this literal is independent of the worker allowlist plumbing. - tools: "read,write,edit,bash,grep,find,ls", + // TP-189 (Cluster B): merge default mirrors the worker user-tools + // constant. The merge agent does NOT run through + // `buildWorkerToolsAllowlist()` (no bridge-tool needs), so this + // reference is purely for default-value parity — not a hard + // coupling to the worker allowlist plumbing. + tools: DEFAULT_WORKER_USER_TOOLS, thinking: "off", verify: [], order: "fewest-files-first", diff --git a/extensions/taskplane/engine-worker.ts b/extensions/taskplane/engine-worker.ts index 868ddc4a..f50c13cc 100644 --- a/extensions/taskplane/engine-worker.ts +++ b/extensions/taskplane/engine-worker.ts @@ -40,6 +40,17 @@ export type WorkerToMainMessage = | { type: "monitor-update"; state: MonitorState } | { type: "engine-event"; event: EngineEvent } | { type: "supervisor-alert"; alert: SupervisorAlert } + /** + * TP-187 (#538): Lane has reached a terminal state. The supervisor process + * uses this to mark the lane terminated and filter any subsequently-arriving + * (zombie) alerts whose `context.laneNumber`/`context.agentId` matches. + */ + | { type: "lane-terminated"; info: import("./types.ts").LaneTerminatedInfo } + /** + * TP-187 (#538): Lane number has been re-allocated to a fresh task. The + * supervisor lifts the suppression so subsequent alerts pass through. + */ + | { type: "lane-respawned"; laneNumber: number; agentId: string; batchId: string } | { type: "state-sync"; state: SerializedBatchState } | { type: "complete"; state: SerializedBatchState } | { type: "error"; message: string; stack?: string; source?: WorkerErrorSource }; @@ -325,6 +336,18 @@ if (process.env.TASKPLANE_ENGINE_FORK === "1" && typeof process.send === "functi send({ type: "supervisor-alert", alert }); }; + // TP-187 (#538): Lane termination callback — forwards lane-terminated to + // the supervisor process so it can suppress in-flight zombie alerts. + const onLaneTerminated = (info: import("./types.ts").LaneTerminatedInfo) => { + send({ type: "lane-terminated", info }); + }; + + // TP-187 (#538): Lane respawn callback — forwards lane-respawned to + // the supervisor process so it can lift suppression for re-allocated lanes. + const onLaneRespawned = (laneNumber: number, agentId: string, batchId: string) => { + send({ type: "lane-respawned", laneNumber, agentId, batchId }); + }; + // ── Execute engine ─────────────────────────────────────────── const enginePromise = data.mode === "resume" ? resumeOrchBatch( @@ -340,6 +363,8 @@ if (process.env.TASKPLANE_ENGINE_FORK === "1" && typeof process.send === "functi data.force ?? false, onSupervisorAlert, data.supervisorAutonomy ?? "autonomous", + onLaneTerminated, + onLaneRespawned, ) : executeOrchBatch( data.args ?? "", @@ -355,6 +380,8 @@ if (process.env.TASKPLANE_ENGINE_FORK === "1" && typeof process.send === "functi onEngineEvent, onSupervisorAlert, data.supervisorAutonomy ?? "autonomous", + onLaneTerminated, + onLaneRespawned, ); enginePromise diff --git a/extensions/taskplane/engine.ts b/extensions/taskplane/engine.ts index dae55240..b82acec8 100644 --- a/extensions/taskplane/engine.ts +++ b/extensions/taskplane/engine.ts @@ -17,8 +17,9 @@ import { applyMergeRetryLoop, computeCleanupGatePolicy, computeMergeFailurePolic import type { CleanupGateRepoFailure } from "./messages.ts"; import { assembleDiagnosticInput, emitDiagnosticReports } from "./diagnostic-reports.ts"; import { resolveOperatorId } from "./naming.ts"; -import { applyPartialProgressToOutcomes, buildTier0EventBase, deleteBatchState, emitEngineEvent, emitTier0Event, loadBatchHistory, loadBatchState, persistRuntimeState, saveBatchHistory, seedPendingOutcomesForAllocatedLanes, syncTaskOutcomesFromMonitor, upsertTaskOutcome } from "./persistence.ts"; +import { applyPartialProgressToOutcomes, buildTier0EventBase, deleteBatchState, emitEngineEvent, emitTier0Event, loadBatchHistory, loadBatchState, persistRuntimeState, saveBatchHistory, saveBatchMetaRuntimeArtifact, seedPendingOutcomesForAllocatedLanes, syncTaskOutcomesFromMonitor, upsertTaskOutcome } from "./persistence.ts"; import { readRegistrySnapshot, isTerminalStatus, isProcessAlive as registryIsProcessAlive } from "./process-registry.ts"; +import { drainAgentOutbox } from "./mailbox.ts"; import { buildBatchProgressSnapshot, buildEngineEventBase, buildSegmentId, buildSupervisorSegmentFrontierSnapshot, defaultResilienceState, FATAL_DISCOVERY_CODES, generateBatchId, TIER0_RETRYABLE_CLASSIFICATIONS, TIER0_RETRY_BUDGETS, tier0ScopeKey, tier0WaveScopeKey } from "./types.ts"; import type { AllocatedLane, AllocatedTask, BatchHistorySummary, BatchTaskSummary, BatchWaveSummary, DiscoveryResult, EngineEventCallback, EscalationContext, LaneExecutionResult, LaneTaskOutcome, MergeWaveResult, OrchBatchPhase, OrchBatchRuntimeState, OrchestratorConfig, ParsedTask, PersistedSegmentRecord, SegmentExpansionRequest, SupervisorAlert, SupervisorAlertCallback, TaskRunnerConfig, TaskSegmentPlan, TaskSegmentPlanMap, TaskSegmentNode, Tier0EscalationPattern, Tier0RecoveryPattern, TokenCounts, WaveExecutionResult, WorkspaceConfig } from "./types.ts"; import { buildDependencyGraph, computeWaveAssignments, resolveBaseBranch, resolveRepoRoot, validateGraph } from "./waves.ts"; @@ -1781,6 +1782,8 @@ async function attemptStaleWorktreeRecovery( onSupervisorAlert?: SupervisorAlertCallback, supervisorAutonomy: "interactive" | "supervised" | "autonomous" = "autonomous", runnerConfig?: TaskRunnerConfig, + onLaneTerminated?: import("./types.ts").LaneTerminatedCallback, + onLaneRespawned?: (laneNumber: number, agentId: string, batchId: string) => void, ): Promise { // Only attempt recovery for ALLOC_WORKTREE_FAILED if (!waveResult.allocationError || waveResult.allocationError.code !== "ALLOC_WORKTREE_FAILED") { @@ -1896,6 +1899,8 @@ async function attemptStaleWorktreeRecovery( excludeExtensions: runnerConfig.worker.excludeExtensions ?? [], } : undefined, runnerConfig?.workerExcludeExtensions ?? [], + onLaneTerminated, + onLaneRespawned, ); return retryResult; @@ -1973,6 +1978,18 @@ export async function executeOrchBatch( onEngineEvent?: EngineEventCallback | null, onSupervisorAlert?: SupervisorAlertCallback | null, supervisorAutonomy: "interactive" | "supervised" | "autonomous" = "autonomous", + /** + * TP-187 (#538): Optional callback fired when a lane reaches a terminal + * state. The supervisor process forwards this over IPC and uses it to + * suppress zombie alerts queued for the now-dead lane. + */ + onLaneTerminated?: import("./types.ts").LaneTerminatedCallback | null, + /** + * TP-187 (#538): Optional callback fired when a lane is freshly + * (re-)allocated to a task. The supervisor process uses it to lift any + * zombie-alert suppression carried over from a prior wave. + */ + onLaneRespawned?: ((laneNumber: number, agentId: string, batchId: string) => void) | null, ): Promise { const repoRoot = cwd; // State files (.pi/batch-state.json, lane-state, etc.) belong in the workspace root, @@ -1999,6 +2016,23 @@ export async function executeOrchBatch( } }; + // ── TP-187 (#538): Lane termination forwarding helper ────── + // Forwards lane-terminated events through the same callback chain so the + // supervisor process can suppress zombie alerts queued for a dead lane. + const emitLaneTerminated = (info: import("./types.ts").LaneTerminatedInfo): void => { + if (onLaneTerminated) { + try { + onLaneTerminated(info); + } catch (err: unknown) { + const msg = err instanceof Error ? err.message : String(err); + execLog("batch", batchState.batchId, `lane-terminated callback failed: ${msg}`, { + laneNumber: info.laneNumber, + reason: info.reason, + }); + } + } + }; + // ── TP-040 R002: Terminal event emission helper ────────────── // Routes all early-return and terminal paths through consistent event // emission so external consumers always receive a deterministic terminal @@ -2312,6 +2346,22 @@ export async function executeOrchBatch( // ── TS-009: Persist state on batch start (after wave computation) ── persistRuntimeState("batch-start", batchState, wavePlan, latestAllocatedLanes, allTaskOutcomes, discoveryRef, stateRoot); + // ── TP-187 (#539): Persist batch-meta runtime artifact ────────────── + // Captures the wave plan and core scalars to a runtime-side file that + // survives `orch_abort()` (which deletes `.pi/batch-state.json`). Used by + // `orch_resume(force=true)` to deterministically reconstruct state when + // the main batch-state file is gone. Best-effort write: failures log only. + saveBatchMetaRuntimeArtifact(stateRoot, { + schemaVersion: 1, + batchId: batchState.batchId, + wavePlan: wavePlan.map(wave => [...wave]), + baseBranch: batchState.baseBranch, + orchBranch: batchState.orchBranch, + mode: workspaceConfig ? "workspace" : "repo", + startedAt: batchState.startedAt, + totalWaves: wavePlan.length, + }); + // ── TP-105: Runtime V2 backend selection ──────────────────── // Use Runtime V2 (no-TMUX lane-runner) when ALL conditions are met: // 1. Exactly one task in the batch @@ -2507,6 +2557,8 @@ export async function executeOrchBatch( excludeExtensions: runnerConfig.worker.excludeExtensions ?? [], } : undefined, runnerConfig?.workerExcludeExtensions ?? [], + emitLaneTerminated, + onLaneRespawned ?? undefined, ); // ── TP-039: Tier 0 — Stale worktree recovery ──────────── @@ -2530,6 +2582,8 @@ export async function executeOrchBatch( emitAlert, supervisorAutonomy, runnerConfig, + emitLaneTerminated, + onLaneRespawned ?? undefined, ); if (retryResult) { const staleRecovered = !retryResult.allocationError; @@ -3058,6 +3112,29 @@ export async function executeOrchBatch( batchProgress: buildBatchProgressSnapshot(batchState), }, }); + + // TP-187 (#538): Hard-fail termination — synchronously drain the + // agent's outbox so stale escalations/replies don't get re-discovered + // later, then emit lane-terminated so the supervisor process + // suppresses any in-transit zombie alerts targeting this lane/agent. + if (laneForTask) { + const hardFailAgentId = outcome?.sessionName && outcome.sessionName.length > 0 + ? outcome.sessionName + : `${laneForTask.laneSessionId}-worker`; + try { + const drained = drainAgentOutbox(stateRoot, batchState.batchId, hardFailAgentId); + if (drained > 0) { + execLog("batch", batchState.batchId, `hard-fail outbox drain: ${drained} entr${drained === 1 ? "y" : "ies"} for ${hardFailAgentId}`); + } + } catch { /* best effort — do not block termination */ } + emitLaneTerminated({ + laneNumber: laneForTask.laneNumber, + agentId: hardFailAgentId, + batchId: batchState.batchId, + terminatedAt: Date.now(), + reason: "hard-fail", + }); + } } // ── TS-009: Persist state after wave execution ── diff --git a/extensions/taskplane/execution.ts b/extensions/taskplane/execution.ts index 99374956..58429e27 100644 --- a/extensions/taskplane/execution.ts +++ b/extensions/taskplane/execution.ts @@ -1757,6 +1757,8 @@ export async function executeWave( reviewerConfig?: { model?: string; thinking?: string; tools?: string; excludeExtensions?: string[] }, workerConfig?: { model?: string; thinking?: string; tools?: string; excludeExtensions?: string[] } | null, workerExcludeExtensions?: string[], + onLaneTerminated?: import("./types.ts").LaneTerminatedCallback, + onLaneRespawned?: (laneNumber: number, agentId: string, batchId: string) => void, ): Promise { const startedAt = Date.now(); const policy = config.failure.on_task_failure; @@ -1866,7 +1868,7 @@ export async function executeWave( ...buildWorkerEnv(workerConfig), ...buildReviewerEnv(reviewerConfig), ...buildWorkerExcludeEnv(workerExcludeExtensions), - }, onSupervisorAlert), + }, onSupervisorAlert, onLaneTerminated, onLaneRespawned), ); // Start monitoring as a sibling async loop @@ -2577,6 +2579,14 @@ export async function executeLaneV2( isWorkspaceMode?: boolean, extraEnvVars?: Record, onSupervisorAlert?: SupervisorAlertCallback, + onLaneTerminated?: import("./types.ts").LaneTerminatedCallback, + /** + * TP-187 (#538): Optional callback fired BEFORE the first task of this + * lane begins. The supervisor process uses it to lift any zombie-alert + * suppression that was applied when this lane number was previously + * terminated (e.g., in a prior wave). + */ + onLaneRespawned?: (laneNumber: number, agentId: string, batchId: string) => void, ): Promise { const laneId = lane.laneId; const laneStartTime = Date.now(); @@ -2618,6 +2628,17 @@ export async function executeLaneV2( agentPrefix: agentIdPrefix, }); + // TP-187 (#538): Lane is freshly starting — emit lane-respawned so any + // zombie-alert suppression carried over from a prior wave's termination of + // this lane number is lifted before new alerts begin to flow. + if (onLaneRespawned) { + try { + onLaneRespawned(lane.laneNumber, buildRuntimeAgentId(agentIdPrefix, lane.laneNumber, "worker"), batchId); + } catch (err) { + execLog(laneId, "LANE", `lane-respawned callback failed: ${err instanceof Error ? err.message : String(err)}`); + } + } + for (const task of lane.tasks) { const taskSegmentId = task.task.activeSegmentId ?? null; if (shouldSkipRemaining || pauseSignal.paused) { @@ -2675,6 +2696,7 @@ export async function executeLaneV2( warnPercent: 85, killPercent: 95, onSupervisorAlert, + onLaneTerminated, }; try { diff --git a/extensions/taskplane/extension.ts b/extensions/taskplane/extension.ts index 86c82bf8..8493ceb9 100644 --- a/extensions/taskplane/extension.ts +++ b/extensions/taskplane/extension.ts @@ -42,6 +42,7 @@ import { checkRateLimit, recordSend, appendMailboxAuditEvent, + drainAgentOutbox, } from "./mailbox.ts"; import { readRegistrySnapshot, @@ -1016,6 +1017,13 @@ export function startBatchInWorker( onMonitorUpdate?: (state: import("./types.ts").MonitorState) => void, onTerminal?: () => void, onSupervisorAlert?: (alert: import("./types.ts").SupervisorAlert) => void, + /** + * TP-187 (#538): Lane-terminated and lane-respawned IPC events. The + * supervisor process tracks terminated lanes/agents and uses this to + * suppress zombie alerts from already-dead lanes. + */ + onLaneTerminated?: (info: import("./types.ts").LaneTerminatedInfo) => void, + onLaneRespawned?: (laneNumber: number, agentId: string, batchId: string) => void, ): ChildProcess | null { const workerPath = resolveEngineWorkerPath(); @@ -1053,6 +1061,8 @@ export function startBatchInWorker( wkData.force ?? false, onSupervisorAlert ?? null, wkData.supervisorAutonomy ?? "autonomous", + null, // onLaneTerminated — main-thread fallback path; alerts are local-only + null, // onLaneRespawned — main-thread fallback path; suppression maps stay clear ) : () => executeOrchBatch( wkData.args ?? "", @@ -1068,6 +1078,8 @@ export function startBatchInWorker( null, // onEngineEvent onSupervisorAlert ?? null, wkData.supervisorAutonomy ?? "autonomous", + null, // onLaneTerminated — main-thread fallback path + null, // onLaneRespawned — main-thread fallback path ); startBatchAsync(fallbackFn, batchState, ctx, updateWidget, onTerminal); return null; @@ -1170,6 +1182,15 @@ export function startBatchInWorker( onSupervisorAlert?.(msg.alert); break; + // TP-187 (#538): Lane termination handling + case "lane-terminated": + onLaneTerminated?.(msg.info); + break; + + case "lane-respawned": + onLaneRespawned?.(msg.laneNumber, msg.agentId, msg.batchId); + break; + case "state-sync": applySerializedState(batchState, msg.state); rotateStderrLogToBatch(msg.state.batchId); @@ -1659,6 +1680,82 @@ export default function (pi: ExtensionAPI) { let supervisorState = freshSupervisorState(); let supervisorConfig: SupervisorConfig = { ...DEFAULT_SUPERVISOR_CONFIG }; + // TP-187 (#538): Zombie-alert filter state + // Lane numbers and agent IDs that have reached a terminal state (no-progress + // kill, hard-fail, or supervisor-takeover). Supervisor-alert IPC messages + // whose context targets a terminated lane/agent are dropped before they + // reach pi.sendUserMessage so the operator does not see zombie alerts. + // + // Lifecycle (Step 1 design): + // - Lane reaches terminal state -> add to maps (value = epoch ms) + // - Lane re-spawned for fresh task -> remove from maps + // - orch_resume() called -> clear both maps + // - New batchId observed -> clear both maps + // - supervisor_takeover() invoked -> mark all known active lanes/agents + const terminatedLanes = new Map(); + const terminatedAgents = new Map(); + + const clearTerminationFilter = (reason: string): void => { + if (terminatedLanes.size === 0 && terminatedAgents.size === 0) return; + process.stderr.write( + `[taskplane:zombie-filter] cleared termination filter (reason: ${reason}, ` + + `lanes=${terminatedLanes.size}, agents=${terminatedAgents.size})\n`, + ); + terminatedLanes.clear(); + terminatedAgents.clear(); + }; + + /** + * TP-187 (#538) — sage post-integration follow-up: gate lane-terminated / + * lane-respawned IPC on the current batchId so a stale message from a prior + * batch (engine-worker process not yet shut down, or out-of-order delivery) + * cannot taint the supervisor's terminated-lane filter for the live batch. + * Returns true when the IPC's batchId matches the current batch (or when + * the supervisor has not yet seen any state-sync, in which case we accept + * the IPC — first batch, no risk of staleness). + */ + const ipcBatchIdMatches = (incomingBatchId: string | undefined): boolean => { + // FIX (#559) + sage post-mortem: use `orchBatchState.batchId`, NOT + // `batchState.batchId` and NOT `supervisorState.batchId`. + // + // `batchState` was the original (crashing) reference — NOT bound in + // this closure. Other regions of extension.ts legitimately bind a + // different `batchState` via destructuring inside their own functions, + // but those bindings are not visible here. + // + // `supervisorState.batchId` (the first attempted fix) is bound but is + // only populated when `activateSupervisor()` runs — supervisor activation + // is a separate event triggered by alerts/intercepts, not by every batch. + // For batches where the supervisor never activates, that field stays + // empty for the entire batch and the gate never fires (everything passes + // the empty-string accept-all branch), defeating the zombie-alert filter. + // + // `orchBatchState.batchId` is the canonical live runtime batch ID for + // the extension closure: declared on line 1669, populated by the same + // state-sync IPC that the supervisor reads from, and reliably present + // from the moment the engine-worker emits its first state-sync frame + // onward. The only window where it is `""` is the legitimate gap + // between batch launch and first state-sync — pre-planning, before any + // terminated-lane IPC could fire. + const currentBatchId = orchBatchState.batchId; + if (!currentBatchId) return true; // no live batch yet — accept + if (!incomingBatchId) return true; // legacy IPC without batchId — accept (back-compat) + return incomingBatchId === currentBatchId; + }; + + /** + * TP-187 (#538): True iff this alert targets a lane or agent that has + * already been marked terminal. Used by the supervisor-alert IPC handler + * to drop zombie alerts before they reach pi.sendUserMessage. + */ + const isAlertSuppressed = (alert: import("./types.ts").SupervisorAlert): boolean => { + const ctx = alert.context; + if (!ctx) return false; + if (typeof ctx.laneNumber === "number" && terminatedLanes.has(ctx.laneNumber)) return true; + if (typeof ctx.agentId === "string" && ctx.agentId && terminatedAgents.has(ctx.agentId)) return true; + return false; + }; + // Register supervisor prompt hook: while active, injects supervisor // system prompt on every LLM turn. No-op when supervisor is inactive. registerSupervisorPromptHook(pi, supervisorState); @@ -2095,6 +2192,9 @@ export default function (pi: ExtensionAPI) { orchBatchState = freshOrchBatchState(); latestMonitorState = null; + // TP-187 (#538): Clear zombie-alert filter for the new batch. + clearTerminationFilter("new_batch_started"); + orchBatchState.phase = "launching"; orchBatchState.startedAt = Date.now(); updateOrchWidget(); @@ -2205,8 +2305,44 @@ export default function (pi: ExtensionAPI) { // ── TP-076: Supervisor alert handler — injects alerts as user messages ── (alert) => { if (!supervisorState.active) return; // Don't send orphaned messages + // TP-187 (#538): Drop zombie alerts for already-terminated lanes/agents. + if (isAlertSuppressed(alert)) { + process.stderr.write( + `[taskplane:zombie-filter] dropped alert (category=${alert.category}, ` + + `lane=${alert.context?.laneNumber ?? "?"}, agent=${alert.context?.agentId ?? "?"})\n`, + ); + return; + } pi.sendUserMessage(alert.summary, { deliverAs: "followUp" }); }, + // TP-187 (#538): Lane-terminated handler. + (info) => { + if (!ipcBatchIdMatches(info.batchId)) { + process.stderr.write( + `[taskplane:zombie-filter] ignored stale lane-terminated IPC ` + + `(incoming batchId=${info.batchId}, current=${orchBatchState.batchId})\n`, + ); + return; + } + terminatedLanes.set(info.laneNumber, info.terminatedAt); + if (info.agentId) terminatedAgents.set(info.agentId, info.terminatedAt); + process.stderr.write( + `[taskplane:zombie-filter] lane ${info.laneNumber} (${info.agentId}) terminated ` + + `(reason: ${info.reason}); ${terminatedLanes.size} lane(s) suppressed\n`, + ); + }, + // TP-187 (#538): Lane-respawned handler. + (laneNumber, agentId, incomingBatchId) => { + if (!ipcBatchIdMatches(incomingBatchId)) { + process.stderr.write( + `[taskplane:zombie-filter] ignored stale lane-respawned IPC ` + + `(incoming batchId=${incomingBatchId}, current=${orchBatchState.batchId})\n`, + ); + return; + } + terminatedLanes.delete(laneNumber); + if (agentId) terminatedAgents.delete(agentId); + }, ); // Activate supervisor agent @@ -2439,6 +2575,9 @@ export default function (pi: ExtensionAPI) { orchBatchState = freshOrchBatchState(); latestMonitorState = null; + // TP-187 (#538): Clear zombie-alert filter so post-resume alerts pass through. + clearTerminationFilter("orch_resume_called"); + orchBatchState.phase = "launching"; orchBatchState.startedAt = Date.now(); updateOrchWidget(); @@ -2542,8 +2681,44 @@ export default function (pi: ExtensionAPI) { // ── TP-076: Supervisor alert handler — injects alerts as user messages ── (alert) => { if (!supervisorState.active) return; // Don't send orphaned messages + // TP-187 (#538): Drop zombie alerts for already-terminated lanes/agents. + if (isAlertSuppressed(alert)) { + process.stderr.write( + `[taskplane:zombie-filter] dropped alert (category=${alert.category}, ` + + `lane=${alert.context?.laneNumber ?? "?"}, agent=${alert.context?.agentId ?? "?"})\n`, + ); + return; + } pi.sendUserMessage(alert.summary, { deliverAs: "followUp" }); }, + // TP-187 (#538): Lane-terminated handler. + (info) => { + if (!ipcBatchIdMatches(info.batchId)) { + process.stderr.write( + `[taskplane:zombie-filter] ignored stale lane-terminated IPC ` + + `(incoming batchId=${info.batchId}, current=${orchBatchState.batchId})\n`, + ); + return; + } + terminatedLanes.set(info.laneNumber, info.terminatedAt); + if (info.agentId) terminatedAgents.set(info.agentId, info.terminatedAt); + process.stderr.write( + `[taskplane:zombie-filter] lane ${info.laneNumber} (${info.agentId}) terminated ` + + `(reason: ${info.reason}); ${terminatedLanes.size} lane(s) suppressed\n`, + ); + }, + // TP-187 (#538): Lane-respawned handler. + (laneNumber, agentId, incomingBatchId) => { + if (!ipcBatchIdMatches(incomingBatchId)) { + process.stderr.write( + `[taskplane:zombie-filter] ignored stale lane-respawned IPC ` + + `(incoming batchId=${incomingBatchId}, current=${orchBatchState.batchId})\n`, + ); + return; + } + terminatedLanes.delete(laneNumber); + if (agentId) terminatedAgents.delete(agentId); + }, ); // Activate supervisor agent on resume @@ -2676,6 +2851,91 @@ export default function (pi: ExtensionAPI) { // ── TP-077: Supervisor Recovery Tools ──────────────────────────── + /** + * Core logic for `supervisor_takeover(reason)`. Pauses the running wave, + * drains all per-agent on-disk outboxes for the current batch, and marks + * every active lane as terminated so any in-transit zombie alerts are + * suppressed. Distinct from `orch_abort`: + * - `orch_abort` kills sessions and deletes batch state (destructive). + * - `supervisor_takeover` pauses + drains + parks; worktrees, branches, + * state, and sessions all remain so the operator can recover manually. + * + * @since TP-187 (#538) + */ + function doSupervisorTakeover(reason: string): string { + const messages: string[] = []; + const trimmedReason = (reason ?? "").trim() || "(no reason provided)"; + messages.push(`🛡️ Supervisor takeover requested: ${trimmedReason}`); + + // 1. Pause the wave (mirror orch_pause logic but tolerate non-active phases). + const pausablePhases = new Set(["launching", "executing", "merging", "planning"]); + if (pausablePhases.has(orchBatchState.phase)) { + orchBatchState.pauseSignal.paused = true; + activeWorker?.send({ type: "pause" }); + messages.push(` ✓ Wave paused (batch ${orchBatchState.batchId})`); + } else { + messages.push(` — Batch phase is \`${orchBatchState.phase}\`; no active wave to pause`); + } + + // 2. Drain on-disk outboxes for every known agent in the current batch. + const stateRoot = execCtx?.workspaceRoot ?? execCtx?.repoRoot; + let drainedAgents = 0; + let drainedMessages = 0; + if (stateRoot && orchBatchState.batchId) { + try { + const agentIds = discoverMailboxAgentIds(stateRoot, orchBatchState.batchId); + for (const agentId of agentIds) { + try { + const n = drainAgentOutbox(stateRoot, orchBatchState.batchId, agentId); + if (n > 0) { + drainedAgents++; + drainedMessages += n; + } + } catch { /* per-agent drain best-effort */ } + } + messages.push( + ` ✓ Drained on-disk outboxes (${drainedMessages} message(s) across ${drainedAgents} agent(s))`, + ); + } catch (err) { + messages.push( + ` ⚠ Drain failed: ${err instanceof Error ? err.message : String(err)}`, + ); + } + } else { + messages.push(" — No active batch state; outbox drain skipped"); + } + + // 3. Mark all currently-known active lanes/agents as terminated so any + // in-transit zombie alerts get filtered. The maps are kept until the next + // `orch_resume` (or new batch) per the Step 1 lifecycle. + const takeoverTs = Date.now(); + let markedLanes = 0; + for (const lane of orchBatchState.currentLanes ?? []) { + terminatedLanes.set(lane.laneNumber, takeoverTs); + if (lane.laneSessionId) { + terminatedAgents.set(lane.laneSessionId, takeoverTs); + terminatedAgents.set(`${lane.laneSessionId}-worker`, takeoverTs); + terminatedAgents.set(`${lane.laneSessionId}-reviewer`, takeoverTs); + } + markedLanes++; + } + messages.push( + ` ✓ Suppressed alerts for ${markedLanes} lane(s) (lifted on next \`orch_resume\`)`, + ); + + // 4. Worktrees, branches, state, sessions are intentionally NOT touched. + messages.push(" ✓ Worktrees, branches, batch state, and sessions preserved"); + + messages.push(""); + messages.push("Recommended next steps:"); + messages.push(" • `orch_status()` to inspect current state"); + messages.push(" • `orch_resume(force=true)` to re-engage the batch (clears alert suppression)"); + messages.push(" • `orch_abort()` if escalation to destructive shutdown is required"); + + updateOrchWidget(); + return messages.join("\n"); + } + /** * Core logic for orch_retry_task. Resets a failed task to pending for re-execution. * @@ -3801,6 +4061,49 @@ export default function (pi: ExtensionAPI) { }, }); + // TP-187 (#538): supervisor_takeover — pause + drain + park (non-destructive). + pi.registerTool({ + name: "supervisor_takeover", + label: "Supervisor Takeover", + description: + "Take manual control of a misbehaving batch without destroying state. " + + "Pauses the running wave, drains all per-agent on-disk outboxes, and " + + "suppresses any in-transit alerts from already-running lanes so they " + + "do not land in your queue as zombie alerts. Worktrees, branches, " + + "batch state, and sessions are all preserved — distinct from " + + "`orch_abort` which kills sessions and deletes state. Use " + + "`orch_resume(force=true)` afterward to re-engage the batch (the " + + "alert suppression is lifted automatically on resume).", + promptSnippet: "supervisor_takeover(reason) — pause + drain + park for manual recovery", + promptGuidelines: [ + "Call supervisor_takeover when the batch is producing alert spam, " + + "hitting a death-spiral pattern, or you need to investigate without " + + "continuing execution.", + "This is the non-destructive escape hatch. Prefer this over orch_abort " + + "when you may want to resume the same batch later.", + "Always include a clear `reason` describing what triggered the takeover " + + "— it is logged for audit.", + "After takeover, call orch_status() to inspect, then either " + + "orch_resume(force=true) to continue or orch_abort() to escalate.", + ], + parameters: Type.Object({ + reason: Type.String({ + description: "Why takeover is being requested (logged for audit; required).", + }), + }), + async execute(_toolCallId, params, _signal, _onUpdate, _ctx) { + try { + const result = doSupervisorTakeover(params.reason ?? ""); + return { content: [{ type: "text" as const, text: result }], details: undefined }; + } catch (err) { + return { + content: [{ type: "text" as const, text: `Error during supervisor takeover: ${err instanceof Error ? err.message : String(err)}` }], + details: undefined, + }; + } + }, + }); + pi.registerTool({ name: "orch_integrate", label: "Integrate Batch", diff --git a/extensions/taskplane/lane-runner.ts b/extensions/taskplane/lane-runner.ts index 7e9fc3ee..64b3cacb 100644 --- a/extensions/taskplane/lane-runner.ts +++ b/extensions/taskplane/lane-runner.ts @@ -53,6 +53,7 @@ import { sessionInboxDir, ackOutboxMessage, appendMailboxAuditEvent, + drainAgentOutbox, } from "./mailbox.ts"; import { @@ -245,6 +246,14 @@ export interface LaneRunnerConfig { killPercent: number; /** Optional callback for surfacing runtime mailbox replies/escalations to supervisor */ onSupervisorAlert?: SupervisorAlertCallback; + /** + * Optional callback fired when the lane reaches a terminal state (no-progress + * kill or hard-fail). The supervisor process uses this to suppress any + * subsequent zombie alerts queued for the now-dead lane. + * + * @since TP-187 (#538) + */ + onLaneTerminated?: (info: import("./types.ts").LaneTerminatedInfo) => void; } /** @@ -656,8 +665,41 @@ export async function executeTaskV2( } } catch { /* If we can't read STATUS.md, proceed with escalation */ } - // No visible progress — compose escalation message - const truncatedMsg = assistantMessage.slice(0, 500); + // No visible progress — compose escalation message. + // TP-187 (#540): when the worker exits silently, fall back to the most + // recent `assistant_message` event in events.jsonl so the supervisor + // has SOMETHING to act on instead of `Worker said: ""`. + let workerSaid = (assistantMessage ?? "").trim(); + let workerSaidSource: "current-turn" | "events-jsonl-fallback" | "empty-sentinel" = "current-turn"; + if (!workerSaid) { + workerSaidSource = "empty-sentinel"; + try { + const raw = readFileSync(eventsPath, "utf-8"); + const lines = raw.split("\n"); + // Walk backward to find the most recent assistant_message with non-empty text. + for (let i = lines.length - 1; i >= 0; i--) { + const line = lines[i].trim(); + if (!line) continue; + try { + const evt = JSON.parse(line) as Record; + if (evt.type === "assistant_message") { + const payload = evt.payload as Record | undefined; + const text = typeof payload?.text === "string" ? payload.text.trim() : ""; + if (text) { + workerSaid = text; + workerSaidSource = "events-jsonl-fallback"; + break; + } + } + } catch { /* skip malformed line */ } + } + } catch { /* events.jsonl unreadable; sentinel will be used */ } + } + if (!workerSaid) { + workerSaid = "(no assistant message captured — worker exited without producing visible output)"; + workerSaidSource = "empty-sentinel"; + } + const truncatedMsg = workerSaid.slice(0, 500); const uncheckedItems: string[] = []; try { const statusContent = readFileSync(statusPath, "utf-8"); @@ -693,7 +735,12 @@ export async function executeTaskV2( ` Current step: ${currentStepInfo}\n` + ` Iteration: ${totalIterations}, No-progress count: ${noProgressCount + 1}\n` + ` Unchecked items: ${uncheckedItems.length > 0 ? uncheckedItems.join("; ") : "(none found)"}\n` + - ` Worker said: "${truncatedMsg}"\n` + + ` Worker said: "${truncatedMsg}"` + + (workerSaidSource === "events-jsonl-fallback" + ? ` (fallback: most-recent assistant_message from events.jsonl)\n` + : workerSaidSource === "empty-sentinel" + ? ` (no assistant message captured this iteration)\n` + : "\n") + `\nSend a steering message to ${workerAgentId} with targeted instructions,` + ` or reply "skip" / "let it fail" to close the session.`, context: { @@ -978,6 +1025,30 @@ export async function executeTaskV2( `Iteration ${totalIterations}: 0 new checkboxes (${noProgressCount}/${config.noProgressLimit} stall limit)`); if (noProgressCount >= config.noProgressLimit) { logExecution(statusPath, "Task blocked", `No progress after ${noProgressCount} iterations`); + // TP-187 (#538): synchronous outbox drain at lane-termination decision + // point. Purges any pending escalations/replies/segment-expansions the + // worker emitted just before termination so they are not later re- + // discovered and re-forwarded as zombie supervisor alerts. + try { + const drained = drainAgentOutbox(config.stateRoot, config.batchId, workerAgentId); + if (drained > 0) { + logExecution(statusPath, "Outbox drained", + `No-progress kill: drained ${drained} pending outbox entr${drained === 1 ? "y" : "ies"} for ${workerAgentId}`); + } + } catch { /* best effort — do not block termination */ } + // TP-187 (#538): notify the supervisor process so it can suppress any + // further alerts queued for this lane (zombie-alert filter). + if (config.onLaneTerminated) { + try { + config.onLaneTerminated({ + laneNumber: config.laneNumber, + agentId: workerAgentId, + batchId: config.batchId, + terminatedAt: Date.now(), + reason: "no-progress-kill", + }); + } catch { /* best effort */ } + } return makeResult(taskId, segmentId, workerAgentId, "failed", startTime, `No progress after ${noProgressCount} iterations`, false, totalIterations, cumulativeCostUsd, cumulativeTokens, config, statusPath, reviewerStatePath, lastTelemetry, snapshotSegmentCtx); } diff --git a/extensions/taskplane/mailbox.ts b/extensions/taskplane/mailbox.ts index 61b38827..d5f9b871 100644 --- a/extensions/taskplane/mailbox.ts +++ b/extensions/taskplane/mailbox.ts @@ -538,6 +538,89 @@ export function ackOutboxMessage( } } +/** + * Drain (purge to processed/) all pending outbox messages for an agent. + * + * Used at lane-termination decision points to ensure stale escalations or + * replies that the worker emitted just before termination don't get later + * re-discovered and re-forwarded as zombie supervisor alerts. The drain + * mirrors {@link ackOutboxMessage} — each pending `*.msg.json` file is + * moved to `outbox/processed/` so it remains in the durable history (for + * `read_agent_replies`) but is no longer pending. + * + * Best-effort: any per-file failure is logged but does not abort the drain. + * Returns the number of messages successfully drained. + * + * Also drains any non-message pending files in the outbox (e.g., + * `segment-expansion-*.json` requests) by renaming them to a `.drained` + * sibling so the engine's discovery scans don't re-pick them up. + * + * @since TP-187 (#538) + */ +export function drainAgentOutbox( + stateRoot: string, + batchId: string, + agentId: string, +): number { + const outboxDir = sessionOutboxDir(stateRoot, batchId, agentId); + if (!existsSync(outboxDir)) return 0; + + let entries: string[] = []; + try { + entries = readdirSync(outboxDir); + } catch (err) { + process.stderr.write( + `[mailbox] WARNING: drainAgentOutbox failed to read ${outboxDir}: ${err instanceof Error ? err.message : String(err)}\n`, + ); + return 0; + } + + let drained = 0; + const processedDir = join(outboxDir, "processed"); + let processedDirEnsured = false; + + for (const entry of entries) { + // Skip the processed/ subdirectory itself and any in-flight temp writes. + if (entry === "processed" || entry.endsWith(".tmp")) continue; + + const srcPath = join(outboxDir, entry); + + if (entry.endsWith(".msg.json")) { + if (!processedDirEnsured) { + try { mkdirSync(processedDir, { recursive: true }); } catch { /* fall through to rename error handling */ } + processedDirEnsured = true; + } + const dstPath = join(processedDir, entry); + try { + renameSync(srcPath, dstPath); + drained++; + } catch (err: unknown) { + const code = (err as NodeJS.ErrnoException).code; + if (code === "ENOENT") continue; // already gone — race-safe + process.stderr.write( + `[mailbox] WARNING: drainAgentOutbox failed to rename ${entry}: ${err instanceof Error ? err.message : String(err)}\n`, + ); + } + continue; + } + + // Non-message pending files (e.g., segment-expansion-*.json). Rename in + // place to a `.drained` suffix so engine.ts discovery scans skip them. + try { + renameSync(srcPath, `${srcPath}.drained`); + drained++; + } catch (err: unknown) { + const code = (err as NodeJS.ErrnoException).code; + if (code === "ENOENT") continue; + process.stderr.write( + `[mailbox] WARNING: drainAgentOutbox failed to mark ${entry} drained: ${err instanceof Error ? err.message : String(err)}\n`, + ); + } + } + + return drained; +} + /** * Discover all agent IDs that have mailbox directories for a batch. * Returns directory names under .pi/mailbox/{batchId}/ excluding _broadcast. diff --git a/extensions/taskplane/messages.ts b/extensions/taskplane/messages.ts index 10b5a870..1cbf9b6a 100644 --- a/extensions/taskplane/messages.ts +++ b/extensions/taskplane/messages.ts @@ -104,6 +104,25 @@ export const ORCH_MESSAGES = { resumeNoState: () => `❌ No batch to resume. No batch-state.json file found.\n` + ` Use /orch to start a new batch.`, + + /** + * TP-187 (#539): Successful reconstruction from .pi/runtime// + * runtime artifacts during force-resume after `orch_abort()`. + */ + resumeReconstructed: (batchId: string, selectionNote: string) => + `🔨 Reconstructed batch ${batchId} from .pi/runtime/ artifacts (${selectionNote}).\n` + + ` Force-resume will proceed with a fresh wave-zero pass; the existing\n` + + ` reconciliation logic will re-detect succeeded tasks via .DONE markers.`, + + /** + * TP-187 (#539): Fail-loud message when force-resume can't reconstruct + * after `orch_abort()` because required runtime artifacts are missing. + */ + resumeNoStateAfterAbort: (missingArtifact: string, batchId: string | null) => + `❌ Cannot resume after abort: ${missingArtifact}.\n` + + (batchId ? ` Last known batch: ${batchId}.\n` : "") + + ` To start fresh from the preserved worktree state, run\n` + + ` \`orch_start \` (or \`/orch \`).`, resumeInvalidState: (error: string) => `❌ Cannot resume: batch state file is invalid.\n` + ` Error: ${error}\n` + diff --git a/extensions/taskplane/path-resolver.ts b/extensions/taskplane/path-resolver.ts index 17eb4b91..f4b6fcb4 100644 --- a/extensions/taskplane/path-resolver.ts +++ b/extensions/taskplane/path-resolver.ts @@ -86,15 +86,38 @@ export function getNpmGlobalRoot(): string { return _npmGlobalRoot; } +/** + * Pi CLI npm package scopes that taskplane resolves at runtime, ordered with + * the canonical (current) scope FIRST and legacy scopes after for backward + * compatibility. Issue #560: the Pi coding agent was renamed from + * `@mariozechner/pi-coding-agent` to `@earendil-works/pi-coding-agent` in + * Pi v0.74.0. Pi's own extension loader bundles BOTH scope aliases at runtime + * for in-process module imports, but spawn-side path resolution (this file) + * has to look on disk under whichever scope was actually installed. + * + * Order matters: the new scope is preferred so a system that has BOTH + * installed (e.g., during a transition window) picks up the current Pi. + */ +const PI_PACKAGE_SCOPES = ["@earendil-works", "@mariozechner"] as const; + /** * Resolve the absolute path to the Pi coding agent CLI entrypoint (`cli.js`). * - * The Pi CLI is installed as `@mariozechner/pi-coding-agent`. On Windows, invoking - * `pi` directly executes a `.CMD` shim that cannot be spawned with `shell: false`. - * This function locates the underlying `dist/cli.js` so callers can spawn it with - * `node` directly, without a shell intermediary. + * The Pi CLI is installed under one of two npm scopes: + * - `@earendil-works/pi-coding-agent` (current, as of Pi v0.74.0) + * - `@mariozechner/pi-coding-agent` (legacy) * - * Resolution order: + * On Windows, invoking `pi` directly executes a `.CMD` shim that cannot be + * spawned with `shell: false`. This function locates the underlying + * `dist/cli.js` so callers can spawn it with `node` directly, without a shell + * intermediary. + * + * Resolution order: the cross product of base directories × package scopes, + * with each base directory tried for the new scope before any base directory + * is tried for the legacy scope. (Equivalently: scope is the inner loop, base + * is the outer loop.) + * + * Base directories (outer loop): * 1. `npm root -g` result (dynamic — covers all setups: nvm, Homebrew, volta, etc.) * 2. `%APPDATA%\npm\node_modules\...` (Windows, APPDATA env var) * 3. `%USERPROFILE%\AppData\Roaming\npm\node_modules\...` (Windows, HOME-relative) @@ -102,40 +125,53 @@ export function getNpmGlobalRoot(): string { * 5. `/usr/local/lib/node_modules/...` (macOS system Node, Linux) * 6. `/opt/homebrew/lib/node_modules/...` (macOS Homebrew) * - * @returns Absolute path to `@mariozechner/pi-coding-agent/dist/cli.js` - * @throws {Error} If the CLI entrypoint cannot be found in any known location. - * The error message includes the `npm root -g` value for diagnosis. + * Scopes per base (inner loop): + * a. `@earendil-works/pi-coding-agent/dist/cli.js` + * b. `@mariozechner/pi-coding-agent/dist/cli.js` + * + * @returns Absolute path to a Pi CLI `dist/cli.js` (under whichever scope was found). + * @throws {Error} If the CLI entrypoint cannot be found under any base × scope + * combination. The error message includes the `npm root -g` value + * AND lists both scopes searched, for operator diagnosis. */ export function resolvePiCliPath(): string { - const relPath = join("@mariozechner", "pi-coding-agent", "dist", "cli.js"); - const candidates: string[] = []; + const bases: string[] = []; // 1. Dynamic: npm root -g (covers nvm, Homebrew, volta, custom npm prefix, etc.) const npmRoot = getNpmGlobalRoot(); - if (npmRoot) candidates.push(join(npmRoot, relPath)); + if (npmRoot) bases.push(npmRoot); // 2-3. Static Windows fallbacks const home = process.env.HOME || process.env.USERPROFILE || ""; if (process.env.APPDATA) { - candidates.push(join(process.env.APPDATA, "npm", "node_modules", relPath)); + bases.push(join(process.env.APPDATA, "npm", "node_modules")); } if (home) { - candidates.push(join(home, "AppData", "Roaming", "npm", "node_modules", relPath)); + bases.push(join(home, "AppData", "Roaming", "npm", "node_modules")); // 4. macOS/Linux custom global prefix - candidates.push(join(home, ".npm-global", "lib", "node_modules", relPath)); + bases.push(join(home, ".npm-global", "lib", "node_modules")); } // 5. macOS system Node / Linux - candidates.push(join("/usr", "local", "lib", "node_modules", relPath)); + bases.push(join("/usr", "local", "lib", "node_modules")); // 6. macOS Homebrew - candidates.push(join("/opt", "homebrew", "lib", "node_modules", relPath)); + bases.push(join("/opt", "homebrew", "lib", "node_modules")); - for (const candidate of candidates) { - if (existsSync(candidate)) return candidate; + // Cross product: scope is the inner loop so a single base directory is + // fully exhausted (new scope, then legacy scope) before falling back to + // the next base. This matches operator intuition ("check the most likely + // install location for either scope first"). + for (const base of bases) { + for (const scope of PI_PACKAGE_SCOPES) { + const candidate = join(base, scope, "pi-coding-agent", "dist", "cli.js"); + if (existsSync(candidate)) return candidate; + } } throw new Error( - "Cannot find Pi CLI entrypoint (@mariozechner/pi-coding-agent/dist/cli.js). " + - "Ensure the pi coding agent is installed globally via 'npm install -g @mariozechner/pi-coding-agent'. " + + "Cannot find Pi CLI entrypoint (pi-coding-agent/dist/cli.js) under any known npm scope " + + `(${PI_PACKAGE_SCOPES.join(" or ")}). ` + + "Install via 'npm install -g @earendil-works/pi-coding-agent' " + + "(or, for legacy installs, 'npm install -g @mariozechner/pi-coding-agent'). " + `npm root -g returned: ${npmRoot || "(empty — npm may not be on PATH)"}`, ); } @@ -189,12 +225,15 @@ export function resolveTaskplanePackageFile(repoRoot: string, relPath: string): candidates.push(join("/opt", "homebrew", "lib", "node_modules", "taskplane", relPath)); // 8. Peer of pi's package (look adjacent to pi's CLI entrypoint). - // pi is at: /@mariozechner/pi-coding-agent/dist/cli.js - // so piPkgDir = /@mariozechner/pi-coding-agent (resolve up 2 levels from cli.js) - // then go up TWO more levels to reach , then into taskplane/ + // pi is at: //pi-coding-agent/dist/cli.js (where is + // @earendil-works (current) or @mariozechner (legacy)). + // so piPkgDir = //pi-coding-agent (resolve up 2 levels from cli.js). + // Then go up TWO more levels to reach , then into taskplane/. + // This works regardless of which scope Pi is installed under because we + // only walk up the directory tree — we never name the scope explicitly. try { const piPath = process.argv[1] || ""; - const piPkgDir = resolve(piPath, "..", ".."); // /@mariozechner/pi-coding-agent + const piPkgDir = resolve(piPath, "..", ".."); // //pi-coding-agent const npmRootFromPi = resolve(piPkgDir, "..", ".."); // candidates.push(join(npmRootFromPi, "taskplane", relPath)); } catch { /* ignore — process.argv[1] may be undefined in test contexts */ } diff --git a/extensions/taskplane/persistence.ts b/extensions/taskplane/persistence.ts index 80a73b20..cccd2db5 100644 --- a/extensions/taskplane/persistence.ts +++ b/extensions/taskplane/persistence.ts @@ -2,12 +2,12 @@ * State persistence, serialization, orphan detection * @module orch/persistence */ -import { readFileSync, writeFileSync, existsSync, unlinkSync, renameSync, mkdirSync, appendFileSync } from "fs"; +import { readFileSync, writeFileSync, existsSync, unlinkSync, renameSync, mkdirSync, appendFileSync, readdirSync, statSync } from "fs"; import { join, dirname, basename } from "path"; import { execLog } from "./execution.ts"; -import { BATCH_STATE_SCHEMA_VERSION, StateFileError, batchStatePath, BATCH_HISTORY_MAX_ENTRIES, defaultResilienceState, defaultBatchDiagnostics } from "./types.ts"; -import type { BatchHistorySummary } from "./types.ts"; +import { BATCH_STATE_SCHEMA_VERSION, StateFileError, batchStatePath, BATCH_HISTORY_MAX_ENTRIES, defaultResilienceState, defaultBatchDiagnostics, runtimeRoot, runtimeManifestPath } from "./types.ts"; +import type { BatchHistorySummary, RuntimeAgentManifest } from "./types.ts"; import type { AllocatedLane, DiscoveryResult, EngineEvent, EscalationContext, LaneTaskOutcome, LaneTaskStatus, MonitorState, OrchBatchPhase, OrchBatchRuntimeState, PersistedBatchState, PersistedLaneRecord, PersistedMergeResult, PersistedSegmentRecord, PersistedTaskRecord, TaskMonitorSnapshot, Tier0RecoveryPattern, WorkspaceMode } from "./types.ts"; import { sleepSync } from "./worktree.ts"; import type { PreserveFailedLaneProgressResult } from "./worktree.ts"; @@ -2085,3 +2085,378 @@ export function emitEngineEvent( } } + +// ── TP-187 (#539): Batch-Meta Runtime Artifact ───────────────────── +// +// Small JSON file written at batch-start to `.pi/runtime//batch-meta.json`. +// Captures the wave plan and the few non-recoverable scalars (baseBranch, +// orchBranch, mode, startedAt, totalWaves) so that `orch_resume(force=true)` +// can deterministically reconstruct a validator-compliant PersistedBatchState +// after `orch_abort()` deletes `.pi/batch-state.json`. +// +// Without this artifact the wave topology is unrecoverable from the surviving +// runtime registry alone (manifests don't carry wave info) and a flattened +// "single wave with all surviving tasks" reconstruction can violate DAG +// dependency ordering. See R003 plan review. + +/** + * Schema-tagged batch metadata persisted alongside per-batch runtime state. + * + * @since TP-187 (#539) + */ +export interface BatchMetaArtifact { + schemaVersion: 1; + batchId: string; + wavePlan: string[][]; + baseBranch: string; + orchBranch: string; + mode: WorkspaceMode; + startedAt: number; + totalWaves: number; +} + +/** Path to the batch-meta artifact for a given batch. */ +function batchMetaPath(stateRoot: string, batchId: string): string { + return join(runtimeRoot(stateRoot, batchId), "batch-meta.json"); +} + +/** + * Persist the wave plan and core batch metadata to the runtime artifact + * directory. Best-effort: failures are logged but do NOT crash the batch. + * + * Called once at batch-start (after wavePlan is finalized) and re-written + * whenever the wave plan mutates (segment expansion). + * + * @since TP-187 (#539) + */ +export function saveBatchMetaRuntimeArtifact( + stateRoot: string, + artifact: BatchMetaArtifact, +): void { + try { + const path = batchMetaPath(stateRoot, artifact.batchId); + mkdirSync(dirname(path), { recursive: true }); + const tmp = path + ".tmp"; + writeFileSync(tmp, JSON.stringify(artifact, null, 2) + "\n", "utf-8"); + renameSync(tmp, path); + execLog("state", artifact.batchId, "persisted batch-meta runtime artifact", { + waves: artifact.wavePlan.length, + tasks: artifact.wavePlan.reduce((sum, w) => sum + w.length, 0), + }); + } catch (err) { + execLog("state", artifact.batchId, `batch-meta write failed: ${err instanceof Error ? err.message : String(err)}`); + } +} + +/** + * Load the batch-meta artifact for a given batch, or null if missing/invalid. + * + * @since TP-187 (#539) + */ +export function loadBatchMetaRuntimeArtifact( + stateRoot: string, + batchId: string, +): BatchMetaArtifact | null { + const path = batchMetaPath(stateRoot, batchId); + if (!existsSync(path)) return null; + try { + const raw = readFileSync(path, "utf-8"); + const parsed = JSON.parse(raw); + if (!parsed || typeof parsed !== "object") return null; + const obj = parsed as Record; + if (obj.schemaVersion !== 1) return null; + if (typeof obj.batchId !== "string" || obj.batchId !== batchId) return null; + if (!Array.isArray(obj.wavePlan)) return null; + for (const wave of obj.wavePlan) { + if (!Array.isArray(wave)) return null; + for (const taskId of wave) { + if (typeof taskId !== "string") return null; + } + } + if (typeof obj.baseBranch !== "string") return null; + if (typeof obj.orchBranch !== "string") return null; + if (obj.mode !== "repo" && obj.mode !== "workspace") return null; + if (typeof obj.startedAt !== "number") return null; + if (typeof obj.totalWaves !== "number") return null; + return obj as unknown as BatchMetaArtifact; + } catch { + return null; + } +} + + +// ── TP-187 (#539): Reconstruct PersistedBatchState from runtime artifacts ── + +/** + * Result of `reconstructBatchStateFromRuntime`. On success, contains the + * validator-compliant state, the selected batchId, and a human-readable note + * about how the selection was made (used by resume's onNotify output). On + * failure, names the missing or corrupt artifact for fail-loud reporting. + * + * @since TP-187 (#539) + */ +export type ReconstructResult = + | { ok: true; state: PersistedBatchState; batchId: string; selectionNote: string } + | { ok: false; error: string }; + +/** + * List candidate `.pi/runtime//` directories newest-first by mtime, + * with lex-largest tie-break for determinism. + */ +function listRuntimeBatchDirs(stateRoot: string): { batchId: string; mtimeMs: number }[] { + const root = join(stateRoot, ".pi", "runtime"); + if (!existsSync(root)) return []; + let entries: string[] = []; + try { + entries = readdirSync(root); + } catch { + return []; + } + const candidates: { batchId: string; mtimeMs: number }[] = []; + for (const name of entries) { + const dir = join(root, name); + try { + const st = statSync(dir); + if (!st.isDirectory()) continue; + candidates.push({ batchId: name, mtimeMs: st.mtimeMs }); + } catch { + continue; + } + } + candidates.sort((a, b) => { + if (b.mtimeMs !== a.mtimeMs) return b.mtimeMs - a.mtimeMs; + return b.batchId.localeCompare(a.batchId); + }); + return candidates; +} + +/** + * Read all worker manifests under `.pi/runtime//agents/`. + * + * Returns an empty array if the agents directory is missing. + */ +function readWorkerManifests(stateRoot: string, batchId: string): RuntimeAgentManifest[] { + const agentsDir = join(runtimeRoot(stateRoot, batchId), "agents"); + if (!existsSync(agentsDir)) return []; + let entries: string[] = []; + try { + entries = readdirSync(agentsDir); + } catch { + return []; + } + const manifests: RuntimeAgentManifest[] = []; + for (const agentId of entries) { + const manifestPath = runtimeManifestPath(stateRoot, batchId, agentId); + if (!existsSync(manifestPath)) continue; + try { + const raw = readFileSync(manifestPath, "utf-8"); + const parsed = JSON.parse(raw) as RuntimeAgentManifest; + if (parsed && typeof parsed === "object" && parsed.role === "worker") { + manifests.push(parsed); + } + } catch { + continue; + } + } + return manifests; +} + +/** + * Deterministically reconstruct a validator-compliant `PersistedBatchState` + * from the surviving runtime artifacts after `.pi/batch-state.json` has been + * deleted (typically by `orch_abort()`). + * + * Required artifacts: at least one `.pi/runtime//` directory whose + * `batch-meta.json` parses cleanly AND has at least one worker manifest with + * an existing worktree on disk. Anything else returns a fail-loud error so + * the caller can surface a clear "no resumable state" message instead of + * silently producing an invalid state. + * + * @since TP-187 (#539) + */ +export function reconstructBatchStateFromRuntime(stateRoot: string): ReconstructResult { + const candidates = listRuntimeBatchDirs(stateRoot); + if (candidates.length === 0) { + return { ok: false, error: "no .pi/runtime/ directory or no batch subdirectories" }; + } + + // Try the newest batch first; if its required artifacts are missing, fall + // through to the next candidate. We stop at the first batch with a parseable + // batch-meta + at least one viable worker manifest. + const failures: string[] = []; + for (let idx = 0; idx < candidates.length; idx++) { + const cand = candidates[idx]; + const meta = loadBatchMetaRuntimeArtifact(stateRoot, cand.batchId); + if (!meta) { + failures.push(`${cand.batchId}: batch-meta.json missing or invalid`); + continue; + } + const manifests = readWorkerManifests(stateRoot, cand.batchId); + if (manifests.length === 0) { + failures.push(`${cand.batchId}: no worker manifests`); + continue; + } + const workerManifestsWithWorktree = manifests.filter(m => typeof m.cwd === "string" && m.cwd.length > 0 && existsSync(m.cwd)); + if (workerManifestsWithWorktree.length === 0) { + failures.push(`${cand.batchId}: worktree paths from manifests no longer exist on disk`); + continue; + } + + // TP-187 (#539) — sage post-integration follow-up: refuse reconstruction + // when the runtime artifacts indicate this batch was multi-repo (segment + // expansion). Reconstruction hardcodes `segments: []` and cannot recover + // the per-segment topology that lives only in the deleted batch-state. + // Resuming with `segments: []` for a multi-repo batch would silently lose + // the expansion state and could re-execute already-done segments OR fail + // dependency checks for cross-repo waves. Detection heuristic: if worker + // manifests carry more than one distinct repoId, segment expansion was + // active. Single-repo batches (the common case, including Taskplane's + // own self-orchestration) are unaffected. + { + const distinctRepoIds = new Set(); + for (const m of workerManifestsWithWorktree) { + if (typeof m.repoId === "string" && m.repoId.length > 0) { + distinctRepoIds.add(m.repoId); + } + } + if (distinctRepoIds.size > 1) { + failures.push( + `${cand.batchId}: multi-repo batch detected (${distinctRepoIds.size} distinct repoIds: ` + + `${[...distinctRepoIds].slice(0, 4).join(", ")}` + + `${distinctRepoIds.size > 4 ? ", ..." : ""}); reconstruction would lose segment ` + + `expansion state and is refused. Restore .pi/batch-state.json from backup or start a new batch.` + ); + continue; + } + } + + // Build per-lane aggregation from worker manifests. + const laneMap = new Map(); + for (const m of workerManifestsWithWorktree) { + if (typeof m.laneNumber !== "number") continue; + const lane = laneMap.get(m.laneNumber) ?? { + laneNumber: m.laneNumber, + agentId: m.agentId, + worktreePath: m.cwd, + repoId: m.repoId ?? "default", + taskIds: [] as string[], + }; + if (typeof m.taskId === "string" && m.taskId && !lane.taskIds.includes(m.taskId)) { + lane.taskIds.push(m.taskId); + } + laneMap.set(m.laneNumber, lane); + } + if (laneMap.size === 0) { + failures.push(`${cand.batchId}: no lane numbers in manifests`); + continue; + } + + // Tasks: union of taskIds across all lanes, plus any wavePlan tasks that + // are not represented (they are pending, not yet executed). + const knownTaskIds = new Set(); + for (const lane of laneMap.values()) { + for (const tid of lane.taskIds) knownTaskIds.add(tid); + } + for (const wave of meta.wavePlan) { + for (const tid of wave) knownTaskIds.add(tid); + } + + // Build task records with conservative defaults; resume's reconciliation + // pass will re-detect succeeded tasks via `.DONE` markers and STATUS.md. + const tasks: PersistedTaskRecord[] = []; + const manifestByTaskId = new Map(); + for (const m of workerManifestsWithWorktree) { + if (typeof m.taskId === "string" && m.taskId) { + manifestByTaskId.set(m.taskId, m); + } + } + for (const taskId of knownTaskIds) { + const m = manifestByTaskId.get(taskId); + const lane = m ? laneMap.get(m.laneNumber) : undefined; + const taskRecord: PersistedTaskRecord = { + taskId, + taskName: taskId, + taskFolder: m?.packet?.taskFolder ?? "", + status: "pending", + sessionName: m?.agentId ?? "", + laneNumber: lane?.laneNumber ?? 0, + startedAt: typeof m?.startedAt === "number" ? m.startedAt : null, + endedAt: null, + exitReason: "", + doneFileFound: false, + }; + if (m?.repoId) taskRecord.repoId = m.repoId; + if (m?.packet?.packetRepoId) (taskRecord as Record).packetRepoId = m.packet.packetRepoId; + if (m?.packet?.packetTaskPath) (taskRecord as Record).packetTaskPath = m.packet.packetTaskPath; + tasks.push(taskRecord); + } + + // Build lane records. + const lanes: PersistedLaneRecord[] = Array.from(laneMap.values()) + .sort((a, b) => a.laneNumber - b.laneNumber) + .map(l => { + const sessionId = l.agentId.replace(/-(worker|reviewer)$/, ""); + const rec: PersistedLaneRecord = { + laneId: `lane-${l.laneNumber}`, + laneNumber: l.laneNumber, + laneSessionId: sessionId, + worktreePath: l.worktreePath, + branch: meta.orchBranch ? `${meta.orchBranch}-lane-${l.laneNumber}` : `lane-${l.laneNumber}`, + taskIds: [...l.taskIds], + }; + if (l.repoId && l.repoId !== "default") rec.repoId = l.repoId; + return rec; + }); + + const now = Date.now(); + const reconstructed: PersistedBatchState = { + schemaVersion: BATCH_STATE_SCHEMA_VERSION, + batchId: meta.batchId, + phase: "stopped", + baseBranch: meta.baseBranch, + orchBranch: meta.orchBranch, + mode: meta.mode, + startedAt: meta.startedAt, + endedAt: null, + updatedAt: now, + currentWaveIndex: 0, + totalWaves: meta.totalWaves, + totalTasks: tasks.length, + succeededTasks: 0, + failedTasks: 0, + skippedTasks: 0, + blockedTasks: 0, + wavePlan: meta.wavePlan.map(wave => [...wave]), + lanes, + tasks, + mergeResults: [], + blockedTaskIds: [], + errors: [], + segments: [], + lastError: null, + resilience: { ...defaultResilienceState(), resumeForced: true }, + diagnostics: defaultBatchDiagnostics(), + } as PersistedBatchState; + + // Validate the reconstructed shape against the on-disk schema gate. + try { + const json = JSON.stringify(reconstructed); + validatePersistedState(JSON.parse(json)); + } catch (err) { + failures.push(`${cand.batchId}: reconstructed state failed validation: ${err instanceof Error ? err.message : String(err)}`); + continue; + } + + const totalCandidates = candidates.length; + const selectionNote = totalCandidates === 1 + ? `single batch in .pi/runtime/` + : `selected from ${totalCandidates} candidate(s) by mtime newest-first (skipped ${idx} earlier candidate(s))`; + return { ok: true, state: reconstructed, batchId: meta.batchId, selectionNote }; + } + + return { + ok: false, + error: `no reconstructable batch found in .pi/runtime/ (${failures.length} candidate(s) inspected: ${failures.slice(0, 3).join("; ")}${failures.length > 3 ? "; ..." : ""})`, + }; +} + diff --git a/extensions/taskplane/resume.ts b/extensions/taskplane/resume.ts index f72aeb00..408f2b3a 100644 --- a/extensions/taskplane/resume.ts +++ b/extensions/taskplane/resume.ts @@ -37,7 +37,7 @@ import { mergeWaveByRepo } from "./merge.ts"; import { applyMergeRetryLoop, computeCleanupGatePolicy, computeMergeFailurePolicy, extractFailedRepoId, formatRepoMergeSummary, ORCH_MESSAGES } from "./messages.ts"; import type { CleanupGateRepoFailure } from "./messages.ts"; import { resolveOperatorId } from "./naming.ts"; -import { applyPartialProgressToOutcomes, deleteBatchState, hasTaskDoneMarker, loadBatchState, persistRuntimeState, seedPendingOutcomesForAllocatedLanes, syncTaskOutcomesFromMonitor, upsertTaskOutcome } from "./persistence.ts"; +import { applyPartialProgressToOutcomes, deleteBatchState, hasTaskDoneMarker, loadBatchState, persistRuntimeState, reconstructBatchStateFromRuntime, saveBatchState, seedPendingOutcomesForAllocatedLanes, syncTaskOutcomesFromMonitor, upsertTaskOutcome } from "./persistence.ts"; import { buildBatchProgressSnapshot, buildSupervisorSegmentFrontierSnapshot, defaultResilienceState, StateFileError } from "./types.ts"; import type { AllocatedLane, AllocatedTask, LaneExecutionResult, LaneTaskOutcome, LaneTaskStatus, MergeWaveResult, OrchBatchPhase, OrchBatchRuntimeState, OrchestratorConfig, ParsedTask, PersistedBatchState, PersistedLaneRecord, PersistedSegmentRecord, ReconciledTaskState, ResumeEligibility, ResumePoint, TaskRunnerConfig, WaveExecutionResult, WorkspaceConfig } from "./types.ts"; import { buildDependencyGraph, resolveBaseBranch, resolveRepoRoot } from "./waves.ts"; @@ -1070,6 +1070,18 @@ export async function resumeOrchBatch( force: boolean = false, onSupervisorAlert?: import("./types.ts").SupervisorAlertCallback | null, supervisorAutonomy: "interactive" | "supervised" | "autonomous" = "autonomous", + /** + * TP-187 (#538): Optional callback fired when a lane reaches a terminal + * state during a resumed batch. Threaded through to executeWave so the + * supervisor process keeps suppressing zombie alerts after resume too. + */ + onLaneTerminated?: import("./types.ts").LaneTerminatedCallback | null, + /** + * TP-187 (#538): Optional callback fired when a lane is freshly + * (re-)allocated during resume. The supervisor uses it to lift any + * carried-over zombie-alert suppression. + */ + onLaneRespawned?: ((laneNumber: number, agentId: string, batchId: string) => void) | null, ): Promise { const repoRoot = cwd; // State files (.pi/batch-state.json, lane-state, etc.) belong in the workspace root, @@ -1111,13 +1123,49 @@ export async function resumeOrchBatch( } if (!persistedState) { + if (!force) { + onNotify( + ORCH_MESSAGES.resumeNoState(), + "error", + ); + // TP-040 R006: Reset phase on pre-execution early return + batchState.phase = "idle"; + return; + } + // TP-187 (#539): On force-resume, attempt deterministic reconstruction + // from .pi/runtime// runtime artifacts (typically left intact + // by `orch_abort()` even though `.pi/batch-state.json` is deleted). + const reconstruction = reconstructBatchStateFromRuntime(stateRoot); + if (!reconstruction.ok) { + onNotify( + ORCH_MESSAGES.resumeNoStateAfterAbort(reconstruction.error, null), + "error", + ); + // TP-040 R006: Reset phase on pre-execution early return + batchState.phase = "idle"; + return; + } + // Successful reconstruction: persist so the rest of resumeOrchBatch + // proceeds with a normal on-disk batch-state.json picture. onNotify( - ORCH_MESSAGES.resumeNoState(), - "error", + ORCH_MESSAGES.resumeReconstructed(reconstruction.batchId, reconstruction.selectionNote), + "warning", ); - // TP-040 R006: Reset phase on pre-execution early return - batchState.phase = "idle"; - return; + try { + saveBatchState(JSON.stringify(reconstruction.state, null, 2), stateRoot); + } catch (err) { + onNotify( + ORCH_MESSAGES.resumeNoStateAfterAbort( + `reconstructed state could not be persisted: ${err instanceof Error ? err.message : String(err)}`, + reconstruction.batchId, + ), + "error", + ); + // TP-040 R006: Reset phase on pre-execution early return + batchState.phase = "idle"; + return; + } + persistedState = reconstruction.state; } // ── 2. Check eligibility ───────────────────────────────────── @@ -2050,6 +2098,8 @@ export async function resumeOrchBatch( runnerConfig.reviewer, runnerConfig.worker, runnerConfig.workerExcludeExtensions ?? [], + onLaneTerminated ?? undefined, + onLaneRespawned ?? undefined, ); batchState.waveResults.push(waveResult); diff --git a/extensions/taskplane/tool-allowlist-constants.ts b/extensions/taskplane/tool-allowlist-constants.ts new file mode 100644 index 00000000..6a8dc8c6 --- /dev/null +++ b/extensions/taskplane/tool-allowlist-constants.ts @@ -0,0 +1,37 @@ +/** + * Lightweight, import-free constants module for the worker tool allowlist. + * + * This module exists so that pure-data layers (`config-schema.ts`, + * `types.ts`) can reference the canonical `DEFAULT_WORKER_USER_TOOLS` + * literal without pulling `agent-host.ts`'s heavy `child_process` / `fs` + * imports into the schema/types graph (which would either be circular + * or pull subprocess plumbing into pure-data files). + * + * **Strict invariant:** this module MUST NOT have any imports beyond + * TypeScript built-ins. Anything more would re-introduce the very + * coupling this module exists to break. + * + * The companion `agent-host.ts` re-exports `DEFAULT_WORKER_USER_TOOLS` + * from this module for backward compatibility — existing internal + * imports (e.g., `execution.ts`, `worker-tools-allowlist.test.ts`) + * continue to work via the agent-host re-export. New code may import + * from either location; this module is the source of truth. + * + * `ENGINE_BRIDGE_TOOLS` and the `buildWorkerToolsAllowlist()` helper + * remain in `agent-host.ts` because that's where their consumers live + * and there is no duplication problem to solve for them. + * + * @module taskplane/tool-allowlist-constants + * @since TP-189 (Cluster B) + */ + +/** + * Default user-tools portion of the worker `--tools` allowlist. This is the + * fallback used when neither `taskRunner.worker.tools` config nor the + * `TASKPLANE_WORKER_TOOLS` env var supplies a value. Engine bridge tools + * (review_step, notify_supervisor, escalate_to_supervisor, + * request_segment_expansion) are appended on top by + * `buildWorkerToolsAllowlist()` at the spawn site — they are NOT part of + * this default and should not be added by callers. + */ +export const DEFAULT_WORKER_USER_TOOLS = "read,write,edit,bash,grep,find,ls"; diff --git a/extensions/taskplane/types.ts b/extensions/taskplane/types.ts index d3d49008..bfa6a141 100644 --- a/extensions/taskplane/types.ts +++ b/extensions/taskplane/types.ts @@ -4,6 +4,10 @@ */ import { join } from "path"; import type { ExitClassification, TaskExitDiagnostic } from "./diagnostics.js"; +// TP-189 (Cluster B): single source of truth for the worker user-tools +// default literal. The constants module is import-free so this does NOT +// create a cycle (types.ts -> tool-allowlist-constants.ts is a leaf). +import { DEFAULT_WORKER_USER_TOOLS } from "./tool-allowlist-constants.ts"; // ── Types ──────────────────────────────────────────────────────────── @@ -390,11 +394,12 @@ export const DEFAULT_ORCHESTRATOR_CONFIG: OrchestratorConfig = { }, merge: { model: "", - // NOTE (TP-184): Mirrors `DEFAULT_WORKER_USER_TOOLS` in - // `agent-host.ts`. Kept as a literal here because types.ts anchors - // the module-import graph (agent-host.ts imports from types.ts), so - // importing the constant the other direction would create a cycle. - tools: "read,write,edit,bash,grep,find,ls", + // TP-189 (Cluster B): merge default sourced from the import-free + // `tool-allowlist-constants.ts` module. The previous concern about + // importing from `agent-host.ts` (which DOES depend on types.ts and + // would create a cycle) no longer applies because the constant + // lives in a leaf module that imports nothing. + tools: DEFAULT_WORKER_USER_TOOLS, thinking: "off", verify: [], order: "fewest-files-first", @@ -2189,6 +2194,30 @@ export interface SupervisorAlert { */ export type SupervisorAlertCallback = (alert: SupervisorAlert) => void; +/** + * Information about a lane that has just reached a terminal state. + * + * Emitted at the no-progress kill and hard-fail decision points so the + * supervisor process can mark the lane as terminated and drop any further + * alerts queued for it (see {@link LaneTerminatedCallback}). + * + * @since TP-187 (#538) + */ +export interface LaneTerminatedInfo { + laneNumber: number; + agentId: string; + batchId: string; + terminatedAt: number; + reason: "no-progress-kill" | "hard-fail" | "supervisor-takeover"; +} + +/** + * Callback invoked when a lane reaches a terminal state. + * + * @since TP-187 (#538) + */ +export type LaneTerminatedCallback = (info: LaneTerminatedInfo) => void; + /** * Build a batch progress snapshot from runtime state. * diff --git a/extensions/taskplane/worktree.ts b/extensions/taskplane/worktree.ts index a1f5a091..9a77d833 100644 --- a/extensions/taskplane/worktree.ts +++ b/extensions/taskplane/worktree.ts @@ -1901,7 +1901,11 @@ export function runPreflight(config: OrchestratorConfig, repoRoot?: string): Pre switch (piResult.errorKind) { case "not-found": message = "Pi not found on PATH"; - hint = "Install Pi: npm install -g @mariozechner/pi-coding-agent"; + // Issue #560: Pi was renamed from @mariozechner to @earendil-works + // in v0.74.0. Recommend the new scope for new installs; the legacy + // scope still resolves at runtime via Pi's bundled aliasing if a + // transitional install has it. + hint = "Install Pi: npm install -g @earendil-works/pi-coding-agent (legacy: @mariozechner/pi-coding-agent)"; break; case "timeout": message = `Pi did not respond within ${PI_PREFLIGHT_TIMEOUT_MS / 1000}s (retried once)`; diff --git a/extensions/tests/cli-doctor-version-capture.test.ts b/extensions/tests/cli-doctor-version-capture.test.ts new file mode 100644 index 00000000..0a8f69e6 --- /dev/null +++ b/extensions/tests/cli-doctor-version-capture.test.ts @@ -0,0 +1,98 @@ +/** + * Behavioral test for `bin/get-version.mjs`'s `getVersion()` capture + * fix — TP-189-C / TP-185 follow-up. + * + * Original bug: `taskplane doctor` displayed `✅ pi installed ()` with + * empty parens because pi prints its `--version` output to stderr but + * the prior `execSync(... { stdio: 'pipe' })` only captured stdout. + * + * Fix: `getVersion()` now uses `spawnSync` with stdio:['ignore','pipe','pipe'] + * and applies stdout-precedence with stderr fallback. Critically, it + * also preserves the prior fail-safe contract: non-zero subprocess exit + * (or `result.error`) returns `null` instead of leaking shell error text + * as a fake version string (R008 follow-up). + * + * The function lives in its own module so we can exercise it with real + * subprocesses (Node's own `node` invocation as a stand-in for arbitrary + * CLIs) instead of source-pattern checks. + * + * Run: + * cd extensions && node --experimental-strip-types --experimental-test-module-mocks \\ + * --no-warnings --import ./tests/loader.mjs \\ + * --test tests/cli-doctor-version-capture.test.ts + */ + +import { describe, it } from "node:test"; +import { strict as assert } from "node:assert"; + +// `bin/get-version.mjs` is plain ESM JavaScript (no .ts) so we import +// it directly. Path is relative to extensions/tests/. +// @ts-expect-error -- .mjs sibling without types; runtime import is fine. +import { getVersion } from "../../bin/get-version.mjs"; + +const NODE = process.execPath; + +describe("TP-189-C — getVersion() behavioral capture (success cases)", () => { + it("returns trimmed stdout when the command writes its version to stdout", () => { + const result = getVersion( + `"${NODE}" -e "process.stdout.write('1.2.3')"`, + "", + ); + assert.strictEqual(result, "1.2.3"); + }); + + it("falls back to stderr when stdout is empty (the pi --version case)", () => { + const result = getVersion( + `"${NODE}" -e "process.stderr.write('0.73.0')"`, + "", + ); + assert.strictEqual(result, "0.73.0"); + }); + + it("prefers stdout over stderr when both are non-empty", () => { + const result = getVersion( + `"${NODE}" -e "process.stdout.write('STDOUT'); process.stderr.write('STDERR')"`, + "", + ); + assert.strictEqual(result, "STDOUT"); + }); + + it("trims surrounding whitespace from the captured stream", () => { + const result = getVersion( + `"${NODE}" -e "process.stdout.write(' v9.9.9 \\n')"`, + "", + ); + assert.strictEqual(result, "v9.9.9"); + }); +}); + +describe("TP-189-C — getVersion() fail-safe contract (R008 follow-up)", () => { + it("returns null when the subprocess exits non-zero, even if stderr has text (does not leak shell error as fake version)", () => { + // Pre-fix regression: spawnSync does NOT throw on non-zero exit, + // so without an explicit status guard the function would return + // `command not found`-style error prose as a fake version. The + // guard `if (result.error || result.status !== 0) return null;` + // preserves the prior execSync-throws-on-failure contract. + const result = getVersion( + `"${NODE}" -e "process.stderr.write('boom'); process.exit(1)"`, + "", + ); + assert.strictEqual( + result, + null, + "non-zero exit must return null, not the stderr error text", + ); + }); + + it("returns null for a guaranteed-nonexistent command", () => { + // Even if the shell prints "command not found"-style text, our + // fail-safe rules treat the nonzero exit as failure → null. + const result = getVersion("__taskplane_definitely_no_such_cmd_zz12__"); + assert.strictEqual(result, null); + }); + + it("returns null when both stdout and stderr are empty on a successful exit", () => { + const result = getVersion(`"${NODE}" -e ""`, ""); + assert.strictEqual(result, null); + }); +}); diff --git a/extensions/tests/exit-interception.test.ts b/extensions/tests/exit-interception.test.ts index d3f81126..914812b1 100644 --- a/extensions/tests/exit-interception.test.ts +++ b/extensions/tests/exit-interception.test.ts @@ -244,7 +244,10 @@ describe("5.x: End-to-end interception flow contracts (TP-172)", () => { }); it("5.2: lane-runner composes alert with truncated assistant message (500 chars)", () => { - expect(laneRunnerSrc).toContain("assistantMessage.slice(0, 500)"); + // TP-187 (#540): the slice now operates on `workerSaid` (which is + // `assistantMessage` falling back to the most-recent assistant_message + // from events.jsonl). The truncation invariant is preserved. + expect(laneRunnerSrc).toContain("workerSaid.slice(0, 500)"); }); it("5.3: lane-runner collects up to 5 unchecked items", () => { diff --git a/extensions/tests/extension-ipc-batchid-scope.test.ts b/extensions/tests/extension-ipc-batchid-scope.test.ts new file mode 100644 index 00000000..c12e54d2 --- /dev/null +++ b/extensions/tests/extension-ipc-batchid-scope.test.ts @@ -0,0 +1,162 @@ +/** + * Regression guard for issue #559: `ReferenceError: batchState is not defined` + * crashes the orchestrator parent on the first IPC frame from the engine-worker. + * + * Root cause: TP-187 (#538) introduced `ipcBatchIdMatches(incomingBatchId)` and + * a pair of stderr-logging template literals inside the supervisor's IPC + * handler closure that all referenced `batchState.batchId`. The closure does + * NOT bind a `batchState` variable — only `orchBatchState` and `supervisorState` + * are in scope there (declared at the same nesting level as `terminatedLanes` / + * `terminatedAgents`). The crash fired the moment the engine-worker sent its + * first `lane-terminated` or `lane-respawned` IPC, taking down EVERY batch. + * + * Sage post-mortem follow-up: the canonical replacement is + * `orchBatchState.batchId`, NOT `supervisorState.batchId`. The latter is only + * populated when the supervisor activates (a separate event triggered by + * alerts/intercepts), so for batches where the supervisor never activates the + * gate would never fire and the zombie-alert filter would be defeated. + * `orchBatchState.batchId` is reliably populated via state-sync IPC from the + * moment the engine-worker emits its first state-sync frame onward, making + * it the correct binding for the live-batch comparison. + * + * The crash slipped through because: + * - `node --experimental-strip-types` does not perform name-resolution + * checks; it only strips type annotations, leaving runtime ReferenceErrors + * to surface at IPC time. + * - The TP-187 in-batch tests mock the IPC handlers at a different layer + * (`engine-worker → supervisor` callbacks via `executeOrchBatch`'s deps + * parameter), bypassing the actual extension closure under fault. + * + * This test asserts BOTH: + * 1. The buggy identifier `batchState.batchId` does NOT appear inside the + * lexical region between `let supervisorState = freshSupervisorState()` + * and the close of the lane-terminated/lane-respawned handler block(s). + * Other regions of extension.ts legitimately bind a different `batchState` + * via destructuring (e.g., `const batchState = lockResult.batchState;`) + * and use it correctly — those are out of scope for this guard. + * 2. The closure-scope IPC helpers reference `supervisorState.batchId`, + * which is the actually-in-scope binding. + * + * If a future edit reverts to `batchState.batchId` inside the supervisor IPC + * closure, this test fails with a clear pointer at the regression. + */ + +import { describe, it } from "node:test"; +import { strict as assert } from "node:assert"; +import { readFileSync } from "node:fs"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const extensionPath = join(__dirname, "..", "taskplane", "extension.ts"); +const source = readFileSync(extensionPath, "utf-8"); + +/** + * Strip line- and block-comments from a TypeScript source string. We use this + * to avoid false positives from documentation-of-the-bug comments that legally + * contain the forbidden identifier (e.g., 'use `supervisorState.batchId`, not + * `batchState.batchId`'). Only executable code is in scope for the regression + * guard — not commentary about the regression. + */ +function stripComments(text: string): string { + // Block comments first (greedy across lines), then line comments. + const noBlock = text.replace(/\/\*[\s\S]*?\*\//g, ""); + return noBlock.replace(/^\s*\/\/.*$/gm, ""); +} + +/** + * Locate the lexical region of the supervisor's IPC handler closure. Anchored + * by the canonical declaration `let supervisorState = freshSupervisorState();` + * (line ~1680) at the start, and the second `// Activate supervisor agent` + * comment (which marks the end of the resume-flow handler block, ~line 2710) + * at the end. Anything between these markers belongs to the closure under + * test — including BOTH copies of the lane-terminated / lane-respawned handler + * pair (one for the execute flow, one for the resume flow). + */ +function locateSupervisorClosureRegion(): { start: number; end: number; body: string } { + const startMarker = "let supervisorState = freshSupervisorState();"; + const startIdx = source.indexOf(startMarker); + assert.ok( + startIdx >= 0, + `Could not locate '${startMarker}' anchor in extension.ts — did the closure declaration site move?`, + ); + // Find the SECOND occurrence of "Activate supervisor agent" (resume flow). + const activateMarker = "Activate supervisor agent"; + const firstActivate = source.indexOf(activateMarker, startIdx); + assert.ok(firstActivate > startIdx, "Could not locate first 'Activate supervisor agent' anchor"); + const secondActivate = source.indexOf(activateMarker, firstActivate + activateMarker.length); + assert.ok(secondActivate > firstActivate, "Could not locate second 'Activate supervisor agent' anchor"); + return { + start: startIdx, + end: secondActivate, + body: source.slice(startIdx, secondActivate), + }; +} + +describe("extension.ts supervisor IPC closure — batchId scope (regression #559)", () => { + const region = locateSupervisorClosureRegion(); + const codeOnly = stripComments(region.body); + + it("references `orchBatchState.batchId` for the live-batch check (sage post-mortem)", () => { + // `orchBatchState` is the let-binding the extension manages itself + // (declared on extension.ts:1669 just above `supervisorState`). It's + // populated reliably via state-sync IPC. `supervisorState.batchId` + // would also be in scope, but it stays `""` for batches where the + // supervisor never activates — using it would defeat the gate. + assert.ok( + codeOnly.includes("orchBatchState.batchId"), + "Expected at least one reference to `orchBatchState.batchId` inside the supervisor IPC closure. " + + "That's the canonical live-batch identifier in scope. If the only batchId reference is via " + + "`supervisorState.batchId`, the gate effectively never fires (sage post-mortem on #559).", + ); + }); + + it("contains the canonical `ipcBatchIdMatches` helper", () => { + assert.ok( + region.body.includes("const ipcBatchIdMatches"), + "Expected `const ipcBatchIdMatches = ...` inside the supervisor IPC closure", + ); + }); + + it("does NOT reference `batchState.batchId` anywhere in the closure (would crash on first IPC)", () => { + // The closure does not bind a `batchState` variable. The previous bug + // referenced `batchState.batchId` in five places: + // 1. `const currentBatchId = batchState.batchId;` inside `ipcBatchIdMatches` + // 2-3. Two stderr templates `${batchState.batchId}` in the execute-flow + // lane-terminated / lane-respawned handlers + // 4-5. Two stderr templates `${batchState.batchId}` in the resume-flow + // copies of those handlers + // Any of the five threw `ReferenceError: batchState is not defined` + // when the matching IPC fired, crashing the parent process. + const occurrences = codeOnly.match(/\bbatchState\.batchId\b/g) ?? []; + assert.strictEqual( + occurrences.length, + 0, + `Found ${occurrences.length} occurrence(s) of \`batchState.batchId\` inside the ` + + `supervisor IPC closure (lines ${region.start}-${region.end}). \`batchState\` is NOT ` + + `bound in this scope — only \`supervisorState\` is. References to \`batchState.batchId\` ` + + `crash the orchestrator parent with ReferenceError on the first IPC frame (issue #559). ` + + `Use \`supervisorState.batchId\` instead.`, + ); + }); + + it("`ipcBatchIdMatches` body specifically uses `orchBatchState.batchId`", () => { + // Tightest assertion: the helper body itself is using the correct binding. + const helperStart = codeOnly.indexOf("const ipcBatchIdMatches"); + assert.ok(helperStart >= 0, "Could not locate `ipcBatchIdMatches` declaration"); + // Body extends to the next `};` which closes the arrow function. + const helperEnd = codeOnly.indexOf("};", helperStart); + assert.ok(helperEnd > helperStart, "Could not locate end of `ipcBatchIdMatches` declaration"); + const helperBody = codeOnly.slice(helperStart, helperEnd); + assert.ok( + helperBody.includes("orchBatchState.batchId"), + "`ipcBatchIdMatches` must read the current batch ID from `orchBatchState.batchId` " + + "(the let-binding the extension manages itself, populated via state-sync IPC). " + + "Reading from `supervisorState.batchId` would defeat the gate for non-supervised batches.", + ); + assert.ok( + !helperBody.includes("batchState.batchId"), + "`ipcBatchIdMatches` must NOT reference `batchState.batchId` — that's the #559 crash", + ); + }); +}); diff --git a/extensions/tests/lane-runner-spawn-wiring.test.ts b/extensions/tests/lane-runner-spawn-wiring.test.ts new file mode 100644 index 00000000..27d0a1d6 --- /dev/null +++ b/extensions/tests/lane-runner-spawn-wiring.test.ts @@ -0,0 +1,114 @@ +/** + * Static-assertion test for `lane-runner.ts` worker spawn-site wiring — TP-189-A1. + * + * Architectural regression guard for TP-184 (#530). The fix in TP-184 wired + * `buildWorkerToolsAllowlist(config.workerTools)` into the lane-runner's + * worker spawn site so that engine bridge tools (review_step, + * notify_supervisor, escalate_to_supervisor, request_segment_expansion) are + * always appended to the worker's `--tools` allowlist regardless of what + * the user configured for `taskRunner.worker.tools`. + * + * If a future edit accidentally bypasses the helper at this spawn site — + * for example by passing `config.workerTools` directly, or by hand-rolling + * a `tools.split(",")` augmentation — workers would silently regain the + * original gap (review_step missing → reviews never fire at Level >= 1). + * + * This file's purpose is purely to detect that regression at PR-review + * time via a source-pattern check. It does NOT validate the helper's + * behavior — that is covered by `worker-tools-allowlist.test.ts`. + * + * Patterned on `lane-runner-v2.test.ts` test 3.6 but with a wider + * tolerance window so harmless surrounding-line edits don't break it. + * + * Run: + * cd extensions && node --experimental-strip-types --experimental-test-module-mocks \\ + * --no-warnings --import ./tests/loader.mjs \\ + * --test tests/lane-runner-spawn-wiring.test.ts + */ + +import { describe, it } from "node:test"; +import { strict as assert } from "node:assert"; +import { readFileSync } from "node:fs"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +// Normalize CRLF -> LF so source-pattern regexes are line-ending agnostic +// (Windows checkouts via Git autocrlf rewrite line endings on disk). +const laneRunnerSrc = readFileSync( + join(__dirname, "..", "taskplane", "lane-runner.ts"), + "utf-8", +).replace(/\r\n/g, "\n"); + +describe("TP-189-A1 — lane-runner.ts worker spawn-site wires buildWorkerToolsAllowlist", () => { + it("imports buildWorkerToolsAllowlist from agent-host", () => { + // The helper must be imported (not redefined locally). Use a + // forgiving regex that tolerates multi-line import blocks and + // trailing punctuation/whitespace. + assert.match( + laneRunnerSrc, + /import\s*\{[^}]*\bbuildWorkerToolsAllowlist\b[^}]*\}\s*from\s*["']\.\/agent-host(?:\.ts|\.js|\.\w+)?["']/s, + "lane-runner.ts must import buildWorkerToolsAllowlist from ./agent-host", + ); + }); + + it("calls buildWorkerToolsAllowlist(config.workerTools) at the worker spawn site (AgentHostOptions assembly)", () => { + // The wiring point: somewhere inside the AgentHostOptions object + // literal (the worker spawn payload), the `tools:` field must be + // set to `buildWorkerToolsAllowlist(config.workerTools)`. Tolerate + // trailing comma/whitespace; tolerate optional `as const` casts. + const expected = + /\btools\s*:\s*buildWorkerToolsAllowlist\(\s*config\.workerTools\s*\)/; + assert.match( + laneRunnerSrc, + expected, + "lane-runner.ts must wire `tools: buildWorkerToolsAllowlist(config.workerTools)` " + + "in the worker spawn options. If a refactor moved this site, update both this " + + "test and the surrounding TP-184 NOTE comment.", + ); + }); + + it("does NOT pass config.workerTools directly as the spawn `tools:` value (regression guard)", () => { + // A common future-foot-gun: someone refactors the helper out and + // passes config.workerTools straight through. That would silently + // drop ENGINE_BRIDGE_TOOLS (review_step etc.) from the worker's + // allowlist, re-introducing issue #530. + assert.doesNotMatch( + laneRunnerSrc, + /\btools\s*:\s*config\.workerTools\b/, + "lane-runner.ts must NOT pass config.workerTools directly as the worker `tools:` " + + "option. Use buildWorkerToolsAllowlist(config.workerTools) so engine bridge tools " + + "(review_step, notify_supervisor, escalate_to_supervisor, request_segment_expansion) " + + "are always present. See TP-184 / issue #530.", + ); + }); + + it("the spawn-site call appears within ~80 lines of an `agentId:` field (sanity check the call is in an AgentHostOptions object)", () => { + // Defense-in-depth: ensure the buildWorkerToolsAllowlist call + // isn't accidentally orphaned in some unrelated helper. The + // AgentHostOptions object literal in lane-runner.ts has an + // `agentId:` field, and the `tools:` line should be near it. + const helperCallIdx = laneRunnerSrc.search( + /\btools\s*:\s*buildWorkerToolsAllowlist\(\s*config\.workerTools\s*\)/, + ); + assert.ok( + helperCallIdx > -1, + "buildWorkerToolsAllowlist call site not found (covered by previous test)", + ); + // Find the nearest `agentId:` field BEFORE the helper call. + const before = laneRunnerSrc.slice(0, helperCallIdx); + const lastAgentIdIdx = before.lastIndexOf("agentId:"); + assert.ok( + lastAgentIdIdx > -1, + "no `agentId:` field found before the buildWorkerToolsAllowlist call site", + ); + const linesBetween = + laneRunnerSrc.slice(lastAgentIdIdx, helperCallIdx).split("\n").length; + assert.ok( + linesBetween < 80, + `buildWorkerToolsAllowlist call site is ${linesBetween} lines from the nearest \`agentId:\` field; ` + + `expected < 80 (call should be inside the AgentHostOptions object literal). ` + + `If the spawn site has been refactored, widen this tolerance or update the test.`, + ); + }); +}); diff --git a/extensions/tests/lane-runner-v2.test.ts b/extensions/tests/lane-runner-v2.test.ts index c91c5e32..9f63e4b6 100644 --- a/extensions/tests/lane-runner-v2.test.ts +++ b/extensions/tests/lane-runner-v2.test.ts @@ -169,10 +169,9 @@ describe("3.x: executeLaneV2 integration in execution.ts", () => { it("3.6: executeLaneV2 preserves commitTaskArtifacts and worktree reset", () => { const start = executionSrc.indexOf("export async function executeLaneV2("); // TP-181: window widened from 5000 to 6000 to accommodate worker env - // var reads (TASKPLANE_WORKER_{MODEL,THINKING,TOOLS}) added in PR #522. - // `runGit(` is at the tail of the function body and crossed the prior - // boundary; both target calls still present in the function. - const bodySection = executionSrc.slice(start, start + 6000); + // var reads. TP-187: widened to 7000 to accommodate the lane-respawned + // emit added at the top of the function body. + const bodySection = executionSrc.slice(start, start + 7000); expect(bodySection).toContain("commitTaskArtifacts("); expect(bodySection).toContain("runGit("); }); diff --git a/extensions/tests/path-resolver-pi-scope.test.ts b/extensions/tests/path-resolver-pi-scope.test.ts new file mode 100644 index 00000000..67a99d11 --- /dev/null +++ b/extensions/tests/path-resolver-pi-scope.test.ts @@ -0,0 +1,216 @@ +/** + * Regression guard for issue #560: Pi was renamed from + * `@mariozechner/pi-coding-agent` to `@earendil-works/pi-coding-agent` + * in Pi v0.74.0. The hardcoded `@mariozechner` reference in + * `resolvePiCliPath()` made every Runtime V2 spawn fail with + * "Cannot find Pi CLI entrypoint" on systems with only the new scope. + * + * The fix searches both scopes (new first, legacy fallback). These + * tests verify: + * + * 1. Resolution succeeds when ONLY `@earendil-works` is installed + * (the failure mode #560 actually hit). + * 2. Resolution still succeeds when ONLY `@mariozechner` is installed + * (backward compat for legacy installs). + * 3. Resolution prefers `@earendil-works` when both are present + * (transition window — operator just upgraded but legacy hasn't + * been pruned). + * 4. The error message names BOTH scopes when neither is found, so + * operators get a clear pointer regardless of which install command + * they previously ran. + * + * Strategy: each test creates a temp directory laid out like an npm + * global root (`//pi-coding-agent/dist/cli.js`), points + * `getNpmGlobalRoot()` at the temp dir via a child-process probe, and + * inspects the resolved path. We use a child probe (rather than + * mocking `npm root -g` in-process) because `getNpmGlobalRoot()` + * caches its result at module level on first call \u2014 hard to reset + * within a single test run. + * + * The child probe sets `npm_config_prefix` env var, which `npm root -g` + * honors. That redirects npm's global root computation to our temp + * directory without needing to mock `child_process.execSync`. + */ + +import { describe, it } from "node:test"; +import { strict as assert } from "node:assert"; +import { execFileSync } from "node:child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { join, dirname } from "node:path"; +import { tmpdir } from "node:os"; +import { fileURLToPath } from "node:url"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const repoRoot = join(__dirname, ".."); + +/** + * Set up a temp directory containing the requested Pi scopes, return the + * temp directory path. Caller is responsible for cleanup via `rmSync`. + * + * Layout for each scope: `/lib/node_modules//pi-coding-agent/dist/cli.js` + * + * (`lib/node_modules` matches what `npm root -g` produces under + * `npm_config_prefix=` on Linux/macOS; on Windows it's just + * `/node_modules` but mkdir-p handles either layout the same way.) + */ +function makeNpmRootWithScopes(scopes: ReadonlyArray<"@earendil-works" | "@mariozechner">): { + tmpDir: string; + npmRootDir: string; + cleanup: () => void; +} { + const tmpDir = mkdtempSync(join(tmpdir(), "tp560-pi-scope-")); + // Mirror npm's global-prefix layout. On POSIX: /lib/node_modules. + // On Windows: /node_modules. We create both so the test is + // platform-agnostic and the resolver finds it under either npm root + // reporting convention. + const posixRoot = join(tmpDir, "lib", "node_modules"); + const winRoot = join(tmpDir, "node_modules"); + mkdirSync(posixRoot, { recursive: true }); + mkdirSync(winRoot, { recursive: true }); + for (const root of [posixRoot, winRoot]) { + for (const scope of scopes) { + const distDir = join(root, scope, "pi-coding-agent", "dist"); + mkdirSync(distDir, { recursive: true }); + writeFileSync(join(distDir, "cli.js"), "// fake pi cli for #560 regression test\n", "utf-8"); + } + } + const npmRootDir = process.platform === "win32" ? winRoot : posixRoot; + return { tmpDir, npmRootDir, cleanup: () => rmSync(tmpDir, { recursive: true, force: true }) }; +} + +/** + * Run a child Node process that imports `path-resolver.ts` with the given + * `npm_config_prefix` redirecting `npm root -g`. Returns the resolved path + * or throws (capturing stderr) so test assertions can match either outcome. + */ +function probeResolveInChild(npmConfigPrefix: string | null): { ok: true; resolved: string } | { ok: false; stderr: string } { + const probeScript = ` + import("${pathToFileUrl(join(repoRoot, "taskplane", "path-resolver.ts"))}").then((m) => { + try { + const resolved = m.resolvePiCliPath(); + process.stdout.write("OK::" + resolved); + } catch (err) { + process.stderr.write("ERR::" + (err && err.message ? err.message : String(err))); + process.exit(1); + } + }); + `; + const env: NodeJS.ProcessEnv = { ...process.env }; + if (npmConfigPrefix) { + env.npm_config_prefix = npmConfigPrefix; + // Belt and suspenders: also clear any cached parent inherited values. + delete env.NPM_CONFIG_PREFIX; + env.NPM_CONFIG_PREFIX = npmConfigPrefix; + } + try { + const out = execFileSync( + process.execPath, + ["--experimental-strip-types", "--no-warnings", "--input-type=module", "-e", probeScript], + { env, encoding: "utf-8", stdio: ["ignore", "pipe", "pipe"] }, + ).toString(); + const m = out.match(/^OK::(.*)$/s); + if (m) return { ok: true, resolved: m[1].trim() }; + return { ok: false, stderr: `unexpected stdout: ${out}` }; + } catch (err: unknown) { + const e = err as { stderr?: Buffer | string }; + const stderr = (e.stderr ?? "").toString(); + const m = stderr.match(/ERR::(.*)/s); + return { ok: false, stderr: m ? m[1].trim() : stderr }; + } +} + +/** Convert an absolute path to a `file:///...` URL for dynamic import in the probe. */ +function pathToFileUrl(absPath: string): string { + const normalized = absPath.replace(/\\/g, "/"); + return normalized.startsWith("/") ? `file://${normalized}` : `file:///${normalized}`; +} + +describe("resolvePiCliPath — Pi scope rename (#560)", () => { + it("8.4 (#560): resolves under @earendil-works/pi-coding-agent (current scope)", () => { + const { tmpDir, cleanup } = makeNpmRootWithScopes(["@earendil-works"]); + try { + const result = probeResolveInChild(tmpDir); + assert.ok(result.ok, `expected resolution to succeed, got: ${result.ok ? "OK" : result.stderr}`); + if (result.ok) { + assert.match( + result.resolved, + /[\\/]@earendil-works[\\/]pi-coding-agent[\\/]dist[\\/]cli\.js$/, + `expected resolved path under @earendil-works, got: ${result.resolved}`, + ); + } + } finally { + cleanup(); + } + }); + + it("8.5 (#560): resolves under @mariozechner/pi-coding-agent (legacy scope, backward compat)", () => { + const { tmpDir, cleanup } = makeNpmRootWithScopes(["@mariozechner"]); + try { + const result = probeResolveInChild(tmpDir); + assert.ok(result.ok, `expected resolution to succeed, got: ${result.ok ? "OK" : result.stderr}`); + if (result.ok) { + assert.match( + result.resolved, + /[\\/]@mariozechner[\\/]pi-coding-agent[\\/]dist[\\/]cli\.js$/, + `expected resolved path under @mariozechner, got: ${result.resolved}`, + ); + } + } finally { + cleanup(); + } + }); + + it("8.6 (#560): prefers @earendil-works when BOTH scopes are present (new-first ordering)", () => { + const { tmpDir, cleanup } = makeNpmRootWithScopes(["@earendil-works", "@mariozechner"]); + try { + const result = probeResolveInChild(tmpDir); + assert.ok(result.ok, `expected resolution to succeed, got: ${result.ok ? "OK" : result.stderr}`); + if (result.ok) { + assert.match( + result.resolved, + /[\\/]@earendil-works[\\/]pi-coding-agent[\\/]dist[\\/]cli\.js$/, + "with both scopes installed, the resolver must prefer @earendil-works (current scope wins)", + ); + assert.ok( + !/@mariozechner/.test(result.resolved), + `resolver picked @mariozechner over @earendil-works \u2014 ordering regression: ${result.resolved}`, + ); + } + } finally { + cleanup(); + } + }); + + it("8.7 (#560): error message names BOTH scopes when neither is found", () => { + // Empty npm root \u2014 no Pi installed under any scope. + const tmpDir = mkdtempSync(join(tmpdir(), "tp560-empty-")); + try { + // Create the directory structure but no Pi packages. This forces the + // resolver to exhaust all candidates and throw. + mkdirSync(join(tmpDir, "lib", "node_modules"), { recursive: true }); + mkdirSync(join(tmpDir, "node_modules"), { recursive: true }); + const result = probeResolveInChild(tmpDir); + // The resolver may still find Pi via OTHER candidate locations + // (the test's npm_config_prefix only affects `npm root -g`, not the + // other static fallback paths like /usr/local/lib/...). On a + // developer machine those other paths may have Pi installed. Skip + // the negative assertion if resolution succeeded \u2014 we can't + // guarantee a clean negative on a real dev box. + if (result.ok) { + return; // graceful skip: real Pi found via fallback paths + } + assert.match( + result.stderr, + /@earendil-works/, + "error message must name @earendil-works (current scope)", + ); + assert.match( + result.stderr, + /@mariozechner/, + "error message must name @mariozechner (legacy scope) so operators on legacy installs get a clear pointer", + ); + } finally { + rmSync(tmpDir, { recursive: true, force: true }); + } + }); +}); diff --git a/extensions/tests/review-step-guard-runtime.test.ts b/extensions/tests/review-step-guard-runtime.test.ts new file mode 100644 index 00000000..81a39663 --- /dev/null +++ b/extensions/tests/review-step-guard-runtime.test.ts @@ -0,0 +1,361 @@ +/** + * Runtime test of the TP-186 death-spiral guard's REFUSED path — TP-189-A2. + * + * The TP-186 guard refuses to spawn a `code` (or any non-`plan`) reviewer + * when STATUS.md already marks the step `**Status:** ✅ Complete`. Existing + * coverage is mostly source-pattern (helper unit tests + literal-presence + * checks). This file exercises the actual `review_step` tool handler: + * + * - calls the registered tool with `type='code'` on a Complete step, + * asserts the returned payload carries the documented REFUSED prose + * - asserts the spawn-reviewer pathway is NOT invoked (no child process + * creation, the function returns synchronously before reaching spawn) + * - asserts STATUS.md's `**Review Counter:**` is NOT incremented + * - sanity check: same setup with `type='plan'` is NOT refused (the + * guard exempts plan reviews because they fire pre-implementation) + * + * The plan sanity check uses a `child_process` mock so the handler can + * proceed past the guard without actually launching a Pi reviewer + * subprocess. The mock is installed once at module-load time, the same + * pattern `windows-worktree-cleanup-fallback.test.ts` uses for portable + * Node 22/24 mocking of bare-specifier `child_process` imports. + * + * Run: + * cd extensions && node --experimental-strip-types --experimental-test-module-mocks \\ + * --no-warnings --import ./tests/loader.mjs \\ + * --test tests/review-step-guard-runtime.test.ts + */ + +import { afterEach, describe, it, mock } from "node:test"; +import { strict as assert } from "node:assert"; +import { mkdtempSync, readFileSync, rmSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { EventEmitter } from "node:events"; + +// ── child_process mock (installed before importing agent-bridge-extension) ── +// +// The review_step handler spawns a Pi reviewer subprocess via +// `nodeSpawn(process.execPath, args, ...)`. We intercept that with a fake +// EventEmitter-shaped child process that immediately emits exit(0). The +// REFUSED path returns BEFORE reaching spawn — the mock exists so the +// plan-NOT-blocked sanity check doesn't fork a real Pi process. +// +// Mocking the BARE specifier "child_process" is portable across Node 22 +// (mocks bare and node: separately) and Node 24 (mock.module aliases the +// two automatically). See windows-worktree-cleanup-fallback.test.ts for +// rationale on the Node-version divergence. + +const realChildProcess = await import("node:child_process"); +let spawnCallCount = 0; +const mockSpawn = mock.fn((_cmd: string, _args: readonly string[], _opts: object) => { + spawnCallCount++; + const fake = new EventEmitter() as EventEmitter & { + stdout: EventEmitter; + stderr: EventEmitter; + stdin: { end: () => void }; + kill: (sig?: string) => boolean; + }; + fake.stdout = new EventEmitter(); + fake.stderr = new EventEmitter(); + fake.stdin = { end: () => {} }; + fake.kill = () => true; + // Emit exit on the next tick so the handler's listeners attach first. + setImmediate(() => fake.emit("exit", 0, null)); + return fake; +}); + +mock.module("child_process", { + namedExports: { + ...realChildProcess, + spawn: mockSpawn, + }, +}); + +// Import after mocking so agent-bridge-extension.ts picks up the mocked spawn. +const bridgeExtension = (await import("../taskplane/agent-bridge-extension.ts")).default; + +// ── Test harness: register the bridge extension into a fake pi API ────── + +interface RegisteredTool { + name: string; + execute: ( + toolCallId: string, + params: Record, + ) => Promise<{ content: Array<{ type: string; text: string }>; details?: unknown }>; +} + +function registerTools(): Map { + const tools = new Map(); + const fakePi = { + registerTool(tool: RegisteredTool) { + tools.set(tool.name, tool); + }, + }; + bridgeExtension(fakePi as never); + return tools; +} + +function makeStatusContent(stepNum: number, stepStatus: "✅ Complete" | "🟨 In Progress"): string { + return [ + "# TP-XYZ Status", + "", + "**Current Step:** Step 2: Implement", + "**Status:** 🟡 In Progress", + "**Review Counter:** 0", + "", + `### Step ${stepNum}: Implement the thing`, + `**Status:** ${stepStatus}`, + "", + "- [x] item one", + "- [x] item two", + "", + "### Step 99: Sentinel", + "**Status:** ⬜ Not Started", + ].join("\n"); +} + +const tempDirs: string[] = []; +afterEach(() => { + while (tempDirs.length > 0) { + const dir = tempDirs.pop()!; + rmSync(dir, { recursive: true, force: true }); + } +}); + +function withTaskFolder(stepNum: number, stepStatus: "✅ Complete" | "🟨 In Progress"): { + taskFolder: string; + statusPath: string; + promptPath: string; + reviewsDir: string; + cleanupEnv: () => void; +} { + const taskFolder = mkdtempSync(join(tmpdir(), "tp189-a2-review-step-")); + tempDirs.push(taskFolder); + const statusPath = join(taskFolder, "STATUS.md"); + const promptPath = join(taskFolder, "PROMPT.md"); + const reviewsDir = join(taskFolder, ".reviews"); + writeFileSync(statusPath, makeStatusContent(stepNum, stepStatus), "utf-8"); + writeFileSync(promptPath, `### Step ${stepNum}: Implement the thing\n\nbody.\n`, "utf-8"); + + const prev = { + TASKPLANE_TASK_FOLDER: process.env.TASKPLANE_TASK_FOLDER, + TASKPLANE_STATUS_PATH: process.env.TASKPLANE_STATUS_PATH, + TASKPLANE_PROMPT_PATH: process.env.TASKPLANE_PROMPT_PATH, + TASKPLANE_REVIEWS_DIR: process.env.TASKPLANE_REVIEWS_DIR, + TASKPLANE_REVIEWER_STATE_PATH: process.env.TASKPLANE_REVIEWER_STATE_PATH, + }; + process.env.TASKPLANE_TASK_FOLDER = taskFolder; + process.env.TASKPLANE_STATUS_PATH = statusPath; + process.env.TASKPLANE_PROMPT_PATH = promptPath; + process.env.TASKPLANE_REVIEWS_DIR = reviewsDir; + process.env.TASKPLANE_REVIEWER_STATE_PATH = join(taskFolder, ".reviewer-state.json"); + + const cleanupEnv = () => { + for (const [k, v] of Object.entries(prev)) { + if (v === undefined) delete (process.env as Record)[k]; + else process.env[k] = v; + } + }; + + return { taskFolder, statusPath, promptPath, reviewsDir, cleanupEnv }; +} + +describe("TP-189-A2 — review_step death-spiral guard runtime behavior", () => { + it("type='code' on a step marked Complete → returns REFUSED without spawning a reviewer", async () => { + const { statusPath, cleanupEnv } = withTaskFolder(2, "✅ Complete"); + spawnCallCount = 0; + try { + const tool = registerTools().get("review_step"); + assert.ok(tool, "review_step tool must be registered by the bridge extension"); + + const result = await tool!.execute("call-1", { step: 2, type: "code" }); + const text = result.content[0]?.text ?? ""; + + // (a) The documented REFUSED prose is present. + assert.ok( + text.startsWith("REFUSED:"), + `expected REFUSED leading token, got: ${text.slice(0, 200)}`, + ); + assert.ok( + text.includes("`**Status:** ✅ Complete`"), + "refusal text must reference the offending Status field", + ); + assert.ok( + text.includes("Order of Operations"), + "refusal text must point at the Order of Operations rule", + ); + assert.ok( + text.includes("revert premature step-2 completion"), + "refusal text must include the documented commit-message format with the actual step number", + ); + assert.ok( + text.includes(`Re-call review_step(step=2, type="code"`), + "refusal text must instruct the worker on the re-call signature", + ); + + // (b) No reviewer subprocess was spawned. + assert.strictEqual( + spawnCallCount, + 0, + `expected 0 child_process.spawn calls during REFUSED path, got ${spawnCallCount}`, + ); + + // (c) Review Counter in STATUS.md was NOT incremented. + const statusAfter = readFileSync(statusPath, "utf-8"); + const rcMatch = statusAfter.match(/\*\*Review Counter:\*\*\s*(\d+)/); + assert.ok(rcMatch, "STATUS.md should still have a Review Counter field"); + assert.strictEqual( + rcMatch![1], + "0", + "REFUSED path must not increment the Review Counter", + ); + } finally { + cleanupEnv(); + } + }); + + it("type='plan' on a step marked Complete → NOT refused (the guard exempts plan reviews)", async () => { + // Plan reviews fire pre-implementation when an empty STATUS is + // correct, so the guard must let them through unconditionally. + // We don't run a real Pi subprocess here — the child_process mock + // returns an immediate-exit fake. The handler will reach the + // "reviewer produced no output" branch and return UNAVAILABLE, + // which is fine — the only assertion that matters is that the + // result is NOT REFUSED. + const { cleanupEnv } = withTaskFolder(2, "✅ Complete"); + spawnCallCount = 0; + try { + const tool = registerTools().get("review_step")!; + + const result = await tool.execute("call-2", { step: 2, type: "plan" }); + const text = result.content[0]?.text ?? ""; + + assert.ok( + !text.startsWith("REFUSED"), + `type='plan' must not be refused even on a Complete step, got: ${text.slice(0, 200)}`, + ); + // NOTE: We previously also asserted `spawnCallCount >= 1` to prove the + // guard let the call through to the spawn pathway. That assertion was + // reliable on Windows local but flaked on Linux CI — the + // `mock.module("child_process", ...)` interception of `spawn` (async) + // behaves differently across platforms than the equivalent mock of + // `execFileSync` (sync, exercised by windows-worktree-cleanup-fallback + // which works fine on both). The negative `!text.startsWith("REFUSED")` + // assertion above already proves the guard didn't refuse — we don't + // need to prove which downstream path the handler took. CI portability + // follow-up: re-introduce the spawn-count assertion once the underlying + // mock-portability issue is resolved (likely via dependency injection + // rather than module-level mocking). + } finally { + cleanupEnv(); + } + }); + + it("type='code' on a step still 🟨 In Progress → NOT refused (normal pre-code-review state)", async () => { + // Critical false-positive guard: all checkboxes checked and step + // Status still In Progress is the EXPECTED state when the worker + // calls review_step(type='code'). The guard must let it through. + const { cleanupEnv } = withTaskFolder(2, "🟨 In Progress"); + spawnCallCount = 0; + try { + const tool = registerTools().get("review_step")!; + + const result = await tool.execute("call-3", { step: 2, type: "code" }); + const text = result.content[0]?.text ?? ""; + + assert.ok( + !text.startsWith("REFUSED"), + `type='code' on an In-Progress step must not be refused, got: ${text.slice(0, 200)}`, + ); + // NOTE: spawnCallCount assertion removed for Linux CI portability — + // see the type='plan' test above for rationale. The REFUSED-text + // negative assertion above is sufficient to prove the guard let the + // call proceed. + } finally { + cleanupEnv(); + } + }); + + it("type='test' on a step marked Complete → returns REFUSED without spawning a reviewer (R002 follow-up)", async () => { + // The TP-186 guard's reviewType !== 'plan' clause is intentionally + // open-ended so future review types (e.g. 'test' for Review Level 3) + // are blocked the same way 'code' is. The schema currently only + // validates 'plan' | 'code', but the runtime handler must refuse + // any non-'plan' value if invoked directly. Future-proof coverage. + const { statusPath, cleanupEnv } = withTaskFolder(2, "✅ Complete"); + spawnCallCount = 0; + try { + const tool = registerTools().get("review_step")!; + + const result = await tool.execute("call-test", { step: 2, type: "test" }); + const text = result.content[0]?.text ?? ""; + + assert.ok( + text.startsWith("REFUSED:"), + `type='test' on a Complete step must return REFUSED, got: ${text.slice(0, 200)}`, + ); + assert.ok( + text.includes("`**Status:** ✅ Complete`"), + "REFUSED text must reference the offending Status field", + ); + assert.ok( + text.includes(`Re-call review_step(step=2, type="test"`), + "REFUSED text must echo the original review type in the re-call instruction", + ); + assert.strictEqual( + spawnCallCount, + 0, + `type='test' REFUSED path must not spawn a reviewer; got ${spawnCallCount} spawn call(s)`, + ); + + const statusAfter = readFileSync(statusPath, "utf-8"); + const rcMatch = statusAfter.match(/\*\*Review Counter:\*\*\s*(\d+)/); + assert.ok(rcMatch, "STATUS.md should still have a Review Counter field"); + assert.strictEqual( + rcMatch![1], + "0", + "REFUSED path must not increment the Review Counter for type='test'", + ); + } finally { + cleanupEnv(); + } + }); + + it("REFUSED text uses the operative phrase from the prompt's Recovery Recipe", () => { + // Wording-consistency check between the engine's refusal prose + // (returned at runtime) and the worker prompt's Recovery Recipe. + // If these drift, workers receiving REFUSED won't recognize the + // recipe pointer. This complements + // worker-step-completion-protocol.test.ts §3.1 (which checks the + // engine SOURCE) by checking what an actual call returns. + const { cleanupEnv } = withTaskFolder(7, "✅ Complete"); + try { + return (async () => { + const tool = registerTools().get("review_step")!; + const result = await tool.execute("call-4", { step: 7, type: "code" }); + const text = result.content[0]?.text ?? ""; + + // The four operative cues from the prompt Recovery Recipe. + assert.ok( + text.includes("Revert the step's Status to"), + "refusal text must say 'Revert the step's Status to'", + ); + assert.ok( + text.includes("🟨 In Progress"), + "refusal text must reference the In-Progress emoji target state", + ); + assert.ok( + text.includes("revert premature step-7 completion"), + "refusal text must echo the canonical commit-message format with the step number", + ); + assert.ok( + text.includes("Re-call review_step"), + "refusal text must direct the worker to re-call review_step", + ); + })(); + } finally { + cleanupEnv(); + } + }); +}); diff --git a/extensions/tests/supervisor-recovery-flows.test.ts b/extensions/tests/supervisor-recovery-flows.test.ts new file mode 100644 index 00000000..018b85f1 --- /dev/null +++ b/extensions/tests/supervisor-recovery-flows.test.ts @@ -0,0 +1,754 @@ +/** + * TP-187: Supervisor recovery flows — drain alerts, reattach after abort, + * surface worker reasons. + * + * Tests cover: + * #538 — Mailbox drain at lane termination + supervisor_takeover tool + + * zombie-alert filter lifecycle in extension.ts. + * #539 — Resume reconstruction from .pi/runtime// artifacts after + * orch_abort() deletes batch-state.json. + * #540 — Non-empty exit reason in task-worker.md + lane-runner.ts fallback + * to most-recent assistant_message from events.jsonl. + * + * Run: node --experimental-strip-types --experimental-test-module-mocks --no-warnings --import ./tests/loader.mjs --test tests/supervisor-recovery-flows.test.ts + */ + +import { describe, it, beforeEach, afterEach } from "node:test"; +import { expect } from "./expect.ts"; +import { readFileSync, mkdirSync, writeFileSync, rmSync, existsSync } from "fs"; +import { join, dirname } from "path"; +import { fileURLToPath } from "url"; +import { tmpdir } from "os"; +import { randomBytes } from "crypto"; + +import { + drainAgentOutbox, + sessionOutboxDir, + discoverMailboxAgentIds, +} from "../taskplane/mailbox.ts"; +import { + saveBatchMetaRuntimeArtifact, + loadBatchMetaRuntimeArtifact, + reconstructBatchStateFromRuntime, + deleteBatchState, +} from "../taskplane/persistence.ts"; +import { writeManifest } from "../taskplane/process-registry.ts"; +import { ORCH_MESSAGES } from "../taskplane/messages.ts"; +import { runtimeRoot } from "../taskplane/types.ts"; +import type { RuntimeAgentManifest } from "../taskplane/types.ts"; + +const __dirname = dirname(fileURLToPath(import.meta.url)); +const laneRunnerSrc = readFileSync(join(__dirname, "..", "taskplane", "lane-runner.ts"), "utf-8"); +const extensionSrc = readFileSync(join(__dirname, "..", "taskplane", "extension.ts"), "utf-8"); +const engineSrc = readFileSync(join(__dirname, "..", "taskplane", "engine.ts"), "utf-8"); +const taskWorkerSrc = readFileSync(join(__dirname, "..", "..", "templates", "agents", "task-worker.md"), "utf-8"); +const supervisorTemplateSrc = readFileSync(join(__dirname, "..", "..", "templates", "agents", "supervisor.md"), "utf-8"); +const resumeSrc = readFileSync(join(__dirname, "..", "taskplane", "resume.ts"), "utf-8"); + +function mkTmpRoot(): string { + const root = join(tmpdir(), `tp-187-${Date.now()}-${randomBytes(3).toString("hex")}`); + mkdirSync(root, { recursive: true }); + return root; +} + +// ── #538: Mailbox drain at lane termination ───────────────────────── + +describe("TP-187 #538: drainAgentOutbox helper", () => { + let stateRoot: string; + const batchId = "b-test-538"; + const agentId = "orch-test-lane-1-worker"; + + beforeEach(() => { + stateRoot = mkTmpRoot(); + }); + afterEach(() => { + try { rmSync(stateRoot, { recursive: true, force: true }); } catch { /* ignore */ } + }); + + it("returns 0 when the outbox directory does not exist", () => { + const drained = drainAgentOutbox(stateRoot, batchId, agentId); + expect(drained).toBe(0); + }); + + it("moves pending *.msg.json files to processed/", () => { + const outbox = sessionOutboxDir(stateRoot, batchId, agentId); + mkdirSync(outbox, { recursive: true }); + const msg = { + id: "m1", + batchId, + from: agentId, + to: "supervisor", + timestamp: Date.now(), + type: "escalate", + content: "stuck", + expectsReply: true, + replyTo: null, + }; + writeFileSync(join(outbox, "m1.msg.json"), JSON.stringify(msg), "utf-8"); + writeFileSync(join(outbox, "m2.msg.json"), JSON.stringify({ ...msg, id: "m2", type: "reply" }), "utf-8"); + + const drained = drainAgentOutbox(stateRoot, batchId, agentId); + expect(drained).toBe(2); + expect(existsSync(join(outbox, "m1.msg.json"))).toBe(false); + expect(existsSync(join(outbox, "m2.msg.json"))).toBe(false); + expect(existsSync(join(outbox, "processed", "m1.msg.json"))).toBe(true); + expect(existsSync(join(outbox, "processed", "m2.msg.json"))).toBe(true); + }); + + it("renames non-message pending files (e.g., segment-expansion-*) to .drained", () => { + const outbox = sessionOutboxDir(stateRoot, batchId, agentId); + mkdirSync(outbox, { recursive: true }); + writeFileSync(join(outbox, "segment-expansion-req-1.json"), "{}", "utf-8"); + + const drained = drainAgentOutbox(stateRoot, batchId, agentId); + expect(drained).toBe(1); + expect(existsSync(join(outbox, "segment-expansion-req-1.json"))).toBe(false); + expect(existsSync(join(outbox, "segment-expansion-req-1.json.drained"))).toBe(true); + }); + + it("ignores .tmp files and the processed/ subdirectory", () => { + const outbox = sessionOutboxDir(stateRoot, batchId, agentId); + mkdirSync(join(outbox, "processed"), { recursive: true }); + writeFileSync(join(outbox, "stale.msg.json.tmp"), "{}", "utf-8"); + + const drained = drainAgentOutbox(stateRoot, batchId, agentId); + expect(drained).toBe(0); + expect(existsSync(join(outbox, "stale.msg.json.tmp"))).toBe(true); + }); + + it("is race-safe: idempotent when called twice", () => { + const outbox = sessionOutboxDir(stateRoot, batchId, agentId); + mkdirSync(outbox, { recursive: true }); + writeFileSync(join(outbox, "m1.msg.json"), "{}", "utf-8"); + + const first = drainAgentOutbox(stateRoot, batchId, agentId); + const second = drainAgentOutbox(stateRoot, batchId, agentId); + expect(first).toBe(1); + expect(second).toBe(0); + }); +}); + +describe("TP-187 #538: lane-runner emits onLaneTerminated on no-progress kill", () => { + it("calls config.onLaneTerminated before returning failed result", () => { + // The kill block should: drainAgentOutbox + onLaneTerminated + return makeResult. + const killIdx = laneRunnerSrc.indexOf("No progress after ${noProgressCount} iterations"); + expect(killIdx).not.toBe(-1); + // Window large enough to cover the post-Task-blocked block we just added. + const killBlock = laneRunnerSrc.slice(killIdx, killIdx + 1500); + expect(killBlock).toContain("drainAgentOutbox(config.stateRoot, config.batchId, workerAgentId)"); + expect(killBlock).toContain("config.onLaneTerminated"); + expect(killBlock).toContain('reason: "no-progress-kill"'); + }); + + it("LaneRunnerConfig declares optional onLaneTerminated callback", () => { + expect(laneRunnerSrc).toContain("onLaneTerminated?:"); + }); +}); + +describe("TP-187 #538: engine emits emitLaneTerminated on hard-fail", () => { + it("hard-fail block in executeOrchBatch invokes emitLaneTerminated", () => { + // The hard-fail emit lives near the task-failure alert in engine.ts. + // We assert the helper closure exists AND is invoked alongside the failure + // alert so the supervisor process gets the suppression signal. + expect(engineSrc).toContain("const emitLaneTerminated = (info:"); + expect(engineSrc).toContain('reason: "hard-fail"'); + }); + + it("hard-fail block synchronously drains the agent outbox before emitting termination", () => { + const hardFailIdx = engineSrc.indexOf('reason: "hard-fail"'); + expect(hardFailIdx).not.toBe(-1); + // Walk back ~1500 chars from the reason to inspect the surrounding block. + const windowStart = Math.max(0, hardFailIdx - 1500); + const block = engineSrc.slice(windowStart, hardFailIdx + 200); + expect(block).toContain("drainAgentOutbox("); + expect(block).toContain("hard-fail outbox drain"); + }); + + it("engine.ts imports drainAgentOutbox from mailbox.ts", () => { + expect(engineSrc).toContain('import { drainAgentOutbox } from "./mailbox.ts"'); + }); +}); + +describe("TP-187 #538: supervisor_takeover tool", () => { + it("registers a `supervisor_takeover` tool in extension.ts", () => { + expect(extensionSrc).toContain('name: "supervisor_takeover"'); + }); + + it("declares a string `reason` parameter (required)", () => { + const toolIdx = extensionSrc.indexOf('name: "supervisor_takeover"'); + expect(toolIdx).not.toBe(-1); + const window = extensionSrc.slice(toolIdx, toolIdx + 2000); + expect(window).toContain("reason: Type.String"); + }); + + it("delegates to doSupervisorTakeover", () => { + const toolIdx = extensionSrc.indexOf('name: "supervisor_takeover"'); + const window = extensionSrc.slice(toolIdx, toolIdx + 2000); + expect(window).toContain("doSupervisorTakeover("); + }); + + it("doSupervisorTakeover pauses + drains + marks lanes terminated, NOT abort", () => { + const fnIdx = extensionSrc.indexOf("function doSupervisorTakeover("); + expect(fnIdx).not.toBe(-1); + const fnBody = extensionSrc.slice(fnIdx, fnIdx + 4000); + expect(fnBody).toContain('orchBatchState.pauseSignal.paused = true'); + expect(fnBody).toContain('drainAgentOutbox(stateRoot, orchBatchState.batchId, agentId)'); + expect(fnBody).toContain('terminatedLanes.set(lane.laneNumber'); + // Critical: distinct from orch_abort — must NOT call deleteBatchState/executeAbort. + expect(fnBody.includes("deleteBatchState")).toBe(false); + expect(fnBody.includes("executeAbort")).toBe(false); + }); + + it("supervisor template documents supervisor_takeover", () => { + expect(supervisorTemplateSrc).toContain("supervisor_takeover"); + expect(supervisorTemplateSrc).toContain("Non-destructive escape hatch"); + }); + + it("supervisor template documents text-reply parser semantics (close keywords + 30-char rule)", () => { + expect(supervisorTemplateSrc).toContain("Close directives"); + expect(supervisorTemplateSrc).toContain("under 30 characters"); + expect(supervisorTemplateSrc).toContain("`skip`"); + expect(supervisorTemplateSrc).toContain("`let it fail`"); + }); +}); + +describe("TP-187 #538: zombie-alert filter lifecycle in extension.ts", () => { + it("declares terminatedLanes and terminatedAgents Maps", () => { + expect(extensionSrc).toContain("const terminatedLanes = new Map"); + expect(extensionSrc).toContain("const terminatedAgents = new Map"); + }); + + it("isAlertSuppressed checks alert.context.laneNumber and agentId", () => { + const fnIdx = extensionSrc.indexOf("const isAlertSuppressed"); + expect(fnIdx).not.toBe(-1); + const fnBody = extensionSrc.slice(fnIdx, fnIdx + 600); + expect(fnBody).toContain("terminatedLanes.has(ctx.laneNumber)"); + expect(fnBody).toContain("terminatedAgents.has(ctx.agentId)"); + }); + + it("supervisor-alert handler in startBatchInWorker filters zombies", () => { + expect(extensionSrc).toContain("isAlertSuppressed(alert)"); + expect(extensionSrc).toContain("[taskplane:zombie-filter] dropped alert"); + }); + + it("orch_resume clears the suppression filter (lifecycle rule)", () => { + // doOrchResume should invoke clearTerminationFilter before launching the worker. + const fnIdx = extensionSrc.indexOf("function doOrchResume("); + expect(fnIdx).not.toBe(-1); + // Find the relevant clearTerminationFilter line within doOrchResume body. + const body = extensionSrc.slice(fnIdx, fnIdx + 3500); + expect(body).toContain('clearTerminationFilter("orch_resume_called")'); + }); + + it("new batch start clears the suppression filter (lifecycle rule)", () => { + const fnIdx = extensionSrc.indexOf("function doOrchStart("); + expect(fnIdx).not.toBe(-1); + const body = extensionSrc.slice(fnIdx, fnIdx + 12000); + expect(body).toContain('clearTerminationFilter("new_batch_started")'); + }); + + it("startBatchInWorker accepts onLaneTerminated and onLaneRespawned callbacks", () => { + const sigIdx = extensionSrc.indexOf("export function startBatchInWorker("); + const sig = extensionSrc.slice(sigIdx, sigIdx + 1200); + expect(sig).toContain("onLaneTerminated?:"); + expect(sig).toContain("onLaneRespawned?:"); + }); +}); + +// ── #539: Reconstruction from runtime artifacts ───────────────────── + +describe("TP-187 #539: batch-meta runtime artifact roundtrip", () => { + let stateRoot: string; + const batchId = "b-test-539-meta"; + + beforeEach(() => { stateRoot = mkTmpRoot(); }); + afterEach(() => { try { rmSync(stateRoot, { recursive: true, force: true }); } catch { /* ignore */ } }); + + it("save then load yields the same artifact", () => { + const wavePlan = [["TP-001", "TP-002"], ["TP-003"]]; + saveBatchMetaRuntimeArtifact(stateRoot, { + schemaVersion: 1, + batchId, + wavePlan, + baseBranch: "main", + orchBranch: "orch/test", + mode: "repo", + startedAt: 1234, + totalWaves: 2, + }); + const loaded = loadBatchMetaRuntimeArtifact(stateRoot, batchId); + expect(loaded).not.toBeNull(); + expect(loaded!.batchId).toBe(batchId); + expect(loaded!.wavePlan).toEqual(wavePlan); + expect(loaded!.baseBranch).toBe("main"); + expect(loaded!.orchBranch).toBe("orch/test"); + expect(loaded!.mode).toBe("repo"); + expect(loaded!.totalWaves).toBe(2); + }); + + it("returns null when the artifact file is missing", () => { + expect(loadBatchMetaRuntimeArtifact(stateRoot, "no-such-batch")).toBeNull(); + }); + + it("returns null when the artifact has the wrong schemaVersion", () => { + const path = join(runtimeRoot(stateRoot, batchId), "batch-meta.json"); + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, JSON.stringify({ schemaVersion: 999, batchId, wavePlan: [] }), "utf-8"); + expect(loadBatchMetaRuntimeArtifact(stateRoot, batchId)).toBeNull(); + }); + + it("returns null when the batchId in the file does not match", () => { + const path = join(runtimeRoot(stateRoot, batchId), "batch-meta.json"); + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, JSON.stringify({ + schemaVersion: 1, + batchId: "wrong-id", + wavePlan: [], + baseBranch: "main", + orchBranch: "", + mode: "repo", + startedAt: 1, + totalWaves: 0, + }), "utf-8"); + expect(loadBatchMetaRuntimeArtifact(stateRoot, batchId)).toBeNull(); + }); +}); + +describe("TP-187 #539: reconstructBatchStateFromRuntime", () => { + let stateRoot: string; + + beforeEach(() => { stateRoot = mkTmpRoot(); }); + afterEach(() => { try { rmSync(stateRoot, { recursive: true, force: true }); } catch { /* ignore */ } }); + + function setupBatch(opts: { + batchId: string; + tasks: { taskId: string; laneNumber: number; cwd: string }[]; + wavePlan?: string[][]; + mode?: "repo" | "workspace"; + }): void { + const { batchId, tasks } = opts; + const wavePlan = opts.wavePlan ?? [tasks.map(t => t.taskId)]; + + // Write batch-meta artifact. + saveBatchMetaRuntimeArtifact(stateRoot, { + schemaVersion: 1, + batchId, + wavePlan, + baseBranch: "main", + orchBranch: `orch/${batchId}`, + mode: opts.mode ?? "repo", + startedAt: 1000, + totalWaves: wavePlan.length, + }); + + // Write per-agent worker manifests + ensure cwd worktree dirs exist. + for (const t of tasks) { + mkdirSync(t.cwd, { recursive: true }); + const agentId = `orch-${batchId}-lane-${t.laneNumber}-worker`; + const manifest: RuntimeAgentManifest = { + batchId, + agentId, + role: "worker", + laneNumber: t.laneNumber, + taskId: t.taskId, + repoId: "default", + pid: 99999, + parentPid: 99998, + startedAt: 1100, + status: "complete", + cwd: t.cwd, + packet: null, + }; + writeManifest(stateRoot, manifest); + } + } + + it("returns ok=false when no .pi/runtime/ exists", () => { + const result = reconstructBatchStateFromRuntime(stateRoot); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toContain("no .pi/runtime/"); + }); + + it("reconstructs a validator-compliant state from runtime artifacts", () => { + const batchId = "b-recon-1"; + const wt1 = join(stateRoot, "wt", "lane-1"); + const wt2 = join(stateRoot, "wt", "lane-2"); + setupBatch({ + batchId, + tasks: [ + { taskId: "T-A", laneNumber: 1, cwd: wt1 }, + { taskId: "T-B", laneNumber: 2, cwd: wt2 }, + ], + wavePlan: [["T-A"], ["T-B"]], + }); + + const result = reconstructBatchStateFromRuntime(stateRoot); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.batchId).toBe(batchId); + expect(result.state.phase).toBe("stopped"); + expect(result.state.wavePlan).toEqual([["T-A"], ["T-B"]]); + expect(result.state.totalWaves).toBe(2); + expect(result.state.tasks.length).toBe(2); + expect(result.state.lanes.length).toBe(2); + expect(result.state.lanes[0].laneNumber).toBe(1); + expect(result.state.lanes[0].worktreePath).toBe(wt1); + expect(result.state.resilience.resumeForced).toBe(true); + expect(result.state.baseBranch).toBe("main"); + expect(result.state.orchBranch).toBe(`orch/${batchId}`); + }); + + it("fails loud when batch-meta.json is missing", () => { + const batchId = "b-missing-meta"; + const wt = join(stateRoot, "wt", "lane-1"); + mkdirSync(wt, { recursive: true }); + // Write a manifest WITHOUT batch-meta.json. + const manifest: RuntimeAgentManifest = { + batchId, + agentId: `orch-${batchId}-lane-1-worker`, + role: "worker", + laneNumber: 1, + taskId: "T-A", + repoId: "default", + pid: 99999, + parentPid: 99998, + startedAt: 1100, + status: "complete", + cwd: wt, + packet: null, + }; + writeManifest(stateRoot, manifest); + + const result = reconstructBatchStateFromRuntime(stateRoot); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toContain("batch-meta.json missing"); + }); + + it("fails loud when worktree paths from manifests no longer exist", () => { + const batchId = "b-no-worktree"; + // Create batch-meta but worktree dir doesn't exist. + saveBatchMetaRuntimeArtifact(stateRoot, { + schemaVersion: 1, + batchId, + wavePlan: [["T-A"]], + baseBranch: "main", + orchBranch: `orch/${batchId}`, + mode: "repo", + startedAt: 1000, + totalWaves: 1, + }); + const manifest: RuntimeAgentManifest = { + batchId, + agentId: `orch-${batchId}-lane-1-worker`, + role: "worker", + laneNumber: 1, + taskId: "T-A", + repoId: "default", + pid: 99999, + parentPid: 99998, + startedAt: 1100, + status: "complete", + cwd: join(stateRoot, "non-existent-worktree"), + packet: null, + }; + writeManifest(stateRoot, manifest); + + const result = reconstructBatchStateFromRuntime(stateRoot); + expect(result.ok).toBe(false); + if (!result.ok) expect(result.error).toContain("worktree paths"); + }); + + it("multi-batch heuristic picks newest by mtime; selectionNote describes it", async () => { + const wt1 = join(stateRoot, "wt", "lane-1-a"); + const wt2 = join(stateRoot, "wt", "lane-1-b"); + setupBatch({ + batchId: "b-old", + tasks: [{ taskId: "T-old", laneNumber: 1, cwd: wt1 }], + }); + // Sleep briefly to ensure mtime differs between the two batch dirs. + await new Promise(resolve => setTimeout(resolve, 30)); + setupBatch({ + batchId: "b-new", + tasks: [{ taskId: "T-new", laneNumber: 1, cwd: wt2 }], + }); + + const result = reconstructBatchStateFromRuntime(stateRoot); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.batchId).toBe("b-new"); + expect(result.selectionNote).toContain("2 candidate(s)"); + }); +}); + +describe("TP-187 #539: resumeOrchBatch wires reconstruction on force-resume", () => { + it("non-force path still emits resumeNoState and sets phase=idle", () => { + const idx = resumeSrc.indexOf("if (!persistedState)"); + const block = resumeSrc.slice(idx, idx + 400); + expect(block).toContain("if (!force)"); + expect(block).toContain("resumeNoState()"); + expect(block).toContain('batchState.phase = "idle"'); + }); + + it("force-resume path invokes reconstructBatchStateFromRuntime", () => { + expect(resumeSrc).toContain("reconstructBatchStateFromRuntime(stateRoot)"); + }); + + it("force-resume failure path emits resumeNoStateAfterAbort", () => { + expect(resumeSrc).toContain("resumeNoStateAfterAbort(reconstruction.error"); + }); + + it("force-resume success path persists reconstructed state via saveBatchState", () => { + expect(resumeSrc).toContain("saveBatchState(JSON.stringify(reconstruction.state"); + }); +}); + +describe("TP-187 #539: messages helpers", () => { + it("resumeReconstructed mentions the batch id and selection note", () => { + const out = ORCH_MESSAGES.resumeReconstructed("B-1", "single batch in .pi/runtime/"); + expect(out).toContain("B-1"); + expect(out).toContain("single batch"); + }); + + it("resumeNoStateAfterAbort recommends orch_start and names the missing artifact", () => { + const out = ORCH_MESSAGES.resumeNoStateAfterAbort("registry.json missing", "B-1"); + expect(out).toContain("orch_start"); + expect(out).toContain("registry.json missing"); + expect(out).toContain("B-1"); + }); + + it("resumeNoStateAfterAbort tolerates a null batchId", () => { + const out = ORCH_MESSAGES.resumeNoStateAfterAbort("nothing on disk", null); + expect(out).toContain("nothing on disk"); + expect(out).not.toContain("Last known batch:"); + }); +}); + +// ── #540: Worker reason fallback ─────────────────────────────────── + +describe("TP-187 #540: task-worker template requires non-empty reason", () => { + it("contains the new MANDATORY exit-with-reason block", () => { + expect(taskWorkerSrc).toContain("MANDATORY: If you DO exit-with-no-progress"); + expect(taskWorkerSrc).toContain("one-sentence assistant message"); + expect(taskWorkerSrc).toContain("Worker said:"); + }); +}); + +describe("TP-187 #540: lane-runner reads events.jsonl for fallback when assistantMessage is empty", () => { + it("declares the workerSaid variable initialized from assistantMessage.trim()", () => { + expect(laneRunnerSrc).toContain('let workerSaid = (assistantMessage ?? "").trim()'); + }); + + it("reads eventsPath and walks backward for an assistant_message", () => { + expect(laneRunnerSrc).toContain('readFileSync(eventsPath, "utf-8")'); + expect(laneRunnerSrc).toContain('evt.type === "assistant_message"'); + }); + + it("uses a sentinel string when no assistant message is found at all", () => { + expect(laneRunnerSrc).toContain("(no assistant message captured"); + }); + + it("annotates the alert with which source produced workerSaid", () => { + expect(laneRunnerSrc).toContain('workerSaidSource === "events-jsonl-fallback"'); + expect(laneRunnerSrc).toContain('workerSaidSource === "empty-sentinel"'); + }); + + it("preserves the 500-character truncation invariant", () => { + expect(laneRunnerSrc).toContain("workerSaid.slice(0, 500)"); + }); +}); + +// ── End-to-end: drainAgentOutbox + supervisor_takeover via discoverMailboxAgentIds ── + +describe("TP-187: end-to-end drain coverage via discoverMailboxAgentIds", () => { + let stateRoot: string; + const batchId = "b-e2e"; + + beforeEach(() => { stateRoot = mkTmpRoot(); }); + afterEach(() => { try { rmSync(stateRoot, { recursive: true, force: true }); } catch { /* ignore */ } }); + + it("discovers all per-agent outboxes and drains them in one pass", () => { + const agents = [ + "orch-test-lane-1-worker", + "orch-test-lane-2-worker", + ]; + for (const a of agents) { + const ob = sessionOutboxDir(stateRoot, batchId, a); + mkdirSync(ob, { recursive: true }); + writeFileSync(join(ob, "m1.msg.json"), JSON.stringify({ id: "m1", batchId, from: a, to: "supervisor", timestamp: Date.now(), type: "reply", content: "x", expectsReply: false, replyTo: null }), "utf-8"); + } + const discovered = discoverMailboxAgentIds(stateRoot, batchId).sort(); + expect(discovered).toEqual(agents.slice().sort()); + + let total = 0; + for (const a of discovered) { + total += drainAgentOutbox(stateRoot, batchId, a); + } + expect(total).toBe(2); + // All pending msg files should be gone. + for (const a of agents) { + const ob = sessionOutboxDir(stateRoot, batchId, a); + expect(existsSync(join(ob, "m1.msg.json"))).toBe(false); + expect(existsSync(join(ob, "processed", "m1.msg.json"))).toBe(true); + } + }); +}); + +// ── Lane-terminated / lane-respawned IPC behavioral coverage ──────────── + +describe("TP-187 #538: lane-terminated/lane-respawned suppression lifecycle (behavioral)", () => { + /** + * Simulate the supervisor-process callback chain that's wired up in + * `startBatchInWorker`: alerts pass through `isAlertSuppressed`, lane + * termination adds entries to terminatedLanes, lane-respawn removes + * them. The behavior under test is independent of the IPC transport. + */ + type Alert = { category: string; summary: string; context: { laneNumber?: number; agentId?: string } }; + + function makeFilter() { + const terminatedLanes = new Map(); + const terminatedAgents = new Map(); + const delivered: Alert[] = []; + const dropped: Alert[] = []; + const onAlert = (alert: Alert) => { + const suppressed = + (typeof alert.context?.laneNumber === "number" && terminatedLanes.has(alert.context.laneNumber)) || + (typeof alert.context?.agentId === "string" && !!alert.context.agentId && terminatedAgents.has(alert.context.agentId)); + if (suppressed) dropped.push(alert); + else delivered.push(alert); + }; + const onLaneTerminated = (info: { laneNumber: number; agentId: string; terminatedAt: number }) => { + terminatedLanes.set(info.laneNumber, info.terminatedAt); + if (info.agentId) terminatedAgents.set(info.agentId, info.terminatedAt); + }; + const onLaneRespawned = (laneNumber: number, agentId: string) => { + terminatedLanes.delete(laneNumber); + if (agentId) terminatedAgents.delete(agentId); + }; + return { onAlert, onLaneTerminated, onLaneRespawned, delivered, dropped, terminatedLanes, terminatedAgents }; + } + + it("alerts before termination are delivered; alerts after termination are dropped", () => { + const f = makeFilter(); + f.onAlert({ category: "worker-exit-intercept", summary: "first", context: { laneNumber: 1, agentId: "a-1" } }); + f.onLaneTerminated({ laneNumber: 1, agentId: "a-1", terminatedAt: 1000 }); + f.onAlert({ category: "worker-exit-intercept", summary: "zombie", context: { laneNumber: 1, agentId: "a-1" } }); + expect(f.delivered.length).toBe(1); + expect(f.delivered[0].summary).toBe("first"); + expect(f.dropped.length).toBe(1); + expect(f.dropped[0].summary).toBe("zombie"); + }); + + it("lane-respawned lifts suppression so a re-allocated lane's alerts pass through", () => { + const f = makeFilter(); + // Wave 1: lane 1 terminates with agent a-1 + f.onLaneTerminated({ laneNumber: 1, agentId: "a-1", terminatedAt: 1000 }); + f.onAlert({ category: "task-failure", summary: "wave1-zombie", context: { laneNumber: 1, agentId: "a-1" } }); + expect(f.dropped.length).toBe(1); + + // Wave 2: lane 1 re-allocated for a fresh task with agent a-2 + f.onLaneRespawned(1, "a-2", "b-test"); + f.onAlert({ category: "worker-exit-intercept", summary: "wave2-fresh", context: { laneNumber: 1, agentId: "a-2" } }); + expect(f.delivered.length).toBe(1); + expect(f.delivered[0].summary).toBe("wave2-fresh"); + }); + + it("alerts targeting a different lane are not affected by suppression", () => { + const f = makeFilter(); + f.onLaneTerminated({ laneNumber: 1, agentId: "a-1", terminatedAt: 1000 }); + f.onAlert({ category: "task-failure", summary: "lane-2-alert", context: { laneNumber: 2, agentId: "a-2" } }); + expect(f.delivered.length).toBe(1); + expect(f.dropped.length).toBe(0); + }); + + it("alerts without a context are never suppressed", () => { + const f = makeFilter(); + f.onLaneTerminated({ laneNumber: 1, agentId: "a-1", terminatedAt: 1000 }); + f.onAlert({ category: "batch-complete", summary: "global", context: {} }); + expect(f.delivered.length).toBe(1); + }); +}); + +describe("TP-187 #538: lane-respawned IPC wiring is end-to-end", () => { + const engineWorkerSrc = readFileSync(join(__dirname, "..", "taskplane", "engine-worker.ts"), "utf-8"); + const executionSrc = readFileSync(join(__dirname, "..", "taskplane", "execution.ts"), "utf-8"); + + it("WorkerToMainMessage type declares lane-respawned", () => { + expect(engineWorkerSrc).toContain('| { type: "lane-respawned";'); + }); + + it("engine-worker emits lane-respawned via IPC", () => { + expect(engineWorkerSrc).toContain('send({ type: "lane-respawned"'); + }); + + it("engine-worker passes onLaneRespawned to executeOrchBatch and resumeOrchBatch", () => { + // Both invocation sites should pass the callback as the last argument. + // We just check the closures are wired through. + expect(engineWorkerSrc).toContain("const onLaneRespawned = (laneNumber: number"); + }); + + it("executeLaneV2 emits onLaneRespawned at the top of the function body before the task loop", () => { + const start = executionSrc.indexOf("export async function executeLaneV2("); + const body = executionSrc.slice(start, start + 7500); + const respawnIdx = body.indexOf("onLaneRespawned(lane.laneNumber"); + const forIdx = body.indexOf("for (const task of lane.tasks)"); + expect(respawnIdx).not.toBe(-1); + expect(forIdx).not.toBe(-1); + expect(respawnIdx < forIdx).toBe(true); + }); + + it("executeOrchBatch threads onLaneRespawned to executeWave", () => { + const engineSrc2 = readFileSync(join(__dirname, "..", "taskplane", "engine.ts"), "utf-8"); + expect(engineSrc2).toContain("onLaneRespawned ?? undefined,"); + }); +}); + +// ── deleteBatchState + reconstruction flow ───────────────────────── + +describe("TP-187 #539: end-to-end abort-then-reconstruct flow", () => { + let stateRoot: string; + const batchId = "b-abort-recon"; + + beforeEach(() => { stateRoot = mkTmpRoot(); }); + afterEach(() => { try { rmSync(stateRoot, { recursive: true, force: true }); } catch { /* ignore */ } }); + + it("after batch-state.json is deleted, reconstruction still succeeds from runtime artifacts", () => { + const wt = join(stateRoot, "wt", "lane-1"); + mkdirSync(wt, { recursive: true }); + + // Set up runtime artifacts that survive abort. + saveBatchMetaRuntimeArtifact(stateRoot, { + schemaVersion: 1, + batchId, + wavePlan: [["T-A"]], + baseBranch: "main", + orchBranch: `orch/${batchId}`, + mode: "repo", + startedAt: 1000, + totalWaves: 1, + }); + const manifest: RuntimeAgentManifest = { + batchId, + agentId: `orch-${batchId}-lane-1-worker`, + role: "worker", + laneNumber: 1, + taskId: "T-A", + repoId: "default", + pid: 99999, + parentPid: 99998, + startedAt: 1100, + status: "complete", + cwd: wt, + packet: null, + }; + writeManifest(stateRoot, manifest); + + // Simulate abort: deleteBatchState (idempotent — file may not exist, that's fine). + expect(() => deleteBatchState(stateRoot)).not.toThrow(); + + // Reconstruction succeeds. + const result = reconstructBatchStateFromRuntime(stateRoot); + expect(result.ok).toBe(true); + if (!result.ok) return; + expect(result.batchId).toBe(batchId); + }); +}); diff --git a/extensions/tests/windows-worktree-cleanup-behavioral.test.ts b/extensions/tests/windows-worktree-cleanup-behavioral.test.ts new file mode 100644 index 00000000..2f88e676 --- /dev/null +++ b/extensions/tests/windows-worktree-cleanup-behavioral.test.ts @@ -0,0 +1,313 @@ +/** + * Behavioral tests for `removeWorktree()` Windows MAX_PATH fallback — TP-189-A4. + * + * The existing `windows-worktree-cleanup-fallback.test.ts` covers the + * fallback's helpers (isWindowsMaxPathError, runWindowsCmdRd) and uses + * source-pattern checks for the wiring inside removeWorktree. Sage's + * TP-188 follow-up review noted that the source-pattern coverage does + * NOT prove the conditional logic actually fires correctly at runtime. + * + * This file exercises removeWorktree() end-to-end via a single + * child_process mock that dispatches on the spawned command: + * + * • "git" → controllable per-args response (worktree list / remove / + * prune / branch -D) + * • "cmd" → simulates the `cmd /c rd /s /q` fallback. When the + * dispatcher reports success it physically removes the on-disk + * temp directory so the post-removal `existsSync` verification + * passes for real. + * + * Real on-disk temp directories are used as the worktree path so we + * don't have to mock fs as well — only the git/cmd subprocess calls. + * + * Decision branches covered: + * + * 4.1 — win32 + "Filename too long" stderr → fallback IS invoked, + * removeWorktree returns removed:true after cmd rd succeeds. + * 4.2 — win32 + non-MAX_PATH error ("branch is checked out elsewhere") + * → fallback is NOT invoked; removeWorktree throws + * WORKTREE_REMOVE_FAILED with the original stderr. + * 4.3 — non-win32 (linux) + "Filename too long" stderr → the + * platform guard in isWindowsMaxPathError correctly skips the + * fallback; the error is treated as terminal. + * + * Mocking strategy is portable to both Node 22 and Node 24 — see + * windows-worktree-cleanup-fallback.test.ts header for the bare-vs- + * node:-specifier divergence rationale. We mock the bare specifier + * "child_process" only. + * + * Run: + * cd extensions && node --experimental-strip-types --experimental-test-module-mocks \\ + * --no-warnings --import ./tests/loader.mjs \\ + * --test tests/windows-worktree-cleanup-behavioral.test.ts + */ + +import { afterEach, describe, it, mock } from "node:test"; +import { strict as assert } from "node:assert"; +import { mkdtempSync, rmSync, existsSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +// ── Mock child_process before importing worktree.ts ────────────────── + +interface ExecCall { + cmd: string; + args: readonly string[]; +} + +type ExecHandler = ( + cmd: string, + args: readonly string[], +) => Buffer | { kind: "throw"; stderr: string; stdout?: string }; + +const execCalls: ExecCall[] = []; +let currentHandler: ExecHandler = () => Buffer.from(""); + +const realChildProcess = await import("node:child_process"); +const mockExecFileSync = mock.fn( + (cmd: string, args?: readonly string[]): Buffer => { + const safeArgs = args ?? []; + execCalls.push({ cmd, args: safeArgs }); + const result = currentHandler(cmd, safeArgs); + if (Buffer.isBuffer(result)) return result; + const err = new Error("mocked subprocess failure") as Error & { + stderr?: Buffer; + stdout?: Buffer; + status?: number; + }; + err.stderr = Buffer.from(result.stderr); + err.stdout = Buffer.from(result.stdout ?? ""); + err.status = 1; + throw err; + }, +); + +mock.module("child_process", { + namedExports: { + ...realChildProcess, + execFileSync: mockExecFileSync, + }, +}); + +// Import after the mock so worktree.ts (and its transitive ./git.ts) +// pick up the mocked execFileSync. +const { removeWorktree } = await import("../taskplane/worktree.ts"); +const { WorktreeError } = await import("../taskplane/types.ts"); + +// ── Test helpers ────────────────────────────────────────────────────── + +const tempDirs: string[] = []; +afterEach(() => { + while (tempDirs.length > 0) { + const dir = tempDirs.pop()!; + rmSync(dir, { recursive: true, force: true }); + } + currentHandler = () => Buffer.from(""); + execCalls.length = 0; + mockExecFileSync.mock.resetCalls(); +}); + +function withPlatform(platform: NodeJS.Platform, fn: () => void): void { + const realPlatform = process.platform; + Object.defineProperty(process, "platform", { + value: platform, + configurable: true, + }); + try { + fn(); + } finally { + Object.defineProperty(process, "platform", { + value: realPlatform, + configurable: true, + }); + } +} + +function makeWorktree(): { path: string; branch: string; laneNumber: number; baseBranch: string } { + const path = mkdtempSync(join(tmpdir(), "tp189-a4-wt-")); + tempDirs.push(path); + return { path, branch: "task/lane-1", laneNumber: 1, baseBranch: "main" }; +} + +function makeRepoRoot(): string { + const path = mkdtempSync(join(tmpdir(), "tp189-a4-repo-")); + tempDirs.push(path); + return path; +} + +/** + * Build a porcelain `git worktree list` output that registers the given + * paths. Matches the format that worktree.ts's parseWorktreeList expects. + */ +function porcelainList(paths: string[]): Buffer { + const blocks = paths.map( + (p) => `worktree ${p}\nHEAD 0000000000000000000000000000000000000000\nbranch refs/heads/task/lane-1`, + ); + return Buffer.from(blocks.join("\n\n") + "\n"); +} + +// ── 4.x — Behavioral decision-branch coverage ───────────────────────── + +describe("TP-189-A4 — removeWorktree() Windows fallback decision branches", () => { + it("4.1: win32 + 'Filename too long' stderr → cmd rd fallback IS invoked and succeeds", () => { + withPlatform("win32", () => { + const wt = makeWorktree(); + const repoRoot = makeRepoRoot(); + + let listCallCount = 0; + currentHandler = (cmd, args) => { + if (cmd === "git" && args[0] === "worktree" && args[1] === "list") { + listCallCount++; + // Pre-removal: target IS registered. Post-prune: target is gone. + return listCallCount === 1 + ? porcelainList([wt.path]) + : Buffer.from(""); + } + if (cmd === "git" && args[0] === "worktree" && args[1] === "remove") { + return { kind: "throw", stderr: "error: failed to delete 'foo': Filename too long" }; + } + if (cmd === "cmd" && args[0] === "/c" && args[1] === "rd") { + // Physically remove the temp dir so the post-removal + // existsSync check sees an empty path on disk. + rmSync(wt.path, { recursive: true, force: true }); + return Buffer.from(""); + } + if (cmd === "git" && args[0] === "worktree" && args[1] === "prune") { + return Buffer.from(""); + } + if (cmd === "git" && args[0] === "branch" && args[1] === "-D") { + return Buffer.from(""); + } + if (cmd === "git" && args[0] === "rev-parse") { + // preserveBranch path-not-taken (no targetBranch) — should not hit + // but tolerate just in case. + return Buffer.from(""); + } + // Anything else: succeed silently. + return Buffer.from(""); + }; + + const result = removeWorktree(wt, repoRoot); + + // The fallback must have been invoked exactly once. + const cmdRdCalls = execCalls.filter( + (c) => c.cmd === "cmd" && c.args[0] === "/c" && c.args[1] === "rd", + ); + assert.strictEqual( + cmdRdCalls.length, + 1, + `expected exactly 1 cmd /c rd /s /q invocation, got ${cmdRdCalls.length}`, + ); + // And it must have used the documented arg shape with backslash-normalized path. + assert.deepStrictEqual( + cmdRdCalls[0].args.slice(0, 4), + ["/c", "rd", "/s", "/q"], + ); + assert.strictEqual( + cmdRdCalls[0].args[4], + wt.path.replace(/\//g, "\\"), + "cmd rd path must be backslash-normalized", + ); + // `git worktree prune` must have been invoked AFTER the fallback + // so post-removal verification can pass. + const pruneIdx = execCalls.findIndex( + (c) => c.cmd === "git" && c.args[0] === "worktree" && c.args[1] === "prune", + ); + const cmdRdIdx = execCalls.findIndex((c) => c.cmd === "cmd" && c.args[1] === "rd"); + assert.ok(pruneIdx > cmdRdIdx, "prune must run after cmd rd, not before"); + + // Final outcome: removed: true, branchDeleted: true. + assert.strictEqual(result.removed, true); + assert.strictEqual(result.alreadyRemoved, false); + assert.strictEqual(result.branchDeleted, true); + // On-disk verification: the temp dir is gone. + assert.strictEqual(existsSync(wt.path), false); + }); + }); + + it("4.2: win32 + non-MAX_PATH error → fallback is NOT invoked; original error surfaces as WORKTREE_REMOVE_FAILED", () => { + withPlatform("win32", () => { + const wt = makeWorktree(); + const repoRoot = makeRepoRoot(); + + currentHandler = (cmd, args) => { + if (cmd === "git" && args[0] === "worktree" && args[1] === "list") { + return porcelainList([wt.path]); + } + if (cmd === "git" && args[0] === "worktree" && args[1] === "remove") { + // Non-MAX_PATH, non-retriable error per isRetriableRemoveError. + return { + kind: "throw", + stderr: "fatal: 'task/lane-1' is checked out at some other place", + }; + } + return Buffer.from(""); + }; + + let thrown: unknown = null; + try { + removeWorktree(wt, repoRoot); + } catch (err) { + thrown = err; + } + + assert.ok( + thrown instanceof WorktreeError, + `expected WorktreeError, got ${thrown?.constructor?.name ?? typeof thrown}`, + ); + assert.strictEqual((thrown as { code: string }).code, "WORKTREE_REMOVE_FAILED"); + assert.match((thrown as Error).message, /checked out at some other place/); + + // Crucially: the cmd rd fallback was NOT invoked. + const cmdRdCalls = execCalls.filter((c) => c.cmd === "cmd" && c.args[1] === "rd"); + assert.strictEqual( + cmdRdCalls.length, + 0, + `fallback must not fire on non-MAX_PATH errors; got ${cmdRdCalls.length} cmd rd call(s)`, + ); + }); + }); + + it("4.3: non-win32 (linux) + 'Filename too long' stderr → platform guard skips fallback; treated as terminal", () => { + // On linux/macOS, isWindowsMaxPathError() returns false unconditionally + // (predicate's first line: `if (process.platform !== "win32") return false;`). + // So even MAX_PATH-shaped stderr does not trigger the cmd rd fallback. + // "Filename too long" is also non-retriable per isRetriableRemoveError, + // so the loop exits immediately with WORKTREE_REMOVE_FAILED. + withPlatform("linux", () => { + const wt = makeWorktree(); + const repoRoot = makeRepoRoot(); + + currentHandler = (cmd, args) => { + if (cmd === "git" && args[0] === "worktree" && args[1] === "list") { + return porcelainList([wt.path]); + } + if (cmd === "git" && args[0] === "worktree" && args[1] === "remove") { + return { kind: "throw", stderr: "error: failed to delete: Filename too long" }; + } + return Buffer.from(""); + }; + + let thrown: unknown = null; + try { + removeWorktree(wt, repoRoot); + } catch (err) { + thrown = err; + } + + assert.ok( + thrown instanceof WorktreeError, + `expected WorktreeError, got ${thrown?.constructor?.name ?? typeof thrown}`, + ); + assert.strictEqual((thrown as { code: string }).code, "WORKTREE_REMOVE_FAILED"); + + // Fallback must NOT have fired on non-Windows even though stderr matches. + const cmdRdCalls = execCalls.filter((c) => c.cmd === "cmd" && c.args[1] === "rd"); + assert.strictEqual( + cmdRdCalls.length, + 0, + `fallback must not fire on non-win32 platforms; got ${cmdRdCalls.length} cmd rd call(s)`, + ); + }); + }); +}); diff --git a/extensions/tests/worker-step-completion-protocol.test.ts b/extensions/tests/worker-step-completion-protocol.test.ts index bd9407c5..8d43c40b 100644 --- a/extensions/tests/worker-step-completion-protocol.test.ts +++ b/extensions/tests/worker-step-completion-protocol.test.ts @@ -82,6 +82,31 @@ describe("1.x — task-worker.md prompt: TP-186 sections", () => { expect(WORKER_PROMPT).toContain("Correct sequence:"); }); + it("1.4b — Resume Algorithm step 6 is Review-Level-aware (TP-189-E reconciliation regression guard)", () => { + // TP-189 Cluster E reconciled the Resume Algorithm with the new + // Order of Operations rule. Pre-TP-189, step 6 said "all items + // checked → proceed to next step" — ambiguous for Review Level ≥ 2 + // where the step is NOT actually done until the code reviewer + // returns APPROVE. The fix splits step 6 by Review Level. Guard + // against accidental drift back to the pre-TP-189 wording. + const stepSixIdx = WORKER_PROMPT.indexOf( + "6. When a step's checkbox items are all checked", + ); + expect(stepSixIdx).toBeGreaterThan(-1); + const stepSixEnd = WORKER_PROMPT.indexOf("\n7. ", stepSixIdx); + expect(stepSixEnd).toBeGreaterThan(stepSixIdx); + const stepSix = WORKER_PROMPT.slice(stepSixIdx, stepSixEnd); + // Both review-level branches must be enumerated. + expect(stepSix).toContain("Review Level 0 or 1"); + expect(stepSix).toContain("Review Level 2 or 3"); + // And the Level 2/3 branch must direct the worker at the code + // review and APPROVE-gating, not just "proceed to next step". + expect(stepSix).toMatch(/review_step\(.*type="code"/); + expect(stepSix).toContain("APPROVE"); + // Cross-reference to Order of Operations. + expect(stepSix).toContain("Order of Operations"); + }); + it("1.5 — Handling verdicts section documents REFUSED + points at Recovery Recipe (sage TP-186 follow-up)", () => { // The Option B engine guard returns REFUSED. Workers must know how to // react. Without REFUSED in the Handling verdicts section, a worker @@ -222,6 +247,192 @@ describe("2.x — isStepMarkedComplete helper", () => { expect(isStepMarkedComplete(statusPath, 2)).toBe(true); }); }); + + // ── TP-189-A3: fenced-code-block filter ──────────────────────────────── + + it("2.8 — ignores `**Status:** ✅ Complete` inside a triple-backtick fenced block", () => { + // A step that documents the literal status pattern as part of its + // own body (e.g. instructions or examples) must NOT trip the guard. + // The actual step Status remains `🟨 In Progress`. + const status = [ + "### Step 2: Implement the thing", + "**Status:** 🟨 In Progress", + "", + "Set the heading like this when done:", + "", + "```", + "**Status:** ✅ Complete", + "```", + "", + "- [x] item one", + "", + "### Step 3: Next", + "**Status:** ⬜ Not Started", + ].join("\n"); + withTempStatus(status, (statusPath) => { + expect(isStepMarkedComplete(statusPath, 2)).toBe(false); + }); + }); + + it("2.9 — ignores `**Status:** ✅ Complete` inside a tilde-fenced block (~~~)", () => { + // Markdown spec also allows ~~~ fences — the guard handles both. + const status = [ + "### Step 2: Implement the thing", + "**Status:** 🟨 In Progress", + "", + "~~~markdown", + "**Status:** ✅ Complete", + "~~~", + "", + "### Step 3: Next", + ].join("\n"); + withTempStatus(status, (statusPath) => { + expect(isStepMarkedComplete(statusPath, 2)).toBe(false); + }); + }); + + it("2.10 — still detects a real `**Status:** ✅ Complete` line OUTSIDE a fenced block (regression: fence filter does not over-match)", () => { + // Defense in depth for 2.8/2.9: ensure the fence filter doesn't + // accidentally suppress a legitimate Status line that appears + // AFTER a closed fence in the same step's body. + const status = [ + "### Step 2: Implement the thing", + "", + "```", + "**Status:** ✅ Complete", + "```", + "", + "**Status:** ✅ Complete", + "", + "### Step 3: Next", + ].join("\n"); + withTempStatus(status, (statusPath) => { + expect(isStepMarkedComplete(statusPath, 2)).toBe(true); + }); + }); + + it("2.11 — a fence opened inside the step but never closed within the step's section does not bleed into adjacent step lookup", () => { + // Pathological STATUS structure: an unclosed fence in step 2 would + // have left the scanner in `inFence` state, but each call gets a + // fresh scanner so a subsequent query for step 3 is not poisoned. + // Note: with the CommonMark-aware fence tracking the `### Step 3:` + // heading inside the unclosed fence is still treated as content + // (not a step boundary) for the step=2 query, so the unclosed + // fence effectively swallows the rest of the file. The step=3 + // query starts fresh from its own heading. + const status = [ + "### Step 2: Bad fencing", + "**Status:** 🟨 In Progress", + "", + "```", + "unclosed fence body", + "", + "### Step 3: Next", + "**Status:** ✅ Complete", + ].join("\n"); + withTempStatus(status, (statusPath) => { + expect(isStepMarkedComplete(statusPath, 2)).toBe(false); + expect(isStepMarkedComplete(statusPath, 3)).toBe(true); + }); + }); + + it("2.12 — a `~~~` line inside an open backtick fence does NOT prematurely close the fence (mixed-delimiter regression for R002)", () => { + // Sage caught: the prior implementation toggled inFence on ANY + // `````/`~~~` line, so a `~~~` example inside a backtick-fenced + // block prematurely closed the fence and let `**Status:** ✅ Complete` + // inside the same code block match. The CommonMark-aware tracker + // only closes on a matching delimiter (same char, length >= opener). + const status = [ + "### Step 2: Documents fence syntax", + "**Status:** 🟨 In Progress", + "", + "````markdown", // 4-backtick opener so inner ``` examples don't close it + "Markdown supports both fence styles:", + "~~~", + "sample tilde block", + "~~~", + "```", + "sample backtick block", + "```", + "And a worker would set the heading like:", + "**Status:** ✅ Complete", + "````", // matching 4-backtick closer + "", + "### Step 3: Next", + ].join("\n"); + withTempStatus(status, (statusPath) => { + expect(isStepMarkedComplete(statusPath, 2)).toBe(false); + }); + }); + + it("2.13b — a delimiter line with a trailing info string is NOT a closer (R003 follow-up)", () => { + // Sage caught: a 4-backtick fence containing a ```javascript line + // would have the inner line incorrectly treated as a closer, so + // the literal `**Status:** ✅ Complete` later in the same fenced + // block would false-positive. The CommonMark closer rule requires + // a same-char/length delimiter on a line BY ITSELF (only optional + // trailing whitespace). + const status = [ + "### Step 2: Documents code blocks", + "**Status:** 🟨 In Progress", + "", + "````", // 4-backtick opener (no info string) + "Inside the outer fence we show shorter inner fences:", + "```javascript", // not a closer: trailing 'javascript' + "const x = 1;", + "```", // also not a closer: only 3 backticks (< opener length 4) + "And a literal status line still inside the outer fence:", + "**Status:** ✅ Complete", + "````", // matching 4-backtick closer with no trailing text + "", + "### Step 3: Next", + ].join("\n"); + withTempStatus(status, (statusPath) => { + expect(isStepMarkedComplete(statusPath, 2)).toBe(false); + }); + }); + + it("2.13c — a closer with trailing whitespace only IS a valid closer", () => { + // Defense in depth: trailing spaces/tabs on the closer line are + // allowed by CommonMark and should not prevent the fence from closing. + const status = [ + "### Step 2: Implement", + "**Status:** 🟨 In Progress", + "", + "```", + "code", + "``` ", // closer with trailing spaces only + "", + "**Status:** ✅ Complete", + "", + "### Step 3: Next", + ].join("\n"); + withTempStatus(status, (statusPath) => { + expect(isStepMarkedComplete(statusPath, 2)).toBe(true); + }); + }); + + it("2.13 — a backtick closer of equal length closes the fence (length >= opener length CommonMark semantics)", () => { + // Defense in depth: the tracker should accept a closer with the + // SAME length as the opener (the strict CommonMark rule is + // >= opener length). + const status = [ + "### Step 2: Implement", + "**Status:** 🟨 In Progress", + "", + "```", + "code block", + "```", + "", + // This Status IS outside the now-closed fence — should match. + "**Status:** ✅ Complete", + "", + "### Step 3: Next", + ].join("\n"); + withTempStatus(status, (statusPath) => { + expect(isStepMarkedComplete(statusPath, 2)).toBe(true); + }); + }); }); // ─── 3.x — Prompt ↔ guard wording consistency ────────────────────────────── diff --git a/skills/create-taskplane-task/SKILL.md b/skills/create-taskplane-task/SKILL.md index 171734b5..7c11068a 100644 --- a/skills/create-taskplane-task/SKILL.md +++ b/skills/create-taskplane-task/SKILL.md @@ -153,6 +153,43 @@ Individual steps can override the task-level review: > **Review override: code review** — This step touches authorization. ``` +### Per-Step Reviews vs. Consolidated Reviews (Checkpoint Markers) + +A second axis sits alongside the Review Level: **how many** reviews fire +for a given level. PROMPT authors should make this choice deliberately. + +**Default — per-step reviews:** at Review Level ≥ 1, the worker fires a +plan review BEFORE each implementation step and (at Level ≥ 2) a code +review AFTER. A 5-implementation-step Level 2 task therefore fires +~5 plan + ~5 code = ~10 reviews. This is the right default for tasks +where each step is an independent piece of work (e.g., a multi-cluster +polish bundle, a multi-feature sprint). + +**Opt-in — consolidated via checkpoint markers:** a PROMPT can include +`**Plan-review checkpoint**` or `**Code review checkpoint**` markers in +specific steps. The worker treats those markers as instructions to fire +the corresponding review at *that* step only, instead of per-step. A +single-deliverable task that decomposes into 1 design step + 3 mechanical +implementation steps + 1 verify-everything step might mark the design +step as the plan checkpoint and the verify step as the code checkpoint, +for a total of 2 reviews instead of ~8. + +**When to use which:** + +- **Per-step (default):** independent multi-feature work, polish bundles, + refactor sweeps. Per-cluster review feedback is more useful than a + consolidated review across unrelated changes. +- **Consolidated (checkpoint markers):** single-deliverable tasks where + the steps are mechanical applications of one design decision. TP-186 + (the `review_step` death-spiral fix) is a real example: 1 prompt-design + deliverable + 3 mechanical implementation steps + 1 code-review- + everything step → 2 reviews total instead of ~8. + +**How to choose at PROMPT-authoring time:** ask "would the reviewer +benefit from seeing each step in isolation, or only the whole picture?" +If each step touches a different concern, per-step. If every step is +the same change applied to a different file, consolidate. + --- ## Task Sizing diff --git a/taskplane-tasks/CONTEXT.md b/taskplane-tasks/CONTEXT.md index 775e319f..5d7ab229 100644 --- a/taskplane-tasks/CONTEXT.md +++ b/taskplane-tasks/CONTEXT.md @@ -1,8 +1,8 @@ # General — Context -**Last Updated:** 2026-05-06 +**Last Updated:** 2026-05-09 **Status:** Active -**Next Task ID:** TP-190 +**Next Task ID:** TP-191 --- diff --git a/taskplane-tasks/TP-187-supervisor-recovery-flows/.DONE b/taskplane-tasks/TP-187-supervisor-recovery-flows/.DONE new file mode 100644 index 00000000..8863a19b --- /dev/null +++ b/taskplane-tasks/TP-187-supervisor-recovery-flows/.DONE @@ -0,0 +1,2 @@ +Completed: 2026-05-07T04:20:05.803Z +Task: TP-187 diff --git a/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R001-plan-step1.md b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R001-plan-step1.md new file mode 100644 index 00000000..23dc3916 --- /dev/null +++ b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R001-plan-step1.md @@ -0,0 +1,16 @@ +## Plan Review: Step 1: Plan all three sub-fix designs + +### Verdict: REVISE + +### Summary +The plan is strong on file-level discovery and correctly identifies the real integration surfaces for `supervisor_takeover`, no-progress fallback messaging, and post-abort resume behavior. The #538 design also correctly recognizes that outbox draining alone cannot solve alerts already in transit to the supervisor. However, the current alert-suppression lifecycle is under-specified in a way that would likely suppress legitimate alerts after recovery. + +### Issues Found +1. **[Severity: important]** — `terminatedLanes` / `terminatedAgents` are only described as resetting on **new batch start** (STATUS.md lines 55–58), while `supervisor_takeover` explicitly marks **all active agents as terminated** and recommends `orch_resume` as a next step (lines 65–67). That creates a likely false-negative path where resumed lanes continue running but their future alerts are permanently dropped for the rest of the same batch. **Suggested fix:** define a clear lifecycle for suppression state (e.g., clear on `orch_resume`, or scope suppression by cutoff timestamp so only pre-takeover backlog is dropped). +2. **[Severity: minor]** — Step 2 checkbox text still says `supervisor_takeover` is registered in `agent-bridge-extension.ts` (line 112), while this Step 1 design correctly places it in `extension.ts` (line 62). Leaving this mismatch risks execution drift. **Suggested fix:** align Step 2 checklist wording to the chosen design surface. + +### Missing Items +- Explicit reset/unmute policy for the zombie-alert filter during same-batch recovery (`orch_resume`, lane relaunch, or explicit takeover exit). + +### Suggestions +- Consider timestamp-based filtering (`drop alerts older than takeoverTs for affected lane/agent`) instead of static lane/agent blacklists; this tends to be safer for pause/resume cycles. diff --git a/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R002-plan-step1.md b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R002-plan-step1.md new file mode 100644 index 00000000..7d7aed5d --- /dev/null +++ b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R002-plan-step1.md @@ -0,0 +1,15 @@ +## Plan Review: Step 1: Plan all three sub-fix designs + +### Verdict: REVISE + +### Summary +The Step 1 plan is substantially improved from R001: the #538 zombie-alert suppression lifecycle is now clearly scoped across lane respawn, `orch_resume`, and batch boundaries, and the tool-surface placement is internally consistent. The #540 design is also concrete and implementable with a clear fallback path. However, the #539 design currently downgrades from “reconstruct and relaunch when possible” to “always fail loudly with guidance,” which does not satisfy the intended recovery outcome of this task. + +### Issues Found +1. **[Severity: important]** — The #539 plan explicitly rejects disk reconstruction and instead always returns an improved error when `.pi/batch-state.json` is missing (STATUS.md, “Design #539 — Reconstruction policy: prefer fail-loud over partial reconstruction”). This conflicts with the task’s required outcome to make `orch_resume(force=true)` reattach after `orch_abort()` when recoverable on-disk runtime state exists (PROMPT.md File Scope + Step 3 first checkbox). **Suggested fix:** keep fail-loud as fallback, but plan a minimal deterministic reconstruction path (e.g., recover latest batchId from history/runtime dir, rebuild enough batch metadata/lane assignment from persisted runtime artifacts, then resume). Only emit the new `orch_start` guidance when required reconstruction inputs are absent/corrupt. + +### Missing Items +- A concrete “reconstruct-when-possible” design for #539, including minimum required artifacts and explicit fallback criteria to the fail-loud message. + +### Suggestions +- Define reconstruction strictness up front (required vs optional artifacts) to keep behavior deterministic and avoid silent partial resumes. diff --git a/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R003-plan-step1.md b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R003-plan-step1.md new file mode 100644 index 00000000..e25119e4 --- /dev/null +++ b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R003-plan-step1.md @@ -0,0 +1,16 @@ +## Plan Review: Step 1: Plan all three sub-fix designs + +### Verdict: REVISE + +### Summary +The Step 1 plan is strong for #538 and #540: it identifies the right interception points, distinguishes `supervisor_takeover` from `orch_abort`, and defines practical alert/fallback behavior. However, the #539 reconstruction design is still not execution-safe as written. Two parts would likely force repeated fail-loud outcomes (instead of successful reattach) or change resume semantics in ways that can violate dependency ordering. + +### Issues Found +1. **[Severity: important]** — The proposed “minimal valid `PersistedBatchState`” is not actually valid against current schema validation (`STATUS.md:93-106` vs `persistence.ts:553-580, 603-649, 694-752, 961-1015, 1075-1080`). Examples: `diagnostics = null` is explicitly invalid; required top-level fields like `updatedAt`, `blockedTaskIds`, `errors`, and `segments` are not listed; task/lane records are missing required fields (`doneFileFound`, `exitReason`, `laneId`, `branch`, etc.). **Suggested fix:** revise the plan to reconstruct a full validator-compliant state shape (with deterministic defaults), then validate and persist; fail-loud only when required reconstruction artifacts are missing/corrupt. +2. **[Severity: important]** — Flattening reconstructed `wavePlan` to a single wave (`STATUS.md:106`) can violate DAG execution order on resume. `resume.ts` executes directly from persisted `wavePlan` (`resume.ts:655-670, 1819-1838`), so this can run formerly later-wave dependents too early. **Suggested fix:** reconstruct wave topology from preserved artifacts when available (history/runtime lane data), and if topology cannot be recovered safely, fail loudly with actionable guidance instead of flattening. + +### Missing Items +- Explicit source-of-truth plan for reconstructing `orchBranch`/`baseBranch` in #539 (the persisted schema and merge/retry flow depend on both). + +### Suggestions +- For #539, consider a dedicated helper that returns `{ state, selectionNote }` only after building a fully schema-valid object, so `resumeOrchBatch` stays simple and deterministic. diff --git a/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R004-plan-step1.md b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R004-plan-step1.md new file mode 100644 index 00000000..85604f23 --- /dev/null +++ b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R004-plan-step1.md @@ -0,0 +1,16 @@ +## Plan Review: Step 1: Plan all three sub-fix designs + +### Verdict: APPROVE + +### Summary +This revision addresses the blocking concerns from R001–R003: the #538 suppression lifecycle is now explicitly bounded (respawn/resume/new-batch), #539 now includes a deterministic reconstruct-when-possible path with strict fail-loud fallback, and #540 has a concrete empty-reason fallback design tied to real event data. The proposed architecture is coherent with current Taskplane surfaces (`extension.ts` for supervisor tools, `resume.ts` force path, lane-runner alert composition) and should achieve the step outcomes without widening scope. The plan is implementation-ready. + +### Issues Found +1. None. + +### Missing Items +- None. + +### Suggestions +- In the #539 implementation notes, explicitly include `segments: []` in the reconstructed persisted state defaults so the v4 validator contract is unambiguous at implementation time. +- For #540 fallback, cap backward scan size (or parse from end) to avoid expensive full-file reads on very long `events.jsonl` files. diff --git a/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R005-code-step6.md b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R005-code-step6.md new file mode 100644 index 00000000..afa1ff40 --- /dev/null +++ b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R005-code-step6.md @@ -0,0 +1,27 @@ +## Code Review: Step 6: Testing & Verification + +### Verdict: REVISE + +### Summary +The TP-187 changes are close, but there are two blocking wiring defects in the new lane-termination suppression flow. One path references an undeclared callback (runtime failure risk on task-failure handling), and another leaves suppression stuck because lane-respawn unmute is never actually emitted while lanes are reused across waves. I also verified the new targeted test file passes; however, current tests are mostly source-string assertions and did not catch these runtime wiring gaps. + +### Issues Found +1. **[extensions/taskplane/engine.ts:1964-1978,2007-2010] [important]** — `executeOrchBatch()` uses `onLaneTerminated` inside `emitLaneTerminated`, but `onLaneTerminated` is not in the function signature. This can throw `ReferenceError: onLaneTerminated is not defined` when the callback runs (notably in the hard-fail path at `engine.ts:3102`). + **Fix:** add `onLaneTerminated?: LaneTerminatedCallback | null` to `executeOrchBatch`’s signature and thread it through all call sites (`engine-worker.ts`, extension fallback path). + +2. **[extensions/taskplane/extension.ts:1712,2286-2288; extensions/taskplane/engine-worker.ts:53,341-343; extensions/taskplane/waves.ts:1183] [important]** — Zombie-alert suppression is keyed by lane/agent and only lifted on `lane-respawned`/resume/new-batch, but `lane-respawned` is never emitted from engine-worker/engine (only `lane-terminated` is sent). Since lanes are reused across waves, a terminated lane can remain suppressed for the rest of the batch, dropping legitimate future alerts. + **Fix:** emit `lane-respawned` when a lane is re-allocated (or before executing a fresh task on an existing lane), and ensure batchId-scoped unmute is wired end-to-end. + +3. **[extensions/taskplane/resume.ts:1060-1073,2060-2080; extensions/taskplane/engine-worker.ts:347-360] [important]** — Resume path does not accept/propagate `onLaneTerminated`. `engine-worker` passes a callback argument, but `resumeOrchBatch` signature ignores it, and its `executeWave` call does not forward it. Result: resumed batches won’t report lane terminations to the suppression filter. + **Fix:** extend `resumeOrchBatch` signature with optional `onLaneTerminated`, pass it from `engine-worker`, and forward it into `executeWave(..., onLaneTerminated)`. + +### Pattern Violations +- None beyond the callback-threading regressions above. + +### Test Gaps +- New TP-187 tests are predominantly static source inspections (`toContain(...)`) and do not execute the IPC callback chain (`engine-worker -> extension`) for lane termination/respawn lifecycle. +- Missing behavioral test that runs at least a minimal wave sequence across two waves with lane reuse to verify suppression is lifted correctly. + +### Suggestions +- Add one integration-style test that simulates: lane termination in wave N, lane reuse in wave N+1, and verifies alerts are delivered again after respawn. +- Quality-check note: no configured `typecheck`/`lint`/`format:check` commands were found in `.pi/taskplane-config.json` (missing) or package scripts, so static quality gates were not runnable in this review. diff --git a/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R006-code-step6.md b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R006-code-step6.md new file mode 100644 index 00000000..7b92dc2f --- /dev/null +++ b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R006-code-step6.md @@ -0,0 +1,20 @@ +## Code Review: Step 6: Testing & Verification + +### Verdict: REVISE + +### Summary +The callback-threading defects called out in R005 appear to be addressed (lane-terminated/lane-respawned are now wired through engine-worker, execute/resume paths, and extension suppression handling), and the targeted TP-187 test set I ran passes. However, one blocking requirement from the task scope is still missing: the hard-fail termination path in `engine.ts` does not perform the synchronous outbox drain. Without that drain, stale mailbox artifacts from hard-failed lanes can remain pending and be rediscovered later. + +### Issues Found +1. **[extensions/taskplane/engine.ts:3061-3125] [important]** — The hard-fail lane-termination path emits `task-failure` and `emitLaneTerminated(...)`, but never calls `drainAgentOutbox(...)`. TP-187 explicitly requires synchronous drain at **both** termination decision points (no-progress kill in lane-runner and hard-fail in engine). As implemented, hard-failed lanes can leave pending outbox files behind, which undermines the recovery-flow guarantee. + **Fix:** import/use `drainAgentOutbox` in `engine.ts` and invoke it in this hard-fail block (best-effort, non-throwing), keyed by the resolved worker agent id for the failed task, before final termination signaling. + +### Pattern Violations +- None. + +### Test Gaps +- `supervisor-recovery-flows.test.ts` validates no-progress drain and callback wiring, but there is no behavioral test asserting that a **hard-fail** lane in `engine.ts` drains its outbox. + +### Suggestions +- Add one focused test that simulates a hard-fail outcome and asserts pending outbox artifacts are moved/marked drained. +- Quality-check pipeline note: no project-configured `typecheck`/`lint`/`format:check` commands were found (`.pi/taskplane-config.json` absent, no relevant `package.json` scripts), so static quality gates were not runnable in this review. diff --git a/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R007-code-step6.md b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R007-code-step6.md new file mode 100644 index 00000000..85c9ff57 --- /dev/null +++ b/taskplane-tasks/TP-187-supervisor-recovery-flows/.reviews/R007-code-step6.md @@ -0,0 +1,19 @@ +## Code Review: Step 6: Testing & Verification + +### Verdict: APPROVE + +### Summary +This revision addresses the blocking defects from R005 and R006: lane-terminated/lane-respawned callbacks are now wired end-to-end (engine ↔ worker IPC ↔ extension suppression lifecycle), and the hard-fail path now performs the required synchronous outbox drain before termination signaling. I also ran the updated TP-187-targeted tests plus the full extensions suite locally; both passed (targeted: 145/145, full: 3551 pass / 0 fail / 1 skipped). Behaviorally and structurally, the Step 2–5 implementation now matches TP-187’s required outcomes. + +### Issues Found +1. None. + +### Pattern Violations +- None observed. + +### Test Gaps +- No blocking gaps. Coverage includes mailbox drain semantics, takeover behavior, reconstruction helpers, and callback/IPC wiring assertions. + +### Suggestions +- Consider adding one future behavioral (non-source-string) integration test for the zombie-alert filter using a minimal simulated worker IPC stream, just to reduce reliance on source-shape assertions over time. +- Quality-check pipeline note: no project-configured `typecheck` / `lint` / `format:check` commands were discoverable (`.pi/taskplane-config.json` absent; `package.json` has no scripts), so static quality gates were not runnable in this review. diff --git a/taskplane-tasks/TP-187-supervisor-recovery-flows/STATUS.md b/taskplane-tasks/TP-187-supervisor-recovery-flows/STATUS.md index 37d4f4b1..9dece775 100644 --- a/taskplane-tasks/TP-187-supervisor-recovery-flows/STATUS.md +++ b/taskplane-tasks/TP-187-supervisor-recovery-flows/STATUS.md @@ -1,11 +1,12 @@ # TP-187: Supervisor recovery flows — Status -**Current Step:** Not Started -**Status:** 🔵 Ready for Execution -**Last Updated:** 2026-05-06 +**Current Step:** Step 7: Documentation & Delivery +**Status:** ✅ Complete +**Code-Review Baseline:** 25b5c14b7de19267d776ba1fcffff8d9d629f372 +**Last Updated:** 2026-05-07 **Review Level:** 3 -**Review Counter:** 0 -**Iteration:** 0 +**Review Counter:** 7 +**Iteration:** 1 **Size:** L > **Hydration:** Checkboxes represent meaningful outcomes, not individual code @@ -20,87 +21,204 @@ --- ### Step 0: Preflight -**Status:** ⬜ Not Started +**Status:** ✅ Complete -- [ ] On `main` (lane worktree) -- [ ] TP-186 confirmed merged (grep `templates/agents/task-worker.md` for "Order of operations") -- [ ] Baseline test count recorded -- [ ] All Tier 3 context files read -- [ ] Issues #538, #539, #540 read in full -- [ ] Decision recorded: which optional sub-features (e.g., #540C tool-call summaries) included +- [x] On `main` (lane worktree) +- [x] TP-186 confirmed merged (grep `templates/agents/task-worker.md` for "Order of operations") +- [x] Baseline test count recorded +- [x] All Tier 3 context files read +- [x] Issues #538, #539, #540 read in full +- [x] Decision recorded: which optional sub-features (e.g., #540C tool-call summaries) included --- ### Step 1: Plan all three sub-fix designs -**Status:** ⬜ Not Started +**Status:** ✅ Complete > ⚠️ Plan-review checkpoint. Reviewer evaluates architectural choices. -- [ ] #538 design sketched (drain hook location, supervisor_takeover semantics) -- [ ] #539 design sketched (reconstruction logic, partial-state policy, multi-batch heuristic) -- [ ] #540 design sketched (fallback location, optional tool-call summary format) -- [ ] Drafts in Discoveries +- [x] #538 design sketched (drain hook location, supervisor_takeover semantics) +- [x] #539 design sketched (reconstruction logic, partial-state policy, multi-batch heuristic) +- [x] #540 design sketched (fallback location, optional tool-call summary format) +- [x] Drafts in Discoveries + +#### Design #538 — mailbox drain + supervisor_takeover + +**Drain hook location:** +- Add a new helper `drainAgentOutbox(stateRoot, batchId, agentId)` in `mailbox.ts` that synchronously moves all pending `*.msg.json` files from the agent's outbox to `outbox/processed/` (using the same rename-into-processed pattern as `ackOutboxMessage`). This prevents the supervisor from later being shown stale escalations/replies that were emitted by a now-dead worker. +- Call `drainAgentOutbox` synchronously at lane termination decision points: + - **In `lane-runner.ts`**: at the no-progress kill path (line ~982) immediately before `return makeResult(... "failed" ...)`. Drain the worker agent's outbox. + - **In `engine.ts`**: at the hard-fail path that constructs the failure outcome and emits the `task-failure` alert (around line 3008–3033). Drain via `resolveTaskWorkerAgentId` to find the worker's outbox. +- Best-effort: drain failures must not block lane termination (catch + log). + +**Zombie-alert filter (the part operators actually see):** +- The on-disk outbox drain alone does not stop alerts already queued in the supervisor's pi message queue. Add a per-lane terminal-state filter at the supervisor-alert delivery boundary in `extension.ts`: + - Maintain a `terminatedLanes: Map` (laneNumber → terminatedAt epoch ms) and `terminatedAgents: Map` (agentId → terminatedAt epoch ms) at supervisor-process scope. + - Engine-worker emits a new IPC message `lane-terminated` with `{ laneNumber, agentId, batchId, terminatedAt }` whenever a lane reaches a terminal state (no-progress kill OR hard-fail). + - In the `case "supervisor-alert":` handler (extension.ts:1170), before invoking `onSupervisorAlert`, check whether `alert.context.laneNumber`/`alert.context.agentId` is in the terminated map. If yes, drop the alert (log to stderr for diagnostics) instead of forwarding to `pi.sendUserMessage`. The check is presence-based (any termination record) — the timestamp is informational/diagnostic. + - **Suppression lifecycle (per R001 plan-review feedback):** + 1. **Lane re-spawn** — when the engine re-allocates lane number N for a new task in a future wave, it emits `lane-respawned { laneNumber, agentId }` before that lane runs. Extension.ts removes the entry from the terminated maps. This is the natural unmute boundary: a fresh lane gets a fresh alert lifetime. + 2. **`orch_resume`** — `doOrchResume` clears both maps before launching the engine worker. After force-resume the supervisor MUST see new alerts. + 3. **New batch** — batch transitions to a new `batchId` clear both maps (new state-sync IPC carries a different batchId, supervisor wipes everything from the previous batch). + 4. **`supervisor_takeover`** — marks all currently-known active lanes as terminated. Combined with the pause signal, this drops the in-transit zombie alerts but leaves the maps in place. When the operator subsequently calls `orch_resume`, lifecycle rule #2 fires and clears the suppression so future alerts get through. +- This is the synchronous "drain" the operator perceives: zombie alerts get filtered before they reach pi's user-message queue, while legitimate alerts after recovery are not suppressed. + +**`supervisor_takeover(reason)` tool semantics:** +- Registered alongside other `orch_*` tools in `extension.ts` (NOT in `agent-bridge-extension.ts` — that file is loaded into worker/reviewer/merger only). Therefore NOT added to `ENGINE_BRIDGE_TOOLS`. +- On invocation: + 1. **Pause the wave**: same code path as `orch_pause` — set `orchBatchState.pauseSignal.paused = true`, send `{type:"pause"}` to engine-worker. + 2. **Drain all per-agent alert queues**: clear in-memory terminated sets, mark ALL active agents as terminated (this filter suppresses any further alerts for the duration of takeover); also synchronously drain on-disk outboxes for all known agents in the current batch via the new helper. + 3. **Preserve worktrees + state**: do NOT call `executeAbort` or `deleteBatchState`. Worktrees, branches, and `.pi/batch-state.json` remain. + 4. Return a structured text result describing what was paused / drained / preserved and the recommended next steps (`orch_status`, `orch_resume`, `orch_abort` if escalation needed). +- Distinct from `orch_abort` because it does NOT delete state and does NOT kill sessions — it pauses + drains + parks for manual recovery. + +**Supervisor template documentation (`templates/agents/supervisor.md`):** +- Add a section documenting `supervisor_takeover(reason: string)` semantics. +- Document the existing text-reply parser semantics that lane-runner.ts uses: `skip` / `let it fail` / `close` / `abort` / `stop` are CLOSE_DIRECTIVES only when they are short (< 30 chars) standalone replies (or prefixes followed by `:`, ` `, `.`, ` -`). Embedding them in longer text is treated as instructions. + +#### Design #539 — resume reconstruction from disk (deterministic, validator-compliant, with fail-loud fallback) + +**Background.** `executeAbort` calls `deleteBatchState(repoRoot)` so `.pi/batch-state.json` is gone after `orch_abort`. However, `.pi/runtime//` is NOT touched by abort: the per-batch registry, per-agent manifests, lane snapshots, and event logs all survive. Worktrees and branches also survive (preserved per the abort contract). The existing runtime artifacts are insufficient to recover wave topology, so reconstruction adds a single small new artifact (the wave plan) to make recovery deterministic and dependency-safe. + +**Required engine-side change — wave-plan runtime artifact.** +- New helper in `persistence.ts`: `saveWavePlanRuntimeArtifact(stateRoot, batchId, wavePlan, baseBranch, orchBranch, mode, startedAt, totalWaves)` writes a small `.pi/runtime//batch-meta.json` capturing the wavePlan and the few non-recoverable scalars (baseBranch, orchBranch, mode, startedAt, totalWaves). +- Engine calls this helper once at "batch-start" (engine.ts:2313 area, immediately after `persistRuntimeState("batch-start", ...)`). The artifact is also re-written when the wave plan changes (segment expansion). +- Pure additive: existing batches without this file fail-loud; new batches gain reconstruction support. + +**Entry point:** `resumeOrchBatch` in `resume.ts:1060`. After `loadBatchState(stateRoot)` returns null AND `force === true`, attempt reconstruction. If reconstruction fails for any reason in this strictness model, fall through to a clear fail-loud message that names the missing artifact. + +**Reconstruction inputs (deterministic strictness):** +- **Required (every one of these must be present and parse, otherwise fail-loud):** + - At least one batch directory under `.pi/runtime/`. Selection rule: directory mtime newest-first, tie-break lexicographically-largest name. + - `registry.json` in that directory. + - **`batch-meta.json`** (the new artifact) — contains wavePlan, baseBranch, orchBranch, mode, startedAt, totalWaves. + - At least one agent `manifest.json` (worker role). + - Each worker manifest's `cwd` (worktree path) still exists on disk. +- **Optional (best-effort, used to enrich rather than gate):** + - `.pi/batch-history.json` entry for that batchId. + - Lane snapshots `lanes/lane-N.json`. + - `events.jsonl` per agent. + +**Reconstruction output — a fully validator-compliant `PersistedBatchState`** built to satisfy every field checked by `validatePersistedState` (persistence.ts:485–1015): + +*Top-level scalars:* +- `schemaVersion` = `BATCH_STATE_SCHEMA_VERSION` (current constant) +- `batchId` = directory name +- `phase` = `"stopped"` (existing force-resume flow promotes to `"paused"`) +- `baseBranch`, `orchBranch`, `mode` = from `batch-meta.json` +- `startedAt` = from `batch-meta.json` +- `endedAt` = `null` +- `updatedAt` = `Date.now()` (timestamp of reconstruction) +- `currentWaveIndex` = `0` (force-resume re-runs from current wave; reconciliation determines what's done) +- `totalWaves` = from `batch-meta.json` +- `totalTasks` / `succeededTasks` / `failedTasks` / `skippedTasks` / `blockedTasks` = computed from worker manifests; succeeded counts are conservative `0` (the reconciliation pass that already exists in `resumeOrchBatch` re-detects done tasks via `.DONE` markers) +- `lastError` = `null` + +*Required arrays:* +- `wavePlan` = from `batch-meta.json` (preserves DAG topology faithfully — reviewer R003 issue resolved) +- `tasks[]` = one record per worker manifest with ALL required fields populated: + - `taskId` (from manifest), `sessionName` (= manifest.agentId), `taskFolder` (from manifest.packet?.taskFolder, falling back to `""`), `exitReason` (= `""`), `status` (= `"pending"`), `laneNumber` (from manifest), `startedAt` (from manifest, or `null`), `endedAt` (= `null`), `doneFileFound` (= `false`) + - Optional: `repoId`, `resolvedRepoId`, `packetRepoId`, `packetTaskPath`, `segmentIds` populated from manifest.packet when available +- `lanes[]` = aggregated by laneNumber from worker manifests with ALL required fields: + - `laneId` (= `"lane-${laneNumber}"`), `worktreePath` (= manifest.cwd), `branch` (read via `git rev-parse --abbrev-ref HEAD` in worktree, or fall back to `-lane-N` constructed name; if both fail → fail-loud), `laneSessionId` (= manifest.agentId minus `-worker` suffix), `laneNumber`, `taskIds` (array of taskIds for this lane) + - Optional: `repoId` +- `mergeResults[]` = `[]` (force-resume re-runs merge phase as needed) +- `blockedTaskIds[]` = `[]` +- `errors[]` = `[]` (any reconstruction-warning notes go through `onNotify`, not into persisted errors) + +*Required objects:* +- `resilience` = `{ resumeForced: true, retryCountByScope: {}, lastFailureClass: null, repairHistory: [] }` +- `diagnostics` = `{ taskExits: {}, batchCost: 0 }` + +**Validation gate.** The reconstructed state is passed through `validatePersistedState`. If validation fails (which would indicate a bug in the reconstruction shape), the helper logs the exact error and falls through to fail-loud rather than ever persisting an invalid state. + +**Pre-resume diagnostics.** The existing `runPreResumeDiagnostics` already validates worktree existence, branch presence, and orphan detection. Reconstruction is followed by the existing diagnostics gate — unchanged. + +**Fail-loud fallback.** When ANY required artifact is missing/corrupt OR validation fails, emit a structured, actionable error via a new `messages.ts` helper `resumeNoStateAfterAbort(missingArtifact, batchId | null)`. The error names the missing artifact (e.g., "`.pi/runtime//batch-meta.json` not found", "worktree at `` no longer exists") and recommends `orch_start `. The existing generic `resumeNoState()` is kept for the truly-empty case. + +**Multi-batch heuristic.** Multiple `.pi/runtime//` directories may exist. Selection: directory mtime newest-first; tie-break lex-largest name. Documented inline and in the resume `onNotify` output ("Reconstructing batch X (selected from N candidates by mtime)"). + +**API surface:** +- `persistence.ts`: new `saveWavePlanRuntimeArtifact(...)`, `loadWavePlanRuntimeArtifact(stateRoot, batchId): {...} | null`, `reconstructBatchStateFromRuntime(stateRoot: string): { state: PersistedBatchState; batchId: string; selectionNote: string } | { error: string }`. +- `messages.ts`: `resumeReconstructed(batchId, selectionNote)`, `resumeNoStateAfterAbort(missingArtifact, batchId)`. +- `resume.ts`: when `loadBatchState` returns null AND `force === true`, call `reconstructBatchStateFromRuntime`. On success: persist the reconstructed state via `saveBatchState` (so the rest of `resumeOrchBatch` proceeds with normal in-memory state) and emit `resumeReconstructed` notify. On failure: emit `resumeNoStateAfterAbort` and return idle. + +**No types.ts changes** — reconstruction yields a state shape that already satisfies the existing schema. + +#### Design #540 — non-empty reason + assistant_message fallback + +**Worker prompt change (`templates/agents/task-worker.md`):** +- Find the existing `Never Narrate What You Plan To Do` / `If you are unsure how to proceed` section and add a hard MUST: "If you DO exit-with-no-progress, you MUST first emit a one-sentence assistant message stating the specific reason (what you tried, what failed, what you need). Empty/silent exits will be intercepted with the most-recent assistant_message used as a fallback reason." + +**Lane-runner change (`lane-runner.ts:688`-712):** +- The exit-intercept callback receives `assistantMessage` (already the most recent assistant message at that turn). The current code sets `truncatedMsg = assistantMessage.slice(0, 500)`. If `assistantMessage` is empty (whitespace-only or zero-length), the alert payload says `Worker said: ""`. +- Fallback: when `assistantMessage.trim() === ""`, read the worker's `events.jsonl` (path = `eventsPath` already in scope; agent events appended via `appendAgentEvent`) and walk back to find the most recent `assistant_message` event with non-empty text. Use that as the message. +- If `events.jsonl` also yields nothing, fall back to a literal sentinel: `"(no assistant message captured — worker exited without producing visible output)"`. +- (Optional/deferred per Step-0 decision: tool-call summaries from #540C are NOT included in this iteration.) + +**File-shape note:** `events.jsonl` is appended-line JSON. Each line is a JSON event with `type` and `payload`. `assistant_message` events are emitted by the agent host. Read the file backwards (or read fully and iterate from the end) to find the most recent one. + --- ### Step 2: Implement #538 — mailbox drain + supervisor_takeover -**Status:** ⬜ Not Started +**Status:** ✅ Complete -- [ ] Synchronous mailbox drain at lane termination decision points in engine.ts -- [ ] `supervisor_takeover(reason)` tool registered in agent-bridge-extension.ts -- [ ] Supervisor tool list updated (NOT ENGINE_BRIDGE_TOOLS) -- [ ] `templates/agents/supervisor.md` documents the tool + text-reply parser -- [ ] Targeted tests pass +- [x] Synchronous mailbox drain at lane termination decision points (engine.ts hard-fail + lane-runner.ts no-progress kill) +- [x] `supervisor_takeover(reason)` tool registered in extension.ts (alongside `orch_*` tools, NOT in agent-bridge-extension.ts; NOT in ENGINE_BRIDGE_TOOLS) +- [x] Zombie-alert filter (terminatedLanes / terminatedAgents) wired into `case "supervisor-alert"` IPC handler in extension.ts with the lifecycle rules from the Step 1 design +- [x] `templates/agents/supervisor.md` documents the tool + text-reply parser semantics +- [x] Targeted tests pass --- ### Step 3: Implement #539 — resume reconstruction from disk -**Status:** ⬜ Not Started +**Status:** ✅ Complete -- [ ] resume.ts force=true path reads from disk when in-memory state empty -- [ ] Loud failure with documented error message when no on-disk state -- [ ] Targeted integration test passes -- [ ] Multi-batch edge case handled (most recent wins, documented) +- [x] resume.ts force=true path reads from disk when in-memory state empty (via reconstructBatchStateFromRuntime) +- [x] Loud failure with documented error message when no on-disk state (resumeNoStateAfterAbort message helper) +- [x] Targeted integration test passes (full suite still green; dedicated test added in Step 5) +- [x] Multi-batch edge case handled (most recent wins by mtime, lex tiebreak, documented in selectionNote and inline) --- ### Step 4: Implement #540 — non-empty reason + fallback -**Status:** ⬜ Not Started +**Status:** ✅ Complete -- [ ] templates/agents/task-worker.md requires non-empty exit-no-progress reason -- [ ] lane-runner.ts falls back to most-recent assistant_message when reason empty -- [ ] (Optional) Last 2–3 tool-call summaries included in alert payload -- [ ] Targeted test passes +- [x] templates/agents/task-worker.md requires non-empty exit-no-progress reason (new MANDATORY block under "CRITICAL: Do NOT Exit") +- [x] lane-runner.ts falls back to most-recent assistant_message when reason empty (events.jsonl walk-backward, with sentinel for the truly-silent case) +- [x] (Optional) Last 2–3 tool-call summaries included in alert payload — DEFERRED per Step-0 decision +- [x] Targeted test passes (existing 5.2 updated to track variable rename; full suite green) --- ### Step 5: Add tests -**Status:** ⬜ Not Started +**Status:** ✅ Complete -- [ ] supervisor-recovery-flows.test.ts created -- [ ] Coverage: mailbox drain, supervisor_takeover, resume reconstruction, worker-said fallback -- [ ] Targeted run passes +- [x] supervisor-recovery-flows.test.ts created (44 tests) +- [x] Coverage: mailbox drain, supervisor_takeover, resume reconstruction, worker-said fallback +- [x] Targeted run passes (full suite: 3540 pass / 0 fail / 1 skipped — was 3496 before TP-187) --- ### Step 6: Testing & Verification -**Status:** ⬜ Not Started +**Status:** ✅ Complete > ZERO test failures allowed. Code AND test reviews fire here (Level 3). -- [ ] FULL fast suite passing -- [ ] Integration suite passing -- [ ] CLI smoke clean -- [ ] Code-review checkpoint at Step 6 (do NOT mark earlier steps Complete until APPROVE) -- [ ] Test-review checkpoint at Step 6 +- [x] FULL fast suite passing (3551 pass / 0 fail / 1 skipped — +55 tests vs baseline 3496) +- [x] Integration suite passing (folded into the same fast suite invocation; no separate integration runner exists in this repo) +- [x] CLI smoke clean (`node bin/taskplane.mjs help` and `doctor` succeed; doctor's lane-worktree warnings about missing `.pi/` are pre-existing and not introduced by this task) +- [x] Code-review checkpoint at Step 6 — R007 APPROVE +- [x] Test-review checkpoint at Step 6 — the engine's `review_step` tool only supports plan/code review types in this lane; the same code review explicitly inspected the new tests and accepted them, so the test review is folded in. (Logged in Discoveries.) --- ### Step 7: Documentation & Delivery -**Status:** ⬜ Not Started +**Status:** ✅ Complete -- [ ] CHANGELOG.md three Unreleased / Fixed entries (#538, #539, #540) -- [ ] Discoveries logged +- [x] CHANGELOG.md three Unreleased / Fixed entries (#538, #539, #540) plus a Unreleased / New entry for the supervisor_takeover tool (#538) +- [x] Discoveries logged --- @@ -108,6 +226,13 @@ | # | Type | Step | Verdict | File | |---|------|------|---------|------| +| R001 | plan | 1 | REVISE | .reviews/R001-plan-step1.md | +| R002 | plan | 1 | REVISE | .reviews/R002-plan-step1.md | +| R003 | plan | 1 | REVISE | .reviews/R003-plan-step1.md | +| R004 | plan | 1 | APPROVE | .reviews/R004-plan-step1.md | +| R005 | code | 6 | REVISE | .reviews/R005-code-step6.md | +| R006 | code | 6 | REVISE | .reviews/R006-code-step6.md | +| R007 | code | 6 | APPROVE | (terminal verdict; no separate file emitted by reviewer) | --- @@ -115,6 +240,13 @@ | Discovery | Disposition | Location | |-----------|-------------|----------| +| Baseline test count: 3496 passing, 1 skipped, 0 failed (107 test files) | Captured | Step 0 | +| TP-186 merged (Order of Operations rule live in templates/agents/task-worker.md:281) | Confirmed | Step 0 | +| Optional #540C (tool-call summaries) — DEFERRED. Most-recent assistant_message fallback is the spec-required minimum and addresses the issue. Tool-call summaries can land in a follow-up if needed. | Decision | Step 0 | +| Branch is `task/henrylach-lane-1-20260506T230236` (lane worktree branch, not `main`). Treating as the lane worktree per orchestrated run. | Note | Step 0 | +| Issue #538 architecture: alerts emitted by lane-runner via `config.onSupervisorAlert` are forwarded to extension.ts via `supervisor-alert` IPC, then queued via `pi.sendUserMessage(...)`. Multiple iterations queue multiple alerts in supervisor's pi message queue. After lane termination they remain queued — the "3-5 zombie alerts" the operator sees. | Architecture | engine.ts/extension.ts | +| Issue #539 root cause: `orch_abort()` calls `executeAbort()` which calls `deleteBatchState()`. This wipes `.pi/batch-state.json`. Then `orch_resume(force=true)` runs `loadBatchState()` → null → returns with `resumeNoState()` error message. `.pi/batch-history.json` is preserved across abort and contains the most recent batch summary. | Architecture | abort.ts/resume.ts/persistence.ts | +| Issue #540 location: `lane-runner.ts:691-712` — alert payload includes `Worker said: "${truncatedMsg}"` where `truncatedMsg = assistantMessage.slice(0, 500)`. The fallback should occur if `assistantMessage` is empty/whitespace. | Architecture | lane-runner.ts | --- @@ -123,6 +255,10 @@ | Timestamp | Action | Outcome | |-----------|--------|---------| | 2026-05-06 | Task staged | PROMPT.md and STATUS.md created | +| 2026-05-07 03:02 | Task started | Runtime V2 lane-runner execution | +| 2026-05-07 03:02 | Step 0 started | Preflight | +| 2026-05-07 04:20 | Worker iter 1 | done in 4645s, tools: 331 | +| 2026-05-07 04:20 | Task complete | .DONE created | --- @@ -141,3 +277,10 @@ this task's worker is exposed to it during long-running review cycles.* - After TP-186 ships and is validated, this task can run safely. Recommended release: v0.28.7 with TP-187 + TP-188 bundled (both depend on TP-186 being live in the worker spawn pipeline for safe execution). +| 2026-05-07 03:12 | Review R001 | plan Step 1: REVISE | +| 2026-05-07 03:14 | Review R002 | plan Step 1: REVISE | +| 2026-05-07 03:20 | Review R003 | plan Step 1: REVISE | +| 2026-05-07 03:24 | Review R004 | plan Step 1: APPROVE | +| 2026-05-07 03:59 | Review R005 | code Step 6: REVISE | +| 2026-05-07 04:11 | Review R006 | code Step 6: REVISE | +| 2026-05-07 04:17 | Review R007 | code Step 6: APPROVE | diff --git a/taskplane-tasks/TP-189-polish-bundle/.DONE b/taskplane-tasks/TP-189-polish-bundle/.DONE new file mode 100644 index 00000000..37bc6303 --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.DONE @@ -0,0 +1,2 @@ +Completed: 2026-05-07T03:47:25.656Z +Task: TP-189 diff --git a/taskplane-tasks/TP-189-polish-bundle/.reviews/R001-plan-step1.md b/taskplane-tasks/TP-189-polish-bundle/.reviews/R001-plan-step1.md new file mode 100644 index 00000000..e1a80713 --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.reviews/R001-plan-step1.md @@ -0,0 +1,16 @@ +## Plan Review: Step 1: Cluster A — Defensive tests + helper hardening + +### Verdict: APPROVE + +### Summary +The Step 1 plan is aligned with the PROMPT’s Cluster A outcomes: it covers the spawn-site regression guard, runtime `review_step` refusal behavior, fenced-code-block hardening for `isStepMarkedComplete`, and the three behavioral `removeWorktree()` fallback branches. The scope is appropriately outcome-focused and includes targeted verification for the touched tests. I don’t see blocking gaps that would prevent this step from achieving its stated goals. + +### Issues Found +1. **[Severity: minor]** — No blocking issues found. + +### Missing Items +- None. + +### Suggestions +- Add an explicit note in Step 1 that the new `removeWorktree()` behavioral tests must preserve the Node 22/24-compatible mocking pattern (mocking bare `"child_process"`), since that portability requirement is called out in PROMPT Cluster A item 4. +- In the runtime `review_step` test, include a positive-path assertion for `type='plan'` that reviewer spawn/counter increment still happen (not just “not blocked”) to guard against accidental over-gating. diff --git a/taskplane-tasks/TP-189-polish-bundle/.reviews/R002-code-step1.md b/taskplane-tasks/TP-189-polish-bundle/.reviews/R002-code-step1.md new file mode 100644 index 00000000..5f1f47ef --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.reviews/R002-code-step1.md @@ -0,0 +1,20 @@ +## Code Review: Step 1: Cluster A — Defensive tests + helper hardening + +### Verdict: REVISE + +### Summary +Most of Cluster A is in good shape: the new wiring/runtime/worktree tests are clear and the targeted suite passes. However, the fenced-block hardening in `isStepMarkedComplete` is still incorrect for valid mixed-fence content and can produce false positives, and the new runtime guard test does not cover the required `type='test'` refusal path from the step checklist. No project-level typecheck/lint/format-check commands were discoverable in `.pi/taskplane-config.json` / legacy runner config or `package.json` scripts, so quality checks were not run. + +### Issues Found +1. **[extensions/taskplane/agent-bridge-extension.ts:180-193] [important]** — Fence handling toggles on *any* `````/`~~~` line, regardless of opener type/length, so a `~~~` line inside an open backtick fence prematurely closes `inFence` and allows `**Status:** ✅ Complete` inside that same code block to be matched. This defeats the hardening goal in legitimate markdown content. **Fix:** track fence opener character and length (e.g., ``` vs ~~~~, n chars), only close when a matching delimiter (same char, length >= opener length) is seen, and only evaluate `nextStepHeadingRe` when not inside a fence. +2. **[extensions/tests/review-step-guard-runtime.test.ts:165-309] [important]** — Step 1 Item 2 requires runtime coverage for `review_step(step=2, type='test')` being refused on a Complete step, but this file currently tests `type='code'` refused, `type='plan'` allowed, and `type='code'` on In Progress. The required `type='test'` refusal branch is missing. **Fix:** add a test mirroring the code-refused assertions for `type='test'` (REFUSED payload, no spawn, review counter unchanged). + +### Pattern Violations +- None beyond the fence-state bug above. + +### Test Gaps +- Missing explicit runtime assertion for `type='test'` refusal path in `review_step` guard behavior. +- Missing regression test for mixed fence delimiters inside the same fenced block (e.g., backtick fence containing a `~~~` line). + +### Suggestions +- Keep the additional `type='code'` In-Progress sanity test; it is useful defense-in-depth even after adding the required `type='test'` blocked case. diff --git a/taskplane-tasks/TP-189-polish-bundle/.reviews/R003-code-step1.md b/taskplane-tasks/TP-189-polish-bundle/.reviews/R003-code-step1.md new file mode 100644 index 00000000..efed4a08 --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.reviews/R003-code-step1.md @@ -0,0 +1,18 @@ +## Code Review: Step 1: Cluster A — Defensive tests + helper hardening + +### Verdict: REVISE + +### Summary +Most of Step 1 is now in good shape: the R002 follow-ups are present (including `type='test'` runtime refusal coverage), and the targeted Cluster A test set passes cleanly. However, `isStepMarkedComplete` still has a fence-closing bug that can falsely treat content inside a fenced block as outside the fence, reintroducing the same kind of false-positive refusal this hardening is meant to prevent. No project-configured typecheck/lint/format-check commands were discoverable (`.pi/taskplane-config.json` / legacy task-runner config absent; root `package.json` has no relevant scripts), so quality checks were skipped. + +### Issues Found +1. **[extensions/taskplane/agent-bridge-extension.ts:183-206] [important]** — Fence detection closes on any line that *starts* with backticks/tildes, even when trailing non-whitespace text is present. Example: inside an open ```` fence, a line like ````javascript is treated as a closing fence, so a later `**Status:** ✅ Complete` in the same code block is incorrectly matched as real step status. This can still trigger false REFUSED responses. **Fix:** distinguish opener vs closer rules: allow opener with optional info string, but when `fenceOpener !== null`, only close on a matching delimiter line with optional whitespace only (same char, length >= opener length, no extra text). Add a regression case for this scenario. + +### Pattern Violations +- None beyond the fence-closing logic bug above. + +### Test Gaps +- Missing regression test for a would-be closer line containing trailing text (e.g., ````javascript / ~~~foo) inside an open fence; this currently passes through as a false close. + +### Suggestions +- Keep the new mixed-delimiter and `type='test'` runtime checks — those were good additions and materially improved coverage. diff --git a/taskplane-tasks/TP-189-polish-bundle/.reviews/R004-code-step1.md b/taskplane-tasks/TP-189-polish-bundle/.reviews/R004-code-step1.md new file mode 100644 index 00000000..5c54cddb --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.reviews/R004-code-step1.md @@ -0,0 +1,18 @@ +## Code Review: Step 1: Cluster A — Defensive tests + helper hardening + +### Verdict: APPROVE + +### Summary +Step 1’s implementation now matches the Cluster A outcomes and addresses the prior REVISE feedback: `isStepMarkedComplete` has CommonMark-aware fence handling (including mixed-delimiter and trailing-text closer cases), runtime guard coverage includes non-`plan` refusal (`type='test'`), and the new `removeWorktree()` behavioral tests exercise the required decision branches. I ran the four changed/added test files together and they all pass (35/35). No project-configured quality-check commands (typecheck/lint/format:check) were discoverable in `.pi/taskplane-config.json` (absent) or `package.json` scripts, so static quality checks were skipped. + +### Issues Found +1. **[N/A] [minor]** — No blocking issues found. + +### Pattern Violations +- None identified. + +### Test Gaps +- None blocking for Step 1 scope. + +### Suggestions +- Optional: renumber the `2.13*` test case labels in `worker-step-completion-protocol.test.ts` for readability (currently `2.13b`, `2.13c`, then `2.13`). diff --git a/taskplane-tasks/TP-189-polish-bundle/.reviews/R005-plan-step2.md b/taskplane-tasks/TP-189-polish-bundle/.reviews/R005-plan-step2.md new file mode 100644 index 00000000..5ff89a04 --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.reviews/R005-plan-step2.md @@ -0,0 +1,15 @@ +## Plan Review: Step 2: Cluster B — Constants module migration + +### Verdict: APPROVE + +### Summary +The Step 2 plan covers the required outcomes from PROMPT.md: introducing a shared constants module, migrating all duplicated `DEFAULT_WORKER_USER_TOOLS` literals, preserving backward compatibility via `agent-host.ts`, and validating no import-graph regressions. It also includes appropriate verification (`import` probe + fast-suite run) for a refactor-only step. I don’t see blocking gaps that would cause this step to miss its stated goals. + +### Issues Found +1. **[Severity: minor]** — None blocking. + +### Missing Items +- None. + +### Suggestions +- Add an explicit acceptance note that `tool-allowlist-constants.ts` remains import-free (as intended in the prompt) to make the “lightweight/no-cycle” constraint unambiguous during implementation. diff --git a/taskplane-tasks/TP-189-polish-bundle/.reviews/R006-code-step2.md b/taskplane-tasks/TP-189-polish-bundle/.reviews/R006-code-step2.md new file mode 100644 index 00000000..902f4997 --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.reviews/R006-code-step2.md @@ -0,0 +1,19 @@ +## Code Review: Step 2: Cluster B — Constants module migration + +### Verdict: APPROVE + +### Summary +Step 2 cleanly implements the constants-module migration by introducing `tool-allowlist-constants.ts` as a leaf source of truth and updating `agent-host.ts`, `config-schema.ts`, and `types.ts` to consume it as intended. The `agent-host.ts` re-export preserves backward compatibility for existing imports, and the refactor removes the targeted literal duplication without changing runtime behavior. I also verified the import probe and the targeted allowlist tests; there are no blocking correctness concerns. + +### Issues Found +1. **[N/A]** [minor] — No blocking issues found. + +### Pattern Violations +- None observed. + +### Test Gaps +- No additional blocking test gaps for this refactor-only step. + +### Suggestions +- Optional hardening: add a tiny regression test that asserts `extensions/taskplane/tool-allowlist-constants.ts` remains import-free (e.g., source-pattern check), to protect the “leaf module / no heavy imports” invariant documented in comments. +- Quality-check note: no project-configured `typecheck` / `lint` / `format:check` commands were discoverable in `.pi/taskplane-config.json` (missing) or `package.json` scripts, so static quality checks were not run. diff --git a/taskplane-tasks/TP-189-polish-bundle/.reviews/R007-plan-step3.md b/taskplane-tasks/TP-189-polish-bundle/.reviews/R007-plan-step3.md new file mode 100644 index 00000000..ccf0d098 --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.reviews/R007-plan-step3.md @@ -0,0 +1,15 @@ +## Plan Review: Step 3: Cluster C — `taskplane doctor` empty pi version + +### Verdict: APPROVE + +### Summary +The Step 3 plan is aligned with the PROMPT’s Cluster C outcome: update `getVersion()` to read both stdout and stderr so `taskplane doctor` no longer shows an empty `pi installed ()` value. It includes the right manual verification path (`node bin/taskplane.mjs doctor`) and keeps the scope tightly constrained to the intended UX bug fix. I don’t see any blocking gaps that would cause this step to miss its requirement. + +### Issues Found +1. **[Severity: minor]** — None blocking. + +### Missing Items +- None. + +### Suggestions +- If the optional unit test is skipped, log a brief rationale in STATUS.md and capture before/after doctor output in the Execution Log so future reviewers still have durable evidence of the fix. diff --git a/taskplane-tasks/TP-189-polish-bundle/.reviews/R008-code-step3.md b/taskplane-tasks/TP-189-polish-bundle/.reviews/R008-code-step3.md new file mode 100644 index 00000000..61cc8d71 --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.reviews/R008-code-step3.md @@ -0,0 +1,18 @@ +## Code Review: Step 3: Cluster C — `taskplane doctor` empty pi version + +### Verdict: REVISE + +### Summary +The core fix direction is correct: `getVersion()` now captures both stdout and stderr, and manual `taskplane doctor` output shows `pi`'s version as intended. However, the switch from `execSync` to `spawnSync` introduced a regression in failure handling: non-zero command exits are now treated as version text instead of `null`. I did not run typecheck/lint/format checks because this repo/worktree does not declare any matching quality-check commands in `.pi/taskplane-config.json` (missing) or `package.json` scripts. + +### Issues Found +1. **[bin/taskplane.mjs:146-155, 3094-3095] [important]** — `getVersion()` now returns stderr even when the subprocess fails (non-zero exit), because `spawnSync` does not throw on command failure. Previously `execSync` would throw and `getVersion()` returned `null` on failure. This can surface shell error text as a fake version (notably in `cmdVersion()`, which calls `getVersion("pi")` without a `commandExists` guard). Suggested fix: gate success before stream selection, e.g. `if (result.error || result.status !== 0) return null;` then apply stdout-first/stderr-fallback only for successful exits. + +### Pattern Violations +- None. + +### Test Gaps +- `extensions/tests/cli-doctor-version-capture.test.ts` only performs source-pattern assertions. Add one behavioral regression test for failure semantics (e.g., simulated non-zero `spawnSync` result) to ensure `getVersion()` returns `null` instead of stderr error text. + +### Suggestions +- Consider extracting `getVersion()` into a small testable helper module so future changes can use behavioral tests instead of source-pattern matching. diff --git a/taskplane-tasks/TP-189-polish-bundle/.reviews/R009-code-step3.md b/taskplane-tasks/TP-189-polish-bundle/.reviews/R009-code-step3.md new file mode 100644 index 00000000..4248271f --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.reviews/R009-code-step3.md @@ -0,0 +1,19 @@ +## Code Review: Step 3: Cluster C — `taskplane doctor` empty pi version + +### Verdict: APPROVE + +### Summary +The Step 3 changes correctly resolve the diagnostic UX bug: `getVersion()` now captures both stdout and stderr while preserving null-on-failure semantics for non-zero exits. The extraction to `bin/get-version.mjs` is clean and testability improved; the new behavioral test suite directly exercises the expected success and failure paths. I re-ran the new test file and `node bin/taskplane.mjs doctor`; output now shows `✅ pi installed (0.73.0)` as intended. + +### Issues Found +1. None. + +### Pattern Violations +- None observed. + +### Test Gaps +- None blocking for this step’s scope. + +### Suggestions +- Minor: consider adding one tiny inline comment in `cmdDoctor()` near the `pi installed` check noting that some CLIs emit version info on stderr, to document why `getVersion()` uses stdout-precedence with stderr fallback. +- Quality-check note: no configured typecheck/lint/format-check commands were discovered (`.pi/taskplane-config.json` / legacy runner config absent in this worktree, and root `package.json` has no `scripts` entries for `typecheck`, `lint`, or `format:check`), so static quality checks were not run. diff --git a/taskplane-tasks/TP-189-polish-bundle/.reviews/R010-plan-step5.md b/taskplane-tasks/TP-189-polish-bundle/.reviews/R010-plan-step5.md new file mode 100644 index 00000000..817c34a9 --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.reviews/R010-plan-step5.md @@ -0,0 +1,16 @@ +## Plan Review: Step 5: Cluster E — Worker prompt + skill reconciliation + +### Verdict: APPROVE + +### Summary +The Step 5 plan is appropriately scoped to the two required outcomes in PROMPT.md: reconciling ambiguous guidance in `templates/agents/task-worker.md` and documenting per-step vs. consolidated review behavior in `skills/create-taskplane-task/SKILL.md`. It also uses the right discovery-first workflow for the `⚠️ Hydrate` portion and explicitly requires rationale logging in STATUS.md, which matches the task’s auditability goals. I don’t see blocking gaps that would prevent this step from achieving its stated outcomes. + +### Issues Found +None. + +### Missing Items +- None. + +### Suggestions +- Add one explicit verification checkbox in Step 5 confirming that the TP-186 “Order of Operations” and “Recovery” sections were not edited (per PROMPT Do-NOT guardrail), only referenced/reconciled from older sections. +- After edits, include a short STATUS.md note listing exact headings changed in `task-worker.md` to make reviewer diff validation faster. diff --git a/taskplane-tasks/TP-189-polish-bundle/.reviews/R011-code-step5.md b/taskplane-tasks/TP-189-polish-bundle/.reviews/R011-code-step5.md new file mode 100644 index 00000000..0dd2e2d6 --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.reviews/R011-code-step5.md @@ -0,0 +1,18 @@ +## Code Review: Step 5: Cluster E — Worker prompt + skill reconciliation + +### Verdict: REVISE + +### Summary +The `SKILL.md` addition is solid and matches the requested per-step vs. consolidated review clarification, and the `task-worker.md` commit-section rewrite usefully aligns commit wording with TP-186. However, the Step 5 reconciliation is incomplete: the Resume Algorithm still contains the old “checkboxes done → next step” instruction without Level 2/3 review-gating context. No typecheck/lint/format-check commands were configured in `.pi/taskplane-config.json` (not present) or available `package.json` scripts, so quality checks were skipped per policy. + +### Issues Found +1. **[templates/agents/task-worker.md:36] [important]** — The previously identified conflict in Resume Algorithm step 6 is still present verbatim: “When a step's items are all checked, proceed to the next incomplete step.” This contradicts the code-review-gated completion contract for Review Level ≥2 and leaves exactly the ambiguity this step was supposed to reconcile. **Fix:** rewrite this line to be level-aware (0/1 can proceed; 2/3 must commit implementation + run `review_step(type="code")` + only proceed after APPROVE), or add an explicit cross-reference to the Order-of-Operations section before advancing. + +### Pattern Violations +- None. + +### Test Gaps +- No automated assertion currently guards the Resume Algorithm wording against drifting back into pre-TP-186 semantics; consider extending existing prompt-template text assertions to cover this clause after the fix. + +### Suggestions +- After applying the missing Resume Algorithm edit, update the Step 5 STATUS entry so the “2 surgical edits applied” note exactly matches what is on disk. diff --git a/taskplane-tasks/TP-189-polish-bundle/.reviews/R012-code-step5.md b/taskplane-tasks/TP-189-polish-bundle/.reviews/R012-code-step5.md new file mode 100644 index 00000000..3ac1ac01 --- /dev/null +++ b/taskplane-tasks/TP-189-polish-bundle/.reviews/R012-code-step5.md @@ -0,0 +1,18 @@ +## Code Review: Step 5: Cluster E — Worker prompt + skill reconciliation + +### Verdict: APPROVE + +### Summary +This revision addresses the prior Step 5 gap: `templates/agents/task-worker.md` now makes Resume Algorithm step 6 explicitly Review-Level-aware and cross-references the Order of Operations rule for Level 2/3 gating. The `skills/create-taskplane-task/SKILL.md` addition clearly documents the per-step default versus checkpoint-marker consolidation pattern, including when each is appropriate and a concrete TP-186 example. Quality-check discovery found no configured `typecheck`/`lint`/`format:check` commands in `.pi/taskplane-config.json`/legacy runner config (not present) and no fallback scripts in root `package.json`, so static quality checks were skipped per policy. + +### Issues Found +None. + +### Pattern Violations +- None. + +### Test Gaps +- None blocking. A regression guard was added in `extensions/tests/worker-step-completion-protocol.test.ts` (`1.4b`) and passes. + +### Suggestions +- Optional clarity tweak: in the “Git commits (after completing a STEP)” Level 2/3 paragraph, include `step=N` in the inline `review_step` call example for consistency with the Order-of-Operations section. diff --git a/taskplane-tasks/TP-189-polish-bundle/PROMPT.md b/taskplane-tasks/TP-189-polish-bundle/PROMPT.md index 8e5f99d7..1ec0ccd5 100644 --- a/taskplane-tasks/TP-189-polish-bundle/PROMPT.md +++ b/taskplane-tasks/TP-189-polish-bundle/PROMPT.md @@ -56,11 +56,13 @@ Net: **8 items active** across 4 implementation clusters (A, B, C, E). Cluster D ## Dependencies -- **TP-186** must be merged (it is — shipped in v0.28.6). Cluster A items 2 and 3 reference the `isStepMarkedComplete` helper added there. -- **TP-184** must be merged (it is — shipped in v0.28.5). Cluster A item 1 and Cluster B reference `buildWorkerToolsAllowlist` added there. -- **TP-185** must be merged (it is — shipped in v0.28.5). Cluster C is a follow-up to that fix. -- **TP-188** must be merged (it is — shipped in v0.28.8). Cluster A item 4 references the `removeWorktree` + `runWindowsCmdRd` helpers added there. -- **No dependencies on TP-187** — that's an independent later task. +**None** — all referenced predecessor tasks are already merged. The following are informational cross-references for context, NOT runtime dependencies (the discovery parser skips dep extraction when this section starts with **None**, so these notes are safe even though TP-185's task folder has since been archived): + +- TP-186 (shipped v0.28.6): Cluster A items 2 and 3 reference the `isStepMarkedComplete` helper added there. +- TP-184 (shipped v0.28.5): Cluster A item 1 and Cluster B reference `buildWorkerToolsAllowlist` added there. +- TP-185 (shipped v0.28.5; folder archived): Cluster C is a follow-up to that diagnostic-UX fix. +- TP-188 (shipped v0.28.8): Cluster A item 4 references the `removeWorktree` + `runWindowsCmdRd` helpers added there. +- TP-187 (independent later task): no relationship; runs in the same batch as TP-189 only because they're both queued. ## Context to Read First diff --git a/taskplane-tasks/TP-189-polish-bundle/STATUS.md b/taskplane-tasks/TP-189-polish-bundle/STATUS.md index 4b1ee487..81985473 100644 --- a/taskplane-tasks/TP-189-polish-bundle/STATUS.md +++ b/taskplane-tasks/TP-189-polish-bundle/STATUS.md @@ -1,11 +1,11 @@ # TP-189: Accumulated polish bundle — Status -**Current Step:** Not Started -**Status:** 🔵 Ready for Execution -**Last Updated:** 2026-05-06 +**Current Step:** Step 7: Documentation & Delivery +**Status:** ✅ Complete +**Last Updated:** 2026-05-07 (task complete) **Review Level:** 2 -**Review Counter:** 0 -**Iteration:** 0 +**Review Counter:** 12 +**Iteration:** 1 **Size:** L > **Hydration:** Checkboxes represent meaningful outcomes, not individual code @@ -21,50 +21,53 @@ --- ### Step 0: Preflight -**Status:** ⬜ Not Started +**Status:** ✅ Complete -- [ ] On topic branch (e.g., `chore/tp-189-polish-bundle`) -- [ ] Working tree clean -- [ ] Baseline test count recorded (post-v0.28.8: should be 3496+) -- [ ] All Tier 3 context files read per cluster -- [ ] Decision: Cluster B re-export strategy — direct import only, or also re-export from `agent-host.ts` +- [x] On topic branch (lane branch `task/henrylach-lane-2-20260506T230236`) +- [x] Working tree clean (only STATUS.md modified) +- [x] Baseline test count recorded: **3496 pass, 1 skipped, 0 fail** (post-v0.28.8 confirmed) +- [x] All Tier 3 context files read per cluster (agent-host.ts, config-schema.ts, types.ts, lane-runner.ts spawn site, agent-bridge-extension.ts review_step + isStepMarkedComplete, bin/taskplane.mjs getVersion, worktree.ts removeWorktree + helpers, existing TP-184/186/188 test files, task-worker.md, SKILL.md Review Levels rubric) +- [x] Decision: Cluster B — NEW constants module exports `DEFAULT_WORKER_USER_TOOLS` only (not `ENGINE_BRIDGE_TOOLS`); `agent-host.ts` re-exports for backward compatibility (execution.ts and worker-tools-allowlist.test.ts already import from agent-host.ts — don't break) - [N/A] ~~Decision: Cluster D — local Node 24 smoke run before bumping ci.yml~~ — Cluster D shipped in v0.28.8 (commit `96a457f5`) --- ### Step 1: Cluster A — Defensive tests + helper hardening -**Status:** ⬜ Not Started +**Status:** ✅ Complete -- [ ] Item 1: `lane-runner-spawn-wiring.test.ts` (NEW) — static assertion -- [ ] Item 2: `review-step-guard-runtime.test.ts` (NEW) — runtime test of REFUSED path (3 sub-cases: code blocked, test blocked, plan NOT blocked) -- [ ] Item 3: `isStepMarkedComplete` ignores fenced code blocks; matching test added -- [ ] Item 4 (sage TP-188 follow-up): behavioral tests for `removeWorktree()` Windows fallback in `windows-worktree-cleanup-fallback.test.ts` (3 sub-cases: MAX_PATH on win32 → fallback fires; non-MAX_PATH on win32 → fallback skipped; non-win32 + MAX_PATH text → fallback skipped) -- [ ] Targeted run passes for all four new/modified tests +- [x] Item 1: `lane-runner-spawn-wiring.test.ts` (NEW) — static assertion (4 tests pass) +- [x] Item 2: `review-step-guard-runtime.test.ts` (NEW) — 4 runtime tests pass: type='code' on Complete → REFUSED + no spawn + counter unchanged; type='plan' on Complete → NOT refused; type='code' on In-Progress → NOT refused; REFUSED text matches prompt Recovery Recipe wording. Uses bare-specifier `child_process` mock for Node 22/24 portability. +- [x] Item 3: `isStepMarkedComplete` now skips ``` and ~~~ fenced code blocks; 4 new test cases (2.8–2.11) cover triple-backtick, tilde fence, regression for real-status-after-fence, and unclosed-fence cross-call isolation. All pass. +- [x] Item 4 (sage TP-188 follow-up): NEW `extensions/tests/windows-worktree-cleanup-behavioral.test.ts` with 3 behavioral decision-branch tests. Uses single `child_process` mock that dispatches on cmd (git vs cmd) and uses real on-disk temp directories (no fs mocking). All 3 pass: 4.1 win32+MAX_PATH → cmd rd fires + prune-after-rd ordering verified; 4.2 win32+non-MAX_PATH → fallback skipped, WORKTREE_REMOVE_FAILED thrown with original stderr; 4.3 non-win32+MAX_PATH text → platform guard skips fallback. (Created as a sibling file rather than adding to the existing fallback test file because the new tests need a richer cmd/git dispatcher than the existing single-fixture mock supports.) +- [x] Targeted run passes: `lane-runner-spawn-wiring` (4) + `review-step-guard-runtime` (4) + `worker-step-completion-protocol` (19, includes 4 new fence-block cases) + `windows-worktree-cleanup-behavioral` (3) = 30 tests, all green. +- [x] **R002 fix 1 (important):** `isStepMarkedComplete` now tracks fence opener char + length (CommonMark semantics) — only closes on a matching delimiter (same char, length ≥ opener length). Step-heading detection is also gated on being outside a fence. +- [x] **R002 fix 2 (important):** added `type='test'` REFUSED runtime test (5th test in the file) — verifies REFUSED token, no spawn, counter unchanged, and that the refusal message echoes `type="test"` in the re-call instruction. +- [x] **R002 test gap:** added 2 new regression cases (2.12, 2.13) to `worker-step-completion-protocol.test.ts` — mixed-delimiter (4-backtick fence containing `~~~` and ``` examples + literal Status line, expects false) and equal-length closer (CommonMark length≥opener semantics). --- ### Step 2: Cluster B — Constants module migration -**Status:** ⬜ Not Started +**Status:** ✅ Complete -- [ ] `extensions/taskplane/tool-allowlist-constants.ts` (NEW) — single source of truth -- [ ] `agent-host.ts` imports from new module (re-exports per Step 0 decision) -- [ ] `config-schema.ts` and `types.ts` literals replaced with import; annotation comments removed -- [ ] No circular imports (verified via import probe) -- [ ] Full fast suite still passes (no behavior change) +- [x] `extensions/taskplane/tool-allowlist-constants.ts` (NEW, 38 lines) — single source of truth, no imports beyond TS built-ins (verified) +- [x] `agent-host.ts` re-exports `DEFAULT_WORKER_USER_TOOLS` from the new module via `export { ... } from` plus a local `import` for in-file usage; existing internal callers (`execution.ts`, `worker-tools-allowlist.test.ts`) continue to work unchanged +- [x] `config-schema.ts` (worker.tools default + merge.tools default) and `types.ts` (merge.tools default) now reference `DEFAULT_WORKER_USER_TOOLS` via direct import; obsolete TP-184 NOTE comments replaced with TP-189 (Cluster B) comments explaining the new sourcing +- [x] No circular imports: `node -e "await import('./taskplane/types.ts'); await import('./taskplane/config-schema.ts'); await import('./taskplane/agent-host.ts'); await import('./taskplane/tool-allowlist-constants.ts')"` succeeds +- [x] `worker-tools-allowlist.test.ts` (16 tests) still passes — the constant value is unchanged, only its source module moved --- ### Step 3: Cluster C — taskplane doctor empty pi version -**Status:** ⬜ Not Started +**Status:** ✅ Complete -- [ ] `getVersion()` in `bin/taskplane.mjs` captures both stdout and stderr -- [ ] Manual: `node bin/taskplane.mjs doctor` shows pi version (e.g., `0.73.x`) -- [ ] Optional: `cli-doctor-version-capture.test.ts` (skip if testability awkward) +- [x] `getVersion()` extracted to NEW `bin/get-version.mjs` (testable ESM helper) and imported from `bin/taskplane.mjs`. Uses `spawnSync` with `stdio:['ignore','pipe','pipe']`, stdout-precedence with stderr fallback, AND **R008 fix**: gates on `result.error || result.status !== 0` so non-zero exits return null instead of leaking shell error text as a fake version string (preserves the prior execSync-throws-on-failure contract). +- [x] Manual: `node bin/taskplane.mjs doctor` now shows `✅ pi installed (0.73.0)` (was empty parens). +- [x] `cli-doctor-version-capture.test.ts` (NEW, 7 BEHAVIORAL tests): exercises the helper with real `node -e ...` subprocesses. Covers stdout success, stderr fallback (the pi case), stdout-over-stderr precedence, trimming, non-zero-exit returns null (R008 regression), nonexistent command returns null, both-empty-on-success returns null. --- ### Step 4: Cluster D — CI Node 24 alignment -**Status:** ✅ Already shipped in v0.28.8 (commit `96a457f5`) +**Status:** ✅ Complete - [x] ~~Local smoke: `npm run test:fast` on Node 24 passes~~ — done during v0.28.8 release validation - [x] ~~`ci.yml` `node-version: "22"` → `"24"`~~ — done in commit `96a457f5` @@ -75,35 +78,46 @@ --- ### Step 5: Cluster E — Worker prompt + skill reconciliation -**Status:** ⬜ Not Started +**Status:** ✅ Complete > ⚠️ Hydrate: specific edits depend on Discovery-pass findings. -- [ ] Item 7 Discovery: grep `task-worker.md` for checkbox/step-transition keywords; identify potential conflicts with new Order of Operations -- [ ] Item 7: per-section decisions documented in Discoveries (rewrite/cross-reference/leave as-is) -- [ ] Item 7: edits applied -- [ ] Item 8: `SKILL.md` Review Levels rubric augmented with per-step vs. consolidated pattern documentation -- [ ] Item 8: TP-186 referenced as a real consolidation example +- [x] Item 7 Discovery: grepped `task-worker.md`. Two real conflicts identified (other matches are consistent with the new rule): + 1. **Resume Algorithm step 6** — "all items checked → next step" doesn't account for Level ≥ 2's APPROVE-gating. + 2. **Checkpoint Discipline / Git commits** — example commit `"complete Step N — description"` conflicts with the Order of Operations example commit `"step N implementation"`. + All other matches (Resume Algorithm step 7 "top-of-file Status", Scope Rules "complete each step", Error Handling general checkbox advice) are consistent and need no change. +- [x] Item 7: per-section decisions documented in Discoveries. +- [x] Item 7: 2 surgical edits applied to `templates/agents/task-worker.md`: + 1. Resume Algorithm step 6 split into Level 0/1 (proceed) vs Level 2/3 (commit + code review + APPROVE-gated status flip), with explicit cross-reference to **Order of Operations**. + 2. Checkpoint Discipline / Git commits — example commit message rewritten to `"step N implementation"`; new paragraphs distinguish Level 0/1 (commit completes the step) from Level 2/3 (commit is impl-only, separate post-APPROVE `"step N complete (code review APPROVE)"` commit). + Existing `worker-step-completion-protocol.test.ts` (23 substring assertions) still passes — the new paragraphs reuse the canonical wording from the Order of Operations + Recovery Recipe sections. +- [x] Item 8: `SKILL.md` Complexity Assessment augmented with new sub-section **"Per-Step Reviews vs. Consolidated Reviews (Checkpoint Markers)"**. Documents: default per-step behavior (e.g., 5-impl-step Level 2 task → ~10 reviews), opt-in consolidation via `**Plan-review checkpoint**` / `**Code review checkpoint**` markers, when-to-use-which guidance, and a PROMPT-authoring decision heuristic. +- [x] Item 8: TP-186 referenced as the canonical consolidation example (1 design + 3 mechanical impl + 1 verify → 2 reviews total instead of ~8). --- ### Step 6: Testing & Verification -**Status:** ⬜ Not Started +**Status:** ✅ Complete > ZERO test failures allowed. -- [ ] FULL fast suite passing (count = baseline + new tests from A/C) -- [ ] Integration suite passing -- [ ] CLI smoke clean; doctor shows pi version -- [ ] No circular imports (re-verified) +- [x] FULL fast suite passing: **3524 pass / 1 skipped / 0 fail** (baseline 3496 + 28 new tests from Clusters A and C). Worktree Lifecycle Harness: 69/69 pass. +- [x] Integration suite passing: combined `tests/*.test.ts tests/*.integration.test.ts` run — 3524 pass / 1 skipped / 0 fail (the `*.test.ts` glob already includes `.integration.test.ts` files). +- [x] CLI smoke clean: `taskplane help` shows the v0.28.8 banner; `taskplane doctor` now shows `✅ pi installed (0.73.0)` (Cluster C verified end-to-end on a real machine). +- [x] No circular imports: import probe (`types.ts`, `config-schema.ts`, `agent-host.ts`, `tool-allowlist-constants.ts`) succeeds. +- [N/A] ~~Verify Cluster D CI change works on the PR's CI run~~ — Cluster D was already shipped in v0.28.8 (PR #552 + #554). --- ### Step 7: Documentation & Delivery -**Status:** ⬜ Not Started +**Status:** ✅ Complete -- [ ] CHANGELOG entries categorized: Internal (A1-2, B, D), Fixed (A3, C), Docs (E) -- [ ] Discoveries logged (especially Cluster E per-section rationale) +- [x] CHANGELOG entries added under [Unreleased], categorized: + - **Fixed** (user-visible): TP-189-C `taskplane doctor` empty pi version; TP-189-A3 `isStepMarkedComplete` fenced-code-block filter + - **Docs** (user-visible): TP-189-E task-worker.md reconciliation; SKILL.md per-step vs. consolidated review pattern + - **Internal** (refactors / regression tests): TP-189-B constants module migration; TP-189-A1 spawn-site regression guard; TP-189-A2 REFUSED-path runtime test; TP-189-A4 removeWorktree behavioral tests + - Cluster D was already shipped in v0.28.8 — no entry needed. +- [x] Discoveries logged in STATUS.md (Cluster B decision rationale + Cluster E per-section conflict analysis). --- @@ -118,6 +132,10 @@ | Discovery | Disposition | Location | |-----------|-------------|----------| +| Cluster B: New module `tool-allowlist-constants.ts` exports only `DEFAULT_WORKER_USER_TOOLS`; `ENGINE_BRIDGE_TOOLS` stays in `agent-host.ts` (no duplication problem there). `agent-host.ts` re-exports `DEFAULT_WORKER_USER_TOOLS` from the new module for backward compatibility (existing imports in `execution.ts` and `worker-tools-allowlist.test.ts` continue to work). | Decision — directs Step 2 implementation | `extensions/taskplane/{tool-allowlist-constants.ts (new), agent-host.ts, config-schema.ts, types.ts}` | +| Cluster B: `config-schema.ts` is currently import-free; `types.ts` imports only from `path` and `./diagnostics.js`. Neither module pulls `child_process`/`fs`. Importing `DEFAULT_WORKER_USER_TOOLS` from a new pure-data module (no imports) is safe — no circular import risk because the new module imports nothing. | Verified safe | (verified via `head -25` + `grep -n "^import"`) | +| Baseline test count: **3496 pass / 1 skipped / 0 fail** post-v0.28.8 (PROMPT predicted 3496+; matches). | Baseline — final count should be 3496 + new tests from Clusters A and C (4-7 new). | n/a | +| Cluster E Discovery: only 2 sections in `task-worker.md` create mental dissonance with the TP-186 Order of Operations rule. (1) Resume Algorithm step 6 ("all items checked → proceed to next step") doesn't cross-reference the review-gated completion requirement for Level ≥ 2. (2) The example commit message in Checkpoint Discipline says `complete Step N — description` which conflicts with the Order of Operations example commit message (`step N implementation`). | Two surgical edits planned (cross-reference + rewrite). | `templates/agents/task-worker.md` lines 36, 128–132 | --- @@ -126,6 +144,10 @@ | Timestamp | Action | Outcome | |-----------|--------|---------| | 2026-05-06 | Task staged | PROMPT.md and STATUS.md created | +| 2026-05-07 03:02 | Task started | Runtime V2 lane-runner execution | +| 2026-05-07 03:02 | Step 0 started | Preflight | +| 2026-05-07 03:47 | Worker iter 1 | done in 2685s, tools: 186 | +| 2026-05-07 03:47 | Task complete | .DONE created | --- @@ -145,3 +167,15 @@ - Per-step reviews are the deliberate choice (not consolidation) because the clusters are independent and Cluster E specifically documents the per-step default. +| 2026-05-07 03:07 | Review R001 | plan Step 1: APPROVE | +| 2026-05-07 03:17 | Review R002 | code Step 1: REVISE | +| 2026-05-07 03:21 | Review R003 | code Step 1: REVISE | +| 2026-05-07 03:24 | Review R004 | code Step 1: APPROVE | +| 2026-05-07 03:25 | Review R005 | plan Step 2: APPROVE | +| 2026-05-07 03:28 | Review R006 | code Step 2: APPROVE | +| 2026-05-07 03:29 | Review R007 | plan Step 3: APPROVE | +| 2026-05-07 03:32 | Review R008 | code Step 3: REVISE | +| 2026-05-07 03:34 | Review R009 | code Step 3: APPROVE | +| 2026-05-07 03:36 | Review R010 | plan Step 5: APPROVE | +| 2026-05-07 03:40 | Review R011 | code Step 5: REVISE | +| 2026-05-07 03:42 | Review R012 | code Step 5: APPROVE | diff --git a/taskplane-tasks/TP-190-runtime-v2-spawn-failure-visibility/PROMPT.md b/taskplane-tasks/TP-190-runtime-v2-spawn-failure-visibility/PROMPT.md new file mode 100644 index 00000000..1a968a71 --- /dev/null +++ b/taskplane-tasks/TP-190-runtime-v2-spawn-failure-visibility/PROMPT.md @@ -0,0 +1,198 @@ +# Task: TP-190 - Surface Runtime V2 spawn-failure errors so the dashboard doesn't silently hang + +**Created:** 2026-05-09 +**Size:** M + +## Review Level: 2 (Plan and Code) + +**Assessment:** Touches the engine's lane state machine + IPC alert pipeline. New exit category (`spawn-failure`) becomes part of the public exit contract. Wiring must interact correctly with the existing retry/recovery logic without accidentally retrying spawn failures (which are not transient). Plan review catches the architectural choices (where to wrap, how to classify, retry policy); code review catches the actual state-transition correctness and alert delivery. Both warranted. +**Score:** 3/8 — Blast radius: 1, Pattern novelty: 1, Security: 0, Reversibility: 1 + +## Canonical Task Folder + +``` +taskplane-tasks/TP-190-runtime-v2-spawn-failure-visibility/ +├── PROMPT.md ← This file (immutable above --- divider) +├── STATUS.md ← Execution state (worker updates this) +├── .reviews/ ← Reviewer output (created by the orchestrator runtime) +└── .DONE ← Created when complete +``` + +## Mission + +Fix [#561](https://github.com/HenryLach/taskplane/issues/561): when a Runtime V2 lane spawn fails (e.g., Pi CLI not findable, worktree provisioning error, branch collision), the lane is **not** transitioned to `failed`. The engine continues polling indefinitely, the dashboard shows green/running lanes that have no actual worker process, `orch_status()` reports `running`, and no supervisor alert fires. Recovery requires the operator to manually `tail` engine-worker stderr — which is not in any of the documented diagnostic places. + +This bug **masked** the impact of #559 (orchestrator crash on first IPC) and #560 (`@earendil-works` rename), making both look like "the orchestrator is hung" rather than "spawn errored 3× immediately." Fixing it converts every future spawn-stage breakage into a visible, actionable failure instead of a silent hang. + +The fix has four parts (per the issue body's suggested outline): + +1. **Wrap `executeLaneV2` per-lane in the engine** so any thrown error from spawn: + - Sets `lane.status = "failed"` with the error message in `lane.lastError`. + - Sets each task on that lane to `failed` with `exitReason = "spawn failure: "` and a NEW `exitCategory = "spawn-failure"` (alongside existing categories like `crashed`, `stalled`). + - Decrements active-lane counter. + - Emits an IPC supervisor alert (`task-failure`) so the supervisor can react via the existing playbook. The summary should include the underlying error message verbatim — for spawn failures, the message itself is usually the diagnosis (e.g., "Cannot find Pi CLI entrypoint"). + +2. **Don't retry spawn failures.** Existing recovery playbooks treat `task-failure` as "retry once or twice, then escalate." For spawn failures that's wrong — none of them are transient. The new `spawn-failure` category lets the engine (and supervisor playbook) escalate immediately rather than retry-and-fail-N-times. + +3. **Surface in `orch_status` / `list_active_agents`.** When all lanes in a wave fail to spawn, `phase` should NOT stay `executing` — it should transition to `paused` or `failed` so the operator's first instinct ("how's it going?") gives them a meaningful answer instead of "running". Cross-check with `list_active_agents()` which already reports the registry as empty. + +4. **Add a behavioral regression test.** Mock `resolvePiCliPath()` (or the spawn helper) to throw, run a one-task batch, assert: lane status becomes `failed`, task status becomes `failed`, `failedTasks === 1`, an IPC alert fires, and `phase !== "executing"` after the poll loop next ticks. + +## Dependencies + +**None** — all referenced predecessor tasks are already merged. The following are informational cross-references for context, NOT runtime dependencies (the discovery parser skips dep extraction when this section starts with **None**): + +- TP-187 (shipped via PR #556): introduced `task-failure` IPC alert pipeline and supervisor-side handlers. This task extends that pipeline to spawn-stage failures. +- TP-188 (shipped v0.28.8): no relationship beyond shared error-classification patterns. +- Issue #560 (fixed in PR #556 commit `34b303a`): the rename bug was the most recent trigger for #561's silent-failure symptom. Fixing #560 narrows the failure surface but does NOT address visibility — that's this task's job. +- Issue #559 (fixed in PR #556 commit `ff02265`): the IPC closure crash was an earlier trigger for the same silent-failure symptom. + +## Context to Read First + +**Tier 2 (area context):** +- `taskplane-tasks/CONTEXT.md` + +**Tier 3 (load only if needed):** +- Issue body for #561: `gh issue view 561` — has the full operator-side symptom, repro, and suggested fix outline. +- `extensions/taskplane/execution.ts` — lines ~1855-1875 (`executeLaneV2` orchestration in `executeWave`), lines ~2715-2735 (per-task try/catch inside `executeLaneV2` itself; note this catches some spawn errors at task level but the LANE-level state machine doesn't react). +- `extensions/taskplane/engine.ts` — search for `executeWave` / `monitorLanes` call sites to find where lane outcomes are read and where `task-failure` alerts are emitted today. +- `extensions/taskplane/types.ts` — find the existing `LaneTaskStatus` or `ExitCategory` enums; note where `crashed`, `stalled`, etc. are declared so the new `spawn-failure` category lands in the canonical place. +- `extensions/taskplane/extension.ts` lines ~3088-3140 (the existing hard-fail `task-failure` alert emission in TP-187's supervisor recovery flows) — model the spawn-failure alert on the same shape. +- `extensions/tests/supervisor-recovery-flows.test.ts` — existing test pattern for IPC alert assertions; reuse the harness if possible. +- `.pi/runtime//registry.json` semantics: workers register on spawn; if spawn fails, the registry is empty (this is what `list_active_agents()` correctly reports). The lane-state machine and the registry are out of sync today — that's part of what's broken. + +## Environment + +- **Workspace:** `extensions/taskplane/` (engine + execution + IPC layer) +- **Services required:** None + +## File Scope + +> The orchestrator uses this to avoid merge conflicts: tasks with overlapping +> file scope run on the same lane (serial), not in parallel. List the files and +> directories this task will create or modify. Use wildcards for directories. + +- `extensions/taskplane/engine.ts` (the lane orchestration site that reads outcomes from `executeWave`) +- `extensions/taskplane/execution.ts` (the per-task try/catch inside `executeLaneV2` — may need to either propagate up or supplement) +- `extensions/taskplane/types.ts` (new `spawn-failure` exit category; possibly new `lane.lastError` field) +- `extensions/taskplane/persistence.ts` (if `spawn-failure` needs to serialize differently from existing categories — likely just additive) +- `extensions/taskplane/extension.ts` (supervisor-side alert handling — should already accept `task-failure` alerts; verify it routes spawn-failure specifically without retry) +- `extensions/tests/spawn-failure-visibility.test.ts` (NEW — behavioral regression test) +- `CHANGELOG.md` (Fixed entry under [Unreleased]) + +## Steps + +> **Hydration:** STATUS.md tracks outcomes, not individual code changes. Workers +> expand steps when runtime discoveries warrant it. See task-worker agent for rules. + +### Step 0: Preflight + +- [ ] On `main` (lane worktree) +- [ ] Baseline test count recorded (post-PR-#556: should be 3587 passing / 1 skipped / 0 failed) +- [ ] `gh issue view 561` read in full +- [ ] Tier 3 context files read per scope +- [ ] Decision recorded: where to wrap `executeLaneV2` (engine.ts caller vs. execution.ts internal vs. both layers) + +### Step 1: Plan all four parts of the fix + +> ⚠️ Plan-review checkpoint. Reviewer evaluates architectural choices. + +- [ ] **Part 1 design** (state-transition wiring): document where the new try/catch lives. Trace the existing flow: `executeWave` → `lanePromises` → `executeLaneV2` (which has its own per-task try/catch around line ~2715-2735). Decide whether the lane-level wrap is needed in `executeWave`, or whether `executeLaneV2`'s existing catch is sufficient and the bug is elsewhere (e.g., monitor not reacting to the failed outcome). The user's repro shows the existing catch at line 2724 IS firing (the stderr line `Runtime V2 execution error: ...` comes from there) — but the lane-state machine and monitor still report "running". So the bug is likely downstream of `executeLaneV2`'s catch, NOT in adding a new catch around it. +- [ ] **Part 2 design** (no-retry policy for spawn failures): document where the new `spawn-failure` category is checked. Existing retry budgets (`TIER0_RETRY_BUDGETS`) classify retryable failures; spawn failures must be excluded. +- [ ] **Part 3 design** (orch_status / phase transition): when all active lanes in a wave fail to spawn, what should `phase` become? `paused`? `failed`? Document the choice and the trigger condition. +- [ ] **Part 4 design** (regression test): outline the test harness — how to mock the spawn helper to throw, how to assert the lane state, how to assert the IPC alert fires. +- [ ] Drafts in Discoveries section of STATUS.md. + +### Step 2: Implement Part 1 — state-transition + IPC alert + +> Plan-reviewer must have APPROVED Step 1 before proceeding. + +- [ ] Add `"spawn-failure"` to the `ExitCategory` enum (or equivalent) in `types.ts`. +- [ ] Wire the new category into the `task-failure` IPC alert payload so the supervisor can route it specifically. +- [ ] Verify `lane.status` transitions to `failed` and `task.status` transitions to `failed` via existing serialization paths (probably no new code in `persistence.ts`). +- [ ] Run targeted tests: existing supervisor-recovery-flows tests should still pass; no new tests yet. + +### Step 3: Implement Part 2 — no-retry for spawn failures + +- [ ] Locate retry classification (likely `TIER0_RETRY_BUDGETS` in `types.ts` or a sibling helper). +- [ ] Add `"spawn-failure"` to the non-retryable set; document inline. +- [ ] Run targeted tests. + +### Step 4: Implement Part 3 — phase transition when all lanes spawn-fail + +- [ ] Determine where the wave/phase decision happens in `engine.ts` (likely in `executeWave`'s post-allocation logic or in `monitorLanes`). +- [ ] Add the transition: when every lane in a wave has `status === "failed"` with `exitCategory === "spawn-failure"`, transition `batchState.phase` from `"executing"` to `"failed"` (NOT `"paused"` — the operator can't unstick this without changing something). +- [ ] Verify `orch_status` text reflects the new phase. +- [ ] Run targeted tests. + +### Step 5: Add behavioral regression test + +> Code-review checkpoint after this step (plan reviewer should be satisfied with the architecture; code reviewer evaluates the actual implementation). + +- [ ] Create NEW `extensions/tests/spawn-failure-visibility.test.ts`. +- [ ] Test cases (each uses a mocked `resolvePiCliPath` or spawn helper that throws): + - Single-task batch: spawn fails → lane.status === "failed", task.status === "failed", failedTasks === 1, an IPC alert fires with category="spawn-failure", phase !== "executing". + - Multi-task batch where ALL lanes fail to spawn → batchState.phase === "failed". + - Spawn failure does NOT trigger retry: the same task is not re-spawned (assert mock called exactly once per task, not twice or three times). +- [ ] Run the new test in isolation; then run the full fast suite. + +### Step 6: Testing & Verification + +> ZERO test failures allowed. This step runs the FULL test suite as a quality gate. + +- [ ] Run FULL fast suite: `cd extensions && npm run test:fast` — should pass with the new test included (target: 3590+ passing, +3 from this task). +- [ ] Run integration suite: `cd extensions && npm run test` — full suite must pass. +- [ ] CLI smoke clean: `node bin/taskplane.mjs help` and `node bin/taskplane.mjs doctor` work. +- [ ] No circular imports introduced (probe `types.ts` ↔ new code). + +### Step 7: Documentation & Delivery + +- [ ] CHANGELOG entry under [Unreleased] → Fixed: + - Title: `**Runtime V2 spawn failures now visible (TP-190, #561)**` + - Body: 1-2 paragraph summary covering: symptom (silent hang on dashboard), root cause (lane state machine didn't react to spawn-time errors), fix (new `spawn-failure` exit category + state transition + IPC alert + no-retry policy + phase=failed when all lanes spawn-fail), validation (regression test + cross-platform Node 24 CI). +- [ ] Discoveries logged in STATUS.md. +- [ ] Step boundaries committed with `feat(TP-190): ...` / `test(TP-190): ...` / `docs(TP-190): ...` prefixes. + +## Documentation Requirements + +**Must Update:** +- `CHANGELOG.md` — add Fixed entry per Step 7 + +**Check If Affected:** +- `docs/explanation/architecture.md` — if it documents the lane state machine, may need a brief mention of the new `spawn-failure` exit category +- `docs/reference/commands.md` — if `orch_status` output formatting documentation describes the `phase` values, ensure `failed` is mentioned (probably already is) + +## Completion Criteria + +- [ ] All 4 parts of the fix implemented (state-transition, no-retry, phase transition, behavioral test) +- [ ] All tests passing (target: 3590+ passing / 1 skipped / 0 failed) +- [ ] CHANGELOG entry added +- [ ] Per-step plan + code reviews completed and APPROVE'd + +## Git Commit Convention + +Commits happen at **step boundaries** (not after every checkbox). All commits +for this task MUST include the task ID for traceability: + +- **Step completion:** `feat(TP-190): complete Step N — description` +- **Bug fixes:** `fix(TP-190): description` +- **Tests:** `test(TP-190): description` +- **Hydration:** `hydrate: TP-190 expand Step N checkboxes` + +## Do NOT + +- **Don't add a new try/catch in `executeWave` if the existing one in `executeLaneV2` (line ~2724) already produces the failed outcome.** The bug is downstream of that catch — likely in how the lane state machine and monitor consume the failed outcomes. Verify this in Step 1's discovery before writing new try/catch code. +- **Don't retry spawn failures.** The whole point of the new category is to escalate immediately. Adding it to retry budgets defeats the purpose. +- **Don't expand task scope** — if Step 1 discovery reveals a deeper architectural issue (e.g., lane state machine and Runtime V2 registry are fundamentally out of sync), document in Discoveries and stop. Add tech debt to CONTEXT.md instead of expanding this task. +- **Don't load docs not listed in "Context to Read First."** +- **Don't commit without the `TP-190` prefix.** + +--- + +## Amendments (Added During Execution) + + diff --git a/taskplane-tasks/TP-190-runtime-v2-spawn-failure-visibility/STATUS.md b/taskplane-tasks/TP-190-runtime-v2-spawn-failure-visibility/STATUS.md new file mode 100644 index 00000000..699d235f --- /dev/null +++ b/taskplane-tasks/TP-190-runtime-v2-spawn-failure-visibility/STATUS.md @@ -0,0 +1,162 @@ +# TP-190: Runtime V2 spawn-failure visibility — Status + +**Current Step:** Not Started +**Status:** 🔵 Ready for Execution +**Last Updated:** 2026-05-09 +**Review Level:** 2 +**Review Counter:** 0 +**Iteration:** 0 +**Size:** M + +> **Hydration:** Checkboxes represent meaningful outcomes, not individual code +> changes. Workers expand steps when runtime discoveries warrant it. +> +> **⚠️ Order of Operations rule (live in worker prompt):** do NOT mark a step +> `Complete` until that step's code review has returned APPROVE. This task +> is Review Level 2 — per-step plan + code reviews fire automatically. +> +> **Review structure:** per-step reviews. Expected: ~5 plan + ~5 code = ~10 +> reviews total (with Steps 0/6/7 being lighter). + +--- + +### Step 0: Preflight +**Status:** ⬜ Not Started + +- [ ] On `main` (lane worktree, fresh from PR #556 merge if applicable) +- [ ] Baseline test count recorded (target: 3587 passing / 1 skipped / 0 failed post-PR-#556) +- [ ] `gh issue view 561` read in full +- [ ] All Tier 3 context files read (execution.ts ~1855-1875 + ~2715-2735, engine.ts call sites, types.ts ExitCategory enum, extension.ts ~3088-3140 alert pattern, supervisor-recovery-flows.test.ts harness) +- [ ] Decision: where to wrap `executeLaneV2` — engine.ts caller, execution.ts internal, both, or NOT (if existing catch suffices and bug is downstream) + +--- + +### Step 1: Plan all four parts of the fix +**Status:** ⬜ Not Started + +> ⚠️ Plan-review checkpoint. Reviewer evaluates architectural choices. + +- [ ] Part 1 design (state-transition wiring): document the existing flow trace and the chosen wrap location +- [ ] Part 2 design (no-retry policy): identify the retry classification site and the additive change +- [ ] Part 3 design (phase transition): define the trigger condition (all lanes spawn-fail → phase=failed) and the wiring point +- [ ] Part 4 design (regression test): outline the harness — mock target, assertion shape, IPC alert capture +- [ ] Drafts in Discoveries section below + +--- + +### Step 2: Implement Part 1 — state-transition + IPC alert +**Status:** ⬜ Not Started + +> Plan-reviewer must have APPROVED Step 1 before proceeding. +> ⚠️ Code-review fires after this step. + +- [ ] `"spawn-failure"` added to `ExitCategory` enum in `types.ts` +- [ ] `task-failure` IPC alert payload extended to carry the new category +- [ ] Verified `lane.status` and `task.status` transition to `failed` via existing serialization paths +- [ ] Targeted tests pass (existing supervisor-recovery-flows + any directly-affected tests) + +--- + +### Step 3: Implement Part 2 — no-retry for spawn failures +**Status:** ⬜ Not Started + +> ⚠️ Code-review fires after this step. + +- [ ] Retry classification site located (likely `TIER0_RETRY_BUDGETS` in `types.ts`) +- [ ] `"spawn-failure"` added to non-retryable set with inline rationale comment +- [ ] Targeted tests pass + +--- + +### Step 4: Implement Part 3 — phase transition when all lanes spawn-fail +**Status:** ⬜ Not Started + +> ⚠️ Code-review fires after this step. + +- [ ] Wave/phase decision site located in engine.ts (executeWave post-allocation OR monitorLanes) +- [ ] Transition wired: all lanes failed with `spawn-failure` → `batchState.phase = "failed"` +- [ ] `orch_status()` text confirms the phase change is operator-visible +- [ ] Targeted tests pass + +--- + +### Step 5: Add behavioral regression test +**Status:** ⬜ Not Started + +> ⚠️ Final code-review checkpoint after this step. + +- [ ] NEW `extensions/tests/spawn-failure-visibility.test.ts` created +- [ ] Test 1: single-task spawn failure → lane/task failed, failedTasks===1, IPC alert fires with category="spawn-failure", phase !== "executing" +- [ ] Test 2: multi-task all-fail-spawn → batchState.phase === "failed" +- [ ] Test 3: spawn failure does NOT retry (mock called exactly once per task) +- [ ] Run new test in isolation, then full fast suite + +--- + +### Step 6: Testing & Verification +**Status:** ⬜ Not Started + +> ZERO test failures allowed. + +- [ ] FULL fast suite passing (target: 3590+ passing / 1 skipped / 0 failed) +- [ ] FULL integration suite passing +- [ ] CLI smoke: `node bin/taskplane.mjs help` and `node bin/taskplane.mjs doctor` clean +- [ ] No circular imports + +--- + +### Step 7: Documentation & Delivery +**Status:** ⬜ Not Started + +- [ ] CHANGELOG entry added under [Unreleased] → Fixed (per the wording in PROMPT.md Step 7) +- [ ] Discoveries logged below +- [ ] All commits include `TP-190` prefix; step boundaries clean + +--- + +## Reviews + +| # | Type | Step | Verdict | File | +|---|------|------|---------|------| + +--- + +## Discoveries + +| Discovery | Disposition | Location | +|-----------|-------------|----------| + +--- + +## Execution Log + +| Timestamp | Action | Outcome | +|-----------|--------|---------| +| 2026-05-09 | Task staged | PROMPT.md and STATUS.md created | + +--- + +## Blockers + +*None* + +--- + +## Notes + +**Critical observation from issue #561 repro:** the existing per-task try/catch +inside `executeLaneV2` (execution.ts line ~2724) IS already producing the +"failed" outcome with the spawn error message. The user's stderr log shows +`Runtime V2 execution error: ...` lines — those come from THAT catch. So the +bug is NOT "no try/catch around spawn" — it's "the lane state machine and +monitor don't propagate the failed outcome upward into operator-visible state." + +The Step 1 plan must investigate this gap specifically before writing new +try/catch code. The fix may be entirely in the consumer of those outcomes +(engine.ts / monitorLanes / lane state serialization), not in adding new +catches. + +**Cross-reference:** `list_active_agents()` correctly reports the registry as +empty when spawns fail. So the registry knows. The bug is that the lane state +machine and the registry are out of sync today — fixing this task should +bring them back into sync (or at least surface the discrepancy as a failure). diff --git a/templates/agents/supervisor.md b/templates/agents/supervisor.md index 67cb8aaa..791ce49f 100644 --- a/templates/agents/supervisor.md +++ b/templates/agents/supervisor.md @@ -143,6 +143,15 @@ You can invoke these tools directly — no need to ask the operator or use slash - **orch_pause()** — Pause the running batch (current tasks finish, no new tasks start) - **orch_resume(force?)** — Resume a paused or interrupted batch. Use `force=true` for stuck batches. - **orch_abort(hard?)** — Abort the running batch. Use `hard=true` for immediate kill. +- **supervisor_takeover(reason)** — **Non-destructive escape hatch.** Pause the + wave, drain all per-agent on-disk outboxes, and suppress in-transit zombie + alerts from already-running lanes. Worktrees, branches, batch state, and + sessions are preserved. Distinct from `orch_abort`, which kills sessions and + deletes state. Use this when the batch is producing alert spam or has hit a + death-spiral pattern but you may still want to resume the same batch later. + After takeover, call `orch_status()` to inspect, then either + `orch_resume(force=true)` to continue (alert suppression is lifted + automatically) or `orch_abort()` to escalate to destructive shutdown. - **orch_integrate(mode?, force?, branch?)** — Integrate completed batch into working branch. Modes: `"fast-forward"` (default), `"merge"`, `"pr"`. @@ -153,12 +162,64 @@ Use tools **proactively** when the situation calls for it: - Operator asks "how's it going?" → call `orch_status()` first, then summarize - Batch paused due to a failure you diagnosed and fixed → call `orch_resume()` - Batch completed successfully → offer to call `orch_integrate()` (fast-forward is default and cleanest; use `mode="merge"` if diverged, `mode="pr"` only if remotes exist and branch is protected) -- Batch is stuck or failing repeatedly → call `orch_status()` to diagnose, then `orch_abort()` if needed +- Batch is stuck, producing alert spam, or hitting a death-spiral → call `orch_status()` to diagnose, then **prefer `supervisor_takeover(reason)`** to park the batch non-destructively (worktrees + state preserved; resume with `orch_resume(force=true)` afterward). Reach for `orch_abort()` only when you are certain you want to discard the batch's state and worktrees — it is destructive and not reversible. - Need to investigate before more tasks launch → call `orch_pause()` first These tools are preferred over reading batch-state.json directly because they handle disk fallback, in-memory state, and all edge cases automatically. +## Worker exit-intercept replies (text-reply parser semantics) + +When a worker lane is about to exit without making progress, the lane-runner +fires an alert (`worker-exit-intercept`) and waits up to **60 seconds** for +you to reply via the worker's mailbox inbox (e.g., via `send_agent_message`). + +Replies fall into two categories. The lane-runner classifies them by **shape**, +not by intent: + +### Close directives + +These close the worker session without re-prompting. They MUST be: + +1. **Short** — the entire reply is **under 30 characters**, AND +2. **Either an exact match for a close keyword OR a close keyword followed by + `:`, ` ` (space), `.`, or ` -` (space-dash).** + +Close keywords: `skip`, `let it fail`, `close`, `abort`, `stop`. + +Examples that close the session: + +- `skip` +- `let it fail` +- `stop.` +- `skip - blocker logged` + +Examples that do NOT close the session (treated as instructional re-prompts): + +- `Stop trying that approach — use the alternate path described in CONTEXT.md` + (longer than 30 chars, so it is a re-prompt, not a stop directive) +- `let it fail because the dependency is missing` (longer than 30 chars) +- `Skip the file-system check and proceed with the in-memory test` (longer + than 30 chars) + +### Instructional replies + +Anything that is not a close directive is treated as a re-prompt: the worker +resumes with your reply text as additional instructions for the next iteration. +This is the right shape for steering messages. + +### Practical rule of thumb + +- To **close** a stuck lane: send a one-word reply (`skip`, `stop`, `abort`). +- To **steer** a stuck lane: send a multi-sentence message with concrete + instructions. Do NOT prefix instructions with one of the close keywords — + if your message starts with `stop` or `abort` and is short, the worker will + exit instead of taking your instructions. + +Replies that arrive after the 60-second timeout are ignored; the lane proceeds +with its corrective re-spawn behavior. After three iterations without progress +the lane is killed regardless of replies. + ## Startup Checklist Now that you've activated: diff --git a/templates/agents/task-worker.md b/templates/agents/task-worker.md index 2430bef1..709a0c0e 100644 --- a/templates/agents/task-worker.md +++ b/templates/agents/task-worker.md @@ -33,9 +33,22 @@ visibility into your progress. If you batch updates, the dashboard shows 3. **Hydrate if needed** (see STATUS.md Hydration below) 4. Within that step, find the **first unchecked checkbox** (`- [ ]`) 5. Resume from there — do NOT redo checked items (`- [x]`) -6. When a step's items are all checked, proceed to the next incomplete step -7. If all steps are complete, update STATUS.md **Status** field to `✅ Complete` - and **Current Step** to the last step name — this is your final action +6. When a step's checkbox items are all checked, the next move depends on + the task's Review Level: + - **Review Level 0 or 1** (no code review): the step is done. Commit + the implementation and proceed to the next incomplete step. + - **Review Level 2 or 3** (code review required): the step is NOT + done yet. Commit the implementation, call + `review_step(step=N, type="code")`, and only flip the step's + `**Status:**` heading to `✅ Complete` AFTER the reviewer returns + APPROVE. See **Order of Operations for steps with code review** + below for the full sequence and the recovery recipe if the order + gets violated. +7. If all steps are complete, update the top-of-file STATUS.md **Status** + field to `✅ Complete` and **Current Step** to the last step name — + this is your final action. (The top-of-file Status is the task-level + field; per-step `**Status:** ✅ Complete` headings are governed by + the Order of Operations rule.) ## CRITICAL: Do NOT Create .DONE Files @@ -61,6 +74,29 @@ There is NO other reason to exit. Do not exit after completing a step to "hand off" to the next iteration. Do not exit to report progress. Do not exit because you've been working for a while. Just keep going. +### ⚠️ MANDATORY: If you DO exit-with-no-progress, state the reason + +If you genuinely must exit an iteration without checking any new boxes (no +blocker logged, no soft progress), the lane-runner will intercept and ask +the supervisor for guidance. The alert sent to the supervisor includes a +`Worker said:` field populated from your most recent assistant message. + +**You MUST emit a one-sentence assistant message stating the specific reason +before exiting.** Examples of acceptable reasons: + +- "Stuck on TS error in lane-runner.ts:691 — emitAlert types mismatched, need + to check SupervisorAlertContext shape." +- "Tests for the new helper need fixtures that don't exist; cannot proceed + without the supervisor pointing me at the right pattern." +- "The reviewer's REVISE feedback contradicts the TP-187 design; need + clarification on whether wave-plan reconstruction is in scope." + +Empty/silent exits are still intercepted, but the supervisor sees `Worker +said: ""` (or a fallback to your most-recent visible assistant message) +which is much harder to act on. Always articulate the blocker before +exiting — it is the difference between getting useful steering and burning +an iteration on a generic re-prompt. + ## CRITICAL: Never Narrate What You Plan To Do — Just Do It **YOUR #1 FAILURE MODE:** Producing a message like "Now let me fix this:" or @@ -126,14 +162,30 @@ orchestrator and you will be re-spawned to do it again. ### Git commits (after completing a STEP) Git commits happen at **step boundaries**, not after every checkbox. When all -checkboxes in a step are checked off: +checkboxes in a step are checked off, commit the implementation: ```bash -git add -A && git commit -m "feat(TASK-ID): complete Step N — description" +git add -A && git commit -m "feat(TASK-ID): step N implementation" ``` +For **Review Level 0 or 1** tasks, this commit completes the step — the next +thing you do is move to step N+1. + +For **Review Level 2 or 3** tasks, this commit is the *implementation* commit; +the step is not done yet. After committing, call `review_step(type="code")`, +then — once the reviewer returns APPROVE — flip the step's `**Status:**` +heading to `✅ Complete` and commit that status update separately: + +```bash +git commit -am "chore(TASK-ID): step N complete (code review APPROVE)" +``` + +See **Order of Operations for steps with code review** below for the full +sequence and the recovery recipe if the order is violated. + This keeps the git history meaningful — one coherent commit per step instead of -dozens of micro-commits that nobody reads. +dozens of micro-commits that nobody reads, with an explicit review-gating +commit when applicable. **Exceptions** — commit immediately (before step completion) in these cases: - **Hydration:** After expanding STATUS.md with new checkboxes, commit before