[sonichi]

What changed, and why

Two follow-ups from rui's approving review of #4908 (issue #4949), in skills/claude-gemini/scripts/agy-browser.sh:

1. A signal between the EXIT trap and the launcher fork. on_exit ran with no launcher, kept the handshake dir and said the launcher would read $HS/cancel — nothing ever would. $! is set at the fork, before $launcher is assigned, so on_exit now reads ${launcher:-${!:-}}: empty means no launcher exists, and the dir is removed with no message. A forked-but-not-yet-reported launcher keeps the existing cancel-file path. (${!:-}, not $!: the script runs set -u.)
2. The failing-ps branch of group_gone is now tested. No code change there; the branch already returned "not gone" on a failed or empty ps -Ao pgid=,stat=.

Before/after

The on_exit unit trio, extracted from the script and driven in bash (none = no launcher forked):

# parent (origin/main f60ac4a02):
  parent/none: FAIL (agy-browser.sh: Chrome's launcher has not reported its process group; it will read /var/folders/.../cancel ...)
  parent/dead: pass   parent/forked: pass   parent/alive: pass
# head 5fa139ef3:
  head/none: pass   head/dead: pass   head/forked: pass   head/alive: pass

The new real-Chrome pstable case (a ps hook fails exactly -Ao pgid=,stat= while the starter is SIGTERMed under the registry hold): at head the fake Chrome is signalled and exits, the dir is kept, and the exit prints the unproved-gone message. Mutation control — group_gone returning 0 on a failed or empty ps:

AssertionError: pstable: an unreadable table was not reported: browser: started on http://127.0.0.1:51843 ...
rc=1

Note on the message the case accepts: TERM ends the fake Chrome, so before KILL the dead leader can no longer prove ownership over an unreadable table and end_group returns 2 ("could not prove process group … $HS is kept"); with a TERM-ignoring Chrome it would return 1 ("did not exit"). The case accepts either, and asserts the dir is kept and the group was signalled — the three facts the mutation flips.

Tests run at 5fa139e

python3 tests/claude-gemini-agy-browser.test.py → PASS 29/29 agy-browser.sh (was 27). git diff origin/main...HEAD | bash scripts/review-checks.sh → PASS.

Not in this PR

#4915's nits (profile-wide kill on failed start, installer pin, CDP note) — separate.

🤖 Generated with Claude Code

[github-actions]

Coverage Gate

✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%.

Diff Coverage

Diff: origin/main...HEAD, staged and unstaged changes

No lines with coverage information in this diff.

[john-the-dev]

Reviewed at 5fa139ef3ea45297011f62e6d38ef3b990f6e2f6. This closes both follow-ups from my approval of #4908 (review 5361220778). I checked each by running it, not by reading.

Follow-up 1: signal between trap on_exit EXIT and launcher=$!. Verified.

• agy-browser.sh:158-159: launcher="${launcher:-${!:-}}", then rm -rf "$HS" and return when it is empty. The $! fallback only works if nothing else is backgrounded before the launcher. I grepped the script: the only & job is the launcher fork at :314, and command substitutions and pipelines do not set $!. So an empty $! does mean "no launcher forked", and a set $! with an unset $launcher means the fork happened but was not yet recorded, which goes down the existing cancel-file path.
• rm safety: :156 returns unless ${HS:-} is non-empty and settled is empty, and HS is only ever mktemp -d output (:299, installed before the trap at :300). No path to rm -rf of an empty or unscoped value.
• Forked path: with a launcher present, the old [[ -n "${launcher:-}" ]] && guard is redundant, so calling launcher_state unconditionally is equivalent.
• Control: the script reverted to origin/main (f60ac4a) with the PR's test file gives AssertionError: on_exit/none: agy-browser.sh: Chrome's launcher has not reported its process group; it will read .../cancel .... At head it gives PASS 29/29. The forked case sets launcher='' with a live sleep &, so it exercises the $! fallback specifically.

Follow-up 2: failing-ps branch of group_gone. Verified.

• Mutation: group_gone changed to || return 0 and [[ -n "$rows" ]] || return 0 (ps failure or an empty table read as "gone"), with __pycache__ cleared. Result: AssertionError: pstable: an unreadable table was not reported: browser: started on http://127.0.0.1:54193 ..., rc=1. Restored, it passes.
• The case runs against the test's FAKE_CHROME process on this Mac (macOS). It did not skip.

Other checks

• gh pr diff 4965 | bash scripts/review-checks.sh --diff: PASS (hardcoded-paths, root-artifacts, prose-cap).
• CI: shell tests (lanes) is red, and tsc + tests (clean install) is only its aggregator. The one gating failure is tests/watcher-inbox-resolver.test.sh ("a fast resolver returns at once ... elapsed 30s of 30"). That is a timing bound in the watcher resolver, and this diff cannot reach it. The same lane passed on main and on the other PR runs today. A rerun is needed before merge. This is not a finding against the diff.
• No license/cla status is present on this head.

Nits (not blocking)

• Test 27 still runs under the bash -x diagnosis plumbing. That was my third nit on #4908; it is out of scope here and still open.

No blockers.

[keweichen]

REQUEST_CHANGES at exact head 5fa139ef3ea45297011f62e6d38ef3b990f6e2f6.

Blocker — the no-launcher cleanup fails under the shipped macOS Bash

agy-browser.sh ships with #!/bin/bash and set -u, but the new fallback at lines 157-159 is not compatible with macOS /bin/bash 3.2 when no background job exists. That is precisely the pre-launcher state this change is meant to handle.

Direct control on this Mac:

$ /bin/bash --version
GNU bash, version 3.2.57(1)-release
$ /bin/bash -c 'set -u; launcher="${launcher:-${!:-}}"'
/bin/bash: $!: unbound variable
rc=127

The same command succeeds under the PATH-resolved Homebrew Bash 5.2. The new test invokes unqualified bash at the extracted-function probe, so it selects Bash 5.2 on this host and reports 29/29 while missing the production interpreter.

I reran the entire exact-head test with only the outer PATH constrained so bash resolves to the shipped /bin/bash:

AssertionError: on_exit/none:
.../on_exit.sh: line 4: $!: unbound variable

The direct production-path signal repro has the same result: on_exit aborts at line 158, the handshake directory remains, and no cancel file exists. Thus issue #4949's no-launcher leak remains on the primary shipped macOS interpreter.

Use a Bash-3.2-safe way to read an unset $!, and pin the no-launcher case to /bin/bash (ideally also the real trap-before-fork path) so PATH cannot select a newer shell.

Other evidence

The normal PATH run passes 29/29; keeping the head test while pointing SCRIPT at main fails the new on_exit/none arm, so the arm otherwise discriminates the intended old behavior. Diff review checks, git diff --check, and shell syntax pass. Hosted Actions are currently green (Windows skipped), but license/cla is absent rather than green.

Worst case is limited to the opt-in agy-browser start path and a leaked handshake directory when a signal lands before the launcher fork; it does not justify shipping a fix that still fails on the platform interpreter. Changes requested; this head is not ready to merge.

[sonichi]

@keweichen — confirmed on this Mac too: /bin/bash 3.2.57 rejects ${!:-} under set -u when no job exists, and my extracted-function probe used PATH's Bash 5.2, so it never saw the shipped interpreter. Fixing both: a 3.2-safe read of an unset $! in on_exit, and the probe pinned to /bin/bash. Before/after under /bin/bash to follow here.

[sonichi]

@keweichen — fixed at 21bc7b763.

Change. on_exit reads $! with -u off (set +u; launcher="${launcher:-$!}"; set -u), which Bash 3.2 accepts with no job ever started; ${!:-} did not. The unit probe now runs /bin/bash explicitly, so PATH cannot select Homebrew's 5.2.

Before/after, the extracted on_exit trio under /bin/bash 3.2.57 (arm64-apple-darwin25), set -u:

5fa139ef3:  none: FAIL (…/on_exit.sh: line 4: $!: unbound variable)   dead: pass  forked: pass  alive: pass
21bc7b763:  none: pass  dead: pass  forked: pass  alive: pass

Direct proof of the form: /bin/bash -c 'set -u; set +u; launcher="${launcher:-$!}"; set -u; echo "[$launcher]"' → [], and with a sleep & before it → the pid.

Full suite at 21bc7b763 with the probe on /bin/bash: PASS 29/29. review-checks PASS. The real trap-before-fork path is still covered only by the extracted trio; I did not find a deterministic seam for a signal in that sub-millisecond window without adding one to production code, and left it out rather than add a test-only hook.

[github-actions]

@cla-assistant check

[john-the-dev]

Re-approving at 21bc7b763. The delta from 5fa139ef3 is the on_exit read of $!, plus the test now running /bin/bash explicitly.

I verified it on macOS's shipped /bin/bash, GNU bash 3.2.57 (arm64):

• Old expression: set -u; x="${launcher:-${!:-}}" fails with $!: unbound variable when no job has been started. keweichen's finding is confirmed.
• New expression, with no job: set +u; x="${launcher:-$!}"; set -u gives an empty x, which is the correct "no launcher" path.
• New expression after sleep 1 &: it picks up the pid, as before.
• tests/claude-gemini-agy-browser.test.py at head passes 29/29.

Toggling -u only around that single assignment keeps the rest of on_exit strict. Pinning the probe to /bin/bash is the right call, because that is what the #!/bin/bash shebang runs on macOS.

keweichen's CHANGES_REQUESTED at 5fa139ef3 is exactly this /bin/bash 3.2 failure, and this head fixes it. keweichen should still re-review to clear their own change request.

My earlier approval at 5fa139ef3 missed this. The probe ran the bash on PATH (5.x), not the shipped interpreter. Thanks, keweichen, for catching it.

[keweichen]

APPROVE — exact head 21bc7b76378ce03f2b3ae60fd652e5d0e31fd672 resolves my Bash-3.2 blocker and is ready to merge.

Verified on this Mac’s shipped /bin/bash 3.2.57:

• With no prior job, the new set +u; launcher="${launcher:-$!}"; set -u path produces an empty launcher and restores nounset; after sleep &, it captures the exact child PID.
• The clean exact-head suite passes 29/29 both normally and with PATH constrained so every bash invocation uses /bin/bash.
• Reverting only the production expression to the prior ${!:-} form while retaining the current test makes the full production-shell suite fail at on_exit/none with $!: unbound variable.
• Extracted actual EXIT/TERM controls remove the no-launcher handshake directory silently while preserving exit status; dead, forked, and live-launcher controls also pass.

The cumulative diff passes bash -n, git diff --check, and review-checks.sh; hosted checks and CLA are green (Windows skipped). Worst-case scope remains the opt-in agy-browser startup cleanup, with no config, migration, or resident-service change.

Non-blocking follow-up: a signal after the child has already written pgid but before the parent records launcher/LSTART can still leave that Chrome alive. I reproduced the same fail-safe survivor on base f60ac4a0, prior head 5fa139ef3, and this head, so this PR neither introduces nor worsens it; it should be tracked separately. The PR body and test docstring also still describe the prior head/27 scenarios, while the exact-head author comment carries the corrected evidence.