diff --git a/.claude-plugin/skills/revdiff/SKILL.md b/.claude-plugin/skills/revdiff/SKILL.md index 1904a36e..01622b5f 100644 --- a/.claude-plugin/skills/revdiff/SKILL.md +++ b/.claude-plugin/skills/revdiff/SKILL.md @@ -138,6 +138,8 @@ The resolver and launcher MUST run in the same bash invocation — the resolver **Disconnect-resilient tmux window mode**: when running under tmux, prefix the launcher with `REVDIFF_TMUX_WINDOW=1` to open revdiff in a persistent, server-owned tmux window instead of a client-owned `display-popup`. The review then survives a dropped SSH or tmux client — reattach and it is still there. This is a launcher environment variable, not a revdiff flag. +**Pane-scoped overlay (herdr)**: when running under herdr, `REVDIFF_HERDR_PANE=1` opens revdiff in a zoomed split of the agent's own pane instead of a new fullscreen tab, keeping the agent pane one keypress away. The user sets it in the environment; it falls back to the tab overlay on an older herdr CLI. This is a launcher environment variable, not a revdiff flag. + **Pane-scoped overlay (agterm)**: when running in an agterm split, `REVDIFF_AGTERM_PANE=1` opens revdiff in the agent's own pane instead of over the whole session, leaving the sibling pane live and visible. The user sets it in the environment; it is ignored outside a split. This is a launcher environment variable, not a revdiff flag. The script: diff --git a/.claude-plugin/skills/revdiff/references/config.md b/.claude-plugin/skills/revdiff/references/config.md index 74e11b7b..2ca07ab7 100644 --- a/.claude-plugin/skills/revdiff/references/config.md +++ b/.claude-plugin/skills/revdiff/references/config.md @@ -79,6 +79,10 @@ When launched via the Claude Code plugin skill, revdiff opens in a terminal over Set `REVDIFF_TMUX_WINDOW=1` in the launcher's environment to open revdiff in a persistent, server-owned tmux window instead of a client-owned `display-popup`. A dropped SSH or tmux client tears down a popup and kills the review, but a server-owned window survives the disconnect — reattach and the live review is still there. This is a launcher environment variable, not a revdiff flag. +## Pane-Scoped Overlay (herdr) + +Set `REVDIFF_HERDR_PANE=1` in the launcher's environment to open revdiff in a zoomed split of the agent's own herdr pane instead of a new fullscreen tab, so the agent pane stays one keypress away. It needs a herdr whose CLI carries `pane split`, `pane get` and `pane close`; an unsupported CLI or a refused split falls back to the tab overlay; a split that succeeds but returns no usable pane id fails closed with a warning rather than opening a second surface, and may leave a stray pane to close by hand. This is a launcher environment variable, not a revdiff flag. + ## Pane-Scoped Overlay (agterm) Set `REVDIFF_AGTERM_PANE=1` in the launcher's environment to open revdiff in the agent's own split pane instead of over the whole session, leaving the sibling pane live and visible. It applies only when that session is split — the session-wide overlay stands otherwise, and the launcher retries session-wide if agterm refuses the pane. The review gets pane width rather than session width, which is why it is opt-in. This is a launcher environment variable, not a revdiff flag. diff --git a/.claude-plugin/skills/revdiff/references/usage.md b/.claude-plugin/skills/revdiff/references/usage.md index a045fa14..c5586d3b 100644 --- a/.claude-plugin/skills/revdiff/references/usage.md +++ b/.claude-plugin/skills/revdiff/references/usage.md @@ -351,6 +351,10 @@ Override the history directory with `--history-dir`, `REVDIFF_HISTORY_DIR` env v Set `REVDIFF_TMUX_WINDOW=1` in the launcher's environment to open revdiff in a persistent, server-owned tmux window instead of a client-owned `display-popup`. A dropped SSH or tmux client tears down a popup and kills the review, but a server-owned window survives the disconnect — reattach and the live review is still there. This is a launcher environment variable, not a revdiff flag. +## Pane-Scoped Overlay (herdr) + +Set `REVDIFF_HERDR_PANE=1` in the launcher's environment to open revdiff in a zoomed split of the agent's own herdr pane instead of a new fullscreen tab, so the agent pane stays one keypress away. It needs a herdr whose CLI carries `pane split`, `pane get` and `pane close`; an unsupported CLI or a refused split falls back to the tab overlay; a split that succeeds but returns no usable pane id fails closed with a warning rather than opening a second surface, and may leave a stray pane to close by hand. This is a launcher environment variable, not a revdiff flag. + ## Pane-Scoped Overlay (agterm) Set `REVDIFF_AGTERM_PANE=1` in the launcher's environment to open revdiff in the agent's own split pane instead of over the whole session, leaving the sibling pane live and visible. It applies only when that session is split — the session-wide overlay stands otherwise, and the launcher retries session-wide if agterm refuses the pane. The review gets pane width rather than session width, which is why it is opt-in. This is a launcher environment variable, not a revdiff flag. diff --git a/.claude-plugin/skills/revdiff/scripts/launch-revdiff.sh b/.claude-plugin/skills/revdiff/scripts/launch-revdiff.sh index 2b47e7b0..b33d52ac 100755 --- a/.claude-plugin/skills/revdiff/scripts/launch-revdiff.sh +++ b/.claude-plugin/skills/revdiff/scripts/launch-revdiff.sh @@ -289,19 +289,232 @@ fi # herdr: open a new fullscreen tab via the herdr CLI (must precede kitty — # inside herdr-in-kitty KITTY_LISTEN_ON is set, so the kitty branch would -# otherwise win and open an overlay window herdr cannot composite into its panes) +# otherwise win and open an overlay window herdr cannot composite into its panes). +# REVDIFF_HERDR_PANE=1 instead runs the review in a zoomed split of the agent's +# own pane, which keeps that pane one keypress away; it is self-contained and +# never falls through to the tab path below except when the split is declined. if [ "${HERDR_ENV:-}" = "1" ] && command -v herdr >/dev/null 2>&1; then + # $HERDR_PANE_ID is injected by herdr into every managed pane, and the tab path + # below reuses that name for the pane it creates — copy the caller's id out first + HERDR_CALLER_PANE="${HERDR_PANE_ID:-}" + # non-empty once we own a review pane that has to be closed on every exit path + HERDR_TARGET="" + # the pane's own shell touches this the instant it starts the launch script, so + # ownership is decided by evidence FROM the pane rather than by `pane run` returning: + # herdr may have started the review before the CLI call returns, and may not have + # started it after. Assigned once $SENTINEL exists, below. `touch ... || true` and not + # `: >`, because a redirection failure on a special builtin kills a POSIX shell outright, + # which would stop revdiff running at all; a $TMPBASE the pane cannot write already + # breaks the sentinel the same way, for the tab path too. + HERDR_STARTED="" + # set BEFORE `pane run` and never cleared again, so the in-flight window is owned + # rather than ambiguous: a pending signal is serviced the instant the call returns, + # before any later assignment could run. The marker is still needed for the failure + # path, where it is the only evidence that a refused-looking dispatch actually started. + HERDR_DISPATCHED=0 + + # all three are used at runtime; a CLI lacking `get` would read every liveness poll + # as a failure and abandon a live review. zoom stays unprobed: it is purely cosmetic. + herdr_supports_pane_mode() { + herdr pane split --help 2>/dev/null | grep -q -- '--direction' \ + && herdr pane get --help >/dev/null 2>&1 \ + && herdr pane close --help >/dev/null 2>&1 + } + + # idempotent, and clears HERDR_TARGET before shelling out so a signal during the + # close cannot re-enter through the EXIT trap and close the same id twice + herdr_close_pane() { + local target="$HERDR_TARGET" + HERDR_TARGET="" + [ -n "$target" ] || return 0 + if ! herdr pane close "$target" >/dev/null 2>&1; then + herdr pane zoom "$target" --off >/dev/null 2>&1 || true + printf 'revdiff: could not close herdr review pane %s\n' "$target" >&2 + fi + return 0 + } + + # EXIT-trap cleanup. A review that has STARTED but not FINISHED belongs to the user: + # SKILL.md promises the driving agent that a launcher killed on timeout leaves revdiff + # open with nothing lost, so we must not close it and must not delete the script it is + # running. Anything else is ours -- a pane that never started one (close it, it holds + # only a shell) or one that already finished (close it, it is done). + # shellcheck disable=SC2317,SC2329 # invoked from the EXIT trap below, which shellcheck cannot + # see. both codes: shellcheck <0.11 reports this as SC2317, 0.11+ as SC2329 + herdr_cleanup_unlaunched() { + # pane mode only: the tab path never sets HERDR_TARGET, so its launch-script + # lifecycle stays exactly what master does here and what every other + # script-using backend does -- the trap removes it, unconditionally + if [ -n "$HERDR_TARGET" ] && [ ! -f "$SENTINEL" ] && + { [ -f "$HERDR_STARTED" ] || [ "$HERDR_DISPATCHED" = 1 ]; }; then + return 0 + fi + herdr_close_pane + rm -f "$LAUNCH_SCRIPT" "$HERDR_STARTED" + return 0 + } + SENTINEL=$(mktemp "$TMPBASE/revdiff-done-XXXXXX") rm -f "$SENTINEL" + HERDR_STARTED="$SENTINEL.started" LAUNCH_SCRIPT=$(mktemp "$TMPBASE/revdiff-launch-XXXXXX") - trap 'rm -f "$OUTPUT_FILE" "$ERR_FILE" "$SENTINEL" "$SENTINEL.tmp" "$LAUNCH_SCRIPT"' EXIT + trap 'herdr_cleanup_unlaunched || true; rm -f "$OUTPUT_FILE" "$ERR_FILE" "$SENTINEL" "$SENTINEL.tmp" "$HERDR_STARTED"' EXIT + # same shape as the agterm branch above: INT/TERM exit through the EXIT trap so a + # signal never skips cleanup or leaves an unnormalised status + trap 'exit 130' INT + trap 'exit 143' TERM cat > "$LAUNCH_SCRIPT" </dev/null || true $(write_rc_cmd "$SENTINEL") +rm -f "\$0" LAUNCHER chmod +x "$LAUNCH_SCRIPT" + if [ "${REVDIFF_HERDR_PANE:-}" = 1 ] && [ -n "$HERDR_CALLER_PANE" ] && herdr_supports_pane_mode; then + # A trapped signal is deferred until the in-flight foreground command returns, then + # runs BEFORE the next statement -- so `exit 143` during the split would fire after + # the pane exists but before HERDR_TARGET names it, and the EXIT trap would have + # nothing to close. Record the signal instead of acting on it, and honor it once + # ownership is held. This is a trap with a COMMAND, which children reset to default; + # `trap '' INT TERM` would be SIG_IGN, inherited across exec, and would make a hung + # herdr unkillable (see CLAUDE.md). It also adds no deferral that `exit 143` did not + # already have: both wait out the same wedged split. + HERDR_SIGNALLED=0 + trap 'HERDR_SIGNALLED=130' INT + trap 'HERDR_SIGNALLED=143' TERM + if HERDR_NEW=$(herdr pane split --pane "$HERDR_CALLER_PANE" --direction right \ + --cwd "$CWD" --focus 2>&1); then + if command -v jq >/dev/null 2>&1; then + HERDR_TARGET=$(printf '%s' "$HERDR_NEW" | jq -r '.result.pane.pane_id // empty' 2>/dev/null || true) + fi + if [ -z "$HERDR_TARGET" ]; then + HERDR_TARGET=$(printf '%s' "$HERDR_NEW" | grep -o '"pane_id":"[^"]*"' | head -1 | cut -d'"' -f4 || true) + fi + # the grep fallback is positional, so a response listing the source pane first + # would hand us the caller's own id — closing that would kill the agent's pane + if [ "$HERDR_TARGET" = "$HERDR_CALLER_PANE" ]; then + HERDR_TARGET="" + fi + if [ -z "$HERDR_TARGET" ]; then + # never guess: a pane found by diffing `pane list` may belong to another + # herdr client, and closing that would destroy the user's own work + printf 'revdiff: herdr pane split returned no pane id; a stray review pane may remain: %s\n' \ + "$HERDR_NEW" >&2 + exit 1 + fi + # ownership is held: back to exiting traps, and pay any signal deferred above + # through the EXIT trap, which can now close the pane it created + trap 'exit 130' INT + trap 'exit 143' TERM + [ "$HERDR_SIGNALLED" != 0 ] && exit "$HERDR_SIGNALLED" + herdr pane zoom "$HERDR_TARGET" --on >/dev/null 2>&1 || true + # claim ownership BEFORE dispatching. A pending signal is serviced the moment + # this call returns and before any assignment after it, so ownership taken + # afterwards would miss a dispatch herdr had already accepted. Never cleared + # again -- the refusal path below says why restoring a reset here is unsafe. + HERDR_DISPATCHED=1 + if ! herdr pane run "$HERDR_TARGET" "sh $(sq "$LAUNCH_SCRIPT")" >/dev/null 2>&1; then + # this branch does trust the return code, unlike the trap above: a lost + # response after a successful dispatch would close a just-started review, + # but stranding a pane on every genuine dispatch failure is the worse default + echo "error: herdr pane run failed for pane $HERDR_TARGET" >&2 + # herdr refused the command, so nothing was dispatched -- unless the pane + # announced itself anyway, which would mean the response was lost after + # delivery. Give that a moment to show up, then close only a pane that + # never announced. Paid on the failure path only. + # HERDR_DISPATCHED deliberately stays 1 across the grace: the trap reads it, + # so clearing it here would let a signal landing inside the sleep close the + # pane before the marker had its chance to appear -- the exact unknown-state + # destruction claim-before-dispatch exists to prevent. Clearing it after the + # sleep would be dead anyway, since herdr_close_pane discharges ownership by + # emptying HERDR_TARGET, which is what the trap gates on. + # + # The grace must therefore RUN TO COMPLETION even while being signalled: any + # exit taken inside it hands the trap a state it must read as "may be live" + # and preserve, stranding a pane the finished evidence check would close. + # Two things are needed for that, and neither alone is enough: + # - record the signal instead of exiting on it, as the split window does; + # - survive the sleep itself being killed. A process-group signal + # (interactive Ctrl-C, `kill -- -pgid`) hits the foreground `sleep` too, + # which then returns nonzero and lets `set -e` abort the script before + # the evidence check -- resurrecting the strand despite the trap. + # The grace is therefore measured in ELAPSED TIME, not in completed sleeps: + # $SECONDS is set by the shell from the wall clock, so a killed `sleep` costs + # an early wakeup and nothing else, and the interval is served no matter how + # many signals arrive. Counting sleeps instead can end with zero time + # elapsed, leaving an absent marker that proves nothing. `|| true` keeps an + # interrupted sleep from tripping errexit; the loop body cannot fail + # otherwise, so it cannot re-arm it either. The marker check is in the + # condition, so the common path leaves as soon as the pane announces itself + # rather than always paying the full interval. + # + # `-lt 2`, not `-lt 1`: $SECONDS counts whole-second boundaries crossed since + # the assignment, so it can reach 1 a millisecond later if the assignment + # lands just before a boundary. Waiting for the second boundary guarantees a + # full second was served; the cost is up to about two seconds, on a failure path only. + HERDR_SIGNALLED=0 + trap 'HERDR_SIGNALLED=130' INT + trap 'HERDR_SIGNALLED=143' TERM + SECONDS=0 + while [ ! -f "$HERDR_STARTED" ] && [ "$SECONDS" -lt 2 ]; do + sleep 0.3 || true + done + # the interval was served, so an absent marker is real evidence + [ -f "$HERDR_STARTED" ] || herdr_close_pane + trap 'exit 130' INT + trap 'exit 143' TERM + # the refusal is why we are exiting, so it owns the status; a signal that + # arrived during the grace has already been honored by the close above + exit 1 + fi + HERDR_MISSES=0 + HERDR_POLL=0.3 + while [ ! -f "$SENTINEL" ]; do + if HERDR_GET=$(herdr pane get "$HERDR_TARGET" 2>&1); then + HERDR_MISSES=0 + HERDR_POLL=0.3 + elif printf '%s' "$HERDR_GET" | grep -q '"code":"pane_not_found"'; then + # authoritative: the pane is gone, so ownership is already discharged -- + # nothing to close, and nothing to complain about + HERDR_TARGET="" + break + else + # a generic error is NOT proof of death (socket hiccup, server restart), + # and there is no safe deadline when the API cannot report liveness -- + # any bound turns unknown liveness into a closed live review. Warn once + # per outage and back off, then keep waiting, exactly as the tab path + # below does for a human review. Both reset on a good poll. + HERDR_MISSES=$((HERDR_MISSES + 1)) + if [ "$HERDR_MISSES" = 10 ]; then + printf 'revdiff: herdr pane get keeps failing, still waiting for the review: %s\n' \ + "$HERDR_GET" >&2 + # stop hammering a control plane that is down; the sentinel is still + # checked every iteration, so finishing costs at most one interval + HERDR_POLL=2 + fi + fi + sleep "$HERDR_POLL" + done + rc=$(read_rc "$SENTINEL") + # the review is over either way: the script ran (and removed itself), or the + # pane died before it could and nothing will ever open it now. Only a launcher + # killed mid-review leaves it behind, which is deliberate -- the pane may still + # be about to open it. + rm -f "$LAUNCH_SCRIPT" "$HERDR_STARTED" + herdr_close_pane + print_output_and_exit "${rc:-1}" + fi + # nothing was created — fall through to the tab path. Restore the exiting traps + # first: the recording trap must not survive into a path with no pane to protect, + # or a signal there would be swallowed instead of ending the launcher. + trap 'exit 130' INT + trap 'exit 143' TERM + [ "$HERDR_SIGNALLED" != 0 ] && exit "$HERDR_SIGNALLED" + printf 'revdiff: herdr pane split declined, using tab overlay: %s\n' "$HERDR_NEW" >&2 + fi + # pin the tab to the caller's workspace: without --workspace, herdr tab create # targets the server's focused workspace (what the user is currently viewing), # not the caller's workspace diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 43b25c02..ba4a9b76 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -66,5 +66,12 @@ jobs: python3 .github/scripts/validate-frontmatter.py - name: shellcheck + # pinned: the runner image's shellcheck drifts, and a `shellcheck disable=` written + # against a newer local version can pass locally and fail here. SC2317 was split into + # SC2329 in 0.11, so a suppression naming only one code silently breaks on the other. + # digest-pinned: a docker tag is mutable, and a retag would change the checker + # silently -- the same failure class the pin exists to prevent. + # Contributors can reproduce this exact check with the same image. run: | - find . -name '*.sh' -not -path './.git/*' -not -path './vendor/*' -print0 | xargs -0 shellcheck + find . -name '*.sh' -not -path './.git/*' -not -path './vendor/*' -print0 \ + | xargs -0 docker run --rm -v "$PWD":/mnt:ro koalaman/shellcheck:v0.11.0@sha256:61862eba1fcf09a484ebcc6feea46f1782532571a34ed51fedf90dd25f925a8d diff --git a/CLAUDE.md b/CLAUDE.md index 29ac43aa..ca4c4b3d 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -70,8 +70,10 @@ TUI for reviewing diffs, files, and documents with inline annotations, built wit - `usage.md` — examples, key bindings, output format - **Adding a new CLI flag requires SKILL.md updates, not just reference docs.** `references/config.md` and `references/usage.md` document the flag's *existence*; `SKILL.md` teaches AI agents *when* to pass it during automatic launches (e.g. "pass `--untracked` when the recent change likely created new untracked files"). Without a SKILL.md entry, AI agents using the plugin will not know to pass the flag even though it's documented. Apply the same update to `plugins/codex/skills/revdiff/SKILL.md` (keep in sync with `.claude-plugin/skills/revdiff/SKILL.md`) and to `plugins/pi/skills/revdiff/SKILL.md` (which lists user-facing command examples). The launcher scripts (`launch-revdiff.sh`) pass `"$@"` through, so no script changes are needed beyond updating the usage-comment header for documentation parity. - **Launchers must parse under bash 3.2** (`/bin/bash` on stock macOS, where `#!/usr/bin/env bash` resolves unless a newer bash is on PATH). Its parser scans a `$( )` command substitution for quotes **before** it processes a heredoc inside it, so an odd number of apostrophes in a heredoc opened **inside** one opens a quote that never closes and the whole script fails to parse — reported a hundred lines later, on an unrelated valid line. Two heredocs per launcher are nested that way, the `GHOSTTY_TERM_ID=$(osascript ...)` and `ITERM_NEW_SESSION=$(osascript ...)` captures; the close-pane heredocs beside them are plain commands and are unaffected. Keep the nested bodies apostrophe-free; balancing them is not a fix, since a later edit to one word re-breaks it. No parse check can guard this in CI — ubuntu and every Homebrew bash accept the broken form — so `TestLauncherNestedHeredocsHaveNoApostrophes` guards it textually instead, and `/bin/bash -n` is the direct check on macOS. Introduced by #309, reported in #314. +- **shellcheck is pinned in CI** (`koalaman/shellcheck:v0.11.0`, digest-pinned in `.github/workflows/ci.yml` because a docker tag is mutable and a retag would change the checker silently) because the runner image's version drifts and a `# shellcheck disable=` is version-sensitive: `SC2317` ("command appears unreachable") was split into `SC2329` ("function never invoked") in 0.11, so a suppression naming only the code your local shellcheck emits passes locally and fails CI — which is exactly how #344 broke. Name **both** codes. The one place this bites is `herdr_cleanup_unlaunched`: it is the only trap in the launcher that calls a *function* rather than inlining its commands, and shellcheck cannot see invocations inside the trap's quoted string. The warning is a false positive — mutating the function's body fails three `TestHerdrSignalPaneOwnership` cases — so suppress it, don't inline the function to appease the linter. Reproduce CI exactly with `find . -name '*.sh' -not -path './.git/*' -not -path './vendor/*' -print0 | xargs -0 docker run --rm -v "$PWD":/mnt:ro koalaman/shellcheck:v0.11.0@sha256:61862eba1fcf09a484ebcc6feea46f1782532571a34ed51fedf90dd25f925a8d`. - **Launcher override chain**: both Claude plugins resolve their launcher script via `resolve-launcher.sh` through `user → bundled` layers (first executable wins). The planning plugin's user layer is `${CLAUDE_PLUGIN_DATA}/scripts/` under Claude and `${PLUGIN_DATA}/scripts/` under Codex. There is **no project-level (`.claude/...` or `.codex/...`) executable layer by design** — the planning hook fires automatically in any repo, and a repo-controlled launcher would run on routine agent actions. The Pi extension and manual Codex diff-review skill do not use this plugin-data override. -- **Overlay stderr relay**: `launch-revdiff.sh` appends `2>$ERR_FILE` to `REVDIFF_CMD` once, right after the argument loop, so every backend captures revdiff's stderr without per-backend edits (the redirect stays the trailing token through `write_rc_cmd` / `write_fifo_rc_cmd`, the heredoc launch scripts, and the later `/usr/bin/env` prepend). `print_output_and_exit` replays the file on any exit code other than 0 or 10 — those two are successes, and revdiff writes ordinary warnings to stderr, so relaying them would put noise on every successful review. All ten `EXIT` traps in `launch-revdiff.sh` list `$ERR_FILE` — an override that omits it leaks the file into `$TMPDIR`, which `TestShellLaunchersPreserveAnnotationExitCode` catches by asserting no `revdiff-err-*` survives the run. The stderr expectations live in that test because a second launcher-by-backend pass puts the `app` package over the timeout in `make race` (now `-timeout=100s`, matching CI; the matrix alone runs ~53s); a new launcher-wide behavior belongs in the same matrix, gated per launcher via `relaysStderr` (`launch-plan-review.sh` has no relay). The sourced agent-deck window backend (`agentdeck-window.sh`) is the eleventh execution path and carries no relay code of its own: it builds its command through `write_rc_cmd`, exits through `print_output_and_exit`, and installs no `EXIT` trap by design, so it rides the base one. It is the only backend absent from `launcherBackends()`, so the relay tests do not cover it. `plugins/revdiff-planning/scripts/launch-plan-review.sh` has no relay: its per-backend exit tails are duplicated inline with no shared helpers. +- **Overlay stderr relay**: `launch-revdiff.sh` appends `2>$ERR_FILE` to `REVDIFF_CMD` once, right after the argument loop, so every backend captures revdiff's stderr without per-backend edits (the redirect stays the trailing token through `write_rc_cmd` / `write_fifo_rc_cmd`, the heredoc launch scripts, and the later `/usr/bin/env` prepend). `print_output_and_exit` replays the file on any exit code other than 0 or 10 — those two are successes, and revdiff writes ordinary warnings to stderr, so relaying them would put noise on every successful review. All ten `EXIT` traps in `launch-revdiff.sh` list `$ERR_FILE` — an override that omits it leaks the file into `$TMPDIR`, which `TestShellLaunchersPreserveAnnotationExitCode` catches by asserting no `revdiff-err-*` survives the run. The stderr expectations live in that test because a second launcher-by-backend pass puts the `app` package over the timeout in `make race` (now `-timeout=180s`, matching CI); a new launcher-wide behavior belongs in the same matrix, gated per launcher via `relaysStderr` (`launch-plan-review.sh` has no relay). The sourced agent-deck window backend (`agentdeck-window.sh`) is the eleventh execution path and carries no relay code of its own: it builds its command through `write_rc_cmd`, exits through `print_output_and_exit`, and installs no `EXIT` trap by design, so it rides the base one. It is the only backend absent from `launcherBackends()`, so the relay tests do not cover it. `plugins/revdiff-planning/scripts/launch-plan-review.sh` has no relay: its per-backend exit tails are duplicated inline with no shared helpers. +- **herdr pane-scoped overlay (`REVDIFF_HERDR_PANE=1`)**: opt-in, off by default; unset, the `herdr tab create` + `pane run` block is reached byte-identical, because pane mode is a block ahead of it that exits on its own rather than a flag threaded through the shared path. The tab block itself is byte-identical, but the EXIT trap and the generated launch script ARE shared — the ownership machinery there is inert in tab mode only because `HERDR_TARGET` stays empty, so treat both as pane/tab common code when editing. **`$HERDR_PANE_ID` is a name collision** — herdr injects it into every managed pane and the tab path uses it as its own local, so the caller's id is copied to `HERDR_CALLER_PANE` first; losing it types the launch command into the agent's own shell. Ownership is `HERDR_TARGET` (non-empty = a pane owes a close); `herdr_close_pane` clears it before shelling out so a re-entrant call cannot close twice. **Cleanup is decided by evidence from the pane, not by `pane run` returning** — herdr may start the review before that call returns, or not after it. The dispatched script's first line touches `$SENTINEL.started`, and `HERDR_DISPATCHED` flips to 1 once `pane run` returns success; `herdr_cleanup_unlaunched` preserves the pane while **either** signal says a review may exist and the sentinel says it has not finished, closing and cleaning up otherwise (never started — holds only a shell; already finished — done). `HERDR_DISPATCHED` is claimed **before** `pane run` and never cleared, because a pending signal is serviced the instant that call returns and before any later assignment — ownership taken afterwards would miss a dispatch herdr had already accepted, and clearing it on the refusal path would let a signal landing inside that path's 300ms evidence grace close a pane whose review may already be running. Nothing needs the clear: `herdr_close_pane` discharges ownership by emptying `HERDR_TARGET`, which is what `herdr_cleanup_unlaunched` gates on. A launcher killed while herdr is wedged inside the call therefore preserves the pane: the state is unknown, and unknown is never destroyed. The marker still matters on the failure path, where it is the only evidence that a refused-looking dispatch actually started. For the same reason a nonzero `pane run` does not close a pane that has already announced itself. That is what `SKILL.md` promises the driving agent: a launcher killed on timeout leaves a *live* review open with nothing lost. Ownership does not depend on the order of `pane run` and the cosmetic `pane zoom` — the marker settles it — so the zoom stays first and revdiff starts in an already-zoomed pane rather than being resized during startup. The marker is written with `touch … || true`, never `: >`: a redirection failure on a special builtin kills a POSIX shell outright and revdiff would never run. After dispatch the script belongs to the pane (it ends with `rm -f "$0"`); the completion path removes it for a pane that died first. `TestHerdrSignalPaneOwnership` signals a real launcher to pin both halves; three of its cases reach the trap's close — wedged in `pane split`, interrupted in `pane split`, and wedged in `pane zoom` — all of them pre-dispatch, where the pane provably holds only a shell; every later case is the preserve half — without it the close is mutation-invisible. The block also adds `trap 'exit 130' INT` / `trap 'exit 143' TERM` for agterm parity, and the **tab** path inherits them. They do not change *whether* cleanup runs — bash runs the EXIT trap on an untrapped SIGTERM too, same 143 — they change *when*: a trapped signal is deferred until the in-flight foreground command returns, so cleanup observes the state after the pending `herdr` call finishes rather than racing it. Pane-mode ownership depends on that ordering; for the tab path it is a no-op beyond a deterministic exit code. A trapped signal is deferred until the in-flight foreground command returns and then runs **before the next statement**, so `trap 'exit 143' TERM` across `pane split` would exit after the pane exists but before `HERDR_TARGET` names it, leaving the EXIT trap nothing to close. The split-and-parse window therefore installs *recording* traps (`trap 'HERDR_SIGNALLED=130' INT`, `trap 'HERDR_SIGNALLED=143' TERM`) and pays the signal once ownership is held; both exits from that window — success and the tab fallthrough — must restore `exit 130`/`exit 143` first, or a signal on a path with no pane to protect is swallowed and the launcher hangs (`killed on the tab fallthrough still exits` is the guard, via a bounded wait). The refusal path's 300ms evidence grace records signals for the same reason: exiting mid-grace hands the trap a state it must read as "may be live" and preserve, stranding a pane the completed check would have closed. **A recording trap alone is not enough there**, because a *process-group* signal (interactive Ctrl-C, `kill -- -pgid`) also kills the foreground `sleep`: it returns nonzero and `set -e` aborts before the evidence check, resurrecting the strand. The grace therefore retries the sleep so one full interval elapses, bounded so repeated signals cannot stall the exit (`sleep` sits in an `if` condition, and an `if` with a false condition returns 0, so the loop cannot re-arm errexit). **An absent marker is evidence only if an interval was actually served**: enough signals to kill every attempt leave the state unknown, so the launcher exits without closing and the trap preserves the pane (`a grace that never completes preserves the pane`). Each trap records its **own** status (`130` for INT, `143` for TERM) rather than a boolean, or a recorded Ctrl-C is paid as a fixed `exit 143` and misreports the signal. Reaching the grace deterministically needs no timing: a PATH-injected `sleep` raises the signal *before* sleeping, so it is already pending when the grace sleep becomes the foreground command (`FAKE_SLEEP_KILL=parent|group`, the group variant requiring `setpgid` or the kill lands on `go test` itself). A trap with a *command* is reset to default in children, which is why this is safe where SIG_IGN is not. Do **not** bracket the create window with `trap '' INT TERM`: `trap ''` is SIG_IGN, inherited across `exec`, so the herdr child ignores INT/TERM and a hung server makes the launcher unkillable except by SIGKILL. The probe covers `pane split`/`get`/`close` (all three are used at runtime; a CLI lacking `get` abandons a live review); `pane zoom` is unprobed as cosmetic. The wait loop treats `{"error":{"code":"pane_not_found"}}` as authoritative death (nothing closed, nothing warned) and any other error as transient — it keeps polling, warning once per outage streak at 10 misses and backing the poll off from 0.3s to 2s so a downed control plane is not hammered (both reset on a good poll; the sentinel is still checked every iteration), because a generic error is not evidence the pane died and any deadline turns unknown liveness into a closed live review. **Never guess which pane to close**: a split that returns no parseable id warns and exits 1 rather than diffing `pane list`, since a recovered id may belong to another herdr client; an id equal to `$HERDR_CALLER_PANE` is rejected for the same reason. - **agterm pane-scoped overlay (`REVDIFF_AGTERM_PANE=1`)**: opt-in, off by default — unset leaves the agterm branch's call exactly as it was. It adds `--pane $AGTERM_PANE` to `session overlay open` so the review covers the agent's pane alone instead of the whole session, and it needs all four of: the env var set to `1`, `$AGTERM_PANE` being `left`/`right` (`scratch` is full-coverage with no sibling), a `--pane`-capable agtermctl (`agterm_supports_pane_overlay`, which short-circuits before the split read), and a split confirmed by `agterm_session_split` — a **window-scoped** `tree --json` read (`tree` defaults to the FRONTMOST window, so an unscoped read finds no session and reports every split as absent) parsed with jq, which reports "not split" when jq is missing. That probe exists because `--pane` reached agtermctl only after agterm v0.9.0; it reads the PATH agtermctl, which is not always the CLI of the running app, and the post-call fallback is what covers the skew: on a nonzero exit whose captured agtermctl stderr matches `pane overlay already open|pane not visible`, the launcher retries session-wide (agterm refused before running revdiff, so nothing is re-executed — the grep is gated on agterm's own message precisely so a revdiff failure never triggers a second review). agtermctl's stderr is captured separately from revdiff's (`$ERR_FILE`) and replayed either way; its stdout is dropped because `print_output_and_exit` owns the launcher's stdout. Both launcher copies carry it; `TestAgtermPaneOverlayOptIn` covers the gate, the fallback, and the default path. Known limitation, documented beside the gate: `$AGTERM_PANE` is baked into the shell's environ at spawn, so a pane agterm promoted into the main slot keeps `right` — promote-then-re-split scopes the overlay to the NEW sibling instead of this pane, and the fallback cannot catch it because that pane genuinely exists and agterm raises no error. `session status` takes a stable `--pane-id` token for exactly this, `overlay open` does not yet, and failing closed to the session-wide overlay is deliberately not the answer. - **Launcher env vars don't reach the tmux/zellij popup**: `launch-revdiff.sh` spawns the revdiff process in a fresh shell inside the multiplexer popup that does NOT inherit the parent shell's environment, so env-var config set before the launch is dropped (e.g. `REVDIFF_THEME=gruvbox launch-revdiff.sh HEAD~10` does not apply the theme). Pass it as a CLI flag instead: `launch-revdiff.sh --theme gruvbox HEAD~10`. Applies to any env-var-configurable option launched through the overlay. - **Testing locally**: `claude --plugin-dir .claude-plugin` loads the diff-review skill from this checkout without going through the marketplace; `claude --plugin-dir plugins/revdiff-planning` does the same for the planning hook. Use `/reload-plugins` to pick up file edits mid-session. diff --git a/README.md b/README.md index 99c5585e..8ee713ae 100644 --- a/README.md +++ b/README.md @@ -86,7 +86,7 @@ The plugin requires one of the following terminals since Claude Code itself cann | **agterm** | `agtermctl session overlay open … --block` (full-pane overlay, blocks until quit) | `$AGTERM_SESSION_ID` env var | | **tmux** | `display-popup` (blocks until quit) | `$TMUX` env var | | **Zellij** | `zellij run --floating` | `$ZELLIJ` env var | -| **herdr** | `herdr tab create` + `herdr pane run` (new tab) | `$HERDR_ENV` env var | +| **herdr** | `herdr tab create` + `herdr pane run` (new tab), or a zoomed `herdr pane split` with `REVDIFF_HERDR_PANE=1` | `$HERDR_ENV` env var | | **kitty** | `kitty @ launch --type=overlay` | `$KITTY_LISTEN_ON` env var | | **wezterm** | `wezterm cli split-pane` | `$WEZTERM_PANE` env var | | **Kaku** | `kaku cli split-pane` (same API as wezterm) | `$WEZTERM_PANE` env var | @@ -115,6 +115,8 @@ Priority: agterm → tmux → Zellij → herdr → kitty → wezterm/Kaku → cm > **Disconnect-resilient tmux window mode:** set `REVDIFF_TMUX_WINDOW=1` in the launcher's environment to open revdiff in a persistent, server-owned tmux window instead of a client-owned `display-popup`. A dropped SSH or tmux client tears down a popup and kills the review, but a server-owned window survives the disconnect — reattach and the live review is still there. This is a launcher environment variable, not a revdiff flag. +> **Pane-scoped overlay (herdr):** Set `REVDIFF_HERDR_PANE=1` in the launcher's environment to open revdiff in a zoomed split of the agent's own herdr pane instead of a new fullscreen tab, so the agent pane stays one keypress away. It needs a herdr whose CLI carries `pane split`, `pane get` and `pane close`; an unsupported CLI or a refused split falls back to the tab overlay; a split that succeeds but returns no usable pane id fails closed with a warning rather than opening a second surface, and may leave a stray pane to close by hand. This is a launcher environment variable, not a revdiff flag. + > **Pane-scoped overlay (agterm):** set `REVDIFF_AGTERM_PANE=1` in the launcher's environment to open revdiff in the agent's own split pane instead of over the whole session, leaving the sibling pane live and visible. It applies only when that session is split — the session-wide overlay stands otherwise, and the launcher retries session-wide if agterm refuses the pane. The review gets pane width rather than session width, which is why it is opt-in. This is a launcher environment variable, not a revdiff flag. **Install:** diff --git a/app/plugin_exit_code_test.go b/app/plugin_exit_code_test.go index 853af38c..bbc58a80 100644 --- a/app/plugin_exit_code_test.go +++ b/app/plugin_exit_code_test.go @@ -13,7 +13,9 @@ import ( "runtime" "strconv" "strings" + "syscall" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -25,6 +27,9 @@ type cmdReq struct { stdin string args []string env map[string]string + // own process group: required whenever the command may signal its own group, or the + // kill lands on `go test` itself and takes the whole run down + setpgid bool } type cmdResult struct { @@ -408,6 +413,476 @@ func TestAgtermPaneOverlayOptIn(t *testing.T) { } } +// TestHerdrSignalPaneOwnership pins both halves of the EXIT-trap contract by actually +// signaling the launcher. While the pane provably holds nothing but a shell -- created, but +// not yet reached by a dispatch -- a killed launcher must close it, or a zoomed shell pane +// is stranded over the agent's own. From the dispatch onward the state is no longer provable +// and the review may be the user's, so it must survive us, per SKILL.md. The pre-dispatch +// cases -- wedged in the split, interrupted in the split, wedged in the zoom -- are the ones +// that reach the trap's close; the tab fallthrough closes nothing, and every case from the +// dispatch onward is the preserve half. +func TestHerdrSignalPaneOwnership(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("shell launchers are not used on windows") + } + root := testRepoRoot(t) + + launchers := map[string]string{ + "claude": filepath.Join(".claude-plugin", "skills", "revdiff", "scripts", "launch-revdiff.sh"), + "codex": filepath.Join("plugins", "codex", "skills", "revdiff", "scripts", "launch-revdiff.sh"), + } + tests := []struct { + name string + extraEnv map[string]string + // the fake logs every call BEFORE acting on it, so `pane run` marks "pane exists, + // not yet started". For the live case a logged call is not enough: ownership is + // evidence-based now, so the test must see the pane's own start marker on disk. + waitFor string // a recorded herdr call + waitForStart bool // the launch script's .started marker, sentinel still absent + // the hang/delay only has to outlast the signal, which lands in milliseconds. Kept + // short because killing the launcher orphans the fake's sleep. + wantClose bool + sig syscall.Signal // defaults to SIGTERM + wantCode int // 0 skips the check + + }{ + { + // the split is declined, so the launcher falls through to the tab overlay with + // no pane to protect. The recording trap must not survive into that path: the + // bounded wait above is what catches it if it does. + name: "killed on the tab fallthrough still exits", + extraEnv: map[string]string{ + "FAKE_HERDR_SPLIT_FAIL": "1", "FAKE_HERDR_NO_SENTINEL": "1", + }, + waitFor: "tab create", + wantClose: false, + }, + { + // wedged in `pane split` itself: the pane comes into existence during the call, + // so a signal deferred across it would otherwise be paid before HERDR_TARGET + // names the pane, leaving the EXIT trap nothing to close. The launcher records + // the signal instead and honors it once ownership is held. + name: "killed during the split closes the pane it created", + extraEnv: map[string]string{"FAKE_HERDR_SPLIT_HANG": "3"}, + waitFor: "pane split --pane", + wantClose: true, + }, + { + // same window, but SIGINT: the recording trap keeps each signal's own status, + // so this must exit 130. A boolean flag paid as a fixed `exit 143` reports the + // wrong signal here while still closing the pane, which no close assertion sees. + name: "an interrupt during the split reports its own status", + extraEnv: map[string]string{"FAKE_HERDR_SPLIT_HANG": "3"}, + waitFor: "pane split --pane", + sig: syscall.SIGINT, + wantCode: 130, + wantClose: true, + }, + { + // wedged in `pane zoom`, which runs after the split and before any dispatch. + // Nothing can be running in that pane yet, so this is the one state where the + // trap has to destroy what it created -- the half no other case reaches, and + // the reason master carried a textual guard over the trap's close. + name: "killed before any dispatch closes the pane", + extraEnv: map[string]string{"FAKE_HERDR_ZOOM_HANG": "3"}, + waitFor: "pane zoom " + fakeHerdrPaneID, + wantClose: true, + }, + { + // herdr is wedged inside `pane run`: it may or may not have delivered the + // command, so the state is unknown and must not be destroyed. Ownership is + // claimed before the call precisely so this window is preserved. + name: "killed while herdr is wedged preserves the pane", + extraEnv: map[string]string{"FAKE_HERDR_RUN_HANG": "3"}, + waitFor: "pane run " + fakeHerdrPaneID, + wantClose: false, + }, + { + // dispatch was accepted but the pane's shell has not touched the marker yet. + // Unknown state must not be destroyed: this is the window HERDR_DISPATCHED + // covers, and requiring the marker alone would close a review that may exist. + name: "killed after dispatch but before the marker leaves the pane open", + extraEnv: map[string]string{"FAKE_HERDR_NO_SENTINEL": "1"}, + waitFor: "pane run " + fakeHerdrPaneID, + wantClose: false, + }, + { + // `pane run` returns, so the review is live and must outlive us + name: "killed during a live review leaves the pane open", + extraEnv: map[string]string{"FAKE_HERDR_ASYNC": "1", "FAKE_REVDIFF_DELAY": "5"}, + waitForStart: true, + wantClose: false, + }, + } + + for lname, lpath := range launchers { + for _, tc := range tests { + t.Run(lname+"/"+tc.name, func(t *testing.T) { + backend := launcherBackend{name: "herdr", command: "herdr", env: map[string]string{"HERDR_ENV": "1"}} + env := fakeLauncherEnv(t, launcherRun{backend: backend, code: exitCodeAnnotations, output: "x\n"}) + argsFile := filepath.Join(env["TMPDIR"], "herdr-args") + env["FAKE_HERDR_ARGS_FILE"] = argsFile + env["HERDR_PANE_ID"] = fakeHerdrCallerPaneID + env["REVDIFF_HERDR_PANE"] = "1" + env["FAKE_HERDR_PANE"] = "1" + maps.Copy(env, tc.extraEnv) + + cmd := exec.Command("bash", filepath.Join(root, lpath)) //nolint:gosec // fixed repo path + cmd.Dir, cmd.Env = root, mergeEnv(env) + require.NoError(t, cmd.Start()) + + require.Eventually(t, func() bool { + if tc.waitForStart { + // the review is genuinely mid-flight: started, not yet finished + started, _ := filepath.Glob(filepath.Join(env["TMPDIR"], "revdiff-done-*.started")) + done, _ := filepath.Glob(filepath.Join(env["TMPDIR"], "revdiff-done-??????")) + return len(started) > 0 && len(done) == 0 + } + raw, err := os.ReadFile(argsFile) //nolint:gosec // test-owned temp file + return err == nil && strings.Contains(string(raw), tc.waitFor) + }, 15*time.Second, 20*time.Millisecond, "launcher never reached the phase under test") + sig := tc.sig + if sig == 0 { + sig = syscall.SIGTERM + } + require.NoError(t, cmd.Process.Signal(sig)) + // bounded on purpose: the launcher records signals across the windows where + // it owns an unnamed pane, so a restore it forgets would swallow the SIGTERM + // and hang here rather than fail. The longest fake hang is 3s. + done := make(chan struct{}) + var waitErr error + go func() { waitErr = cmd.Wait(); close(done) }() + select { + case <-done: + case <-time.After(30 * time.Second): + _ = cmd.Process.Kill() + t.Fatal("launcher did not exit after SIGTERM") + } + + if tc.wantCode != 0 { + // the recording trap stores the signal's own status, so a recorded INT + // must still report 130 and not the TERM value it is paid alongside + assert.Equal(t, tc.wantCode, commandExitCode(waitErr), "exit status after %v", sig) + } + + raw, err := os.ReadFile(argsFile) //nolint:gosec // test-owned temp file + require.NoError(t, err) + calls := strings.Split(strings.TrimSpace(string(raw)), "\n") + assert.Equal(t, tc.wantClose, countHerdrCalls(calls, "pane close "+fakeHerdrPaneID) > 0, + "pane close after %v; calls=%v", sig, calls) + }) + } + } +} + +// ids the fake herdr backend hands out; the caller pane is what the test injects as +// $HERDR_PANE_ID, so asserting against these pins who each call actually targets +const ( + fakeHerdrCallerPaneID = "w1:p0" // the agent's own pane, must never be operated on + fakeHerdrPaneID = "w1-2" // returned by `pane split` + fakeHerdrTabRootPaneID = "w1-1" // returned by `tab create` +) + +// counts recorded calls matching prefix on a TOKEN boundary, so "pane close w1-2" is not +// satisfied by a call that targeted w1-20. A bare strings.HasPrefix would match it. +func countHerdrCalls(calls []string, prefix string) int { + n := 0 + for _, c := range calls { + if strings.Contains(c, "--help") { + continue + } + if c == prefix || strings.HasPrefix(c, prefix+" ") { + n++ + } + } + return n +} + +// TestHerdrPaneOverlayOptIn covers REVDIFF_HERDR_PANE=1, which splits the caller's own +// herdr pane instead of opening a fullscreen tab. The failure paths carry the weight: +// with the synchronous fake the sentinel exists before the wait loop starts, so the +// liveness probe is only observable in the deferred-sentinel cases. +func TestHerdrPaneOverlayOptIn(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("shell launchers are not used on windows") + } + + root := testRepoRoot(t) + launchers := []struct { + name string + path string + }{ + {name: "claude", path: ".claude-plugin/skills/revdiff/scripts/launch-revdiff.sh"}, + {name: "codex", path: "plugins/codex/skills/revdiff/scripts/launch-revdiff.sh"}, + } + + cases := []struct { + name string + env map[string]string + // wantCode defaults to exitCodeAnnotations when zero + wantCode int + wantTabCreate bool + wantSplit bool // a non---help `pane split` was attempted + wantClose bool + wantZoom bool // the split must be zoomed, not just created + wantGet bool + wantRetriedGet bool + // upper bound on `pane get` calls, to pin the poll backoff. Near-fail-safe: a + // loaded runner makes FEWER polls per second, so slowness alone lowers the count. + // It could only flake the other way if the fake's sentinel timer were delayed + // disproportionately more than the poll loop -- ~4s of one-sided skew. + maxGets int + wantStderr string + notStderr string + // a preserved live review keeps its launch script on purpose: the pane still has + // to execute it, so the usual no-temp-files-left assertion does not apply + keepsScript bool + // lower bound on the launcher's own runtime. Used to pin that the refusal grace was + // actually served: a close proves nothing on its own, since a launcher that skipped + // the wait closes too. Only a lower bound is safe -- a loaded runner can only make + // the run longer, never shorter. + minDuration time.Duration + }{ + { + name: "without the opt-in the review opens a tab", + env: map[string]string{"FAKE_HERDR_PANE": "1"}, + wantTabCreate: true, + }, + { + name: "opt-in splits the caller pane and zooms the review pane", + env: map[string]string{"REVDIFF_HERDR_PANE": "1", "FAKE_HERDR_PANE": "1"}, + wantSplit: true, + wantZoom: true, + wantClose: true, + }, + { + name: "opt-in on an older cli falls back to a tab", + env: map[string]string{"REVDIFF_HERDR_PANE": "1"}, + wantTabCreate: true, + }, + { + // both vars are required: with SPLIT_FAIL alone the probe gate would divert + // this down the older-cli path and no split would ever be attempted + name: "a refused split falls back to a tab", + env: map[string]string{"REVDIFF_HERDR_PANE": "1", "FAKE_HERDR_PANE": "1", "FAKE_HERDR_SPLIT_FAIL": "1"}, + wantSplit: true, + wantTabCreate: true, + }, + { + name: "a deferred sentinel exercises the liveness probe", + env: map[string]string{"REVDIFF_HERDR_PANE": "1", "FAKE_HERDR_PANE": "1", "FAKE_HERDR_ASYNC": "1", "FAKE_REVDIFF_DELAY": "0.5"}, + wantSplit: true, + wantZoom: true, + wantGet: true, + wantClose: true, + }, + { + // pane_not_found is authoritative: stop at once, do not close a pane that is + // provably gone, and do not complain about failing to + name: "a confirmed dead pane stops without closing", + env: map[string]string{"REVDIFF_HERDR_PANE": "1", "FAKE_HERDR_PANE": "1", + "FAKE_HERDR_NO_SENTINEL": "1", "FAKE_HERDR_GET_GONE": "1"}, + wantCode: 1, + wantSplit: true, + wantZoom: true, + wantGet: true, + notStderr: "could not close", + }, + { + // a generic error is not evidence the pane died, and there is no safe deadline + // when the API cannot report liveness: the loop must keep waiting through a + // control-plane outage and still finish the review normally. Sustained failures + // outlast the warn threshold here, then the sentinel lands. A 0.3s poll puts the + // tenth miss at ~3.2s against a 5s sentinel, so the warning lands with roughly + // 1.5x headroom; iterations slower than ~0.5s would flake it (fail, never + // false-pass). After the warn the poll backs off to 2s, so the run totals ~11 + // gets, not the ~16 a flat poll would make. + name: "a control-plane outage does not end a live review", + env: map[string]string{"REVDIFF_HERDR_PANE": "1", "FAKE_HERDR_PANE": "1", + "FAKE_HERDR_GET_FAIL": "1", "FAKE_HERDR_ASYNC": "1", "FAKE_REVDIFF_DELAY": "5"}, + wantSplit: true, + wantZoom: true, + wantGet: true, + wantClose: true, + wantRetriedGet: true, + // ~11 with the 2s backoff after miss 10; a flat 0.3s poll would make ~16 + maxGets: 13, + wantStderr: "still waiting for the review", + }, + { + // the only state where nothing was dispatched: herdr refused and the pane never + // announced itself, so the pane it created is ours to clean up + name: "a refused pane run closes the pane it created", + env: map[string]string{"REVDIFF_HERDR_PANE": "1", "FAKE_HERDR_PANE": "1", + "FAKE_HERDR_RUN_FAIL": "1"}, + wantCode: 1, + wantSplit: true, + wantZoom: true, + wantClose: true, + }, + { + // the refusal grace, signaled deterministically: the injected `sleep` raises + // the signal before sleeping, so it is already pending when the grace sleep + // becomes the foreground command. Launcher-directed, so the grace sleep itself + // survives -- this pins the designed behavior, that a signal mid-grace still + // completes the evidence check and closes an unannounced pane. + name: "a signal during the refusal grace still closes the pane", + env: map[string]string{"REVDIFF_HERDR_PANE": "1", "FAKE_HERDR_PANE": "1", + "FAKE_HERDR_RUN_FAIL": "1", "FAKE_SLEEP_KILL": "parent"}, + wantCode: 1, + wantSplit: true, + wantZoom: true, + wantClose: true, + }, + { + // same, but the signal goes to the process group, so it kills the grace `sleep` + // too. Without the retry the killed sleep returns nonzero, `set -e` aborts + // before the evidence check, and the trap's preserve rule strands the pane. + name: "a process-group signal during the grace still closes the pane", + env: map[string]string{"REVDIFF_HERDR_PANE": "1", "FAKE_HERDR_PANE": "1", + "FAKE_HERDR_RUN_FAIL": "1", "FAKE_SLEEP_KILL": "group"}, + wantCode: 1, + wantSplit: true, + wantZoom: true, + wantClose: true, + }, + { + // every sleep in the grace is killed, so nothing is ever waited out by counting + // sleeps -- but the grace is measured on the wall clock, so the interval is + // served regardless and the absent marker is still real evidence. Pins that no + // amount of signaling can talk the launcher out of serving the grace. + name: "a relentlessly signaled grace is still served", + env: map[string]string{"REVDIFF_HERDR_PANE": "1", "FAKE_HERDR_PANE": "1", + "FAKE_HERDR_RUN_FAIL": "1", "FAKE_SLEEP_KILL": "group-always"}, + wantCode: 1, + wantSplit: true, + wantZoom: true, + wantClose: true, + // the close alone would also pass against a launcher that never waited, so the + // duration is the assertion that matters here + minDuration: 900 * time.Millisecond, + }, + { + // a nonzero `pane run` is not evidence the review did not start -- the response + // can be lost after delivery. The launcher must not close a pane that already + // announced itself, the same rule the wait loop applies to `pane get`. + name: "a lost pane run response does not close a started review", + env: map[string]string{"REVDIFF_HERDR_PANE": "1", "FAKE_HERDR_PANE": "1", + "FAKE_HERDR_RUN_FAIL_AFTER_START": "1", "FAKE_REVDIFF_DELAY": "3"}, + wantCode: 1, + wantSplit: true, + wantZoom: true, + keepsScript: true, + }, + { + // the worst failure this feature could have: adopting the caller's own id and + // closing the agent's terminal. The grep fallback is positional, so a response + // naming the source pane first must be refused. + name: "a split that returns the caller's own pane is refused", + env: map[string]string{"REVDIFF_HERDR_PANE": "1", "FAKE_HERDR_PANE": "1", + "FAKE_HERDR_SPLIT_JSON": `{"result":{"pane":{"pane_id":"` + fakeHerdrCallerPaneID + `"}}}`}, + wantCode: 1, + wantSplit: true, + wantStderr: "stray review pane may remain", + }, + } + + output := "## file.go:1 (+)\ncomment\n" + for _, launcher := range launchers { + for _, tc := range cases { + t.Run(launcher.name+"/"+tc.name, func(t *testing.T) { + backend := launcherBackend{name: "herdr", command: "herdr", env: map[string]string{ + "HERDR_ENV": "1", + }} + env := fakeLauncherEnv(t, launcherRun{backend: backend, code: exitCodeAnnotations, output: output}) + argsFile := filepath.Join(env["TMPDIR"], "herdr-args") + env["FAKE_HERDR_ARGS_FILE"] = argsFile + // the pane gate needs a caller pane id; cleanOverlayEnv blanks it, so + // without this every opt-in case would silently degrade to tab mode + env["HERDR_PANE_ID"] = fakeHerdrCallerPaneID + maps.Copy(env, tc.env) + + started := time.Now() + res := runTestCmd(t, cmdReq{ + dir: root, + name: "bash", + args: []string{filepath.Join(root, launcher.path)}, + env: env, + // the group-kill case signals its own process group; without this the + // signal reaches the test binary instead of the launcher + setpgid: strings.HasPrefix(tc.env["FAKE_SLEEP_KILL"], "group"), + }) + if tc.minDuration > 0 { + assert.GreaterOrEqual(t, time.Since(started), tc.minDuration, + "the refusal grace was not served") + } + + wantCode := tc.wantCode + if wantCode == 0 { + wantCode = exitCodeAnnotations + } + assert.Equal(t, wantCode, res.code) + if wantCode == exitCodeAnnotations { + assert.Equal(t, output, res.stdout) + } + if tc.wantStderr != "" { + assert.Contains(t, res.stderr, tc.wantStderr) + } + if tc.notStderr != "" { + assert.NotContains(t, res.stderr, tc.notStderr) + } + + raw, err := os.ReadFile(argsFile) //nolint:gosec // path is a test-owned temp file + require.NoError(t, err) + calls := strings.Split(strings.TrimSpace(string(raw)), "\n") + + closes, gets := countHerdrCalls(calls, "pane close"), countHerdrCalls(calls, "pane get") + assert.Equal(t, tc.wantSplit, countHerdrCalls(calls, "pane split") > 0, "pane split; calls=%v", calls) + assert.Equal(t, tc.wantTabCreate, countHerdrCalls(calls, "tab create") > 0, "tab create; calls=%v", calls) + assert.Equal(t, tc.wantClose, closes > 0, "pane close; calls=%v", calls) + assert.Equal(t, tc.wantGet, gets > 0, "pane get; calls=%v", calls) + + // teardown is idempotent: the inline call and the EXIT trap's call must not + // both close, and a boolean cannot see a double close + assert.LessOrEqual(t, closes, 1, "pane closed more than once; calls=%v", calls) + if tc.wantTabCreate { + assert.Equal(t, 1, countHerdrCalls(calls, "tab close"), "tab create needs exactly one close; calls=%v", calls) + } + if tc.maxGets > 0 { + assert.LessOrEqual(t, gets, tc.maxGets, + "poll did not back off during the outage; calls=%v", calls) + } + if tc.wantRetriedGet { + // the transient path must retry, not give up on the first miss + assert.Greater(t, gets, 1, "transient failures must be retried; calls=%v", calls) + } + if tc.wantSplit { + // anchored to the caller, or herdr splits whichever pane is focused + assert.Equal(t, 1, countHerdrCalls(calls, "pane split --pane "+fakeHerdrCallerPaneID), "split must anchor to the caller; calls=%v", calls) + } + if tc.wantZoom { + assert.Equal(t, 1, countHerdrCalls(calls, "pane zoom "+fakeHerdrPaneID+" --on"), "the split must be zoomed; calls=%v", calls) + } + if closes > 0 { + // the review pane herdr returned, never the agent's own + assert.Equal(t, 1, countHerdrCalls(calls, "pane close "+fakeHerdrPaneID), "close must target the returned pane; calls=%v", calls) + } + // never-guess: the launcher must not enumerate panes to find one + assert.Zero(t, countHerdrCalls(calls, "pane list"), "must not enumerate panes; calls=%v", calls) + + assert.Empty(t, leftoverStderrCaptures(t, env["TMPDIR"])) + // a launcher that ran to completion owns no temp files: the dispatched script + // removes itself, and the completion path removes it for a pane that died + if !tc.keepsScript { + left, err := filepath.Glob(filepath.Join(env["TMPDIR"], "revdiff-launch-*")) + require.NoError(t, err) + assert.Empty(t, left, "leaked launch script; calls=%v", calls) + } + }) + } + } +} + func TestPlanReviewHookAnnotationExitCodes(t *testing.T) { python := python3Path(t) @@ -1236,6 +1711,9 @@ func runTestCmd(t *testing.T, r cmdReq) cmdResult { cmd := exec.Command(r.name, r.args...) //nolint:gosec // tests execute fixed repo scripts and temp fixtures cmd.Dir = r.dir cmd.Env = mergeEnv(r.env) + if r.setpgid { + cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} + } if r.stdin != "" { cmd.Stdin = strings.NewReader(r.stdin) } @@ -1338,6 +1816,30 @@ func fakeLauncherEnv(t *testing.T, r launcherRun) map[string]string { binDir := filepath.Join(tmp, "bin") writeExecutable(t, filepath.Join(binDir, "revdiff"), testFixtureScript(t, "fake-revdiff-output.sh")) writeExecutable(t, filepath.Join(binDir, r.backend.command), testFixtureScript(t, "fake-overlay-backend.sh")) + // A `sleep` that signals before it sleeps. Used to reach the refusal path's 300ms + // evidence grace deterministically: the signal is already pending when the grace sleep + // becomes the foreground command, so there is no window to race. FAKE_SLEEP_KILL picks + // the target -- "parent" is a launcher-directed signal, "group" emulates Ctrl-C, which + // kills this sleep too, and "group-always" keeps doing it so every retry dies. + // + // The once-marker makes "group" hit only the FIRST sleep of the run, which on the + // refusal path is the grace. That assumption is load-bearing: combining FAKE_SLEEP_KILL + // with anything that sleeps earlier (a backend with its own wait loop, or + // FAKE_REVDIFF_DELAY) spends the marker on the wrong sleep and the case passes while + // testing nothing. Keep these cases on the refusal path. + // + // The marker is written BEFORE the kill so the retry that follows runs a real sleep. + writeExecutable(t, filepath.Join(binDir, "sleep"), `#!/bin/sh +if [ -n "${FAKE_SLEEP_KILL:-}" ] && + { [ "$FAKE_SLEEP_KILL" = group-always ] || [ ! -f "$TMPDIR/sleep-killed" ]; }; then + : > "$TMPDIR/sleep-killed" + case "$FAKE_SLEEP_KILL" in + group | group-always) kill -TERM 0 ;; + *) kill -TERM "$PPID" ;; + esac +fi +exec /bin/sleep "$@" +`) env := cleanOverlayEnv() maps.Copy(env, r.backend.env) @@ -1356,6 +1858,8 @@ func cleanOverlayEnv() map[string]string { "HERDR_ENV": "", "HERDR_SOCKET_PATH": "", "HERDR_PANE_ID": "", + "HERDR_WORKSPACE_ID": "", + "REVDIFF_HERDR_PANE": "", "KITTY_LISTEN_ON": "", "KITTY_WINDOW_ID": "", "WEZTERM_PANE": "", diff --git a/app/testdata/plugin-exit-code/fake-overlay-backend.sh b/app/testdata/plugin-exit-code/fake-overlay-backend.sh index fb0391e5..f4e82c58 100644 --- a/app/testdata/plugin-exit-code/fake-overlay-backend.sh +++ b/app/testdata/plugin-exit-code/fake-overlay-backend.sh @@ -101,6 +101,10 @@ case "$cmd_name" in exit 0 ;; herdr) + # record every invocation so tests can assert the exact call sequence + if [ -n "${FAKE_HERDR_ARGS_FILE:-}" ]; then + echo "$*" >> "$FAKE_HERDR_ARGS_FILE" + fi case "${1:-} ${2:-}" in "tab create") # ids satisfy both the jq path (.result.tab.tab_id / @@ -111,10 +115,92 @@ case "$cmd_name" in "tab close") exit 0 ;; + "pane split") + # the --direction feature probe; FAKE_HERDR_PANE=1 fakes a CLI new + # enough to carry pane mode + if [ "${3:-}" = "--help" ]; then + if [ "${FAKE_HERDR_PANE:-}" = "1" ]; then + echo " --direction [possible values: right, down]" + exit 0 + fi + exit 1 + fi + [ "${FAKE_HERDR_SPLIT_FAIL:-}" = "1" ] && { echo "herdr: split refused" >&2; exit 1; } + # malformed payload: exercises the never-guess bail-out + if [ -n "${FAKE_HERDR_SPLIT_JSON:-}" ]; then + echo "$FAKE_HERDR_SPLIT_JSON" + exit 0 + fi + # HANG wedges the launcher inside the split itself. The call is already + # recorded above, so a test can signal deterministically while the pane is + # being created and the launcher cannot yet name it. + [ -n "${FAKE_HERDR_SPLIT_HANG:-}" ] && sleep "$FAKE_HERDR_SPLIT_HANG" + echo '{"result":{"pane":{"pane_id":"w1-2"}}}' + ;; + "pane get") + if [ "${3:-}" = "--help" ]; then + [ "${FAKE_HERDR_PANE:-}" = "1" ] && exit 0 + exit 1 + fi + # GONE is authoritative death; FAIL is a generic/transient error. The + # launcher must treat these differently, so the fake must too. + if [ "${FAKE_HERDR_GET_GONE:-}" = "1" ]; then + echo '{"error":{"code":"pane_not_found","message":"pane w1-2 not found"},"id":"cli:pane:get"}' + exit 1 + fi + [ "${FAKE_HERDR_GET_FAIL:-}" = "1" ] && { echo "herdr: connection reset" >&2; exit 1; } + echo '{"result":{"pane":{"pane_id":"w1-2"}}}' + ;; + "pane close") + if [ "${3:-}" = "--help" ]; then + [ "${FAKE_HERDR_PANE:-}" = "1" ] && exit 0 + exit 1 + fi + exit 0 + ;; + "pane zoom") + # HANG wedges the launcher after the pane exists but before anything is + # dispatched into it -- the window where a signal must reach the trap's + # close. Seconds, so the SIGTERM (milliseconds away) always lands inside it. + [ -n "${FAKE_HERDR_ZOOM_HANG:-}" ] && sleep "$FAKE_HERDR_ZOOM_HANG" + exit 0 + ;; "pane run") # herdr pane run ; run the launch command and # report success, mimicking herdr's fire-and-forget (the real rc # still arrives via the sentinel the launch script writes) + # herdr refuses outright: nothing was dispatched, and nothing announced + [ "${FAKE_HERDR_RUN_FAIL:-}" = "1" ] && exit 1 + # hang and never dispatch, so a test can signal the launcher while the pane + # exists but holds nothing but a shell. It must NOT fall through to the eval: + # a trapped TERM is deferred until this foreground call returns, so dispatching + # afterwards would let the marker appear and turn the case into a + # finished-review test. + if [ -n "${FAKE_HERDR_RUN_HANG:-}" ]; then + sleep "$FAKE_HERDR_RUN_HANG" + exit 0 + fi + # the review really starts, but the response is lost: a nonzero return that + # is NOT evidence the review failed to start + if [ "${FAKE_HERDR_RUN_FAIL_AFTER_START:-}" = "1" ]; then + # background it so the review is still RUNNING when the response is + # lost: delivered, announced, response lost. The marker beats the + # launcher's check because that path waits 0.3s before deciding, which + # is far longer than an `sh` spawn -- no head start is needed here. + ( eval "${4:-}" || true ) >/dev/null 2>&1 & + exit 1 + fi + if [ "${FAKE_HERDR_NO_SENTINEL:-}" = "1" ]; then + # model a pane that dies without ever writing the sentinel + exit 0 + fi + if [ "${FAKE_HERDR_ASYNC:-}" = "1" ]; then + # dispatch immediately, like herdr does, and let the review itself take + # time (FAKE_REVDIFF_DELAY). That is what makes the wait loop iterate and + # leaves a genuine "started but not finished" window for signal tests. + ( eval "${4:-}" || true ) >/dev/null 2>&1 & + exit 0 + fi eval "${4:-}" || true exit 0 ;; diff --git a/app/testdata/plugin-exit-code/fake-revdiff-output.sh b/app/testdata/plugin-exit-code/fake-revdiff-output.sh index 11659a6b..52d67aa4 100644 --- a/app/testdata/plugin-exit-code/fake-revdiff-output.sh +++ b/app/testdata/plugin-exit-code/fake-revdiff-output.sh @@ -8,6 +8,10 @@ fi if [ -n "${FAKE_STDERR:-}" ]; then printf "%s" "$FAKE_STDERR" >&2 fi +# lets a test hold a review open: the script has started, the sentinel is not yet written +if [ -n "${FAKE_REVDIFF_DELAY:-}" ]; then + sleep "$FAKE_REVDIFF_DELAY" +fi out="" for arg in "$@"; do case "$arg" in diff --git a/plugins/codex/skills/revdiff/SKILL.md b/plugins/codex/skills/revdiff/SKILL.md index 8c4736c5..5a4340bc 100644 --- a/plugins/codex/skills/revdiff/SKILL.md +++ b/plugins/codex/skills/revdiff/SKILL.md @@ -150,6 +150,8 @@ $SCRIPT_DIR/launch-revdiff.sh [base] [against] [--staged] [--untracked] [--only= **Disconnect-resilient tmux window mode**: when running under tmux, prefix the launcher with `REVDIFF_TMUX_WINDOW=1` to open revdiff in a persistent, server-owned tmux window instead of a client-owned `display-popup`. The review then survives a dropped SSH or tmux client — reattach and it is still there. This is a launcher environment variable, not a revdiff flag. +**Pane-scoped overlay (herdr)**: when running under herdr, `REVDIFF_HERDR_PANE=1` opens revdiff in a zoomed split of the agent's own pane instead of a new fullscreen tab, keeping the agent pane one keypress away. The user sets it in the environment; it falls back to the tab overlay on an older herdr CLI. This is a launcher environment variable, not a revdiff flag. + **Pane-scoped overlay (agterm)**: when running in an agterm split, `REVDIFF_AGTERM_PANE=1` opens revdiff in the agent's own pane instead of over the whole session, leaving the sibling pane live and visible. The user sets it in the environment; it is ignored outside a split. This is a launcher environment variable, not a revdiff flag. The script: diff --git a/plugins/codex/skills/revdiff/scripts/launch-revdiff.sh b/plugins/codex/skills/revdiff/scripts/launch-revdiff.sh index 1cc05c5b..f444acf5 100755 --- a/plugins/codex/skills/revdiff/scripts/launch-revdiff.sh +++ b/plugins/codex/skills/revdiff/scripts/launch-revdiff.sh @@ -290,19 +290,232 @@ fi # herdr: open a new fullscreen tab via the herdr CLI (must precede kitty — # inside herdr-in-kitty KITTY_LISTEN_ON is set, so the kitty branch would -# otherwise win and open an overlay window herdr cannot composite into its panes) +# otherwise win and open an overlay window herdr cannot composite into its panes). +# REVDIFF_HERDR_PANE=1 instead runs the review in a zoomed split of the agent's +# own pane, which keeps that pane one keypress away; it is self-contained and +# never falls through to the tab path below except when the split is declined. if [ "${HERDR_ENV:-}" = "1" ] && command -v herdr >/dev/null 2>&1; then + # $HERDR_PANE_ID is injected by herdr into every managed pane, and the tab path + # below reuses that name for the pane it creates — copy the caller's id out first + HERDR_CALLER_PANE="${HERDR_PANE_ID:-}" + # non-empty once we own a review pane that has to be closed on every exit path + HERDR_TARGET="" + # the pane's own shell touches this the instant it starts the launch script, so + # ownership is decided by evidence FROM the pane rather than by `pane run` returning: + # herdr may have started the review before the CLI call returns, and may not have + # started it after. Assigned once $SENTINEL exists, below. `touch ... || true` and not + # `: >`, because a redirection failure on a special builtin kills a POSIX shell outright, + # which would stop revdiff running at all; a $TMPBASE the pane cannot write already + # breaks the sentinel the same way, for the tab path too. + HERDR_STARTED="" + # set BEFORE `pane run` and never cleared again, so the in-flight window is owned + # rather than ambiguous: a pending signal is serviced the instant the call returns, + # before any later assignment could run. The marker is still needed for the failure + # path, where it is the only evidence that a refused-looking dispatch actually started. + HERDR_DISPATCHED=0 + + # all three are used at runtime; a CLI lacking `get` would read every liveness poll + # as a failure and abandon a live review. zoom stays unprobed: it is purely cosmetic. + herdr_supports_pane_mode() { + herdr pane split --help 2>/dev/null | grep -q -- '--direction' \ + && herdr pane get --help >/dev/null 2>&1 \ + && herdr pane close --help >/dev/null 2>&1 + } + + # idempotent, and clears HERDR_TARGET before shelling out so a signal during the + # close cannot re-enter through the EXIT trap and close the same id twice + herdr_close_pane() { + local target="$HERDR_TARGET" + HERDR_TARGET="" + [ -n "$target" ] || return 0 + if ! herdr pane close "$target" >/dev/null 2>&1; then + herdr pane zoom "$target" --off >/dev/null 2>&1 || true + printf 'revdiff: could not close herdr review pane %s\n' "$target" >&2 + fi + return 0 + } + + # EXIT-trap cleanup. A review that has STARTED but not FINISHED belongs to the user: + # SKILL.md promises the driving agent that a launcher killed on timeout leaves revdiff + # open with nothing lost, so we must not close it and must not delete the script it is + # running. Anything else is ours -- a pane that never started one (close it, it holds + # only a shell) or one that already finished (close it, it is done). + # shellcheck disable=SC2317,SC2329 # invoked from the EXIT trap below, which shellcheck cannot + # see. both codes: shellcheck <0.11 reports this as SC2317, 0.11+ as SC2329 + herdr_cleanup_unlaunched() { + # pane mode only: the tab path never sets HERDR_TARGET, so its launch-script + # lifecycle stays exactly what master does here and what every other + # script-using backend does -- the trap removes it, unconditionally + if [ -n "$HERDR_TARGET" ] && [ ! -f "$SENTINEL" ] && + { [ -f "$HERDR_STARTED" ] || [ "$HERDR_DISPATCHED" = 1 ]; }; then + return 0 + fi + herdr_close_pane + rm -f "$LAUNCH_SCRIPT" "$HERDR_STARTED" + return 0 + } + SENTINEL=$(mktemp "$TMPBASE/revdiff-done-XXXXXX") rm -f "$SENTINEL" + HERDR_STARTED="$SENTINEL.started" LAUNCH_SCRIPT=$(mktemp "$TMPBASE/revdiff-launch-XXXXXX") - trap 'rm -f "$OUTPUT_FILE" "$ERR_FILE" "$SENTINEL" "$SENTINEL.tmp" "$LAUNCH_SCRIPT"' EXIT + trap 'herdr_cleanup_unlaunched || true; rm -f "$OUTPUT_FILE" "$ERR_FILE" "$SENTINEL" "$SENTINEL.tmp" "$HERDR_STARTED"' EXIT + # same shape as the agterm branch above: INT/TERM exit through the EXIT trap so a + # signal never skips cleanup or leaves an unnormalised status + trap 'exit 130' INT + trap 'exit 143' TERM cat > "$LAUNCH_SCRIPT" </dev/null || true $(write_rc_cmd "$SENTINEL") +rm -f "\$0" LAUNCHER chmod +x "$LAUNCH_SCRIPT" + if [ "${REVDIFF_HERDR_PANE:-}" = 1 ] && [ -n "$HERDR_CALLER_PANE" ] && herdr_supports_pane_mode; then + # A trapped signal is deferred until the in-flight foreground command returns, then + # runs BEFORE the next statement -- so `exit 143` during the split would fire after + # the pane exists but before HERDR_TARGET names it, and the EXIT trap would have + # nothing to close. Record the signal instead of acting on it, and honor it once + # ownership is held. This is a trap with a COMMAND, which children reset to default; + # `trap '' INT TERM` would be SIG_IGN, inherited across exec, and would make a hung + # herdr unkillable (see CLAUDE.md). It also adds no deferral that `exit 143` did not + # already have: both wait out the same wedged split. + HERDR_SIGNALLED=0 + trap 'HERDR_SIGNALLED=130' INT + trap 'HERDR_SIGNALLED=143' TERM + if HERDR_NEW=$(herdr pane split --pane "$HERDR_CALLER_PANE" --direction right \ + --cwd "$CWD" --focus 2>&1); then + if command -v jq >/dev/null 2>&1; then + HERDR_TARGET=$(printf '%s' "$HERDR_NEW" | jq -r '.result.pane.pane_id // empty' 2>/dev/null || true) + fi + if [ -z "$HERDR_TARGET" ]; then + HERDR_TARGET=$(printf '%s' "$HERDR_NEW" | grep -o '"pane_id":"[^"]*"' | head -1 | cut -d'"' -f4 || true) + fi + # the grep fallback is positional, so a response listing the source pane first + # would hand us the caller's own id — closing that would kill the agent's pane + if [ "$HERDR_TARGET" = "$HERDR_CALLER_PANE" ]; then + HERDR_TARGET="" + fi + if [ -z "$HERDR_TARGET" ]; then + # never guess: a pane found by diffing `pane list` may belong to another + # herdr client, and closing that would destroy the user's own work + printf 'revdiff: herdr pane split returned no pane id; a stray review pane may remain: %s\n' \ + "$HERDR_NEW" >&2 + exit 1 + fi + # ownership is held: back to exiting traps, and pay any signal deferred above + # through the EXIT trap, which can now close the pane it created + trap 'exit 130' INT + trap 'exit 143' TERM + [ "$HERDR_SIGNALLED" != 0 ] && exit "$HERDR_SIGNALLED" + herdr pane zoom "$HERDR_TARGET" --on >/dev/null 2>&1 || true + # claim ownership BEFORE dispatching. A pending signal is serviced the moment + # this call returns and before any assignment after it, so ownership taken + # afterwards would miss a dispatch herdr had already accepted. Never cleared + # again -- the refusal path below says why restoring a reset here is unsafe. + HERDR_DISPATCHED=1 + if ! herdr pane run "$HERDR_TARGET" "sh $(sq "$LAUNCH_SCRIPT")" >/dev/null 2>&1; then + # this branch does trust the return code, unlike the trap above: a lost + # response after a successful dispatch would close a just-started review, + # but stranding a pane on every genuine dispatch failure is the worse default + echo "error: herdr pane run failed for pane $HERDR_TARGET" >&2 + # herdr refused the command, so nothing was dispatched -- unless the pane + # announced itself anyway, which would mean the response was lost after + # delivery. Give that a moment to show up, then close only a pane that + # never announced. Paid on the failure path only. + # HERDR_DISPATCHED deliberately stays 1 across the grace: the trap reads it, + # so clearing it here would let a signal landing inside the sleep close the + # pane before the marker had its chance to appear -- the exact unknown-state + # destruction claim-before-dispatch exists to prevent. Clearing it after the + # sleep would be dead anyway, since herdr_close_pane discharges ownership by + # emptying HERDR_TARGET, which is what the trap gates on. + # + # The grace must therefore RUN TO COMPLETION even while being signalled: any + # exit taken inside it hands the trap a state it must read as "may be live" + # and preserve, stranding a pane the finished evidence check would close. + # Two things are needed for that, and neither alone is enough: + # - record the signal instead of exiting on it, as the split window does; + # - survive the sleep itself being killed. A process-group signal + # (interactive Ctrl-C, `kill -- -pgid`) hits the foreground `sleep` too, + # which then returns nonzero and lets `set -e` abort the script before + # the evidence check -- resurrecting the strand despite the trap. + # The grace is therefore measured in ELAPSED TIME, not in completed sleeps: + # $SECONDS is set by the shell from the wall clock, so a killed `sleep` costs + # an early wakeup and nothing else, and the interval is served no matter how + # many signals arrive. Counting sleeps instead can end with zero time + # elapsed, leaving an absent marker that proves nothing. `|| true` keeps an + # interrupted sleep from tripping errexit; the loop body cannot fail + # otherwise, so it cannot re-arm it either. The marker check is in the + # condition, so the common path leaves as soon as the pane announces itself + # rather than always paying the full interval. + # + # `-lt 2`, not `-lt 1`: $SECONDS counts whole-second boundaries crossed since + # the assignment, so it can reach 1 a millisecond later if the assignment + # lands just before a boundary. Waiting for the second boundary guarantees a + # full second was served; the cost is up to about two seconds, on a failure path only. + HERDR_SIGNALLED=0 + trap 'HERDR_SIGNALLED=130' INT + trap 'HERDR_SIGNALLED=143' TERM + SECONDS=0 + while [ ! -f "$HERDR_STARTED" ] && [ "$SECONDS" -lt 2 ]; do + sleep 0.3 || true + done + # the interval was served, so an absent marker is real evidence + [ -f "$HERDR_STARTED" ] || herdr_close_pane + trap 'exit 130' INT + trap 'exit 143' TERM + # the refusal is why we are exiting, so it owns the status; a signal that + # arrived during the grace has already been honored by the close above + exit 1 + fi + HERDR_MISSES=0 + HERDR_POLL=0.3 + while [ ! -f "$SENTINEL" ]; do + if HERDR_GET=$(herdr pane get "$HERDR_TARGET" 2>&1); then + HERDR_MISSES=0 + HERDR_POLL=0.3 + elif printf '%s' "$HERDR_GET" | grep -q '"code":"pane_not_found"'; then + # authoritative: the pane is gone, so ownership is already discharged -- + # nothing to close, and nothing to complain about + HERDR_TARGET="" + break + else + # a generic error is NOT proof of death (socket hiccup, server restart), + # and there is no safe deadline when the API cannot report liveness -- + # any bound turns unknown liveness into a closed live review. Warn once + # per outage and back off, then keep waiting, exactly as the tab path + # below does for a human review. Both reset on a good poll. + HERDR_MISSES=$((HERDR_MISSES + 1)) + if [ "$HERDR_MISSES" = 10 ]; then + printf 'revdiff: herdr pane get keeps failing, still waiting for the review: %s\n' \ + "$HERDR_GET" >&2 + # stop hammering a control plane that is down; the sentinel is still + # checked every iteration, so finishing costs at most one interval + HERDR_POLL=2 + fi + fi + sleep "$HERDR_POLL" + done + rc=$(read_rc "$SENTINEL") + # the review is over either way: the script ran (and removed itself), or the + # pane died before it could and nothing will ever open it now. Only a launcher + # killed mid-review leaves it behind, which is deliberate -- the pane may still + # be about to open it. + rm -f "$LAUNCH_SCRIPT" "$HERDR_STARTED" + herdr_close_pane + print_output_and_exit "${rc:-1}" + fi + # nothing was created — fall through to the tab path. Restore the exiting traps + # first: the recording trap must not survive into a path with no pane to protect, + # or a signal there would be swallowed instead of ending the launcher. + trap 'exit 130' INT + trap 'exit 143' TERM + [ "$HERDR_SIGNALLED" != 0 ] && exit "$HERDR_SIGNALLED" + printf 'revdiff: herdr pane split declined, using tab overlay: %s\n' "$HERDR_NEW" >&2 + fi + # pin the tab to the caller's workspace: without --workspace, herdr tab create # targets the server's focused workspace (what the user is currently viewing), # not the caller's workspace diff --git a/site/docs.html b/site/docs.html index f0431f22..05b3c53b 100644 --- a/site/docs.html +++ b/site/docs.html @@ -702,7 +702,7 @@

Terminal support

agtermagtermctl session overlay open … --block (full-pane overlay, blocks until quit)$AGTERM_SESSION_ID tmuxdisplay-popup (blocks until quit)$TMUX zellijzellij run --floating$ZELLIJ env var - herdrherdr tab create + herdr pane run (new tab)$HERDR_ENV + herdrherdr tab create + herdr pane run (new tab), or a zoomed herdr pane split with REVDIFF_HERDR_PANE=1$HERDR_ENV kittykitty @ launch --type=overlay$KITTY_LISTEN_ON weztermwezterm cli split-pane$WEZTERM_PANE kakukaku cli split-pane (same API as wezterm)$WEZTERM_PANE @@ -717,6 +717,7 @@

Terminal support

{ "permissions": { "excludedCommands": ["*/launch-revdiff.sh*"] } }

Terminals that use CLI tools instead of AppleScript (agterm, tmux, Zellij, herdr, kitty, wezterm, Kaku, cmux) are not affected.

Disconnect-resilient tmux window mode: set REVDIFF_TMUX_WINDOW=1 in the launcher's environment to open revdiff in a persistent, server-owned tmux window instead of a client-owned display-popup. A dropped SSH or tmux client tears down a popup and kills the review, but a server-owned window survives the disconnect — reattach and the live review is still there. This is a launcher environment variable, not a revdiff flag.

+

Pane-scoped overlay (herdr): set REVDIFF_HERDR_PANE=1 in the launcher's environment to open revdiff in a zoomed split of the agent's own herdr pane instead of a new fullscreen tab, so the agent pane stays one keypress away. It needs a herdr whose CLI carries pane split, pane get and pane close; an unsupported CLI or a refused split falls back to the tab overlay; a split that succeeds but returns no usable pane id fails closed with a warning rather than opening a second surface, and may leave a stray pane to close by hand. This is a launcher environment variable, not a revdiff flag.

Pane-scoped overlay (agterm): set REVDIFF_AGTERM_PANE=1 in the launcher's environment to open revdiff in the agent's own split pane instead of over the whole session, leaving the sibling pane live and visible. It applies only when that session is split — the session-wide overlay stands otherwise, and the launcher retries session-wide if agterm refuses the pane. The review gets pane width rather than session width, which is why it is opt-in. This is a launcher environment variable, not a revdiff flag.

Plugin usage

diff --git a/site/index.html b/site/index.html index b620395e..617b3e6c 100644 --- a/site/index.html +++ b/site/index.html @@ -317,7 +317,7 @@

Works with your terminal

herdr
-
New tab
+
New tab or split pane
kitty