fix(launchers): keep the nested AppleScript heredocs parsable under bash 3.2 - #315
Merged
Conversation
The comment added beside the overlay-title fix carries three apostrophes. bash 3.2 scans a command substitution for quotes before it processes a heredoc nested inside it, so the odd count opens a quote that never closes and the whole script fails to parse — reported about a hundred lines later, in the emacs branch, on a line that is valid. On stock macOS `#!/usr/bin/env bash` resolves to /bin/bash 3.2.57, so every plugin-launched review exits without running revdiff for any user without a newer bash on PATH. CI does not see it: ubuntu and Homebrew bash both parse it fine, and TestShellLaunchersPreserveAnnotationExitCode only fails when PATH bash is 3.2. Rewords all three copies to avoid apostrophes rather than balancing them — balancing leaves the next one-word edit free to break it again — and states the constraint in the comment itself so it is visible at the point of edit. Related to #314
The reworded comment has nothing pinning it: no test greps the AppleScript heredoc bodies, the launcher tests invoke the scripts through PATH bash (5.x on CI and on any machine with Homebrew bash), and shellcheck accepts the broken form. Reintroducing "session's" would go green everywhere and ship a launcher that fails to start for stock-macOS users. A parse check cannot guard it in CI, but a textual one can and is portable: scan each launcher for heredocs opened inside a command substitution — the two osascript captures per file, not the plain close-pane heredocs beside them — and assert their bodies carry no apostrophe. Also narrows the CLAUDE.md bullet, which claimed the hazard covers every AppleScript heredoc in the iTerm2 and Ghostty branches. Only the nested pair is affected; measured under bash 3.2, the same apostrophe in a plain heredoc parses fine. Related to #314
There was a problem hiding this comment.
Pull request overview
Fixes a macOS compatibility regression where the plugin launcher scripts fail to parse under the stock /bin/bash (bash 3.2) due to apostrophes inside AppleScript heredocs nested in $(...) command substitutions, and adds a portable regression guard to prevent reintroduction.
Changes:
- Reword the nested AppleScript heredoc comment in all launcher script copies to remove apostrophes and explicitly document the constraint inline.
- Add a Go test that textually scans for heredocs opened on lines that also open
$(...)and asserts their bodies contain no apostrophes (portable across CI bash versions). - Document the bash 3.2 nested-heredoc parsing constraint in
CLAUDE.md.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
plugins/revdiff-planning/scripts/launch-plan-review.sh |
Removes apostrophes from the nested AppleScript heredoc comment and documents the bash 3.2 constraint inline. |
plugins/codex/skills/revdiff/scripts/launch-revdiff.sh |
Same nested AppleScript heredoc comment rewording to keep bash 3.2 parsing intact. |
.claude-plugin/skills/revdiff/scripts/launch-revdiff.sh |
Same nested AppleScript heredoc comment rewording to keep bash 3.2 parsing intact. |
app/plugin_exit_code_test.go |
Adds TestLauncherNestedHeredocsHaveNoApostrophes and a small scanner to prevent regressions without relying on bash 3.2 in CI. |
CLAUDE.md |
Documents the bash 3.2 heredoc-in-$(...) apostrophe constraint and the rationale for the textual guard. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
all three launcher scripts fail to parse under bash 3.2, the bash macOS ships as
/bin/bash, so every plugin-launched review exits without running revdiff for anyone whose PATH bash is the stock one.the AppleScript comment added by fad239e carries three apostrophes (
session's,parent's,profile's). That heredoc sits inside a$( )command substitution, and bash 3.2 scans the substitution for quotes before it processes the heredoc, so an odd count opens a quote that never closes. The error surfaces about a hundred lines later, in the emacs branch, on a line that is perfectly valid:reworded all three copies to avoid apostrophes rather than balance them. Two apostrophes pair and the file parses again, so a partial fix looks like a working fix, and the same one-word edit later would break it again. The constraint is stated in the comment itself.
regression guard. no parse check can catch this on CI: ubuntu bash and every Homebrew bash accept the broken form, and the launcher tests invoke the scripts through PATH bash.
TestLauncherNestedHeredocsHaveNoApostrophesguards it textually instead, scanning each launcher for heredocs opened inside a command substitution and asserting their bodies carry no apostrophe. Verified to fail whensession'sis put back.also narrowed the CLAUDE.md note. Only the two
$( )-nested osascript captures per launcher are affected; the close-pane heredocs beside them are plain commands and parse fine with an apostrophe, measured under 3.2.verification:
/bin/bash -nexits 0 on all three, was 2. With bash 3.2 forced onto PATHTestShellLaunchersPreserveAnnotationExitCodepasses, and fails without the fix with the reported error.reported by @p4elkin with the diagnosis and the fix already worked out.
Related to #314