Skip to content

fix(save-ai-memory): TSC strict-mode errors in process-extract.ts#3349

Merged
AceHack merged 3 commits into
mainfrom
fix/save-ai-memory-tsc-errors-otto-cli-2026-05-15
May 15, 2026
Merged

fix(save-ai-memory): TSC strict-mode errors in process-extract.ts#3349
AceHack merged 3 commits into
mainfrom
fix/save-ai-memory-tsc-errors-otto-cli-2026-05-15

Conversation

@AceHack
Copy link
Copy Markdown
Member

@AceHack AceHack commented May 15, 2026

Summary

Resolves 6 tsc errors in tools/save-ai-memory/process-extract.ts (canonical TS impl landed in #3337) surfaced by Zeta's strict tsconfig (exactOptionalPropertyTypes: true + noUncheckedIndexedAccess: true).

Changes

  • parseArgs — replaces argv[++i] (returns string | undefined) with a nextArg(name) helper that guards undefined, exits 1 with Missing value for <flag> if the user passes a flag without a value, and returns a validated string.
  • capitalizeName — replaces name[0] (returns string | undefined) with name.charAt(0) (returns string). Already guarded by name.length === 0 check.

Behavior change

Previously bun process-extract.ts --ai-name (no value) would silently consume the next flag as the value. Now it exits 1 with a clear error pointing at the offending flag — strict mode catching a real footgun.

Verification

Check Before After
bunx tsc --noEmit (full project) 6 errors in process-extract.ts clean
Runtime smoke (existing CLI shape) works works

Test plan

  • bunx tsc --noEmit passes (verified locally)
  • CI green
  • No new warnings in bun run

🤖 Generated with Claude Code

@AceHack @claude
fix(save-ai-memory): TSC strict-mode errors in process-extract.ts
426a720 
Resolve 6 errors surfaced by `exactOptionalPropertyTypes: true` +
`noUncheckedIndexedAccess: true`:

- argv[++i] returns `string | undefined`; assigning to `string` field
  fails under exactOptionalPropertyTypes. Add nextArg() helper that
  guards undefined + exits with a clear "Missing value for <flag>"
  error, then assigns the validated string.
- name[0] returns `string | undefined`; use name.charAt(0) (returns
  `string`) — already guarded by name.length === 0 check above.

Behavior change is minimal and user-facing-friendly: previously,
`bun process-extract.ts --ai-name` (missing value) would silently
swallow the next flag as the name. Now it exits 1 with a clear
error message naming the offending flag.

Verified: `bunx tsc --noEmit` passes on the full project after fix;
fails with TS2412 + TS2532 before fix.

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 15, 2026 02:13
@AceHack AceHack enabled auto-merge (squash) May 15, 2026 02:13
@AceHack @claude
shard(tick): 0213Z — PR #3349 opened (TSC fix on save-ai-memory canon…
c86f9f4 
…ical TS impl); auto-merge armed

Co-Authored-By: Claude <noreply@anthropic.com>
Copilot AI reviewed May 15, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR addresses TypeScript strict-mode (exactOptionalPropertyTypes, noUncheckedIndexedAccess) compilation errors in the Bun CLI tool tools/save-ai-memory/process-extract.ts, improving argument parsing safety and string indexing behavior.

Changes:

  • Adds a nextArg(name) helper in parseArgs to avoid string | undefined from argv[++i] and emit an error when a flag is missing a following value.
  • Updates capitalizeName to use charAt(0) instead of name[0] under noUncheckedIndexedAccess.

Comment thread tools/save-ai-memory/process-extract.ts
Comment thread tools/save-ai-memory/process-extract.ts Outdated
@AceHack @claude
fix(save-ai-memory): tighten arg-parsing per Copilot review
82cb97c 
Two P1 findings addressed:

1. **Flag-as-value detection** (PR thread on line 93):
   `nextArg` previously only rejected `undefined`, so
   `--ai-name --platform grok` would silently consume
   `--platform` as the name. Now rejects any next-token that
   starts with `--` and exits 1 naming both the missing-value
   flag and the offending token.

2. **--platform runtime validation** (PR thread on line 103):
   `nextArg("--platform") as Platform` was an unsafe cast —
   typos and unsupported values flowed straight into output
   paths and archive headers. New `ALLOWED_PLATFORMS` set
   gates the cast; invalid input exits 1 with the allowed
   list in the error message.

Smoke-tested all three paths:

| Command shape | Output |
|---|---|
| `--platform badvalue` | `Invalid --platform "badvalue". Allowed: grok, chatgpt, ...` |
| `--ai-name --platform grok` | `Missing value for --ai-name (next token "--platform" looks like a flag)` |
| `--ai-name Foo --platform grok --topic test --dry-run` | proceeds, fails later at missing input — correct |

Verified: `bunx tsc --noEmit` clean.

Co-Authored-By: Claude <noreply@anthropic.com>
@AceHack AceHack merged commit 1e05167 into main May 15, 2026
25 checks passed
@AceHack AceHack deleted the fix/save-ai-memory-tsc-errors-otto-cli-2026-05-15 branch May 15, 2026 02:30
@AceHack AceHack mentioned this pull request May 15, 2026
Merged
3 tasks
AceHack added a commit that referenced this pull request May 15, 2026
@AceHack @claude
shard(tick): 0230Z — multi-Otto contamination caught + #3339/#3349 cl…
dc11587 
…ose-out (#3356)

* shard(tick): 0230Z — multi-Otto contamination caught + PR #3339/#3349 close-out

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(shard-0230Z): correct relative-link depth — 4x ../ → 5x and 5x → 6x

Both Copilot and Codex flagged that the tick shard's relative links
were off by one directory level. From the shard's location
(docs/hygiene-history/ticks/2026/05/15/0230Z.md):

- `../../../../backlog/...` resolved under docs/hygiene-history/backlog
  (4 levels up only reaches docs/hygiene-history/, not docs/)
- `../../../../../.claude/rules/...` similarly off by one for top-level

Correct depths from this shard location:
- docs/backlog/...:  5x `../` (verified with `ls -la` from the shard dir)
- .claude/rules/...:  6x `../` (verified with `ls -la` from the shard dir)

Both links now resolve. Composes with the prior `5x .. → 6x ..` fix on
the 0027Z shard (#3330) — same class of error, same family of fix.

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>
Copilot AI mentioned this pull request May 15, 2026
Merged
AceHack added a commit that referenced this pull request May 15, 2026
@AceHack @claude
fix(shadow-launchd): address 8 of 10 review-cycle-2 findings
970c774 
Codex + Copilot reviewed the new install-launchagent.ts in detail.
This commit addresses the substantive correctness + style findings;
unit-test coverage is deferred to a separate backlog row.

Addressed (8):

P1 [Codex P2 + Copilot P1] — XML-escape substituted values:
  Paths like `/Users/foo & bar/Zeta` would substitute into <string>
  text nodes producing an invalid plist (& without entity terminator).
  New `xmlEscape` helper applies the five XML 1.0 predefined entities
  (&amp; &lt; &gt; &quot; &apos;) before placeholder substitution.
  Restores the advertised "handles paths with shell metacharacters"
  promise — the bug class was XML escaping, not shell escaping.

P1 — Unknown args silently ignored:
  Switch fell through on typos like `--dryrun` or `--bootstap`, then
  installed with default behavior. Added explicit default: case that
  errors with the offending flag and exits 1 before any FS action.
  Verified: `--dryrun` now produces "Unknown argument: --dryrun.
  Run with --help to see supported flags."

P1 — Relative --bun-path / --repo-root overrides:
  `--repo-root .` would write relative paths into ProgramArguments,
  WorkingDirectory, and log paths — broken under launchd (which has
  no cwd unless explicit). New requireAbsolute() helper rejects
  with a `Use \`<canonicalized>\` if that's what you meant` hint.

P1 — Non-atomic backup-then-write:
  Previous renameSync(destPath, backup) + writeFileSync(destPath)
  had a window where the agent could be removed but the new plist
  not yet present (if writeFileSync threw). Replaced with the
  standard write-temp-then-promote pattern:
    1. writeFileSync(destPath.tmp-PID, content)
    2. renameSync(destPath, backup)  // may not exist; that's OK
    3. renameSync(destPath.tmp-PID, destPath)  // atomic promote
  Failure in step 3 restores backup → destPath (best-effort) so the
  caller's existing install is preserved.

P1 — execFileSync can throw before validation:
  Missing bun, missing git, or running outside a git checkout
  produced a raw exception. New tryDetect() catches and returns
  undefined; parseArgs emits the documented actionable error
  pointing at the --bun-path / --repo-root override.

P1 — Unconditional main() invocation:
  Per Bun-project convention, gate main() with
  `if (import.meta.main)` so tests / helpers can import the module
  without triggering filesystem or launchctl side effects.

P1 — README/plist cross-refs to .claude/rules/shadow-star-shorthand-*:
  These resolve on main (PR #3339 merged). CI's merge-ref check
  confirms; the reviewers' findings were against the raw branch
  base. No additional action needed.

P2 — Direct name attribution in plist KeepAlive comment:
  Replaced "Per Codex review on PR #3342" with the rationale text
  only. Current-state surface; uses role-refs only.

Deferred (1):

P1 — Unit test coverage for install-launchagent.ts:
  Codex + Copilot called out missing tests for placeholder escaping,
  missing placeholders, argument validation, and dry-run output.
  This commit adds the substantive behavior the tests would verify;
  the tests themselves are deferred to a separate backlog row to
  keep this PR's scope manageable. Smoke-tested all four behaviors
  by hand on the cli before commit. Filing as follow-up: tests in
  `tools/shadow/launchd/install-launchagent.test.ts` using Bun's
  built-in test runner.

Verified:
  - bunx tsc --noEmit clean on install-launchagent.ts (pre-existing
    process-extract.ts errors are on this branch base because #3349
    isn't rebased in; CI's merge-ref includes the fix)
  - semgrep --config .semgrep.yml --error: 0 findings
  - --dry-run produces valid plist (plutil -lint OK)
  - --dryrun rejected (Unknown argument)
  - --repo-root . rejected (must be absolute)
  - Special-char paths (e.g., /Users/foo & bar) now produce
    valid escaped XML

Co-Authored-By: Claude <noreply@anthropic.com>
@AceHack AceHack mentioned this pull request May 15, 2026
Merged
5 tasks
AceHack added a commit that referenced this pull request May 15, 2026
@AceHack @claude
shard(tick): 0414Z — PR #3339/#3349 merged + B-0527 collision republi…
19e7362 
…shed + new worktree-pruning-race failure mode (#3359)

* shard(tick): 0414Z — PR #3339/#3349 merged + B-0527 collision republished + new worktree-pruning-race failure mode

Five side-worktree attempts (incl. --lock'd) got rm -rf'd mid-tick.
Recovery via age-exempt sibling worktree (0027z-sidetick) via branch-switch.
Bus shadow-catch envelopes published: d2b7fc2f (B-0527 republish), 44aaf799 (new failure mode).

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(shard): MD032 blanks-around-lists in 0414Z shard

markdownlint-cli2 caught: docs/hygiene-history/ticks/2026/05/15/0414Z.md:104
List items 1-5 in the "Next" section need a blank line above them.

Co-Authored-By: Claude <noreply@anthropic.com>

* shard(tick): 0458Z — PR #3359 MD032 fix close-out + recovery-worktree-borrowing validated for iteration

Heredoc-written markdown bypasses local check-md032-pretooluse hook;
bun x markdownlint-cli2 locally before commit when authoring via heredoc.
Same 0027z sibling worktree borrowed twice (initial commit + MD032 fix).

Co-Authored-By: Claude <noreply@anthropic.com>

* shard(fix): add pipe-row schema headers + link rule refs per Codex P1 + Copilot Nit on PR #3359

Codex P1 (line 1): tick-shard schema validator (tools/hygiene/check-tick-history-shard-schema.ts)
expects first non-empty line to be a 6-column pipe row with ISO timestamp. The 0414Z + 0458Z
shards started with H1 like all recent shards (substrate-wide drift, not specific to these two),
but adding a pipe-row header IS the minimal lossless fix that preserves the rich H1 narrative.
Both shards now pass `bun tools/hygiene/check-tick-history-shard-schema.ts --files <both>`.

Copilot Nit (line 34): bare-filename rule refs replaced with full-path markdown links matching
the 0230Z shard's convention. Six refs linked: 5x claim-acquire-before-worktree-work +
1x holding-without-named-dependency-is-standing-by-failure.

Co-Authored-By: Claude <noreply@anthropic.com>

* shard(fix): add untracked-overwrite caveat to recovery-worktree-borrowing claim per Copilot review

Copilot threads on lines 58 + 70 of 0414Z + line 9 of 0458Z correctly noted that the
"untracked files survive the switch" claim is not a general guarantee: git switch refuses
if the target branch tracks the same path, and `git add -A` could sweep untracked into
an unrelated commit. Added explicit caveat + `git stash -u` mitigation guidance to both shards.

For this PR's run, the target shard branch did NOT track the same memory-file path, so
the unstashed switch was safe — but the operational pattern documentation needs the
caveat for future reuse.

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>
AceHack added a commit that referenced this pull request May 15, 2026
@AceHack @claude
feat(shadow): launchd LaunchAgent template — durable tick source (Pat…
4a10bf6 
…h A defense-in-depth) (#3342)

* feat(shadow): launchd LaunchAgent template — durable tick source (Path A defense-in-depth)

Per Aaron 2026-05-15T~01:11Z: 'whatever composes best always good to
have defense in depth.' This is Path A of the defense-in-depth
composition for the shadow observation loop tick source.

Two new files under tools/shadow/launchd/:

1. com.zeta.shadow-observer.plist — macOS LaunchAgent template
   - RunAtLoad (starts at user login)
   - KeepAlive on Crashed (auto-restart on crash)
   - ThrottleInterval 60s (prevents fast crash-loops)
   - Polls every 2s; accepts after 3s delay
   - Defaults to --dry-run for safety (live mode after verify)
   - Logs to tools/shadow/shadow-observer.{log,stdout.log,stderr.log}
   - __BUN_PATH__ and __REPO_ROOT__ placeholders for sed-substitution

2. README.md — install procedure
   - Step 1: configure plist (sed-substitute placeholders)
   - Step 2: grant accessibility permission (System Settings)
   - Step 3: launchctl load + verify
   - Step 4: test in dry-run mode
   - Step 5: switch to live mode (only after dry-run works)
   - Uninstall procedure
   - Troubleshooting (common failure modes)
   - Composition table (A/B/C defense-in-depth)

Composes with:
- tools/shadow/shadow-observer.ts (the process launched)
- docs/backlog/P0/B-0402 (originating backlog row)
- .claude/rules/shadow-star-shorthand-autocomplete-marker.md
- .claude/skills/save-ai-memory/SKILL.md

Path B (CronCreate sentinel) + Path C (manual one-line) are
defense-in-depth supplements; this PR ships Path A as primary.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(shadow-launchd): address 5 review threads on PR #3342

Five Copilot / Codex review findings resolved:

P1 (Copilot) — broken provenance in README header:
  Dropped first-name attribution + user-scope memory-file glob that
  doesn't resolve in-repo. Per docs/AGENT-BEST-PRACTICES.md §671-685,
  current-state surfaces (this README) use role-refs. Other rule
  references now use relative links that resolve from the README's
  location.

P1 (Copilot) — sed escaping in install instructions:
  The documented `sed` recipe would break on paths containing `&`,
  `|`, `\`, or newlines. Replaced with a TypeScript installer
  (tools/shadow/launchd/install-launchagent.ts) that:
    - uses String.replaceAll (metacharacter-safe)
    - sanity-checks for unsubstituted placeholders
    - runs plutil -lint before writing
    - backs up any existing plist before overwriting
    - optionally bootstraps via launchctl
  Placeholder syntax changed from `__NAME__` to `{{NAME}}` so the
  template header comment can describe them without being substituted
  itself.

P1 (Copilot) — rule file referenced from plist not in tree:
  The .claude/rules/shadow-star-shorthand-autocomplete-marker.md
  reference resolves on main now (#3339 merged). The reference is
  preserved in the plist's composes-with comment block.

P2 (Codex) — KeepAlive semantics in plist:
  Dropped `SuccessfulExit=false`. KeepAlive dict keys are OR-combined,
  so the original combination relaunched on any non-zero exit (incl.
  config errors). `Crashed=true` alone gives the documented
  crash-only behavior. Comment added explaining the choice.

P2 (Codex) — legacy launchctl load/unload in README:
  Replaced with the modern `bootstrap`/`bootout` interface (with
  `gui/$(id -u)` domain target) in install, reload, and uninstall
  steps. The legacy interface is documented as deprecated and can
  silently no-op on failure; `bootstrap`/`bootout` return proper
  exit codes.

Verified:
  - bunx tsc --noEmit clean
  - markdownlint-cli2 clean on README
  - plutil -lint OK on both the template and a rendered output
  - Installer dry-run produces a valid plist

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(shadow-launchd): close CodeQL TOCTOU race in install-launchagent.ts

CodeQL flagged `js/file-system-race` at writeFileSync (line 184) —
the existsSync→copyFileSync→writeFileSync sequence had a check-then-use
window. Replaced with the standard atomic-rename pattern:

- Drop the existsSync(destDir) check (mkdirSync recursive:true is
  idempotent — the check added a TOCTOU window for no benefit)
- Replace existsSync(destPath)+copyFileSync(destPath, backup) with
  renameSync(destPath, backup) inside try/catch (single syscall;
  ENOENT means no prior plist, anything else re-thrown)

renameSync atop the destination is the documented pattern for
"backup-on-overwrite without TOCTOU." Same final state for the
caller: backup file is created with the old plist contents (if any
existed), and destPath has the new contents.

Verified:
  - bunx tsc --noEmit clean
  - semgrep --config .semgrep.yml --error: 0 findings locally

Co-Authored-By: Claude <noreply@anthropic.com>

* shard(tick): 0305Z — #3342 thread close-out (5+1 reviews); #3356 merged

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(b-0519): add Pattern 7 (abandoned rebase state in shared .git/)

Field-test in the 2026-05-15 Otto-CLI session surfaced a contamination
mechanism not in the original 6 patterns: a peer agent's mid-interactive-
rebase state, left sitting in `.git/rebase-merge/` for hours, can be
resumed by another process and detach HEAD on the peer's branch
mid-tick.

Distinct from Patterns 1-6 (checkout/reset races) because the
contaminating event is paused-then-resumed metadata, not a fresh
checkout. Branch-guard defense (Pattern 5) caught it.

Added:
- Pattern 7 section with full forensics + field-test shard pointers
  (0213Z + 0230Z, both merged via PR #3356)
- Cheap mechanization candidate: `tools/hygiene/check-stale-rebase-state.ts`
  diagnostic that warns on `.git/rebase-merge/` older than 1h
- last_updated frontmatter bumped to 2026-05-15

Substrate-honest stance preserved: do not touch peer's rebase state.
Pivot to dedicated worktrees; branch refs are global, so work lands
on the right ref regardless.

Verified: markdownlint clean.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(shadow-launchd): address 8 of 10 review-cycle-2 findings

Codex + Copilot reviewed the new install-launchagent.ts in detail.
This commit addresses the substantive correctness + style findings;
unit-test coverage is deferred to a separate backlog row.

Addressed (8):

P1 [Codex P2 + Copilot P1] — XML-escape substituted values:
  Paths like `/Users/foo & bar/Zeta` would substitute into <string>
  text nodes producing an invalid plist (& without entity terminator).
  New `xmlEscape` helper applies the five XML 1.0 predefined entities
  (&amp; &lt; &gt; &quot; &apos;) before placeholder substitution.
  Restores the advertised "handles paths with shell metacharacters"
  promise — the bug class was XML escaping, not shell escaping.

P1 — Unknown args silently ignored:
  Switch fell through on typos like `--dryrun` or `--bootstap`, then
  installed with default behavior. Added explicit default: case that
  errors with the offending flag and exits 1 before any FS action.
  Verified: `--dryrun` now produces "Unknown argument: --dryrun.
  Run with --help to see supported flags."

P1 — Relative --bun-path / --repo-root overrides:
  `--repo-root .` would write relative paths into ProgramArguments,
  WorkingDirectory, and log paths — broken under launchd (which has
  no cwd unless explicit). New requireAbsolute() helper rejects
  with a `Use \`<canonicalized>\` if that's what you meant` hint.

P1 — Non-atomic backup-then-write:
  Previous renameSync(destPath, backup) + writeFileSync(destPath)
  had a window where the agent could be removed but the new plist
  not yet present (if writeFileSync threw). Replaced with the
  standard write-temp-then-promote pattern:
    1. writeFileSync(destPath.tmp-PID, content)
    2. renameSync(destPath, backup)  // may not exist; that's OK
    3. renameSync(destPath.tmp-PID, destPath)  // atomic promote
  Failure in step 3 restores backup → destPath (best-effort) so the
  caller's existing install is preserved.

P1 — execFileSync can throw before validation:
  Missing bun, missing git, or running outside a git checkout
  produced a raw exception. New tryDetect() catches and returns
  undefined; parseArgs emits the documented actionable error
  pointing at the --bun-path / --repo-root override.

P1 — Unconditional main() invocation:
  Per Bun-project convention, gate main() with
  `if (import.meta.main)` so tests / helpers can import the module
  without triggering filesystem or launchctl side effects.

P1 — README/plist cross-refs to .claude/rules/shadow-star-shorthand-*:
  These resolve on main (PR #3339 merged). CI's merge-ref check
  confirms; the reviewers' findings were against the raw branch
  base. No additional action needed.

P2 — Direct name attribution in plist KeepAlive comment:
  Replaced "Per Codex review on PR #3342" with the rationale text
  only. Current-state surface; uses role-refs only.

Deferred (1):

P1 — Unit test coverage for install-launchagent.ts:
  Codex + Copilot called out missing tests for placeholder escaping,
  missing placeholders, argument validation, and dry-run output.
  This commit adds the substantive behavior the tests would verify;
  the tests themselves are deferred to a separate backlog row to
  keep this PR's scope manageable. Smoke-tested all four behaviors
  by hand on the cli before commit. Filing as follow-up: tests in
  `tools/shadow/launchd/install-launchagent.test.ts` using Bun's
  built-in test runner.

Verified:
  - bunx tsc --noEmit clean on install-launchagent.ts (pre-existing
    process-extract.ts errors are on this branch base because #3349
    isn't rebased in; CI's merge-ref includes the fix)
  - semgrep --config .semgrep.yml --error: 0 findings
  - --dry-run produces valid plist (plutil -lint OK)
  - --dryrun rejected (Unknown argument)
  - --repo-root . rejected (must be absolute)
  - Special-char paths (e.g., /Users/foo & bar) now produce
    valid escaped XML

Co-Authored-By: Claude <noreply@anthropic.com>

* backlog(p3): B-0528 — unit tests for shadow launchd installer (deferred from PR #3342)

PR #3342 review surfaced a P1 finding: no unit tests cover the new
install-launchagent.ts's safety-critical placeholder substitution,
XML escaping, argument validation, dry-run output, or atomic-promote
pattern. The PR's substantive correctness fixes landed in `970c774`;
this row tracks the deferred test-coverage work.

Scope is bounded (Bun test runner, 6 test categories, follows the
existing tools/shadow/shadow-observer.test.ts pattern). Effort: S.
P3 because the install script's behavior is now correct (smoke-tested
by hand); tests harden against regression rather than fix a current
bug.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(shadow-launchd): availability-preserving install pattern (Codex review)

Codex re-flagged the rename-then-write pattern (in `970c774`) — even
with backup-restore on failure, there is a brief window between
`renameSync(destPath, backup)` and `renameSync(tmpDest, destPath)`
where the canonical path has no plist. If launchd were to load
during that window, it would see no agent.

Replaced with the availability-preserving pattern:
  1. Read existing destPath into memory (skipped on ENOENT)
  2. Write new content to a temp file alongside destPath
  3. Atomically rename(tmp, destPath) — POSIX replaces in one syscall
  4. Side-car the in-memory old content as a timestamped backup

The canonical path is NEVER in a missing state. Backup-write happens
AFTER the install is in place; if it fails, the install is still
good (just no backup). Read-failure handling is similarly defensive:
log + proceed without backup rather than aborting the install.

Verified:
  - bunx tsc --noEmit clean on install-launchagent.ts
  - --dry-run produces valid plist (plutil -lint OK)

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(shadow-launchd): address review-cycle-3 findings (6 of 7)

P1 — Restore SuccessfulExit=false in KeepAlive (reviewer-conflict
resolution):
  Earlier cycle dropped this on Codex review (config errors would
  loop-restart). This cycle Copilot countered: Bun TypeScript runtime
  errors exit non-zero rather than signal-terminate, so the previous
  `Crashed=true` only setting left those failure modes unhandled.
  Restored both keys with an extended plist comment explaining the
  trade-off. ThrottleInterval=60s + --dry-run-by-default already
  cover the "config error restart loop" concern.

P1 — Add sonarjs/no-os-command-from-path suppressions:
  Matches the convention in tools/github/poll-pr-gate.ts +
  tools/peer-call/. Applied at tryDetect, plutilLint, and the three
  launchctl bootstrap/bootout calls. Comments cite the precedent.

P2 — Export helper functions for testability:
  `import.meta.main` guard was added in `970c774` but the functions
  were not exported. Added `export` to main(), parseArgs(),
  requireAbsolute(), tryDetect(), xmlEscape(), substitutePlaceholders(),
  plutilLint(), and the Args interface. Unblocks the B-0528 test file.

P1 — Update 0305Z tick shard's "PREFER" code snippet:
  The earlier shard documented the rename-then-write pattern as
  "PREFER", but that pattern still has a window where destPath is
  empty. Updated to show 3-pattern progression (AVOID copyFileSync
  → ALSO AVOID rename-then-write → PREFER write-temp-then-atomic-
  replace) so the historical trail is accurate and prevents future
  authors from copying the deprecated pattern.

Deferred (1):

P1 — Cross-ref findings on README.md:5 + plist:24 + ts:185:
  README.md:5 and plist:24 reference
  `.claude/rules/shadow-star-shorthand-autocomplete-marker.md` which
  IS on main (#3339 merged). The reviewer's check is against the raw
  branch base; CI's merge-ref check passes. ts:185 is the test
  coverage finding, already filed as B-0528 (deferred per scope
  management).

Verified:
  - bunx tsc --noEmit clean on install-launchagent.ts
  - markdownlint clean on 0305Z.md
  - plutil -lint OK on template + rendered output
  - --dry-run produces valid plist

Co-Authored-By: Claude <noreply@anthropic.com>

* shard(tick): 0329Z — #3342 review-cycle-3 close-out; B-0519 P7 + B-0528 filed

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(shadow-launchd): reconcile docs+shard+backlog with current plist state

Cycle 4 Copilot review caught three documentation surfaces that drifted
from the actual implementation when `SuccessfulExit=false` was restored
in cycle 3:

1. README.md:13 "Restarts on crash (KeepAlive on Crashed only)" — wrong
   description of the bullet. Updated to "Restarts on crash AND on
   non-zero exit (KeepAlive Crashed=true + SuccessfulExit=false) —
   covers signal-termination AND Bun runtime errors". Added the
   ThrottleInterval=60s loop-cap context to the next bullet.

2. 0305Z tick shard row 17 + delta row 79 — described the plist as
   "now crash-only-restart" and showed the After-column as Crashed-only.
   Both rewritten to reflect the cycle-3 reviewer-conflict resolution:
   restored both keys with documented trade-off, net same shape as
   before the PR.

3. B-0528 test-scope item #6 — described atomic-promote with backup-
   restore on rename failure, but current code uses availability-
   preserving pattern (read-old-into-memory → write temp → atomic
   replace → side-car backup). Rewrote the scope with the actual
   failure modes the tests should cover, including the "destPath
   never missing during install" invariant.

Two remaining unresolved threads (cross-refs to
.claude/rules/shadow-star-shorthand-autocomplete-marker.md from
README:5 and plist:24) flag a file that IS on main via #3339. The
reviewer checks against the branch's diff base, which doesn't include
the merged rule. Resolved as outdated on the PR; CI merge-ref check
confirms the reference resolves at merge time.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(shadow-launchd): tighten path validation + harden tmp-file handling

Cycle-4 (post-`59d5c26`) Copilot review surfaced two more correctness
findings — both addressed:

P1 — --bun-path validation accepts dirs / non-executables:
  Previous `existsSync(bunPath)` would pass `/tmp` or `~/Downloads/bun`
  (non-executable copy) and write a LaunchAgent that can't start.
  Replaced with `statSync` + `isFile()` + `accessSync(X_OK)` so the
  installer refuses non-executable / non-regular-file overrides
  pre-write. New error paths:
    - "exists but is not a regular file" (e.g., --bun-path /tmp)
    - "is not executable by the current user" (chmod hint)
  --repo-root similarly gated to require an existing directory.
  Verified both error paths by hand.

P1 — Predictable /tmp filename in plutilLint (symlink attack):
  Previous `/tmp/zeta-shadow-launchagent-${PID}.plist` had a
  predictable name in a world-writable directory. A pre-created
  symlink at that path would cause writeFileSync to follow the link
  and clobber a file the attacker has write access to but not the
  current user. Replaced with `mkdtempSync(tmpdir() + "zeta-shadow-
  launchagent-")` which creates a 0700 directory with a random
  suffix — symlink-resistant, and we clean up both the candidate
  file and the directory in a `finally` block so we don't leak.

Threads remaining after this push (auto-resolution candidates):
  - Test coverage P1 (5th time flagged): already filed as B-0528
    with rationale. Will reply on the thread.
  - Cross-refs to .claude/rules/shadow-star-shorthand-* (5th time
    flagged): file IS on main via #3339. CI merge-ref check confirms.
    Will resolve as outdated.

Verified:
  - bunx tsc --noEmit clean on install-launchagent.ts
  - --dry-run produces valid plist
  - --bun-path /tmp rejected as "not a regular file"
  - --bun-path <non-executable> rejected as "not executable"

Co-Authored-By: Claude <noreply@anthropic.com>

* shard(tick): 0543Z — #3342 review-cycles 4+5 close-out (9 more findings)

Co-Authored-By: Claude <noreply@anthropic.com>

---------

Co-authored-by: Claude <noreply@anthropic.com>
Copilot AI mentioned this pull request May 15, 2026
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@AceHack