feat(launcher): herdr pane-scoped overlay (REVDIFF_HERDR_PANE=1) - #344
Conversation
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.
a9d3a9e to
9b652e5
Compare
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. owning a pane means owning its teardown, and the rules are recorded in CLAUDE.md because none of them are guessable from the code alone: - never guess which pane to close. a split that returns no usable pane id warns and exits rather than diffing `pane list`, since a recovered id may belong to another herdr client. - `pane_not_found` is authoritative death; any other error is transient, so the wait loop keeps polling with a backoff rather than treating an unreachable control plane as a dead review. - cleanup is decided by evidence from the pane, not by `pane run` returning. the dispatched script touches a marker, ownership is claimed before the call, and unknown state is preserved rather than destroyed -- a launcher killed on timeout leaves a live review open with nothing lost. - a trapped signal is deferred until the in-flight command returns and then runs before the next statement, so the windows where a pane exists but is not yet named record the signal instead of exiting on it, and pay it once ownership is held. the refusal path's evidence grace is measured in elapsed wall clock rather than completed sleeps, because a process-group signal kills the foreground `sleep` itself -- counting sleeps can end with no time elapsed, leaving an absent marker that proves nothing. tested with `TestHerdrPaneOverlayOptIn` and `TestHerdrSignalPaneOwnership` (40 subtests over both launcher copies), the latter signalling a real launcher to pin both halves of the ownership contract. verified by hand against herdr 0.8.2, and all four launcher scripts parse clean under real bash 3.2.57. also corrects CLAUDE.md's stale `-timeout=100s`; the Makefile has been 180s since e522b26. pins shellcheck in CI. the runner image's version drifts and `shellcheck disable=` is version-sensitive -- SC2317 was split into SC2329 in 0.11, so a suppression naming only the code the local shellcheck emits passes locally and fails CI. the launcher needs one such suppression: `herdr_cleanup_unlaunched` is the only trap that calls a function instead of inlining, and shellcheck cannot see invocations inside the trap's quoted string. it is a false positive -- mutating the function's body fails three signal tests.
9b652e5 to
6166fe4
Compare
the REVDIFF_HERDR_PANE bullet described an earlier iteration of the launcher's refusal path. Four claims did not match the merged code: a 300ms grace (it is a wall-clock interval, SECONDS -lt 2), HERDR_DISPATCHED set after pane run returns (it is claimed before the call), the sleep sitting in an if condition (it is a while loop with || true), and a preserve-on-exhausted-retries rule pinned by a test that does not exist. The shipped behavior closes the pane once the interval has elapsed with no marker, and "a relentlessly signaled grace is still served" asserts that with a minimum duration. Related to #344
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.Owning a pane means owning its teardown
Most of the work here is the teardown contract, not the split. The rules are in the CLAUDE.md entry because none of them are guessable from the code alone:
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; any other error is transient. The wait loop keeps polling with a backoff and one warning per outage, because a generic error is not evidence the pane died and any deadline turns unknown liveness into a closed live review.pane runreturning. herdr may start the review before that call returns, or not after it. The dispatched script touches a marker, ownership is claimed before the call, and unknown state is preserved rather than destroyed — so a launcher killed on timeout leaves a live review open with nothing lost, which is what SKILL.md promises the driving agent.exitin the windows where a pane exists but is not yet named would leave the trap nothing to close. Those windows record the signal and pay it once ownership is held. The refusal path's evidence grace additionally survives its ownsleepbeing killed by a process-group signal, and treats an absent marker as evidence only if an interval actually elapsed.Testing
TestHerdrPaneOverlayOptInandTestHerdrSignalPaneOwnership— 40 subtests across both launcher copies, asserting the recorded herdr call sequence. The signal test sends real SIGTERM/SIGINT to a live launcher to pin both halves of the ownership contract: the pre-dispatch cases must close the pane they created, everything from the dispatch onward must preserve it.The grace cases are deterministic rather than timing-based: a PATH-injected
sleepraises the signal before sleeping, so it is already pending when the grace sleep becomes the foreground command and there is no window to race.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.All four launcher scripts also parse clean under real bash 3.2.57 (via the
bash:3.2container). That may be of separate interest: CLAUDE.md notes the nested-heredoc apostrophe hazard can't be guarded in CI because ubuntu and every Homebrew bash accept the broken form — abash:3.2container is a viable guard for it.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.One CI change is bundled in
ci.ymlnow runs shellcheck from a digest-pinnedkoalaman/shellcheck:v0.11.0container instead of whateverubuntu-latestships. This PR is what exposed the problem:SC2317("command appears unreachable") was split intoSC2329in 0.11, so a# shellcheck disable=naming only the code your local shellcheck emits passes locally and fails CI. The launcher needs exactly one such suppression —herdr_cleanup_unlaunchedis the only trap that calls a function rather than inlining, and shellcheck cannot see invocations inside the trap's quoted string. It is a false positive: mutating the function body fails three signal tests. The suppression names both codes, so it is correct on any version with or without the pin; the pin stops the next recurrence and gives contributors a command that reproduces CI exactly. Happy to split this out if you would rather keep the PR purely to the launcher.Launcher-only otherwise, works with any binary, so no plugin/marketplace/package version bumps.