From 451cacc76df4591cc3016aec7058022378f6571a Mon Sep 17 00:00:00 2001 From: mrchatam <287639636+mrchatam@users.noreply.github.com> Date: Sat, 26 Sep 2026 23:40:51 +0330 Subject: [PATCH 1/9] Add task handoff records and the parked needs_approval status Every finished task now carries a handoff record (state, failed_checks, owner, next_action, resume, context, updated_at/updated_by), derived deterministically from the verdict, daemon-run tests, integrity checks, blocked tool calls and the worker's RESULT block. It is persisted in task.json, returned in task_result and summarized in task_status and list_tasks; tasks from 0.1.0 get a derived record on the fly. Workers that report needs_approval/needs_input (or blocked with a request or after blocked tool calls) park the task in status needs_approval (verdict stays blocked). Parked tasks report terminal: true, parked: true, are skipped by retention unless retention.parked_days is set, and can be resumed with continue_task. The worker contract gains the needs_approval/needs_input statuses and an optional needs: line. --- adapters/kilo/config/AGENTS.md | 5 +- lib/audit.mjs | 2 +- lib/config.mjs | 3 +- lib/handoff.mjs | 214 ++++++++++++++++++++++++++++ lib/tasks.mjs | 245 +++++++++++++++++++++++++++++---- 5 files changed, 440 insertions(+), 29 deletions(-) create mode 100644 lib/handoff.mjs diff --git a/adapters/kilo/config/AGENTS.md b/adapters/kilo/config/AGENTS.md index 0c00363..28a94a5 100644 --- a/adapters/kilo/config/AGENTS.md +++ b/adapters/kilo/config/AGENTS.md @@ -6,14 +6,15 @@ You are an unattended coding worker. A supervisor agent will read your final mes 2. **Git.** Never commit, push, merge, rebase, reset, stash, switch/checkout branches, create branches/tags, or change git config/remotes. Leave every change uncommitted; the supervisor handles integration. `git status`, `git diff`, `git log` and `git checkout -- ` / `git restore ` are fine. Create and edit files with the write/edit tools, not shell heredocs or `echo >`: a shell command whose text mentions `git commit`/`git push` (even inside a heredoc body) is denied by policy, while file content written with the write/edit tools is not checked. 3. **Conventions.** Read the repo's README / AGENTS.md and the code around your change first. Follow existing style and structure. Keep the change minimal and focused on the task. Do not add dependencies unless the task explicitly requires it. 4. **Verify.** Run the test command given in the task (otherwise discover the project's tests) before finishing, and fix failures you caused. Commands run in a sandbox without network access, so do not try to install packages or reach the internet. Any toolchain caches the operator provides (for example a read-only Go module cache or npm cache) are preconfigured through environment variables: do not set or override HOME, XDG_* or tool cache variables, and never create caches or tool directories (`go/`, `.config/`, `node_modules/.cache` and similar) inside the worktree unless the task asks for it. If a build tool still fails for environment reasons (missing toolchain, dependency not in the offline cache), stop retrying and report the exact error under concerns with status blocked or partial. -5. **No questions.** If something is ambiguous, choose the most reasonable interpretation, proceed, and record the assumption under concerns. +5. **No questions.** If something is ambiguous, choose the most reasonable interpretation, proceed, and record the assumption under concerns. Only when you truly cannot continue without a human (an approval, e.g. to add a dependency or run an action the policy/sandbox blocked, or an answer only the owner has), stop and finish with status needs_approval (or needs_input) and put the one exact request under needs. 6. **Stay on task.** Do not refactor unrelated code, rewrite tests to make them pass unless the task says the tests are wrong, or delete tests. 7. **Keep going until done.** Every reply before your final one must contain a tool call. Never end a reply with only a statement of what you will do next ("Now let me update X:"); that ends the run with the work unfinished. Do the step instead. 8. **Finish** with this block as the very last thing in your final message (plain text, no code fence): ## RESULT -status: done | partial | blocked +status: done | partial | blocked | needs_approval | needs_input summary: <1-3 sentences on what you changed and why> files_changed: tests: -> concerns: +needs: diff --git a/lib/audit.mjs b/lib/audit.mjs index 328e5bd..279b343 100644 --- a/lib/audit.mjs +++ b/lib/audit.mjs @@ -18,7 +18,7 @@ export function audit(kind, data) { export function sanitizeParams(method, params) { if (method === "offer_env") return { names: Object.keys(params?.env || {}) } const p = { ...(params || {}) } - for (const k of ["task", "instructions"]) { + for (const k of ["task", "instructions", "next_action", "note"]) { if (typeof p[k] === "string") { p[`${k}_sha256`] = sha256(p[k]) p[`${k}_chars`] = p[k].length diff --git a/lib/config.mjs b/lib/config.mjs index 1a740e9..4a570d5 100644 --- a/lib/config.mjs +++ b/lib/config.mjs @@ -52,7 +52,8 @@ const DEFAULTS = { retry: { max_retries: 2, backoff_sec: [30, 120], provider_cooldown_sec: 60 }, // Automatic cleanup of finished tasks. After worktree_days the worktree + branch + per-task backend data // are removed (the diff stays archived in the task dir); after task_days the task dir itself is deleted. - retention: { enabled: true, worktree_days: 7, task_days: 30, sweep_interval_min: 60 }, + // Parked tasks (status needs_approval) are skipped unless parked_days is set (worktree only, never the task dir). + retention: { enabled: true, worktree_days: 7, task_days: 30, parked_days: null, sweep_interval_min: 60 }, default_backend: "kilo", bwrap: null, // null = auto: bwrap bundled with the Kilo CLI package (if installed), then `bwrap` on PATH // Per-backend settings. bin null = auto: $WH__BIN, /-cli/bin/, then on PATH. diff --git a/lib/handoff.mjs b/lib/handoff.mjs new file mode 100644 index 0000000..d4a7ba8 --- /dev/null +++ b/lib/handoff.mjs @@ -0,0 +1,214 @@ +// Handoff records: who must act next on a finished task, what exactly they must do, and how to resume. +// Derived deterministically by the daemon from the verdict and the data it already has (test results, +// integrity checks, blocked tool calls, the worker's ## RESULT block); the model is never asked to +// produce it. The supervisor or a human can update it later (update_handoff / approve_task). +import { head } from "./util.mjs" + +export const HANDOFF_STATES = ["done", "needs_fix", "needs_review", "needs_approval", "needs_input", "blocked", "retryable", "closed"] +// Handoff states that park a task (status needs_approval) until a human answers with approve_task. +export const PARK_STATES = new Set(["needs_approval", "needs_input"]) +// Handoff states that need nobody's attention. +export const QUIET_STATES = new Set(["done", "closed"]) +// Handoff states approve_task accepts. +export const APPROVABLE_STATES = new Set(["needs_approval", "needs_input", "blocked"]) +export const OWNER_RE = /^[A-Za-z0-9][A-Za-z0-9 _.:@/+-]{0,79}$/ + +// Worker-reported status from the ## RESULT block, normalized: "needs approval" / "needs-input" / +// "approval_required" / "awaiting approval" -> needs_approval / needs_input; otherwise the first word. +export function normalizeWorkerStatus(s) { + if (typeof s !== "string") return s + const v = s.toLowerCase().trim() + let m + if ((m = v.match(/^(?:needs?|requires?|awaiting|waiting[\s_-]+for)[\s_-]*(approval|input|human[\s_-]+input|answer|decision)/))) return m[1] === "approval" || m[1] === "decision" ? "needs_approval" : "needs_input" + if ((m = v.match(/^(approval|input)[\s_-]*(required|needed)/))) return m[1] === "approval" ? "needs_approval" : "needs_input" + return v.split(/[\s|,]/)[0] +} + +// Names/lines of failing tests in test output (unittest, pytest, node:test, go, cargo, jest/mocha-ish). +export function extractFailingTests(output, max = 10) { + if (!output) return [] + const out = [] + const add = (s) => { + s = head(String(s).trim(), 200) + if (s && !out.includes(s) && out.length < max) out.push(s) + } + for (const line of String(output).split("\n")) { + let m + if ((m = line.match(/^(?:FAIL|ERROR): (\S+ \([^)]+\))/))) add(m[1]) // unittest + else if ((m = line.match(/^(?:FAILED|ERROR) (\S+::\S+)/))) add(m[1]) // pytest -rf / summary + else if ((m = line.match(/^\s*not ok \d+ - (.+?)(?:\s+#.*)?$/))) add(m[1]) // node:test / TAP + else if ((m = line.match(/^\s*--- FAIL: (\S+)/))) add(m[1]) // go + else if ((m = line.match(/^test (\S+) \.\.\. FAILED/))) add(m[1]) // cargo + else if ((m = line.match(/^\s*[✕✗×]\s+(.+)$/))) add(m[1]) // jest/vitest + } + return out +} + +function hasText(s) { + return typeof s === "string" && s.trim() && !/^(none|n\/a|-)\.?$/i.test(s.trim()) +} + +const SETUP_ERROR_RE = /missing credentials|cannot run:|is disabled|unknown profile|failed to start the .* worker|worktree is missing|not installed/i + +// Build the handoff record for a finished task. `t` is the persisted task (t.result set by finalize). +export function deriveHandoff(t, { by = "daemon", at = new Date().toISOString() } = {}) { + const r = t.result || {} + const verdict = r.verdict || null + const status = t.status + const tr = r.test_results || {} + const wr = r.worker_reported || null + const wStatus = wr?.status || null + const request = hasText(wr?.needs) ? head(wr.needs.trim(), 500) : null + const concerns = Array.isArray(r.remaining_concerns) ? r.remaining_concerns : [] + const errors = Array.isArray(r.errors) ? r.errors : [] + const worktreeAlive = !t.worktree_removed && !!(r.worktree_path || t.worktree_path) + const sessionReusable = worktreeAlive && !!t.session_id + const failed = [] + + // ---- failed checks (facts, most important first) ---- + if (verdict === "integrity_violation" || (r.integrity && (r.integrity.commits_made > 0 || r.integrity.main_clone_unchanged === false))) { + failed.push({ check: "integrity", commits_made: r.integrity?.commits_made ?? null, main_clone_unchanged: r.integrity?.main_clone_unchanged ?? null, detail: concerns.filter((c) => c.startsWith("INTEGRITY")).join("; ") || "integrity check failed" }) + } + if (tr.executed && !tr.passed) { + const failing = Array.isArray(tr.failing_tests) && tr.failing_tests.length ? tr.failing_tests : extractFailingTests(tr.tail) + failed.push({ check: "tests", command: tr.command, exit_code: tr.exit_code, timed_out: !!tr.timed_out, counts: tr.counts || {}, failing_tests: failing, excerpt: head(String(tr.tail || "").trim().split("\n").slice(-12).join("\n"), 800), log: "task_details kind=test_log" }) + } + if (status === "timeout" || verdict === "timeout") failed.push({ check: "timeout", detail: errors.find((e) => /timeout/i.test(e)) || "wall-clock timeout reached; the worker was stopped" }) + if (status === "stalled" || verdict === "stalled") failed.push({ check: "stall", detail: errors.find((e) => /stall|no activity/i.test(e)) || "no worker activity; the worker was stopped" }) + if (status === "interrupted" || verdict === "interrupted") failed.push({ check: "interrupted", detail: errors.find((e) => /restart|stopped/i.test(e)) || "the daemon stopped while the worker was running" }) + if (verdict === "worker_error") failed.push({ check: "worker_error", detail: head(errors.slice(-3).join("; ") || "the worker exited with an error", 600) }) + if ((r.activity?.blocked_calls || 0) > 0) { + failed.push({ check: "blocked_calls", count: r.activity.blocked_calls, examples: (r.blocked_examples || []).slice(0, 5).map((b) => ({ tool: b.tool, input: head(b.input || "", 160), error: head(b.error || "", 160) })), log: "task_details kind=activity" }) + } + if (wStatus && wStatus !== "done") failed.push({ check: "worker_status", reported: wStatus, ...(request ? { request } : {}) }) + if (!wr && status === "completed" && t.mode !== "review") failed.push({ check: "result_block", detail: "worker did not end with the required ## RESULT block" }) + if (verdict === "no_changes") failed.push({ check: "no_changes", detail: "implement task produced an empty diff" }) + + // ---- state, owner, next action, resume tool ---- + let state, owner, next, tool, args + const id = t.id + const cont = (instructions, extra = {}) => ({ task_id: id, instructions, ...extra }) + const testsFailing = failed.find((f) => f.check === "tests") + const firstConcern = concerns.find((c) => !/^(daemon-run tests failed|INTEGRITY|worker reported status|\d+ tool call\(s\) were blocked)/.test(c)) + const blockedEx = (r.blocked_examples || [])[0] + const blockedDesc = blockedEx ? `${blockedEx.tool} \`${head(String(blockedEx.input || "").replace(/\s+/g, " "), 120)}\`` : null + + if (verdict === "integrity_violation") { + state = "blocked" + owner = "human" + next = `Human must inspect ${t.branch || "the task branch"} before anything else: ${failed[0].detail}. Do not merge. Then either cleanup_task with discard_unmerged_changes=true, or continue_task after fixing the repository state.` + tool = null + } else if (status === "needs_approval" || (verdict === "blocked" && PARK_STATES.has(wStatus))) { + state = wStatus === "needs_input" ? "needs_input" : "needs_approval" + owner = "human" + const what = request || firstConcern || (blockedDesc ? `the worker needs ${blockedDesc}, which the sandbox/policy blocked` : "see the worker summary") + next = state === "needs_input" + ? `Human must answer the worker: ${what}. Then call approve_task with decision=approve and the answer as instructions (or decision=reject).` + : `Human must approve or deny: ${what}. Then call approve_task with decision=approve (optionally with instructions) or decision=reject. Approval does not lift sandbox rules: an action the sandbox forbids (network, installs) must be done by the human first.` + tool = "approve_task" + args = { task_id: id, decision: "approve", instructions: "" } + } else if (verdict === "blocked") { + state = "blocked" + owner = "human" + const what = request || firstConcern || (blockedDesc ? `blocked action ${blockedDesc}` : "the worker reported status blocked without details (see summary)") + next = `Human must resolve the blocker: ${what}. Then call approve_task (decision=approve) or continue_task to resume, or approve_task decision=reject to close.` + tool = "approve_task" + args = { task_id: id, decision: "approve", instructions: "" } + } else if (verdict === "tests_failed") { + state = "needs_fix" + owner = "supervisor" + const names = testsFailing?.failing_tests?.length ? testsFailing.failing_tests.slice(0, 3).join(", ") : null + const what = names ? `the failing test(s) ${names}` : `the failing tests (\`${tr.command}\` exit ${tr.exit_code}${tr.timed_out ? ", timed out" : ""})` + next = `Run continue_task with instructions to fix ${what}; see task_details kind=test_log.` + tool = "continue_task" + args = cont(`The daemon-run tests failed: \`${tr.command}\` exited ${tr.exit_code}.${names ? ` Failing: ${names}.` : ""} Fix the cause (do not weaken or delete tests), rerun the tests and report.`) + } else if (verdict === "worker_error") { + const setup = errors.some((e) => SETUP_ERROR_RE.test(e)) + if (setup) { + state = "blocked" + owner = "human" + next = `Human must fix the setup: ${head(errors.find((e) => SETUP_ERROR_RE.test(e)), 300)}. Then continue_task to retry.` + tool = "continue_task" + args = cont("The setup problem was fixed. Continue the task from where you left off.") + } else { + state = "retryable" + owner = "supervisor" + next = `Run continue_task to retry (worker error: ${head(errors.slice(-1)[0] || "see task_details kind=stderr", 200)}).` + tool = "continue_task" + args = cont("The previous run ended with an error. Check `git status` / `git diff`, continue from where you left off and finish with the ## RESULT block.") + } + } else if (verdict === "timeout" || verdict === "stalled" || verdict === "interrupted") { + state = "retryable" + owner = "supervisor" + const why = verdict === "timeout" ? "hit the wall-clock timeout" : verdict === "stalled" ? "stalled (no activity)" : "was interrupted by a daemon stop/restart" + next = `Run continue_task to resume: the worker ${why}${verdict === "timeout" ? "; consider a larger timeout_minutes or a narrower instruction" : ""}.` + tool = "continue_task" + args = cont("Resume the task where you left off. Check `git status` / `git diff` for what is already done, finish the remaining work, run the tests and end with the ## RESULT block.", verdict === "timeout" ? { timeout_minutes: Math.min(120, Math.round((t.timeout_min || 30) * 1.5)) } : {}) + } else if (verdict === "cancelled") { + state = "closed" + owner = "supervisor" + next = "Nothing pending (cancelled). continue_task resumes it; cleanup_task removes the worktree when the work is no longer needed." + tool = null + } else if (verdict === "no_changes") { + state = "needs_fix" + owner = "supervisor" + next = "Worker made no changes. Read the summary; if work was expected, run continue_task with more specific instructions (files, functions, acceptance criteria); otherwise close with update_handoff state=closed." + tool = "continue_task" + args = cont("You made no changes. Implement the task now: .") + } else if (verdict === "success" || verdict === "success_untested") { + const partial = wStatus === "partial" + const noBlock = failed.some((f) => f.check === "result_block") + if (partial || noBlock) { + state = "needs_fix" + owner = "supervisor" + next = partial + ? `Worker reported status partial${firstConcern ? ` (${head(firstConcern, 200)})` : ""}. Run continue_task with instructions to finish the remaining work.` + : "Worker stopped without the ## RESULT block (possibly unfinished). Run continue_task telling it to finish and report, or review the diff and accept." + tool = "continue_task" + args = cont(partial ? "Finish the remaining work you reported as partial, rerun the tests and end with the ## RESULT block." : "Finish the task if anything is left, run the tests and end with the ## RESULT block.") + } else if (verdict === "success_untested") { + state = "needs_review" + owner = "supervisor" + next = `No tests ran (${tr.reason || "no test command"}). Review the diff by hand (task_details kind=diff) before merging ${t.branch}; then cleanup_task.` + tool = null + } else if (t.mode === "review") { + state = "done" + owner = "supervisor" + next = "Read the review findings in summary and remaining_concerns; act on them (continue_task on the reviewed task) or accept." + tool = null + } else { + state = "done" + owner = "supervisor" + next = `Review the diff (task_details kind=diff) and merge branch ${t.branch}, then cleanup_task.` + tool = null + } + } else { + state = "needs_review" + owner = "supervisor" + next = "Read task_result and decide." + tool = null + } + + return { + state, owner, next_action: head(next, 1000), + failed_checks: failed, + resume: { + task_id: id, tool, args: tool ? args : null, session_reusable: sessionReusable, session_id: t.session_id || null, + worktree_path: worktreeAlive ? t.worktree_path : null, worktree_exists: worktreeAlive, branch: t.branch || null, base_commit: t.base_commit || null, + }, + context: { + verdict, status, summary: head(r.summary || "", 600), worker_status: wStatus, worker_request: request, + remaining_concerns: concerns.slice(0, 10), diffstat: r.diffstat || null, files_changed: Array.isArray(r.files_changed) ? r.files_changed.length : 0, + diff_path: r.diff_path || null, diff: "task_details kind=diff", previous_attempts: (t.previous_results || []).length, + }, + notes: [], + derived: true, + updated_at: at, updated_by: by, + } +} + +// Short form for task_status / list_tasks. +export function handoffSummary(h) { + if (!h) return null + return { state: h.state, owner: h.owner, next_action: head(h.next_action || "", 300), failed_checks: (h.failed_checks || []).map((f) => f.check), updated_at: h.updated_at, updated_by: h.updated_by } +} diff --git a/lib/tasks.mjs b/lib/tasks.mjs index e8c75aa..4692b50 100644 --- a/lib/tasks.mjs +++ b/lib/tasks.mjs @@ -12,9 +12,17 @@ import { audit } from "./audit.mjs" import { loadFromStore } from "./credentials.mjs" import { outerSandboxArgs } from "./sandbox.mjs" import { getBackend, profileBackend, backendProblems, backendSummary } from "../adapters/index.mjs" +import { deriveHandoff, handoffSummary, normalizeWorkerStatus, extractFailingTests, HANDOFF_STATES, PARK_STATES, QUIET_STATES, APPROVABLE_STATES, OWNER_RE } from "./handoff.mjs" const TERMINAL = new Set(["completed", "failed", "timeout", "stalled", "cancelled", "interrupted"]) const ACTIVE = new Set(["queued", "running", "testing", "finalizing", "retry_wait"]) +// Parked: the worker has finished (no process, result ready) but the task waits for a human decision +// (approve_task / continue_task). Not closed, so retention never removes its worktree automatically +// (unless retention.parked_days is set). Pollers treat it like a terminal status (task_status terminal: true). +const PARKED = new Set(["needs_approval"]) +const settled = (s) => TERMINAL.has(s) || PARKED.has(s) +export { TERMINAL, PARKED, ACTIVE } +const hasText = (s) => typeof s === "string" && !!s.trim() && !/^(none|n\/a|-)\.?$/i.test(s.trim()) const TASK_ID_RE = /^wh-[0-9]{8}-[0-9]{6}-[0-9a-f]{4}$/ class UserError extends Error {} @@ -54,12 +62,12 @@ export function parseResultBlock(text) { const idx = text.lastIndexOf("## RESULT") if (idx < 0) return null const body = text.slice(idx + 9).replace(/```/g, "") - const keys = ["status", "summary", "files_changed", "tests", "concerns"] + const keys = ["status", "summary", "files_changed", "tests", "concerns", "needs"] const out = {} let cur = null for (const raw of body.split("\n")) { const line = raw.replace(/^\s*[-*]\s*/, "") - const m = line.match(/^\s*\**(status|summary|files_changed|tests|concerns)\**\s*:\s*(.*)$/i) + const m = line.match(/^\s*\**(status|summary|files_changed|tests|concerns|needs)\**\s*:\s*(.*)$/i) if (m) { cur = m[1].toLowerCase() out[cur] = m[2].trim() @@ -68,7 +76,7 @@ export function parseResultBlock(text) { } } if (!keys.some((k) => k in out)) return null - if (out.status) out.status = out.status.toLowerCase().split(/[\s|,]/)[0] + if (out.status) out.status = normalizeWorkerStatus(out.status) return out } @@ -145,10 +153,7 @@ export class Manager { if (b.installed && b.home) ensureDir(b.home, 0o755) } catch {} } - for (const id of fs.readdirSync(dirs.tasks)) { - const t = readJsonSafe(path.join(dirs.tasks, id, "task.json")) - if (t && t.id === id) this.tasks.set(id, t) - } + this.load() await this.recover() this.timer = setInterval(() => this.schedule().catch((e) => audit("error", { where: "schedule", error: e.message })), 2000) this.schedule().catch(() => {}) @@ -159,6 +164,15 @@ export class Manager { setTimeout(() => this.autoSweep(), 30000).unref?.() } + // Read every persisted task.json (tasks, their results and handoff records survive restarts). + load() { + ensureDir(dirs.tasks) + for (const id of fs.readdirSync(dirs.tasks)) { + const t = readJsonSafe(path.join(dirs.tasks, id, "task.json")) + if (t && t.id === id) this.tasks.set(id, t) + } + } + async recover() { const cfg = daemonConfig() for (const t of this.tasks.values()) { @@ -304,7 +318,7 @@ export class Manager { if (mode !== "review") throw new UserError("review_task_id requires mode='review'") const target = this.get(params.review_task_id) if (target.repo !== repo.name) throw new UserError("review_task_id belongs to a different repo") - if (!TERMINAL.has(target.status)) throw new UserError("the task to review is still active; wait until it finishes") + if (!settled(target.status)) throw new UserError("the task to review is still active; wait until it finishes") const patchFile = path.join(this.taskDir(target.id), "diff.patch") if (!fs.existsSync(patchFile) || fs.statSync(patchFile).size === 0) throw new UserError("the task to review has no diff") review = { task_id: target.id, base_commit: target.base_commit, patch: patchFile } @@ -353,20 +367,39 @@ export class Manager { const run = t.runs[t.runs.length - 1] const live = this.live.get(id) return { - task_id: t.id, status: t.status, terminal: TERMINAL.has(t.status), phase: t.phase, repo: t.repo, mode: t.mode, profile: t.profile, backend: run?.backend || t.backend || "kilo", model: run?.model || t.model, + task_id: t.id, status: t.status, terminal: settled(t.status), parked: PARKED.has(t.status), phase: t.phase, repo: t.repo, mode: t.mode, profile: t.profile, backend: run?.backend || t.backend || "kilo", model: run?.model || t.model, session_id: t.session_id, runs: t.runs.length, retries: t.retries, elapsed_s: t.started_at ? Math.round(((t.finished_at ? Date.parse(t.finished_at) : Date.now()) - Date.parse(t.started_at)) / 1000) : 0, seconds_since_last_activity: live ? Math.round((Date.now() - live.lastActivity) / 1000) : null, activity: { turns: t.stats.turns, tool_calls: t.stats.tool_calls, tool_errors: t.stats.tool_errors, blocked_calls: t.stats.blocked_calls }, verdict: t.result?.verdict || null, queue_position: this.queuePosition(t), retry_at: t.retry_at || null, - hint: TERMINAL.has(t.status) ? "Call task_result for the structured summary." : "Still working; poll again later.", + handoff: settled(t.status) ? handoffSummary(this.handoffOf(t)) : null, + hint: PARKED.has(t.status) + ? "Parked: waiting for a human decision. Read handoff.next_action; answer with approve_task (or continue_task)." + : TERMINAL.has(t.status) ? "Call task_result for the structured summary; handoff.next_action says what to do next." : "Still working; poll again later.", } } result(id) { const t = this.get(id) - if (!t.result) return { task_id: t.id, status: t.status, terminal: TERMINAL.has(t.status), message: "No result yet; the task is still active. Poll task_status." } - return t.result + if (!t.result) return { task_id: t.id, status: t.status, terminal: settled(t.status), message: "No result yet; the task is still active. Poll task_status." } + return { ...t.result, status: t.status, handoff: this.handoffOf(t) } + } + + // The task's handoff record: the persisted one, or (tasks finished before v0.2.0) derived on the fly. + // Worktree/session facts are refreshed, since cleanup can remove the worktree after the handoff was written. + handoffOf(t) { + if (!t.result || !settled(t.status)) return null + const h = t.handoff || redact(deriveHandoff(t, { at: t.finished_at || t.updated_at })) + const alive = !t.worktree_removed + return { ...h, resume: { ...h.resume, worktree_exists: alive, worktree_path: alive ? t.worktree_path : null, session_reusable: !!(alive && h.resume?.session_reusable) } } + } + + needsAttention(t) { + if (!settled(t.status)) return false + if (PARKED.has(t.status)) return true + const h = this.handoffOf(t) + return !!h && !QUIET_STATES.has(h.state) } details(id, kind = "activity", offset = 0, maxBytes, run) { @@ -391,8 +424,12 @@ export class Manager { list(filter = {}) { let arr = [...this.tasks.values()] - if (filter.status) arr = arr.filter((t) => (filter.status === "active" ? ACTIVE.has(t.status) : filter.status === "terminal" ? TERMINAL.has(t.status) : t.status === filter.status)) + const f = filter.status + // 'terminal' = no worker running (includes parked tasks); 'parked' = waiting for approve_task; + // 'needs_attention' = parked tasks plus finished tasks whose handoff is not done/closed. + if (f) arr = arr.filter((t) => (f === "active" ? ACTIVE.has(t.status) : f === "terminal" ? settled(t.status) : f === "parked" ? PARKED.has(t.status) : f === "needs_attention" ? this.needsAttention(t) : t.status === f)) if (filter.repo) arr = arr.filter((t) => t.repo === filter.repo) + if (filter.owner) arr = arr.filter((t) => String(this.handoffOf(t)?.owner || "").toLowerCase() === String(filter.owner).toLowerCase()) arr.sort((a, b) => (a.created_at < b.created_at ? 1 : -1)) const limit = Math.min(Math.max(1, Number(filter.limit) || 20), 200) return { @@ -400,21 +437,26 @@ export class Manager { tasks: arr.slice(0, limit).map((t) => ({ task_id: t.id, status: t.status, verdict: t.result?.verdict || null, repo: t.repo, mode: t.mode, profile: t.profile, created_at: t.created_at, finished_at: t.finished_at, task: head(t.task, 120), branch: t.branch, worktree_removed: t.worktree_removed, + handoff: settled(t.status) ? (({ state, owner, next_action }) => ({ state, owner, next_action }))(handoffSummary(this.handoffOf(t)) || {}) : null, })), } } - async continueTask(id, instructions, timeoutMinutes, profileName) { + async continueTask(id, instructions, timeoutMinutes, profileName, { approval = null } = {}) { const t = this.get(id) - if (!TERMINAL.has(t.status)) throw new UserError(`task is ${t.status}; continue_task works only on finished tasks (cancel it first if needed)`) + if (!settled(t.status)) throw new UserError(`task is ${t.status}; continue_task works only on finished or parked tasks (cancel it first if needed)`) if (t.worktree_removed) throw new UserError("task worktree was cleaned up; delegate a new task instead") if (typeof instructions !== "string" || !instructions.trim()) throw new UserError("instructions are required") if (instructions.length > daemonConfig().limits.task_max_chars) throw new UserError("instructions too long") const profile = this.profile(profileName || t.profile) this.checkBackend(profile) const timeout = this.timeoutMin(timeoutMinutes) - if (t.result) t.previous_results.push({ at: now(), status: t.status, verdict: t.result.verdict, summary: t.result.summary }) + const h = this.handoffOf(t) + if (t.result) t.previous_results.push({ at: now(), status: t.status, verdict: t.result.verdict, summary: t.result.summary, handoff: h ? { state: h.state, owner: h.owner, next_action: h.next_action } : undefined, approval: approval || undefined }) + if (approval) t.approvals = [...(t.approvals || []), approval].slice(-20) t.result = null + t.handoff = null + delete t.parked_from t.status = "queued" t.phase = "follow-up queued" t.finished_at = null @@ -424,7 +466,7 @@ export class Manager { t.pending_profile_origin = profile.name t.pending_run = { kind: "continue", message: instructions, profile: profile.name, timeout_min: timeout } this.save(t) - this.event(t, "continue_queued", { profile: profile.name }) + this.event(t, "continue_queued", { profile: profile.name, ...(approval ? { approval: approval.decision, by: approval.by } : {}) }) await this.schedule() return { task_id: t.id, status: t.status, session_id: t.session_id, profile: profile.name, backend: profile.backend, resumes_session: this.sessionBackend(t) === profile.backend && !!t.session_id, next_step: "Poll task_status; then task_result." } } @@ -445,12 +487,133 @@ export class Manager { await this.finalize(t, { tests: false, status: "cancelled" }) return { task_id: id, status: "cancelled" } } + if (PARKED.has(t.status)) return this.closeTask(t, "supervisor", "cancelled via cancel_task") throw new UserError(`task is ${t.status}; nothing to cancel`) } + // ---------- handoff / approval ---------- + // Who is acting: an explicit `by`, else "human (cli)" for the operator CLI, else "supervisor" (MCP). + actor(by, caller) { + if (by !== undefined && by !== null && by !== "") { + if (typeof by !== "string" || !OWNER_RE.test(by)) throw new UserError("by must be a short name (letters, digits, space, _.:@/+-; max 80 chars)") + return by + } + return caller?.client === "workhorse" ? "human (cli)" : "supervisor" + } + + checkText(name, v, max = 2000) { + if (v === undefined || v === null) return undefined + if (typeof v !== "string" || !v.trim() || v.length > max) throw new UserError(`${name} must be a non-empty string up to ${max} chars`) + return v.trim() + } + + handoff(id) { + const t = this.get(id) + if (!settled(t.status)) return { task_id: t.id, status: t.status, handoff: null, message: "The task is still active; the handoff is written when it finishes." } + return { task_id: t.id, status: t.status, verdict: t.result?.verdict || null, handoff: this.handoffOf(t), approvals: t.approvals || [] } + } + + // Supervisor/human edits of the handoff. Setting state needs_approval/needs_input parks a finished task; + // setting any other state on a parked task unparks it (back to the status it had, else completed). + updateHandoff(p = {}, caller = null) { + const allowed = new Set(["task_id", "owner", "next_action", "note", "state", "by"]) + for (const k of Object.keys(p)) if (!allowed.has(k)) throw new UserError(`unknown parameter '${k}'`) + const t = this.get(p.task_id) + if (!settled(t.status)) throw new UserError(`task is ${t.status}; the handoff exists once the task has finished`) + if (p.state !== undefined && !HANDOFF_STATES.includes(p.state)) throw new UserError(`state must be one of ${HANDOFF_STATES.join(", ")}`) + if (p.owner !== undefined && (typeof p.owner !== "string" || !OWNER_RE.test(p.owner))) throw new UserError("owner must be 'supervisor', 'human' or a short name (letters, digits, space, _.:@/+-; max 80 chars)") + const nextAction = this.checkText("next_action", p.next_action) + const note = this.checkText("note", p.note) + if (p.state === undefined && p.owner === undefined && nextAction === undefined && note === undefined) throw new UserError("nothing to update: pass state, owner, next_action and/or note") + const who = this.actor(p.by, caller) + const at = now() + const h = { ...this.handoffOf(t) } + const changed = [] + if (p.state !== undefined && p.state !== h.state) { h.state = p.state; changed.push("state") } + if (p.owner !== undefined && p.owner !== h.owner) { h.owner = p.owner; changed.push("owner") } + if (nextAction !== undefined) { + h.next_action = nextAction + changed.push("next_action") + } + if (note !== undefined) { + h.notes = [...(h.notes || []), { at, by: who, note }].slice(-20) + changed.push("note") + } + h.history = [...(h.history || []), { at, by: who, event: "update", changed, state: h.state, owner: h.owner }].slice(-20) + h.derived = false + h.updated_at = at + h.updated_by = who + const from = t.status + if (p.state !== undefined) { + if (PARK_STATES.has(p.state) && TERMINAL.has(t.status)) { + t.parked_from = t.status + t.status = "needs_approval" + if (p.owner === undefined) h.owner = "human" + } else if (!PARK_STATES.has(p.state) && PARKED.has(t.status)) { + t.status = t.parked_from || "completed" + delete t.parked_from + } + } + t.handoff = redact(h) + this.save(t) + this.event(t, "handoff_updated", { by: who, changed, state: h.state, owner: h.owner, ...(from !== t.status ? { from_status: from } : {}) }) + return { task_id: t.id, status: t.status, handoff: this.handoffOf(t) } + } + + // Answer a parked/blocked task. approve: resume via the continue path with the approval in the + // follow-up message. reject: with instructions, resume and tell the worker not to do it; without, + // close the task (status cancelled, handoff closed; the worktree is kept). + async approve(p = {}, caller = null) { + const allowed = new Set(["task_id", "decision", "instructions", "note", "by", "timeout_minutes", "profile"]) + for (const k of Object.keys(p)) if (!allowed.has(k)) throw new UserError(`unknown parameter '${k}'`) + const t = this.get(p.task_id) + if (p.decision !== "approve" && p.decision !== "reject") throw new UserError("decision must be 'approve' or 'reject'") + if (!settled(t.status)) throw new UserError(`task is ${t.status}; nothing to approve yet`) + const h = this.handoffOf(t) + if (!PARKED.has(t.status) && !APPROVABLE_STATES.has(h?.state)) throw new UserError(`task is not waiting for approval (status ${t.status}, handoff state ${h?.state || "none"}); use continue_task or update_handoff`) + const instructions = this.checkText("instructions", p.instructions, daemonConfig().limits.task_max_chars) + const note = this.checkText("note", p.note) + const who = this.actor(p.by, caller) + const request = h?.context?.worker_request || h?.next_action || "the pending request" + const rec = { at: now(), by: who, decision: p.decision, request: head(request, 300), ...(note ? { note } : {}), ...(instructions ? { instructions: head(instructions, 300) } : {}) } + const logApproval = () => this.event(t, "approval", { decision: p.decision, by: who, handoff_state: h?.state || null, resumed: !(p.decision === "reject" && !instructions) }) + if (p.decision === "reject" && !instructions) { + logApproval() + return this.closeTask(t, who, `approval rejected by ${who}${note ? `: ${note}` : ""}`, rec) + } + const msg = p.decision === "approve" + ? `APPROVED by ${who}: ${request}${note ? `\nNote: ${note}` : ""}\n\n${instructions || "Proceed with the approved action and finish the task."}\n\n(Approval does not change the sandbox: actions it blocks stay blocked. If the approved action needs network or an install, the operator has done it outside, or you must report that under concerns.)` + : `DENIED by ${who}: ${request}. Do not do that.${note ? `\nNote: ${note}` : ""}\n\n${instructions}` + const r = await this.continueTask(t.id, msg, p.timeout_minutes, p.profile, { approval: rec }) + logApproval() + return { ...r, decision: p.decision, approval: rec } + } + + closeTask(t, who, reason, rec = null) { + const from = t.status + const h = { ...this.handoffOf(t) } + t.status = "cancelled" + delete t.parked_from + if (t.result) t.result = { ...t.result, status: "cancelled" } + if (rec) t.approvals = [...(t.approvals || []), rec].slice(-20) + const at = now() + h.state = "closed" + h.owner = "supervisor" + h.next_action = `Closed (${reason}). The worktree is kept: merge any useful partial diff, continue_task to resume, or cleanup_task (discard_unmerged_changes=true) to drop it.` + h.resume = { ...h.resume, tool: null, args: null } + h.history = [...(h.history || []), { at, by: who, event: "closed", reason, from_status: from }].slice(-20) + h.derived = false + h.updated_at = at + h.updated_by = who + t.handoff = redact(h) + this.save(t) + this.event(t, "closed", { by: who, from_status: from, reason }) + return { task_id: t.id, status: t.status, ...(rec ? { decision: rec.decision } : {}), handoff: this.handoffOf(t) } + } + async cleanup(id, discard = false) { const t = this.get(id) - if (!TERMINAL.has(t.status)) throw new UserError(`task is ${t.status}; cancel it or wait before cleanup`) + if (!settled(t.status)) throw new UserError(`task is ${t.status}; cancel it or wait before cleanup`) if (t.worktree_removed) return { task_id: id, message: "already cleaned up", diff_path: path.join(this.taskDir(id), "diff.patch") } const repo = this.repo(t.repo) const wt = t.worktree_path @@ -486,16 +649,29 @@ export class Manager { // Remove old finished work. worktree_days: worktree + workhorse/ branch + per-task backend data of tasks // finished longer ago (the diff is archived to the task dir first, merged or not). task_days: delete // the task dir itself. Explicit params override daemon.json `retention` (0 = everything finished). - async sweep({ worktree_days, task_days, dry_run = false, trigger = "manual" } = {}) { + // Parked tasks (needs_approval) are skipped: their worktree waits for the human, unless parked_days + // (param or retention.parked_days) is set, in which case only the worktree is removed after that many + // days since the handoff was last updated. Parked task dirs are never purged automatically. + async sweep({ worktree_days, task_days, parked_days, dry_run = false, trigger = "manual" } = {}) { const ret = daemonConfig().retention || {} const wd = worktree_days ?? ret.worktree_days const td = task_days ?? ret.task_days - for (const [k, v] of [["worktree_days", wd], ["task_days", td]]) + const pd = parked_days ?? ret.parked_days + for (const [k, v] of [["worktree_days", wd], ["task_days", td], ["parked_days", pd]]) if (v !== null && v !== undefined && (typeof v !== "number" || !Number.isFinite(v) || v < 0)) throw new UserError(`${k} must be a number >= 0`) const age = (t) => (Date.now() - Date.parse(t.finished_at || t.updated_at || t.created_at)) / 86400000 - const out = { dry_run: !!dry_run, worktree_days: wd ?? null, task_days: td ?? null, worktrees_removed: [], tasks_deleted: [], errors: [] } + const out = { dry_run: !!dry_run, worktree_days: wd ?? null, task_days: td ?? null, parked_days: pd ?? null, worktrees_removed: [], tasks_deleted: [], parked_kept: [], errors: [] } for (const t of [...this.tasks.values()]) { - if (!TERMINAL.has(t.status) || this.live.has(t.id)) continue + if (this.live.has(t.id)) continue + if (PARKED.has(t.status)) { + const pa = (Date.now() - Date.parse(t.handoff?.updated_at || t.finished_at || t.updated_at || t.created_at)) / 86400000 + if (pd != null && pa >= pd && !t.worktree_removed) { + out.worktrees_removed.push(t.id) + if (!dry_run) await this.removeTaskWorktree(t, trigger).catch((e) => out.errors.push(`${t.id}: ${e.message}`)) + } else if (!t.worktree_removed) out.parked_kept.push(t.id) + continue + } + if (!TERMINAL.has(t.status)) continue const a = age(t) try { if (wd != null && a >= wd && !t.worktree_removed) { @@ -553,8 +729,17 @@ export class Manager { let finished24 = 0 const counts = {} let oldestWorktreeDays = null + let parked = 0 + let attention = 0 + let oldestParkedDays = null for (const t of this.tasks.values()) { counts[t.status] = (counts[t.status] || 0) + 1 + if (PARKED.has(t.status)) { + parked++ + const d = (Date.now() - Date.parse(t.handoff?.updated_at || t.finished_at || t.updated_at)) / 86400000 + oldestParkedDays = Math.max(oldestParkedDays ?? 0, Math.round(d * 10) / 10) + } + if (this.needsAttention(t)) attention++ if (t.finished_at && Date.parse(t.finished_at) >= day && t.result) { finished24++ verdicts[t.result.verdict] = (verdicts[t.result.verdict] || 0) + 1 @@ -579,6 +764,7 @@ export class Manager { running: this.live.size, tasks_by_status: counts, default_profile: pc.default_profile, profiles, finished_last_24h: finished24, verdicts_last_24h: verdicts, retention: cfg.retention, oldest_unremoved_worktree_days: oldestWorktreeDays, + parked_tasks: parked, oldest_parked_days: oldestParkedDays, needs_attention: attention, disk_free_mb: diskFreeMb, mem_available_mb: memAvailableMb(), default_backend: cfg.default_backend, bwrap: cfg.bwrap, backends: backendSummary(cfg).map((b) => ({ name: b.name, status: b.status, installed: b.installed, bin: b.bin, expected_version: cfg.backends[b.name]?.expected_version || null })), @@ -1088,6 +1274,7 @@ export class Manager { return { executed: true, command: cmd, source, sandboxed: cfg.test_sandbox.enabled, exit_code: res.code, timed_out: res.timedOut, passed: res.code === 0 && !res.timedOut, counts, duration_s: Math.round((Date.now() - started) / 100) / 10, + ...(res.code === 0 && !res.timedOut ? {} : { failing_tests: extractFailingTests(output) }), tail: tail(output, cfg.limits.test_tail_chars), } } @@ -1144,7 +1331,7 @@ export class Manager { let verdict if (commits || mainUnchanged === false) verdict = "integrity_violation" else if (status !== "completed") verdict = status === "failed" ? "worker_error" : status - else if (worker?.status === "blocked") verdict = "blocked" + else if (worker?.status === "blocked" || PARK_STATES.has(worker?.status)) verdict = "blocked" else if (t.mode === "implement" && diff.files.length === 0) verdict = "no_changes" else if (testRes.executed) verdict = testRes.passed ? "success" : "tests_failed" else verdict = "success_untested" @@ -1157,9 +1344,14 @@ export class Manager { })() const price = pc.price_per_mtok || null const est = price ? ((t.stats.tokens.input + t.stats.tokens.cache_read) * (price.input || 0) + (t.stats.tokens.output + t.stats.tokens.reasoning) * (price.output || 0)) / 1e6 : null + // Park the task for a human when the worker asks for approval/input, or reports blocked with a concrete + // request or after policy/sandbox-blocked tool calls (an action it considers necessary). + let finalStatus = status + if (verdict === "blocked" && (PARK_STATES.has(worker?.status) || (worker?.status === "blocked" && (hasText(worker.needs) || t.stats.blocked_calls > 0)))) finalStatus = "needs_approval" const first = t.runs[0] const last = t.runs[t.runs.length - 1] const finishedAt = now() + status = finalStatus t.status = status t.finished_at = finishedAt t.phase = "finished" @@ -1167,7 +1359,7 @@ export class Manager { task_id: t.id, session_id: t.session_id, status, verdict, mode: t.mode, repo: t.repo, profile: last?.profile || t.profile, backend: last?.backend || t.backend || "kilo", worker_model: last?.model || t.model, agent: t.agent, review_of: t.review_of || undefined, summary: head(worker?.summary || (t.last_text || "").trim() || "(no final message from worker)", cfg.limits.summary_max_chars), - worker_reported: worker ? { status: worker.status, files_changed: worker.files_changed, tests: worker.tests } : null, + worker_reported: worker ? { status: worker.status, files_changed: worker.files_changed, tests: worker.tests, ...(hasText(worker.needs) ? { needs: head(worker.needs.trim(), 500) } : {}) } : null, files_changed: t.mode === "review" && !reviewTouched ? [] : diff.files, reviewed_files: t.mode === "review" ? diff.files : undefined, diffstat: diff.shortstat, diff_path: path.join(dir, "diff.patch"), diff_bytes: Buffer.byteLength(diff.patch), worktree_path: t.worktree_removed ? null : t.worktree_path, branch: t.branch, base_ref: t.base_ref, base_commit: t.base_commit, tests_executed: [testRes.executed ? { command: testRes.command, by: "daemon", source: testRes.source, sandboxed: testRes.sandboxed } : null, worker?.tests ? { command: worker.tests, by: "worker (self-reported)" } : null].filter(Boolean), @@ -1188,8 +1380,11 @@ export class Manager { details_hint: "task_details kinds: activity, diff, test_log, final_message, raw_events, stderr", }) t.errors = [] + delete t.parked_from + t.handoff = redact(deriveHandoff(t, { at: finishedAt })) this.save(t) - this.event(t, "finished", { verdict, files: diff.files.length, tests_passed: testRes.passed ?? null }) + this.event(t, "finished", { verdict, files: diff.files.length, tests_passed: testRes.passed ?? null, handoff_state: t.handoff.state, owner: t.handoff.owner }) + if (PARKED.has(t.status)) this.event(t, "parked", { handoff_state: t.handoff.state, request: t.handoff.context?.worker_request || null }) this.schedule().catch(() => {}) } From 8a5eeb0cae23f0e79d6ae41f4144fea5f28f2dfc Mon Sep 17 00:00:00 2001 From: mrchatam <287639636+mrchatam@users.noreply.github.com> Date: Sat, 26 Sep 2026 23:40:51 +0330 Subject: [PATCH 2/9] Add update_handoff and approve_task (MCP, RPC, CLI) update_handoff edits owner, next_action, state and notes; parking states park a finished task, other states unpark it. approve_task approves (resume via the continue path with the approval noted) or rejects (close, or redirect with instructions). list_tasks gains the needs_attention and parked filters and an owner filter. CLI: workhorse handoff/approve/reject/attention, tasks --status/--owner, cleanup-old --parked-days. All decisions go to the audit log. --- bin/workhorse | 46 +++++++++++++++++++++++++++++++++++++++++----- bin/workhorse-mcp | 42 ++++++++++++++++++++++++++++++++++++------ lib/daemon.mjs | 7 +++++-- 3 files changed, 82 insertions(+), 13 deletions(-) diff --git a/bin/workhorse b/bin/workhorse index eecb78e..243dcae 100755 --- a/bin/workhorse +++ b/bin/workhorse @@ -13,9 +13,19 @@ Service: status quick daemon status (exit 1 if not running) health [--json] full check for a daily cron/agent run (exit 1 on any FAIL) logs [N] | audit [N] tail daemon.log / audit.jsonl - tasks [N] recent tasks - cleanup-old [--days N] [--task-days N] [--dry-run] - remove finished worktrees older than N days (default: daemon.json retention) + tasks [N] [--status S] [--owner O] + recent tasks (S: a status, active, terminal, parked, needs_attention) + attention [N] tasks that need someone: parked (needs_approval) or handoff not done/closed + cleanup-old [--days N] [--task-days N] [--parked-days N] [--dry-run] + remove finished worktrees older than N days (default: daemon.json retention; + parked tasks are kept unless --parked-days / retention.parked_days is set) +Handoff and approval: + handoff [--state S] [--owner O] [--next ""] [--note ""] [--by NAME] + show the task's handoff record, or update it when options are given + approve [--instructions ""] [--note ""] [--timeout MIN] [--profile P] [--by NAME] + approve a parked/blocked task and resume it in the same session + worktree + reject [--instructions ""] [--note ""] [--by NAME] + deny it: close the task, or resume with --instructions (do something else) Config (the config dir is root-owned once locked: use sudo for these): repos show the allowlist add-repo [--name N] [--test ""] [--allow-test ""]... [--base BRANCH] @@ -123,13 +133,39 @@ try { out(execFileSync("tail", ["-n", rest[0] || "20", path.join(dirs.logs, "audit.jsonl")]).toString()) break case "tasks": - out(await rpc("list_tasks", { limit: Number(rest[0]) || 20 })) + case "attention": { + const f = flags(rest, { status: "str", owner: "str" }) + const status = cmd === "attention" ? "needs_attention" : f.status + out(await rpc("list_tasks", { limit: Number(f._[0]) || 20, ...(status ? { status } : {}), ...(f.owner ? { owner: f.owner } : {}) })) break + } + case "handoff": { + const f = flags(rest, { state: "str", owner: "str", next: "str", note: "str", by: "str" }) + const id = f._[0] + if (!id) throw new Error("usage: workhorse handoff [--state S] [--owner O] [--next \"\"] [--note \"\"] [--by NAME]") + const upd = { ...(f.state ? { state: f.state } : {}), ...(f.owner ? { owner: f.owner } : {}), ...(f.next ? { next_action: f.next } : {}), ...(f.note ? { note: f.note } : {}) } + const caller = { caller: { client: "workhorse" } } + out(Object.keys(upd).length ? await rpc("update_handoff", { task_id: id, ...upd, ...(f.by ? { by: f.by } : {}) }, caller) : await rpc("get_handoff", { task_id: id }, caller)) + break + } + case "approve": + case "reject": { + const f = flags(rest, { instructions: "str", note: "str", timeout: "str", profile: "str", by: "str" }) + const id = f._[0] + if (!id) throw new Error(`usage: workhorse ${cmd} [--instructions ""] [--note ""]`) + out(await rpc("approve_task", { + task_id: id, decision: cmd, + ...(f.instructions ? { instructions: f.instructions } : {}), ...(f.note ? { note: f.note } : {}), + ...(f.timeout ? { timeout_minutes: Number(f.timeout) } : {}), ...(f.profile ? { profile: f.profile } : {}), ...(f.by ? { by: f.by } : {}), + }, { caller: { client: "workhorse" } })) + break + } case "cleanup-old": { - const f = flags(rest, { days: "str", "task-days": "str", "dry-run": "bool" }) + const f = flags(rest, { days: "str", "task-days": "str", "parked-days": "str", "dry-run": "bool" }) out(await rpc("cleanup_old", { ...(f.days !== undefined ? { worktree_days: Number(f.days) } : {}), ...(f["task-days"] !== undefined ? { task_days: Number(f["task-days"]) } : {}), + ...(f["parked-days"] !== undefined ? { parked_days: Number(f["parked-days"]) } : {}), dry_run: !!f["dry-run"], }, { caller: { client: "workhorse" }, timeoutMs: 600000 })) break diff --git a/bin/workhorse-mcp b/bin/workhorse-mcp index af991b2..f1c0e2f 100755 --- a/bin/workhorse-mcp +++ b/bin/workhorse-mcp @@ -10,7 +10,7 @@ import { daemonConfig, secretEnvNames, VERSION } from "../lib/config.mjs" const caller = { shim_pid: process.pid, ppid: process.ppid, client: process.env.WH_CALLER || null } const server = new McpServer({ name: "grok-workhorse", version: VERSION }, { instructions: - "Delegate coding tasks to a sandboxed coding-agent worker (Kilo CLI, OpenCode, ... per profile) running on the model profiles the owner configured (see list_models). Workflow: list_repos / list_models once -> delegate_task -> poll task_status every 30-60 s until terminal -> task_result (small structured summary) -> optionally task_details for the diff or logs -> review/merge the branch yourself -> cleanup_task. Workers never commit, merge or push; changes stay uncommitted in the task's worktree.", + "Delegate coding tasks to a sandboxed coding-agent worker (Kilo CLI, OpenCode, ... per profile) running on the model profiles the owner configured (see list_models). Workflow: list_repos / list_models once -> delegate_task -> poll task_status every 30-60 s until terminal -> task_result (small structured summary; its handoff.next_action says exactly what to do next and who must do it) -> optionally task_details for the diff or logs -> review/merge the branch yourself -> cleanup_task. Status needs_approval means the task is parked for a human decision: relay handoff.next_action and answer with approve_task. Record your own handoffs with update_handoff. Workers never commit, merge or push; changes stay uncommitted in the task's worktree.", }) let ready = null @@ -76,11 +76,11 @@ tool( }, ) -tool("task_status", "Cheap progress check for a task: status (queued, running, retry_wait, testing, completed, failed, timeout, stalled, cancelled, interrupted), phase, elapsed time, seconds since last worker activity, turn/tool counts, and verdict once finished.", { task_id: taskId }) +tool("task_status", "Cheap progress check for a task: status (queued, running, retry_wait, testing, completed, failed, timeout, stalled, cancelled, interrupted, or needs_approval = parked for a human decision), phase, elapsed time, seconds since last worker activity, turn/tool counts, and once finished the verdict and a handoff summary (state, owner, next_action). terminal is true once no worker is running (including needs_approval).", { task_id: taskId }) tool( "task_result", - "Structured result of a finished task (small): verdict (success, tests_failed, no_changes, success_untested, worker_error, timeout, stalled, cancelled, interrupted, blocked, integrity_violation), worker summary, files_changed + diffstat, daemon-run test results, errors, remaining_concerns, branch/worktree/diff_path, timings, token usage/cost, activity counts. Use task_details for the full diff or logs.", + "Structured result of a finished task (small): verdict (success, tests_failed, no_changes, success_untested, worker_error, timeout, stalled, cancelled, interrupted, blocked, integrity_violation), worker summary, files_changed + diffstat, daemon-run test results, errors, remaining_concerns, branch/worktree/diff_path, timings, token usage/cost, activity counts, and the handoff record: state (done, needs_fix, needs_review, needs_approval, needs_input, blocked, retryable, closed), failed_checks, owner (who acts next), next_action (one exact instruction), resume (tool + suggested args, session/worktree/branch) and context. Use task_details for the full diff or logs.", { task_id: taskId }, ) @@ -98,7 +98,7 @@ tool( tool( "continue_task", - "Send follow-up instructions to a finished (or failed/timed-out/interrupted/cancelled) task. The worker resumes the same agent session in the same worktree (when the profile uses the same backend; otherwise a fresh session in the same worktree), then tests and the result are recomputed. Use it for fixes after your review instead of starting over.", + "Send follow-up instructions to a finished (or failed/timed-out/interrupted/cancelled/needs_approval) task. The worker resumes the same agent session in the same worktree (when the profile uses the same backend; otherwise a fresh session in the same worktree), then tests and the result are recomputed. Use it for fixes after your review instead of starting over.", { task_id: taskId, instructions: z.string().min(1).max(20000).describe("What to change or fix next"), @@ -111,10 +111,11 @@ tool("cancel_task", "Stop a queued or running task. The worktree and partial cha tool( "list_tasks", - "List recent tasks (newest first) with status and verdict.", + "List recent tasks (newest first) with status, verdict and handoff (state, owner, next_action).", { - status: z.string().max(20).optional().describe("Filter: a status name, 'active' or 'terminal'"), + status: z.string().max(20).optional().describe("Filter: a status name, 'active', 'terminal', 'parked' (needs_approval) or 'needs_attention' (parked tasks plus finished tasks whose handoff is not done/closed)"), repo: z.string().max(64).optional(), + owner: z.string().max(80).optional().describe("Filter by handoff owner, e.g. 'human' or 'supervisor'"), limit: z.number().int().min(1).max(200).optional().describe("Default 20"), }, ) @@ -128,4 +129,33 @@ tool( }, ) +const by = z.string().max(80).optional().describe("Who is acting, recorded in the handoff and audit log (default 'supervisor')") + +tool( + "update_handoff", + "Update a finished task's handoff record so a later agent or a human can pick it up without guessing: owner (who acts next), next_action (one exact instruction), state and/or a note. Setting state needs_approval or needs_input parks the task for a human (status needs_approval, worktree kept); any other state unparks it.", + { + task_id: taskId, + state: z.enum(["done", "needs_fix", "needs_review", "needs_approval", "needs_input", "blocked", "retryable", "closed"]).optional(), + owner: z.string().max(80).optional().describe("'supervisor', 'human' or a named owner"), + next_action: z.string().max(2000).optional().describe("One exact, concrete instruction for the owner"), + note: z.string().max(2000).optional().describe("Free-text note appended to the handoff"), + by, + }, +) + +tool( + "approve_task", + "Answer a task that is parked (needs_approval) or blocked. decision=approve resumes it in the same session and worktree with the approval (and optional instructions) sent to the worker. decision=reject with instructions resumes it telling the worker not to do it; without instructions it closes the task (status cancelled, worktree kept). Approval does not lift sandbox rules.", + { + task_id: taskId, + decision: z.enum(["approve", "reject"]), + instructions: z.string().max(20000).optional().describe("Answer or extra instructions for the worker"), + note: z.string().max(2000).optional().describe("Reason, recorded with the decision"), + timeout_minutes: z.number().min(0).max(1440).optional(), + profile: z.string().max(32).optional(), + by, + }, +) + await server.connect(new StdioServerTransport()) diff --git a/lib/daemon.mjs b/lib/daemon.mjs index 8ebf265..aa542a7 100644 --- a/lib/daemon.mjs +++ b/lib/daemon.mjs @@ -29,7 +29,7 @@ export async function startDaemon() { const methods = { health: () => ({ ok: true, version: VERSION, pid: process.pid, running: mgr.live.size, tasks: mgr.tasks.size, credentials: Object.keys(mgr.secretEnv()), credential_sources: mgr.credentialSources() }), health_report: () => ({ ok: true, ...mgr.healthReport(), credential_sources: mgr.credentialSources() }), - cleanup_old: (p) => mgr.sweep({ worktree_days: p.worktree_days, task_days: p.task_days, dry_run: p.dry_run === true, trigger: "manual" }), + cleanup_old: (p) => mgr.sweep({ worktree_days: p.worktree_days, task_days: p.task_days, parked_days: p.parked_days, dry_run: p.dry_run === true, trigger: "manual" }), offer_env: (p) => mgr.offerEnv(p.env), delegate_task: (p) => mgr.delegate(p), task_status: (p) => mgr.status(p.task_id), @@ -38,6 +38,9 @@ export async function startDaemon() { continue_task: (p) => mgr.continueTask(p.task_id, p.instructions, p.timeout_minutes, p.profile), cancel_task: (p) => mgr.cancel(p.task_id), list_tasks: (p) => mgr.list(p), + get_handoff: (p) => mgr.handoff(p.task_id), + update_handoff: (p, caller) => mgr.updateHandoff(p, caller), + approve_task: (p, caller) => mgr.approve(p, caller), cleanup_task: (p) => mgr.cleanup(p.task_id, p.discard_unmerged_changes), list_repos: () => mgr.listRepos(), list_models: () => mgr.listModels(), @@ -75,7 +78,7 @@ export async function startDaemon() { if (!fn) return send(400, { error: `unknown method ${String(method).slice(0, 40)}` }) const started = Date.now() try { - const result = await fn(params || {}) + const result = await fn(params || {}, caller) if (method !== "health" && method !== "health_report" && !(method === "offer_env" && !result?.changed)) audit("rpc", { method, caller, params: sanitizeParams(method, params), ok: true, ms: Date.now() - started, task_id: params?.task_id || result?.task_id }) send(200, { result }) } catch (e) { From 575093e0cbf7986fba84ca856ed963790884626d Mon Sep 17 00:00:00 2001 From: mrchatam <287639636+mrchatam@users.noreply.github.com> Date: Sat, 26 Sep 2026 23:40:58 +0330 Subject: [PATCH 3/9] Test handoff derivation, approval flow, update_handoff and persistence Pure unit tests for each verdict's handoff, Manager-level flow tests that run without a backend CLI (needs_approval parking, retention, approve/reject, update_handoff, restart persistence, list filters), and a mock-LLM integration test through the daemon. --- package.json | 2 +- test/daemon.test.mjs | 30 +++ test/handoff-flow.test.mjs | 268 +++++++++++++++++++++++++ test/handoff.test.mjs | 179 +++++++++++++++++ test/mcp.test.mjs | 2 +- test/mock/scenarios/needsapproval.json | 21 ++ 6 files changed, 500 insertions(+), 2 deletions(-) create mode 100644 test/handoff-flow.test.mjs create mode 100644 test/handoff.test.mjs create mode 100644 test/mock/scenarios/needsapproval.json diff --git a/package.json b/package.json index 02c8723..78cf3a3 100644 --- a/package.json +++ b/package.json @@ -26,7 +26,7 @@ "scripts": { "setup": "npm ci && npm ci --prefix adapters/kilo/config && npm ci --prefix adapters/opencode/config", "test": "node --test --test-timeout=600000 test/*.test.mjs", - "test:unit": "node --test test/unit.test.mjs test/config.test.mjs test/adapters.test.mjs", + "test:unit": "node --test test/unit.test.mjs test/config.test.mjs test/adapters.test.mjs test/handoff.test.mjs test/handoff-flow.test.mjs", "test:live": "WH_LIVE_TEST=1 node --test --test-timeout=1800000 test/live.test.mjs" }, "engines": { diff --git a/test/daemon.test.mjs b/test/daemon.test.mjs index 19db82c..0df6b35 100644 --- a/test/daemon.test.mjs +++ b/test/daemon.test.mjs @@ -96,6 +96,10 @@ H.itest("escape attempts are blocked and nothing leaves the worktree", async () assert.match(act, /BLOCKED[^\n]*git commit/) assert.match(act, /BLOCKED[^\n]*git push/) assert.equal(res.verdict, "blocked") + // worker reported blocked after blocked tool calls: parked for a human, with the blocked calls as failed checks + assert.equal(res.status, "needs_approval") + assert.equal(res.handoff.owner, "human") + assert.ok(res.handoff.failed_checks.find((c) => c.check === "blocked_calls").count >= 5) if (H.TEST_BACKEND === "kilo") { assert.ok(res.activity.blocked_calls >= 8, `blocked_calls=${res.activity.blocked_calls}`) assert.match(act, /urlopen[^\n]*(Errno|Network|unreachable|resolution|URLError)/i) @@ -138,6 +142,32 @@ H.itest("continue_task resumes the same session and worktree", async () => { assert.equal(res.activity.runs, 2) }) +H.itest("needs_approval parks the task with a handoff; update_handoff and approve_task resume it", async () => { + const r = await delegate({ task: "MOCK_SCENARIO=needsapproval", test_command: "python3 -m unittest tests.test_stats -v" }) + const st = await H.waitFor(rpc, r.task_id, H.isTerminal) + assert.equal(st.status, "needs_approval") + assert.equal(st.parked, true) + assert.equal(st.handoff.state, "needs_approval") + const res = await rpc("task_result", { task_id: r.task_id }) + assert.equal(res.verdict, "blocked") + assert.equal(res.handoff.owner, "human") + assert.match(res.handoff.next_action, /approve adding NOTES\.md/) + assert.equal(res.handoff.resume.session_reusable, true) + assert.ok((await rpc("list_tasks", { status: "needs_attention", owner: "human" })).tasks.some((t) => t.task_id === r.task_id)) + const u = await rpc("update_handoff", { task_id: r.task_id, note: "owner is checking", by: "tester" }) + assert.equal(u.handoff.notes[0].note, "owner is checking") + const a = await rpc("approve_task", { task_id: r.task_id, decision: "approve", instructions: "Go ahead." }) + assert.equal(a.resumes_session, true) + await H.waitFor(rpc, r.task_id, H.isTerminal) + const res2 = await rpc("task_result", { task_id: r.task_id }) + assert.equal(res2.status, "completed") + assert.equal(res2.session_id, res.session_id) + assert.deepEqual(res2.files_changed.map((f) => f.path), ["NOTES.md"]) + assert.equal(res2.handoff.state, "done") + const log = fs.readFileSync(path.join(env.dataDir, "logs/audit.jsonl"), "utf8") + for (const e of ["parked", "handoff_updated", "approval"]) assert.match(log, new RegExp(`"event":"${e}"`)) +}) + H.itest("review mode works on a copy of another task's diff and cannot modify the original", async () => { const r = await rpc("delegate_task", { repo: H.REPO_NAME, mode: "review", review_task_id: doneTask.task_id, task: "Review for correctness. MOCK_SCENARIO=review_try_edit", test_command: "python3 -m unittest tests.test_core -v" }) assert.notEqual(r.worktree_path, doneTask.worktree_path) diff --git a/test/handoff-flow.test.mjs b/test/handoff-flow.test.mjs new file mode 100644 index 0000000..627425e --- /dev/null +++ b/test/handoff-flow.test.mjs @@ -0,0 +1,268 @@ +// Handoff + approval flow through the task Manager, without a backend CLI or sandbox: tasks are created +// with delegate(), a worker run is simulated (worktree edits + final message), then finalize() runs the +// real diff/test/verdict/handoff code. Runs in CI. +import { test, before, after } from "node:test" +import assert from "node:assert/strict" +import fs from "node:fs" +import os from "node:os" +import path from "node:path" +import { execFileSync } from "node:child_process" +import { fileURLToPath } from "node:url" + +const APP = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..") +const root = fs.mkdtempSync(path.join(os.tmpdir(), "kwt-handoff-")) +const cfgDir = path.join(root, "config") +const dataDir = path.join(root, "data") +const repoPath = path.join(root, "src-repo") +fs.mkdirSync(cfgDir, { recursive: true }) +fs.mkdirSync(repoPath, { recursive: true }) +fs.writeFileSync(path.join(repoPath, "a.txt"), "one\n") +const g = (...a) => execFileSync("git", ["-C", repoPath, ...a], { stdio: "ignore" }) +g("init", "-q", "-b", "main") +g("add", "-A") +g("-c", "user.name=test", "-c", "user.email=test@localhost", "commit", "-qm", "init") +const PASS = "echo '# pass 1'; exit 0" +const FAIL = "echo 'not ok 1 - adds numbers'; echo '# fail 1'; exit 1" +fs.writeFileSync(path.join(cfgDir, "daemon.json"), JSON.stringify({ + test_sandbox: { enabled: false }, worker_sandbox: { enabled: false }, retention: { enabled: false, worktree_days: 7, task_days: 30 }, + timeouts: { default_min: 5, min_min: 0.05, max_min: 30, stall_min: 5, test_min: 1, kill_grace_sec: 1 }, secret_env: [], min_mem_available_mb: 0, +})) +fs.writeFileSync(path.join(cfgDir, "repos.json"), JSON.stringify({ repos: { demo: { path: repoPath, default_base: "main", test_command: PASS, allowed_test_commands: [".*"] } } })) +fs.writeFileSync(path.join(cfgDir, "profiles.json"), fs.readFileSync(path.join(APP, "test/fixtures/profiles.json"), "utf8").replaceAll("__MOCK_PORT__", "9")) +process.env.WH_CONFIG_DIR = cfgDir +process.env.WH_DATA_DIR = dataDir + +const { Manager } = await import("../lib/tasks.mjs") +const { dirs } = await import("../lib/config.mjs") + +// No worker is ever spawned: scheduling and the backend check are stubbed out. +function manager() { + const m = new Manager() + m.schedule = async () => {} + m.checkBackend = () => {} + m.load() + return m +} +let mgr +before(() => { + for (const d of Object.values(dirs)) fs.mkdirSync(d, { recursive: true }) + mgr = manager() +}) +after(() => fs.rmSync(root, { recursive: true, force: true })) + +const RESULT = (status, extra = "") => `Work report.\n\n## RESULT\nstatus: ${status}\nsummary: did some work\nfiles_changed: b.txt\ntests: sh -> ok\nconcerns: none\n${extra}` +// Create a task and simulate a finished worker run. +async function finishedTask({ text, edit = true, test = PASS, blocked = [] } = {}) { + const d = await mgr.delegate({ repo: "demo", task: "add b.txt", test_command: test }) + const t = mgr.tasks.get(d.task_id) + t.test_command = test + if (edit) fs.writeFileSync(path.join(t.worktree_path, "b.txt"), "two\n") + t.session_id = "ses_test" + t.session_backend = t.backend + t.last_text = text + t.started_at = new Date().toISOString() + t.runs.push({ n: t.runs.length + 1, kind: "initial", profile: t.profile, backend: t.backend, started_at: t.started_at, finished_at: new Date().toISOString(), exit_code: 0 }) + for (const b of blocked) { t.stats.blocked_calls++; t.blocked.push(b) } + await mgr.finalize(t, { tests: true, status: "completed" }) + return t +} +const audit = () => fs.readFileSync(path.join(dirs.logs, "audit.jsonl"), "utf8").trim().split("\n").map((l) => JSON.parse(l)) + +test("success: done handoff in task_result, status and list_tasks", async () => { + const t = await finishedTask({ text: RESULT("done") }) + assert.equal(t.status, "completed") + const r = mgr.result(t.id) + assert.equal(r.verdict, "success") + assert.equal(r.handoff.state, "done") + assert.equal(r.handoff.owner, "supervisor") + assert.equal(r.handoff.updated_by, "daemon") + assert.equal(r.handoff.resume.session_reusable, true) + assert.equal(r.handoff.resume.worktree_path, t.worktree_path) + const s = mgr.status(t.id) + assert.equal(s.terminal, true) + assert.equal(s.parked, false) + assert.equal(s.handoff.state, "done") + assert.equal(mgr.list({}).tasks.find((x) => x.task_id === t.id).handoff.state, "done") + assert.ok(!mgr.list({ status: "needs_attention" }).tasks.some((x) => x.task_id === t.id)) + assert.ok(mgr.list({ status: "completed" }).tasks.some((x) => x.task_id === t.id), "plain status filter still works") +}) + +test("tests_failed: needs_fix with the failing test, in needs_attention", async () => { + const t = await finishedTask({ text: RESULT("done"), test: FAIL }) + const r = mgr.result(t.id) + assert.equal(r.verdict, "tests_failed") + assert.deepEqual(r.test_results.failing_tests, ["adds numbers"]) + assert.equal(r.handoff.state, "needs_fix") + assert.match(r.handoff.next_action, /adds numbers/) + assert.ok(mgr.list({ status: "needs_attention" }).tasks.some((x) => x.task_id === t.id)) + assert.ok(mgr.list({ status: "terminal" }).tasks.some((x) => x.task_id === t.id)) +}) + +test("needs_approval: worker request parks the task; filters, retention and approve", async () => { + const t = await finishedTask({ text: RESULT("needs_approval", "needs: approve adding dependency left-pad to package.json") }) + assert.equal(t.status, "needs_approval") + const r = mgr.result(t.id) + assert.equal(r.status, "needs_approval") + assert.equal(r.verdict, "blocked", "verdict stays in the existing set") + assert.equal(r.worker_reported.needs, "approve adding dependency left-pad to package.json") + assert.equal(r.handoff.state, "needs_approval") + assert.equal(r.handoff.owner, "human") + assert.match(r.handoff.next_action, /left-pad/) + assert.equal(r.handoff.resume.tool, "approve_task") + const s = mgr.status(t.id) + assert.equal(s.terminal, true, "pollers stop polling a parked task") + assert.equal(s.parked, true) + assert.match(s.hint, /approve_task/) + for (const f of [{ status: "needs_approval" }, { status: "parked" }, { status: "needs_attention" }, { status: "needs_attention", owner: "human" }, { status: "terminal" }]) + assert.ok(mgr.list(f).tasks.some((x) => x.task_id === t.id), JSON.stringify(f)) + assert.ok(!mgr.list({ status: "active" }).tasks.some((x) => x.task_id === t.id)) + assert.ok(!mgr.list({ owner: "supervisor" }).tasks.some((x) => x.task_id === t.id)) + // retention never removes a parked worktree unless parked_days is set + const sw = await mgr.sweep({ worktree_days: 0, dry_run: true }) + assert.ok(!sw.worktrees_removed.includes(t.id)) + assert.ok(sw.parked_kept.includes(t.id)) + assert.ok((await mgr.sweep({ worktree_days: 0, parked_days: 0, dry_run: true })).worktrees_removed.includes(t.id)) + await mgr.sweep({ worktree_days: 0, task_days: 0, dry_run: false, trigger: "test" }).catch(() => {}) + assert.ok(fs.existsSync(t.worktree_path), "parked worktree survives a real sweep") + assert.ok(mgr.tasks.has(t.id)) + const h = mgr.healthReport() + assert.ok(h.parked_tasks >= 1 && h.needs_attention >= 1) + // approve resumes via the continue path + const a = await mgr.approve({ task_id: t.id, decision: "approve", instructions: "Use version 1.3.0.", note: "fine by me", by: "reviewer-1" }) + assert.equal(a.status, "queued") + assert.equal(a.decision, "approve") + assert.equal(a.resumes_session, true) + assert.equal(t.pending_run.kind, "continue") + assert.match(t.pending_run.message, /APPROVED by reviewer-1: approve adding dependency left-pad/) + assert.match(t.pending_run.message, /Use version 1\.3\.0\./) + assert.equal(t.approvals.length, 1) + assert.equal(t.approvals[0].by, "reviewer-1") + const prev = t.previous_results.at(-1) + assert.equal(prev.handoff.state, "needs_approval") + assert.equal(prev.approval.decision, "approve") + assert.equal(mgr.status(t.id).handoff, null, "no handoff while active") + // the follow-up run finishes + t.last_text = RESULT("done") + await mgr.finalize(t, { tests: true, status: "completed" }) + assert.equal(t.status, "completed") + assert.equal(mgr.result(t.id).handoff.state, "done") + assert.equal(mgr.result(t.id).handoff.context.previous_attempts, 1) + const ev = audit().filter((l) => l.task_id === t.id).map((l) => l.event) + for (const e of ["parked", "approval", "continue_queued", "finished"]) assert.ok(ev.includes(e), e) +}) + +test("blocked with a blocked tool call parks; plain blocked does not", async () => { + const t = await finishedTask({ text: RESULT("blocked"), edit: false, blocked: [{ tool: "bash", input: "npm install left-pad", error: "workhorse guard blocked: network" }] }) + assert.equal(t.status, "needs_approval") + assert.match(mgr.result(t.id).handoff.next_action, /npm install left-pad/) + const p = await finishedTask({ text: RESULT("blocked"), edit: false }) + assert.equal(p.status, "completed") + assert.equal(mgr.result(p.id).verdict, "blocked") + assert.equal(mgr.result(p.id).handoff.state, "blocked") +}) + +test("reject: closes a parked task (worktree kept), or resumes it with instructions", async () => { + const t = await finishedTask({ text: RESULT("needs_input", "needs: which config format, YAML or TOML?") }) + assert.equal(mgr.result(t.id).handoff.state, "needs_input") + const r = await mgr.approve({ task_id: t.id, decision: "reject", note: "out of scope" }) + assert.equal(r.status, "cancelled") + assert.equal(r.handoff.state, "closed") + assert.equal(r.handoff.updated_by, "supervisor") + assert.match(r.handoff.next_action, /approval rejected by supervisor: out of scope/) + assert.equal(mgr.result(t.id).verdict, "blocked") + assert.equal(mgr.result(t.id).status, "cancelled") + assert.ok(fs.existsSync(t.worktree_path)) + assert.ok(!mgr.list({ status: "needs_attention" }).tasks.some((x) => x.task_id === t.id)) + await assert.rejects(mgr.approve({ task_id: t.id, decision: "approve" }), /not waiting for approval/) + // continue_task still works from the closed task + assert.equal((await mgr.continueTask(t.id, "do it after all")).status, "queued") + + const u = await finishedTask({ text: RESULT("needs_approval", "needs: approve deleting the legacy module") }) + const r2 = await mgr.approve({ task_id: u.id, decision: "reject", instructions: "Keep the legacy module; deprecate it instead." }, { client: "workhorse" }) + assert.equal(r2.status, "queued") + assert.match(u.pending_run.message, /^DENIED by human \(cli\): approve deleting the legacy module\. Do not do that\./) + assert.match(u.pending_run.message, /deprecate it instead/) + + const c = await finishedTask({ text: RESULT("needs_approval", "needs: approve X") }) + assert.equal((await mgr.cancel(c.id)).status, "cancelled") + assert.equal(mgr.result(c.id).handoff.state, "closed") + const ev = audit().filter((l) => l.task_id === t.id).map((l) => l.event) + for (const e of ["approval", "closed"]) assert.ok(ev.includes(e), e) +}) + +test("approve_task validation", async () => { + const t = await finishedTask({ text: RESULT("done") }) + await assert.rejects(mgr.approve({ task_id: t.id, decision: "approve" }), /not waiting for approval/) + await assert.rejects(mgr.approve({ task_id: t.id, decision: "maybe" }), /decision must be/) + await assert.rejects(mgr.approve({ task_id: t.id, decision: "approve", bogus: 1 }), /unknown parameter 'bogus'/) + await assert.rejects(mgr.approve({ task_id: "wh-bad", decision: "approve" }), /invalid task_id/) + const q = await mgr.delegate({ repo: "demo", task: "queued only" }) + await assert.rejects(mgr.approve({ task_id: q.task_id, decision: "approve" }), /nothing to approve yet/) +}) + +test("update_handoff: owner/next_action/note, parking by state, validation", async () => { + const t = await finishedTask({ text: RESULT("done"), test: FAIL }) + const u = mgr.updateHandoff({ task_id: t.id, owner: "release-bot", next_action: "Fix adds numbers in calc.js, then rerun npm test", note: "flaky on CI too", by: "supervisor-1" }) + assert.equal(u.handoff.owner, "release-bot") + assert.equal(u.handoff.next_action, "Fix adds numbers in calc.js, then rerun npm test") + assert.equal(u.handoff.notes[0].note, "flaky on CI too") + assert.equal(u.handoff.updated_by, "supervisor-1") + assert.equal(u.handoff.derived, false) + assert.equal(u.handoff.state, "needs_fix", "state untouched") + assert.deepEqual(u.handoff.history.at(-1).changed, ["owner", "next_action", "note"]) + assert.ok(mgr.list({ owner: "release-bot" }).tasks.some((x) => x.task_id === t.id)) + // park for a human, then unpark + const p = mgr.updateHandoff({ task_id: t.id, state: "needs_approval", next_action: "Human must decide whether the flaky test may be skipped" }, { client: "workhorse" }) + assert.equal(p.status, "needs_approval") + assert.equal(p.handoff.owner, "human") + assert.equal(p.handoff.updated_by, "human (cli)") + assert.equal(t.parked_from, "completed") + const back = mgr.updateHandoff({ task_id: t.id, state: "closed" }) + assert.equal(back.status, "completed") + assert.equal(back.handoff.state, "closed") + assert.ok(!mgr.list({ status: "needs_attention" }).tasks.some((x) => x.task_id === t.id)) + // validation + assert.throws(() => mgr.updateHandoff({ task_id: t.id }), /nothing to update/) + assert.throws(() => mgr.updateHandoff({ task_id: t.id, state: "whatever" }), /state must be one of/) + assert.throws(() => mgr.updateHandoff({ task_id: t.id, owner: "x\ny" }), /owner must be/) + assert.throws(() => mgr.updateHandoff({ task_id: t.id, note: " " }), /note must be/) + assert.throws(() => mgr.updateHandoff({ task_id: t.id, note: "ok", by: "$(id)" }), /by must be/) + assert.throws(() => mgr.updateHandoff({ task_id: t.id, owner: "a", extra: 1 }), /unknown parameter 'extra'/) + const q = await mgr.delegate({ repo: "demo", task: "still queued" }) + assert.throws(() => mgr.updateHandoff({ task_id: q.task_id, owner: "human" }), /once the task has finished/) + assert.ok(audit().some((l) => l.task_id === t.id && l.event === "handoff_updated" && l.by === "supervisor-1")) +}) + +test("persistence across restart, and tasks from v0.1.0 without a handoff", async () => { + const parked = await finishedTask({ text: RESULT("needs_approval", "needs: approve raising the timeout") }) + const edited = await finishedTask({ text: RESULT("done") }) + mgr.updateHandoff({ task_id: edited.id, owner: "human", note: "check the wording" }) + const legacy = await finishedTask({ text: RESULT("done"), test: FAIL }) + const file = path.join(dirs.tasks, legacy.id, "task.json") + const j = JSON.parse(fs.readFileSync(file, "utf8")) + delete j.handoff + delete j.result.test_results.failing_tests + fs.writeFileSync(file, JSON.stringify(j)) + const before = { p: mgr.result(parked.id).handoff, e: mgr.result(edited.id).handoff } + + const m2 = manager() // a restarted daemon reads task.json files + assert.equal(m2.status(parked.id).status, "needs_approval") + assert.deepEqual(m2.result(parked.id).handoff, before.p) + assert.deepEqual(m2.result(edited.id).handoff, before.e) + assert.equal(m2.result(edited.id).handoff.notes[0].note, "check the wording") + const lh = m2.result(legacy.id).handoff + assert.equal(lh.state, "needs_fix", "derived on the fly for old tasks") + assert.match(lh.next_action, /adds numbers/) + assert.ok(m2.list({ status: "parked" }).tasks.some((x) => x.task_id === parked.id)) + await m2.recover() // restart recovery leaves parked tasks alone + assert.equal(m2.tasks.get(parked.id).status, "needs_approval") + m2.schedule = async () => {} + const a = await m2.approve({ task_id: parked.id, decision: "approve" }) + assert.equal(a.status, "queued") + assert.match(m2.tasks.get(parked.id).pending_run.message, /approve raising the timeout/) + // cleanup removes the worktree; the handoff's resume facts follow + const done = await finishedTask({ text: RESULT("done") }) + await mgr.cleanup(done.id, true) + assert.equal(mgr.result(done.id).handoff.resume.worktree_exists, false) + assert.equal(mgr.result(done.id).handoff.resume.session_reusable, false) +}) diff --git a/test/handoff.test.mjs b/test/handoff.test.mjs new file mode 100644 index 0000000..fe63746 --- /dev/null +++ b/test/handoff.test.mjs @@ -0,0 +1,179 @@ +// Handoff derivation: pure unit tests (no daemon, no backend CLI). +import { test } from "node:test" +import assert from "node:assert/strict" +import { deriveHandoff, extractFailingTests, normalizeWorkerStatus, handoffSummary, HANDOFF_STATES } from "../lib/handoff.mjs" +import { parseResultBlock } from "../lib/tasks.mjs" + +const ID = "wh-20260926-120000-ab12" +function task(result = {}, extra = {}) { + const base = { + id: ID, mode: "implement", status: "completed", branch: `workhorse/${ID}`, worktree_path: `/data/worktrees/r/${ID}`, worktree_removed: false, + session_id: "ses_1", base_commit: "abc123", timeout_min: 30, previous_results: [], + ...extra, + } + base.result = { + verdict: "success", status: base.status, summary: "did it", worker_reported: { status: "done" }, files_changed: [{ path: "a.py" }], diffstat: "1 file changed", + diff_path: `/data/tasks/${ID}/diff.patch`, worktree_path: base.worktree_path, test_results: { executed: true, passed: true, command: "pytest", exit_code: 0 }, + errors: [], remaining_concerns: [], integrity: { commits_made: 0, main_clone_unchanged: true }, activity: { blocked_calls: 0 }, blocked_examples: [], + ...result, + } + return base +} +const checks = (h) => h.failed_checks.map((c) => c.check) + +test("parseResultBlock reads needs and normalizes approval statuses", () => { + const r = parseResultBlock("x\n## RESULT\nstatus: needs approval\nsummary: stuck\nfiles_changed: none\ntests: none\nconcerns: none\nneeds: approve adding left-pad to package.json") + assert.equal(r.status, "needs_approval") + assert.equal(r.needs, "approve adding left-pad to package.json") + assert.equal(parseResultBlock("## RESULT\nstatus: needs_input").status, "needs_input") + assert.equal(parseResultBlock("## RESULT\nstatus: blocked | partial").status, "blocked") + for (const [a, b] of [["Needs-Approval", "needs_approval"], ["approval required", "needs_approval"], ["awaiting input", "needs_input"], ["needs human input", "needs_input"], ["done.", "done."], ["partial, mostly", "partial"]]) assert.equal(normalizeWorkerStatus(a), b, a) +}) + +test("extractFailingTests understands common runners", () => { + const out = [ + "FAIL: test_divide (tests.test_core.TestCore.test_divide)", "ERROR: test_mul (tests.test_core.TestCore)", + "FAILED tests/test_x.py::test_y - AssertionError", "not ok 3 - adds numbers", "--- FAIL: TestParse (0.00s)", "test parser::works ... FAILED", " ✕ renders header (5 ms)", + ].join("\n") + assert.deepEqual(extractFailingTests(out), ["test_divide (tests.test_core.TestCore.test_divide)", "test_mul (tests.test_core.TestCore)", "tests/test_x.py::test_y", "adds numbers", "TestParse", "parser::works", "renders header (5 ms)"]) + assert.deepEqual(extractFailingTests(""), []) + assert.equal(extractFailingTests(Array.from({ length: 30 }, (_, i) => `not ok ${i} - t${i}`).join("\n")).length, 10) +}) + +test("success -> done, owner supervisor, merge next action, resume facts", () => { + const h = deriveHandoff(task(), { at: "2026-09-26T12:00:00.000Z" }) + assert.equal(h.state, "done") + assert.equal(h.owner, "supervisor") + assert.match(h.next_action, /merge branch workhorse\/wh-/) + assert.deepEqual(h.failed_checks, []) + assert.equal(h.resume.task_id, ID) + assert.equal(h.resume.tool, null) + assert.equal(h.resume.session_reusable, true) + assert.equal(h.resume.worktree_path, `/data/worktrees/r/${ID}`) + assert.equal(h.resume.branch, `workhorse/${ID}`) + assert.equal(h.context.summary, "did it") + assert.equal(h.context.diffstat, "1 file changed") + assert.equal(h.updated_at, "2026-09-26T12:00:00.000Z") + assert.equal(h.updated_by, "daemon") + for (const k of ["state", "failed_checks", "owner", "next_action", "resume", "context", "updated_at", "updated_by"]) assert.ok(k in h, k) + assert.ok(HANDOFF_STATES.includes(h.state)) +}) + +test("success variants: review mode, partial, missing RESULT block, untested", () => { + assert.match(deriveHandoff(task({ files_changed: [] }, { mode: "review" })).next_action, /review findings/) + const partial = deriveHandoff(task({ worker_reported: { status: "partial" }, remaining_concerns: ["divide not done"] })) + assert.equal(partial.state, "needs_fix") + assert.equal(partial.resume.tool, "continue_task") + assert.match(partial.next_action, /partial \(divide not done\)/) + assert.ok(checks(partial).includes("worker_status")) + const noBlock = deriveHandoff(task({ worker_reported: null })) + assert.equal(noBlock.state, "needs_fix") + assert.ok(checks(noBlock).includes("result_block")) + const untested = deriveHandoff(task({ verdict: "success_untested", test_results: { executed: false, reason: "no test command configured or detected" } })) + assert.equal(untested.state, "needs_review") + assert.match(untested.next_action, /No tests ran \(no test command configured or detected\)/) +}) + +test("tests_failed -> needs_fix with the failing check, test names and a continue_task suggestion", () => { + const h = deriveHandoff(task({ + verdict: "tests_failed", + test_results: { executed: true, passed: false, command: "python3 -m unittest -v", exit_code: 1, timed_out: false, counts: { ran: 5, failures: 1 }, tail: "FAIL: test_divide (tests.test_core.TestCore)\nRan 5 tests\nFAILED (failures=1)" }, + remaining_concerns: ["daemon-run tests failed (exit 1); see task_details kind=test_log"], + })) + assert.equal(h.state, "needs_fix") + assert.equal(h.owner, "supervisor") + const c = h.failed_checks.find((x) => x.check === "tests") + assert.equal(c.command, "python3 -m unittest -v") + assert.equal(c.exit_code, 1) + assert.deepEqual(c.failing_tests, ["test_divide (tests.test_core.TestCore)"]) + assert.match(c.excerpt, /FAILED \(failures=1\)/) + assert.match(h.next_action, /^Run continue_task with instructions to fix the failing test\(s\) test_divide/) + assert.equal(h.resume.tool, "continue_task") + assert.equal(h.resume.args.task_id, ID) + assert.match(h.resume.args.instructions, /do not weaken or delete tests/) + // failing_tests computed by the daemon from the full log win over the tail + const h2 = deriveHandoff(task({ verdict: "tests_failed", test_results: { executed: true, passed: false, command: "npm test", exit_code: 1, tail: "", failing_tests: ["adds numbers"] } })) + assert.match(h2.next_action, /adds numbers/) +}) + +test("no_changes -> needs_fix", () => { + const h = deriveHandoff(task({ verdict: "no_changes", files_changed: [] })) + assert.equal(h.state, "needs_fix") + assert.ok(checks(h).includes("no_changes")) + assert.equal(h.resume.tool, "continue_task") +}) + +test("worker_error -> retryable, or blocked on a human for setup problems", () => { + const r = deriveHandoff(task({ verdict: "worker_error", errors: ["kilo exited with code 1; see task_details kind=stderr"], test_results: { executed: false } }, { status: "failed" })) + assert.equal(r.state, "retryable") + assert.equal(r.owner, "supervisor") + assert.ok(checks(r).includes("worker_error")) + const s = deriveHandoff(task({ verdict: "worker_error", errors: ["missing credentials: NVIDIA_API_KEY. Not in the workhorse daemon env"], test_results: { executed: false } }, { status: "failed" })) + assert.equal(s.state, "blocked") + assert.equal(s.owner, "human") + assert.match(s.next_action, /^Human must fix the setup: missing credentials/) +}) + +test("timeout, stalled, interrupted -> retryable with continue_task", () => { + const t = deriveHandoff(task({ verdict: "timeout", errors: ["wall-clock timeout of 30 min reached"] }, { status: "timeout" })) + assert.equal(t.state, "retryable") + assert.deepEqual(checks(t), ["timeout"]) + assert.equal(t.resume.args.timeout_minutes, 45) + const s = deriveHandoff(task({ verdict: "stalled", errors: ["no activity for 15 min (stalled)"] }, { status: "stalled" })) + assert.deepEqual(checks(s), ["stall"]) + assert.match(s.next_action, /stalled/) + const i = deriveHandoff(task({ verdict: "interrupted", test_results: { executed: false } }, { status: "interrupted" })) + assert.equal(i.state, "retryable") + assert.ok(checks(i).includes("interrupted")) + assert.equal(i.resume.session_reusable, true) +}) + +test("cancelled -> closed", () => { + const h = deriveHandoff(task({ verdict: "cancelled", test_results: { executed: false } }, { status: "cancelled" })) + assert.equal(h.state, "closed") + assert.equal(h.resume.tool, null) +}) + +test("blocked: parked needs_approval / needs_input use the worker's request; plain blocked goes to a human", () => { + const blockedEx = [{ tool: "bash", input: "npm install left-pad", error: "workhorse guard blocked: network" }] + const p = deriveHandoff(task({ verdict: "blocked", worker_reported: { status: "needs_approval", needs: "approve adding left-pad" }, activity: { blocked_calls: 1 }, blocked_examples: blockedEx }, { status: "needs_approval" })) + assert.equal(p.state, "needs_approval") + assert.equal(p.owner, "human") + assert.match(p.next_action, /^Human must approve or deny: approve adding left-pad/) + assert.equal(p.resume.tool, "approve_task") + assert.equal(p.context.worker_request, "approve adding left-pad") + const bc = p.failed_checks.find((c) => c.check === "blocked_calls") + assert.equal(bc.count, 1) + assert.equal(bc.examples[0].input, "npm install left-pad") + const q = deriveHandoff(task({ verdict: "blocked", worker_reported: { status: "needs_input", needs: "which API version?" } }, { status: "needs_approval" })) + assert.equal(q.state, "needs_input") + assert.match(q.next_action, /^Human must answer the worker: which API version\?/) + // parked because of blocked calls, no explicit request: the blocked action is named + const b = deriveHandoff(task({ verdict: "blocked", worker_reported: { status: "blocked" }, activity: { blocked_calls: 1 }, blocked_examples: blockedEx, remaining_concerns: ["1 tool call(s) were blocked by policy/sandbox (see task_details kind=activity)"] }, { status: "needs_approval" })) + assert.equal(b.state, "needs_approval") + assert.match(b.next_action, /bash `npm install left-pad`/) + const plain = deriveHandoff(task({ verdict: "blocked", worker_reported: { status: "blocked" }, remaining_concerns: ["go toolchain missing"] })) + assert.equal(plain.state, "blocked") + assert.equal(plain.owner, "human") + assert.match(plain.next_action, /go toolchain missing/) +}) + +test("integrity_violation -> blocked on a human, no resume tool", () => { + const h = deriveHandoff(task({ verdict: "integrity_violation", integrity: { commits_made: 2, main_clone_unchanged: true }, remaining_concerns: ["INTEGRITY: 2 commit(s) were created on workhorse/x (workers must not commit)"] })) + assert.equal(h.state, "blocked") + assert.equal(h.owner, "human") + assert.equal(h.failed_checks[0].check, "integrity") + assert.equal(h.failed_checks[0].commits_made, 2) + assert.match(h.next_action, /Do not merge/) + assert.equal(h.resume.tool, null) +}) + +test("removed worktree: session not reusable; summary form", () => { + const h = deriveHandoff(task({}, { worktree_removed: true })) + assert.equal(h.resume.session_reusable, false) + assert.equal(h.resume.worktree_path, null) + const s = handoffSummary(deriveHandoff(task({ verdict: "tests_failed", test_results: { executed: true, passed: false, command: "x", exit_code: 2 } }))) + assert.deepEqual(Object.keys(s), ["state", "owner", "next_action", "failed_checks", "updated_at", "updated_by"]) + assert.deepEqual(s.failed_checks, ["tests"]) + assert.equal(handoffSummary(null), null) +}) diff --git a/test/mcp.test.mjs b/test/mcp.test.mjs index 9002517..cfe38bc 100644 --- a/test/mcp.test.mjs +++ b/test/mcp.test.mjs @@ -37,7 +37,7 @@ after(async () => { H.itest("tools are exposed with schemas", async () => { const c = await mkClient() const { tools } = await c.listTools() - assert.deepEqual(tools.map((t) => t.name).sort(), ["cancel_task", "cleanup_task", "continue_task", "delegate_task", "list_models", "list_repos", "list_tasks", "task_details", "task_result", "task_status"]) + assert.deepEqual(tools.map((t) => t.name).sort(), ["approve_task", "cancel_task", "cleanup_task", "continue_task", "delegate_task", "list_models", "list_repos", "list_tasks", "task_details", "task_result", "task_status", "update_handoff"]) for (const t of tools) assert.ok(t.description.length > 40, t.name) await c.close() }) diff --git a/test/mock/scenarios/needsapproval.json b/test/mock/scenarios/needsapproval.json new file mode 100644 index 0000000..5560117 --- /dev/null +++ b/test/mock/scenarios/needsapproval.json @@ -0,0 +1,21 @@ +{ + "steps": [ + { + "text": "I need a decision before going further.\n\n## RESULT\nstatus: needs_approval\nsummary: Stopped before adding a dependency.\nfiles_changed: none\ntests: not run\nconcerns: none\nneeds: approve adding NOTES.md with the release notes" + }, + { + "tool_calls": [ + { + "name": "write", + "arguments": { + "filePath": "NOTES.md", + "content": "release notes\n" + } + } + ] + }, + { + "text": "Added the notes after approval.\n\n## RESULT\nstatus: done\nsummary: Added NOTES.md as approved.\nfiles_changed: NOTES.md\ntests: python3 -m unittest -> pass\nconcerns: none" + } + ] +} From bc61fdfc13d7983de69e77bb01b224fac7e1666d Mon Sep 17 00:00:00 2001 From: mrchatam <287639636+mrchatam@users.noreply.github.com> Date: Sat, 26 Sep 2026 23:40:58 +0330 Subject: [PATCH 4/9] Docs and version 0.2.0 for task handoff and approval docs/handoff.md (fields, verdict table, parked status, approval flow), architecture/configuration/security/troubleshooting updates, README feature and CLI entries, delegation skill guidance (read handoff.next_action, relay approvals, hand off with update_handoff), CHANGELOG 0.2.0 (Unreleased) and package version 0.2.0. --- CHANGELOG.md | 48 +++++++ README.md | 12 +- docs/architecture.md | 30 ++++- docs/configuration.md | 2 +- docs/handoff.md | 142 ++++++++++++++++++++ docs/security.md | 4 + docs/troubleshooting.md | 11 ++ grok-template/workhorse-delegation/SKILL.md | 23 +++- package-lock.json | 4 +- package.json | 2 +- 10 files changed, 265 insertions(+), 13 deletions(-) create mode 100644 docs/handoff.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 7ed5684..6d2b85c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,53 @@ All notable changes to this project are documented here. The format follows ## [Unreleased] +## [0.2.0] - Unreleased + +Task handoff records and a human-approval state, based on community feedback. A supervisor should keep a +blocked task as blocked, with the failed check, the owner and the exact next action, so a later agent +can resume it without guessing. + +### Added +- **Handoff record** on every finished task (`task.handoff`, persisted in `task.json`): `state` (`done`, + `needs_fix`, `needs_review`, `needs_approval`, `needs_input`, `blocked`, `retryable`, `closed`), + `failed_checks` (tests with command, exit code, failing test names and an excerpt; integrity; blocked + tool calls; timeout, stall or interrupt; worker error; missing `## RESULT`; no changes), `owner`, + `next_action`, `resume` (suggested tool + args, session reusable, worktree, branch), `context`, + `notes`, `history`, `updated_at` / `updated_by`. The daemon derives it deterministically from the + verdict and the data it already has. `task_result` returns it in full, and `task_status` and + `list_tasks` return a summary. Tasks finished before 0.2.0 get a derived record on the fly. +- **Parked status `needs_approval`**: the task waits for a human decision when the worker reports + `status: needs_approval` / `needs_input`, or `blocked` with a request or after blocked tool calls. + No worker runs. `task_status` reports `terminal: true, parked: true`. The worktree is kept by retention + unless `retention.parked_days` is set, and even then the task record is never purged automatically. +- MCP tools `update_handoff` (owner, next_action, state, note; state `needs_approval` / `needs_input` + parks a task and any other state unparks it) and `approve_task` (`approve` resumes through the + continue path with the approval noted, and `reject` closes the task or redirects it with + instructions). +- `list_tasks` filters `status: "needs_attention"` (parked tasks plus finished tasks whose handoff is + not done or closed) and `status: "parked"`, and a new `owner` filter. +- CLI: `workhorse handoff ` (show or update), `workhorse approve `, `workhorse reject `, + `workhorse attention`, `workhorse tasks --status/--owner`, `workhorse cleanup-old --parked-days N`. +- Worker contract: RESULT statuses `needs_approval` and `needs_input`, plus an optional `needs:` line + for the one exact request. +- `test_results.failing_tests` (names parsed from the full test log) when tests fail. +- `health_report`: `parked_tasks`, `oldest_parked_days`, `needs_attention`. +- Audit events `parked`, `handoff_updated`, `approval`, `closed`. `finished` now carries + `handoff_state` and `owner`. +- Docs: [docs/handoff.md](docs/handoff.md). The delegation skill draft now tells the supervisor to follow + `handoff.next_action`, relay approvals and hand off with `update_handoff`. + +### Changed +- `continue_task` also accepts parked (`needs_approval`) tasks. Each `previous_results` entry records + the handoff it replaced, and the approval that answered it. +- `cancel_task` on a parked task closes it (status `cancelled`, handoff `closed`). +- `list_tasks status: "terminal"` also includes parked tasks, since no worker is running. + +### Compatibility +- Existing statuses, verdicts and tool arguments are unchanged. A parked task's verdict is the existing + `blocked`. Tasks that used to end as `completed` + `blocked` are now `needs_approval` + `blocked` when + the worker made a concrete request or hit blocked tool calls. Without either, they stay `completed`. + ## [0.1.0] - 2026-09-26 First public release. @@ -34,4 +81,5 @@ First public release. - Draft Grok Bot skills in `grok-template/` (getting started, delegation). [Unreleased]: https://github.com/mrchatam/Grok-workhorse/compare/v0.1.0...HEAD +[0.2.0]: https://github.com/mrchatam/Grok-workhorse/compare/v0.1.0...HEAD [0.1.0]: https://github.com/mrchatam/Grok-workhorse/releases/tag/v0.1.0 diff --git a/README.md b/README.md index e956e7f..f41e6e2 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ you (or your supervisor) review the branch and decide. - Repo: https://github.com/mrchatam/Grok-workhorse - License: MIT (vendored skills: MIT, see [NOTICE](NOTICE)) -- Status: v0.1.0, Linux only +- Status: v0.2.0 in development (latest release v0.1.0), Linux only ## Features @@ -23,6 +23,7 @@ you (or your supervisor) review the branch and decide. - **Defense in depth**: a repo allowlist, a test-command allowlist (anchored regexes), an allowlist of clone hosts, a guard plugin/hook for git mutation, network clients and secret access, a read-only git object store, and root-owned config. - **Structured results**: verdict (`success`, `tests_failed`, `no_changes`, `blocked`, `integrity_violation`, ...), diffstat, daemon-run test results, the worker's self-report, concerns, token usage and timings. Raw logs are paged on demand. - **Follow-ups and reviews**: `continue_task` resumes the same session and worktree. `mode: "review"` runs a read-only reviewer on another task's diff. +- **Handoff records and human approval**: every finished task says who acts next (`owner`), the one exact `next_action`, which checks failed, and how to resume. A worker that needs a human decision parks the task as `needs_approval` (worktree kept) until someone answers with `approve_task`. Supervisors record their own handoffs with `update_handoff`. See [docs/handoff.md](docs/handoff.md). - **Operations**: stall detection (15 min by default), wall-clock timeouts, cancel, recovery after a daemon restart, retention sweeps, an append-only JSONL audit log, and `workhorse health` for daily checks. - **Credentials from the environment first** (daemon env, or the MCP connector env passed through the shim), with an optional secret-store fallback. Keys never appear in argv, logs or results. @@ -44,7 +45,7 @@ flowchart LR R --> M ``` -More detail: [docs/architecture.md](docs/architecture.md). +More detail: [docs/architecture.md](docs/architecture.md). Handoff records and the approval flow: [docs/handoff.md](docs/handoff.md). ## Security model (short version) @@ -166,8 +167,8 @@ tests only. See [docs/adapters.md](docs/adapters.md) for the adapter interface a - **getting-started**: walks a new user through choosing a provider and model, adding repos, storing the key securely, running the installer, registering the stdio connector and running the first task. - **delegation**: how a supervisor should delegate. It reads `list_models` and `list_repos`, writes - self-contained task descriptions, polls, reviews the result and diff, uses `continue_task` for fixes, - and merges or cleans up. + self-contained task descriptions, polls, reviews the result and diff, follows `handoff.next_action`, + uses `continue_task` for fixes, relays approvals, and merges or cleans up. Copy them into your Grok Bot skills if you want them. Nothing in this repo installs them automatically. @@ -178,6 +179,9 @@ workhorse start|stop|restart|status supervisor + daemon (runs as the service workhorse health [--json] full check (exit 1 on FAIL), good for a daily cron workhorse backends adapters: status, installed, capabilities workhorse tasks | logs | audit recent tasks, daemon log, audit log +workhorse attention tasks that need someone (parked or handoff not done) +workhorse handoff [--owner O --next "..." --note "..." --state S] show or update a handoff +workhorse approve | reject answer a parked (needs_approval) task workhorse cleanup-old [--days N] retention sweep now workhorse repos | add-repo | remove-repo | validate | check-provider | hello ``` diff --git a/docs/architecture.md b/docs/architecture.md index b5af289..d3ed19c 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -28,7 +28,8 @@ sequenceDiagram | `bin/workhorse-mcp` | Stateless stdio MCP server. It starts the supervisor/daemon on demand, forwards each tool call over the daemon's unix socket (mode 0600 inside a 0700 dir, plus a bearer token) and offers credentials from its own environment to the daemon (held in memory only). | | `bin/workhorse-supervisor`, `bin/workhorsed` | Supervisor loop and daemon (JSON-RPC over HTTP on the unix socket). The daemon refuses to run as root. | | `bin/workhorse` | Operator CLI: start/stop, health, repos, validate, check-provider, hello, backends, retention. | -| `lib/tasks.mjs` | Task manager: validation, queue and admission (per-provider concurrency, memory), run lifecycle, stall/timeout, retries and fallback chain, finalization (tests, diff, verdict), recovery, retention. | +| `lib/tasks.mjs` | Task manager: validation, queue and admission (per-provider concurrency, memory), run lifecycle, stall/timeout, retries and fallback chain, finalization (tests, diff, verdict, handoff), approval/handoff updates, recovery, retention. | +| `lib/handoff.mjs` | Deterministic handoff derivation (state, failed checks, owner, next action, resume hints) from a finished task. | | `lib/sandbox.mjs` | Builds the outer bwrap argument list: hide paths, read-only and read-write binds, per-task mounts. | | `lib/git.mjs` | Clones (allowlisted hosts only), worktrees, diffs, main-clone fingerprint. | | `lib/config.mjs` | Config loading, defaults, auto-detection of backend binaries, bwrap and toolchain dirs. | @@ -40,7 +41,8 @@ sequenceDiagram ## Task lifecycle `queued` → `running` → (`retry_wait` → `queued` …) → `testing` → `finalizing` → one terminal state: -`completed`, `failed`, `timeout`, `stalled`, `cancelled` or `interrupted`. +`completed`, `failed`, `timeout`, `stalled`, `cancelled` or `interrupted`, or the parked state +`needs_approval` when the worker asked for a human decision (see below). The verdict is computed by the daemon in this order of precedence: @@ -51,12 +53,34 @@ The verdict is computed by the daemon in this order of precedence: 5. `success` / `tests_failed`, based on the daemon-run tests 6. `success_untested`: no test command was configured or detected +## Handoff and approval + +Finalization also writes a **handoff record** (`task.handoff`, returned by `task_result`, summarized +in `task_status` and `list_tasks`): `state`, `failed_checks`, `owner`, `next_action`, `resume`, +`context`, `updated_at`/`updated_by`. It is derived from the verdict and the data above, never written +by the model, and it persists in `task.json`. + +When the worker reports `needs_approval` / `needs_input` (or `blocked` with a request or after blocked +tool calls), the task is **parked** in status `needs_approval` with verdict `blocked`. No process +runs. `task_status` reports `terminal: true, parked: true`. Retention keeps the worktree, and +`approve_task` resumes the task through the `continue_task` path (approve), or closes it (reject). +`update_handoff` lets a supervisor or human change owner, next action, state and notes. Setting state +`needs_approval` / `needs_input` parks a finished task, and any other state unparks it. + +``` +completed/failed/... ──update_handoff state=needs_approval──▶ needs_approval ──approve_task approve──▶ queued (continue path) + ▲ │ └──approve_task reject──▶ cancelled (handoff closed) + └────────────── update_handoff other state ──────────────┘ +``` + +Details, field reference and the verdict-to-handoff table: [handoff.md](handoff.md). + ## Data layout (`data_dir`) ``` repos// main clones (workers see only their .git, read-only) worktrees/// one worktree per task (branch workhorse/) -tasks// task.json, activity.log, run-N.events.jsonl, run-N.stderr.log, diff.patch, test.log +tasks// task.json (incl. result + handoff), activity.log, run-N.events.jsonl, run-N.stderr.log, diff.patch, test.log backend-data// per-task agent data (session DB, snapshots, per-task settings) kilo-home/, opencode-home/ backend HOMEs (config/code dirs root-owned after lock-config) logs/ daemon.log, audit.jsonl diff --git a/docs/configuration.md b/docs/configuration.md index 5aa48a4..43eea67 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -76,7 +76,7 @@ Every key is optional. Defaults are in `lib/config.mjs`. | `max_concurrent` | 2 | Tasks running at once | | `timeouts` | default 30, min 1, max 120, **stall 15**, test 10 (minutes), kill_grace_sec 10 | | | `retry` | 2 retries, back-off [30, 120] s, provider cooldown 60 s | For retryable model API errors | -| `retention` | enabled, worktree_days 7, task_days 30, sweep every 60 min | Automatic cleanup | +| `retention` | enabled, worktree_days 7, task_days 30, parked_days null, sweep every 60 min | Automatic cleanup. Parked (`needs_approval`) tasks are skipped unless `parked_days` is set; then only their worktree is removed that many days after the last handoff update ([handoff.md](handoff.md)) | | `default_backend` | `kilo` | Backend for profiles without `backend` | | `bwrap` | auto: bundled with Kilo, else `bwrap` on PATH | | | `backends.` | `bin` (auto: `$WH__BIN`, `/-cli/bin/`, PATH), `expected_version`, `config_dir`, `home` | Per-backend settings; `kilo` also has `session_retry_limit` | diff --git a/docs/handoff.md b/docs/handoff.md new file mode 100644 index 0000000..1beb4fc --- /dev/null +++ b/docs/handoff.md @@ -0,0 +1,142 @@ +# Handoff records and the approval flow + +Every finished task carries a **handoff record**: who must act next, the one exact thing they must do, +which checks failed, and how to resume. A later agent (or a human) can pick the task up from the +record alone, without reading the whole conversation or guessing. The idea came from community +feedback: a supervisor is more useful when it keeps a blocked task as blocked, with the failed check, +the owner and the exact next action. + +The daemon derives the record deterministically from the verdict and data it already has (daemon-run +test results, integrity checks, blocked tool calls, the worker's `## RESULT` block). The model is never +asked to write it. When the worker reports `status: blocked`, `needs_approval` or `needs_input` with a +`needs:` line, that request is quoted in the record. + +## Where it appears + +| Where | What | +|---|---| +| `task_result` | the full record under `handoff` | +| `task_status` | `handoff`: state, owner, next_action (first 300 chars), names of failed checks, updated_at/by | +| `list_tasks` | `handoff`: state, owner, next_action per task; filters `status: "needs_attention"`, `status: "parked"`, `owner` | +| `task.json` | persisted with the task, so it survives daemon restarts | +| audit log | `finished` (with `handoff_state`, `owner`), `parked`, `handoff_updated`, `approval`, `closed` events | + +Tasks finished before v0.2.0 have no stored record. For those the daemon derives one when asked. + +## Fields + +```jsonc +{ + "state": "needs_fix", // done | needs_fix | needs_review | needs_approval | needs_input | blocked | retryable | closed + "owner": "supervisor", // who acts next: "supervisor", "human" or a named owner + "next_action": "Run continue_task with instructions to fix the failing test(s) adds numbers; see task_details kind=test_log.", + "failed_checks": [ // facts, most important first + { "check": "tests", "command": "npm test", "exit_code": 1, "timed_out": false, "counts": { "failed": 1 }, + "failing_tests": ["adds numbers"], "excerpt": "…last lines of the test output…", "log": "task_details kind=test_log" } + ], + "resume": { + "task_id": "wh-20260926-171500-ab12", + "tool": "continue_task", // continue_task | approve_task | null (nothing to resume) + "args": { "task_id": "…", "instructions": "The daemon-run tests failed: …" }, // suggested call + "session_reusable": true, // the worktree exists and the agent session can be resumed + "session_id": "ses_…", "worktree_path": "…/worktrees/demo/wh-…", "worktree_exists": true, + "branch": "workhorse/wh-…", "base_commit": "…" + }, + "context": { + "verdict": "tests_failed", "status": "completed", "summary": "worker summary", + "worker_status": "done", "worker_request": null, "remaining_concerns": ["…"], + "diffstat": "2 files changed, 10 insertions(+)", "files_changed": 2, "diff_path": "…/diff.patch", + "diff": "task_details kind=diff", "previous_attempts": 0 + }, + "notes": [], // appended by update_handoff + "history": [], // who changed what (last 20) + "derived": true, // false once a supervisor or human has edited it + "updated_at": "2026-09-26T17:20:00.000Z", + "updated_by": "daemon" // daemon | supervisor | human (cli) | the `by` value given +} +``` + +`failed_checks[].check` is one of `integrity`, `tests`, `timeout`, `stall`, `interrupted`, +`worker_error`, `blocked_calls` (count plus up to five examples), `worker_status` (the worker reported +something other than `done`), `result_block` (no `## RESULT` block) or `no_changes`. + +## Verdict to handoff + +| Verdict / situation | state | owner | next_action (shape) | +|---|---|---|---| +| `success` | `done` | supervisor | review the diff, merge the branch, cleanup_task | +| `success`, worker reported `partial` or no `## RESULT` block | `needs_fix` | supervisor | continue_task to finish | +| `success_untested` | `needs_review` | supervisor | review the diff by hand (no tests ran: reason) | +| `tests_failed` | `needs_fix` | supervisor | continue_task to fix the named failing tests | +| `no_changes` | `needs_fix` | supervisor | continue_task with more specific instructions, or close | +| `worker_error` | `retryable` | supervisor | continue_task to retry | +| `worker_error` from a setup problem (missing credentials, backend not installed, …) | `blocked` | human | fix the setup, then continue_task | +| `timeout`, `stalled`, `interrupted` | `retryable` | supervisor | continue_task to resume (timeouts suggest a 1.5x `timeout_minutes`) | +| `cancelled` | `closed` | supervisor | nothing pending | +| `blocked` (worker said blocked, no request, no blocked calls) | `blocked` | human | resolve the blocker, then approve_task or continue_task | +| worker asked for approval/input, or blocked with a request or after blocked tool calls | `needs_approval` / `needs_input` | human | approve or deny (or answer) with approve_task | +| `integrity_violation` | `blocked` | human | inspect the branch, do not merge | + +## The `needs_approval` status (parked tasks) + +A task is **parked** (status `needs_approval`) when the worker finished with + +- `status: needs_approval` or `status: needs_input` in its `## RESULT` block, or +- `status: blocked` plus a `needs:` request, or after tool calls the policy or sandbox blocked. + +The verdict stays `blocked`, so the existing verdict set is unchanged. A parked task: + +- has no worker process. `task_status` reports `terminal: true` (so existing pollers stop) and + `parked: true`. Its hint points to `approve_task`. +- is not closed. It appears in `list_tasks` with `status: "needs_approval"`, `"parked"`, + `"needs_attention"` and `"terminal"`. +- keeps its worktree. Retention sweeps skip parked tasks unless `retention.parked_days` (or + `cleanup_old parked_days` / `workhorse cleanup-old --parked-days N`) is set. Even then, only the + worktree is removed, counted from the last handoff update, and the task record is never purged + automatically. `workhorse health` reports `parked_tasks`, `oldest_parked_days` and `needs_attention`. +- can be resumed with `continue_task`, answered with `approve_task`, closed with `approve_task + decision=reject` or `cancel_task`, or cleaned up explicitly with `cleanup_task`. + +`list_tasks` with `status: "needs_attention"` returns parked tasks plus every finished task whose +handoff state is not `done` or `closed` (failed tests, retryable timeouts, blockers, …). + +## Updating the handoff + +`update_handoff` (MCP) / `workhorse handoff --owner … --next … --note … --state …` (CLI) edits a +finished task's record: `owner`, `next_action`, `state`, and a `note` that is appended with a timestamp and +author. `by` names who is acting (default `supervisor` over MCP, `human (cli)` from the CLI). Each +change goes into `history` and the audit log. + +Setting `state` to `needs_approval` or `needs_input` parks a finished task for a human (the owner +becomes `human` unless you pass one), which also protects its worktree from retention. Setting any +other state on a parked task unparks it. It goes back to its previous status, or `completed`. +`workhorse handoff ` without options prints the record and past approvals. + +## Approving or rejecting + +`approve_task` (MCP) / `workhorse approve ` and `workhorse reject ` (CLI) answer a task that is +parked, or whose handoff state is `needs_approval`, `needs_input` or `blocked`: + +- `decision: "approve"` resumes the task through the `continue_task` path: same worktree, and the same + agent session when the backend matches. The follow-up message starts with `APPROVED by : `, then your `instructions` (for `needs_input`, the answer). +- `decision: "reject"` with `instructions` resumes the task with `DENIED by : … Do not do that.` + followed by your instructions. +- `decision: "reject"` without instructions closes the task: status `cancelled`, verdict unchanged, + handoff state `closed`. The worktree is kept (`continue_task` still works, and `cleanup_task` removes it). + +Every decision is stored in `task.approvals` and in the `previous_results` entry of the run it +answered, and is logged as an `approval` event. + +**Approval does not change the sandbox.** It is a decision passed to the worker, not a permission +grant. The guard and bwrap rules still apply, so a worker still cannot reach the network or install +packages. If the approved action needs that (for example adding a dependency to an offline cache), the +human does it outside the sandbox first and then approves. The `next_action` text says so. + +## Supervisor loop (short) + +1. After `task_result`, read `handoff.state`, `owner` and `next_action`. +2. If the owner is `supervisor`, do the next action (often the `resume` suggestion). If it is `human`, + relay `next_action` to the user verbatim and wait for their decision, then call `approve_task`. +3. When you stop working on a task that is not done, record where it stands with `update_handoff` + (owner, one exact next action, a note) so the next agent can resume it without guessing. diff --git a/docs/security.md b/docs/security.md index 3ea6543..4a2cfc9 100644 --- a/docs/security.md +++ b/docs/security.md @@ -70,6 +70,10 @@ - Workers can read the repo's full git history (the object store is bound read-only). - The pattern guards are a second layer. If you find a bypass of the *sandbox*, please report it (see [SECURITY.md](../SECURITY.md)). +- `approve_task` is a coordination signal, not a permission grant. It never widens the sandbox or + the guard. The approver name (`by`) is recorded as given, and any client holding the daemon socket + and token (the supervisor included) can approve. Keep a human in the loop at the supervisor level when + approvals matter. ## Recommendations diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index fea6589..9de0ada 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -47,6 +47,17 @@ The worker tried something the policy denies (a git commit, network, paths outsi `task_result.blocked_examples` shows the calls. That is usually fine. If a legitimate build step needs something, provide it through `ro_binds` and `sandbox_env` rather than loosening the guard. +If the worker considers the blocked action necessary (or asks for approval/input), the task is parked +in status `needs_approval`. `task_result.handoff.next_action` names the exact request. Do the action +yourself if it needs network or an install, then `workhorse approve ` (or `approve_task`), or +`workhorse reject `. See [handoff.md](handoff.md). + +## Parked tasks pile up + +`workhorse attention` lists tasks that need someone. Parked tasks keep their worktree until they are +answered, closed (`workhorse reject `) or cleaned up (`cleanup_task`). To expire them +automatically, set `retention.parked_days`. + ## Builds fail inside the sandbox (Go, npm, pip) There is no network (Kilo) and no writable HOME cache, so dependencies must be available offline. diff --git a/grok-template/workhorse-delegation/SKILL.md b/grok-template/workhorse-delegation/SKILL.md index 69cda34..cacc354 100644 --- a/grok-template/workhorse-delegation/SKILL.md +++ b/grok-template/workhorse-delegation/SKILL.md @@ -35,7 +35,8 @@ Split large work into independent tasks, and never put secrets in the task text. ## 4. Wait `delegate_task` returns a `task_id` immediately. Check `task_status` every 1-2 minutes, not in a tight loop. A few minutes with no turns usually means the provider is queueing. The daemon detects stalls -on its own. +on its own. Stop polling when `terminal` is true. That includes `needs_approval`, which means the task +is parked and waiting for a human decision. ## 5. Review (`task_result`, then `task_details` as needed) - `integrity.commits_made` must be 0 and `main_clone_unchanged` must be true. If not, reject and tell the user. @@ -44,12 +45,30 @@ on its own. weakened tests, hard-coded outputs, new dependencies and stray tool files. - Verdicts: `success`, `tests_failed`, `no_changes`, `success_untested`, `blocked`, `worker_error`, `timeout`, `stalled`, `cancelled`, `interrupted`, `integrity_violation`. +- **Read `handoff` first.** `handoff.state` (done, needs_fix, needs_review, needs_approval, needs_input, + blocked, retryable, closed), `handoff.owner` (who acts next) and `handoff.next_action` (one exact + instruction) tell you what to do. `handoff.failed_checks` lists the facts (failing tests with names, + blocked calls, integrity, timeout or stall). `handoff.resume` has a suggested tool call and says whether + the session and worktree can be reused. Don't guess past it. ## 6. Next step - **Accept**: tell the user what changed, where it is (`branch`, `worktree_path`, `diff_path`) and the test evidence. Commit, push or open a PR only if the user asked. - **Revise**: `continue_task` with specific feedback (same session and worktree). This also works for - `interrupted`, `timeout` and `stalled` tasks. + `interrupted`, `timeout`, `stalled` and `needs_approval` tasks. `handoff.resume.args` is a good + starting point. +- **Human approval** (`status: needs_approval`, `handoff.owner: "human"`): relay `handoff.next_action` + to the user word for word and wait for their answer. Don't approve on their behalf. Then call + `approve_task` with `decision: "approve"` (put their answer or conditions in `instructions`) or + `decision: "reject"` (with `instructions` to redirect the worker, or without them to close the task). + Approval does not lift sandbox rules. If the request needs network or an install, the user must do that + outside first. +- **Hand off**: when you stop before the task is done (the user must decide, you ran out of time, you + are waiting on something), call `update_handoff` with the `owner`, one exact `next_action` and a + `note`, so a later agent or the user can resume without guessing. Use `state: "needs_approval"` or + `"needs_input"` to park it for a human (this also keeps its worktree from retention cleanup), and + `state: "closed"` when it needs nobody. +- **Find open work**: `list_tasks` with `status: "needs_attention"` (optionally `owner: "human"`). - **Second opinion**: `delegate_task` with `mode: "review"` and `review_task_id`. - **Stop**: `cancel_task`. Use `cleanup_task` only once the user no longer needs the work (it archives the patch and refuses unmerged changes unless told to discard them). diff --git a/package-lock.json b/package-lock.json index 591fdf3..7e6bb92 100644 --- a/package-lock.json +++ b/package-lock.json @@ -1,12 +1,12 @@ { "name": "grok-workhorse", - "version": "0.1.0", + "version": "0.2.0", "lockfileVersion": 3, "requires": true, "packages": { "": { "name": "grok-workhorse", - "version": "0.1.0", + "version": "0.2.0", "dependencies": { "@modelcontextprotocol/sdk": "^1.30.1", "zod": "^3.25.76" diff --git a/package.json b/package.json index 78cf3a3..d6dec96 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "grok-workhorse", - "version": "0.1.0", + "version": "0.2.0", "private": true, "type": "module", "description": "Sandboxed coding-agent workers (Kilo CLI, OpenCode, ...) that a supervising AI agent drives over MCP, with any OpenAI-compatible model provider", From 2d4bfb455fea0bd9c5acf800648bb172e866fcd5 Mon Sep 17 00:00:00 2001 From: mrchatam <287639636+mrchatam@users.noreply.github.com> Date: Sat, 26 Sep 2026 23:41:42 +0330 Subject: [PATCH 5/9] Close parked tasks when their worktree is removed cleanup_task on a parked task, or retention with parked_days, now closes it (status cancelled, handoff closed, archived diff named) so no parked task is left waiting on a worktree that no longer exists. Parking a task whose worktree is gone is refused. Handoff history now records the owner/status change caused by parking. --- CHANGELOG.md | 5 +++-- docs/configuration.md | 2 +- docs/handoff.md | 11 +++++++---- lib/tasks.mjs | 25 ++++++++++++++++++------- test/handoff-flow.test.mjs | 16 ++++++++++++++++ 5 files changed, 45 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6d2b85c..af7e083 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,8 +23,9 @@ can resume it without guessing. `list_tasks` return a summary. Tasks finished before 0.2.0 get a derived record on the fly. - **Parked status `needs_approval`**: the task waits for a human decision when the worker reports `status: needs_approval` / `needs_input`, or `blocked` with a request or after blocked tool calls. - No worker runs. `task_status` reports `terminal: true, parked: true`. The worktree is kept by retention - unless `retention.parked_days` is set, and even then the task record is never purged automatically. + No worker runs. `task_status` reports `terminal: true, parked: true`. Retention keeps the worktree + unless `retention.parked_days` is set. With it set, expired parked tasks lose their worktree (the + diff is archived) and are closed. `cleanup_task` on a parked task also closes it. - MCP tools `update_handoff` (owner, next_action, state, note; state `needs_approval` / `needs_input` parks a task and any other state unparks it) and `approve_task` (`approve` resumes through the continue path with the approval noted, and `reject` closes the task or redirects it with diff --git a/docs/configuration.md b/docs/configuration.md index 43eea67..91289fe 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -76,7 +76,7 @@ Every key is optional. Defaults are in `lib/config.mjs`. | `max_concurrent` | 2 | Tasks running at once | | `timeouts` | default 30, min 1, max 120, **stall 15**, test 10 (minutes), kill_grace_sec 10 | | | `retry` | 2 retries, back-off [30, 120] s, provider cooldown 60 s | For retryable model API errors | -| `retention` | enabled, worktree_days 7, task_days 30, parked_days null, sweep every 60 min | Automatic cleanup. Parked (`needs_approval`) tasks are skipped unless `parked_days` is set; then only their worktree is removed that many days after the last handoff update ([handoff.md](handoff.md)) | +| `retention` | enabled, worktree_days 7, task_days 30, parked_days null, sweep every 60 min | Automatic cleanup. Parked (`needs_approval`) tasks are skipped unless `parked_days` is set. When it is set, their worktree is removed that many days after the last handoff update and the task is closed ([handoff.md](handoff.md)) | | `default_backend` | `kilo` | Backend for profiles without `backend` | | `bwrap` | auto: bundled with Kilo, else `bwrap` on PATH | | | `backends.` | `bin` (auto: `$WH__BIN`, `/-cli/bin/`, PATH), `expected_version`, `config_dir`, `home` | Per-backend settings; `kilo` also has `session_retry_limit` | diff --git a/docs/handoff.md b/docs/handoff.md index 1beb4fc..916e8d7 100644 --- a/docs/handoff.md +++ b/docs/handoff.md @@ -91,11 +91,14 @@ The verdict stays `blocked`, so the existing verdict set is unchanged. A parked - is not closed. It appears in `list_tasks` with `status: "needs_approval"`, `"parked"`, `"needs_attention"` and `"terminal"`. - keeps its worktree. Retention sweeps skip parked tasks unless `retention.parked_days` (or - `cleanup_old parked_days` / `workhorse cleanup-old --parked-days N`) is set. Even then, only the - worktree is removed, counted from the last handoff update, and the task record is never purged - automatically. `workhorse health` reports `parked_tasks`, `oldest_parked_days` and `needs_attention`. + `cleanup_old parked_days` / `workhorse cleanup-old --parked-days N`) is set. When it is set, a task + parked longer than that (counted from the last handoff update) loses its worktree (the diff is + archived) and is closed (`updated_by: "retention"`). After that it follows normal retention. A parked + task record is never purged. `workhorse health` reports `parked_tasks`, `oldest_parked_days` and + `needs_attention`. - can be resumed with `continue_task`, answered with `approve_task`, closed with `approve_task - decision=reject` or `cancel_task`, or cleaned up explicitly with `cleanup_task`. + decision=reject` or `cancel_task`, or cleaned up explicitly with `cleanup_task`. Cleaning up also + closes it. `list_tasks` with `status: "needs_attention"` returns parked tasks plus every finished task whose handoff state is not `done` or `closed` (failed tests, retryable timeouts, blockers, …). diff --git a/lib/tasks.mjs b/lib/tasks.mjs index 4692b50..be71350 100644 --- a/lib/tasks.mjs +++ b/lib/tasks.mjs @@ -539,21 +539,22 @@ export class Manager { h.notes = [...(h.notes || []), { at, by: who, note }].slice(-20) changed.push("note") } - h.history = [...(h.history || []), { at, by: who, event: "update", changed, state: h.state, owner: h.owner }].slice(-20) - h.derived = false - h.updated_at = at - h.updated_by = who const from = t.status if (p.state !== undefined) { if (PARK_STATES.has(p.state) && TERMINAL.has(t.status)) { + if (t.worktree_removed) throw new UserError("task worktree was cleaned up; it cannot be parked for approval") t.parked_from = t.status t.status = "needs_approval" - if (p.owner === undefined) h.owner = "human" + if (p.owner === undefined && h.owner !== "human") { h.owner = "human"; changed.push("owner") } } else if (!PARK_STATES.has(p.state) && PARKED.has(t.status)) { t.status = t.parked_from || "completed" delete t.parked_from } } + h.history = [...(h.history || []), { at, by: who, event: "update", changed, state: h.state, owner: h.owner, ...(from !== t.status ? { from_status: from, to_status: t.status } : {}) }].slice(-20) + h.derived = false + h.updated_at = at + h.updated_by = who t.handoff = redact(h) this.save(t) this.event(t, "handoff_updated", { by: who, changed, state: h.state, owner: h.owner, ...(from !== t.status ? { from_status: from } : {}) }) @@ -599,7 +600,9 @@ export class Manager { const at = now() h.state = "closed" h.owner = "supervisor" - h.next_action = `Closed (${reason}). The worktree is kept: merge any useful partial diff, continue_task to resume, or cleanup_task (discard_unmerged_changes=true) to drop it.` + h.next_action = t.worktree_removed + ? `Closed (${reason}). The worktree is gone; the diff is archived at ${path.join(this.taskDir(t.id), "diff.patch")}. Delegate a new task if the work is still needed.` + : `Closed (${reason}). The worktree is kept: merge any useful partial diff, continue_task to resume, or cleanup_task (discard_unmerged_changes=true) to drop it.` h.resume = { ...h.resume, tool: null, args: null } h.history = [...(h.history || []), { at, by: who, event: "closed", reason, from_status: from }].slice(-20) h.derived = false @@ -642,6 +645,7 @@ export class Manager { t.cleanup = { at: now(), merged, discarded_unmerged: !merged, files: files.length } this.save(t) this.event(t, "cleanup", t.cleanup) + if (PARKED.has(t.status)) this.closeTask(t, "supervisor", "worktree removed with cleanup_task while waiting for approval") return { task_id: id, removed_worktree: wt, deleted_branch: t.branch, merged, archived_diff_path: path.join(this.taskDir(id), "diff.patch") } } @@ -667,7 +671,14 @@ export class Manager { const pa = (Date.now() - Date.parse(t.handoff?.updated_at || t.finished_at || t.updated_at || t.created_at)) / 86400000 if (pd != null && pa >= pd && !t.worktree_removed) { out.worktrees_removed.push(t.id) - if (!dry_run) await this.removeTaskWorktree(t, trigger).catch((e) => out.errors.push(`${t.id}: ${e.message}`)) + if (!dry_run) { + try { + await this.removeTaskWorktree(t, trigger) + this.closeTask(t, "retention", `no decision within ${pd} day(s); worktree removed by retention (diff archived)`) + } catch (e) { + out.errors.push(`${t.id}: ${e.message}`) + } + } } else if (!t.worktree_removed) out.parked_kept.push(t.id) continue } diff --git a/test/handoff-flow.test.mjs b/test/handoff-flow.test.mjs index 627425e..d229cbd 100644 --- a/test/handoff-flow.test.mjs +++ b/test/handoff-flow.test.mjs @@ -265,4 +265,20 @@ test("persistence across restart, and tasks from v0.1.0 without a handoff", asyn await mgr.cleanup(done.id, true) assert.equal(mgr.result(done.id).handoff.resume.worktree_exists, false) assert.equal(mgr.result(done.id).handoff.resume.session_reusable, false) + assert.throws(() => mgr.updateHandoff({ task_id: done.id, state: "needs_approval" }), /cannot be parked/) +}) + +test("removing a parked task's worktree closes it (cleanup_task, or retention with parked_days)", async () => { + const a = await finishedTask({ text: RESULT("needs_approval", "needs: approve A") }) + await mgr.cleanup(a.id, true) + assert.equal(a.status, "cancelled") + assert.equal(mgr.result(a.id).handoff.state, "closed") + assert.match(mgr.result(a.id).handoff.next_action, /worktree is gone; the diff is archived/) + const b = await finishedTask({ text: RESULT("needs_approval", "needs: approve B") }) + const sw = await mgr.sweep({ worktree_days: 1000, task_days: 1000, parked_days: 0, trigger: "test" }) + assert.ok(sw.worktrees_removed.includes(b.id)) + assert.equal(fs.existsSync(b.worktree_path), false) + assert.equal(b.status, "cancelled") + assert.equal(mgr.result(b.id).handoff.updated_by, "retention") + assert.ok(mgr.tasks.has(b.id), "the closed task record stays (it follows normal retention from now on)") }) From 4bef48608dd4296b172a0d1a1e2eb413e6d3a410 Mon Sep 17 00:00:00 2001 From: mrchatam <287639636+mrchatam@users.noreply.github.com> Date: Sat, 26 Sep 2026 23:50:54 +0330 Subject: [PATCH 6/9] Fix audit records clobbered by event data (run_started kind) The run_started task event passed the run kind as `kind`, which overwrote the audit record's kind ("task") with "initial"/"continue". It is now `run_kind`. audit() and Manager.event() now guard their reserved fields centrally (ts, kind; task_id, event, status for task events): the reserved value wins and a clashing data key is kept as data_. Regression tests in test/audit.test.mjs and the daemon audit test; audit record format documented in docs/architecture.md. --- CHANGELOG.md | 6 +++++ docs/architecture.md | 12 +++++++++- lib/audit.mjs | 18 +++++++++++++- lib/tasks.mjs | 7 +++--- package.json | 2 +- test/audit.test.mjs | 57 ++++++++++++++++++++++++++++++++++++++++++++ test/daemon.test.mjs | 4 ++++ 7 files changed, 100 insertions(+), 6 deletions(-) create mode 100644 test/audit.test.mjs diff --git a/CHANGELOG.md b/CHANGELOG.md index af7e083..9172989 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -49,6 +49,12 @@ can resume it without guessing. - `cancel_task` on a parked task closes it (status `cancelled`, handoff `closed`). - `list_tasks status: "terminal"` also includes parked tasks, since no worker is running. +### Fixed +- Audit log: the `run_started` event's run kind (`initial`, `continue`, `retry`, `fallback`) overwrote + the record's `kind` (`task`). It is now `run_kind`. Reserved audit fields (`ts`, `kind`, and + `task_id`, `event`, `status` on task events) can no longer be overwritten by event data. A clashing + key is kept as `data_`. + ### Compatibility - Existing statuses, verdicts and tool arguments are unchanged. A parked task's verdict is the existing `blocked`. Tasks that used to end as `completed` + `blocked` are now `needs_approval` + `blocked` when diff --git a/docs/architecture.md b/docs/architecture.md index d3ed19c..e3aa95b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -83,10 +83,20 @@ worktrees/// one worktree per task (branch workhorse/) tasks// task.json (incl. result + handoff), activity.log, run-N.events.jsonl, run-N.stderr.log, diff.patch, test.log backend-data// per-task agent data (session DB, snapshots, per-task settings) kilo-home/, opencode-home/ backend HOMEs (config/code dirs root-owned after lock-config) -logs/ daemon.log, audit.jsonl +logs/ daemon.log, audit.jsonl (one JSON object per line: ts, kind, then event fields) run/ socket, token, pid files (0700) ``` +## Audit log + +`logs/audit.jsonl` is append-only JSONL, redacted like everything else. Every record starts with `ts` +and `kind` (`rpc`, `rpc_denied`, `task`, `credential`, `retention`, `daemon`, `error`). Task records also +carry `task_id`, `event` (`created`, `run_started`, `run_exited`, `finished`, `parked`, +`handoff_updated`, `approval`, `closed`, `cleanup`, …) and the task's `status`. Those fields are +reserved: if event data uses one of them, the reserved value wins and the data value is kept as +`data_`. `run_started` records the run kind (`initial`, `continue`, `retry`, `fallback`) as +`run_kind`. + ## Retries and fallback When a run fails with retryable model API errors (HTTP 429/5xx, or `isRetryable`), the daemon puts diff --git a/lib/audit.mjs b/lib/audit.mjs index 279b343..c175174 100644 --- a/lib/audit.mjs +++ b/lib/audit.mjs @@ -7,9 +7,25 @@ import { now, redact, sha256, head } from "./util.mjs" const AUDIT = () => path.join(dirs.logs, "audit.jsonl") +// Merge event data into a record whose reserved fields must not be overwritten: the reserved values win +// and a conflicting data key is kept as `data_` (so nothing is lost and nothing is clobbered). +export function withReserved(reserved, data) { + const out = { ...reserved } + for (const [k, v] of Object.entries(data || {})) { + if (!Object.hasOwn(reserved, k)) out[k] = v + else if (v !== reserved[k]) out[`data_${k}`] = v + } + return out +} + +// Build one audit record. `ts` and `kind` are reserved. +export function auditRecord(kind, data) { + return redact(withReserved({ ts: now(), kind }, data)) +} + export function audit(kind, data) { try { - fs.appendFileSync(AUDIT(), JSON.stringify(redact({ ts: now(), kind, ...data })) + "\n", { mode: 0o600 }) + fs.appendFileSync(AUDIT(), JSON.stringify(auditRecord(kind, data)) + "\n", { mode: 0o600 }) } catch { /* audit must never crash the daemon */ } diff --git a/lib/tasks.mjs b/lib/tasks.mjs index be71350..b5e5a15 100644 --- a/lib/tasks.mjs +++ b/lib/tasks.mjs @@ -8,7 +8,7 @@ import { dirs, daemonConfig, reposConfig, profilesConfig, secretEnvNames, REPO_N import { now, writeJsonAtomic, readJsonSafe, redact, registerSecret, tail, head, readRange, ensureDir } from "./util.mjs" import * as G from "./git.mjs" import { alive, procStart, killTree } from "./procs.mjs" -import { audit } from "./audit.mjs" +import { audit, withReserved } from "./audit.mjs" import { loadFromStore } from "./credentials.mjs" import { outerSandboxArgs } from "./sandbox.mjs" import { getBackend, profileBackend, backendProblems, backendSummary } from "../adapters/index.mjs" @@ -138,7 +138,8 @@ export class Manager { return t } event(t, kind, data = {}) { - audit("task", { task_id: t.id, event: kind, status: t.status, ...data }) + // task_id, event and status are reserved (a clashing data key is kept as data_); ts/kind are reserved by audit(). + audit("task", withReserved({ task_id: t.id, event: kind, status: t.status }, data)) } activity(t, line) { try { @@ -1019,7 +1020,7 @@ export class Manager { t.retry_at = null t.last_text = t.last_text || null this.save(t) - this.event(t, "run_started", { run: n, kind: spec.kind, profile: profile.name, backend: profile.backend, model: profile.model, pid: child.pid }) + this.event(t, "run_started", { run: n, run_kind: spec.kind, profile: profile.name, backend: profile.backend, model: profile.model, pid: child.pid }) this.activity(t, `run ${n} started (${spec.kind}, profile ${profile.name}, backend ${profile.backend}, model ${profile.model})`) const live = { child, backend, parseState: {}, provider: profile.provider, outFile, errFile, offset: 0, buf: "", errSize: 0, lastActivity: Date.now(), lastSave: 0, killedFor: null, run } this.live.set(t.id, live) diff --git a/package.json b/package.json index d6dec96..f427a1e 100644 --- a/package.json +++ b/package.json @@ -26,7 +26,7 @@ "scripts": { "setup": "npm ci && npm ci --prefix adapters/kilo/config && npm ci --prefix adapters/opencode/config", "test": "node --test --test-timeout=600000 test/*.test.mjs", - "test:unit": "node --test test/unit.test.mjs test/config.test.mjs test/adapters.test.mjs test/handoff.test.mjs test/handoff-flow.test.mjs", + "test:unit": "node --test test/unit.test.mjs test/config.test.mjs test/adapters.test.mjs test/handoff.test.mjs test/handoff-flow.test.mjs test/audit.test.mjs", "test:live": "WH_LIVE_TEST=1 node --test --test-timeout=1800000 test/live.test.mjs" }, "engines": { diff --git a/test/audit.test.mjs b/test/audit.test.mjs new file mode 100644 index 0000000..a5fcf67 --- /dev/null +++ b/test/audit.test.mjs @@ -0,0 +1,57 @@ +// Audit records: reserved fields (ts, kind; task_id, event, status for task events) cannot be clobbered +// by event data. Regression test for run_started, whose run kind used to overwrite the audit `kind`. +import { test } from "node:test" +import assert from "node:assert/strict" +import fs from "node:fs" +import os from "node:os" +import path from "node:path" + +const root = fs.mkdtempSync(path.join(os.tmpdir(), "kwt-audit-")) +process.env.WH_CONFIG_DIR = path.join(root, "config") +process.env.WH_DATA_DIR = path.join(root, "data") +fs.mkdirSync(process.env.WH_CONFIG_DIR, { recursive: true }) +const { withReserved, auditRecord, audit } = await import("../lib/audit.mjs") +const { Manager } = await import("../lib/tasks.mjs") +const { dirs } = await import("../lib/config.mjs") +fs.mkdirSync(dirs.logs, { recursive: true }) +const lines = () => fs.readFileSync(path.join(dirs.logs, "audit.jsonl"), "utf8").trim().split("\n").map((l) => JSON.parse(l)) + +test("withReserved: reserved values win, clashing data keys are kept with a data_ prefix", () => { + assert.deepEqual(withReserved({ kind: "task", event: "x" }, { kind: "initial", run: 1 }), { kind: "task", event: "x", data_kind: "initial", run: 1 }) + assert.deepEqual(withReserved({ kind: "task" }, { kind: "task" }), { kind: "task" }, "same value: no duplicate") + assert.deepEqual(withReserved({ a: 1 }, undefined), { a: 1 }) + const r = auditRecord("rpc", { ts: "forged", kind: "forged", method: "m" }) + assert.equal(r.kind, "rpc") + assert.notEqual(r.ts, "forged") + assert.equal(r.data_ts, "forged") + assert.equal(r.data_kind, "forged") + assert.deepEqual(Object.keys(r).slice(0, 2), ["ts", "kind"]) +}) + +test("task events keep kind/task_id/event/status; run_started records run_kind", () => { + const mgr = new Manager() + const t = { id: "wh-20260926-120000-ab12", status: "running" } + mgr.event(t, "run_started", { run: 1, run_kind: "continue" }) + mgr.event(t, "weird", { kind: "initial", task_id: "wh-other", event: "spoof", status: "completed", ts: "x" }) + audit("credential", { event: "loaded", name: "K" }) + const [a, b, c] = lines().slice(-3) + assert.equal(a.kind, "task") + assert.equal(a.event, "run_started") + assert.equal(a.run_kind, "continue") + assert.equal(b.kind, "task") + assert.equal(b.task_id, t.id) + assert.equal(b.event, "weird") + assert.equal(b.status, "running") + assert.deepEqual([b.data_kind, b.data_task_id, b.data_event, b.data_status, b.data_ts], ["initial", "wh-other", "spoof", "completed", "x"]) + assert.equal(c.kind, "credential") + assert.equal(c.event, "loaded", "event is an ordinary field outside task events") +}) + +test("startRun passes the run kind as run_kind, not kind", () => { + const src = fs.readFileSync(new URL("../lib/tasks.mjs", import.meta.url), "utf8") + const call = src.split("\n").find((l) => l.includes('this.event(t, "run_started"')) + assert.match(call, /run_kind: spec\.kind/) + assert.doesNotMatch(call, /[{,]\s*kind:/) +}) + +test.after(() => fs.rmSync(root, { recursive: true, force: true })) diff --git a/test/daemon.test.mjs b/test/daemon.test.mjs index 0df6b35..5ed109f 100644 --- a/test/daemon.test.mjs +++ b/test/daemon.test.mjs @@ -307,4 +307,8 @@ H.itest("audit log records tool calls and lifecycle events", async () => { assert.ok(lines.some((l) => l.kind === "rpc" && l.method === "delegate_task" && l.ok === false)) assert.ok(lines.some((l) => l.kind === "task" && l.event === "finished")) assert.ok(lines.some((l) => l.kind === "task" && l.event === "cleanup")) + // run kinds are recorded as run_kind and never replace the audit record kind + assert.ok(lines.some((l) => l.kind === "task" && l.event === "run_started" && l.run_kind === "initial")) + assert.ok(lines.some((l) => l.kind === "task" && l.event === "run_started" && l.run_kind === "continue")) + assert.ok(!lines.some((l) => ["initial", "continue", "retry", "fallback"].includes(l.kind))) }) From 66e53c6b849ed856831051e748ac6e261b61932e Mon Sep 17 00:00:00 2001 From: mrchatam <287639636+mrchatam@users.noreply.github.com> Date: Sat, 26 Sep 2026 23:59:42 +0330 Subject: [PATCH 7/9] Fix unfinalized interrupted tasks and the cleanup vs continue race 1. shutdown()/onExit() mark a running task interrupted without finalizing it, and recover() skipped interrupted tasks, so a task stopped by `workhorse stop` / SIGTERM had no result and no handoff (task_result said it was still active). recover() now finalizes interrupted tasks that have no result (idempotent). 2. cleanup_task and retention removed the worktree across several async steps before marking it removed, so a continue_task/approve_task in that window could queue a run into a worktree being deleted. The task is now marked (in memory) before the first await; continue_task, approve_task, update_handoff, a second cleanup and retention refuse or skip it until the removal is done. Regression tests: Manager-level tests for both, and the graceful-SIGTERM recovery integration test now checks the result and handoff. --- CHANGELOG.md | 5 ++++ lib/tasks.mjs | 38 +++++++++++++++++++++++++++++- test/handoff-flow.test.mjs | 48 ++++++++++++++++++++++++++++++++++++++ test/recovery.test.mjs | 4 ++++ 4 files changed, 94 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9172989..4013099 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -50,6 +50,11 @@ can resume it without guessing. - `list_tasks status: "terminal"` also includes parked tasks, since no worker is running. ### Fixed +- A task stopped by a graceful daemon shutdown (`workhorse stop`, SIGTERM) was marked `interrupted` + but never finalized, so it had no result and no handoff. It is now finalized on the next start. +- `cleanup_task` and retention mark a task before their first async step. While its worktree is being + removed, `continue_task`, `approve_task`, `update_handoff` and a second cleanup are refused instead of + queueing a run into a worktree that is being deleted. - Audit log: the `run_started` event's run kind (`initial`, `continue`, `retry`, `fallback`) overwrote the record's `kind` (`task`). It is now `run_kind`. Reserved audit fields (`ts`, `kind`, and `task_id`, `event`, `status` on task events) can no longer be overwritten by event data. A clashing diff --git a/lib/tasks.mjs b/lib/tasks.mjs index b5e5a15..06fc4cf 100644 --- a/lib/tasks.mjs +++ b/lib/tasks.mjs @@ -112,6 +112,7 @@ export class Manager { this.offered = {} this.fromStore = {} // name -> value loaded from daemon.json secret_store_path (memory only) this.startedAt = Date.now() + this.cleaning = new Set() // task ids whose worktree is being removed (cleanup_task / retention) this.storeMissing = {} // name -> reason the store could not provide it (no values) this.cooldown = {} this.shuttingDown = false @@ -189,6 +190,11 @@ export class Manager { } else if (t.status === "testing" || t.status === "finalizing") { this.event(t, "refinalize_on_restart") await this.finalize(t, { tests: t.status === "testing", status: t.pending_final_status || "completed" }) + } else if (t.status === "interrupted" && !t.result) { + // Stopped by a graceful daemon shutdown (SIGTERM / workhorse stop): the worker was killed but the + // task was never finalized, so it has no result or handoff yet. + this.event(t, "finalize_interrupted_on_restart") + await this.finalize(t, { tests: false, status: "interrupted" }) } } } @@ -446,6 +452,7 @@ export class Manager { async continueTask(id, instructions, timeoutMinutes, profileName, { approval = null } = {}) { const t = this.get(id) if (!settled(t.status)) throw new UserError(`task is ${t.status}; continue_task works only on finished or parked tasks (cancel it first if needed)`) + this.assertNotCleaning(t) if (t.worktree_removed) throw new UserError("task worktree was cleaned up; delegate a new task instead") if (typeof instructions !== "string" || !instructions.trim()) throw new UserError("instructions are required") if (instructions.length > daemonConfig().limits.task_max_chars) throw new UserError("instructions too long") @@ -521,6 +528,7 @@ export class Manager { for (const k of Object.keys(p)) if (!allowed.has(k)) throw new UserError(`unknown parameter '${k}'`) const t = this.get(p.task_id) if (!settled(t.status)) throw new UserError(`task is ${t.status}; the handoff exists once the task has finished`) + this.assertNotCleaning(t) if (p.state !== undefined && !HANDOFF_STATES.includes(p.state)) throw new UserError(`state must be one of ${HANDOFF_STATES.join(", ")}`) if (p.owner !== undefined && (typeof p.owner !== "string" || !OWNER_RE.test(p.owner))) throw new UserError("owner must be 'supervisor', 'human' or a short name (letters, digits, space, _.:@/+-; max 80 chars)") const nextAction = this.checkText("next_action", p.next_action) @@ -571,6 +579,7 @@ export class Manager { const t = this.get(p.task_id) if (p.decision !== "approve" && p.decision !== "reject") throw new UserError("decision must be 'approve' or 'reject'") if (!settled(t.status)) throw new UserError(`task is ${t.status}; nothing to approve yet`) + this.assertNotCleaning(t) const h = this.handoffOf(t) if (!PARKED.has(t.status) && !APPROVABLE_STATES.has(h?.state)) throw new UserError(`task is not waiting for approval (status ${t.status}, handoff state ${h?.state || "none"}); use continue_task or update_handoff`) const instructions = this.checkText("instructions", p.instructions, daemonConfig().limits.task_max_chars) @@ -615,10 +624,28 @@ export class Manager { return { task_id: t.id, status: t.status, ...(rec ? { decision: rec.decision } : {}), handoff: this.handoffOf(t) } } + // Refuse to resume a task whose worktree is being removed (the removal has async steps). + assertNotCleaning(t) { + if (this.cleaning.has(t.id)) throw new UserError("task worktree is being cleaned up; wait for cleanup to finish (then delegate a new task)") + } + async cleanup(id, discard = false) { const t = this.get(id) if (!settled(t.status)) throw new UserError(`task is ${t.status}; cancel it or wait before cleanup`) if (t.worktree_removed) return { task_id: id, message: "already cleaned up", diff_path: path.join(this.taskDir(id), "diff.patch") } + this.assertNotCleaning(t) + // Marked before the first await, so continue_task / approve_task cannot queue a run into a worktree + // that is being deleted. + this.cleaning.add(t.id) + try { + return await this.cleanupLocked(t, discard) + } finally { + this.cleaning.delete(t.id) + } + } + + async cleanupLocked(t, discard) { + const id = t.id const repo = this.repo(t.repo) const wt = t.worktree_path let files = [] @@ -667,7 +694,7 @@ export class Manager { const age = (t) => (Date.now() - Date.parse(t.finished_at || t.updated_at || t.created_at)) / 86400000 const out = { dry_run: !!dry_run, worktree_days: wd ?? null, task_days: td ?? null, parked_days: pd ?? null, worktrees_removed: [], tasks_deleted: [], parked_kept: [], errors: [] } for (const t of [...this.tasks.values()]) { - if (this.live.has(t.id)) continue + if (this.live.has(t.id) || this.cleaning.has(t.id)) continue if (PARKED.has(t.status)) { const pa = (Date.now() - Date.parse(t.handoff?.updated_at || t.finished_at || t.updated_at || t.created_at)) / 86400000 if (pd != null && pa >= pd && !t.worktree_removed) { @@ -708,6 +735,15 @@ export class Manager { return out } async removeTaskWorktree(t, trigger) { + this.assertNotCleaning(t) + this.cleaning.add(t.id) + try { + return await this.removeTaskWorktreeLocked(t, trigger) + } finally { + this.cleaning.delete(t.id) + } + } + async removeTaskWorktreeLocked(t, trigger) { const repo = this.repoSafe(t) let files = 0 if (fs.existsSync(t.worktree_path)) { diff --git a/test/handoff-flow.test.mjs b/test/handoff-flow.test.mjs index d229cbd..f324445 100644 --- a/test/handoff-flow.test.mjs +++ b/test/handoff-flow.test.mjs @@ -282,3 +282,51 @@ test("removing a parked task's worktree closes it (cleanup_task, or retention wi assert.equal(mgr.result(b.id).handoff.updated_by, "retention") assert.ok(mgr.tasks.has(b.id), "the closed task record stays (it follows normal retention from now on)") }) + +test("a task stopped by a graceful shutdown (interrupted, no result) is finalized on restart", async () => { + const d = await mgr.delegate({ repo: "demo", task: "interrupted by shutdown", test_command: PASS }) + const t = mgr.tasks.get(d.task_id) + fs.writeFileSync(path.join(t.worktree_path, "partial.txt"), "half done\n") + t.session_id = "ses_int" + t.runs.push({ n: 1, kind: "initial", profile: t.profile, backend: t.backend, started_at: new Date().toISOString(), finished_at: new Date().toISOString(), reason: "shutdown" }) + // what shutdown() / onExit() leave behind: status interrupted, an error, no result + t.status = "interrupted" + t.errors.push("workhorse daemon stopped while this task was running. Use continue_task to resume.") + mgr.save(t) + assert.equal(mgr.result(t.id).message.includes("No result yet"), true) + const m2 = manager() + await m2.recover() + const r = m2.result(t.id) + assert.equal(r.status, "interrupted") + assert.equal(r.verdict, "interrupted") + assert.deepEqual(r.files_changed.map((f) => f.path), ["partial.txt"]) + assert.equal(r.handoff.state, "retryable") + assert.equal(r.handoff.resume.tool, "continue_task") + assert.ok(r.handoff.failed_checks.some((c) => c.check === "interrupted")) + assert.ok(audit().some((l) => l.task_id === t.id && l.event === "finalize_interrupted_on_restart")) + const again = manager() + await again.recover() // idempotent: a finalized interrupted task is left alone + assert.equal(again.result(t.id).timings.finished_at, r.timings.finished_at) +}) + +test("cleanup marks the task before its first async step: continue/approve/update are refused meanwhile", async () => { + const t = await finishedTask({ text: RESULT("needs_approval", "needs: approve Z") }) + const pending = mgr.cleanup(t.id, true) // not awaited: removal is in progress + await assert.rejects(mgr.continueTask(t.id, "more"), /being cleaned up/) + await assert.rejects(mgr.approve({ task_id: t.id, decision: "approve" }), /being cleaned up/) + assert.throws(() => mgr.updateHandoff({ task_id: t.id, note: "x" }), /being cleaned up/) + await assert.rejects(mgr.cleanup(t.id, true), /being cleaned up/) + const sw = await mgr.sweep({ worktree_days: 0, parked_days: 0, dry_run: true }) + assert.ok(!sw.worktrees_removed.includes(t.id), "retention skips a task being cleaned up") + await pending + assert.equal(t.worktree_removed, true) + assert.equal(t.status, "cancelled") + assert.equal(mgr.cleaning.size, 0) + await assert.rejects(mgr.continueTask(t.id, "more"), /cleaned up; delegate a new task/) + // same guard during retention's worktree removal + const u = await finishedTask({ text: RESULT("done") }) + const p2 = mgr.removeTaskWorktree(u, "test") + await assert.rejects(mgr.continueTask(u.id, "more"), /being cleaned up/) + await p2 + assert.equal(mgr.cleaning.size, 0) +}) diff --git a/test/recovery.test.mjs b/test/recovery.test.mjs index 48be825..f714cf7 100644 --- a/test/recovery.test.mjs +++ b/test/recovery.test.mjs @@ -75,4 +75,8 @@ H.itest("graceful SIGTERM marks running tasks interrupted", async () => { assert.equal(fs.existsSync(`/proc/${pid}`), false) daemon = await H.startDaemon(env) assert.equal((await rpc("task_status", { task_id: r.task_id })).status, "interrupted") + // finalized on restart: a result and a retryable handoff exist + const res = await rpc("task_result", { task_id: r.task_id }) + assert.equal(res.verdict, "interrupted") + assert.equal(res.handoff.state, "retryable") }) From bc172f521b2650681e7eedcfe9b9a01bd95031a5 Mon Sep 17 00:00:00 2001 From: mrchatam <287639636+mrchatam@users.noreply.github.com> Date: Sun, 27 Sep 2026 01:12:47 +0330 Subject: [PATCH 8/9] Fix daemon restart socket race, close parked tasks via update_handoff, record approval source - shutdown removes only its own socket (inode check) and pid file; a new daemon waits for a previous one that is still stopping before recovery; workhorse stop/restart wait for exit - update_handoff state=closed on a parked task goes through closeTask (cancelled/closed) - approvals, handoff updates and closes record source {channel, auth} next to free-text by - tests: restart race, closed-on-parked, source; helpers wait for the daemon pid --- CHANGELOG.md | 8 +++++ bin/workhorse | 12 ++++++-- docs/handoff.md | 13 ++++++-- docs/security.md | 3 +- lib/config.mjs | 1 + lib/daemon.mjs | 63 ++++++++++++++++++++++++++++++++++++-- lib/tasks.mjs | 34 +++++++++++++++----- test/credentials.test.mjs | 3 +- test/handoff-flow.test.mjs | 17 ++++++++-- test/helpers.mjs | 21 ++++++++++--- test/recovery.test.mjs | 16 ++++++++++ 11 files changed, 167 insertions(+), 24 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4013099..da2ae15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -48,10 +48,18 @@ can resume it without guessing. the handoff it replaced, and the approval that answered it. - `cancel_task` on a parked task closes it (status `cancelled`, handoff `closed`). - `list_tasks status: "terminal"` also includes parked tasks, since no worker is running. +- `update_handoff state: "closed"` on a parked task closes it like `cancel_task` / reject without + instructions (status `cancelled`, handoff `closed`) instead of unparking it to `completed`. +- Approvals, handoff updates and closes record `source` (`channel` mcp/cli, `auth`) next to the + free-text `by`, so attribution is not only a caller-chosen name. ### Fixed - A task stopped by a graceful daemon shutdown (`workhorse stop`, SIGTERM) was marked `interrupted` but never finalized, so it had no result and no handoff. It is now finalized on the next start. +- The daemon's shutdown removed the socket path unconditionally after finalizing tasks, which could + delete the socket of a daemon started in the meantime. It now removes only its own socket (same + inode), writes `run/daemon.pid`, and a new daemon waits for a previous one that is still stopping + before recovery. `workhorse stop` / `restart` wait for the processes to exit. - `cleanup_task` and retention mark a task before their first async step. While its worktree is being removed, `continue_task`, `approve_task`, `update_handoff` and a second cleanup are refused instead of queueing a run into a worktree that is being deleted. diff --git a/bin/workhorse b/bin/workhorse index 243dcae..85d72c4 100755 --- a/bin/workhorse +++ b/bin/workhorse @@ -77,8 +77,16 @@ async function stop() { fs.mkdirSync(dirs.run, { recursive: true, mode: 0o700 }) fs.writeFileSync(path.join(dirs.run, "stop"), "") if (sup) try { process.kill(sup, "SIGTERM") } catch {} - for (let i = 0; i < 60 && fs.existsSync(SOCKET_PATH); i++) await wait(500) - out("stopped") + // Wait for the processes to exit, not just for the socket to go away: a daemon removes its socket + // first and then finalizes running tasks, and a successor must not start before it is gone. + const { isDaemonPid, daemonPidFromFile } = await import("../lib/daemon.mjs") + const dpid = daemonPidFromFile() + const alive = (pid) => { + try { process.kill(pid, 0); return true } catch (e) { return e.code === "EPERM" } + } + let i = 0 + for (; i < 240 && ((sup && alive(sup)) || (dpid && isDaemonPid(dpid))); i++) await wait(250) + out(i >= 240 ? `still stopping after 60 s (supervisor ${sup || "-"}, daemon ${dpid || "-"})` : "stopped") } try { diff --git a/docs/handoff.md b/docs/handoff.md index 916e8d7..c1a92cd 100644 --- a/docs/handoff.md +++ b/docs/handoff.md @@ -111,8 +111,11 @@ author. `by` names who is acting (default `supervisor` over MCP, `human (cli)` f change goes into `history` and the audit log. Setting `state` to `needs_approval` or `needs_input` parks a finished task for a human (the owner -becomes `human` unless you pass one), which also protects its worktree from retention. Setting any -other state on a parked task unparks it. It goes back to its previous status, or `completed`. +becomes `human` unless you pass one), which also protects its worktree from retention. Setting +`state: "closed"` on a parked task closes it exactly like `approve_task decision=reject` without +instructions or `cancel_task`: status `cancelled`, handoff `closed`, worktree kept (other fields in the +same call are applied afterwards). Setting any other state on a parked task unparks it. It goes back +to its previous status, or `completed`. `workhorse handoff ` without options prints the record and past approvals. ## Approving or rejecting @@ -131,6 +134,12 @@ parked, or whose handoff state is `needs_approval`, `needs_input` or `blocked`: Every decision is stored in `task.approvals` and in the `previous_results` entry of the run it answered, and is logged as an `approval` event. +**Attribution.** `by` is free text chosen by the caller. Each approval, handoff update and close +also records `source`: `channel` (`mcp` for the MCP shim, `cli` for the operator CLI) and `auth` +(what the daemon actually verified). Both channels use the same daemon socket token, so `channel` is +what the client declares and `auth` is `daemon_token`; treat neither `by` nor `channel` as proof of +who acted. + **Approval does not change the sandbox.** It is a decision passed to the worker, not a permission grant. The guard and bwrap rules still apply, so a worker still cannot reach the network or install packages. If the approved action needs that (for example adding a dependency to an offline cache), the diff --git a/docs/security.md b/docs/security.md index 4a2cfc9..ea72a9e 100644 --- a/docs/security.md +++ b/docs/security.md @@ -72,7 +72,8 @@ [SECURITY.md](../SECURITY.md)). - `approve_task` is a coordination signal, not a permission grant. It never widens the sandbox or the guard. The approver name (`by`) is recorded as given, and any client holding the daemon socket - and token (the supervisor included) can approve. Keep a human in the loop at the supervisor level when + and token (the supervisor included) can approve. The recorded `source.channel` (`mcp` / `cli`) is + self-declared by the client for the same reason; `source.auth` says what was verified. Keep a human in the loop at the supervisor level when approvals matter. ## Recommendations diff --git a/lib/config.mjs b/lib/config.mjs index 4a570d5..a050031 100644 --- a/lib/config.mjs +++ b/lib/config.mjs @@ -37,6 +37,7 @@ export const dirs = { } export const SOCKET_PATH = path.join(dirs.run, "daemon.sock") export const TOKEN_PATH = path.join(dirs.run, "token") +export const DAEMON_PID_PATH = path.join(dirs.run, "daemon.pid") export const VERSION = (() => { try { diff --git a/lib/daemon.mjs b/lib/daemon.mjs index aa542a7..ad57902 100644 --- a/lib/daemon.mjs +++ b/lib/daemon.mjs @@ -1,16 +1,64 @@ // workhorse daemon: JSON-RPC over HTTP on a unix socket (0600, in a 0700 dir) + bearer token file (0600). import fs from "node:fs" import http from "node:http" +import net from "node:net" import crypto from "node:crypto" -import { dirs, SOCKET_PATH, TOKEN_PATH } from "./config.mjs" +import { dirs, SOCKET_PATH, TOKEN_PATH, DAEMON_PID_PATH } from "./config.mjs" import { ensureDir, redact, registerSecret } from "./util.mjs" import { audit, sanitizeParams } from "./audit.mjs" import { Manager, UserError } from "./tasks.mjs" import { daemonConfig, secretEnvNames, VERSION } from "./config.mjs" +const sleep = (ms) => new Promise((r) => setTimeout(r, ms)) + +// Is `pid` a live workhorsed process? (/proc check guards against PID reuse; Linux only project.) +export function isDaemonPid(pid) { + if (!Number.isInteger(pid) || pid <= 0 || pid === process.pid) return false + try { + process.kill(pid, 0) + } catch (e) { + if (e.code !== "EPERM") return false + } + try { + return fs.readFileSync(`/proc/${pid}/cmdline`, "utf8").includes("workhorsed") + } catch { + return !fs.existsSync("/proc/self") // no procfs: trust kill(pid, 0) + } +} + +export function daemonPidFromFile() { + try { + return Number(fs.readFileSync(DAEMON_PID_PATH, "utf8").trim()) || null + } catch { + return null + } +} + +const socketAnswers = () => new Promise((resolve) => { + const c = net.connect(SOCKET_PATH) + const done = (v) => { c.destroy(); resolve(v) } + c.once("connect", () => done(true)) + c.once("error", () => done(false)) + setTimeout(() => done(false), 1000).unref() +}) + +// A previous daemon may still be shutting down (finalizing tasks). Starting now would run recovery on +// tasks it still owns and replace its socket, which its shutdown would then delete. Wait for it to exit. +async function waitForPreviousDaemon(maxMs = 60000) { + const t0 = Date.now() + for (;;) { + const pid = daemonPidFromFile() + const alive = pid && isDaemonPid(pid) + if (!alive && !(fs.existsSync(SOCKET_PATH) && (await socketAnswers()))) return + if (Date.now() - t0 > maxMs) throw new Error(`another workhorsed is still running${pid ? ` (pid ${pid})` : ""} and answering on ${SOCKET_PATH}`) + await sleep(200) + } +} + export async function startDaemon() { ensureDir(dirs.run, 0o700) fs.chmodSync(dirs.run, 0o700) + await waitForPreviousDaemon() for (const d of [dirs.tasks, dirs.logs, dirs.worktrees, dirs.repos]) ensureDir(d) let token if (fs.existsSync(TOKEN_PATH)) token = fs.readFileSync(TOKEN_PATH, "utf8").trim() @@ -101,14 +149,23 @@ export async function startDaemon() { }) }) fs.chmodSync(SOCKET_PATH, 0o600) + const socketIno = fs.statSync(SOCKET_PATH).ino + fs.writeFileSync(DAEMON_PID_PATH, `${process.pid}\n`, { mode: 0o600 }) audit("daemon", { event: "started", pid: process.pid, credentials: Object.keys(mgr.secretEnv()), credential_sources: mgr.credentialSources() }) + let stopping = false const stop = async (sig) => { + if (stopping) return + stopping = true audit("daemon", { event: "stopping", signal: sig }) - server.close() + server.close() // Node removes its own socket file here await mgr.shutdown() + // Remove only what is still ours: a successor daemon may own the path by now. + try { + if (fs.statSync(SOCKET_PATH).ino === socketIno) fs.unlinkSync(SOCKET_PATH) + } catch {} try { - fs.unlinkSync(SOCKET_PATH) + if (daemonPidFromFile() === process.pid) fs.unlinkSync(DAEMON_PID_PATH) } catch {} process.exit(0) } diff --git a/lib/tasks.mjs b/lib/tasks.mjs index 06fc4cf..0f67ebf 100644 --- a/lib/tasks.mjs +++ b/lib/tasks.mjs @@ -509,6 +509,13 @@ export class Manager { return caller?.client === "workhorse" ? "human (cli)" : "supervisor" } + // Which channel a request came through, recorded next to the free-text `by`. Both the MCP shim and + // the operator CLI authenticate with the same daemon socket token, so `channel` is what the client + // declares; only `auth` says what was actually verified. + sourceOf(caller) { + return { channel: caller?.client === "workhorse" ? "cli" : "mcp", auth: "daemon_token" } + } + checkText(name, v, max = 2000) { if (v === undefined || v === null) return undefined if (typeof v !== "string" || !v.trim() || v.length > max) throw new UserError(`${name} must be a non-empty string up to ${max} chars`) @@ -535,6 +542,16 @@ export class Manager { const note = this.checkText("note", p.note) if (p.state === undefined && p.owner === undefined && nextAction === undefined && note === undefined) throw new UserError("nothing to update: pass state, owner, next_action and/or note") const who = this.actor(p.by, caller) + const source = this.sourceOf(caller) + // Closing a parked task means the same as rejecting it without instructions: status cancelled, + // handoff closed, worktree kept (closeTask). Other edits in the same call are applied afterwards. + if (p.state === "closed" && PARKED.has(t.status)) { + const res = this.closeTask(t, who, `closed by ${who} via update_handoff`, null, source) + const rest = { ...p } + delete rest.state + if (rest.owner === undefined && nextAction === undefined && note === undefined) return res + return this.updateHandoff(rest, caller) + } const at = now() const h = { ...this.handoffOf(t) } const changed = [] @@ -560,13 +577,13 @@ export class Manager { delete t.parked_from } } - h.history = [...(h.history || []), { at, by: who, event: "update", changed, state: h.state, owner: h.owner, ...(from !== t.status ? { from_status: from, to_status: t.status } : {}) }].slice(-20) + h.history = [...(h.history || []), { at, by: who, source, event: "update", changed, state: h.state, owner: h.owner, ...(from !== t.status ? { from_status: from, to_status: t.status } : {}) }].slice(-20) h.derived = false h.updated_at = at h.updated_by = who t.handoff = redact(h) this.save(t) - this.event(t, "handoff_updated", { by: who, changed, state: h.state, owner: h.owner, ...(from !== t.status ? { from_status: from } : {}) }) + this.event(t, "handoff_updated", { by: who, source, changed, state: h.state, owner: h.owner, ...(from !== t.status ? { from_status: from } : {}) }) return { task_id: t.id, status: t.status, handoff: this.handoffOf(t) } } @@ -585,12 +602,13 @@ export class Manager { const instructions = this.checkText("instructions", p.instructions, daemonConfig().limits.task_max_chars) const note = this.checkText("note", p.note) const who = this.actor(p.by, caller) + const source = this.sourceOf(caller) const request = h?.context?.worker_request || h?.next_action || "the pending request" - const rec = { at: now(), by: who, decision: p.decision, request: head(request, 300), ...(note ? { note } : {}), ...(instructions ? { instructions: head(instructions, 300) } : {}) } - const logApproval = () => this.event(t, "approval", { decision: p.decision, by: who, handoff_state: h?.state || null, resumed: !(p.decision === "reject" && !instructions) }) + const rec = { at: now(), by: who, source, decision: p.decision, request: head(request, 300), ...(note ? { note } : {}), ...(instructions ? { instructions: head(instructions, 300) } : {}) } + const logApproval = () => this.event(t, "approval", { decision: p.decision, by: who, source, handoff_state: h?.state || null, resumed: !(p.decision === "reject" && !instructions) }) if (p.decision === "reject" && !instructions) { logApproval() - return this.closeTask(t, who, `approval rejected by ${who}${note ? `: ${note}` : ""}`, rec) + return this.closeTask(t, who, `approval rejected by ${who}${note ? `: ${note}` : ""}`, rec, source) } const msg = p.decision === "approve" ? `APPROVED by ${who}: ${request}${note ? `\nNote: ${note}` : ""}\n\n${instructions || "Proceed with the approved action and finish the task."}\n\n(Approval does not change the sandbox: actions it blocks stay blocked. If the approved action needs network or an install, the operator has done it outside, or you must report that under concerns.)` @@ -600,7 +618,7 @@ export class Manager { return { ...r, decision: p.decision, approval: rec } } - closeTask(t, who, reason, rec = null) { + closeTask(t, who, reason, rec = null, source = null) { const from = t.status const h = { ...this.handoffOf(t) } t.status = "cancelled" @@ -614,13 +632,13 @@ export class Manager { ? `Closed (${reason}). The worktree is gone; the diff is archived at ${path.join(this.taskDir(t.id), "diff.patch")}. Delegate a new task if the work is still needed.` : `Closed (${reason}). The worktree is kept: merge any useful partial diff, continue_task to resume, or cleanup_task (discard_unmerged_changes=true) to drop it.` h.resume = { ...h.resume, tool: null, args: null } - h.history = [...(h.history || []), { at, by: who, event: "closed", reason, from_status: from }].slice(-20) + h.history = [...(h.history || []), { at, by: who, ...(source ? { source } : {}), event: "closed", reason, from_status: from }].slice(-20) h.derived = false h.updated_at = at h.updated_by = who t.handoff = redact(h) this.save(t) - this.event(t, "closed", { by: who, from_status: from, reason }) + this.event(t, "closed", { by: who, ...(source ? { source } : {}), from_status: from, reason }) return { task_id: t.id, status: t.status, ...(rec ? { decision: rec.decision } : {}), handoff: this.handoffOf(t) } } diff --git a/test/credentials.test.mjs b/test/credentials.test.mjs index 92695dd..82394b7 100644 --- a/test/credentials.test.mjs +++ b/test/credentials.test.mjs @@ -101,8 +101,7 @@ H.itest("entry added later is loaded at task spawn, used, redacted and never per }) H.itest("daemon restart loads the key from the store at startup", async () => { - process.kill(-daemon.pid, "SIGTERM") - await H.sleep(1500) + await H.stopDaemon(daemon) daemon = await H.startDaemon(env) const h = await rpc("health", {}) assert.deepEqual(h.credential_sources, [{ name: "NVIDIA_API_KEY", source: "secret_store" }]) diff --git a/test/handoff-flow.test.mjs b/test/handoff-flow.test.mjs index f324445..d1fe91a 100644 --- a/test/handoff-flow.test.mjs +++ b/test/handoff-flow.test.mjs @@ -182,6 +182,9 @@ test("reject: closes a parked task (worktree kept), or resumes it with instructi assert.equal(r2.status, "queued") assert.match(u.pending_run.message, /^DENIED by human \(cli\): approve deleting the legacy module\. Do not do that\./) assert.match(u.pending_run.message, /deprecate it instead/) + // the channel is recorded separately from the free-text `by` + assert.deepEqual(u.approvals.at(-1).source, { channel: "cli", auth: "daemon_token" }) + assert.deepEqual(t.approvals.at(-1).source, { channel: "mcp", auth: "daemon_token" }) const c = await finishedTask({ text: RESULT("needs_approval", "needs: approve X") }) assert.equal((await mgr.cancel(c.id)).status, "cancelled") @@ -217,9 +220,19 @@ test("update_handoff: owner/next_action/note, parking by state, validation", asy assert.equal(p.handoff.owner, "human") assert.equal(p.handoff.updated_by, "human (cli)") assert.equal(t.parked_from, "completed") - const back = mgr.updateHandoff({ task_id: t.id, state: "closed" }) + // any other state unparks it back to the previous status + const back = mgr.updateHandoff({ task_id: t.id, state: "needs_fix" }) assert.equal(back.status, "completed") - assert.equal(back.handoff.state, "closed") + assert.equal(back.handoff.state, "needs_fix") + // closing a parked task goes through the same path as reject-without-instructions + mgr.updateHandoff({ task_id: t.id, state: "needs_input" }) + const closed = mgr.updateHandoff({ task_id: t.id, state: "closed", note: "not needed any more" }) + assert.equal(closed.status, "cancelled") + assert.equal(closed.handoff.state, "closed") + assert.match(closed.handoff.next_action, /closed by supervisor via update_handoff/) + assert.equal(closed.handoff.notes.at(-1).note, "not needed any more") + assert.deepEqual(closed.handoff.history.find((x) => x.event === "closed").source, { channel: "mcp", auth: "daemon_token" }) + assert.equal(t.parked_from, undefined) assert.ok(!mgr.list({ status: "needs_attention" }).tasks.some((x) => x.task_id === t.id)) // validation assert.throws(() => mgr.updateHandoff({ task_id: t.id }), /nothing to update/) diff --git a/test/helpers.mjs b/test/helpers.mjs index 41aafb1..5417dcb 100644 --- a/test/helpers.mjs +++ b/test/helpers.mjs @@ -158,22 +158,35 @@ export function startMock(env) { }) } +// Starts a daemon and waits until it (this pid) is listening. The daemon itself waits for a previous +// daemon that is still shutting down. export async function startDaemon(env, extraEnv = {}) { - try { fs.unlinkSync(path.join(env.dataDir, "run", "daemon.sock")) } catch {} const p = spawn(process.execPath, [path.join(APP, "bin/workhorsed")], { env: { PATH: process.env.PATH, HOME: process.env.HOME, WH_CONFIG_DIR: env.cfgDir, WH_DATA_DIR: env.dataDir, ...extraEnv }, stdio: ["ignore", fs.openSync(path.join(env.root, "daemon.log"), "a"), fs.openSync(path.join(env.root, "daemon.log"), "a")], detached: true, }) const sock = path.join(env.dataDir, "run", "daemon.sock") - for (let i = 0; i < 300; i++) { - if (fs.existsSync(sock)) break + const pidFile = path.join(env.dataDir, "run", "daemon.pid") + const ours = () => { + try { return Number(fs.readFileSync(pidFile, "utf8")) === p.pid } catch { return false } + } + for (let i = 0; i < 900 && !(ours() && fs.existsSync(sock)); i++) { + if (p.exitCode !== null) throw new Error(`daemon exited with ${p.exitCode} during startup; see ${path.join(env.root, "daemon.log")}`) await sleep(100) } - await sleep(200) + await sleep(100) return p } +// Signals the daemon's process group and waits until the daemon process has exited. +export async function stopDaemon(p, sig = "SIGTERM", timeoutMs = 60000) { + if (!p || p.exitCode !== null || p.signalCode !== null) return + const exited = new Promise((r) => p.once("exit", r)) + try { process.kill(-p.pid, sig) } catch { try { process.kill(p.pid, sig) } catch {} } + await Promise.race([exited, sleep(timeoutMs)]) +} + export async function waitFor(rpc, id, pred, timeoutMs = 120000) { const t0 = Date.now() let st diff --git a/test/recovery.test.mjs b/test/recovery.test.mjs index f714cf7..5a67c13 100644 --- a/test/recovery.test.mjs +++ b/test/recovery.test.mjs @@ -80,3 +80,19 @@ H.itest("graceful SIGTERM marks running tasks interrupted", async () => { assert.equal(res.verdict, "interrupted") assert.equal(res.handoff.state, "retryable") }) + +H.itest("a daemon started while the previous one is still stopping waits for it and keeps its own socket", async () => { + const r = await rpc("delegate_task", { repo: H.REPO_NAME, task: "MOCK_SCENARIO=slowthendone" }) + await H.waitFor(rpc, r.task_id, (s) => s.status === "running") + await H.sleep(2000) + const old = daemon + process.kill(old.pid, "SIGTERM") // old daemon removes its socket, then finalizes the running task + daemon = await H.startDaemon(env) // must wait for the old process instead of racing it + assert.ok(old.exitCode !== null || old.signalCode !== null || !fs.existsSync(`/proc/${old.pid}`), "old daemon exited before the new one listened") + await H.sleep(1500) + const sock = path.join(env.dataDir, "run", "daemon.sock") + assert.ok(fs.existsSync(sock), "the new daemon's socket survived the old daemon's shutdown") + assert.equal((await rpc("health", {})).pid, daemon.pid) + assert.equal(Number(fs.readFileSync(path.join(env.dataDir, "run", "daemon.pid"), "utf8")), daemon.pid) + assert.equal((await rpc("task_status", { task_id: r.task_id })).status, "interrupted") +}) From 90aadb4769990a12afab6808f21125e86aebef66 Mon Sep 17 00:00:00 2001 From: mrchatam <287639636+mrchatam@users.noreply.github.com> Date: Sun, 27 Sep 2026 01:26:35 +0330 Subject: [PATCH 9/9] test: stopDaemon clears its timeout so test processes exit promptly --- test/helpers.mjs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/helpers.mjs b/test/helpers.mjs index 5417dcb..a350fc3 100644 --- a/test/helpers.mjs +++ b/test/helpers.mjs @@ -184,7 +184,9 @@ export async function stopDaemon(p, sig = "SIGTERM", timeoutMs = 60000) { if (!p || p.exitCode !== null || p.signalCode !== null) return const exited = new Promise((r) => p.once("exit", r)) try { process.kill(-p.pid, sig) } catch { try { process.kill(p.pid, sig) } catch {} } - await Promise.race([exited, sleep(timeoutMs)]) + let timer + await Promise.race([exited, new Promise((r) => { timer = setTimeout(r, timeoutMs) })]) + clearTimeout(timer) // do not keep the test process alive for the timeout } export async function waitFor(rpc, id, pred, timeoutMs = 120000) {