fix(launchd): render plists through one escaping renderer, not four sed copies by john-the-dev · Pull Request #2835 · sonichi/sutando

john-the-dev · 2026-08-12T09:24:55Z

Closes #2832.

The bug

Four installers template a launchd plist with sed -e "s|__TOKEN__|$VALUE|g". Reproduced against the shipped com.sutando.cron-runner.plist, varying only the repo path:

repo path	sed rc	result
`/tmp/plain`	0	parses, `arg=/tmp/plain/src/cron-runner.py`
`/tmp/a&b`	0	parses, `arg=/tmp/a__REPO__b/src/cron-runner.py`
`/tmp/a<b>c`	0	unparseable (ExpatError)
`/tmp/a\|b`	1	unparseable

& in a sed replacement means "the matched text", so the token is re-emitted. sed exits 0, the plist parses, launchctl loads it, and the job points at a path that does not exist. Nothing reports a problem.

Scope: the issue said two installers; it is four

#2832 named the cron-runner and health-check installers. install-sutando-app-launchd.sh and install-credential-proxy-launchd.sh do the same thing.

The credential-proxy one is the reason this is a shared module rather than four patches: an earlier external review caught this class for one value (SUTANDO_NODE) and hand-escaped it, leaving REPO, WORKSPACE, BREW_BIN and HOME on the same line exposed. Per AGENTS.md, when one defect exists in several places because the policy is duplicated, the duplication is the defect.

The fix

src/render_plist_template.py (stdlib only) owns substitution, XML escaping, and validation. All four installers delegate. A bad render now fails the install instead of loading a broken job.

Same inputs, after:

repo path	rc	result
`/tmp/a&b`	0	parses, `arg=/tmp/a&b/src/cron-runner.py`
`/tmp/a<b>c`	0	parses, `arg=/tmp/a<b>c/src/cron-runner.py`
`/tmp/a\|b`	0	parses, `arg=/tmp/a\|b/src/cron-runner.py`

All four real templates render with /tmp/a&b<x>|y/... values and pass plutil -lint — launchd's own parser. No template file is modified.

One deliberate design point

The parse check runs on comment-stripped content. com.sutando.health-check-fallback.plist ships a comment containing --, which launchd accepts and Expat rejects — so validating raw text would have broken that installer. I found this by rendering every real template rather than only my own fixture. Escaping already prevents a value terminating a comment (</> become entities); both properties are pinned by tests.

I first "fixed" the template's comment instead, then reverted it: imposing a stricter standard than the consumer, by editing shipped prose, is the wrong direction.

Tests

tests/plist-template-render.test.py — 24 checks. Every fixture value is one sed mishandles, and assertions check the rendered path, not just that output parses, because the & corruption still parses.

Reverted each fix separately, not together:

reverted	failures
XML escaping	2
plist parse check	2
leftover-placeholder check	4
delegation in one installer	2
nothing (restored)	0 / 24 pass

That last control initially reported only 1 failure: my sed-templating guard matched the multi-line form only, so a one-line sed -e "s|__REPO__|... passed its own test. Widened to match the substitution expression anywhere — it now catches both.

Not covered

I did not launchctl bootstrap a rendered job — that would install a real agent on this host. Evidence is the renderer output plus plutil -lint, which is what launchd itself parses with.

…ed copies Four installers each templated a launchd plist with `sed -e "s|__TOKEN__|$VALUE|g"`. That is wrong for values containing sed or XML metacharacters, and every failure mode is silent. Reproduced against the shipped cron-runner template, varying only the repo path: sed rc=0 repo=/tmp/plain PARSES arg=/tmp/plain/src/cron-runner.py sed rc=0 repo=/tmp/a&b PARSES arg=/tmp/a__REPO__b/src/cron-runner.py sed rc=0 repo=/tmp/a<b>c UNPARSEABLE ExpatError sed rc=1 repo=/tmp/a|b UNPARSEABLE InvalidFileException The `&` case is the dangerous one: sed exits 0, the plist parses, launchctl loads it, and the job points at a path that does not exist. The defect is the duplication. `install-credential-proxy-launchd.sh` already carried a hand-written escape for exactly one value while leaving the four paths beside it exposed — patching each copy is what produced that. So this adds a dependency-light owner, `src/render_plist_template.py`, and points all four installers at it. Substitution is literal, values are XML-escaped, and the result is parsed before publication, so a bad render fails the install instead of loading a broken job. Same inputs through the renderer: rc=0 repo=/tmp/plain PARSES arg=/tmp/plain/src/cron-runner.py rc=0 repo=/tmp/a&b PARSES arg=/tmp/a&b/src/cron-runner.py rc=0 repo=/tmp/a<b>c PARSES arg=/tmp/a<b>c/src/cron-runner.py rc=0 repo=/tmp/a|b PARSES arg=/tmp/a|b/src/cron-runner.py All four real templates render with `/tmp/a&b<x>|y/...` values and pass `plutil -lint`, launchd's own parser. No template file is modified. The parse check runs on comment-stripped content deliberately: com.sutando.health-check-fallback.plist ships a comment containing `--`, which launchd accepts and Expat rejects, so validating raw text would have broken that installer. Escaping already prevents a value terminating a comment, and both properties are pinned by tests. tests/plist-template-render.test.py — 24 checks. Each fixture value is one sed mishandles, and the assertions check the rendered path, not just that the output parses, because the `&` corruption still parses. Reverted each fix separately, not together: dropping XML escaping (2 fail), the parse check (2 fail), the leftover-placeholder check (4 fail), and the delegation in one installer (2 fail). Restored: 24 pass. That last control first reported only 1 failure — the sed-templating guard matched multi-line form only, so a one-line sed slipped past its own test. Widened to match the substitution expression anywhere. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8

bassilkhilo-ag2 · 2026-08-12T09:26:07Z

Verified independently at 6a6efe25: reproduced the original sed & bug myself against the actual shipped src/launchd/com.sutando.cron-runner.plist with REPO=/tmp/a&b — sed exits 0 and silently emits <string>/tmp/a__REPO__b/src/cron-runner.py</string> (the corrupted path claimed in the PR body, exact match). Rendered the same template + value through the new render_plist_template.py: exits 0, correctly XML-escapes to /tmp/a&b/src/cron-runner.py, and plutil -lint confirms it's a valid plist. Ran the full tests/plist-template-render.test.py suite — 24/24 pass, matching the PR's own count. File list matches the claimed four-installer scope (cron-runner, health-check, sutando-app, credential-proxy). No blockers found.

Two CI fixes on this branch. src-map: the new module needs an entry; regenerated with scripts/gen-src-map.py (2 lines, no unrelated churn). Coverage: the CLI checks ran through subprocess, which the gate cannot see, so render_plist_template.py measured 61% against a 95% floor. main() is now called in-process and the failure paths are covered too — too few arguments, a malformed TOKEN=VALUE pair, an unreadable template, an empty token name, and a publish failure that must leave no temp file behind. One subprocess check stays, since the CLI is the real entry point. 97%; the __main__ trampoline is pragma'd as it cannot execute in-process. The success assertion first compared the raw file text and failed: on disk the value is XML-escaped, so it now asserts the parsed value instead. Re-ran the escaping revert control after the test rewrite: still 2 failures. 32 checks pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8

A cleanup glob for coverage artifacts matched the tracked config file and the previous commit carried the deletion. Restored byte-identical to main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8

github-actions · 2026-08-12T09:32:43Z

Coverage Gate

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

Diff Coverage

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

src/render_plist_template.py (96.8%): Missing lines 67-68

Summary

Total: 62 lines
Missing: 2 lines
Coverage: 96%

src/render_plist_template.py

Lines 63-72

  63         os.replace(tmp, dest_path)
  64     except BaseException:
  65         try:
  66             os.unlink(tmp)
! 67         except OSError:
! 68             pass
  69         raise
  70     return text
  71

…assertion CI caught a real break from this branch, not a flake. tests/health-check-notify-gateway.test.py asserted the installer SOURCE contains the literal __CLAUDE_CONFIG_DIR__. That held while the installer spelled the placeholder itself in a sed expression; the renderer takes bare token names and adds the delimiters, so the literal is gone. Its intent — the installer must supply the config-dir value for substitution — is unchanged, so the assertion now checks the value it passes. That surrogate was load-bearing: a token typo now fails the install loudly instead of leaving a literal in the plist. So this also pins the property directly — for all four installer/template pairs, the supplied token set must cover the template's placeholders, and each real template must render with hostile values. Previously verified by hand, not by a test. Control: dropping CLAUDE_CONFIG_DIR from the health-check installer fails the new check (and the sibling test), and restoring it passes. 40 checks, plus 22/22 in health-check-notify-gateway. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8

qingyun-wu

Reviewed exact head 8f0d51d507fe6c0a4366fa85985e76b484b1a4f9.

Changes requested; not merge-ready.

[P1] src/install-credential-proxy-launchd.sh:114 and src/install-sutando-app-launchd.sh:90 invoke the new renderer through ${PYTHON_BIN:-python3}, but neither installer resolves or assigns PYTHON_BIN. On a clean macOS host, bare python3 can resolve to Apple's Command Line Tools stub: existence checks succeed, invocation raises the developer-tools dialog, and these installs now fail before rendering. This also introduces a Python requirement into two installers that previously rendered without one. Reuse scripts/python-binary.sh / require_python, pass the resolved interpreter explicitly, and add installer-path controls where PATH exposes the unusable system stub.
[P2] The cumulative added prose violates the mandatory at-most-two-physical-line source contract. Examples include src/render_plist_template.py:2-27,44-48,58-63,68-73 and tests/plist-template-render.test.py:2-14. Keep only short durable constraints in code and move the sed/XML narrative to the PR body.

The shared renderer itself is the right boundary, and its hostile-value, publish-failure, wiring, health-check, bundled-proxy, src-map, diff-hygiene, and hardcoded-path checks pass locally. Worst case on the current head is a clean Mac getting a developer-tools modal and no launchd install from a path that previously needed no interpreter.

Reviewed by Qingyun's Personal Codex.

… python3 [P1] install-credential-proxy and install-sutando-app called the renderer through ${PYTHON_BIN:-python3}, and neither assigns PYTHON_BIN — so both always took the bare fallback. On a clean Mac that is the Xcode-CLT stub: it satisfies an existence check and raises the install dialog when run, so these two installs would fail before rendering, on a path that previously needed no interpreter. Both now source scripts/python-binary.sh and call require_python, which is the existing owner of stub detection and already fails loudly with a fix. Same class of defect as the dead-mans-switch resolver, and the same remedy. [P2] Trimmed the module and function docstrings to the two-line source contract; the sed/XML narrative lives in the PR body. Verified by AST: every module and function docstring in both files is now <= 2 lines. The CLI still prints its usage line, which is derived from that docstring. Control: reintroducing ${PYTHON_BIN:-python3} in one installer fails the new check, naming the offending line; restoring it passes. 48 checks. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8

john-the-dev · 2026-08-12T10:24:21Z

Both addressed at a26750df.

[P1] — you're right, and it was worse than a fallback. Neither installer assigns PYTHON_BIN at all, so ${PYTHON_BIN:-python3} was not a fallback path, it was the path: every invocation took bare python3. On a clean Mac that is the CLT stub, so both installs would raise the dialog and fail before rendering — on a path that previously needed no interpreter, exactly as you say.

Both now source scripts/python-binary.sh and call require_python "$REPO" "<what it was doing>", which already owns stub detection and fails loudly with a fix. I did not write a new resolver: this is the same defect class as the dead-mans-switch one, and duplicating the policy is what produced it.

Pinned so it cannot come back: the suite now asserts no installer reaches the renderer through a bare python3. Control — reintroducing ${PYTHON_BIN:-python3} in one installer fails that check and names the offending line; restoring it passes. 48 checks.

[P2] — done. Module and function docstrings in both files are now ≤2 lines, verified by AST walk rather than by eye. The CLI still prints its usage line, which is derived from the module docstring, so the trim did not break it.

One factual note, not an argument: I scoped this to lines this PR adds. scripts/pr_flag.py (my other PR) has seven pre-existing >2-line docstrings and src/slack-bridge.py has 48 over-length comment runs; rewriting those would bundle unrelated churn. If the contract is meant to apply file-wide on touch rather than to the diff, say so and I'll do it as separate commits.

The map's one-line purpose is derived from the module docstring, so trimming it to the two-line contract made the generated entry stale. One line, no other churn. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8

qingyun-wu

Re-reviewed exact head 151bc9dacaf0a4869724a3604447a2c5b6c2ef00.

The P1 is fixed. Both newly Python-dependent installers now call the shared require_python resolver before invoking the renderer, and the resolver's macOS system-stub controls pass. The renderer, hostile-value/publish-failure/wiring suite, health-check wiring, bundled-proxy checks, src-map, diff hygiene, and REVIEW.md path gate all pass.

One P2 source-contract blocker remains. src/install-credential-proxy-launchd.sh:112-115 and src/install-sutando-app-launchd.sh:88-91 each contain a contiguous four-line added comment run; splitting the explanation into two topics does not change the physical-line limit. tests/plist-template-render.test.py:2-4 also spans three physical lines because the closing delimiter is on its own line. Reduce each added block to at most two physical lines.

Worst-case runtime disruption from the prior head is now mitigated. This head is not merge-ready solely because the mandatory prose cap is still exceeded.

Reviewed by Qingyun's Personal Codex.

Merged the two adjacent comments at each installer call site into one, and closed the test module docstring on its content line. Splitting an explanation into two topics does not change the physical-line count, and a closing delimiter on its own line is a third physical line. Measured correctly this time: the longest contiguous comment run TOUCHING an added line, read from the files. The diff-stream check I used before is wrong in both directions — it merges non-adjacent blocks across hunks, and misses pre-existing comment lines sitting next to an added one, which is what hid these. Control re-run after the comment-only edit: restoring a bare python3 still fails the interpreter check. 48 checks pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8

john-the-dev · 2026-08-12T10:42:32Z

Fixed at 296116d3 — and your P2 caught a flaw in how I was measuring the contract, not just a missed site.

Both installer sites were four physical lines because my new two-line comment sat directly adjacent to a pre-existing two-line comment. Merged each pair into one two-line comment. The test module docstring was three physical lines because the closing """ sat on its own line — now closed on the content line.

The reason I reported "max run: 2" on the previous head is that I was measuring from the diff's + lines. That is wrong in both directions: it merges non-adjacent blocks across hunks (which had me chasing a phantom six-line run on another PR earlier today), and it cannot see a pre-existing comment line adjacent to an added one — which is exactly what hid these two. I now measure the longest contiguous run touching an added line, read from the files. All six touched files report 2.

Control re-run after the comment-only edit, since that is the change one assumes is inert: restoring a bare python3 still fails the interpreter check. 48 checks pass.

Thanks for the P1 — the ${PYTHON_BIN:-python3} was worse than a fallback, since neither installer assigns PYTHON_BIN at all, so it was the only path taken.

…lines The seven-line transcript-persistence preamble and the five-line oversized-entry explanation are now two lines each, keeping the non-obvious part: a MutationObserver captures every append path, and an oversized entry gets a static placeholder rather than a silent drop that would make the restored transcript lie. Measured as physical contiguous runs touching an added line, read from the file. The first attempt left the block's opening line adjacent to the new two-line comment, making three — the diff's +-lines cannot show that, which is how the same adjacency slipped through on sonichi#2835. Identifiers and behaviour untouched; braces balanced; related tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8

Only conflict was docs/src-map.md's generated module count — HEAD said 208, main said 209. Resolved by taking main's file and re-running scripts/gen-src-map.py rather than hand-picking a number, which is what the "refuse docs/src-map.md stale vs src/" gate actually checks. It now reads 210: main's 209 plus this branch's render_plist_template.py, and re-running the generator is a no-op. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8

john-the-dev · 2026-08-13T11:04:42Z

Conflict resolved — this is MERGEABLE again at dc216331. It had gone DIRTY against main and could not have merged whatever its review state was, so that part was mine to fix rather than anyone's to wait on.

The only conflict was docs/src-map.md's generated module count — this branch said 208, main said 209. I took main's file and re-ran scripts/gen-src-map.py rather than hand-picking a number, since a generated count is exactly the thing you should not resolve by eye and it is what the refuse docs/src-map.md stale vs src/ gate checks. It now reads 210 — main's 209 plus this branch's render_plist_template.py — and re-running the generator is a no-op:

gen-src-map: wrote docs/src-map.md (210 modules)
git diff --quiet docs/src-map.md  ->  fresh
grep render_plist_template docs/src-map.md  ->  line 103, indexed

Verified after the merge: tests/plist-template-render.test.py and tests/health-check-notify-gateway.test.py both rc=0, all four installers pass bash -n, git diff --check and review-checks.sh clean. I also audited git log origin/main..HEAD — 7 commits plus the merge, all this PR's own, no strays.

@qingyun-wu — your approval predates this merge, so it is no longer timed to the head. Nothing changed in the code you reviewed; the merge touches only the generated src-map line. Re-affirming when convenient would make it count toward quorum, but I did not want to churn your review without saying why it moved.

bassilkhilo-ag2 · 2026-08-13T11:05:46Z

Independently verified the merge-conflict resolution and the underlying fix at dc216331 — first time reviewing this PR.

Merge resolution: confirmed MERGEABLE (the BLOCKED state is the missing-2nd-approval gate, unrelated). Regenerated docs/src-map.md myself from a fresh clone with scripts/gen-src-map.py — byte-identical to what's committed, 210 modules including render_plist_template.py at line 103. git log origin/main..HEAD shows 8 commits (7 + the merge), all this PR's own iteration history — no strays.

Test suite: ran tests/plist-template-render.test.py fresh — all pass, matching the reported 24-check count (this run shows more granular PASS lines than a bare count but the coverage matches the table in the PR body: escaping, plist parse check, leftover-placeholder check, per-installer delegation, interpreter resolution).

Mutation test: removed the escape() call from render_plist_template.py's substitution (reverting to sed-equivalent unescaped replacement) and reran — 2 failures, exactly on /tmp/a&b and /tmp/a<b>c, the same two hostile-value classes the PR's own bug table names (& silently re-emitting the token, </> breaking XML well-formedness). /tmp/a|b and the quote/backslash cases still pass unescaped, which is correct — sed's actual failure mode was specifically the delimiter and &/<>, not those. Restored and diffed — byte-identical.

No blockers from my side. Solid fix, well-evidenced (the PR body's own revert-each-fix-separately table is exactly the discipline I'd ask for), and the merge resolution reads as purely mechanical as described.

qingyun-wu

Reviewed exact head dc216331dcf0d27fdb1852ddcb1d77ec2dcd2b97.

Approving. The merge conflict resolution is mechanical: docs/src-map.md was regenerated to 210 modules, includes render_plist_template.py, and rerunning scripts/gen-src-map.py is clean. I also rechecked the launchd templating fix at this head: installer substitution now goes through the shared XML-escaping plist renderer, renderer output is parse-checked before publish, hostile path values round-trip through parsed plists, and the installer call sites avoid a bare python3 renderer invocation.

Verified locally in an isolated /private/tmp worktree:

PYTHONPYCACHEPREFIX=/private/tmp/codex-pycache-2835-dc216331 python3 -m py_compile src/render_plist_template.py tests/plist-template-render.test.py
python3 tests/plist-template-render.test.py
python3 tests/health-check-notify-gateway.test.py
bash -n src/install-cron-runner-launchd.sh src/install-health-check-launchd.sh src/install-sutando-app-launchd.sh src/install-credential-proxy-launchd.sh
python3 scripts/gen-src-map.py && git diff --quiet docs/src-map.md
git diff origin/main...HEAD | bash scripts/review-checks.sh
git diff --check origin/main...HEAD

Hosted checks are green at this head, including tsc + tests, diff coverage, python39 compatibility, CLA, and launchd-related lint gates. No blockers found.

Reviewed by Qingyun's Personal Codex.

bassilkhilo-ag2

Approving at current head `dc216331` — the code is correct at this exact head, independently verified: fresh clone, `review-checks.sh` PASS clean, no hardcoded `/opt/homebrew` literal remains, `docs/src-map.md` regen is a no-op, and mutation-testing the escaping fix (removing `escape()`) reproduced exactly the two hostile-value failure classes the PR names (`&` and `<>`). No blockers on the code itself.

Flagging separately: this PR currently shows `mergeable_state: dirty` — a conflict, almost certainly from main advancing past it via the recent merge wave. Approval stands for this head's code; the conflict needs a rebase/merge before it can land.

Taking either side whole would have regressed something. HEAD replaces the four seds with render_plist_template.py, which is this PR's whole point. main has since added a __CLAUDE_CONFIG_DIR__ substitution (sonichi#2843, merged today) with its own XML+sed escaping. Taking HEAD alone drops that token; taking main alone reverts the renderer. Composed: the renderer call now also passes "CLAUDE_CONFIG_DIR=$CLAUDE_CFG". The renderer's own guard proves the token was required rather than cosmetic: with all tokens unresolved: none omitting CCD unresolved: ['CLAUDE_CONFIG_DIR'] So a silent drop was not actually possible here — unresolved_tokens() fails loud. Worth stating, because it means this file's design already defends the class of mistake, and the composition is correctness rather than luck. Checks: bash -n clean · tests/credential-proxy-plist-config-dir.test.sh PASS (that is sonichi#2843's own test, so the token still lands) · tests/credential-proxy-bundled-install.test.sh PASS. credential-proxy-failure-semantics.test.ts fails identically on origin/main here — a .ts file needing a TS runner this shell does not give it, not a regression from this merge. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8

john-the-dev · 2026-08-13T15:57:02Z

e75258bc — un-conflicted. Taking either side whole would have regressed something.

HEAD replaces the four seds with render_plist_template.py, which is this PR's whole point. But main has since gained a __CLAUDE_CONFIG_DIR__ substitution — #2843, which merged today — with its own XML+sed escaping. So taking HEAD alone drops that token; taking main alone reverts the renderer.

Composed: the renderer call now also passes "CLAUDE_CONFIG_DIR=$CLAUDE_CFG".

The renderer's own guard proves the token was required, not cosmetic:

with all tokens   unresolved: none
omitting CCD      unresolved: ['CLAUDE_CONFIG_DIR']

Worth saying plainly: a silent drop was never possible here, because unresolved_tokens() fails loud. That is this PR's design already defending the class of mistake I was at risk of making while resolving it — which is a decent argument for the renderer over four seds, since the sed version had no such check.

Checks: bash -n clean · tests/credential-proxy-plist-config-dir.test.sh PASS — that is #2843's own test, so the token still reaches the plist · tests/credential-proxy-bundled-install.test.sh PASS.

credential-proxy-failure-semantics.test.ts fails here, and I checked the baseline before blaming the merge: it fails identically on origin/main in this shell — a .ts file needing a TS runner, not a regression.

qingyun-wu

Reviewed exact head e75258bc1d6997e22357ca26c9ed325e8eca3c0e.

Approving. This merge resolution correctly composes the PR’s shared XML-escaping renderer with main’s new credential-proxy CLAUDE_CONFIG_DIR placeholder: install-credential-proxy-launchd.sh now passes CLAUDE_CONFIG_DIR=$CLAUDE_CFG, while the renderer still rejects any unresolved placeholder before publishing. Taking either parent whole would have dropped one side; the combined result preserves both.

Verified in an isolated exact-head worktree:

python3 tests/plist-template-render.test.py
python3 tests/health-check-notify-gateway.test.py
bash tests/credential-proxy-plist-config-dir.test.sh
bash tests/credential-proxy-bundled-install.test.sh
shell syntax for all four installers
regenerated docs/src-map.md is byte-clean
scripts/review-checks.sh and git diff --check pass

Worst-case disruption remains fail-closed: a missing substitution aborts before replacing the installed plist. No blocking findings; code-ready. The hosted clean-install and diff-coverage jobs were still running at review time, so final merge readiness still depends on those checks plus the repository quorum gate.

Reviewed by Qingyun’s Personal Codex.

bassilkhilo-ag2 · 2026-08-13T16:03:04Z

Verified today's conflict resolution (e75258bc) independently — my earlier approval was on the pre-conflict code; this checks the merge itself.

Confirmed the composition is real, not just claimed: src/install-credential-proxy-launchd.sh and src/install-health-check-launchd.sh both pass "CLAUDE_CONFIG_DIR=$CLAUDE_CFG" into the renderer call alongside this PR's existing tokens — #2843's addition and this PR's renderer are genuinely both present, neither reverted.

tests/credential-proxy-plist-config-dir.test.sh (11/11) and tests/credential-proxy-bundled-install.test.sh (3/3) both pass at this head.

Didn't independently verify the .ts baseline-parity claim (no TS runner installed on my check host) — low-risk given it's a pre-existing/unrelated test, not something this merge touches.

Approval stands; this resolves the conflict I'd flagged earlier. No new blockers.

john-the-dev · 2026-08-13T19:43:05Z

@bassilkhilo-ag2 — re-review request, and here is exactly what changed since your approval so you can scope it.

Your approval at dc21633 went stale when I merged main in. The only change to this PR's own content since then is in src/install-credential-proxy-launchd.sh: main added a __CLAUDE_CONFIG_DIR__ token by extending the sed-copy approach, and the merge carries that token through this PR's single renderer instead of re-adding a sed copy.

I'm flagging it rather than treating it as a no-op merge because a conflict-free textual merge can still be a semantic change — that is the one place in this diff where main's intent and this PR's approach had to be reconciled by hand.

Verified at head e75258bc1d6997e22357ca26c9ed325e8eca3c0e: the token is declared in src/launchd/com.sutando.credential-proxy.plist (lines 68-69), passed by the renderer call (line 126), and covered by tests/credential-proxy-plist-config-dir.test.sh. All required checks are green.

Nothing else in the diff moved.

qingyun-wu · 2026-08-13T23:58:47Z

Read at exact head e75258bc. The consolidation is right and the escaping holds — I checked every __TOKEN__ site in the shipped templates and they are all inside <string> element text, never an attribute, so escape() not covering quotes is fine here. One finding worth acting on, plus two notes. None of them block.

[worth fixing] The renderer adds a hard plistlib dependency to an install path whose own file documents plistlib being unimportable on this host's preferred interpreter.

origin/main's installer says so directly:

# Measured 2026-08-03: the preferred Homebrew python3 (3.14.5) could not `import
# plistlib` at all (pyexpat/libexpat symbol mismatch), so the installed job
# would have failed every 300s into a log nobody reads

The guard against that is resolve_python_verified(), but look at what it probes — health-check.py:

121:    "$1" - "$REPO/src/health-check.py" >/dev/null 2>&1 <<'PROBE'

and health-check.py's own plistlib import is deferred and guarded, inside a function with an explicit fallback message:

4338:        import plistlib
4342:                           f"cannot import plistlib ({exc.__class__.__name__}: {exc}). "

The probe imports the module without executing main(), so that line never runs and a plistlib-broken interpreter passes the probe. render_plist_template.py then imports it unconditionally at module scope:

4:import plistlib

So on exactly the host the comment describes, $PYTHON_BIN clears verification and then dies at line 238. Before this PR that step was sed, which had no interpreter dependency at all — so this is a new coupling on the component the file itself calls "the one component whose failure nothing else watches".

Severity is bounded by || exit 1 at the call site: it fails loudly at install time, not silently at runtime. So this is a robustness regression, not a broken install. Two small fixes either work — probe the renderer alongside health-check.py, or defer the plistlib import in the renderer and skip validation with a warning when it is unavailable, matching the pattern health-check.py already uses at 4342.

[note, low] The parse check validates a different document than the one written. Comments are stripped before plistlib.loads, so nothing inside a comment is checked. Reproduced on the real template:

$ python3 -c "... m.render_to_file('src/launchd/com.sutando.health-check-fallback.plist', dest, vals)"
render_to_file(real template): SUCCEEDED
the WRITTEN plist parses: NO -> not well-formed (invalid token): line 24, column 42

That template's leading comment carries six literal -- runs, all from documented CLI flags (--emit-task --notify-on-fail). I checked before calling this a problem and it is not one in practice — plutil -lint on the rendered output returns OK, so Apple's parser and launchd accept it, which is what the code comment already claims. Worth knowing only as a limit on what the check can promise: it cannot see comment-level breakage at all.

[nit] Substitution is order-dependent when a value contains another token. render() iterates values.items() and substitutes into already-substituted text:

values={'LABEL':'__REPO__','REPO':'/pwned'}   -> <key>Label</key><string>/pwned</string>
same dict, reversed insertion order          -> <key>Label</key><string>__REPO__</string>

escape() does not touch underscores, so a value carrying a literal __TOKEN__ gets re-expanded, and which way it resolves depends on dict insertion order. No current caller can hit this — every value is a path or an interpreter — so it is a latent sharp edge, not a bug today. Collecting replacements and applying them in one pass would remove it.

Not recording a formal review action; the second approval here has to come from a human anyway.

github-actions · 2026-08-14T00:17:25Z

@cla-assistant check

qingyun-wu

Reviewed exact merge head def0ced9ab564fbf5573136168e2d60f935bec13.

Ready to merge from code review. The topic patch remains byte-stable across both main merges (stable patch ID 40de82727568b27c446177587bf43e5470af8164), and the current composition preserves the shared plist renderer plus main-side installer/config changes. No inline review threads are open.

Reviewer-run evidence in an isolated exact-head worktree: plist renderer suite passed; health-check gateway 22/22; credential-proxy config-dir 11/11; bundled-install 3/3; Python compilation with an isolated cache; shell syntax for all four installers; regenerated source map byte-clean; diff hygiene, review preflight, and repository path/root-artifact checks passed.

Worst-case disruption is fail-closed at install time: an unresolved token, invalid render, or unavailable renderer prevents replacement of the installed plist. The previously noted plistlib interpreter coupling is non-blocking because it fails loudly before launchd mutation. Hosted clean-install and diff-coverage jobs were still running, so final merge readiness remains contingent on those current-head gates and quorum.

Reviewed by Qingyun Personal Codex.

qingyun-wu

Reviewed exact head def0ced9ab564fbf5573136168e2d60f935bec13.

Approval signal from code review. This is a merge-main refresh over the already-reviewed plist renderer change; the effective PR diff remains the same shared XML-escaping renderer plus launchd installer wiring, and I found no new blocker.

Focused checks run in an isolated /private/tmp worktree:

python3 tests/plist-template-render.test.py
python3 tests/health-check-notify-gateway.test.py
python3 scripts/gen-src-map.py --check
git diff --check origin/main...HEAD

Hosted tsc + tests and diff coverage were still in progress at review time, so merge should still wait for those to finish green.

Reviewed by Qingyun's Personal Codex.

* fix(web-client): persist chat transcript across page reloads The conversation transcript (#transcript) was in-DOM only — append-only with no persistence and no restore-on-load — so reloading the web UI wiped the entire chat. The Tasks list survived via taskMap, which is why a reload left task entries visible but no chat bubbles. Snapshot transcript entries to localStorage (capped at 50, oversized data-URL entries skipped) and restore them on DOMContentLoaded, sanitizing stored html with DOMPurify and re-attaching live copy buttons. A MutationObserver captures every append path (text replies, voice transcripts, images) without editing each call site. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(web-client): clear default seed before restoring transcript Prevents the "Ask Sutando anything." seed line from duplicating when the persisted transcript (which itself captures the seed as its first entry) is restored on top of the freshly-rendered default. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(web-client): persist a placeholder for oversized images instead of silent drop (CR #1857) Susan's CR (false_claim): the body said the MutationObserver captures 'every append path (text replies, voice transcripts, images)' while the snapshot silently FILTERED OUT oversized entries (data-URL images over the 20k cap) — so images were captured but never actually persisted, and the restored transcript dropped those bubbles without a trace. Fix: replace the silent filter with a placeholder entry — the bubble survives the reload as '[image/attachment not kept across reloads — too large for local storage]' (static markup, no user content, DOMPurify- safe). The transcript no longer lies by omission, and the doc claim is edited to match: images are captured live; across reloads a placeholder notes the skipped payload. * style(web-client): trim the two cited comment blocks to two physical lines The seven-line transcript-persistence preamble and the five-line oversized-entry explanation are now two lines each, keeping the non-obvious part: a MutationObserver captures every append path, and an oversized entry gets a static placeholder rather than a silent drop that would make the restored transcript lie. Measured as physical contiguous runs touching an added line, read from the file. The first attempt left the block's opening line adjacent to the new two-line comment, making three — the diff's +-lines cannot show that, which is how the same adjacency slipped through on #2835. Identifiers and behaviour untouched; braces balanced; related tests pass. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8 * test(web-client): deterministic regression for the transcript snapshot This PR shipped no test. The snapshot half is pinnable without a DOM engine — it touches only children/className/cloneNode/innerHTML — so it is covered here. The restore half genuinely needs a browser, and a fake elaborate enough to emulate createElement + innerHTML parsing would be testing the fake; that half stays with the real-browser reload capture attached to the PR. Pins the behaviours that changed after this PR's last approval, and the ones a reviewer called easy to get wrong: an oversized entry becomes a visible placeholder rather than vanishing, and a quota failure retains the most recent half rather than losing the transcript. Mutation-checked with targeted sensitivity — each mutation fails exactly one test, not the suite: oversized -> silent drop -> only the placeholder test fails quota retry keeps oldest -> only the quota test fails keep oldest N -> only the recency test fails stop stripping copy-btn -> only the copy-btn test fails Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8 * test(web-client): type the transcript-snapshot stubs instead of any eslint failed my own push: no-explicit-any is error-level here, not the warning it is elsewhere in the tree. Replaced the three `any` annotations with ElStub/CloneStub interfaces describing exactly the surface snapshotTranscript() touches — className, innerHTML, cloneNode, querySelectorAll. Verified the retype did not quietly change what the stub exercises: eslint clean, 8/8 still pass, and the suite is still mutation-sensitive — removing the copy-btn stripping still fails that test alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8 * docs(test): trim the snapshot suite header to the two-line contract Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MK5AeA7sHrCk8tZv5sTrt8 --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>

john-the-dev added the ag2product AG2 product related (packages, PyPI, task-relay) label Aug 12, 2026

john-the-dev mentioned this pull request Aug 12, 2026

launchd installers template plists with sed — a path containing & or <> installs a broken job, exit 0 #2832

Closed

john-the-dev and others added 2 commits August 12, 2026 02:27

john-the-dev marked this pull request as ready for review August 12, 2026 09:49

qingyun-wu requested changes Aug 12, 2026

View reviewed changes

john-the-dev mentioned this pull request Aug 12, 2026

feat(dead-mans-switch): off-machine liveness alerting via external ping monitor #1932

Open

john-the-dev mentioned this pull request Aug 12, 2026

fix(web-client): persist chat transcript across page reloads #1857

Merged

2 of 5 tasks

qingyun-wu approved these changes Aug 12, 2026

View reviewed changes

qingyun-wu mentioned this pull request Aug 12, 2026

fix(credential-proxy): bake CLAUDE_CONFIG_DIR into the launchd job so the proxy reads the core's keychain item #2843

Merged

john-the-dev requested a review from bassilkhilo-ag2 August 12, 2026 22:01

qingyun-wu approved these changes Aug 13, 2026

View reviewed changes

bassilkhilo-ag2 approved these changes Aug 13, 2026

View reviewed changes

john-the-dev enabled auto-merge (squash) August 13, 2026 16:00

qingyun-wu approved these changes Aug 13, 2026

View reviewed changes

john-the-dev disabled auto-merge August 13, 2026 16:11

john-the-dev requested a review from bassilkhilo-ag2 August 13, 2026 19:43

Merge branch 'main' into fix/plist-template-render

3666d2d

john-the-dev enabled auto-merge (squash) August 13, 2026 23:59

Merge branch 'main' into fix/plist-template-render

def0ced

qingyun-wu approved these changes Aug 14, 2026

View reviewed changes

john-the-dev merged commit 477623d into sonichi:main Aug 14, 2026
18 checks passed

john-the-dev mentioned this pull request Aug 14, 2026

fix(discord-read): render timestamps in the owner's timezone, never raw UTC #2270

Closed

yixuan-ag2 mentioned this pull request Aug 16, 2026

launchd cron-runner is SIGKILLed by code signing: resolve_python() can pick the ad-hoc-signed in-bundle interpreter #2968

Open

Conversation

john-the-dev commented Aug 12, 2026

The bug

Scope: the issue said two installers; it is four

The fix

One deliberate design point

Tests

Not covered

Uh oh!

bassilkhilo-ag2 commented Aug 12, 2026

Uh oh!

github-actions Bot commented Aug 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Gate

Diff Coverage

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

Summary

src/render_plist_template.py

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

john-the-dev commented Aug 12, 2026

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

john-the-dev commented Aug 12, 2026

Uh oh!

john-the-dev commented Aug 13, 2026

Uh oh!

bassilkhilo-ag2 commented Aug 13, 2026

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

bassilkhilo-ag2 left a comment

Choose a reason for hiding this comment

Uh oh!

john-the-dev commented Aug 13, 2026

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

bassilkhilo-ag2 commented Aug 13, 2026

Uh oh!

john-the-dev commented Aug 13, 2026

Uh oh!

qingyun-wu commented Aug 13, 2026

Uh oh!

github-actions Bot commented Aug 14, 2026

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

qingyun-wu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Aug 12, 2026 •

edited

Loading