feat(pi): signal herdr:blocked while a revdiff review owns the terminal

[nicdumz]

Title: feat(pi): signal herdr:blocked while a revdiff review owns the terminal

Problem

revdiff_review runs revdiff in-place via spawnSync(..., { stdio: "inherit" }), parking pi's agent loop inside the tool call for the whole review. External supervisors that consume pi lifecycle state (herdr's omp/pi integration, which replaces screen detection for the pane) report working the entire time — no blocked state, no notification, agent wait --until blocked never fires. Fixes #318. Refs herdrdev/herdr#2758 for the same blind spot with hook confirmation dialogs.

Change

Emit herdr:blocked on pi's shared extension event bus around runDirectReview: { active: true, label } before, { active: false } in a finally. herdr's installed omp/pi integration listens for this channel and drives sidebar/notifications/waits from it; EventBus.emit with no subscriber is a no-op, so this costs nothing when herdr isn't involved. 13 lines, no signature changes.

(Disclosure: prepared by an AI coding agent on behalf of the human author, who reviewed and approved this change and text before submission.)

[umputun]

the diagnosis is right and the problem is real, but the signal can't arrive while the review is open, so this doesn't do what it says yet.

blocking: the blocked state can't reach herdr during the review. The emit at plugins/pi/extensions/revdiff.ts:104 and the spawnSync inside runDirectReview are separated by zero event-loop turns: runDirectReview runs synchronously into ctx.ui.custom(factory), and pi calls that factory inside its new Promise executor, where tui.stop() and spawnSync run. On herdr's side the listener is invoked synchronously but the transport is not: herdr:blocked goes to publishState -> queueState -> drainStateQueue -> sendRequestAttempt, which does net.createConnection and only writes from the "connect" callback. That callback needs a loop turn spawnSync denies for the whole review, so herdr gets blocked and then working back to back after revdiff has already exited. Sidebar, notification and agent wait --until blocked all still miss. Fix: the state has to be flushed before the terminal handoff, or the review has to stop blocking the loop (an async spawn awaited with the loop live). That is a design call against pi's TUI teardown, not a line edit.

blocking: this breaks the pi regression test. The harness's fakePi() in app/plugin_exit_code_test.go:881 has no events, so revdiff_review.execute throws TypeError: Cannot read properties of undefined (reading 'emit') and TestPiExtensionExecutableBehavior fails. It skips locally when bun is absent, which is why you wouldn't have seen it, and CI on this PR hasn't run yet. Fix: add an events: { emit(){}, on(){ return () => {}; } } stub to fakePi(), and assert the active:true / active:false pair brackets the review.

plugins/pi/extensions/revdiff.ts:92 - the blocked window opens after resolveLaunchSpec, so the ctx.ui.select prompt it can reach is not covered and a bare /revdiff on a dirty branch still reports working while waiting on the user. This is the one part that would work today, since the loop is live there. Fix: cover every await that waits on the user, and keep active:false firing exactly once on each exit path including the early !launch return.

worth checking before you rework this here: herdr's own omp integration already handles tool_execution_start / tool_execution_end, gated on toolName !== "ask". Adding revdiff_review to that gate would get the same state from herdr's side with no change in revdiff, assuming it clears the same timing problem.

on the design: every herdr touchpoint in this repo is gated on HERDR_ENV and this emit is unconditional. I know it is inert without a subscriber, but I want to settle whether revdiff should own a channel name another project versions before taking it.

per CONTRIBUTING.md a first PR needs an issue and a maintainer reply before code. Issue #318 went up two minutes before this one, so the design was never agreed.

[nicdumz]

Adding revdiff_review to that gate would get the same state from herdr's side with no change in revdiff, assuming it clears the same timing problem.

I main have misunderstood but it seems awkward to, in herdr itself, add special-casing for a specific extension. As in, a little bit backwards: I would expect the extension to integrate well with herdr.
I also do not think that this is a way to circumvent the timing issues you mention as the mechanism to mark the window blocked seemed to have the same flaws.

@umputun I've tried adding a flushPendingIOBestEffort() call, which in theory does not entirely guarantee waking up at the right time but at least as far as local testing goes this seems to do the job. I don't love that this isn't "guaranteed" but perhaps this is a simple small scoped change.

(The downside of making the review not synchronous is... having to deal with potential async events doing other things in the meantime, i'm not sure i'd be equipped to fix that)

[umputun]

the test breakage and the select-prompt gap are fixed, and the HERDR_ENV gate matches what the launchers already do. Before another round on the timing, two questions decide whether any of this belongs in revdiff at all.

the channel name. the gate bounds when herdr:blocked is emitted, not who owns it. The string and the {active,label} shape are herdr's contract, hardcoded here with no version tag and nothing to check against. Every other herdr touchpoint in this repo calls the CLI and reads the result back, plugins/revdiff-planning/scripts/launch-plan-review.sh:178 errors on a nonzero exit or a missing pane id. A fire-and-forget emit has no result. If herdr renames the event or reshapes the payload, this repo stays green and the feature is dead with nothing on either side noticing.

whether the emit is needed here at all. I checked the package: tool_execution_start and tool_execution_end are both on ExtensionAPI in pi 0.74.0, which is the floor package.json declares, and ^0.74.0 on a 0.x package is also its ceiling. The emit window is the tool-execution window, it opens right after resolveReviewCwd and closes as execute() returns. So the version here buys the label string, and costs a foreign versioned contract plus a timing heuristic I have to keep working, against one entry in herdr's toolName !== "ask" gate. Is that gate something you can get changed?

if it is not, then the rest stands.

blocking: two setImmediate turns is the exact minimum, with nothing spare. net.createConnection defers the pipe connect to a nextTick, and libuv completes the connect_req into the pending queue, so afterConnect, the only place the write happens, runs in the pending-callbacks phase of the next loop iteration. The first setImmediate reaches check of iteration N. Its continuation queues the second one, which is deferred to N+1, and N+1's pending phase is where the write finally goes out. So on a unix socket with an empty queue it just makes it. Anything that costs one more turn does not, and drainStateQueue sitting behind an in-flight prior send is exactly that: second /revdiff in a session, and this state is queued behind the first one's socket. Nothing checks the outcome either, runDirectReview walks into spawnSync regardless.

this is one of the two routes I named last round, so the approach is not the problem. Two turns is probable-before-handoff, not observable-before-handoff, and when it misses the symptom is #318 coming back silently and reading as environmental.

the flush has no test. delete flushPendingIOBestEffort and its two call lines and all four new tests still pass, so the entire delta of 6491d05 over 4dbfc46 is uncovered apart from the gate. It is testable here without touching the shared fakes: fakeCtx().ui.custom runs the factory synchronously and resolveLaunchSpec yields no macrotask when args are passed, so with the flush removed there is no macrotask boundary between the emit and the spawn at all. Wrap pi.events.emit on the fake so it does setImmediate(() => order.push("flushed")) before delegating, wrap ctx.ui.custom on a local fakeCtx() to push "spawn", then assert order[0] === "flushed".

app/plugin_exit_code_test.go:1143 also never looks at what execute returned, so it does not prove a review ran at all. testToolReturnsAnnotations:966 asserts Captured 1 annotation for README.md. on the same args, the same assert belongs here.

nits, none blocking:

• plugins/pi/extensions/revdiff.ts:92 and :112, both comment blocks start capitalized. Everything else in the file is lowercase (:287, :325, :610). And :96 and :98-99 restate the code right below them; the parked-loop reason and the line naming who listens are the parts worth keeping.
• nothing documents that HERDR_ENV=1 now changes behavior inside pi. README's only mention of it (:88) is the launcher's overlay-backend table, which is a different mechanism. One bullet in the pi package notes covers it.
• if the two-tick flush survives in any form, say in the comment that it can miss and what happens when it does. Right now nothing tells the next reader that a silent regression to pi extension: herdr never shows the agent as blocked while a revdiff review is open #318 is a live outcome, or why the constant is 2.

[nicdumz]

@umputun herdrdev/herdr@ca4270b#diff-82789c89eb0b273638986575b634c01addd0d47bb32503217c47949ddb22eeeb is the commit which added the Herder<>Pi integration.

You can see there that src/integration/assets/pi/herdr-agent-state.ts wants to be the authoritative state of wether or not a session is blocked. We need to find a way to update this state, otherwise we'd be fighting herdr's own Pi integration.

There is Rust code in the herdr binary / server about state, and the Pi Herdr extension communicates with herdr via pane.report_agent.

It's really important to notice that nothing in that original PR emits herdr:blocked. This makes this event an obvious extension point for Pi extension writers, this is what this Pi event is and why herdr-agent-state listens on those events. https://github.com/search?q=herdr%3Ablocked+path%3A*.ts&type=code shows 140+ Pi extensions using this extension point.

Can we settle on this being the right approach? It seems the past review turns are "doubting" that herdr:blocked is a legitimate extension point. Is that the problem? Should we rope in @ogulcancelik for confirmation here before more review turns?

[nicdumz]

against one entry in herdr's toolName !== "ask" gate. Is that gate something you can get changed?

I understand this suggestion as changing https://github.com/herdrdev/herdr/blob/ffc4e263168f9e81d5bbc14db4b16ca9818d684a/src/integration/assets/omp/herdr-agent-state.ts#L416

from:

    if (event?.toolName !== "ask") {
      return;
    }

to some:

    if (event?.toolName !== "ask" || event?.toolName !== "revdiff_review") {
      return;
    }

To me, changing the main herdr codebase to be aware of the revdiff tool, an optional extension, seems like the wrong change. I don't think this is the clean fix and I doubt that herdr maintainers would accept this kind of hardcoding (if we think about it this means that any of the thousands of extension authors could then ask special-casing, this doesnt scale).

[umputun]

you're right on both design points.

herdr-agent-state.ts:207 listens for herdr:blocked, reads label, toggles on active, keeps a blockedCount for nesting, and emits nothing itself. It's herdr's own published extension point. Objection dropped.

enumerating consumers in herdr doesn't scale either. Won't ask for that.

timing still blocks, and it's worse than a tight tick count. I went looking for a synchronous send, since herdr's cursor/droid/kimi hooks do a blocking connect/sendall/recv from python3. Closed here three ways: Node has no sync unix-socket client, so it means spawnSync on the handoff path; pane.report_agent goes through accept_hook_report (src/terminal/state.rs:1652) whose no-seq branch is return !self.hook_report_sequences.contains_key(source), so an unsequenced report from herdr:pi is dropped once that source has sent a sequenced one; and inventing a seq permanently stales the real extension, which seeds reportSeq = Date.now() * 1000.

that leaves an ack on herdr:blocked. Emitter gets a promise, herdr's listener resolves it once drainStateQueue has the RPC response, revdiff awaits with a timeout and then spawns. execute() is already async, so only delivery becomes observable and the review stays synchronous. Seq, session refs and nesting stay in herdr. Optional in the payload, so the 374 existing producers are unaffected.

that one is generic, which is your own objection satisfied. If you want a herdr maintainer involved, that's the question to put to them.

I'm not merging the two-tick flush meanwhile. It's probable-before-handoff, not observable, and when it misses #318 returns silently and reads as environmental. Worse than the bug because it looks fixed. Still untested too: delete flushPendingIOBestEffort and its two call lines, all four tests pass.

happy to leave this open while the herdr side is settled. If the ack is accepted there, this PR shrinks to a bounded wait plus a delivery-order test and a minimum-version note. If herdr declines, say so and I'll close without prejudice, #318 keeps tracking it.

and the HERDR_ENV=1 doc note from my last round is mine to write, not yours.