feat(b-0528): unit tests for shadow launchd installer (6 categories, 19 tests)#3423
Conversation
Implements all 6 acceptance categories from docs/backlog/P3/B-0528-shadow-launchd-installer-unit-tests-2026-05-15.md (the deferred test-coverage P1 from PR #3375's review): 1. Placeholder substitution (3 tests): replace, multi-replace, unrecognized {{NAME}} exits 1 2. XML escaping (4 tests): five predefined entities, safe chars untouched, & escaped first to avoid double-escape, integration with substitutePlaceholders producing plutil-valid plist for paths containing & < > 3. Argument validation (6 tests, via subprocess): unknown flag rejected, relative --repo-root rejected, relative --bun-path rejected, --repo-root with no value rejected, --repo-root followed by a flag rejected, requireAbsolute happy-path 4. Dry-run output (1 test, via subprocess): renders to stdout matching the repo template, plutil-valid output 5. Default detection (3 tests): tryDetect for which-bun works, nonexistent-binary returns undefined (no throw), git outside checkout returns undefined 6. Availability-preserving install pattern (2 tests for plutilLint safe path, with substrate-honest comment explaining why the atomic-rename failure branches aren't reachable without main() refactoring; deferred for follow-up if reviewers flag it) Bun's test runner; subprocess invocations use spawnSync with args-as-array (no shell, no injection — same convention as the implementation under test). Total: 19 tests / 39 expect calls / all passing. Closes the P1 finding Copilot flagged on PR #3375 line 185 about missing test coverage for safety-critical XML escape / placeholder refusal / argument validation / dry-run output. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c16e9b31db
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Adds a Bun unit-test suite for the macOS LaunchAgent “shadow launchd installer” script (tools/shadow/launchd/install-launchagent.ts) to close backlog item B-0528 by covering placeholder substitution, XML escaping, argument validation, dry-run output, default detection, and plutilLint behavior.
Changes:
- Introduces 19 Bun tests covering 6 acceptance categories for the launchd installer helpers and CLI dry-run path.
- Exercises several error/exit paths via subprocess execution to validate exit codes and stderr output.
- Adds integration checks that validate generated plists via
plutil -lint.
Comments suppressed due to low confidence (2)
tools/shadow/launchd/install-launchagent.test.ts:183
- P0: This test pipes the --dry-run output to
plutil -lint.plutilis macOS-only, so this will fail on Linux/Windows CI or when contributors runbun testoutside macOS. Gate/skip the plutil round-trip whenprocess.platform !== "darwin"(and still keep the pure string assertions active).
const proc = spawnSync(
"bun",
[SCRIPT_PATH, "--dry-run", "--repo-root", repoRoot, "--bun-path", bunPath],
{ encoding: "utf-8" },
);
expect(proc.status).toBe(0);
expect(proc.stdout).toContain("<?xml version=\"1.0\"");
expect(proc.stdout).toContain("<plist version=\"1.0\">");
expect(proc.stdout).toContain(repoRoot);
expect(proc.stdout).toContain(bunPath);
// Verify it's a valid plist by piping to plutil
const dir = mkdtempSync(join(tmpdir(), "test-dry-run-"));
const tmp = join(dir, "rendered.plist");
try {
writeFileSync(tmp, proc.stdout, "utf-8");
const lint = spawnSync("plutil", ["-lint", tmp], { encoding: "utf-8" });
expect(lint.status).toBe(0);
tools/shadow/launchd/install-launchagent.test.ts:235
- P0:
plutilLint()callsplutilandprocess.exit(1)on failure. Ifplutilis missing (non-macOS), this in-process test will terminate the entire test run rather than just failing an assertion. Please skip/gate the wholeplutilLintdescribe block whenprocess.platform !== "darwin"(or whenplutilis not on PATH).
describe("plutilLint", () => {
it("returns without throwing for a valid plist", () => {
const valid = `<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0"><dict/></plist>`;
expect(() => plutilLint(valid)).not.toThrow();
});
Two cross-platform portability findings from PR #3423 review: - Codex P1 line 234: `plutilLint` called in-process hard-exits the test runner on non-darwin hosts when `plutil` is missing (ENOENT → plutilLint's catch calls process.exit(1)). - Copilot P0 line 98 + 167 + 229: subprocess `plutil` calls fail on non-darwin hosts, breaking the cross-platform test suite. Both real. `plutil` is a macOS-only system binary, and the LaunchAgent install path the test file exercises is also macOS-only (launchd is macOS). The pure-helper tests (xmlEscape, substitutePlaceholders, requireAbsolute, tryDetect, argument-validation via subprocess) run cross-platform. Fix: introduce `const IS_DARWIN = process.platform === "darwin"` + `const itDarwin = IS_DARWIN ? it : it.skip` and route the 4 plutil-touching test cases through `itDarwin`: - "substituted values containing & < > produce plutil-valid plist" - "writes rendered plist to stdout, not to ~/Library/LaunchAgents/" - "returns without throwing for a valid plist" (plutilLint) - "exits with code 1 (via subprocess) for an invalid plist" (plutilLint) Result on macOS: 19 pass / 0 fail (unchanged). Result on Linux (expected): 15 pass / 4 skip / 0 fail (suite remains green and the rest of the suite is reachable). Co-authored-by: Claude <noreply@anthropic.com>
…h) (#3743) B-0528 (install-launchagent unit tests) mechanized 2026-05-15 by PR #3423 — tools/shadow/launchd/install-launchagent.test.ts at 273 lines / 19 tests across 6 categories. Row status: open was substrate drift. Third drift-catch this session (B-0506 + B-0530 + this). Co-authored-by: Claude <noreply@anthropic.com>
Summary
Closes B-0528 — the deferred test-coverage P1 from PR #3375's review cycle. All 6 acceptance categories covered with 19 tests / 39 expect calls.
Coverage by category
tryDetect)plutilLintsafe-path / failure-pathSubstrate-honest gap note
The availability-preserving install pattern (read-old-into-memory → write-tmp → atomic-rename → side-car-backup) has its SAFE branches covered via dry-run + plutilLint tests. The UNSAFE branches (atomic-rename failure, backup-write failure) require either mocking
destPathto a tmpdir or running the actual install and cleaning up — neither is straightforward without refactoringmain()to accept a destPath override. Documented in a comment at the bottom of the test file. Filing a follow-up if reviewers flag it.Verification
bunx tsc --noEmitcleanbun test tools/shadow/launchd/install-launchagent.test.ts— 19 pass / 0 fail🤖 Generated with Claude Code