Repository navigation
fix(notify): escape AppleScript specials in pending-question titles - #2322
Conversation
A question title containing a double quote broke the osascript string literal, so every macOS notification fire reported FAILED while that title sat in the top three (observed 2026-07-26 on a live host: two consecutive cron fires failed; direct osascript worked, which localized it to the interpolated titles). Backslashes and double quotes are now escaped before interpolation. Evidence: with a title containing quotes, pre-fix osascript exits non-zero; post-fix the same message notifies with rc=0 (verified live on the affected host). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DjCFofrj7FTrFnhjMidJ2Y
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 65%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
qingyun-wu
left a comment
qingyun-wu
left a comment
There was a problem hiding this comment.
Reviewed the current head c61efcf. No blocking findings.
The change is narrowly scoped to the AppleScript notification string and correctly escapes backslashes before double quotes before passing the script as a direct osascript -e argument. That preserves the existing subprocess boundary and fixes the quoted-title failure without changing pending-question parsing or delivery behavior.
Focused checks run in an isolated /private/tmp checkout:
PYTHONPYCACHEPREFIX=/private/tmp/sutando-pr-2322.pycache python3 -m py_compile src/check-pending-questions.pypython3 tests/pending-questions-honest-notify.test.pypython3 tests/check-pending-questions-bullet.test.pypython3 tests/check-pending-questions-collapse.test.pypython3 tests/check-pending-questions-snippet.test.py- direct mocked
notify_macoscheck for quote and backslash escaping
Approval signal: looks good to merge from this review. Posted as COMMENT because the authenticated account is the PR author.
Reviewed by Qingyun's Personal Codex.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed current head c61efcf. The change is narrowly scoped to escaping AppleScript notification titles, the production diff has no hardcoded host-path additions, the PR includes real-host delivery evidence, all required checks are green, and I independently ran python3 -m py_compile src/check-pending-questions.py successfully.
sonichi
left a comment
sonichi
left a comment
There was a problem hiding this comment.
Approving per Chi's explicit direction (owner override of the bot-approval default). Code review-verified LGTM this session.
bassilkhilo-ag2
left a comment
bassilkhilo-ag2
left a comment
There was a problem hiding this comment.
Approving — and adding an independent real-world reproduction, since the diff alone doesn't show how cleanly this fails.
Confirmed on a separate host today, including the exact trigger and the fix-by-removal.
check-pending-questions.py reported macos=FAILED while the top-3 pending-question titles included one containing a quoted phrase:
## [CLOSED 15:15Z] "Proactive delivery is broken" - it is not
That heading was one of only 3–4 in the file, so it was certainly inside titles[:3]. After I removed that section for unrelated reasons, the next fire reported macos=ok, and every fire since has been ok. The current file has no double quotes in any heading.
The detail worth having: a manual probe masks this bug. When I saw macos=FAILED I tested with osascript -e 'display notification "test" with title "Sutando"', which succeeded — so I concluded the failure was transient and moved on. It wasn't; it was deterministic and content-dependent. A clean probe string exercises none of the escaping. Anyone debugging this without your fix will reach the same wrong conclusion, which is a good argument for landing it.
The escape order is right — \\ before ", so the quote-escape's own backslash isn't doubled. titles[:3] is the only interpolation into the AppleScript literal, and count is an int, so the escaping is complete for this call site.
One nit, non-blocking: notify_macos's docstring says it "Returns True only if osascript actually accepted the notification", which is what made the FAILED signal trustworthy and pointed at the script rather than the transport. Worth keeping that contract in mind for the sibling with title "Sutando" — the title is a constant today, so there's no second injection point, but a future caller passing a dynamic title would reintroduce this. A tiny _as_str() helper would make that structurally impossible rather than incidentally safe.
|
@cla-assistant check |
…3117) * fix(pending-questions): the macOS body sends slugs, not whole titles notify_macos capped the NUMBER of titles (3) but not their LENGTH, so the body scaled with title text. Measured on a live host with 26 questions: 388 chars, and macOS truncates aggressively — the reader saw roughly one partial sentence instead of three question names. Send the slug (text before the first comma) plus an explicit "(+N more)", matching health-check's failure notification, which already sends names rather than prose for the same reason. before len=388 26 pending questions: stale-block-from-down-reviewer, 2026-08-18 - #3088 and #3089 are held by a reviewer... after len=121 26 pending questions: stale-block-from-down-reviewer, engine-bundle-missing-redaction, cron-runner-codesigning (+23 more) #2322 fixed WHICH characters a title may contain; this fixes HOW MUCH of one may ride in the body. Regression pin fails at the parent (403 > 160). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hg4BFoyjHSyYvgHqdGxPfe * fix(pending-questions): the cap is the bound, not the comma Review blocker (liususan091219): the body was bounded by splitting on a comma, and a title need not contain one. `t.split(",", 1)[0]` returns the whole title when there is no comma, so the bound disappeared — measured on their host at 230 chars across 17 live titles, 11 of them comma-less. Their titles are `[<ts> ET] <ask>`; mine are `slug, date — <ask>`. The fix worked here because of what this host happens to produce, which is not a property the code may rely on. Cap the name at 40 chars so the guarantee holds regardless of punctuation. The pin passed only because every fixture title carried a comma — the assertion was real, the input never reached the failing branch. It now leads with a real comma-less title from the reviewer's host. Control at the uncapped expression: 251 not less than 160. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Hg4BFoyjHSyYvgHqdGxPfe * test(pending-questions): pin the cap against three measured title shapes A second host reports 61 live questions, 44 of them with no comma, in three vocabularies this notifier had never been tested against: `[<ts> UTC] …`, `<date> <time> — …`, `[<ts>] …`. None matches the `slug, date` convention the split keys on, so on that host the split degenerates to the whole title and only the cap bounds the body. Control at the uncapped expression, test kept: 199 / 206 / 205 chars against the 160 assertion, one failure per shape. With the cap: 22/22. Shapes contributed by @liususan091219 from a live corpus. * fix(pending-questions): bound the assembled body, not just each name Per-name [:40] leaves the total arithmetic: both the count prefix and the `(+N more)` suffix widen with the queue, so the margin depends on how many questions happen to be pending. Cap the assembled body once instead, so no count width can overrun it. Control, per-name cap only vs this head, same tests: count per-name only with final cap 26 157 chars 157 ok 126 159 chars 159 ok 1226 * 161 chars 159 ok 12226 * 163 chars 159 ok It breaks at four digits, which is exactly the ~3-char margin @sonichi predicted from the two-digit assumption. Also fixes what the assertions measure. _Capture records argv[-1] — the whole `display notification "..." with title "Sutando"` — so asserting on it counted a constant ~44 chars of AppleScript as if it were the body macOS truncates. The older assertions therefore held a stricter bound than they claimed (~116, not 160); the new ones extract the notification body and compare against BODY_MAX directly. Adds the worst-case shape the fixtures never produced: three maximal 40-char names. * test(pending-questions): cover the no-room branch the diff gate flagged `diff coverage >= 95%` failed on 56d2f2a with src/check-pending-questions.py at 90.9%, missing line 273 — the `room <= 0` guard I added defensively and never exercised. The gate was right. Keeping the branch rather than deleting it: without it, `joined[:room - 1]` at room == 0 is `joined[:-1]`, which silently drops a character instead of dropping the names. Reaching it naturally needs a ~63-digit count, so the test lowers BODY_MAX instead of faking an absurd queue. * fix(pending-questions): a blank title contributes no name, not a bare comma qingyun-wu's review nit on #3117: with an empty or whitespace-only title the join emitted separators with nothing between them — before: 5 pending questions: , , real-slug (+2 more) after: 5 pending questions: real-slug (+4 more) Filtering falsy names after the cap also moves the dropped entries into the remainder rather than losing them, so the count stays honest (+2 -> +4). The bound is unaffected: `room` subtracts len(extra) before the names are trimmed, so a wider remainder cannot push the assembled body over BODY_MAX. test_f6 fails at the parent commit with the exact ragged body above and passes here; full file 26/26. * fix(pending-questions): an empty join must not leave a double space When every candidate title is blank the join is empty and `head` already ends in a space, so the body rendered `N pending questions: (+N more)`. Join on the stripped head in that branch. Cosmetic and bounded — rstrip only shortens, so the BODY_MAX invariant is unaffected. test_f7 fails at the parent with `' ' unexpectedly found in '9 pending questions: (+9 more)'` and passes here; suite 27/27. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TNTJu2gT77tcdRJ3TYn83s --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
What
notify_macosinterpolates question titles straight into an AppleScript string literal. A title containing a double quote makesosascriptreject the script — every notification fire reportsmacos=FAILEDfor as long as that title sits in the top three. Backslashes and double quotes are now escaped before interpolation.Before/after evidence (live host, 2026-07-26)
Bug confirmed on
upstream/main: two consecutive cron fires loggedmacos=FAILEDwhile a pending-question title containing a quoted phrase (…health-check says \"all 51 skills…\") was in the top 3; a directosascript -e 'display notification "plain" …'on the same host succeeded, localizing the failure to title interpolation.Post-fix, the escaped path with a quote-and-backslash message:
(notification visibly delivered on the host).
ast.parseclean; no other call sites ofnotify_macos.🤖 Generated with Claude Code
https://claude.ai/code/session_01DjCFofrj7FTrFnhjMidJ2Y