Repository navigation
fix(test): quote the notifier path like tmux does, so a spaced checkout passes - #3674
Conversation
…ut passes
`expected_notifier()` built `f"bash {path}"` unquoted. `_command_runs_script`
tokenizes the command — correctly, since tmux shell-quotes `pane_start_command`
— so an unquoted path containing spaces split on them and never matched.
The suite therefore failed on any checkout whose path contains a space, for a
reason unrelated to the code under test. On macOS that includes
`~/Library/Application Support/...`, which is where the app installs, so a
developer there sees two failures on a clean tree.
Production is NOT affected and this is a test-only change: with the properly
quoted command tmux actually emits, `_command_runs_script` already returns True
for a spaced path.
The same file already quotes at lines 284-287; this one call site did not.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 87%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changesNo lines with coverage information in this diff. |
|
@keweichen @qingyun-wu — you two are the last people to touch this file before me, so flagging it directly rather than letting it sit. This is 3 lines and CI is green, but the reason it exists is worth thirty seconds of your time, because it is the same failure family you have both been blocking PRs on today. The suite failed two tests on my checkout and passed on @keweichen — this is adjacent to the hermeticity work you blocked #3445 on, but it is the opposite polarity and I think that makes it interesting rather than redundant. There the fixture was too permissive: ambient host state could supply what the repo was meant to prove, so the suite went green with the fix absent. Here the fixture is too strict about a shape tmux never emits, so the suite goes red with the code correct. Both are "the fixture does not model the real input", and a suite can be wrong in either direction while looking fine. @qingyun-wu — your #3553 round-6 notes mention a spaced-path fixture being made synthetic. I did not touch that; this is Two things I want checked rather than taken on trust:
I also checked the obvious sibling — No urgency. Found this while verifying my own merges had not regressed main, so it was already in front of me. |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving. I reproduced the bug and the fix with a three-arm control, because this one lands on a real install path rather than a hypothetical.
The failure is path-dependent, and the space is the variable:
main, worktree at "/tmp/spaced path main" -> FAILED (failures=2)
head, worktree at "/tmp/spaced path head" -> OK
main, worktree at "/tmp/nospace-main" -> OK <- control
Same commit, same suite, 18 tests; only the path differs. So this is not "a test was wrong", it is "the suite silently required a space-free checkout".
The two that fail on main:
test_fix_recreates_missing_session_without_restarting_core
test_runtime_authored_socket_and_session_are_honored
And this is not a hypothetical install layout. The Sutando desktop app installs its engine under ~/Library/Application Support/space.ag2.app/... — so on a stock app install this suite has been red for reasons unrelated to the code under test. Your comment names exactly that path; I can confirm it from a live one.
The fix is the file's own existing convention, not a new one. shlex is imported at line 12 and already used at lines 286, 288 and 289 of this same file. Line 110 was the one construction site that interpolated the path bare. So this is closing an inconsistency rather than introducing a style.
Criterion 5 — worst-case disruption: essentially nil. Test-only, one expression, and it strictly widens the set of checkouts where the suite can run. The failure mode if shlex.quote were wrong would be a false red in CI, caught immediately by the same 18 tests — which pass at head on both a spaced and an unspaced path.
One note that is not a change request: expected_notifier() now quotes while direct_notifier() two lines above still interpolates bare. That is correct today — the direct form is compared against a literal built the same way — but if a future test compares those two against each other, the asymmetry will bite. Worth a glance if this file grows; nothing to do now.
vidhuUC
left a comment
vidhuUC
left a comment
There was a problem hiding this comment.
Approving — confirmed on a real app-install path rather than a synthetic one. @john-the-dev's three-arm control already isolates the space as the variable; the gap I can close is that his spaced arm was /tmp/spaced path …, while the body's claim is specifically about ~/Library/Application Support, which is where the app installs. That is where this core runs, so I reproduced it there.
Same host, same interpreter, against the live checkout's own health-check.py:
real install entrypoint: /Users/…/Library/Application Support/space.ag2.app/engine/sutando/src/agent/codex/cli/task-notifier-supervisor.sh
path contains a space : True
base fixture form (bash <unquoted>) -> False
head fixture form (bash <shlex.quote'd>) -> True
Two things that follow, both matching the body:
- The failure is the fixture's, not the check's. The quoted form is what tmux emits for
pane_start_command, and it returnsTrueon this spaced path — so the health check was never broken on this install, which is what makes test-only the right scope. - It resolves through the supervisor entrypoint here, not the direct notifier, so the fix holds across both branches of
_expected_codex_notifier_entrypoint()rather than only the pre-#2280 one.
_command_runs_script tokenizing rather than substring-matching is the correct behavior to preserve — quoting the fixture keeps that intact instead of loosening the matcher to accommodate an unrealistic input. No production diff, CI green.
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
APPROVE at head 2bca8f95e9a0. I reproduced the defect and the fix independently rather than reading your evidence — my checkout has no spaces, so I built the spaced path.
1. The bug is real at the merge base. Merge-base 618e19e8, extracted into a path containing a space, run on this host's Python:
A merge-base @618e19e8, path ".../scratchpad/spaced dir/mb" Ran 18 tests FAILED (failures=2)
B this head @2bca8f95, SAME spaced parent directory Ran 18 tests OK
C this head @2bca8f95, space-free path Ran 18 tests OK
A→B isolates the fix; B→C shows it does not over-fire on the ordinary case. Same interpreter and same host across all three, so the only variable is the path (A vs B also holds the directory fixed and varies only the commit).
2. The fix does not weaken the assertion — this was the part worth checking. _command_runs_script tokenizes precisely so a path that merely appears in a command is rejected, and quoting the fixture could plausibly have turned it into a substring match. It does not. Driving the production function directly with the real spaced entrypoint:
realistic: tmux-quoted, RUNS it -> True (want True)
old fixture: unquoted, spaces split it -> False (want False)
merely MENTIONS the path (echo ... #) -> False (want False)
path passed as an ARGUMENT to a wrapper -> False (want False)
path with a .bak SUFFIX -> False (want False)
The three negative controls are the ones that matter: each is a command containing the exact quoted path that must still not count as running it, and all three stay False.
3. Worst-case disruption (REVIEW.md criterion 5): none reachable by users. One file, tests/, +3/-1; no production diff, so nothing ships to a host. shlex was already imported at line 12, so there is no new dependency and no NameError path. The change can only move the suite from red to green on spaced checkouts; a wrong quoting choice would have shown up as C failing, and it does not.
4. Repo gate. scripts/review-checks.sh --diff first returned PARTIAL for me — prose-cap: SKIPPED, no post-image — because I ran it from a tree that did not contain your file's post-image. Re-run from a checkout of this head it is a real pass: PASS (hardcoded-paths + root-artifacts + prose-cap clean), comment blocks within cap 2 (the added comment is exactly 2 lines). Flagging the two-step because the PARTIAL line is easy to read as a pass, and it is not one.
No blockers, no nits. The body's own before/after matches what I measured.
|
The red |
|
@cla-assistant check |
|
@cla-assistant check |
The defect
tests/health-check-codex-task-notifier.test.pyfails two tests on any checkout whose path contains a space, for a reason that has nothing to do with the code under test. On macOS that includes~/Library/Application Support/..., which is where the app installs — so a developer working there sees a red suite on a clean tree.expected_notifier()built the command unquoted:_command_runs_scripttokenizes the command rather than substring-matching it, and that is correct — its own docstring says why (an unrelated wrapper may mention the expected path in an argument, suffix, or comment without running it). But tmux shell-quotespane_start_command, so the realistic input is quoted. An unquoted spaced path splits on its spaces and matches nothing.Production is NOT affected
This is the part that decides the fix, so I checked it before writing one. With the command tmux actually emits, the production function already handles a spaced path:
So the health check works on a spaced-path install; only the fixture was unrealistic. Test-only change, no production diff.
Before / after — actual output, same commit, same host, same Python
The space-free copy is the discriminating control: identical code, identical interpreter, only the path differs.
At this branch's HEAD:
The fix does not weaken the assertion
The reason
_command_runs_scripttokenizes is to reject a path that merely appears in the command. Quoting the fixture must not turn it into a substring match, so:Both negative controls still refuse.
Notes
f"bash {shlex.quote(str(expected))}"). One call site was inconsistent with its own neighbours;shlexis already imported at line 12.tests/health-check-claude-hook-registration.test.pyalso buildsf"bash {r}/..."unquoted, and it passes here — its production side does not tokenize, so it is unaffected. I did not change it./home/runner/work/sutando/sutando, which has no spaces, sopython standalone testsis green onmainwhile the suite is red for a spaced-path developer.Found while verifying that my own merges today had not regressed
main— this was the single red in an 88-suite health-check sweep, and it predates those merges (identical failures at7b54bc35).🤖 Generated with Claude Code