From 47d881bfbfe78d164fa3af2dc5c35d0f356a29c6 Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Tue, 28 Jul 2026 14:47:14 +0300 Subject: [PATCH 1/4] =?UTF-8?q?feat(cli):=20`kild=20watch`=20=E2=80=94=20t?= =?UTF-8?q?he=20wake=20path=20a=20turn-end=20hook=20cannot=20provide?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Stop hook fires when a turn ENDS. A harness sitting idle is therefore unreachable: mail queues, nothing wakes it, and it finds out whenever its operator next happens to prompt. Both attached agents hit this today — each sat on the other's messages believing comms were fine, and the operator had to say "check your inbox" to break it. No hook can fix it. Every harness hook event fires on a turn, tool or session edge and none ticks while a session is idle, so the wake has to come from outside the hook system. kild cannot push either — not owning the process is what makes an attached agent attached. So kild owns the WAITING and the harness owns the WAKING. `kild watch` blocks until somebody else speaks and exits; whatever background facility the harness has runs it and reacts to that exit. kild learns nothing about the harness. It reads the message LOG, never the inbox. Non-destructive by construction, so a watcher on a timer can never eat what the Stop hook exists to deliver: watch is the wake signal, inbox is the delivery. That also means it needs nothing new from the engine — a cursor, which /messages?since= has always served — so it works against any engine already on main rather than requiring a lockstep upgrade. Credit to helm's agent for that correction; I was about to build it on a pending count it does not need. EXIT CODES ARE THE INTERFACE, and a quiet engine is not a dead one: 0 mail 1 usage 2 quiet (window elapsed) 3 unreachable If quiet and unreachable both exited 0, every harness built on this would inherit the failure the command exists to remove — a wake mechanism that goes silent and is indistinguishable from one with nothing to report. Not retrofittable once harnesses depend on it, so it is decided now. Two bugs found by running it rather than testing it: - The initial cursor fetch was unguarded, so a dead engine at startup exited 1 (usage) instead of 3. A harness could not tell "the engine is gone" from "you typed it wrong" — the exact distinction the codes exist for, defeated at the moment it matters most. Every existing test passed --since, which skips that call, so they all walked straight past it. Now covered. - Timing constants were const declarations in cli.ts, which calls dispatch() at the top of the module — the same temporal-dead-zone failure already paid for once in this codebase. They live in the imported slice now, where evaluation order makes it impossible rather than merely fixed. Class, not instance. Gates: 432 tests, typecheck, lint, e2e 70/70. Dogfooded live against the running engine: a quiet window exits 2, a dead engine exits 3, and a watcher started with no arguments resolved its own kild and woke on a real reply. --- engine/src/cli.attached.test.ts | 108 ++++++++++++++++++++++++++++++++ engine/src/cli.ts | 94 ++++++++++++++++++++++++++- engine/src/kild/watch.test.ts | 89 ++++++++++++++++++++++++++ engine/src/kild/watch.ts | 101 +++++++++++++++++++++++++++++ 4 files changed, 391 insertions(+), 1 deletion(-) create mode 100644 engine/src/kild/watch.test.ts create mode 100644 engine/src/kild/watch.ts diff --git a/engine/src/cli.attached.test.ts b/engine/src/cli.attached.test.ts index 9ceb0f09..64ce7533 100644 --- a/engine/src/cli.attached.test.ts +++ b/engine/src/cli.attached.test.ts @@ -38,6 +38,9 @@ let authHeaders: Array = []; /** What an `attach` answers with, when a test needs it to differ from `drainResponse` — a * send mints its credential through attach, so those tests need both to be distinct. */ let attachResponse: { status: number; body: unknown } | undefined; +/** What `GET /messages` answers with. `kild watch` reads the log, never the inbox, so its + * tests drive this rather than `drainResponse`. */ +let messagesResponse: { status: number; body: unknown } | undefined; beforeAll(async () => { kildHome = await fs.realpath(await fs.mkdtemp(path.join(os.tmpdir(), 'kild-cli-'))); @@ -55,6 +58,9 @@ beforeAll(async () => { if (url.pathname.endsWith('/agents/attach') && attachResponse) { return Response.json(attachResponse.body, { status: attachResponse.status }); } + if (url.pathname.endsWith('/messages') && messagesResponse) { + return Response.json(messagesResponse.body, { status: messagesResponse.status }); + } return Response.json(drainResponse.body, { status: drainResponse.status }); }, }); @@ -420,3 +426,105 @@ test('...but an ordinary `inbox` reports the real cause, not a usage message', a expect(human.stderr).toContain('unreadable attachment record'); expect(human.stderr).not.toContain('usage: kild inbox'); }); + +/** + * `kild watch` — the wake path a turn-end hook cannot provide. Its EXIT CODE is the whole + * interface for whatever background facility runs it, so that is what these assert. + */ +const logMessage = (seq: number, from: string) => ({ + id: `m-${seq}`, + kildId: 'kild-9', + from, + to: ['kild'], + text: 'x', + ts: seq, + seq, +}); + +test('watch exits 0 when somebody else speaks, and says who', async () => { + messagesResponse = { status: 200, body: [logMessage(5, 'claude')] }; + const watched = await runCli([ + 'watch', + 'kild-9', + '--as', + 'kild', + '--since', + '4', + '--timeout', + '3', + ]); + expect(watched.exitCode).toBe(0); + expect(watched.stdout).toContain('@claude'); + messagesResponse = undefined; +}); + +test("watch ignores the watcher's own messages", async () => { + // Otherwise every send would instantly wake the watcher that sent it. + messagesResponse = { status: 200, body: [logMessage(5, 'kild')] }; + const watched = await runCli([ + 'watch', + 'kild-9', + '--as', + 'kild', + '--since', + '4', + '--timeout', + '1', + ]); + expect(watched.exitCode).toBe(2); // quiet, not mail + messagesResponse = undefined; +}); + +test('a quiet engine and a DEAD engine exit differently', async () => { + // The distinction that stops a harness inheriting the silent-failure shape: "nothing + // happened" must never look like "I no longer know". + messagesResponse = { status: 200, body: [] }; + const quiet = await runCli(['watch', 'kild-9', '--as', 'kild', '--since', '1', '--timeout', '1']); + expect(quiet.exitCode).toBe(2); + expect(quiet.stderr).toContain('nothing new'); + messagesResponse = undefined; + + // Nothing listening on this port at all. + const dead = await runCli( + ['watch', 'kild-9', '--as', 'kild', '--since', '1', '--timeout', '30'], + 'http://127.0.0.1:1', + ); + expect(dead.exitCode).toBe(3); + expect(dead.stderr).toContain('unreachable'); + expect(dead.exitCode).not.toBe(quiet.exitCode); +}); + +test('watch never drains — the hook keeps its mail', async () => { + messagesResponse = { status: 200, body: [logMessage(5, 'claude')] }; + drainRequests = []; + await runCli(['watch', 'kild-9', '--as', 'kild', '--since', '4', '--timeout', '3']); + // A watcher that consumed the inbox would eat exactly what the Stop hook exists to deliver. + expect(drainRequests.map((r) => r.path)).not.toContain('/api/kilds/kild-9/inbox/drain'); + expect(drainRequests.every((r) => r.method === 'GET')).toBe(true); + messagesResponse = undefined; +}); + +test('a dead engine is `unreachable` even with no --since to skip the first fetch', async () => { + // Every other watch test passes --since, which skips the initial cursor fetch entirely — so + // they all walked straight past the one call that was unguarded. Without this, a harness + // starting a watcher against a dead engine got exit 1 (usage) instead of 3 (unreachable), + // which is exactly the distinction these codes exist to make. + const dead = await runCli( + ['watch', 'kild-9', '--as', 'kild', '--timeout', '30'], + 'http://127.0.0.1:1', + ); + expect(dead.exitCode).toBe(3); + expect(dead.stderr).toContain('unreachable'); +}); + +test('a bad --timeout is a loud usage error, never a silent default', async () => { + const bad = await runCli(['watch', 'kild-9', '--as', 'kild', '--timeout', 'soon']); + expect(bad.exitCode).toBe(1); + expect(bad.stderr).toContain('--timeout'); +}); + +test('watch with nothing to resolve is a usage error', async () => { + const bare = await runCli(['watch']); + expect(bare.exitCode).toBe(1); + expect(bare.stderr).toContain('usage: kild watch'); +}); diff --git a/engine/src/cli.ts b/engine/src/cli.ts index 32b15fb0..ad09fde9 100755 --- a/engine/src/cli.ts +++ b/engine/src/cli.ts @@ -34,6 +34,14 @@ import { import { compactLiveKilds, formatCompactGitSummary } from './kild/kilds-status.ts'; import { GENERAL_PERSONA, listPersonas } from './kild/personas.ts'; import { addProject, findProject, loadProjects, removeProject } from './kild/projects.ts'; +import { + pollResult, + WATCH_DEFAULT_TIMEOUT_S, + WATCH_EXIT, + WATCH_POLL_MS, + WATCH_TOLERATED_FAILURES, + watchSummary, +} from './kild/watch.ts'; const { values, positionals } = parseArgs({ allowPositionals: true, @@ -56,6 +64,7 @@ const { values, positionals } = parseArgs({ execute: { type: 'boolean', default: false }, // `kild land --execute`: merge for real task: { type: 'string' }, // `kild spawn --task `: the new agent's first message session: { type: 'string' }, // `kild attach|inbox|send --session `: the harness session + timeout: { type: 'string' }, // `kild watch --timeout `: how long to wait }, }); @@ -115,6 +124,8 @@ async function dispatch(): Promise { return kildAttach(action); case 'inbox': return kildInbox(action); + case 'watch': + return kildWatch(action); case 'log': { if (!action) throw new Error('usage: kild log '); return kildLog(action); @@ -127,7 +138,7 @@ async function dispatch(): Promise { return agentsList(); default: console.error( - 'usage: kild …', + 'usage: kild …', ); process.exit(2); } @@ -330,6 +341,87 @@ async function kildAttach(id: string | undefined): Promise { * never stop the operator from finishing a turn. Without the flag it is an ordinary CLI * verb and failures are ordinary loud errors. */ +/** + * `kild watch [ --as ] [--since ] [--timeout ]` — block until + * somebody else speaks in the kild, then exit. + * + * The wake path a turn-end hook cannot provide: a hook fires when a turn ends, so an idle + * harness is unreachable until its operator happens to prompt it. Run this in whatever + * background facility the harness has and react to its exit. + * + * It reads the LOG, never the inbox, so it is non-destructive and cannot eat what the Stop + * hook is there to deliver. Exit codes carry the outcome ({@link WATCH_EXIT}) — in particular + * a quiet engine and a dead one are different codes, so a harness built on this cannot inherit + * the silent-failure shape it exists to remove. + */ +async function kildWatch(idArg: string | undefined): Promise { + const { kildId: id, handle } = await resolveAttachment(idArg, values.as); + if (!id || !handle) { + throw new Error( + 'usage: kild watch [ --as ] [--since ] [--timeout ]\n' + + ' omit id/--as to use the kild this session attached to', + ); + } + const timeout = values.timeout === undefined ? WATCH_DEFAULT_TIMEOUT_S : Number(values.timeout); + if (!Number.isFinite(timeout) || timeout <= 0) { + throw new Error(`--timeout must be a positive number of seconds (got ${values.timeout})`); + } + + // Default to the log's current end, so a watcher started now waits for what happens NEXT + // rather than firing immediately on the conversation it was started in the middle of. + // + // This first call needs the same unreachable handling as the poll loop. Left bare it threw + // to the top-level catch and exited 1 — a *usage* code — so a harness starting a watcher + // against a dead engine could not tell "the engine is gone" from "you typed it wrong". That + // is the distinction the exit codes exist for, defeated at the one moment it matters most. + let cursor: number; + const explicitSince = parseSince(); + if (explicitSince !== undefined) { + cursor = explicitSince; + } else { + try { + cursor = (await kildMessages(id)).at(-1)?.seq ?? 0; + } catch (err) { + console.error(`kild: engine unreachable: ${errText(err)}`); + process.exit(WATCH_EXIT.unreachable); + } + } + const deadline = Date.now() + timeout * 1000; + let failures = 0; + + // Poll FIRST, then sleep: a watcher handed an explicit `--since` that is already behind the + // conversation reacts at once instead of after an interval of avoidable silence. + for (;;) { + let batch: Awaited>; + try { + batch = await kildMessages(id, cursor); + failures = 0; + } catch (err) { + // A restarting engine is not a gone one. Tolerate a few, then say so rather than + // waiting out the window and reporting silence we cannot vouch for. + if (++failures < WATCH_TOLERATED_FAILURES) continue; + console.error(`kild: engine unreachable after ${failures} attempts: ${errText(err)}`); + process.exit(WATCH_EXIT.unreachable); + } + const { incoming, cursor: next } = pollResult(batch, handle, cursor); + cursor = next; + if (incoming.length > 0) { + console.log( + json + ? JSON.stringify({ kildId: id, handle, since: cursor, messages: incoming }, null, 2) + : `${watchSummary(incoming)}\n read with: kild log ${id} --since ${cursor - incoming.length}`, + ); + process.exit(WATCH_EXIT.mail); + } + // Stop when there is no room left for another poll, rather than sleeping past the + // deadline and reporting a window longer than the one that was asked for. + if (Date.now() + WATCH_POLL_MS >= deadline) break; + await new Promise((resolve) => setTimeout(resolve, WATCH_POLL_MS)); + } + console.error(`kild: nothing new in ${timeout}s (cursor ${cursor})`); + process.exit(WATCH_EXIT.quiet); +} + async function kildInbox(idArg: string | undefined): Promise { const claudeStop = values.format === 'claude-stop'; // Omitting both resolves the kild this session attached to — the case the environment diff --git a/engine/src/kild/watch.test.ts b/engine/src/kild/watch.test.ts new file mode 100644 index 00000000..a37b4f98 --- /dev/null +++ b/engine/src/kild/watch.test.ts @@ -0,0 +1,89 @@ +import { describe, expect, test } from 'bun:test'; + +import type { Message } from './kild-types.ts'; +import { pollResult, WATCH_EXIT, watchSummary } from './watch.ts'; + +const message = (seq: number, from: string, to: string[] = ['kild']): Message => ({ + id: `m-${seq}`, + kildId: 'k', + from, + to, + text: 'x', + ts: seq, + seq, +}); + +describe('pollResult', () => { + test('somebody else speaking is a reason to wake', () => { + const { incoming } = pollResult([message(1, 'claude')], 'kild', 0); + expect(incoming).toHaveLength(1); + }); + + test("the watcher's OWN messages never wake it", () => { + // Otherwise every send would immediately re-trigger the watcher that sent it. + const { incoming } = pollResult([message(1, 'kild'), message(2, 'kild')], 'kild', 0); + expect(incoming).toEqual([]); + }); + + test('own messages still advance the cursor', () => { + // Or they would be re-examined on every poll, forever. + const { cursor } = pollResult([message(7, 'kild')], 'kild', 3); + expect(cursor).toBe(7); + }); + + test('a message addressed to someone else still wakes', () => { + // The watcher wants to know the conversation moved. Filtering on `to` would make it deaf + // to everything except its own mail, which is what `inbox` is for. + const { incoming } = pollResult([message(1, 'claude', ['agent'])], 'kild', 0); + expect(incoming).toHaveLength(1); + }); + + test('a mixed batch reports only the others, and advances past all of it', () => { + const batch = [message(4, 'kild'), message(5, 'claude'), message(6, 'kild')]; + const { incoming, cursor } = pollResult(batch, 'kild', 3); + expect(incoming.map((m) => m.seq)).toEqual([5]); + expect(cursor).toBe(6); + }); + + test('an empty batch leaves the cursor exactly where it was', () => { + expect(pollResult([], 'kild', 9)).toEqual({ incoming: [], cursor: 9 }); + }); + + test('the cursor never goes backwards', () => { + // seq is the cursor precisely because it is monotonic; nothing here may undo that. + const { cursor } = pollResult([message(2, 'claude')], 'kild', 10); + expect(cursor).toBe(10); + }); +}); + +describe('watchSummary', () => { + test('names the sender and counts', () => { + expect(watchSummary([message(1, 'claude')])).toBe('1 new message from @claude'); + }); + + test('pluralises, dedupes senders, and keeps first-appearance order', () => { + const batch = [message(1, 'claude'), message(2, 'agent'), message(3, 'claude')]; + expect(watchSummary(batch)).toBe('3 new messages from @claude, @agent'); + }); +}); + +describe('exit codes', () => { + test('a quiet engine and a dead engine are different codes', () => { + // The whole interface for whatever runs this in the background. If these collided, a + // harness could not tell "nothing happened" from "I no longer know", which is the exact + // silent-failure shape this command exists to remove — and it is not retrofittable. + expect(WATCH_EXIT.quiet).not.toBe(WATCH_EXIT.unreachable); + }); + + test('only mail is success', () => { + expect(WATCH_EXIT.mail).toBe(0); + for (const code of [WATCH_EXIT.usage, WATCH_EXIT.quiet, WATCH_EXIT.unreachable]) { + expect(code).toBeGreaterThan(0); + } + }); + + test('every outcome has a distinct code', () => { + const codes = Object.values(WATCH_EXIT); + expect(new Set(codes).size).toBe(codes.length); + }); +}); diff --git a/engine/src/kild/watch.ts b/engine/src/kild/watch.ts new file mode 100644 index 00000000..4529b51a --- /dev/null +++ b/engine/src/kild/watch.ts @@ -0,0 +1,101 @@ +import type { Message } from './kild-types.ts'; + +/** + * Waiting for somebody else to speak — the wake path a turn-end hook cannot provide. + * + * The Stop hook fires when a turn ENDS. A harness sitting idle is therefore unreachable: mail + * queues, nothing wakes it, and it finds out whenever its operator next happens to prompt. + * kild cannot close that itself — it does not own an attached harness's process, which is the + * founding constraint of an attached agent — and no harness hook helps either, because every + * hook event fires on a turn, tool or session edge and none ticks while a session is idle. + * + * So kild owns the *waiting* and the harness owns the *waking*: this blocks until somebody + * else says something and then exits, and whatever background facility the harness has runs it + * and reacts to that exit. kild learns nothing about the harness; the harness needs to know + * nothing about kild's storage. + * + * **This reads the message LOG, never the inbox.** The log is non-destructive, so a watcher + * cannot eat the mail the Stop hook exists to deliver. `watch` is the wake signal; `inbox` is + * the delivery. Keeping those separate is what makes it safe to run one on a timer. + * + * It also means this needs nothing the engine does not already serve — a cursor, which + * `GET /api/kilds/:id/messages?since=` has always had. Watching answers "has anything new + * arrived?". It deliberately does NOT answer "is that agent sitting on undrained mail?", which + * is a question about delivery state and needs surface that does not exist yet. Conflating + * them would make this depend on machinery it does not need. + */ + +/** + * Timing policy lives HERE, not in `cli.ts`. + * + * `cli.ts` calls `dispatch()` at the top of the module, so a `const` declared further down that + * file is still in its temporal dead zone when the first command reaches it — the failure is + * `Cannot access 'X' before initialization`, and it takes out the whole verb. An imported + * module is fully evaluated before the importing module's body runs, so constants here cannot + * have that problem by construction. That is the class fix; the same bug in its + * function-declaration form has already been paid for once in this file's neighbour. + */ +/** How often to ask. Loopback and non-destructive, so this is a wake-latency budget rather + * than a load one. */ +export const WATCH_POLL_MS = 5_000; +/** Consecutive failed polls before the engine counts as gone rather than blipping. A restart + * takes a moment and must not be reported as a dead engine. */ +export const WATCH_TOLERATED_FAILURES = 3; +/** Default window. Long enough to be useful unattended, short enough that a forgotten watcher + * is not immortal. */ +export const WATCH_DEFAULT_TIMEOUT_S = 1_800; + +/** What one poll concluded. */ +export interface WatchPoll { + /** Messages from somebody other than the watcher, in arrival order. Empty means keep going. */ + incoming: Message[]; + /** Where to resume from. Advances past the watcher's OWN messages too, so its own sends + * are not re-examined on every poll — and can never wake it. */ + cursor: number; +} + +/** + * Decide what a batch of messages means for a watcher. + * + * Pure, because the interesting part is the filtering rule and the cursor arithmetic, not the + * HTTP around it. `handle` is excluded rather than `to` being matched: a watcher wants to know + * that the conversation moved, and a message addressed to someone else in the same kild is + * still a reason to look. Only its own voice is not. + */ +export function pollResult(messages: Message[], handle: string, cursor: number): WatchPoll { + return { + incoming: messages.filter((message) => message.from !== handle), + cursor: messages.reduce((furthest, message) => Math.max(furthest, message.seq), cursor), + }; +} + +/** A one-line summary of who is waiting, for the watcher's stdout. Senders are deduped and + * ordered by first appearance, so the line reads like the conversation did. */ +export function watchSummary(incoming: Message[]): string { + const senders: string[] = []; + for (const message of incoming) if (!senders.includes(message.from)) senders.push(message.from); + const who = senders.map((sender) => `@${sender}`).join(', '); + const count = incoming.length; + return `${count} new message${count === 1 ? '' : 's'} from ${who}`; +} + +/** + * Exit codes, which are the whole interface for whatever runs this in the background. + * + * A quiet engine and a DEAD engine must never look alike. If both exited 0, every harness built + * on `watch` would inherit the failure this command exists to remove: a wake mechanism that + * goes silent and is indistinguishable from one that simply had nothing to report. That is not + * retrofittable once harnesses depend on the behaviour, so it is decided here, first. + */ +export const WATCH_EXIT = { + /** Somebody spoke. The summary is on stdout. */ + mail: 0, + /** Usage, or no attachment to resolve — an ordinary loud CLI failure. */ + usage: 1, + /** The window elapsed with nothing new. The engine was reachable throughout; it was quiet. + * A caller that wants to keep waiting simply runs it again. */ + quiet: 2, + /** The engine stopped answering and did not come back. Distinct from `quiet` because the + * answer is "I no longer know", not "nothing happened". */ + unreachable: 3, +} as const; From 87f30945d787216ca89d00dabeead487929245c5 Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Tue, 28 Jul 2026 14:57:45 +0300 Subject: [PATCH 2/4] feat(cli): say that watch consumed nothing, rather than leaving it inferred MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The obvious wrong assumption about a wake verb is that it also marks-as-seen. A harness built on that would drop every message it was woken for and look entirely correct doing it — the wake fires, the harness treats it as delivery, and the mail sits in an inbox nobody drains. Non-destructiveness was in the docstring and the design, but not in anything a caller actually reads. Now the usage text, the human output and the --json shape all say it: 'nothing consumed — still in your inbox', consumed: false, and a pointer at `kild inbox` for the drain. Raised by helm's agent before wiring against it, which is the right time to settle a contract. --- engine/src/cli.attached.test.ts | 33 +++++++++++++++++++++++++++++++++ engine/src/cli.ts | 18 +++++++++++++++--- 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/engine/src/cli.attached.test.ts b/engine/src/cli.attached.test.ts index 64ce7533..98bbd894 100644 --- a/engine/src/cli.attached.test.ts +++ b/engine/src/cli.attached.test.ts @@ -528,3 +528,36 @@ test('watch with nothing to resolve is a usage error', async () => { expect(bare.exitCode).toBe(1); expect(bare.stderr).toContain('usage: kild watch'); }); + +test('watch states plainly that it consumed nothing', async () => { + // The obvious wrong assumption about a wake verb is that it also marks-as-seen. A harness + // built on that would drop every message it was woken for while looking correct, so the + // output has to say otherwise rather than leaving it to be inferred. + messagesResponse = { status: 200, body: [logMessage(5, 'claude')] }; + const watched = await runCli([ + 'watch', + 'kild-9', + '--as', + 'kild', + '--since', + '4', + '--timeout', + '3', + ]); + expect(watched.stdout).toContain('nothing consumed'); + expect(watched.stdout).toContain('kild inbox'); + + const asJson = await runCli([ + 'watch', + 'kild-9', + '--as', + 'kild', + '--since', + '4', + '--timeout', + '3', + '--json', + ]); + expect(JSON.parse(asJson.stdout)).toMatchObject({ consumed: false }); + messagesResponse = undefined; +}); diff --git a/engine/src/cli.ts b/engine/src/cli.ts index ad09fde9..58996e7f 100755 --- a/engine/src/cli.ts +++ b/engine/src/cli.ts @@ -359,7 +359,10 @@ async function kildWatch(idArg: string | undefined): Promise { if (!id || !handle) { throw new Error( 'usage: kild watch [ --as ] [--since ] [--timeout ]\n' + - ' omit id/--as to use the kild this session attached to', + ' omit id/--as to use the kild this session attached to\n' + + ' waits for somebody else to speak, then exits: 0 mail, 2 quiet, 3 engine unreachable\n' + + ' NON-DESTRUCTIVE — it reads the log and consumes nothing. Your mail is still\n' + + ' waiting afterwards; drain it with `kild inbox`.', ); } const timeout = values.timeout === undefined ? WATCH_DEFAULT_TIMEOUT_S : Number(values.timeout); @@ -406,10 +409,19 @@ async function kildWatch(idArg: string | undefined): Promise { const { incoming, cursor: next } = pollResult(batch, handle, cursor); cursor = next; if (incoming.length > 0) { + // Say plainly that nothing was consumed. The obvious wrong assumption about a wake verb + // is that it also marks-as-seen, and a harness built on that would drop every message it + // was woken for while looking entirely correct. console.log( json - ? JSON.stringify({ kildId: id, handle, since: cursor, messages: incoming }, null, 2) - : `${watchSummary(incoming)}\n read with: kild log ${id} --since ${cursor - incoming.length}`, + ? JSON.stringify( + { kildId: id, handle, since: cursor, consumed: false, messages: incoming }, + null, + 2, + ) + : `${watchSummary(incoming)} (nothing consumed — still in your inbox)\n` + + ` read with: kild log ${id} --since ${cursor - incoming.length}\n` + + ` drain with: kild inbox`, ); process.exit(WATCH_EXIT.mail); } From cda8e3b53f6a7cf0f1444d6a9491b4ca6ff598b1 Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Tue, 28 Jul 2026 15:02:58 +0300 Subject: [PATCH 3/4] fix(cli): give watch's failure tolerance the delay it promised MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review found the tolerance loop did not tolerate anything. A failed poll hit `continue`, which jumped past both the deadline check and the sleep — those only ran on the success path — so three "tolerated" failures burned in about eight milliseconds. The constant's own comment says a restart "takes a moment and must not be reported as a dead engine"; the code gave it no moment. Any real restart was declared unreachable, which is the tolerance inverted into exactly the bug it exists to prevent. The bootstrap fetch had the opposite half of the same problem: no tolerance at all, so one blip at startup — in the common no-`--since` shape — exited unreachable immediately. Two copies of one decision, drifted in opposite directions. There is now a single `poll()` both call sites use. Timing is the only thing that separates the fixed loop from the broken one, so the regression test asserts elapsed time, and `--interval` makes that testable without a ten-second test. It is a real knob besides: a harness that wants a snappier wake should not have to fork the CLI for it. Two smaller findings, both the "assert around the risky path" shape: - The `read with` hint was computed as `cursor - incoming.length`, which assumes the new messages are the highest-numbered in the batch. One own message with a higher seq made the printed command skip the very message that woke the watcher — a wrong answer with the confidence of a working command. It now derives from the oldest incoming seq. - `pollResult` filtered own messages but not `seq <= cursor`, and the test that fed it a stale seq asserted only the cursor, never `incoming`. `since` is documented exclusive, so it should not happen — but relying on that means one server-side slip re-delivers a seen message as fresh mail forever. One comparison makes the function correct on its own terms. Gates: 438 tests, typecheck, lint, e2e 70/70. Dogfooded: a dead engine now takes 837ms to declare unreachable rather than 8ms, and still exits 3. --- engine/src/cli.attached.test.ts | 49 +++++++++++++++++++- engine/src/cli.ts | 80 +++++++++++++++++++++++---------- engine/src/kild/watch.test.ts | 15 +++++-- engine/src/kild/watch.ts | 7 ++- 4 files changed, 122 insertions(+), 29 deletions(-) diff --git a/engine/src/cli.attached.test.ts b/engine/src/cli.attached.test.ts index 98bbd894..f2fdfb57 100644 --- a/engine/src/cli.attached.test.ts +++ b/engine/src/cli.attached.test.ts @@ -486,7 +486,7 @@ test('a quiet engine and a DEAD engine exit differently', async () => { // Nothing listening on this port at all. const dead = await runCli( - ['watch', 'kild-9', '--as', 'kild', '--since', '1', '--timeout', '30'], + ['watch', 'kild-9', '--as', 'kild', '--since', '1', '--timeout', '30', '--interval', '0.05'], 'http://127.0.0.1:1', ); expect(dead.exitCode).toBe(3); @@ -510,7 +510,7 @@ test('a dead engine is `unreachable` even with no --since to skip the first fetc // starting a watcher against a dead engine got exit 1 (usage) instead of 3 (unreachable), // which is exactly the distinction these codes exist to make. const dead = await runCli( - ['watch', 'kild-9', '--as', 'kild', '--timeout', '30'], + ['watch', 'kild-9', '--as', 'kild', '--timeout', '30', '--interval', '0.05'], 'http://127.0.0.1:1', ); expect(dead.exitCode).toBe(3); @@ -561,3 +561,48 @@ test('watch states plainly that it consumed nothing', async () => { expect(JSON.parse(asJson.stdout)).toMatchObject({ consumed: false }); messagesResponse = undefined; }); + +test('the "read with" hint reaches the message that woke the watcher', async () => { + // The hint used to be derived by arithmetic on the cursor, which assumed the new messages + // were the highest-numbered ones in the batch. One own-message with a higher seq made the + // printed command skip the very message it was suggesting you read. + messagesResponse = { + status: 200, + body: [logMessage(10, 'kild'), logMessage(11, 'claude'), logMessage(12, 'kild')], + }; + const watched = await runCli([ + 'watch', + 'kild-9', + '--as', + 'kild', + '--since', + '9', + '--timeout', + '3', + ]); + expect(watched.exitCode).toBe(0); + expect(watched.stdout).toContain('--since 10'); // one BELOW the waking message, not 11 + messagesResponse = undefined; +}); + +test('tolerated failures WAIT between attempts instead of spinning', async () => { + // The tolerance existed to give a restarting engine a moment. The retry used `continue`, + // which skipped the sleep entirely, so all three attempts burned in ~8ms and a restart was + // reported as a dead engine — the tolerance inverted into the bug it was meant to prevent. + // Timing is the only thing that distinguishes the fixed loop from the broken one. + const started = Date.now(); + const dead = await runCli( + ['watch', 'kild-9', '--as', 'kild', '--since', '1', '--timeout', '30', '--interval', '0.4'], + 'http://127.0.0.1:1', + ); + const elapsed = Date.now() - started; + expect(dead.exitCode).toBe(3); + // Three attempts means two gaps; anything under one gap means it spun. + expect(elapsed).toBeGreaterThan(700); +}, 10_000); + +test('a bad --interval is a loud usage error', async () => { + const bad = await runCli(['watch', 'kild-9', '--as', 'kild', '--interval', 'fast']); + expect(bad.exitCode).toBe(1); + expect(bad.stderr).toContain('--interval'); +}); diff --git a/engine/src/cli.ts b/engine/src/cli.ts index 58996e7f..c0f677e8 100755 --- a/engine/src/cli.ts +++ b/engine/src/cli.ts @@ -65,6 +65,7 @@ const { values, positionals } = parseArgs({ task: { type: 'string' }, // `kild spawn --task `: the new agent's first message session: { type: 'string' }, // `kild attach|inbox|send --session `: the harness session timeout: { type: 'string' }, // `kild watch --timeout `: how long to wait + interval: { type: 'string' }, // `kild watch --interval `: wake latency vs load }, }); @@ -358,7 +359,7 @@ async function kildWatch(idArg: string | undefined): Promise { const { kildId: id, handle } = await resolveAttachment(idArg, values.as); if (!id || !handle) { throw new Error( - 'usage: kild watch [ --as ] [--since ] [--timeout ]\n' + + 'usage: kild watch [ --as ] [--since ] [--timeout ] [--interval ]\n' + ' omit id/--as to use the kild this session attached to\n' + ' waits for somebody else to speak, then exits: 0 mail, 2 quiet, 3 engine unreachable\n' + ' NON-DESTRUCTIVE — it reads the log and consumes nothing. Your mail is still\n' + @@ -377,35 +378,63 @@ async function kildWatch(idArg: string | undefined): Promise { // to the top-level catch and exited 1 — a *usage* code — so a harness starting a watcher // against a dead engine could not tell "the engine is gone" from "you typed it wrong". That // is the distinction the exit codes exist for, defeated at the one moment it matters most. - let cursor: number; - const explicitSince = parseSince(); - if (explicitSince !== undefined) { - cursor = explicitSince; - } else { - try { - cursor = (await kildMessages(id)).at(-1)?.seq ?? 0; - } catch (err) { - console.error(`kild: engine unreachable: ${errText(err)}`); - process.exit(WATCH_EXIT.unreachable); - } + // Narrowing does not survive into a closure, so pin it once the guard above has run. + const kildId = id; + // Wake latency, and the gap between tolerated retries. Tunable because a harness that + // wants a snappier wake should not have to fork the CLI to get one. + const interval = values.interval === undefined ? WATCH_POLL_MS : Number(values.interval) * 1000; + if (!Number.isFinite(interval) || interval <= 0) { + throw new Error(`--interval must be a positive number of seconds (got ${values.interval})`); } const deadline = Date.now() + timeout * 1000; + const sleep = () => new Promise((resolve) => setTimeout(resolve, interval)); let failures = 0; - // Poll FIRST, then sleep: a watcher handed an explicit `--since` that is already behind the - // conversation reacts at once instead of after an interval of avoidable silence. - for (;;) { - let batch: Awaited>; + /** + * Ask once. Returns the batch, or null when the failure was tolerated and the caller should + * wait and try again; exits `unreachable` once tolerance runs out. + * + * ONE place that knows how much transient failure is acceptable, used by both the bootstrap + * fetch and the poll loop. They had separate copies of this decision and immediately drifted: + * the loop retried with no delay at all, so three "tolerated" failures burned in about eight + * milliseconds and a merely-restarting engine was declared dead — the exact opposite of what + * the tolerance exists for — while the bootstrap fetch tolerated nothing whatsoever. + */ + async function poll( + since: number | undefined, + ): Promise> | null> { try { - batch = await kildMessages(id, cursor); + const batch = await kildMessages(kildId, since); failures = 0; + return batch; } catch (err) { - // A restarting engine is not a gone one. Tolerate a few, then say so rather than - // waiting out the window and reporting silence we cannot vouch for. - if (++failures < WATCH_TOLERATED_FAILURES) continue; + if (++failures < WATCH_TOLERATED_FAILURES) return null; console.error(`kild: engine unreachable after ${failures} attempts: ${errText(err)}`); process.exit(WATCH_EXIT.unreachable); } + } + + // Default to the log's current end, so a watcher started now waits for what happens NEXT + // rather than firing immediately on the conversation it was started in the middle of. A + // blip here is tolerated exactly as one in the loop is — starting an instant before an + // engine finishes restarting must not be reported as an engine that is gone. + let cursor = parseSince(); + while (cursor === undefined) { + const batch = await poll(undefined); + if (batch) cursor = batch.at(-1)?.seq ?? 0; + else await sleep(); + } + + // Poll FIRST, then sleep: a watcher handed an explicit `--since` that is already behind the + // conversation reacts at once instead of after an interval of avoidable silence. + for (;;) { + const batch = await poll(cursor); + if (!batch) { + // Tolerated failure — wait like any other quiet interval rather than spinning. + if (Date.now() + interval >= deadline) break; + await sleep(); + continue; + } const { incoming, cursor: next } = pollResult(batch, handle, cursor); cursor = next; if (incoming.length > 0) { @@ -420,15 +449,20 @@ async function kildWatch(idArg: string | undefined): Promise { 2, ) : `${watchSummary(incoming)} (nothing consumed — still in your inbox)\n` + - ` read with: kild log ${id} --since ${cursor - incoming.length}\n` + + // Derived from the OLDEST incoming seq, not from arithmetic on the cursor. + // `cursor - incoming.length` assumed the new messages were the highest-numbered + // ones in the batch, so a single own-message with a higher seq made the printed + // command skip past the very message that woke the watcher — a wrong answer + // handed over with the confidence of a working command. + ` read with: kild log ${id} --since ${Math.min(...incoming.map((m) => m.seq)) - 1}\n` + ` drain with: kild inbox`, ); process.exit(WATCH_EXIT.mail); } // Stop when there is no room left for another poll, rather than sleeping past the // deadline and reporting a window longer than the one that was asked for. - if (Date.now() + WATCH_POLL_MS >= deadline) break; - await new Promise((resolve) => setTimeout(resolve, WATCH_POLL_MS)); + if (Date.now() + interval >= deadline) break; + await sleep(); } console.error(`kild: nothing new in ${timeout}s (cursor ${cursor})`); process.exit(WATCH_EXIT.quiet); diff --git a/engine/src/kild/watch.test.ts b/engine/src/kild/watch.test.ts index a37b4f98..9ff5df58 100644 --- a/engine/src/kild/watch.test.ts +++ b/engine/src/kild/watch.test.ts @@ -49,10 +49,19 @@ describe('pollResult', () => { expect(pollResult([], 'kild', 9)).toEqual({ incoming: [], cursor: 9 }); }); - test('the cursor never goes backwards', () => { - // seq is the cursor precisely because it is monotonic; nothing here may undo that. - const { cursor } = pollResult([message(2, 'claude')], 'kild', 10); + test('a message at or behind the cursor is neither fresh mail nor a cursor rewind', () => { + // `since` is documented exclusive, so this should never arrive — but if it ever does, + // reporting it as new wakes a watcher on already-seen content, repeatedly. Asserting only + // the cursor here left that half completely unconstrained. + const { incoming, cursor } = pollResult([message(2, 'claude')], 'kild', 10); expect(cursor).toBe(10); + expect(incoming).toEqual([]); + }); + + test('a batch straddling the cursor reports only what is genuinely new', () => { + const batch = [message(9, 'claude'), message(10, 'claude'), message(11, 'claude')]; + const { incoming } = pollResult(batch, 'kild', 10); + expect(incoming.map((m) => m.seq)).toEqual([11]); }); }); diff --git a/engine/src/kild/watch.ts b/engine/src/kild/watch.ts index 4529b51a..930a0dec 100644 --- a/engine/src/kild/watch.ts +++ b/engine/src/kild/watch.ts @@ -64,7 +64,12 @@ export interface WatchPoll { */ export function pollResult(messages: Message[], handle: string, cursor: number): WatchPoll { return { - incoming: messages.filter((message) => message.from !== handle), + // `seq <= cursor` is dropped as well as own messages. `since` is documented exclusive, so + // a message at or behind the cursor should never arrive — but relying on that means one + // server-side slip re-delivers an already-seen message as fresh mail and wakes a watcher + // on stale content, forever. The guard is one comparison and it makes this function + // correct on its own terms rather than on a promise made elsewhere. + incoming: messages.filter((message) => message.from !== handle && message.seq > cursor), cursor: messages.reduce((furthest, message) => Math.max(furthest, message.seq), cursor), }; } From 27b43d79a51618c8be2f4b0a5c44f880e04fab3a Mon Sep 17 00:00:00 2001 From: Rasmus Widing Date: Tue, 28 Jul 2026 15:11:46 +0300 Subject: [PATCH 4/4] fix(cli): one definition of "is there room for another poll" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Verification found the previous fix introduced a new bug of the same shape as the one it fixed. Extracting `poll()` unified how much transient failure is tolerated, but the DEADLINE check stayed inline at each call site — and only the loop's copy got it. The bootstrap fetch, which is the default no-`--since` path, would sleep a full interval on a blip with no regard for the window: a one-second request took six seconds, and the message then reported "nothing new in 1s" after six. One decision, two copies, one updated. Twice now. So the decision has one home. `waitForNextPoll()` answers "is there room for another attempt", and both loops ask it. `expire()` is the single quiet exit, and it reports the time that actually elapsed rather than the timeout that was requested — printing the request made the message a lie in exactly the case worth reporting. The helpers are function declarations rather than const arrows. TypeScript only treats a never-returning call as terminating when it can see the declaration, and hoisting keeps them clear of the temporal dead zone this file has been bitten by twice today. The coverage gap that let it through is closed: nothing asserted bootstrap timing at all, which is why "unify the failure decision" looked complete. There is now a test that fails if a blip during bootstrap overruns the window, and one that pins the elapsed-time message. Writing that second test also corrected me: a dead engine with a short interval exhausts tolerance BEFORE the window closes, so it exits `unreachable`, not `quiet`. My first version of the test asserted otherwise and was wrong; the code was right. Gates: 440 tests, typecheck, lint, e2e 70/70. Dogfooded the exact reported case — 1s window, 6s interval, dead engine — now 25ms instead of 6066ms. --- engine/src/cli.attached.test.ts | 39 +++++++++++++++++++++++++++++ engine/src/cli.ts | 44 ++++++++++++++++++++++++--------- 2 files changed, 72 insertions(+), 11 deletions(-) diff --git a/engine/src/cli.attached.test.ts b/engine/src/cli.attached.test.ts index f2fdfb57..52985ee6 100644 --- a/engine/src/cli.attached.test.ts +++ b/engine/src/cli.attached.test.ts @@ -606,3 +606,42 @@ test('a bad --interval is a loud usage error', async () => { expect(bad.exitCode).toBe(1); expect(bad.stderr).toContain('--interval'); }); + +test('the BOOTSTRAP path respects the window too, not just the poll loop', async () => { + // The fix for the spinning-retry bug unified failure *tolerance* into one helper but left + // the *deadline* check inline at both call sites, and only the loop's copy got it. So a + // single blip during the initial cursor fetch — the default, no-`--since` path — could + // sleep a full interval with no regard for the window: a 1s request took 6s. One decision, + // two copies, one updated. Exactly the shape of the bug it was fixing. + const started = Date.now(); + const overrun = await runCli( + ['watch', 'kild-9', '--as', 'kild', '--timeout', '1', '--interval', '6'], + 'http://127.0.0.1:1', + ); + const elapsed = Date.now() - started; + expect(overrun.exitCode).toBe(2); // quiet — the window closed, the engine was not declared dead + // A 1s window must not become a 6s one because the first fetch happened to fail. + expect(elapsed).toBeLessThan(3_000); +}, 15_000); + +test('the quiet message reports the time that actually elapsed', async () => { + // It printed the requested --timeout verbatim, which made it a lie in precisely the case + // worth reporting: the one where waiting overran the window. A REACHABLE but silent engine + // is what produces `quiet` — a dead one correctly produces `unreachable` instead. + messagesResponse = { status: 200, body: [] }; + const quiet = await runCli([ + 'watch', + 'kild-9', + '--as', + 'kild', + '--since', + '1', + '--timeout', + '1', + '--interval', + '0.2', + ]); + expect(quiet.exitCode).toBe(2); + expect(quiet.stderr).toMatch(/nothing new in \d+s/); + messagesResponse = undefined; +}); diff --git a/engine/src/cli.ts b/engine/src/cli.ts index c0f677e8..e4f9cf3e 100755 --- a/engine/src/cli.ts +++ b/engine/src/cli.ts @@ -386,8 +386,11 @@ async function kildWatch(idArg: string | undefined): Promise { if (!Number.isFinite(interval) || interval <= 0) { throw new Error(`--interval must be a positive number of seconds (got ${values.interval})`); } - const deadline = Date.now() + timeout * 1000; - const sleep = () => new Promise((resolve) => setTimeout(resolve, interval)); + const started = Date.now(); + const deadline = started + timeout * 1000; + function sleep(): Promise { + return new Promise((resolve) => setTimeout(resolve, interval)); + } let failures = 0; /** @@ -414,6 +417,29 @@ async function kildWatch(idArg: string | undefined): Promise { } } + /** + * Wait for the next poll, or report that the window has closed. + * + * The ONE definition of "is there room for another attempt", because there were two and one + * of them was wrong — twice. First the failure path skipped the wait entirely; then the fix + * unified failure *tolerance* into `poll()` but left the *deadline* inline at each call + * site, so the bootstrap fetch had no deadline check at all and a single blip could overrun + * a one-second window by six seconds. Both bugs were the same shape: one decision, two + * copies, one of them updated. + */ + async function waitForNextPoll(): Promise { + if (Date.now() + interval >= deadline) return false; + await sleep(); + return true; + } + + function expire(): never { + // Report the window that actually elapsed. Printing the requested `--timeout` made the + // message a lie in exactly the case worth reporting — the one where waiting overran it. + console.error(`kild: nothing new in ${Math.round((Date.now() - started) / 1000)}s`); + process.exit(WATCH_EXIT.quiet); + } + // Default to the log's current end, so a watcher started now waits for what happens NEXT // rather than firing immediately on the conversation it was started in the middle of. A // blip here is tolerated exactly as one in the loop is — starting an instant before an @@ -422,7 +448,7 @@ async function kildWatch(idArg: string | undefined): Promise { while (cursor === undefined) { const batch = await poll(undefined); if (batch) cursor = batch.at(-1)?.seq ?? 0; - else await sleep(); + else if (!(await waitForNextPoll())) expire(); } // Poll FIRST, then sleep: a watcher handed an explicit `--since` that is already behind the @@ -431,8 +457,7 @@ async function kildWatch(idArg: string | undefined): Promise { const batch = await poll(cursor); if (!batch) { // Tolerated failure — wait like any other quiet interval rather than spinning. - if (Date.now() + interval >= deadline) break; - await sleep(); + if (!(await waitForNextPoll())) break; continue; } const { incoming, cursor: next } = pollResult(batch, handle, cursor); @@ -459,13 +484,10 @@ async function kildWatch(idArg: string | undefined): Promise { ); process.exit(WATCH_EXIT.mail); } - // Stop when there is no room left for another poll, rather than sleeping past the - // deadline and reporting a window longer than the one that was asked for. - if (Date.now() + interval >= deadline) break; - await sleep(); + // Same single definition as the failure path uses: no room for another attempt ends it. + if (!(await waitForNextPoll())) break; } - console.error(`kild: nothing new in ${timeout}s (cursor ${cursor})`); - process.exit(WATCH_EXIT.quiet); + expire(); } async function kildInbox(idArg: string | undefined): Promise {