feat(launcher): herdr pane-scoped overlay (REVDIFF_HERDR_PANE=1) - #344
feat(launcher): herdr pane-scoped overlay (REVDIFF_HERDR_PANE=1)#344rayslava wants to merge 1 commit into
Conversation
under herdr a review replaces the whole tab view, so the agent pane that asked for it disappears for as long as the review lasts. herdr's working layout is an agent in one pane and you beside it, but the backend only ever calls `herdr tab create` + `pane run`, and the agent's pane holds the context the diff has to be judged against. `REVDIFF_AGTERM_PANE=1` already exists for the same reason on agterm, which has the same layout model. `REVDIFF_HERDR_PANE=1` splits the caller's own pane and zooms it, so the review still gets the full tab area but the agent pane is one keypress away. it copies the shape agterm settled on: env-var opt-in, capability probe before passing a new flag, quiet degradation to the existing overlay. unset, the `tab create` block is reached byte-identical, because pane mode is a self-contained block ahead of it rather than a flag threaded through the shared path. two rules worth knowing when editing it, both recorded in CLAUDE.md: a split that returns no usable pane id warns and exits rather than guessing which pane to close, and `pane_not_found` is treated as authoritative death while any other error is transient.
umputun
left a comment
There was a problem hiding this comment.
shape is right and it follows the REVDIFF_AGTERM_PANE pattern from #303 closely. One blocking issue and two smaller ones.
blocking: launch-revdiff.sh:364-383 can close a live review
the ten-miss path closes a pane whose state is unknown. Ten consecutive generic failures take about 2.7s plus the CLI calls, since misses 1-9 sleep and miss 10 breaks before the sleep. If the sentinel is still absent, read_rc returns 1, herdr_close_pane closes the target, and print_output_and_exit 1 never relays the control-plane error sitting in HERDR_GET. A short socket or server interruption can end a live review before the user is done, and the driving agent sees an ordinary launcher failure, so the timeout recovery in SKILL.md never runs.
a generic error is not evidence the pane died, and the pane_not_found branch two lines above already gets that right by refusing to close a pane it knows is gone. My suggestion is to keep polling until the sentinel appears or pane get returns authoritative pane_not_found. Warn once and back off after a threshold if you want the visibility, but there is no safe automatic deadline when the API cannot report liveness, and the tab path below already blocks unbounded for a human review.
neither bounded alternative holds up. Raising the bound still turns unknown liveness into destruction, just less often. Un-zooming instead of closing is better but incomplete, since it still returns exit 1 and so still bypasses SKILL.md recovery, and the EXIT trap closes the pane anyway unless ownership moves with it.
app/plugin_exit_code_test.go:508 currently makes the early close expected, with wantClose: true at :514. That case wants to model recovery instead: more than ten generic failures, then a sentinel, asserting the annotation exit code and output with a single close after completion.
launch-revdiff.sh:330 - the EXIT trap and SKILL.md disagree
the EXIT trap runs on a signal-delivered exit and calls herdr_close_pane, so a harness that kills the launcher on timeout kills the review with it. SKILL.md:155 promises the opposite to the driving agent, that "only the launcher process died, but revdiff itself is still open in the overlay", and tells it to recover the output or durable history. The herdr tab path follows that contract, its trap only removes temp files and tab close stays on the completion path at :438.
Claude Code keeps the launcher running past its timeout, so this reaches the harnesses that kill it outright, which SKILL.md explicitly covers. The pane path should either match that contract or document an honest exception. I do see why the trap is there, a zoomed pane stranded over someone's work is its own bad outcome.
app/plugin_exit_code_test.go does not assert the zoom
deleting herdr pane zoom "$HERDR_TARGET" --on from :358 leaves all 16 subtests green, although the zoom is the behaviour this mode advertises. The fake already records the call, so it needs a wantZoom field asserted on the four cases whose split returns a usable pane id:
assert.Equal(t, 1, countHerdrCalls(calls, "pane zoom "+fakeHerdrPaneID+" --on"), "the split must be zoomed; calls=%v", calls)a bare wantSplit assertion is too broad: the refused split and the caller-id response correctly do not zoom, in both launcher copies. The case named opt-in splits and zooms the caller pane is off too, the caller pane is the split source and the new review pane is the zoom target.
the rest of the integration checks out against herdr 0.8.2: pane split returns the new id at .result.pane.pane_id, pane get and pane close take positional ids, and pane_not_found is a real error code. Both launcher copies parse under bash 3.2, and no version bump is right for a launcher-only change. The -timeout=100s to 180s correction in CLAUDE.md matches the Makefile and CI.
What is the problem?
Under herdr a review replaces the whole tab view, so the agent pane that asked for it disappears for as long as the review lasts. herdr's working layout is an agent in one pane and you beside it, but the backend only ever calls
herdr tab create+pane run. That pane holds the context the diff has to be judged against — what the agent said it was doing, the test output, the error it was fixing — and checking any of it means leaving the review and coming back.REVDIFF_AGTERM_PANE=1already exists for this exact reason on agterm, which has the same layout model. herdr had no equivalent, and no flag, env var, config key or keybinding produces a split today.How does this solve it?
Splits the caller's own pane and zooms it, so the review still gets the full tab area but the agent pane is one keypress away and no tab is created. It fixes the root cause — the backend using the wrong herdr primitive — and copies the shape agterm settled on: env-var opt-in, capability probe before passing a new flag, quiet degradation to the tab overlay when anything is missing.
Unset, nothing changes. Pane mode is a self-contained block ahead of the tab path, so the
tab createblock is reached byte-identical to master.Two rules worth knowing before editing it, both in the CLAUDE.md entry:
pane listto recover it — the pane population can change in between, so a recovered id may belong to another herdr client. An id equal to the caller's own pane is rejected for the same reason.pane_not_foundis authoritative death (nothing closed, nothing warned); any other error is transient, bounded so the wait cannot hang, with ownership kept so teardown still closes.Testing
TestHerdrPaneOverlayOptIn, 8 cases × both launcher copies, asserting the recorded herdr call sequence. PlusTestHerdrTrapClosesPane, a textual guard following the existingTestLauncherNestedHeredocsHaveNoApostrophesprecedent, because no behavioural test reaches the trap's close.Verified by hand against herdr 0.8.2: both modes annotate and exit 10 with the annotation on stdout, pane and tab lists return to their prior set, and killing the review pane mid-review returns promptly.
make testandgolangci-lint run(v2.12.2) are clean;make fmtshells out to~/.claude/format.shso it could not run here, butgolangci-lint fmt --diffis empty.Docs updated in step: README,
site/docs.html,site/index.html, both reference pages, bothSKILL.md, and the CLAUDE.md launcher entry. Also corrects CLAUDE.md's stale-timeout=100s— the Makefile has been 180s since e522b26, and it is the budget these tests spend from.+471 lines, of which 86 are the mandatory byte-identical codex copy of the launcher. Launcher-only, works with any binary, so no plugin/marketplace/package version bumps. I have a longer design write-up if you keep completed plans for contributor features.