From 4dbfc46b7de6c3c9ef5888b6c16494714fc5ecb6 Mon Sep 17 00:00:00 2001 From: Nicolas Dumazet Date: Tue, 18 Aug 2026 10:41:27 +0200 Subject: [PATCH 1/2] feat(pi): signal herdr:blocked while a revdiff review owns the terminal --- plugins/pi/extensions/revdiff.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/plugins/pi/extensions/revdiff.ts b/plugins/pi/extensions/revdiff.ts index 0d237d64..fcf59895 100644 --- a/plugins/pi/extensions/revdiff.ts +++ b/plugins/pi/extensions/revdiff.ts @@ -95,7 +95,19 @@ export default function revdiffExtension(pi: ExtensionAPI): void { } onUpdate?.({ content: [{ type: "text", text: `Launching revdiff for ${launch.label} in ${cwd}...` }], details: null }); - const result = await runDirectReview(ctx, launch, cwd); + // While revdiff owns the terminal (spawnSync stdio:inherit in runDirectReview), + // pi's agent loop is parked inside this tool call, so terminal multiplexers + // watching the agent see "working" even though the session is waiting on the + // user. Signal blocked on pi's shared event bus for the duration of the + // review; herdr's omp/pi integration listens for `herdr:blocked` and drives + // its sidebar/notifications from it. Emitting is free when nothing listens. + pi.events.emit("herdr:blocked", { active: true, label: `revdiff review: ${launch.label}` }); + let result: ReviewResult | undefined; + try { + result = await runDirectReview(ctx, launch, cwd); + } finally { + pi.events.emit("herdr:blocked", { active: false }); + } if (!result) { return toolTextResult("revdiff review did not complete."); } From 6491d0522e446c0fe2a874612c88e2d2380666df Mon Sep 17 00:00:00 2001 From: Nicolas Dumazet Date: Tue, 18 Aug 2026 21:50:36 +0200 Subject: [PATCH 2/2] fix(pi): flush blocked signal before handoff and gate on HERDR_ENV Addresses maintainer feedback on #319: - extend the blocked window to wrap resolveLaunchSpec too, so the ui.select prompt on a dirty branch is covered, not just runDirectReview; a single try/finally guarantees active:false fires exactly once on every exit path, including the early !launch return - yield two macrotask ticks (flushPendingIOBestEffort) before the spawnSync handoff so herdr's queued socket write gets a turn to run before the loop freezes; best-effort, no delivery acknowledgement exists to make this a guarantee - gate the emit and the flush on HERDR_ENV=1, matching every other herdr touchpoint in this repo, so this is a genuine no-op outside herdr rather than just harmless-because-unobserved - fix fakePi() in the regression test harness: it had no events stub, so revdiff_review.execute threw a TypeError the moment it tried to emit; add coverage for the blocked/unblocked pairing across a completed review, a launch-resolution failure, an invalid cwd, and the HERDR_ENV-unset no-op case --- app/plugin_exit_code_test.go | 131 +++++++++++++++++++++++++++++++ plugins/pi/extensions/revdiff.ts | 72 ++++++++++------- 2 files changed, 175 insertions(+), 28 deletions(-) diff --git a/app/plugin_exit_code_test.go b/app/plugin_exit_code_test.go index 576c7dc4..d941e588 100644 --- a/app/plugin_exit_code_test.go +++ b/app/plugin_exit_code_test.go @@ -882,10 +882,12 @@ function fakePi() { const commands = new Map(); const tools = new Map(); const sentMessages: string[] = []; + const emittedEvents: Array<{ type: string; data: unknown }> = []; return { commands, tools, sentMessages, + emittedEvents, registerCommand(name: string, command: any) { commands.set(name, command); }, @@ -895,6 +897,14 @@ function fakePi() { sendUserMessage(message: string) { sentMessages.push(message); }, + events: { + emit(type: string, data: unknown) { + emittedEvents.push({ type, data }); + }, + on(_type: string, _handler: (data: unknown) => void) { + return () => {}; + }, + }, } as any; } @@ -1116,6 +1126,123 @@ async function testArgumentResolution(): Promise { assertArray(shellSplit(shellJoin(roundTrip)), roundTrip, "shellJoin output should shellSplit back to original args"); } +async function withHerdrEnv(fn: () => Promise): Promise { + const old = process.env.HERDR_ENV; + process.env.HERDR_ENV = "1"; + try { + return await fn(); + } finally { + if (old === undefined) { + delete process.env.HERDR_ENV; + } else { + process.env.HERDR_ENV = old; + } + } +} + +async function testHerdrBlockedSignalBracketsReview(): Promise { + const tempDir = mkdtempSync(path.join(tmpdir(), "pi-revdiff-blocked-")); + const fakeBin = path.join(tempDir, "revdiff"); + const argFile = path.join(tempDir, "args.txt"); + writeExecutable(fakeBin, fakeRevdiffScript()); + + const oldBin = process.env.REVDIFF_BIN; + const oldArgFile = process.env.FAKE_ARG_FILE; + process.env.REVDIFF_BIN = fakeBin; + process.env.FAKE_ARG_FILE = argFile; + try { + await withHerdrEnv(async () => { + const pi = fakePi(); + revdiffExtension(pi); + await pi.tools.get("revdiff_review").execute("call-1", { args: "--only README.md" }, undefined, undefined, fakeCtx()); + + testAssert(pi.emittedEvents.length === 2, "expected exactly one blocked/unblocked pair for a completed review"); + testAssert(pi.emittedEvents[0].type === "herdr:blocked", "first event should be herdr:blocked"); + testAssert((pi.emittedEvents[0].data as any).active === true, "review start should signal active: true"); + testAssert(pi.emittedEvents[1].type === "herdr:blocked", "second event should be herdr:blocked"); + testAssert((pi.emittedEvents[1].data as any).active === false, "review end should signal active: false"); + }); + } finally { + if (oldBin === undefined) { + delete process.env.REVDIFF_BIN; + } else { + process.env.REVDIFF_BIN = oldBin; + } + if (oldArgFile === undefined) { + delete process.env.FAKE_ARG_FILE; + } else { + process.env.FAKE_ARG_FILE = oldArgFile; + } + rmSync(tempDir, { recursive: true, force: true }); + } +} + +async function testHerdrBlockedSignalSkippedWithoutHerdrEnv(): Promise { + const tempDir = mkdtempSync(path.join(tmpdir(), "pi-revdiff-blocked-off-")); + const fakeBin = path.join(tempDir, "revdiff"); + const argFile = path.join(tempDir, "args.txt"); + writeExecutable(fakeBin, fakeRevdiffScript()); + + const oldBin = process.env.REVDIFF_BIN; + const oldArgFile = process.env.FAKE_ARG_FILE; + const oldHerdrEnv = process.env.HERDR_ENV; + process.env.REVDIFF_BIN = fakeBin; + process.env.FAKE_ARG_FILE = argFile; + delete process.env.HERDR_ENV; + try { + const pi = fakePi(); + revdiffExtension(pi); + await pi.tools.get("revdiff_review").execute("call-1", { args: "--only README.md" }, undefined, undefined, fakeCtx()); + testAssert(pi.emittedEvents.length === 0, "a completed review should emit nothing when HERDR_ENV is unset"); + } finally { + if (oldBin === undefined) { + delete process.env.REVDIFF_BIN; + } else { + process.env.REVDIFF_BIN = oldBin; + } + if (oldArgFile === undefined) { + delete process.env.FAKE_ARG_FILE; + } else { + process.env.FAKE_ARG_FILE = oldArgFile; + } + if (oldHerdrEnv === undefined) { + delete process.env.HERDR_ENV; + } else { + process.env.HERDR_ENV = oldHerdrEnv; + } + rmSync(tempDir, { recursive: true, force: true }); + } +} + +async function testHerdrBlockedSignalFiresOnceWhenLaunchUnresolved(): Promise { + await withHerdrEnv(async () => { + const pi = fakePi(); + revdiffExtension(pi); + const result = await pi.tools.get("revdiff_review").execute("call-1", { args: "--output" }, undefined, undefined, fakeCtx()); + + testAssert(result.content[0].text === "Could not resolve a revdiff launch target.", "expected the early launch-resolution exit"); + testAssert(pi.emittedEvents.length === 2, "expected exactly one blocked/unblocked pair even when launch resolution fails"); + testAssert((pi.emittedEvents[0].data as any).active === true, "unresolved launch should still have signaled active: true first"); + testAssert((pi.emittedEvents[1].data as any).active === false, "unresolved launch should still signal active: false exactly once"); + }); +} + +async function testHerdrBlockedSignalSkippedForInvalidCwd(): Promise { + const tempDir = mkdtempSync(path.join(tmpdir(), "pi-revdiff-blocked-cwd-")); + const file = path.join(tempDir, "not-a-dir.txt"); + testWriteFileSync(file, "not a directory\n"); + try { + await withHerdrEnv(async () => { + const pi = fakePi(); + revdiffExtension(pi); + await pi.tools.get("revdiff_review").execute("call-1", { cwd: file }, undefined, undefined, fakeCtx()); + testAssert(pi.emittedEvents.length === 0, "an unresolved cwd should exit before any blocked signal is emitted"); + }); + } finally { + rmSync(tempDir, { recursive: true, force: true }); + } +} + async function testRefLikePathArgKeepsRef(): Promise { const oldCwd = process.cwd(); const repo = initGitRepo(); @@ -1214,6 +1341,10 @@ await testCommandRoutesToSkill(); await testToolReturnsAnnotations(); testReviewCwdResolution(); await testToolRejectsInvalidCwd(); +await testHerdrBlockedSignalBracketsReview(); +await testHerdrBlockedSignalSkippedWithoutHerdrEnv(); +await testHerdrBlockedSignalFiresOnceWhenLaunchUnresolved(); +await testHerdrBlockedSignalSkippedForInvalidCwd(); await testToolCwdParameter(); await testSignalTerminatedReviewFails(); await testArgumentResolution(); diff --git a/plugins/pi/extensions/revdiff.ts b/plugins/pi/extensions/revdiff.ts index fcf59895..4177d843 100644 --- a/plugins/pi/extensions/revdiff.ts +++ b/plugins/pi/extensions/revdiff.ts @@ -89,42 +89,58 @@ export default function revdiffExtension(pi: ExtensionAPI): void { return toolTextResult("Could not resolve revdiff working directory."); } - const launch = await resolveLaunchSpec(params.args?.trim() ?? "", ctx, cwd); - if (!launch) { - return toolTextResult("Could not resolve a revdiff launch target."); + // This call may wait on the user (the ui.select prompt below, or + // spawnSync stdio:inherit in runDirectReview) while pi's agent loop is + // parked inside this tool call, so terminal multiplexers watching the + // agent see "working" even though the session is waiting on the user. + // Signal blocked on pi's shared event bus for the whole window; + // herdr's omp/pi integration listens for `herdr:blocked` and drives + // its sidebar/notifications from it. active:false fires exactly once, + // on every exit path. + const herdrActive = process.env.HERDR_ENV === "1"; + if (herdrActive) { + pi.events.emit("herdr:blocked", { active: true, label: `revdiff review in ${cwd}` }); } - - onUpdate?.({ content: [{ type: "text", text: `Launching revdiff for ${launch.label} in ${cwd}...` }], details: null }); - // While revdiff owns the terminal (spawnSync stdio:inherit in runDirectReview), - // pi's agent loop is parked inside this tool call, so terminal multiplexers - // watching the agent see "working" even though the session is waiting on the - // user. Signal blocked on pi's shared event bus for the duration of the - // review; herdr's omp/pi integration listens for `herdr:blocked` and drives - // its sidebar/notifications from it. Emitting is free when nothing listens. - pi.events.emit("herdr:blocked", { active: true, label: `revdiff review: ${launch.label}` }); - let result: ReviewResult | undefined; try { - result = await runDirectReview(ctx, launch, cwd); + const launch = await resolveLaunchSpec(params.args?.trim() ?? "", ctx, cwd); + if (!launch) { + return toolTextResult("Could not resolve a revdiff launch target."); + } + + onUpdate?.({ content: [{ type: "text", text: `Launching revdiff for ${launch.label} in ${cwd}...` }], details: null }); + if (herdrActive) { + // Give the socket write queued by the emit above a couple of + // macrotask ticks to run before spawnSync freezes the loop. + await flushPendingIOBestEffort(); + } + const result = await runDirectReview(ctx, launch, cwd); + if (!result) { + return toolTextResult("revdiff review did not complete."); + } + + if (result.annotations.length === 0) { + return toolTextResult(`Review complete — no annotations for ${result.label}.`, result); + } + + const noun = result.annotations.length === 1 ? "annotation" : "annotations"; + return toolTextResult( + [`Captured ${result.annotations.length} ${noun} for ${result.label}.`, "", "Annotations:", result.rawOutput.trim()].join("\n"), + result, + ); } finally { - pi.events.emit("herdr:blocked", { active: false }); - } - if (!result) { - return toolTextResult("revdiff review did not complete."); - } - - if (result.annotations.length === 0) { - return toolTextResult(`Review complete — no annotations for ${result.label}.`, result); + if (herdrActive) { + pi.events.emit("herdr:blocked", { active: false }); + } } - - const noun = result.annotations.length === 1 ? "annotation" : "annotations"; - return toolTextResult( - [`Captured ${result.annotations.length} ${noun} for ${result.label}.`, "", "Annotations:", result.rawOutput.trim()].join("\n"), - result, - ); }, }); } +async function flushPendingIOBestEffort(): Promise { + await new Promise((resolve) => setImmediate(resolve)); + await new Promise((resolve) => setImmediate(resolve)); +} + function toolTextResult(content: string, details?: ReviewResult) { return { content: [{ type: "text" as const, text: content }], details: details ?? null }; }