Fix: XACA-0512 — Rework migrate.sh update_launchagents to render from templates

[ehlersd]

Summary

Third sibling of the launchagent-render drift (XACA-0476 → XACA-0510 → this).

aiteamforge-migrate.sh::update_launchagents was doing in-place sed -i.bak / sed -i.bak2 path-rewrites on whatever plist already existed in ~/Library/LaunchAgents — never rendering from canonical templates. Drifted or hand-edited plists weren't recoverable, and the hack left .bak / .bak2 droppings.

This rework brings migrate.sh in line with install-kanban.sh and the XACA-0510 fix in upgrade.sh: render *.template → *.new tempfile → diff against live target → atomic mv + launchctl reload only on change. Tempfiles cleaned up on all no-op / DRY_RUN / interrupt paths via _cleanup_migrate_tmpfiles + RETURN trap.

The migrate-specific wrinkle (vs XACA-0510)

The agent set spans two template directories with incompatible substitution maps, so a single sed expression can't serve all three:

Plist filename	Template	Renderer	Placeholders
com.aiteamforge.kanban-backup.plist	share/templates/kanban/backup-plist.template	_render_kanban_template	USER_HOME, AITEAMFORGE_DIR, BACKUP_INTERVAL, PYTHON3_PATH
com.aiteamforge.lcars-health.plist	share/templates/kanban/lcars-health-plist.template	_render_kanban_template	(same — harmless extras)
com.aiteamforge.fleet-monitor.plist	share/templates/fleet-monitor/fleet-launchagent.template.plist	_render_fleet_template	NODE_PATH, FLEET_SERVER_PATH, LOG_DIR, HOMEBREW_PREFIX, HOME_DIR, FLEET_PORT, AITEAMFORGE_DIR

{{AITEAMFORGE_DIR}} resolves to ${NEW_DATA_DIR} (the migration's destination), not ${WORKING_DIR} as in upgrade.sh. That's what makes the rewritten plist correct after a migration completes.

Changes

• libexec/commands/aiteamforge-migrate.sh — added _cleanup_migrate_tmpfiles, _render_kanban_template, _render_fleet_template, and reworked update_launchagents. Module-level XACA_0512_KANBAN_BACKUP_INTERVAL_DEFAULT=900 / XACA_0512_FLEET_MONITOR_PORT_DEFAULT=3000 constants with cross-reference comments; sibling-drift consolidation tracked in XACA-0516.
• tests/test-xaca-0512-migrate-launchagent-render.sh — 16 test cases, all pass. Covers: all-absent → skip; explicit .bak/.bak2 regression assertion (the old sed -i.bak behavior must be gone); kanban + fleet render with no {{…}} placeholders; fleet renders all 7 fleet-specific substitutions including default FLEET_PORT=3000; selective opt-in (kanban present, fleet absent → fleet not materialised); no-op second run (mtime unchanged, no tempfile leak); FORCE=true re-renders; DRY_RUN preserves sentinel + no tempfile leak; missing template warns without crash; DRY_RUN + no-change reports "All LaunchAgents up to date".
• CHANGELOG.md — full Unreleased stanza.

Audit notes

• aiteamforge-migrate-check.sh::analyze_launchagents (line 381) has the same EXPECTED_AGENTS list but is read-only (presence + launchctl list check, no render). No change required.
• Pre-existing tests/test-xaca-0510-launchagent-render.sh sibling: 15/15 still green.

Semantics preserved

• FORCE=true re-renders even when target is up to date
• DRY_RUN=true writes nothing, prints [DRY RUN] Would update, no .new leak
• launchctl unload runs before mv, launchctl load runs after (both tolerate failure)
• Agents absent from ~/Library/LaunchAgents/ are skipped — migrate must not silently materialise agents the user opted out of at install time
• LAUNCHAGENTS_DIR env seam for sandbox tests (M3Pro tap-install ban)

Test plan

• bash tests/test-runner.sh tests/test-xaca-0512-migrate-launchagent-render.sh → 16/16 green
• bash tests/test-runner.sh tests/test-xaca-0510-launchagent-render.sh → 15/15 green (no regression in sibling)
• shellcheck libexec/commands/aiteamforge-migrate.sh tests/test-xaca-0512-migrate-launchagent-render.sh → clean (only pre-existing SC1091 info on sourcing)

🤖 Generated with Claude Code

[ds9-tester-bot]

QA Verification — XACA-0512 (PR #32)

Result: APPROVED — 16/16 new tests + 15/15 regression tests all pass.

Gate 1: Shellcheck

• shellcheck -S warning on both aiteamforge-migrate.sh and test-xaca-0512-migrate-launchagent-render.sh exits 0 (clean at warning+ severity).
• SC1091 (info/sourcing) and SC2129 (style) are both pre-existing on main; confirmed by baseline comparison with git show origin/main:.... No new warnings or errors introduced.

Gate 2: New Test Suite — 16/16 PASS

All 16 test cases verified individually via --verbose run:

1. Missing targets: all three plists absent → function skips all
2. Regression: no .bak/.bak2 files left behind (old sed -i.bak behavior gone)
3. Fresh render: kanban plists rendered with no {{...}} placeholders
4. Fresh render: kanban plists contain resolved NEW_DATA_DIR path
5. Fleet-monitor: target present → rendered from fleet-monitor template dir
6. Fleet-monitor: rendered plist contains all fleet-specific substitutions
7. Fleet-monitor: rendered plist contains resolved FLEET_PORT default (3000)
8. Selective opt-in: kanban-backup present, fleet absent → fleet not materialised
9. No-op: second run with same template leaves file mtime unchanged
10. FORCE=true: re-renders plist even when target is up to date
11. DRY_RUN=true: writes nothing to target plist
12. DRY_RUN=true: prints '[DRY RUN] Would update' for changed agent
13. DRY_RUN=true: no .new tempfile leaked after run
14. Missing template: function reports warning and continues without crash
15. All three sentinels: all plists rendered with no placeholders
16. DRY_RUN=true + no-change: target unchanged, no tempfile, 'up to date' summary

Gate 3: XACA-0510 Regression Suite — 15/15 PASS

No regression in sibling upgrade.sh render logic.

Gate 4: Plist XML Structure Spot-Check

Rendered all three plists with synthetic substitution values using the extraction pattern from the test:

• kanban-backup.plist: no unresolved {{...}} — plutil -lint OK — xmllint OK (726 bytes, well-formed XML)
• lcars-health-plist.plist: no unresolved {{...}} — plutil -lint OK — xmllint OK
• fleet-launchagent.plist: no unresolved {{...}} — plutil -lint OK — xmllint OK
  Fleet-monitor placeholders (NODE_PATH, FLEET_SERVER_PATH, FLEET_PORT, LOG_DIR, HOMEBREW_PREFIX, HOME_DIR, AITEAMFORGE_DIR) all fully substituted.

Edge Case Assertions (all confirmed by test suite)

• DRY_RUN=true with sentinel content → sentinel preserved (test 11, 13)
• FORCE=true → mtime changes when content identical (test 10)
• Missing template → warning emitted, exit code 0, no crash (test 14)
• .bak/.bak2 regression explicitly caught by test 2

Summary

The render-from-template refactor in update_launchagents() is correct and complete. Three-agent dispatch (kanban-backup, lcars-health, fleet-monitor) with per-family substitution maps is working. The migrate-specific {{AITEAMFORGE_DIR}} → NEW_DATA_DIR (not WORKING_DIR) semantic is validated by test 4. No sandbox contamination of ~/Library/LaunchAgents at any point.

[ai-security-review-bot]

Code Review — PR #32 (XACA-0512): migrate.sh render-from-template

Decision: APPROVE

The implementation is solid. It mirrors the XACA-0510 upgrade.sh pattern faithfully, handles the fleet-monitor wrinkle cleanly, and the 16-case test suite is genuine coverage. No blockers found.

---

Security

Injection vectors — CLEAR. The sed substitutions use | as delimiter consistently. The values injected are:

• $HOME — shell-set, not user-controlled
• ${NEW_DATA_DIR} — defaults to $HOME/.aiteamforge; user can override via --new-dir flag, but this is a path they own. A pipe character in their chosen path would break the sed expression, but that would be a self-inflicted, non-escalating error (sed fails, render fn returns 1, agent is skipped with a warning — not a privilege escalation vector).
• ${node_path} — resolved via command -v node, not user input.
• ${kanban_backup_interval} — numeric default or env override; no string injection surface.
• ${fleet_port} — same, numeric.
• ${homebrew_prefix} — from brew --prefix, not user input.

All are system-resolved, not attacker-controlled. Acceptable.

launchctl || true — correct here. During migration, agents may not be loaded (they could be stopped, user may have a fresh session). Both unload-before-render and load-after-render are best-effort. Silencing failures is intentional and matches the upgrade.sh pattern. Not masking anything meaningful.

Tempfile atomicity — mv "$tmpfile" "${target}" after render is atomic on the same filesystem (which LaunchAgents always is). No TOCTOU concern. The RETURN trap + _migrate_tmpfiles array prevents .new leakage on interrupt or early return. Array is re-initialized at the top of update_launchagents on every call, so no stale entries from a previous call.

---

Architecture

Stringly-typed dispatch tuple (filename|render-fn|template-relpath) — this is idiomatic bash for the use case. The alternative (associative arrays keyed by agent name) requires bash 4+ and macOS ships bash 3.2. The pipe-delimited tuple with %%|* / #*| parsing is the correct approach. The XACA-0510 sibling used a colon-delimited pair; this PR extends it to a triplet because it needs to name a render function. Clear, maintainable, no concern.

DRY on shared substitutions — both render functions repeat {{AITEAMFORGE_DIR}} → ${NEW_DATA_DIR}. A _apply_common_substitutions helper was considered; not warranted here. The two functions have different enough substitution sets (4 vs 7 expressions) that merging them would require parameterization that costs more than it saves. The duplication is one line in each function. Noted as non-blocking suggestion below.

NEW_DATA_DIR semantic — correctly uses NEW_DATA_DIR (the migration destination) instead of WORKING_DIR (upgrade.sh's live install). This is what makes the rendered plist correct post-migration. The CHANGELOG comment and inline comment at line 19 both explain this. Good.

RETURN trap stomping — trap '_cleanup_migrate_tmpfiles' RETURN is set inside update_launchagents. Bash scopes traps to the current execution environment, not to functions, so this overwrites any existing RETURN trap in the shell. main() does not set a RETURN trap (verified), and update_launchagents is called from main() not from another function with a RETURN trap. No stomp risk in practice. This is identical to the upgrade.sh pattern.

_migrate_tmpfiles as module-level state — the module-level array is reset at the top of update_launchagents, so concurrent invocations (not possible in practice for a migration script) would clobber each other's cleanup list. Not a real concern here given the serial execution model.

XACA-0516 cross-reference accuracy — verified: install-kanban.sh line 16 sets KANBAN_BACKUP_INTERVAL=900; install-fleet-monitor.sh line 15 sets FLEET_MONITOR_PORT="${FLEET_MONITOR_PORT:-3000}". Both match the defaults in this PR. The comment is accurate.

---

Code Quality

No eval in production code — eval appears only in the test extraction harness (to load the four extracted functions into the test shell). This is a known pattern for testing scripts that have side-effect-heavy main bodies; the awk boundary markers are anchored to the specific comment string at the top of _cleanup_migrate_tmpfiles. Acceptable in test infrastructure.

local declarations in loop (lines 606–612) — this is bash (shebang confirmed: #!/bin/bash), not zsh. In bash, local var=val inside a loop is clean; there is no stdout-emission footgun. The zsh-local-in-loop rule from our knowledge base does not apply here.

shellcheck annotations — SC2329 on _cleanup_migrate_tmpfiles is justified (called indirectly via trap; shellcheck can't trace it). No unjustified suppressions.

set -eo pipefail — already present at script top (line 18). The render functions return 1 on failure without triggering set -e exit because the caller uses if ! "$render_fn" ... which is an explicit check.

---

Tests

Test 2 (.bak/.bak2 regression) — correctly seeds all three targets as empty files, runs the function, then asserts no .bak or .bak2 siblings. If someone reintroduces sed -i.bak, the .bak file will exist and the test fails. Genuine regression guard.

Test 9 (missing template) — verifies both the warning text ("not found") and the exit code (assert_exit_success). Covers both assertions as required.

DRY_RUN on fleet-monitor — three DRY_RUN tests (8a, 8b, 8c) all use the kanban-backup target. No DRY_RUN test exercises the fleet-monitor path specifically. This is a non-blocking gap (see suggestion below).

Template placeholder completeness — verified against the actual templates: backup-plist.template uses {{USER_HOME}}, {{AITEAMFORGE_DIR}}, {{BACKUP_INTERVAL}}, {{PYTHON3_PATH}} — all four are substituted. fleet-launchagent.template.plist uses seven placeholders — all seven are substituted. lcars-health-plist.template uses only {{AITEAMFORGE_DIR}} — handled. No residual {{...}} in any targeted template after render.

Other fleet-monitor templates (fleet-reporter-launchagent.template.plist, funnel-launchagent.template.plist) have different placeholders but are NOT referenced in the agents array. No exposure.

---

Non-Blocking Suggestions

Two kanban subitems filed. See below.

---

Reviewed by: Commander Jett Reno, Chief Technical Instructor

[ai-security-review-bot]

Re-approval of PR #32 after commit 18b2fc3.

Scope check: Only CHANGELOG.md and tests/test-xaca-0512-migrate-launchagent-render.sh modified. No production code touched. Clean.

New test correctness: Seeds FLEET_PLIST (com.aiteamforge.fleet-monitor.plist, defined at line 121), not BACKUP_PLIST — correct target for _render_fleet_template coverage. All four assertions verified against the right variable.

Assertion completeness:

• sentinel preserved (assert_equal FLEET_SENTINEL_DO_NOT_OVERWRITE content) verified
• no .new leak (assert_file_not_exists FLEET_PLIST.new) verified
• 'fleet-monitor' in output (assert_contains output 'fleet-monitor') verified
• 'Would update' in output (assert_contains output 'Would update') verified

Style consistency: Uses reset_launchagents + run_update_launchagents helper + test_start/test_pass — matches existing pattern throughout the file. Comment block explains the gap being closed (the three pre-existing DRY_RUN tests all route through _render_kanban_template; this one exercises _render_fleet_template specifically).

XACA-0512-003 cancellation: Accepted per reviewer's conditional wording and YAGNI rationale. No objection.

All review items resolved. Approved.

[ds9-tester-bot]

Re-QA complete for commit 18b2fc3 (XACA-0512 [Review] subitem XACA-0512-002).

SCOPE VERIFICATION: New commit touches only CHANGELOG.md and tests/test-xaca-0512-migrate-launchagent-render.sh. Production code (aiteamforge-migrate.sh) unchanged from prior approval.

LINT: shellcheck tests/test-xaca-0512-migrate-launchagent-render.sh — clean, zero warnings/errors.

TEST RESULTS:

• XACA-0512 suite: 17/17 PASS (was 16/16; new 17th test is the fleet-monitor DRY_RUN case)
• XACA-0510 sibling suite: 15/15 PASS — no regression

NEW TEST CASE VERIFICATION (test 17):

• Name: 'DRY_RUN=true (fleet-monitor): sentinel preserved, no .new leak, Would update printed'
• Seeds FLEET_PLIST (not BACKUP_PLIST) as sentinel target
• Asserts all four required conditions:
  1. Sentinel preserved — DRY_RUN must not overwrite fleet-monitor target plist
  2. No .new tempfile leak after DRY_RUN run
  3. Output references 'fleet-monitor' agent specifically
  4. Output contains 'Would update'

All verification criteria satisfied. APPROVE.