Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions .claude-plugin/skills/revdiff/scripts/launch-revdiff.sh
Original file line number Diff line number Diff line change
Expand Up @@ -540,10 +540,15 @@ on run argv
set newSession to split horizontally with same profile command cmd
end if
end tell
-- the tab label comes from its active session's name,
-- and the split gets none of its own: it copies the
-- parent's profile but not the session variables that
-- profile's name may interpolate
-- the tab label comes from the name of its active
-- session, and the split gets none of its own: it
-- copies the parent profile but not the session
-- variables that the profile name may interpolate.
-- keep this comment free of apostrophes: bash 3.2
-- scans the enclosing command substitution for quotes
-- before it processes the heredoc, so an odd count
-- here opens a quote that never closes and the whole
-- script fails to parse
set name of newSession to overlayTitle
return id of newSession
end if
Expand Down
1 change: 1 addition & 0 deletions CLAUDE.md
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@ TUI for reviewing diffs, files, and documents with inline annotations, built wit
- `config.md` — options, colors, chroma styles
- `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.
- **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/<launcher>` under Claude and `${PLUGIN_DATA}/scripts/<launcher>` 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.
- **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.
Expand Down
67 changes: 67 additions & 0 deletions app/plugin_exit_code_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"os"
"os/exec"
"path/filepath"
"regexp"
"runtime"
"strconv"
"strings"
Expand Down Expand Up @@ -177,6 +178,72 @@ func TestShellLaunchersPreserveAnnotationExitCode(t *testing.T) {
}
}

// pins #314: an apostrophe in a heredoc nested inside a command substitution
// breaks the whole launcher under bash 3.2, the stock macOS /bin/bash. the
// launchers cannot be parse-checked for it here — CI and most dev machines run
// bash 5, which accepts the broken form — so the guard is textual and portable.
func TestLauncherNestedHeredocsHaveNoApostrophes(t *testing.T) {
root := testRepoRoot(t)
paths := []string{
".claude-plugin/skills/revdiff/scripts/launch-revdiff.sh",
"plugins/codex/skills/revdiff/scripts/launch-revdiff.sh",
"plugins/revdiff-planning/scripts/launch-plan-review.sh",
}

for _, path := range paths {
t.Run(path, func(t *testing.T) {
bodies := nestedHeredocBodies(readRepoFile(t, root, path))
require.NotEmpty(t, bodies, "no command-substitution heredoc found; the scan no longer matches the script")
for _, hd := range bodies {
for _, line := range hd.lines {
assert.NotContains(t, line.text, "'",
"%s:%d is inside the heredoc opened at line %d, which bash 3.2 scans for quotes as part of "+
"the enclosing $( ); an apostrophe here fails the whole script. reword to avoid it",
path, line.num, hd.openedAt)
}
}
})
}
}

type heredocLine struct {
text string
num int
}

type nestedHeredoc struct {
lines []heredocLine
openedAt int
}

// nestedHeredocBodies returns the body of every heredoc opened on a line that also
// opens a command substitution. a heredoc outside one is unaffected by the bash 3.2
// scan, so including it would ban apostrophes the shell handles correctly.
func nestedHeredocBodies(script string) []nestedHeredoc {
opener := regexp.MustCompile(`\$\(.*<<-?\s*'?([A-Za-z_][A-Za-z0-9_]*)'?`)
var found []nestedHeredoc
var current *nestedHeredoc
var terminator string

for i, line := range strings.Split(script, "\n") {
num := i + 1
if current != nil {
if strings.TrimSpace(line) == terminator {
found = append(found, *current)
current = nil
} else {
current.lines = append(current.lines, heredocLine{num: num, text: line})
}
continue
}
if m := opener.FindStringSubmatch(line); m != nil {
current = &nestedHeredoc{openedAt: num}
terminator = m[1]
}
}
return found
}

// every other backend labels its overlay through a flag the exit-code matrix
// already runs (tmux -T, kitty --title); iTerm2 names the session it splits from
// an AppleScript argv, which nothing else in the suite reads
Expand Down
13 changes: 9 additions & 4 deletions plugins/codex/skills/revdiff/scripts/launch-revdiff.sh
Original file line number Diff line number Diff line change
Expand Up @@ -541,10 +541,15 @@ on run argv
set newSession to split horizontally with same profile command cmd
end if
end tell
-- the tab label comes from its active session's name,
-- and the split gets none of its own: it copies the
-- parent's profile but not the session variables that
-- profile's name may interpolate
-- the tab label comes from the name of its active
-- session, and the split gets none of its own: it
-- copies the parent profile but not the session
-- variables that the profile name may interpolate.
-- keep this comment free of apostrophes: bash 3.2
-- scans the enclosing command substitution for quotes
-- before it processes the heredoc, so an odd count
-- here opens a quote that never closes and the whole
-- script fails to parse
set name of newSession to overlayTitle
return id of newSession
end if
Expand Down
13 changes: 9 additions & 4 deletions plugins/revdiff-planning/scripts/launch-plan-review.sh
Original file line number Diff line number Diff line change
Expand Up @@ -418,10 +418,15 @@ on run argv
set newSession to split horizontally with same profile command cmd
end if
end tell
-- the tab label comes from its active session's name,
-- and the split gets none of its own: it copies the
-- parent's profile but not the session variables that
-- profile's name may interpolate
-- the tab label comes from the name of its active
-- session, and the split gets none of its own: it
-- copies the parent profile but not the session
-- variables that the profile name may interpolate.
-- keep this comment free of apostrophes: bash 3.2
-- scans the enclosing command substitution for quotes
-- before it processes the heredoc, so an odd count
-- here opens a quote that never closes and the whole
-- script fails to parse
set name of newSession to overlayTitle
return id of newSession
end if
Expand Down