fix(plists): share the launchd document frame and escape every value - #605
Merged
Merged
Conversation
Block-level duplication, which the function-level audits in #603 and #604 could not see: the plist document frame was copy-pasted into six renderers across three bridges and two services. The copies had already disagreed. Both services ran their values through xml_escape; all three bridges interpolated them raw. A SITE_PATH, service home, log directory, model name, or bot token containing an ampersand rendered a plist that is not well-formed XML, which launchd refuses to load -- so the agent never starts, with no failing test anywhere to say why. Verified against main: a path with '&' in it produces a ParseError at the WorkingDirectory line. The snapshot fixtures could not catch this because they use tidy values like /var/www/site. A golden file locks in whatever was rendered the day it was written, including a bug. Only the frame is shared. The bodies genuinely differ -- the worker schedules with StartInterval where the bridges use KeepAlive, and the WordPress service renders no EnvironmentVariables -- so they stay as readable heredocs. A single renderer taking seven parameters and three optional blocks to absorb that variation would be harder to read than the duplication it removed. All 8 committed snapshots remain byte-identical, so this changes nothing for values that were already safe. tests/plist-rendering.sh renders every plist with a value carrying & < > and asserts the result parses as XML and still contains the value. It fails against main and passes here.
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.
Block-level duplication — repeated inline blocks rather than whole functions, which the audits in #603 and #604 could not see.
The bug this found
A normalised block audit turned up the plist document frame copy-pasted into six renderers across three bridges and two services. The copies had already disagreed:
Both services escaped. All three bridges did not. A
SITE_PATH, service home, log directory,OPENCODE_MODEL, or bot token containing an&renders a plist that is not well-formed XML, which launchd refuses to load — so the agent never starts, and nothing anywhere says why.Verified against
mainby rendering with a hostile path:Line 12 is
WorkingDirectory.The snapshots could not have caught this. They render
/var/www/site— tidy values, no metacharacters. A golden file locks in whatever you rendered the day you wrote it, including a bug. Stability and correctness are different claims, and only one of them was under test.What is shared, and what deliberately is not
Only the frame: five lines in, two out, as
plist_document, which reads the body from stdin.The bodies stay as heredocs in their own files, because they genuinely differ — the worker schedules with
StartIntervalwhere the bridges useKeepAlive, and the WordPress service renders noEnvironmentVariablesat all. A single renderer taking seven parameters and three optional blocks to absorb that variation would be harder to read than the duplication it removed. The duplication worth removing was the part that was identical and wrong in three places, not the part that legitimately varies.Every interpolated plist value now goes through
xml_escape, including the ones inside conditional$(if …)blocks — tokens and model names, the values most likely to carry a metacharacter.Verification
All 8 committed snapshots remain byte-identical. That is the headline: this changes nothing for values that were already safe, and the escaping only engages where the old code was broken.
tests/plist-rendering.sh— 14 assertions, wired into CI. Every plist is rendered witha&b<c>din its paths, home, model and token, then parsed withxml.etreeand checked that the value survived rather than merely parsed. A plist that parses but lost half a path is worse than one that fails to load.It fails against
mainand passes here, which is the only reason to trust it.Full suite on Linux: 74 pass, 4 fail — the same 4 environmental failures as a pristine
mainclone on that host, unchanged all night.bash -nclean.ci-coverage,dead-mechanismandduplicate-mechanismall still green.Not in scope
The block audit found 40 repeated-block clusters; this PR fixes the largest and the only one that was actively wrong. The rest are real but benign — nginx server blocks repeated in
lib/infrastructure.sh, shared preambles betweensetup.shandupgrade.sh. I did not add a block-duplication CI check, because it would either fail on day one or need a 39-entry allowlist, and a checker that ships with an allowlist that large is a checker nobody will ever satisfy.AI assistance: audited and implemented by Claude Opus 4.5 via Claude Code, driven by Chris. The model wrote a maximal-repeated-block audit to pick the target, found the escaping divergence, deliberately scoped the shared abstraction to the frame rather than building a parameterised renderer, and verified the new test fails against main before trusting it.