fix(spm): serialize concurrent scans to protect synthetic Package.swift (DEVA11Y-483)

[Crash0v3rrid3]

What & why

Fixes DEVA11Y-483 (F-014, CWE-362, Medium) — concurrent spm.sh instances in the same directory race on a shared synthetic Package.swift.

PACKAGE_EXISTS is captured once at startup; the EXIT cleanup trap then removes Package.swift/Package.resolved unconditionally when the flag is non‑zero. Two instances in the same CWD both see no manifest, both write one, and the first to exit deletes it out from under the still‑running peer — which then fails with a confusing "no Package.swift in current directory" from SPM.

Change

Guard the setup/scan/cleanup cycle with an atomic per‑directory mkdir lock (only when we own the synthetic manifest, i.e. PACKAGE_EXISTS != 0):

• mkdir is atomic on POSIX filesystems and portable to macOS, which has no flock(1) (the ticket suggested a CWD mutex — this is the portable form).
• A stale lock left by a peer killed before its trap ran is reclaimed after 5 minutes; otherwise we wait up to 300s then exit with a clear message.
• cleanup removes the manifest first, then releases the lock; only the lock owner releases it.
• Applied identically to the bash, zsh and fish variants.

The scan still runs in the working directory (temp‑dir + --package-path was rejected because it would change the plugin's scan target away from the user's sources).

Testing

• bash -n syntax check passes on all three scripts.

Draft for review.

🤖 Generated with Claude Code

[Crash0v3rrid3]

Addressed all review findings in 4d5ea97 — redesigned the lock rather than patching around it.

#	Finding	Resolution
#1	Staggered-start peer skipped the lock, file still deleted	Lock taken unconditionally; ownership decided under the lock from the live FS (not the startup PACKAGE_EXISTS snapshot); only the creator deletes
#2	5-min mtime staleness steals a live long scan's lock	Reclaim by PID liveness (kill -0), never wall-clock age
#3	Stale reclaim was itself racy	Stale lock claimed atomically via rename; only one waiter wins
#4	Wait-timeout hung then aborted git commit	Timeout is non-fatal — skips the scan (exit 0) with a visible waiting…/skipping notice
#5	Non-EEXIST mkdir failure hung 5 min with wrong error	Fail fast with an actionable message
#6	Lock dir leaked into the working tree	Lock now lives under TMPDIR keyed by the package path — never in the tree
#7	Inconsistent log prefix	Standardized on A11y scan:

Also fixed a latent bug the review surfaced: cleanup state is now global, because the EXIT trap fires after a11y_scan returns (locals out of scope) — the previous cut never actually released the lock on a normal run.

Verified: integration test (staggered concurrent runs serialize; both scans see Package.swift throughout; no tree/TMPDIR residue) + unit tests for stale reclaim, live-owner detection, and fail-fast. Applied identically to bash/zsh/fish.

[Crash0v3rrid3]

Claude Code PR Review

PR: #33 • Head: 7ed6134 • Reviewers: stack:pr-review (aggregated correctness/security/testing/perf/quality)

Summary

Serializes concurrent spm.sh scans in the same directory (DEVA11Y-483) with a per-directory lock: the lock dir lives in $TMPDIR keyed by cksum(PWD) (outside the working tree), a crashed peer's lock is reclaimed by PID-liveness (kill -0) via an atomic mv, and ownership of the synthetic Package.swift is decided under-lock from the live filesystem. Also regenerates the three spm.sh.sha256 sidecars.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	—
High	Security	Authentication/authorization checks present	N/A	—
High	Security	Input validation and sanitization	Pass	Lock path derived from cksum(PWD) in $TMPDIR
High	Security	No IDOR — resource ownership validated	N/A	—
High	Security	No SQL injection (parameterized queries)	N/A	—
High	Correctness	Logic is correct, handles edge cases	Pass	mkdir mutual-exclusion + atomic mv reclaim sound
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	rc 0/1/2 handled; no-lock fallback warns
High	Correctness	No race conditions or concurrency issues	Pass	Cleanup can't delete a peer's file; ownership decided under-lock
Medium	Testing	New code has corresponding tests	Fail	No automated test for the lock (hard to test in shell; noted)
Medium	Testing	Error paths and edge cases tested	Fail	Same
Medium	Testing	Existing tests still pass (no regressions)	Pass	—
Medium	Performance	No N+1 queries or unbounded data fetching	Pass	—
Medium	Performance	Long-running tasks use background jobs	Fail	Dead-owner reclaim branch can busy-spin (see finding)
Medium	Quality	Follows existing codebase patterns	Pass	Matches subcommand-dispatch style
Medium	Quality	Changes are focused (single concern)	Pass	—
Low	Quality	Meaningful names, no dead code	Pass	—
Low	Quality	Comments explain why, not what	Pass	Rationale documented
Low	Quality	No unnecessary dependencies added	Pass	cksum is POSIX

Sidecar integrity verified: recomputed shasum -a 256 of all three spm.sh files — all MATCH their regenerated .sha256 sidecars. Self-update is not broken. Variants byte-identical across bash/zsh/fish.

Findings

• File: scripts/{bash,zsh,fish}/spm.sh:78-86

• Severity: Medium

• Reviewer: stack:pr-review

• Issue: In the dead-owner reclaim branch, if mv "$dir" "$stale" cannot succeed (e.g. a sticky-bit /tmp where another user owns the lock dir), the loop continues with no sleep and never reaches the 300s cap (that cap is only checked on the alive-owner wait path) — an unbounded busy-spin that can peg CPU.

• Suggestion: In the reclaim branch add sleep 1; waited=$((waited+1)) and honor the timeout, or cap reclaim attempts and fall through to return 1/2.

• File: scripts/{bash,zsh,fish}/spm.sh:78

• Severity: Low

• Reviewer: stack:pr-review

• Issue: PID-liveness reclaim is subject to PID reuse — a recycled PID makes kill -0 succeed, so a waiter treats a stale lock as alive and waits the full 300s then skips. Fails safe (skips, never runs concurrently) but costs up to 5 min.

• Suggestion: Optionally stamp the pid file with start-time/comm and compare; or accept as documented.

• File: scripts/{bash,zsh,fish}/spm.sh:120-145

• Severity: Low

• Reviewer: stack:pr-review

• Issue: The EXIT trap is installed only after lock acquire + manifest write, and no INT/TERM trap covers a11y_scan; a SIGINT can leave a synthetic Package.swift behind. Same behavior as pre-PR code (not a regression).

• Suggestion: Install trap cleanup EXIT INT TERM immediately after mkdir succeeds.

---

Verdict: PASS — lock design is correct, sidecars verified matching, no High/Critical. Recommend fixing the Medium reclaim busy-spin before/shortly after merge.

[Crash0v3rrid3]

Claude Code PR Review

PR: #33 • Head: 7ed6134 • Reviewers: fallback general-purpose reviewer + orchestrator checksum verification (no in-repo stack:code-review reviewer installed under the working tree)

Summary

Reworks the concurrent-scan lock in the three spm.sh launchers (bash/zsh/fish): an out-of-tree TMPDIR lock keyed by cksum of $PWD, taken unconditionally, with synthetic-Package.swift ownership decided under the lock, PID-liveness stale reclaim claimed atomically via mv, a non-fatal wait-timeout, fail-fast on non-EEXIST mkdir, and global cleanup state. Also regenerates the three spm.sh.sha256 self-update sidecars to match.

Review Table

Priority	Category	Check	Status	Notes
High	Security	No hardcoded secrets or credentials	Pass	None introduced; lock path derived from $PWD cksum
High	Security	Authentication/authorization checks present	N/A	Shell launcher; no auth surface
High	Security	Input validation and sanitization	N/A	No new external input handled
High	Security	No IDOR — resource ownership validated	N/A	Not applicable
High	Security	No SQL injection (parameterized queries)	N/A	No SQL
High	Correctness	Logic is correct, handles edge cases	Pass	Serializes all timings incl. staggered start; ownership under lock; one Medium gap (pid-less lock) below
High	Correctness	Error handling is explicit, no swallowed exceptions	Pass	Fail-fast on non-EEXIST mkdir; non-fatal timeout is intentional
High	Correctness	No race conditions or concurrency issues	Pass	Atomic mkdir mutex + atomic mv reclaim; global cleanup state fixes the earlier trap-scope leak
Medium	Testing	New code has corresponding tests	Fail	No committed automated test for the lock redesign in this PR; verified manually this session (integration + unit). Non-gating
Medium	Testing	Error paths and edge cases tested	Fail	Same as above — manual only
Medium	Testing	Existing tests still pass (no regressions)	Pass	bash -n clean on all three; no behavioral change to existing paths
Medium	Performance	No N+1 queries or unbounded data fetching	N/A	Not applicable
Medium	Performance	Long-running tasks use background jobs	N/A	Scan is the foreground task by design
Medium	Quality	Follows existing codebase patterns	Pass	Mirrors the existing per-variant duplication + self-update model
Medium	Quality	Changes are focused (single concern)	Pass	Lock redesign + matching checksum regeneration only
Low	Quality	Meaningful names, no dead code	Pass	Removed the now-unused PACKAGE_EXISTS snapshot
Low	Quality	Comments explain why, not what	Pass	Rationale comments on lock/global-scope/reclaim
Low	Quality	No unnecessary dependencies added	Pass	Only POSIX tools (cksum, mkdir, mv, kill -0)

Checksum sidecars verified: recomputed SHA-256 of each committed spm.sh equals its spm.sh.sha256 sidecar (bash 1987749e…, zsh c798dc6b…, fish 87bf749b…, name field spm.sh) — self-update will not spuriously abort.

Findings

• File: scripts/bash/spm.sh:65 (identical in zsh/fish)
• Severity: Medium
• Reviewer: fallback
• Issue: Lock acquisition is two non-atomic steps — mkdir "$dir" then echo "$$" > "$dir/pid", and the EXIT-trap cleanup is not installed until after _spm_acquire_lock returns. If the owner dies in that window (SIGKILL/SIGHUP/power loss) or the pid write fails on a full/read-only TMPDIR (its status is unchecked), the lock dir persists with no pid. The stale-reclaim branch requires a present-and-dead pid ([ -n "$owner_pid" ] && ! kill -0 …), so a pid-less lock is never reclaimed: every subsequent scan of that directory waits the full 300s, then prints "another scan is already running… skipping" and exits 0 — silently no-opping the a11y pre-commit gate until the TMPDIR entry is manually removed or OS-reaped.
• Suggestion: Also treat a missing/empty pid as reclaimable — e.g. reclaim when the pid is present-and-dead or (pid absent/empty and the lock dir is older than a short grace via find "$dir" -mmin +1), using the same atomic mv+rm -rf. This closes the poison-lock case without reintroducing mtime-staleness for normal live scans. Optionally check the echo > pid exit status and release+error on failure.

Additional low/nits (non-gating): PID reuse can make a dead owner look alive (inherent to PID locking; worth a comment); the 300s non-fatal timeout lets a commit skip the scan under a genuinely slow peer (documented design decision); the 32-bit cksum key can theoretically collide for two different dirs (astronomically unlikely); and the pre-existing #!/usr/bin/env bash -il shebang splits poorly on some Linux env (out of scope, not touched here).

---

Verdict: PASS — the redesign is sound and closes the prior review's findings; the one Medium (pid-less poison lock) is a worthwhile follow-up but non-gating.