Skip to content

feat(launcher): herdr pane-scoped overlay (REVDIFF_HERDR_PANE=1) - #344

Open
rayslava wants to merge 1 commit into
umputun:masterfrom
rayslava:feat/herdr-pane-overlay
Open

feat(launcher): herdr pane-scoped overlay (REVDIFF_HERDR_PANE=1)#344
rayslava wants to merge 1 commit into
umputun:masterfrom
rayslava:feat/herdr-pane-overlay

Conversation

@rayslava

Copy link
Copy Markdown
Contributor

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=1 already 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?

REVDIFF_HERDR_PANE=1 launch-revdiff.sh HEAD~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 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 create block is reached byte-identical to master.

Two rules worth knowing before editing it, both in the CLAUDE.md entry:

  • Never guess which pane to close. A split that reports success but returns no usable id warns and exits rather than diffing pane list to 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_found is 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. Plus TestHerdrTrapClosesPane, a textual guard following the existing TestLauncherNestedHeredocsHaveNoApostrophes precedent, 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 test and golangci-lint run (v2.12.2) are clean; make fmt shells out to ~/.claude/format.sh so it could not run here, but golangci-lint fmt --diff is empty.

Docs updated in step: README, site/docs.html, site/index.html, both reference pages, both SKILL.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.

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.
@rayslava
rayslava requested a review from umputun as a code owner August 31, 2026 14:11

@umputun umputun left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

2 participants